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

[ELY-2242] Changed OidcRequestAuthenticator.rewrittenRedirectUri to … #1615

Merged
merged 1 commit into from Nov 8, 2021

Conversation

mouseas
Copy link

@mouseas mouseas commented Nov 1, 2021

[ELY-2242] Changed OidcRequestAuthenticator.rewrittenRedirectUri to behave consistently when a redirect rewrite rule is specified vs when none is. Exposed redirect rewrite rules when OidcClientConfiguration is delegated by OidcClientContext.

https://issues.redhat.com/browse/ELY-2242

…ehave consistently when a redirect rewrite rule is specified vs when none is. Exposed redirect rewrite rules when OidcClientConfiguration is delegated by OidcClientContext.
@wildfly-ci
Copy link

Hello, mouseas. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@mouseas
Copy link
Author

mouseas commented Nov 2, 2021

Found a bit more about this in the spec for OAuth 2.0:

Original OAuth 2.0 spec: https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.2

Update for threat model and security considerations: https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3

The original spec allowed for query strings to be dynamic in redirect urls, but the update no longer allows them because they create potential security vulnerabilities. From this I would conclude that the query string is meant to be removed entirely. If that's the case, it makes OidcRequestAuthenticator.stripOauthParametersFromRedirect() redundant. It may also be appropriate to allow the redirect_url to be defined in the config, so Elytron doesn't need to derive it from the url which the user arrives at after authenticating.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks @mouseas!

@fjuma
Copy link
Contributor

fjuma commented Nov 5, 2021

Found a bit more about this in the spec for OAuth 2.0:

Original OAuth 2.0 spec: https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.2

Update for threat model and security considerations: https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3

The original spec allowed for query strings to be dynamic in redirect urls, but the update no longer allows them because they create potential security vulnerabilities. From this I would conclude that the query string is meant to be removed entirely. If that's the case, it makes OidcRequestAuthenticator.stripOauthParametersFromRedirect() redundant. It may also be appropriate to allow the redirect_url to be defined in the config, so Elytron doesn't need to derive it from the url which the user arrives at after authenticating.

Feel free to create an ELY issue for this and we'll take a closer look. Thanks!

@mouseas
Copy link
Author

mouseas commented Nov 5, 2021

Feel free to create an ELY issue for this and we'll take a closer look. Thanks!

What issue type?

@fjuma
Copy link
Contributor

fjuma commented Nov 5, 2021

Feel free to create an ELY issue for this and we'll take a closer look. Thanks!

What issue type?

Feel free to use Task and then we'll investigate further. Thanks.

@Skyllarr
Copy link
Contributor

Skyllarr commented Nov 8, 2021

@mouseas Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants