Skip to content

Commit

Permalink
fix(saml): ensure session cookie survives idp redirect (#801)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@cfieber @anotherchrisberry
cfieber authored and anotherchrisberry committed May 15, 2019
1 parent 000f9e6 commit 271bfab
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 2 deletions.
1 change: 1 addition & 0 deletions gate-saml/gate-saml.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ dependencies{
implementation "com.netflix.spinnaker.kork:kork-exceptions"
implementation "com.netflix.spinnaker.kork:kork-security"
implementation "com.netflix.spectator:spectator-api"
implementation 'org.springframework.session:spring-session-core'

implementation "org.springframework.security.extensions:spring-security-saml2-core"
implementation "org.springframework.security.extensions:spring-security-saml-dsl-core"
Expand Down
8 changes: 6 additions & 2 deletions gate-saml/src/main/groovy/com/netflix/spinnaker/gate/security/saml/SamlSsoConfig.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ import org.springframework.security.core.userdetails.UserDetailsService
import org.springframework.security.core.userdetails.UsernameNotFoundException
import org.springframework.security.extensions.saml2.config.SAMLConfigurer
import org.springframework.security.saml.SAMLCredential
import org.springframework.security.saml.storage.EmptyStorageFactory
import org.springframework.security.saml.userdetails.SAMLUserDetailsService
import org.springframework.security.web.authentication.RememberMeServices
import org.springframework.security.web.authentication.rememberme.TokenBasedRememberMeServices
import org.springframework.session.web.http.DefaultCookieSerializer
import org.springframework.stereotype.Component

import javax.annotation.PostConstruct
Expand All @@ -65,6 +65,9 @@ class SamlSsoConfig extends WebSecurityConfigurerAdapter {
@Autowired
ServerProperties serverProperties

@Autowired
DefaultCookieSerializer defaultCookieSerializer

@Autowired
AuthConfig authConfig

Expand Down Expand Up @@ -130,6 +133,8 @@ class SamlSsoConfig extends WebSecurityConfigurerAdapter {

@Override
void configure(HttpSecurity http) {
//We need our session cookie to come across when we get redirected back from the IdP:
defaultCookieSerializer.setSameSite(null)
authConfig.configure(http)

http
Expand All @@ -149,7 +154,6 @@ class SamlSsoConfig extends WebSecurityConfigurerAdapter {
.protocol(samlSecurityConfigProperties.redirectProtocol)
.hostname(samlSecurityConfigProperties.redirectHostname ?: serverProperties?.address?.hostName)
.basePath(samlSecurityConfigProperties.redirectBasePath)
.storageFactory(new EmptyStorageFactory())
.keyStore()
.storeFilePath(samlSecurityConfigProperties.keyStore)
.password(samlSecurityConfigProperties.keyStorePassword)
Expand Down

2 comments on commit 271bfab

@clanesf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say with 100% certainty that this is related, but it seems like it's worth mentioning here.

So today we tried using master-latest-unvalidated in one of our dev environments because we desperately need a feature that's coming out in 1.14 and I needed to give a team time to work on it. Prior to this we were running 1.13.6.

After upgrading to master-latest-unvalidated my Google oauth authentication tried and failed even though the configuration hadn't changed at all between versions.

After some time hunting down the issue, I was able to see that the redirect_uri that we are passing to the Service Provider has changed. It was previously passing more or less that gateurl/login, but all of a sudden it was sending a google nternal ip/login. This resulted in an error from google saying

Error: invalid_request device_id and device_name are required for private IP:

I was able to correct the redirect URI by using the following halyard command, but it's strange that we've never had to do this for any of our other instances

hal config security authn oauth2 edit --pre-established-redirect-uri <redirect uri>

Again I'm not 100% certain that this is the cause of the issue I'm seeing, but it seems like a possibility.

I'm on Spinnaker Slack with the name Chuck Lane if you need any additional Info.

@sagayd
Copy link

@sagayd sagayd commented on 271bfab Jul 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have similar --pre-established-redirect-uri to ldap authentication also? Currently ldap auth does not have it

Please sign in to comment.