omniauth / omniauth_openid_connect

MIT License
170 stars 187 forks source link

Allow relaxing state check for IdP initiated SSO #122

Closed syakovyn closed 1 year ago

syakovyn commented 2 years ago

According to https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1 state is recommended and is not required:

state
  RECOMMENDED. Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request 
  Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie. 

The current implementation makes state required which, in turn, makes IdP initiated SSO impossible (or complicated with unnecessary redirections to get state parameter).

Does it make sense to add an option to relax check for state? Something like changing OmniAuth::Strategies::OpenIDConnect#callback_phase:

invalid_state = options.state_required && params['state'].to_s.empty? || params['state'] != stored_state

Where state_required defaults to true to be backward compatible but allows relaxing the state check to except missing state when there is no state stored on SP side (IdP initiated SSO).

skycocker commented 2 years ago

https://github.com/omniauth/omniauth_openid_connect/pull/127 WDYT?

syakovyn commented 2 years ago

I like it :) Though, I suppose you will need to add a test to get the PR merged by maintainers.

skycocker commented 2 years ago

Cool. Added a test case for this 💁‍♂️

syakovyn commented 2 years ago

I apologies, I didn't spotted initially that it completely removes checking the state when verify_state is false. I'd leave an error when the stored state doesn't match the passed one. Just ignore a case when when the state is neither passed nor stored. I've made a comment to the pull request: https://github.com/omniauth/omniauth_openid_connect/pull/127/files#r1006801215

stanhu commented 1 year ago

Closing since #127 has been merged.

syakovyn commented 4 months ago

https://github.com/omniauth/omniauth_openid_connect/pull/181 revives the issue in a slightly different form. See https://github.com/omniauth/omniauth_openid_connect/issues/174#issuecomment-2197421935