Skip to content

Implement OpenID Provider Configuration endpoint #143

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

Closed

Conversation

Kehrlann
Copy link
Contributor

@Kehrlann Kehrlann commented Nov 3, 2020

This will deliver #55

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 3, 2020
@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch from 00bec01 to 2dccbf7 Compare November 3, 2020 16:13
@jgrandja jgrandja self-assigned this Nov 3, 2020
@jgrandja jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 3, 2020
@jgrandja jgrandja added this to the 0.1.0 milestone Nov 3, 2020
@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch 2 times, most recently from 08f3104 to 5389abe Compare November 5, 2020 11:22
@Kehrlann Kehrlann marked this pull request as ready for review November 5, 2020 13:09
@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch 15 times, most recently from 8c93524 to cd31229 Compare November 10, 2020 16:00
@Kehrlann
Copy link
Contributor Author

Kehrlann commented Nov 10, 2020

I have introduced org.springframework.security.oauth2.core.ClientAuthenticationMethod2 so that the names match xxx_auth_methods_supported in OpenID Discovery 1.0. It does not derive from org.springframework.security.oauth2.core.ClientAuthenticationMethod in spring-security because that class is final...

I have introduced it in a separate commit but can squash it if you prefer.

@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch 3 times, most recently from 8fa9d0f to ff869f6 Compare November 12, 2020 10:47
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Kehrlann. Please see review comments.

@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch 2 times, most recently from 22e503e to 64207e9 Compare November 16, 2020 16:49
@joshuatcasey
Copy link
Contributor

joshuatcasey commented Nov 16, 2020

Hi from the Application SSO team. We had a team goal to implement a customizable Issuer URL - thanks for beating us to it!

We noticed a few things with 64207e9 that we were curious about.

  1. It appears that issuer is not a required input, and if it's not configured, then endpoint /.well-known/openid-configuration will not be exposed. I feel that issuer URL should be required - perhaps throw an exception? The application layer can choose how to render that error to the user / configurer.
  2. It doesn't look like the configured issuer flows into the token claim iss (at least for access tokens). Is this on the roadmap?
  3. As per the specification language, issuer must use the https scheme with no query or fragment components. I didn't see validation around this part of the specification. What are your thoughts on implementing this directly in the library? I can see a possibility where either http or https is allowed (for local testing/debugging, and other scenarios that don't require TLS everywhere).

Appendices

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

issuer
REQUIRED. URL using the https scheme with no query or fragment component that the OP asserts as its Issuer Identifier. If Issuer discovery is supported (see Section 2), this value MUST be identical to the issuer value returned by WebFinger. This also MUST be identical to the iss Claim value in ID Tokens issued from this Issuer.

https://tools.ietf.org/html/rfc8414#section-2

issuer
REQUIRED. The authorization server's issuer identifier, which is
a URL that uses the "https" scheme and has no query or fragment
components. Authorization server metadata is published at a
location that is ".well-known" according to RFC 5785 [RFC5785]
derived from this issuer identifier, as described in Section 3.
The issuer identifier is used to prevent authorization server mix-
up attacks, as described in "OAuth 2.0 Mix-Up Mitigation"
[MIX-UP].

@Kehrlann
Copy link
Contributor Author

Hi @joshuatcasey ! Glad to help. To your points, here are some of the discussions we had around these topics:

  1. issuer not required: The main idea for keeping the issuer optional (for now) is to stand up an authz server with absolute minimal configuration, even if it doesn't implement all possible extensions. I'll let @jgrandja chime in on this.
  2. using the issuer in tokens: Yes the iss will be populated from the ProviderSettings, it is on the roadmap, not in the scope of this PR though.
  3. issuer validation: That's correct, for local development https is a bit of a pain. We talked about pushing the validation of the URL to the users of the lib instead.

@Kehrlann Kehrlann requested a review from jgrandja November 17, 2020 09:28
@joshuatcasey
Copy link
Contributor

Thanks for the clarifications.

  1. Understood. It will likely be required for app SSO, but I understand if that's not part of the library.
  2. Can you link this on the roadmap? I see that Make it possible to customize the issuer claim and the access token expiration time  #148 was closed in favor of this PR. Happy to submit a PR for this soon.
  3. Yeah, we had the same thought about local development or deployments where "TLS everywhere" isn't enforced. We'll probably add a flag to allow http as well as https and make it very obvious to the configurer that this is unsafe / outside of specification compliance.

@Kehrlann
Copy link
Contributor Author

Kehrlann commented Nov 17, 2020

I think @alek-sys will be making sure the iss claim is correctly populated, probably based off of #145 (that PR brings some changes to the API as well)

@alek-sys
Copy link
Contributor

alek-sys commented Nov 17, 2020

I think @alek-sys will be making sure the iss claim is correctly populated, probably based off of #145 (that PR brings some changes to the API as well)

#145 will abstract JWT token creation behind TokenIssuer interface, but currently the default implementation of JWT access token issuer doesn't have any validations, not even for iss claim. There used to be a mechanism to validate issued token, but I couldn't find any use of it so after chatting with Joe decided to delete it. Maybe iss token schema validation is a reason to bring it back.

@joshuatcasey
Copy link
Contributor

Speaking as an application developer, I want to validate the user's configuration on application startup, so it makes sense that we would validate issuer URL at that time. For now, we'll have some custom logic to perform this validation and we can discuss pushing it upstream to this library later.

Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @Kehrlann. Please see review comments.

@jgrandja
Copy link
Collaborator

@joshuatcasey

I want to validate the user's configuration on application startup

Agreed. We will validate the application supplied configuration at startup and fail-fast on errors. I'm not sure where this will go at the moment but it very well might go in OAuth2AuthorizationServerConfigurer

@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch from 64207e9 to 6d5350d Compare November 19, 2020 11:06
- See https://openid.net/specs/openid-connect-discovery-1_0.html
  sections 3 and 4.
- We introduce here a "ProviderSettings" construct to configure
  the authorization server, starting with endpoint paths (e.g.
  token endpoint, jwk set endpont, ...)
@Kehrlann Kehrlann force-pushed the oidc-provider-metadata branch from dbb743c to 4a2ee31 Compare November 20, 2020 13:18
@Kehrlann
Copy link
Contributor Author

Kehrlann commented Nov 20, 2020

Fixed suggestions and comments.

Could you please take a look at OAuth2AuthorizationServerConfigurer#validateProviderSettings for validating the issuer URI ?
Also added ObjectToSetStringConverter2 here until we add it to Spring Security ; could you please take a look at it before integrating it ? I used ObjectToListStringConverter as an inspiration, but I only added temporary tests for ObjectToSetStringConverter2#convert, I am a bit unsure about #getConvertibleTypes and #matches.

jgrandja added a commit that referenced this pull request Nov 25, 2020
@jgrandja
Copy link
Collaborator

Great work @Kehrlann ! This is now in master!

@jgrandja jgrandja closed this Nov 25, 2020
@Kehrlann Kehrlann deleted the oidc-provider-metadata branch November 25, 2020 13:20
jgrandja added a commit that referenced this pull request Nov 30, 2020
jgrandja added a commit that referenced this pull request Nov 30, 2020
jgrandja added a commit that referenced this pull request Dec 11, 2020
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants