Skip to content

OAuth2ClientPropertiesRegistrationAdapter shouldn't remove issuer's trailing slash #15324

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
jzheaux opened this issue Nov 28, 2018 · 9 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Nov 28, 2018

OAuth2ClientPropertiesRegistrationAdapter removes the trailing slash on the user-provided spring.security.oauth2.client.provider.providername.issuer-uri property.

This causes a problem when the issuer for the OAuth 2.0 provider actually does have a trailing slash.

For example, Auth0's iss field always has a trailing slash.

Once the trailing slash is removed, then issuer validation fails since it differs from the iss claim in JWTs and in the OIDC Discovery endpoint.

There is at least one example of a user working around this in the wild by adding an extra slash. :)

Is it necessary to remove the trailing slash?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2018
@philwebb
Copy link
Member

Looks like the cleanPath method was added for #13210.

@mbhave
Copy link
Contributor

mbhave commented Dec 3, 2018

I looked at this example provided here. The issuer-uri in the example properties had a trailing slash while the response from the server didn't which is why cleanPath was added.

@jzheaux If we didn't clean the path, the issuer-uri in the properties would need to exactly match the one in the server response. If that's an acceptable outcome, we can avoid removing the trailing slash, although that would be a change in behavior for anyone who has a trailing slash configured in their properties.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Dec 3, 2018
@jzheaux
Copy link
Contributor Author

jzheaux commented Dec 4, 2018

I see, @mbhave.

If we didn't clean the path, the issuer-uri in the properties would need to exactly match the one in the server response

This is the desired behavior.

that would be a change in behavior for anyone who has a trailing slash configured in their properties

Agreed. While not ideal to potentially break users, it should be relatively easy for folks to realize what is happening since the error message is detailed enough:

The Issuer "https://provider.org/" provided in the OpenID Configuration did not match the requested issuer "https://provider.org"

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 4, 2018
@jgrandja
Copy link

jgrandja commented Dec 5, 2018

@jzheaux Can you provide a sample of the issuer from the Provider Configuration Response for at least 4 providers. I'm thinking there may be inconsistencies between providers.

Also, does the spec specify how the issuer should be normalized in the response. There may be more info here.

@jzheaux
Copy link
Contributor Author

jzheaux commented Dec 5, 2018

@jgrandja Auth0 is the only provider that I know of whose issuer ends in a slash. Okta, Google, and UAA all do not have a trailing slash in their issuer value:

Okta: https://instance-name.oktapreview.com/oauth2/default
UAA: https://instance-name/uaa/oauth/token
Auth0: https://instance-name.auth0.com/
Google: https://accounts.google.com

Either way, though, I'd vote that the discrepancies regarding discovery be handled inside ClientRegistrations and JwtDecoders as opposed to in Boot since this property is used more broadly than just for discovery.

@jgrandja
Copy link

jgrandja commented Dec 5, 2018

@jzheaux Agreed, we should handle the trailing slash on our end, whether it's in ClientRegistrations or JwtDecoders or a combination of both. I'd say we remove the cleanIssuerPath on the Boot side.

@mbhave
Copy link
Contributor

mbhave commented Dec 5, 2018

sounds good to me (marked it as a bug rather than an enhancement since auth0 doesn't work without the double slash workaround).

@mbhave mbhave added type: bug A general bug and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Dec 5, 2018
@mbhave mbhave modified the milestones: 2.1.x, 2.1.2 Dec 5, 2018
@mbhave mbhave closed this as completed in 3cc441c Dec 11, 2018
@jzheaux
Copy link
Contributor Author

jzheaux commented Dec 12, 2018

Thank you, @mbhave!

@making
Copy link
Member

making commented Dec 27, 2018

FYI
Azure AD's issuer field has a trailing slash as well and I met the same issue as auth0.

https://sts.windows.net/9d14e56a-b1df-4955-9dc6-0a5833c58a1f/.well-known/openid-configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants