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.

Sign up for GitHub

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
Merged

use relative OIDCRedirectURI where applicable #3448

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b419f1
use relative OIDCRedirectURI where applicable
johrstrom Mar 22, 2024
1614b33
simplify this by just using oidc_uri
johrstrom Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion ood-portal-generator/lib/ood_portal_generator/dex.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,14 @@ def client_url
"#{client_protocol}#{client_id}#{client_port}"
end

def apache_redirect_uri
if @view.proxy_server.to_s != @view.servername.to_s
"#{client_protocol}#{@view.proxy_server}/oidc"
else
'/oidc'
end
end

def client_redirect_uri
"#{client_url}/oidc"
end
Expand Down Expand Up @@ -297,7 +305,7 @@ def oidc_attributes
attrs = {
dex_http_port: http_port,
oidc_uri: '/oidc',
oidc_redirect_uri: client_redirect_uri,
oidc_redirect_uri: apache_redirect_uri,
oidc_provider_metadata_url: "#{issuer}/.well-known/openid-configuration",
oidc_client_id: client_id,
oidc_client_secret: client_secret
Expand Down
6 changes: 5 additions & 1 deletion ood-portal-generator/lib/ood_portal_generator/view.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,11 @@ def initialize(opts = {})
@oidc_provider_metadata_url = opts.fetch(:oidc_provider_metadata_url, nil)
@oidc_client_id = opts.fetch(:oidc_client_id, nil)
@oidc_client_secret = opts.fetch(:oidc_client_secret, nil)
@oidc_redirect_uri = "#{protocol}#{servername}#{@oidc_uri}"
@oidc_redirect_uri = if opts.key?(:proxy_server)
"#{protocol}#{@proxy_server}#{@oidc_uri}"
else
@oidc_uri.to_s
end
@oidc_remote_user_claim = opts.fetch(:oidc_remote_user_claim, 'preferred_username')
@oidc_scope = opts.fetch(:oidc_scope, "openid profile email")
@oidc_crypto_passphrase = opts.fetch(:oidc_crypto_passphrase, Digest::SHA1.hexdigest(servername))
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.dex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
OIDCProviderMetadataURL http://example.com/dex/.well-known/openid-configuration
OIDCClientID example.com
OIDCClientSecret 83bc78b7-6f5e-4010-9d80-22f328aa6550
OIDCRedirectURI http://example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim email
OIDCScope "openid profile email"
OIDCCryptoPassphrase 0caaf24ab1a0c33440c06afe99df986365b0781f
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.dex-full
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
OIDCProviderMetadataURL https://example.com/dex/.well-known/openid-configuration
OIDCClientID example.com
OIDCClientSecret 83bc78b7-6f5e-4010-9d80-22f328aa6550
OIDCRedirectURI https://example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim email
OIDCScope "openid profile email"
OIDCCryptoPassphrase 0caaf24ab1a0c33440c06afe99df986365b0781f
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.dex-ldap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
OIDCProviderMetadataURL https://example.com/dex/.well-known/openid-configuration
OIDCClientID example.com
OIDCClientSecret 83bc78b7-6f5e-4010-9d80-22f328aa6550
OIDCRedirectURI https://example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim preferred_username
OIDCScope "openid profile email"
OIDCCryptoPassphrase 0caaf24ab1a0c33440c06afe99df986365b0781f
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.dex-no-proxy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
OIDCProviderMetadataURL https://example.com:5554/.well-known/openid-configuration
OIDCClientID example.com
OIDCClientSecret 83bc78b7-6f5e-4010-9d80-22f328aa6550
OIDCRedirectURI https://example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim email
OIDCScope "openid profile email"
OIDCCryptoPassphrase 0caaf24ab1a0c33440c06afe99df986365b0781f
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.oidc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
OIDCProviderMetadataURL https://idp.example.com/auth/realms/osc/.well-known/openid-configuration
OIDCClientID ondemand.example.com
OIDCClientSecret secret
OIDCRedirectURI http://ondemand.example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim preferred_username
OIDCScope "openid profile email groups"
OIDCCryptoPassphrase e2c5ee12c92a019f19b5e532641ac0da2f9acdac
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.conf.oidc-ssl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
OIDCProviderMetadataURL https://idp.example.com/auth/realms/osc/.well-known/openid-configuration
OIDCClientID ondemand.example.com
OIDCClientSecret secret
OIDCRedirectURI https://ondemand.example.com/oidc
OIDCRedirectURI /oidc
OIDCRemoteUserClaim preferred_username
OIDCScope "openid profile email groups"
OIDCCryptoPassphrase e2c5ee12c92a019f19b5e532641ac0da2f9acdac
Expand Down
2 changes: 1 addition & 1 deletion ood-portal-generator/spec/fixtures/ood-portal.dex-full.proxy.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
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.

OIDCRemoteUserClaim email
OIDCScope "openid profile email"
OIDCCryptoPassphrase 0caaf24ab1a0c33440c06afe99df986365b0781f
Expand Down
Loading