Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Or install it yourself as:

## Supported Ruby Versions

OmniAuth::OpenIDConnect is tested under 2.7, 3.0, 3.1, 3.2, 3.3, 3.4
OmniAuth::OpenIDConnect is tested under 2.7, 3.0, 3.1, 3.2, 3.3, 3.4, 4.0

## Usage

Expand Down
71 changes: 66 additions & 5 deletions lib/omniauth/strategies/openid_connect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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!
Expand All @@ -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
Expand Down Expand Up @@ -250,6 +252,62 @@ def issuer
::OpenIDConnect::Discovery::Provider.discover!(resource).issuer
end

# Helper method to safely check if issuer is empty
def issuer_empty?
options.issuer.nil? || options.issuer.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
issuer = options.issuer.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'
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).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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

Expand All @@ -258,6 +316,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
Expand Down
76 changes: 76 additions & 0 deletions test/lib/omniauth/strategies/openid_connect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,82 @@ def test_request_phase_with_discovery
assert_nil strategy.options.client_options.end_session_endpoint
end

def test_request_phase_with_discovery_http_scheme_and_port
expected_redirect = %r{^http://keycloak:8080/authorize\?client_id=1234&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}$}

strategy.options.client_options.scheme = 'http'
strategy.options.client_options.host = 'keycloak'
strategy.options.client_options.port = 8080
strategy.options.discovery = true

# Simulate provider discovery for issuer (used when options.issuer is empty)
issuer = stub('OpenIDConnect::Discovery::Issuer')
issuer.stubs(:issuer).returns('http://keycloak:8080/realms/quepid')
::OpenIDConnect::Discovery::Provider.stubs(:discover!).with('http://keycloak:8080').returns(issuer)

config = stub('OpenIDConnect::Discovery::Provider::Config')
config.stubs(:issuer).returns('http://keycloak:8080/realms/quepid')
config.stubs(:authorization_endpoint).returns('http://keycloak:8080/authorize')
config.stubs(:token_endpoint).returns('http://keycloak:8080/token')
config.stubs(:userinfo_endpoint).returns('http://keycloak:8080/userinfo')
config.stubs(:jwks_uri).returns('http://keycloak:8080/jwks')
::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('http://keycloak:8080/realms/quepid').returns(config)

strategy.expects(:redirect).with(regexp_matches(expected_redirect))
strategy.request_phase

assert_equal 'http://keycloak:8080/realms/quepid', strategy.options.issuer
assert_equal 'http://keycloak:8080/authorize', strategy.options.client_options.authorization_endpoint
end

def test_discovery_ignores_scheme_less_issuer_and_falls_back_to_client_options
# issuer is present but scheme-less; discovery should not use it as base
strategy.options.issuer = 'keycloak:8080/realms/quepid'
strategy.options.discovery = true
strategy.options.client_options.scheme = 'http'
strategy.options.client_options.host = 'keycloak'
strategy.options.client_options.port = 8080

config = stub('OpenIDConnect::Discovery::Provider::Config')
config.stubs(:issuer).returns('http://keycloak:8080/realms/quepid')
config.stubs(:authorization_endpoint).returns('http://keycloak:8080/authorize')
config.stubs(:token_endpoint).returns('http://keycloak:8080/token')
config.stubs(:userinfo_endpoint).returns('http://keycloak:8080/userinfo')
config.stubs(:jwks_uri).returns('http://keycloak:8080/jwks')

# key assertion: discover! uses http://keycloak:8080 (from client_options), NOT the scheme-less issuer
::OpenIDConnect::Discovery::Provider::Config.expects(:discover!).with('http://keycloak:8080').returns(config)

# call the private method via request_phase which triggers discover!
::OpenIDConnect::Discovery::Provider.stubs(:discover!).returns(stub('OpenIDConnect::Discovery::Issuer', issuer: 'http://keycloak:8080/realms/quepid'))

strategy.expects(:redirect)
strategy.request_phase

assert_equal 'http://keycloak:8080/realms/quepid', strategy.options.issuer
end

def test_configure_discovery_scheme_sets_swd_url_builder_for_http
require 'swd'

# Test with HTTP scheme
strategy.options.issuer = 'http://keycloak:8080/realms/quepid'
strategy.options.discovery = true

# Call the private method to configure discovery scheme
strategy.send(:configure_discovery_scheme)

# Verify SWD.url_builder is set to URI::HTTP for HTTP URLs
assert_equal URI::HTTP, SWD.url_builder

# Test with HTTPS scheme
strategy.options.issuer = 'https://example.com/realms/test'
strategy.send(:configure_discovery_scheme)

# Verify SWD.url_builder is set to URI::HTTPS for HTTPS URLs
assert_equal URI::HTTPS, SWD.url_builder
end

def test_request_phase_with_response_mode
expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$}
strategy.options.issuer = 'example.com'
Expand Down