Add support for known providers to OIDC #22176

stuartwdouglas · 2021-12-14T01:08:06Z

This adds specifc config to enable OIDC login for the main OIDC
providers. Having explicit config options like this makes it easy to
search the documentation/dev UI for it.

Fixes #20783

This adds specifc config to enable OIDC login for the main OIDC providers. Having explicit config options like this makes it easy to search the documentation/dev UI for it. Fixes quarkusio#20783

FroMage · 2021-12-14T09:44:10Z

...oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/KnownOIDCProvidersRecorder.java

+                ret.credentials.secret = Optional.of(config.provider.microsoft.get().secret);
+                ret.authServerUrl = Optional.of("https://login.microsoftonline.com/common/v2.0");
+                //TODO: do we want to hard code this?
+                ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;


Yeah, not sure, I'd have to test it on my app to see if it works.

FroMage · 2021-12-14T09:48:49Z

...oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/KnownOIDCProvidersRecorder.java

+                ret.authentication.scopes = Optional.of(config.provider.github.get().scopes);
+                ret.authentication.userInfoRequired = true;
+                ret.authentication.setIdTokenRequired(false);
+                ret.authentication.setRedirectPath("/Login/githubLoginSuccess");


I think those links are too specific to my app. But in practice:

google/apple/ms works on a single endpoint without requiring UserInfo: everything is in the ID Token

github must be handled specially because UserInfo contains the user ID, name and user name, but we have to hit a second endpoint /user/emails to get the user's email.

facebook must be handled specially because UserInfo contains the user ID, name and email

Perhaps a better endpoint default would be: /{provider}-oidc-login-success?

Naturally, a further layer of abstraction which knows how to fetch the damn fields from all providers and present them to the Quarkus user would be even better: then I only need a single success endpoint :)
But that can wait for another issue.

sberyozkin · 2021-12-14T11:50:24Z

Hi @stuartwdouglas Thanks for taking on this task. I'd like to clarify first, how will the configuration look like ?

For example, a user should be able to override any property in the github provider block.
I've been thinking earlier to have something like this:

#defaul tenant, Github
quarkus.oidc.provider=github
#override the scope
quarkus.oidc.authentication.scope=user:something

#facebook tenant

quarkus.oidc.facebook.provider=facebook
#override the scope
quarkus.oidc.facebook.authentication.scope=user:something

I think for this to work OidcTenantConfig should have a copy constructor the outer OidcTenantConfig is initialized from the github or facebook

sberyozkin · 2021-12-14T11:56:29Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcConfig.java

+     * Config for well known OIDC providers
+     */
+    @ConfigItem
+    public ProvidersConfig provider;


I'm not sure it has to be decoupled from OidcTenantConfig, it has to be part of OidcTenantConfig, tenant specific as needed, or rather, IMHO, something like quarkus.oidc.provider=github should point to an instance of OidcTenantConfig, merged with the OidcTenantConfig referencing it, please see below

Also, just realized, this way won't work with the dynamic TenantConfigResolver - one should be able to do, instead of defining this config statically in application.properties,

