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

use relative OIDCRedirectURI where applicable #3448

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

johrstrom
Copy link
Contributor

Use relative OIDCRedirectURI where applicable, i.e., when not using a proxy server.

This is for partial support for #3442 - at least the OIDC bit.

Comment on lines 84 to 87
OIDCProviderMetadataURL https://example-proxy.com/dex/.well-known/openid-configuration
OIDCClientID example.com
OIDCClientSecret 83bc78b7-6f5e-4010-9d80-22f328aa6550
OIDCRedirectURI https://example.com/oidc
OIDCRedirectURI https://example-proxy.com/oidc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may have been a bug unrelated to relative or absolute URIs.

You see here that OIDCProviderMetadataURL is the proxy_server, but OIDCRedirectURI was the servername. I assume we'd want the OIDCRedirectURI to be the proxy, not the origin server. could it be that nobody runs dex behind a proxy and so nobody has encountered this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow, why not use relative for proxy too? I think the address seen by IDP like Keycloak or Dex would be the address that a user was using to access the OnDemand instance and that is configured to redirect back to after authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow, why not use relative for proxy too? I think the address seen by IDP like Keycloak or Dex would be the address that a user was using to access the OnDemand instance and that is configured to redirect back to after authentication.

I guess my thinking was that apache may not know or use the proxy name. It seems to me that apache would use HTTP_HOST (the apache servername) not HTTP_FORWARDED_FOR (the proxy servername) (or similar IDK if that's the real header name).

I'll re-read the documentation to see if it's smart enough to generate the proxy name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like you're right - their oidc_get_current_url_host helper function can toggle off of forwarded for headers.

https://github.com/OpenIDC/mod_auth_openidc/blob/9ac1e205962dff01a557425c03840712722c82a9/src/util.c#L704-L712

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the PR because it does catch this edge case for proxies, so I can simplify it.

@johrstrom johrstrom requested a review from treydock March 22, 2024 17:37
@johrstrom johrstrom closed this Mar 25, 2024
@johrstrom johrstrom reopened this Mar 25, 2024
@johrstrom johrstrom merged commit 546d06b into master Apr 9, 2024
20 checks passed
@johrstrom johrstrom deleted the relative-oidc-redirect-uri branch April 9, 2024 18:10
johrstrom added a commit that referenced this pull request May 21, 2024
Use relative OIDCRedirectURI where applicable to support multiple servernames.
@johrstrom johrstrom mentioned this pull request May 21, 2024
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.

4 participants