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

Session-cookie with samesite = strict breaks login-procedure #45

Open
baukezwaan opened this issue Apr 4, 2023 · 4 comments · Fixed by #55
Open

Session-cookie with samesite = strict breaks login-procedure #45

baukezwaan opened this issue Apr 4, 2023 · 4 comments · Fixed by #55

Comments

@baukezwaan
Copy link

In the latest release of Silverstripe CMS (>4.12) the samesite-attribute for cookies is configurable. It is recommended to set this value to strict for the cookies and the session-cookie. (see https://docs.silverstripe.org/en/4/changelogs/4.12.0/#cookies-samesite)

I believe this introduces a problem for the SAML-authentication. From what I understand, the problem lies within the session-cookie. This session-cookie should not be loaded, after returning from the IdP (on /saml/asc) - since this is another domain (not samesite). This is roughly same scenario as IdP-initiated logins, where no prior session-cookie is available on the first request.

The problem seems to be specifically in the redirection at the very end of asc() method. Here the user is redirected via 301 to the desired destinationpage. Because of this same-site cookie restriction, this next page doesn't get the proper session-cookie ans Silverstripe starts a new session.

Let me try explain with some screenshots:

Scenario lax:

Same-site set to lax (the session-cookie is held correctly)

/saml/acs

image
the request cookie is also set, because I was using SP-initiated request. This session is reset by the login process. The session-cookie starts with k3d... is send to the next page. The response code of this page is 302.

/home (destination page)

image
the same session-cookie is still there, starting with k3d...

Scenario strict:

Same-site set to strict (the session-cookie is NOT held correctly)

/saml/acs

image
_the request cookie is empty, since the same site attribute prevents this. No problem, a new session-cookie is set starting with nqb... _

/home (destination page)

image
the request cookie is empty, because the 302 redirect is treated with the same cookies as the original request. A new session-cookie is set uk9... and thus not allowing us the keep the logged in state for this page.

Reproduce

Solution

The solutions for us was to redirect the user via the browser (meta-refresh), so the destination-page is allowed to use the same session-cookie. Totally not ideal, but works for now. For inspiration see htmlRedirect in the framework

@mateusz
Copy link
Contributor

mateusz commented Jan 9, 2024

I've actually ran into the same thing just now - for HTTP POST binding across sites even Lax will drop the cookie [definitive citation needed]. I think this is a classic reason why RelayState is used. Unfortunately I think this module does not handle that part, but it is implemented in the underlying onelogin driver.

Would you care to share your solution more specifically? Where did you put the meta-refresh, was it on the SP side and did you have to hook it in?

@baukezwaan
Copy link
Author

Would you care to share your solution more specifically? Where did you put the meta-refresh, was it on the SP side and did you have to hook it in?

@mateusz Although a fix has seems to be implemented, I'll share our work-around. Since i'm not sure if the RelayState is always available and a redirect still might be in the way.

We planned to do this on the SP-side (in our application) by overwriting the asc(0)-method in the SAMLController-class. But ultimately we decided to go with the same-site=lax setting. But the code work-in-progess would looked something along the lines:

SilverStripe\Core\Injector\Injector:
  SilverStripe\SAML\Control\SAMLController:
    class: SAMLControllerExt
    class SAMLControllerExt extends SAMLController
    {
        
        public function acs() {

               // copy exitsing code here....

            $headersSent = headers_sent($file, $line);
            $location = '/';
            $url = Director::absoluteURL($location);
            $urlATT = Convert::raw2htmlatt($url);
            $urlJS = Convert::raw2js($url);
            $title = (Director::isDev() && $headersSent) ? "{$urlATT}... (output started on {$file}, line {$line})" : "{$urlATT}...";
            echo <<<EOT
    <p>Redirecting to <a href="{$urlATT}" title="Click this link if your browser does not redirect you">{$title}</a></p>
    <meta http-equiv="refresh" content="1; url={$urlATT}" />
    <script type="application/javascript">setTimeout(function(){
        window.location.href = "{$urlJS}";
    }, 50);</script>
    EOT
            ;
            exit;

        }
    }

I see that the method has an extention-hook updateLogin, so that would be probably be a better way to implement.

@mateusz
Copy link
Contributor

mateusz commented Jan 29, 2024

As far as I know, RelayState is part of SAML2 spec.

But I think I may have misunderstood your issue - I see what you mean now. This fix will not help, I think the lost cookie effect you describe will not be affected. Thanks for sharing the code - it's probably something that should be sent in as a pull request :-)

@satrun77 - can you please reopen this, as I think my fix fixes another problem altogether.

@satrun77 satrun77 reopened this Jan 29, 2024
@baukezwaan
Copy link
Author

As far as I know, RelayState is part of SAML2 spec.

You are certainly right, but afaik this RelayState is not used by a default login situation. Come to think of it, the problem with losing sessions because of the redirect would probably still exist even if you would use it, since only the URL where to redirect to has changed.

But the support for RelayState is still very useful, so no code has been wasted 👍

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

Successfully merging a pull request may close this issue.

3 participants