Skip to content
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

fix(saml): ensure session cookie survives idp redirect #801

Merged
merged 1 commit into from
May 15, 2019

Conversation

cfieber
Copy link
Contributor

@cfieber cfieber commented May 15, 2019

Fixes an issue where the session cookie was being marked SameSite by the boot2 upgrade
and as a result we would lose an existing session / start a new session after redirect
back from the IdP.

No longer requires EmptyStorageFactory (went in as a workaround for this issue initially),
and fixes redirect after login back to the original request URI being busted.

Fixes an issue where the session cookie was being marked SameSite by the boot2 upgrade
and as a result we would lose an existing session / start a new session after redirect
back from the IdP.

No longer requires EmptyStorageFactory (went in as a workaround for this issue initially),
and fixes redirect after login back to the original request URI being busted.
@anotherchrisberry anotherchrisberry merged commit 271bfab into spinnaker:master May 15, 2019
cfieber added a commit to cfieber/gate that referenced this pull request May 16, 2019
Similar to spinnaker#801 when using oauth removes the samesite attribute from
the session cookie.
@cfieber cfieber deleted the saml_session branch May 16, 2019 16:51
cfieber added a commit that referenced this pull request May 16, 2019
Similar to #801 when using oauth removes the samesite attribute from
the session cookie.
@clanesf
Copy link
Contributor

clanesf commented May 21, 2019

If this is a fix to the oauth redirect uri breaking between 1.13.6 and 1.14, could we please get this cherry-picked into 1.14.1?

spinnakerbot pushed a commit that referenced this pull request May 23, 2019
Similar to #801 when using oauth removes the samesite attribute from
the session cookie.
dibyom pushed a commit that referenced this pull request May 23, 2019
Similar to #801 when using oauth removes the samesite attribute from
the session cookie.
@dibyom
Copy link
Member

dibyom commented May 24, 2019

@spinnakerbot cherry-pick 1.14

spinnakerbot pushed a commit that referenced this pull request May 24, 2019
Fixes an issue where the session cookie was being marked SameSite by the boot2 upgrade
and as a result we would lose an existing session / start a new session after redirect
back from the IdP.

No longer requires EmptyStorageFactory (went in as a workaround for this issue initially),
and fixes redirect after login back to the original request URI being busted.
@spinnakerbot
Copy link
Contributor

Cherry pick successful: #809

maggieneterval pushed a commit that referenced this pull request May 24, 2019
Fixes an issue where the session cookie was being marked SameSite by the boot2 upgrade
and as a result we would lose an existing session / start a new session after redirect
back from the IdP.

No longer requires EmptyStorageFactory (went in as a workaround for this issue initially),
and fixes redirect after login back to the original request URI being busted.
@clanesf
Copy link
Contributor

clanesf commented May 31, 2019

We're still seeing multiple issues in 1.14.2 related to redirectUri that are not present in the 1.13.X branches. Upgrading to 1.14.X is effectively breaking oauth for multiple people.
See https://spinnakerteam.slack.com/archives/C091CCWRJ/p1559273903055200?thread_ts=1558639811.086800&cid=C091CCWRJ

@maggieneterval
Copy link
Contributor

@dibyom @cfieber any idea if the fix mentioned in a comment on this issue is the resolution we should be suggesting here?

@dibyom
Copy link
Member

dibyom commented Jun 3, 2019

I think that works for many cases but not all e.g. if you have internal services trying to access spinnaker via an internal address they now get redirected to the external UI: https://spinnakerteam.slack.com/archives/C091CCWRJ/p1558639811086800

@maggieneterval
Copy link
Contributor

Ah okay, is the root cause the spring boot upgrade? Guessing @cfieber is looking into this? Just asking because it seems to be causing issues for a lot of folks in 1.14 so would love to get a fix patched in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants