How not to send the `state` parameter? about omniauth_openid_connect HOT 21 OPEN

Yeah, I'm trying to avoid adding yet another config parameter here, plus clear up some confusion about how require_state is supposed to work. I'm not sure how require_state ever worked as false because stored_state will always have a value since new_state always sets the value to some random value.

from omniauth_openid_connect.

I think my initial intention was to prevent sending the state parameter during the request phase and make it so that it would not be expected during the callback phase.

@amessinger Thanks, we could consider using require_state to stop sending state as well. https://openid.net/specs/openid-connect-core-1_0.html says this about the authentication response:

state
    OAuth 2.0 state value. REQUIRED if the state parameter is present in the Authorization Request. Clients MUST verify that the state value is equal to the value of state parameter in the Authorization Request. 

A OIDC provider that drops the state from the response is thus not conforming to the spec.

from omniauth_openid_connect.

@syakovyn Interesting, thanks. I suppose we should revert #181 and then introduce a send_state parameter. The require_state setting is confusing, though.

from omniauth_openid_connect.

Just tested the changes shipped in #182. Works like a charm on my local environment (didn't have to change a thing appart from the Gemfile since the API is the same as on my forked implementation).

Thanks a lot, I'd say this issue can be closed.

from omniauth_openid_connect.

@skycocker @syakovyn @bufferoverflow I think require_state was added in #127. Do you see any reason why we couldn't simplify this via:

diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb
index ebfaaa1..6d80a66 100644
--- a/lib/omniauth/strategies/openid_connect.rb
+++ b/lib/omniauth/strategies/openid_connect.rb
@@ -120,7 +120,12 @@ module OmniAuth
       def callback_phase
         error = params['error_reason'] || params['error']
         error_description = params['error_description'] || params['error_reason']
-        invalid_state = (options.require_state && params['state'].to_s.empty?) || params['state'] != stored_state
+        invalid_state =
+          if options.require_state
+            params['state'].to_s.empty? || params['state'] != stored_state
+          else
+            false
+          end
 
         raise CallbackError, error: params['error'], reason: error_description, uri: params['error_uri'] if error
         raise CallbackError, error: :csrf_detected, reason: "Invalid 'state' parameter" if invalid_state

I'm not sure if state is being used somewhere else, but if it's just forwarded back, there's no use in generating it?

from omniauth_openid_connect.

Oh, I see that was proposed in #127 (comment).

The issue here is that if the state is generated by the authorize_uri, chances are the OpenID provider will pass it along. So maybe we should have gone with the original implementation?

from omniauth_openid_connect.

Hi! Thanks for looking into this.

In the mean time, I've gone ahead and made a working though naive implementation of a solution introducing an additional send_state config parameter.

I went with this solution because I didn't want to alter the behavior of the require_state config parameter.

amessinger@2a2510c

Hope this helps!

from omniauth_openid_connect.

I submitted #181 to fix require_state. Please feel to review.

from omniauth_openid_connect.

Thank you @stanhu for this!

The fix you're proposing allows me to complete my scenario of a stateless user agent which is not able to forward the state parameter for reasons out of my reach (aka my client's app :) ).

I think my initial intention was to prevent sending the state parameter during the request phase and make it so that it would not be expected during the callback phase.

I realize the "prevent sending de state parameter" part is more of a nice to have for me. It appears to be something the OIDC specs you mentioned in your PR aligns with me about base on the use of the "RECOMMENDED" key word.

That being said I understand that this library might wish to very slightly deviate from the specs to enforce good practices.

Please forgive me if I appear picky, I think this research and write up also serve me as a learning exercise on the subject.

Have a great rest of your day and thanks again!

from omniauth_openid_connect.

I think your interpretation of the specs is correct.

Maybe require_state could then be renamed send_state since, as I understand, the action of checking the state parameter would become a consequence of the client sending it in the first place?

Or maybe a more encompassing name would be more appropriate such as use_state or enable_state. I understand such a change would be breaking the API of the library so this might not be worth the hassle...

from omniauth_openid_connect.

