Handle discovery based lookup when using http and nested realms (keycloak style)#211
Handle discovery based lookup when using http and nested realms (keycloak style)#211epugh wants to merge 9 commits intoomniauth:masterfrom
Conversation
|
@stanhu would you be willing to trigger the workflows? I'd love to get this PR to passing all the workflows and get your input on what I need to do to have it ready for a merge ;-) |
| 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 |
There was a problem hiding this comment.
Is this stale debugging code? This checks for Rails.logger but doesn't use it. I don't think we should be using a puts here either.
| 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) |
There was a problem hiding this comment.
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.
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.
Great call on this @stanhu ... For some crazy reason I had changed in my Quepid setup
issuer: Rails.application.config.openid_connect_issuer,
to
issuer: { Rails.application.config.openid_connect_issuer },
It's much nicer now!
| # 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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;-).
|
@stanhu OMG... I "code reviewed" the wrong branch, and so of course you got these silly glitches from my dev work... So embarraseed. I'll get these fixed asp. |
|
I've reworked the PR to take into account feedback and done some local testing. Thanks for the suggestions @stanhu... Thoughts on if this is something we want to move forward with? |
|
I did a quick experiment of using a self signed certificate with Keycloak to run it on HTTPS, and of course ran into the fact that the self signed certificate doesn't work with omniauth_openid_connect: I will see if there is a setting to ignore that and accept self signed certificates. |
2 participants
I think I have stumbled across two bugs.
The first is that if I am using a
httpurl with the discovery link, SWD was forcing it to ahttps, regardless of settings. There is a workaround described in the issue #186, however this PR should fix the bug!The second is that Keycload used a nested url for the well known url:
http://keycloak:8080/realms/quepid/.well-known/openid-configurationand it was being converted tohttp://keycloak:8080/.well-known/openid-configuration