public class MyTenantConfigResolver implements TenantConfigResolver { public Uni<OidcTenantConfig> resolve(RoutingContext rc) { OidcTenantConfig github = getDefaultOidcTenantGithubBlock(); // overrride: github.getAuthentication().setScope("user:something"); return github; } } }

@stuartwdouglas Stuart, how about having an OidcTenantConfig provider enum property with (github, etc, values).

This will allow OidcRecorder, when going over every OidcTenantConfig, pick up a referenced well-known provider OidcTenantConfig instance and merge with the current OidcTenantConfig - it will address all the concerns I've expressed in various comments here

sberyozkin · 2021-12-14T12:16:46Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/OidcRecorder.java

+        for (var e : knownProviders.entrySet()) {
+            var provider = e.getValue().get();
+            if (provider.isPresent()) {
+                tenants.put(e.getKey(), provider.get());


Sorry, but it is not correct - it creates a dedicated tenant per every social provider config, but it won't allow to override a specific property using OidcTenantConfig - with this approach we'd end up moving a good portion of OidcTenantConfig properties under the github and other provider specific blocks. I'd say it is a problem.
I understand that GitHub and other config groups you have added are meant to customize - but it creates a parallel stream for configuring things.

But also equally important, it does not allow to vary with the provider, each OIDC tenant allows to have:

quarkus.oidc.github1.provider=github quarkus.oidc.github1.authentication.scopes=a quarkus.oidc.github2.provider=github quarkus.oidc.github2.authentication.scopes=b

It may not be something one would want to do in production but for tests it is very useful.

Yeah, so continuing from the earlier comment, so you have Map<String, Supplier<Optional<OidcTenantConfig>>> knownProviders.
But instead of creating tenants as you have typed here, the following will be more aligned with the OIDC tenancy:

if (oidcTenantConfig.provider.isPresent()) { // Merge - this ensures the github/etc properties are overridable OidcTenantConfig thisIsCurrentTenantConfig = new OidcTenantConfig(knownProviders.get(oidcTenantConfig.provider.get())); // continue dealing with this tenant config }

sberyozkin · 2021-12-14T12:31:25Z

extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/GitHub.java

+import io.quarkus.runtime.annotations.ConfigItem;
+
+@ConfigGroup
+public class GitHub {


IMHO, GitHub (and other providers) groups should extend OidcTenantConfig and override properties with GitHub/etc specific properties

client-id, client-secret, scopes - these are all available in OidcTenantConfig, this is my earlier concern, it is not worth IMHO to start mirroring some of those properties here. By extending OidcTenantConfig they can be set as needed.

I now think that these groups have to be removed, IMHO, the supplied OidcTenantConfig per every provider should be enough

sberyozkin · 2021-12-14T12:33:40Z

...oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/KnownOIDCProvidersRecorder.java

+                ret.credentials.secret = Optional.of(config.provider.github.get().secret);
+                ret.authServerUrl = Optional.of("https://github.com/login/oauth");
+                //TODO: do we want to hard code this?
+                ret.applicationType = OidcTenantConfig.ApplicationType.HYBRID;


HYBRID won't work for GitHub - quarkus-oidc service apps can not handle opaque tokens from providers like GitHub since it has no introspection endpoint - I've opened an issue to make sure a userinfo check is enough

sberyozkin · 2021-12-14T12:34:21Z

...oidc/runtime/src/main/java/io/quarkus/oidc/runtime/providers/KnownOIDCProvidersRecorder.java

+                ret.authentication.scopes = Optional.of(config.provider.github.get().scopes);
+                ret.authentication.userInfoRequired = true;
+                ret.authentication.setIdTokenRequired(false);
+                ret.authentication.setRedirectPath("/Login/githubLoginSuccess");


Yes, it has to be an application specific property

pedroigor

I also agree with @sberyozkin that we could rely on the properties already available from OidcTenantConfig. But looks like there are some improvements on documentation and dev UI.

I would also add a hook for each provider so that the authorization/authentication response could be processed accordingly to the particularities of providers. For instance, github might require calling additional endpoints to fetch info about the subject, etc. Perhaps it could also avoid simplifying the code a bit by not introducing provider-specific behavior into the main code path. Another example is Apple ID, which requires setting up a JWT to authenticate.

sberyozkin · 2021-12-14T13:26:47Z

Hi @pedroigor

Another example is Apple ID, which requires setting up a JWT to authenticate.

@FroMage is on it, the existing JWT gen code will be reused - but will be sent as a client post secret value.

Providing plugin points for provider specific behaviors is a nice idea, I propose though to deal with it separately as this PR is complex in its own way :-)

sberyozkin · 2021-12-24T13:24:22Z

Hi Stuart, I did a quick prototype based on my comments, https://github.com/sberyozkin/quarkus/pull/new/well_known_providers.

It will allow the following variations:

Default tenant:

quarkus.oidc.provider=github
quarkus.oidc.client-id=myclientid
//etc

Single custom tenant which may read better

quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=myclientid
//etc

More than one tenant

quarkus.oidc.github.provider=github
quarkus.oidc.github.client-id=myclientid


quarkus.oidc.apple.provider=apple
quarkus.oidc.apple.client-id=myclientid
//etc

Note especially in the last case - the client authentication requirements may differ (with Apple it would be a fairly complex JWT auth setup) which is what I implied that we'd need to mirror OidcTenantConfig in ProviderConfig in the current PR.

It works in application.properties and dynamically with TenantConfigResolver.

Hope it looks reasonable, I'll try to complete it soon enough, the only real complexity is copying OidcTenantConfig :-)

gsmet · 2022-01-10T19:34:44Z

This one has been superseded by #22572.

Add support for known providers to OIDC

aa51984

This adds specifc config to enable OIDC login for the main OIDC providers. Having explicit config options like this makes it easy to search the documentation/dev UI for it. Fixes quarkusio#20783

stuartwdouglas requested review from FroMage, sberyozkin and pedroigor December 14, 2021 01:08

quarkus-bot bot added the area/oidc label Dec 14, 2021

FroMage requested changes Dec 14, 2021

View reviewed changes

sberyozkin reviewed Dec 14, 2021

View reviewed changes

pedroigor reviewed Dec 14, 2021

View reviewed changes

sberyozkin mentioned this pull request Jan 1, 2022

Add support for well-known OIDC providers #22572

Merged

gsmet closed this Jan 10, 2022

quarkus-bot bot added the triage/invalid This doesn't seem right label Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for known providers to OIDC #22176

Add support for known providers to OIDC #22176

stuartwdouglas commented Dec 14, 2021

FroMage Dec 14, 2021

FroMage Dec 14, 2021

FroMage Dec 14, 2021

sberyozkin commented Dec 14, 2021

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021 •

edited

Loading

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021 •

edited

Loading

sberyozkin Dec 14, 2021 •

edited

Loading

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021

sberyozkin Dec 14, 2021

pedroigor left a comment •

edited

Loading

sberyozkin commented Dec 14, 2021 •

edited

Loading

sberyozkin commented Dec 24, 2021 •

edited

Loading

gsmet commented Jan 10, 2022

Add support for known providers to OIDC #22176

Add support for known providers to OIDC #22176

Conversation

stuartwdouglas commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sberyozkin commented Dec 14, 2021

Choose a reason for hiding this comment

sberyozkin Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sberyozkin Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

sberyozkin Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroigor left a comment • edited Loading

Choose a reason for hiding this comment

sberyozkin commented Dec 14, 2021 • edited Loading

sberyozkin commented Dec 24, 2021 • edited Loading

gsmet commented Jan 10, 2022

sberyozkin Dec 14, 2021 •

edited

Loading

sberyozkin Dec 14, 2021 •

edited

Loading

sberyozkin Dec 14, 2021 •

edited

Loading

pedroigor left a comment •

edited

Loading

sberyozkin commented Dec 14, 2021 •

edited

Loading

sberyozkin commented Dec 24, 2021 •

edited

Loading