Let me know whenever y'all are ready for the PR to be merged. I'll have to double check to see if this is one of the Omniauth libraries that I actually have rubygems release privileges for.

from omniauth_openid_connect.

I am sorry, but I have an issue with the decision made.

I have the 2 scenarios:

1. The service provider sends a state to the IdP and expects it back. I'm expecting the IdP to return the sent state back.
2. The IdP makes a direct request to the service provider without the state. No state check is expected here.

With the previous implementation, I could cover both cases with require_state = false.
Now, there is no check in the first case with require_state = false, or the check fails in the second scenario when require_state = true.

I would be happy with any suggestion that can cover both cases.
Probably, send_state or verify_state?

Thanks,
Sergii

from omniauth_openid_connect.

@syakovyn Thanks for raising your concerns here.

The service provider sends a state to the IdP and expects it back. I'm expecting the IdP to return the sent state back.

As discussed above, if we modified require_state NOT to send the state parameter in the first place, would this handle this case?

The IdP makes a direct request to the service provider without the state. No state check is expected here.

I'm a little confused about this case. In this case, the callback_phase is invoked directly from the IdP without authorize_uri? Perhaps a flow diagram would help.

from omniauth_openid_connect.

As discussed above, if we modified require_state NOT to send the state parameter in the first place, would this handle this case?

I'd like to keep sending the state parameter by my service provider and expect the 3rd party IdP to return it. I'm pretty happy with require_state = true as it does what I need here.

I'm a little confused about this case. In this case, the callback_phase is invoked directly from the IdP without authorize_uri?

Exactly. The IdP keeps registered service providers and allows users to sign into a service provider directly from the IdP site. I am required to support that flow and, as the IdP doesn't have the state to send it fails with an error when require_state = true.

I thought having require_state = false would be reasonable to work in both cases meaning the gem doesn't require the state to be present in the response but validates if it matches if present.

The problem is that, in the first case, I want to send the state and validate it as suggested by the OIDC specification, and, in the second case, it looks like I didn't send the state though it's because the flow starts right away from the callback_phase.

from omniauth_openid_connect.

@syakovyn How does the IdP gain authorization to make this callback, though? Does this mean that anyone could spoof the IdP? UPDATE: I suppose the IdP somehow has a valid ID token?

I see https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin touches on this.

from omniauth_openid_connect.

@stanhu, the IdP (in my case it's Clever) uses the first redirect URI and sends the ID token of the user that initiated that request.

UPDATE: It is similar to Send ID Token directly to app (Okta Simplified) described on https://support.okta.com/help/s/article/How-to-make-the-OIDCOAuth-App-visible-in-Okta-dashboard-and-what-are-the-login-flows-available?language=en_US

My case is also described on https://stackoverflow.com/a/51050953/419735

from omniauth_openid_connect.

Actually, I don't need require_state if the condition is invalid_state = params['state'] != stored_state because, in the 1st scenario, I have a stored state and want it to match the one returned by IdP and in the 2nd scenario I have it neither stored nor passed by an IdP.

Thus, having some option not to have/generate the state should cover my 2 scenarios and this one. E.g. allowing state = nil as a configuration parameter in addition to state = Proc.new { SecureRandom.hex(32) }, storing and sending the generated state only if it can be generated.

I would opt for generating the state (probably should be a default setting) and @amessinger will opt out of generating the state.

Or even reusing the current state option with state = Proc.new { nil } and not sending nil state.
What do you think of it, @stanhu?

from omniauth_openid_connect.

hi @syakovyn, that's fine by me since I've been doing it on my fork 😉 specifically I do agree that sending the state should done by default.

from omniauth_openid_connect.

I think think an explicit send_state is less confusing than requiring state = Proc.new { nil } and allowing a blank state.

require_state is confusing because in the most common case where you expect the authorize step to run, I would expect thatstate be omitted entirely from the request and thus ignored in the response.

from omniauth_openid_connect.

Please take a look at #182.

from omniauth_openid_connect.

The change works for me too. Thanks @stanhu

from omniauth_openid_connect.