Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial support for generic OIDC properties #4398

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Initial support for generic OIDC properties #4398

merged 2 commits into from
Oct 8, 2019

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Oct 6, 2019

Fixes #4392

@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 6, 2019

@stianst @pedroigor @stuartwdouglas, @pmlopes
Please review, I've removed all the properties which are not used, and attempted to clean up, for Pedro to start incrementally adding properties back once he starts enhancing the adapter.
Renamed resource to clientId per the suggestion at the call

@sberyozkin
Copy link
Member Author

Fixes #4392

@stianst
Copy link
Contributor

stianst commented Oct 7, 2019

Having a single auth-server-url only works if it is used as the base for the discovery endpoint. Otherwise, it would not be able to support other IdPs than Keycloak. I see hardcoded URLs that are Keycloak specific here.

Does Vert.X support discovery endpoint currently?

@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 7, 2019

@stianst thanks; the only hardcoded URL is a default relative introspection path here. I've set it to a Keycloak specific value for the KC users not having to type it when they need it. In other cases the users will point to something else for the introspection to work.

The general approach in the Vertx adapter is not to depend on the discovery endpoint be available (which won't be I guess for some servers - given that, as discussed, having this adapter be able to validate the tokens not necessarily produced by the certified OIDC IDPs would be good) but to let the users point with the relative path properties to various important endpoints, see OAuthClientOptions @pmlopes linked to.

I do like the idea of just having a discovery endpoint URL set and be done :-), I don't think VertX supports it yet, but when it will then we will just enhance the adapter a bit, for example, we can add a discovery relative path property added and say if this is set then all other relative paths are not required/will be ignored and the adapter will deduce itself the addresses, or if no relative addresses are set then assume auth-base-url is a discovery url, etc...

Does it sound reasonable ?

@stianst
Copy link
Contributor

stianst commented Oct 7, 2019

Not sure what the purpose of auth-base-url would be unless it points to the discovery endpoint. I would rather have the options a) specify discovery-uri (with or without .well-known/oidc-configuration), or b) specify individual endpoints.

The introspection endpoint can also be found in the discovery endpoint as introspection_endpoint. So no need to set that separately.

For good usability and portability it's important to support the discovery endpoint in Vert.X.

@stianst
Copy link
Contributor

stianst commented Oct 7, 2019

publicKey is more a backup approach for IdPs that don't support discovery endpoint and don't have a proper JWK endpoint. For Keycloak keys should be loaded from jwks_uri from discovery endpoint and not specified directly in the config.

@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 7, 2019

@stianst The purpose is not to require users having to enter something like http://host/idp N times when they need to set the individual endpoints:

introspection=http://host/idp/introspection
jwks=http://host/idp/jwks
etc

instead the users of Keycloak/Okta/etc just do:

auth-base-url=http://host/idp/
introspection-path=introspection
jwks-path=jwks

Once VertX supports the doscovery endpoint we will just say:

auth-base-url=address with a well-known discovery URL suffix
#and if really needed, say if URL path after the host/port is not a recommended OIDC value then add a boolean property such as 'discovery'=true to give the adapter a hint 

or

auth-base-url=http://host/idp/
discovery=./well-known/...

and that is it.

So I don't see any contradiction between what we have now and what will be possible later on once the discovery endpoint is supported hopefully for 1.0.

@sberyozkin
Copy link
Member Author

@stianst re the public key - sure, this property was there (with a 'realm' prefix which I dropped) and having it available for some users would be useful I guess, but of course, as you recommend, we;d encourage the users to use JWK keys and enable the key rotation

@stianst
Copy link
Contributor

stianst commented Oct 7, 2019

@stianst The purpose is not to require users having to enter something like http://host/idp N times when they need to set the individual endpoints:

introspection=http://host/idp/introspection
jwks=http://host/idp/jwks
etc

instead the users of Keycloak/Okta/etc just do:

auth-base-url=http://host/idp/
introspection-path=introspection
jwks-path=jwks

Once VertX supports the doscovery endpoint we will just say:

auth-base-url=address with a well-known discovery URL suffix
#and if really needed, say if URL path after the host/port is not a recommended OIDC value then add a boolean property such as 'discovery'=true to give the adapter a hint 

or

auth-base-url=http://host/idp/
discovery=./well-known/...

and that is it.

So I don't see any contradiction between what we have now and what will be possible later on once the discovery endpoint is supported hopefully for 1.0.

Thanks for the clarification. That makes a lot more sense now.

@sberyozkin
Copy link
Member Author

@stianst thanks Stian, we will definitely encourage the users to go the discovery endpoint way :-) once it is supported. I'll open an issue to track it and will CC you and Pedro

@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 7, 2019

@stuartwdouglas, @pmlopes, @pedroigor FYI, I've removed the code related to the public client and the use of the authorization header because it was not really clear it was required by the adapter itself. Stuart, should be good to go otherwise if we want 0.24.0 Keycloak users starting a new journey without an explicit realm property :-).
I'll need to follow up with some more doc cleanups but they are minor issues

@sberyozkin sberyozkin added this to the 0.24.0 milestone Oct 7, 2019
@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 7, 2019

@stuartwdouglas I've updated the oauth2 token type to bearer; holder-of-key/pop token types might be supported later, but oauth2 is too wide :-), it can be issued by OIDC... Hi Pedro, @pedroigor, FYI

@stuartwdouglas stuartwdouglas merged commit 3f566a1 into quarkusio:master Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make quarkus-oidc properties more generic
3 participants