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.

Sign up for GitHub

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

Jump to bottom

feat: Add workaround for CSRF SameSite=None cookies #1810

Merged
merged 3 commits into from Apr 24, 2020
Merged

feat: Add workaround for CSRF SameSite=None cookies #1810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f11ca0
consent: Workaround for CSRF SameSite=None cookies
Apr 21, 2020
1d4effa
fix(consent): fix few test cases for validate csrf session
Apr 24, 2020
66e9443
docs: mention serve.cookies.same_site_legacy_workaround in the advanc…
Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions .schema/config.schema.json
View file
Open in desktop
Expand Up @@ -346,6 +346,14 @@
"None"
],
"default": "None"
},
"same_site_legacy_workaround": {
"type": "boolean",
"description": "Some older browser versions don’t work with SameSite=None. This option enables the workaround defined in https://web.dev/samesite-cookie-recipes/ which essentially stores a second cookie without SameSite as a fallback.",
"default": false,
"examples": [
true
]
}
}
}
Expand Down
22 changes: 18 additions & 4 deletions consent/helper.go
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ func matchScopes(scopeStrategy fosite.ScopeStrategy, previousConsent []HandledCo
return nil
}

func createCsrfSession(w http.ResponseWriter, r *http.Request, store sessions.Store, name, csrf string, secure bool, sameSiteMode http.SameSite) error {
func createCsrfSession(w http.ResponseWriter, r *http.Request, store sessions.Store, name, csrf string, secure bool, sameSiteMode http.SameSite, sameSiteLegacyWorkaround bool) error {
// Errors can be ignored here, because we always get a session session back. Error typically means that the
// session doesn't exist yet.
session, _ := store.Get(r, name)
Expand All @@ -72,12 +72,14 @@ func createCsrfSession(w http.ResponseWriter, r *http.Request, store sessions.St
if err := session.Save(r, w); err != nil {
return errors.WithStack(err)
}

if sameSiteMode == http.SameSiteNoneMode && sameSiteLegacyWorkaround {
return createCsrfSession(w, r, store, legacyCsrfSessionName(name), csrf, secure, http.SameSiteDefaultMode, false)
}
return nil
}

func validateCsrfSession(r *http.Request, store sessions.Store, name, expectedCSRF string) error {
if cookie, err := store.Get(r, name); err != nil {
func validateCsrfSession(r *http.Request, store sessions.Store, name, expectedCSRF string, sameSiteLegacyWorkaround bool) error {
if cookie, err := getCsrfSession(r, store, name, sameSiteLegacyWorkaround); err != nil {
return errors.WithStack(fosite.ErrRequestForbidden.WithDebug("CSRF session cookie could not be decoded"))
} else if csrf, err := mapx.GetString(cookie.Values, "csrf"); err != nil {
return errors.WithStack(fosite.ErrRequestForbidden.WithDebug("No CSRF value available in the session cookie"))
Expand All @@ -87,3 +89,15 @@ func validateCsrfSession(r *http.Request, store sessions.Store, name, expectedCS

return nil
}

func getCsrfSession(r *http.Request, store sessions.Store, name string, sameSiteLegacyWorkaround bool) (*sessions.Session, error) {
cookie, err := store.Get(r, name)
if sameSiteLegacyWorkaround && (err != nil || len(cookie.Values) == 0) {
return store.Get(r, legacyCsrfSessionName(name))
}
return cookie, err
}

func legacyCsrfSessionName(name string) string {
return name + "_legacy"
}
265 changes: 265 additions & 0 deletions consent/helper_test.go
View file
Open in desktop
Expand Up @@ -22,8 +22,12 @@ package consent

import (
"fmt"
"net/http"
"net/http/httptest"
"testing"

"github.com/gorilla/securecookie"
"github.com/gorilla/sessions"
"github.com/stretchr/testify/assert"

"github.com/ory/fosite"
Expand Down Expand Up @@ -95,3 +99,264 @@ func TestMatchScopes(t *testing.T) {
})
}
}

func TestValidateCsrfSession(t *testing.T) {
type cookie struct {
name string
csrfValue string
sameSite http.SameSite
}
for k, tc := range []struct {
cookies []cookie
csrfValue string
sameSiteLegacyWorkaround bool
expectError bool
}{
{
cookies: []cookie{},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: false,
expectError: true,
},
{
cookies: []cookie{},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: true,
expectError: true,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "WRONG-CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: false,
expectError: true,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "WRONG-CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: true,
expectError: true,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: false,
expectError: false,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: true,
expectError: false,
},
{
cookies: []cookie{
{
name: legacyCsrfSessionName(cookieAuthenticationCSRFName),
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: false,
expectError: true,
},
{
cookies: []cookie{
{
name: legacyCsrfSessionName(cookieAuthenticationCSRFName),
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: true,
expectError: false,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteNoneMode,
},
{
name: legacyCsrfSessionName(cookieAuthenticationCSRFName),
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: false,
expectError: false,
},
{
cookies: []cookie{
{
name: cookieAuthenticationCSRFName,
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteNoneMode,
},
{
name: legacyCsrfSessionName(cookieAuthenticationCSRFName),
csrfValue: "CSRF-VALUE",
sameSite: http.SameSiteDefaultMode,
},
},
csrfValue: "CSRF-VALUE",
sameSiteLegacyWorkaround: true,
expectError: false,
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
store := sessions.NewCookieStore(securecookie.GenerateRandomKey(16))
r := httptest.NewRequest(http.MethodGet, "/", nil)
w := httptest.NewRecorder()

for _, c := range tc.cookies {
session, _ := store.Get(r, c.name)
session.Values["csrf"] = c.csrfValue
session.Options.HttpOnly = true
session.Options.Secure = true
session.Options.SameSite = c.sameSite
err := session.Save(r, w)
assert.NoError(t, err, "failed to save cookie %s", c.name)
}

err := validateCsrfSession(r, store, cookieAuthenticationCSRFName, tc.csrfValue, tc.sameSiteLegacyWorkaround)
if tc.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
})
}
}

func TestCreateCsrfSession(t *testing.T) {
type cookie struct {
httpOnly bool
secure bool
sameSite http.SameSite
}
for _, tc := range []struct {
name string
secure bool
sameSite http.SameSite
sameSiteLegacyWorkaround bool
expectedCookies map[string]cookie
}{
{
name: "csrf_default",
secure: true,
sameSite: http.SameSiteDefaultMode,
sameSiteLegacyWorkaround: false,
expectedCookies: map[string]cookie{
"csrf_default": {
httpOnly: true,
secure: true,
sameSite: http.SameSiteDefaultMode,
},
},
},
{
name: "csrf_lax_insecure",
secure: false,
sameSite: http.SameSiteLaxMode,
sameSiteLegacyWorkaround: false,
expectedCookies: map[string]cookie{
"csrf_lax_insecure": {
httpOnly: true,
secure: false,
sameSite: http.SameSiteLaxMode,
},
},
},
{
name: "csrf_none",
secure: true,
sameSite: http.SameSiteNoneMode,
sameSiteLegacyWorkaround: false,
expectedCookies: map[string]cookie{
"csrf_none": {
httpOnly: true,
secure: true,
sameSite: http.SameSiteNoneMode,
},
},
},
{
name: "csrf_none_fallback",
secure: true,
sameSite: http.SameSiteNoneMode,
sameSiteLegacyWorkaround: true,
expectedCookies: map[string]cookie{
"csrf_none_fallback": {
httpOnly: true,
secure: true,
sameSite: http.SameSiteNoneMode,
},
"csrf_none_fallback_legacy": {
httpOnly: true,
secure: true,
sameSite: http.SameSiteDefaultMode,
},
},
},
{
name: "csrf_strict_fallback_ignored",
secure: true,
sameSite: http.SameSiteStrictMode,
sameSiteLegacyWorkaround: true,
expectedCookies: map[string]cookie{
"csrf_strict_fallback_ignored": {
httpOnly: true,
secure: true,
sameSite: http.SameSiteStrictMode,
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
store := sessions.NewCookieStore(securecookie.GenerateRandomKey(16))
rr := httptest.NewRecorder()
req := httptest.NewRequest(http.MethodGet, "/", nil)

err := createCsrfSession(rr, req, store, tc.name, "value", tc.secure, tc.sameSite, tc.sameSiteLegacyWorkaround)
assert.NoError(t, err)

cookies := make(map[string]cookie)
for _, c := range rr.Result().Cookies() {
cookies[c.Name] = cookie{
httpOnly: c.HttpOnly,
secure: c.Secure,
sameSite: c.SameSite,
}
}
assert.Equal(t, tc.expectedCookies, cookies)
})
}
}
8 changes: 4 additions & 4 deletions consent/strategy_default.go
View file
Open in desktop
Expand Up @@ -276,7 +276,7 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(w http.ResponseWriter, r
return errors.WithStack(err)
}

if err := createCsrfSession(w, r, s.r.CookieStore(), cookieAuthenticationCSRFName, csrf, s.c.ServesHTTPS(), s.c.CookieSameSiteMode()); err != nil {
if err := createCsrfSession(w, r, s.r.CookieStore(), cookieAuthenticationCSRFName, csrf, s.c.ServesHTTPS(), s.c.CookieSameSiteMode(), s.c.CookieSameSiteLegacyWorkaround()); err != nil {
return errors.WithStack(err)
}

Expand Down Expand Up @@ -349,7 +349,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
return nil, errors.WithStack(fosite.ErrRequestUnauthorized.WithDebug("The login request has expired, please try again."))
}

if err := validateCsrfSession(r, s.r.CookieStore(), cookieAuthenticationCSRFName, session.LoginRequest.CSRF); err != nil {
if err := validateCsrfSession(r, s.r.CookieStore(), cookieAuthenticationCSRFName, session.LoginRequest.CSRF, s.c.CookieSameSiteLegacyWorkaround()); err != nil {
return nil, err
}

Expand Down Expand Up @@ -553,7 +553,7 @@ func (s *DefaultStrategy) forwardConsentRequest(w http.ResponseWriter, r *http.R
return errors.WithStack(err)
}

if err := createCsrfSession(w, r, s.r.CookieStore(), cookieConsentCSRFName, csrf, s.c.ServesHTTPS(), s.c.CookieSameSiteMode()); err != nil {
if err := createCsrfSession(w, r, s.r.CookieStore(), cookieConsentCSRFName, csrf, s.c.ServesHTTPS(), s.c.CookieSameSiteMode(), s.c.CookieSameSiteLegacyWorkaround()); err != nil {
return errors.WithStack(err)
}

Expand Down Expand Up @@ -588,7 +588,7 @@ func (s *DefaultStrategy) verifyConsent(w http.ResponseWriter, r *http.Request,
return nil, errors.WithStack(fosite.ErrServerError.WithDebug("The authenticatedAt value was not set."))
}

if err := validateCsrfSession(r, s.r.CookieStore(), cookieConsentCSRFName, session.ConsentRequest.CSRF); err != nil {
if err := validateCsrfSession(r, s.r.CookieStore(), cookieConsentCSRFName, session.ConsentRequest.CSRF, s.c.CookieSameSiteLegacyWorkaround()); err != nil {
return nil, err
}

Expand Down
7 changes: 7 additions & 0 deletions docs/docs/advanced.md
View file
Open in desktop
Expand Up @@ -448,3 +448,10 @@ values are `Strict`, `Lax` or `None`.
If you wish to embed requests to hydra on a third party site (for example an
iframe that periodically polls to check session status) you will need to set the
mode to `None`.

Some [browser versions](https://www.chromium.org/updates/same-site/incompatible-clients)
reject cookies using the `Same-Site=None` attribute. Hydra implements a
[workaround](https://web.dev/samesite-cookie-recipes/#handling-incompatible-clients)
that can be enabled by setting `serve.cookies.same_site_legacy_workaround` to
`true`. This workaround is disabled by default, and only takes effect when
`serve.cookies.same_site_mode` is set to `None`.