-
Notifications
You must be signed in to change notification settings - Fork 204
Handle discovery based lookup when using http and nested realms (keycloak style) #211
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
79b702c
e6e550b
847aad2
20857a0
a7f7458
6b86d25
94e75f0
c6d14b0
361385b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,11 +110,13 @@ def client | |
| end | ||
|
|
||
| def config | ||
| @config ||= ::OpenIDConnect::Discovery::Provider::Config.discover!(options.issuer) | ||
| configure_discovery_scheme | ||
| @config ||= ::OpenIDConnect::Discovery::Provider::Config.discover!(discovery_base_url) | ||
| end | ||
|
|
||
| def request_phase | ||
| options.issuer = issuer if options.issuer.to_s.empty? | ||
| def request_phase | ||
| options.issuer = issuer if issuer_empty? | ||
|
|
||
| discover! | ||
| redirect authorize_uri | ||
| end | ||
|
|
@@ -134,7 +136,7 @@ def callback_phase | |
|
|
||
| return unless valid_response_type? | ||
|
|
||
| options.issuer = issuer if options.issuer.nil? || options.issuer.empty? | ||
| options.issuer = issuer if issuer_empty? | ||
|
|
||
| verify_id_token!(params['id_token']) if configured_response_type == 'id_token' | ||
| discover! | ||
|
|
@@ -157,7 +159,7 @@ def callback_phase | |
|
|
||
| def other_phase | ||
| if logout_path_pattern.match?(current_path) | ||
| options.issuer = issuer if options.issuer.to_s.empty? | ||
| options.issuer = issuer if issuer_empty? | ||
| discover! | ||
| return redirect(end_session_uri) if end_session_uri | ||
| end | ||
|
|
@@ -250,6 +252,72 @@ def issuer | |
| ::OpenIDConnect::Discovery::Provider.discover!(resource).issuer | ||
| end | ||
|
|
||
| # Helper method to safely check if issuer is empty, handling Proc/lambda values | ||
| def issuer_empty? | ||
| return true if options.issuer.nil? | ||
|
|
||
| issuer_value = options.issuer | ||
| issuer_value = issuer_value.call if issuer_value.respond_to?(:call) | ||
| issuer_value.to_s.empty? | ||
| end | ||
|
|
||
| # When discovery is enabled we need a *stable* base URL. | ||
| # | ||
| # If the user provides an issuer without an explicit scheme (e.g. "keycloak:8080/realms/foo"), | ||
| # passing it to discovery can lead to unintended HTTPS/TLS attempts depending on underlying client defaults. | ||
| # | ||
| # Recommended behavior: only trust options.issuer for discovery if it is an absolute URI with http/https scheme. | ||
| # Otherwise, fall back to client_options.scheme/host/port. | ||
| def discovery_base_url | ||
| # Handle Proc/lambda for options.issuer (common pattern for runtime evaluation) | ||
| issuer_value = options.issuer | ||
| issuer_value = issuer_value.call if issuer_value.respond_to?(:call) | ||
| issuer = issuer_value.to_s | ||
|
|
||
| if issuer.match?(/\Ahttps?:\/\//) | ||
| # Use the full issuer URL as-is for discovery | ||
| # The discovery endpoint will be: issuer + '/.well-known/openid-configuration' | ||
| if defined?(Rails) && Rails.logger | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this stale debugging code? This checks for |
||
| puts "[OpenIDConnect] discovery_base_url - using issuer as-is: #{issuer}" | ||
| end | ||
| issuer | ||
| else | ||
| # Issuer doesn't have a scheme, construct URL from client_options | ||
| resource = "#{client_options.scheme}://#{client_options.host}" | ||
| resource = "#{resource}:#{client_options.port}" if client_options.port | ||
| resource | ||
| end | ||
| end | ||
|
|
||
| # Configure the SWD (Simple Web Discovery) library to use the correct URI scheme. | ||
| # | ||
| # The SWD library (used by openid_connect gem for discovery) defaults to URI::HTTPS, | ||
| # which causes SSL connection attempts even when an HTTP URL is explicitly provided. | ||
| # This results in "SSL_connect" errors when connecting to non-SSL servers (e.g., | ||
| # development Keycloak instances running on HTTP). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought there was a reason why HTTPS was enforced. Is this only done for supporting development? I'm a bit concerned this could inadvertently open security holes.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, in my use case, I am running Keycloak on HTTP, and I'm using it for testing of the OpenID integration with the Quepid (http://github.com/o19s/quepid) project. I am explicitly setting up a HTTP url, so it seems like under the principle of least surprise that we should accept that, instead of silently converting to HTTPS and then having an error. I think at a minimum if we aren't willing to accept HTTPS, then a clear error message should be thrown, not a "I tried to do a HTTPS lookup on your HTTP url and it failed due to werid ssl issues" that we have today. My feeling is that if I set up security with HTTP, well then, that is on me.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did the workaround (reference in the related issue) that let me use HTTP. A bit clunky. It feels like either HTTP works or it doesn't work, but not "it works in some cases and not others!". However, there may be nuances of all of this that I am not understanding ;-). |
||
| # | ||
| # This method fixes the issue by setting SWD.url_builder to URI::HTTP or URI::HTTPS | ||
| # based on the actual scheme of the discovery_base_url, ensuring the gem respects | ||
| # the explicitly configured protocol. | ||
| # | ||
| # See: https://github.com/nov/openid_connect/issues/47 | ||
| def configure_discovery_scheme | ||
| base_url = discovery_base_url | ||
| return if base_url.nil? || base_url.empty? | ||
|
|
||
| uri = URI.parse(base_url) | ||
|
|
||
| # Set SWD.url_builder to use HTTP or HTTPS based on the discovery URL scheme | ||
| # This prevents the gem from forcing HTTPS when HTTP is explicitly specified | ||
| if uri.scheme == 'http' | ||
| require 'swd' | ||
| SWD.url_builder = URI::HTTP | ||
| elsif uri.scheme == 'https' | ||
| require 'swd' | ||
| SWD.url_builder = URI::HTTPS | ||
| end | ||
| end | ||
|
|
||
| def discover! | ||
| return unless options.discovery | ||
|
|
||
|
|
@@ -258,6 +326,9 @@ def discover! | |
| client_options.userinfo_endpoint = config.userinfo_endpoint | ||
| client_options.jwks_uri = config.jwks_uri | ||
| client_options.end_session_endpoint = config.end_session_endpoint if config.respond_to?(:end_session_endpoint) | ||
|
|
||
| # Keep issuer consistent with what the provider advertises so later token verification uses the correct issuer. | ||
| options.issuer = config.issuer if config.respond_to?(:issuer) && config.issuer | ||
| end | ||
|
|
||
| def user_info | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should override a config option that was a simple String to a Proc. Why does the issuer need to be dynamic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think maybe it's because we do a lookup, but don't actually resolve it till we get to here? I'll see if there is another way. The whole "you have a proc and you expected a string" was annonying while doing development. And I didn't love the fix. In someways it's all a bit convoluted, and I felt like a more knowledgable person would do a different refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great call on this @stanhu ... For some crazy reason I had changed in my Quepid setup
to
It's much nicer now!