Add Support to set the SameSite attribute on the cookie to mitigate CSRF attacks

[jbwtan]

As I understand Vouch currently, there is no way to stop the browser sending the VouchCookie if a malicious link to the same domain the Vouch cookie is set to in vouch.domains, vouch config.

If we could set the SameSite on the cookie then we could ensure that only links in a browser to the protected resource server work on a subdomain (because only a subdomain will be allowed to send the VouchCookie)

More information on SameSite: https://web.dev/samesite-cookies-explained/

[bnfinet]

yep, that seems like a good idea

[bnfinet]

@jbwtan are you in a position to submit a PR?

[jbwtan]

@bnfinet first time contributing but yes I can give this a go. Do i need to know anything else before submitting a PR?
I was thinking of allowing a configuration value for config.yml e.g:
vouch: cookie: sameSite: Strict|Lax|None

[bnfinet]

yeah that sounds fine, and please add a default (I'm assuming Strict) to cfg.go.

And i should ask, do you see a way that adding default Strict could break existing setups? Perhaps we should at the very least push a log message at log.Info level as the default is set. Maybe it should link to this issue.

and, @jbwtan thanks for helping to improve VP!

[jbwtan]

Adding a default value would potentially break existing setups. Chrome <= Ver79 treats cookies without the SameSite attribute as SameSite=None.

From February onwards "Chrome will treat cookies that have no declared SameSite value as SameSite=Lax cookies. Only cookies with the SameSite=None; Secure setting will be available for external access"
https://blog.chromium.org/2019/10/developers-get-ready-for-new.html.

In order to minimise impact on existing setups, perhaps the default behaviour if the proposed vouch.cookie.sameSite value is not set should be to not to declare any SameSite attribute at all?

[bnfinet]

gotcha, yeah that sounds like the right way to handle it

[jbwtan]

i'm sure you're very busy but is there any way this feature could be prioritised higher given this is a potential security issue? Happy to chip in where I can

[bnfinet]

Just came across this.. "Cookies with SameSite=None must also specify Secure, meaning they require a secure context."
https://web.dev/samesite-cookies-explained/

I think there should be a log.Error or perhaps fatal if none is specified but secure is not set.

It's tempting to just set secure (and explain to the user with log.warn) but since TLS is always handled by nginx I think making sure the end user understands the situation is a bit better. cookie.secure is one of those items that pops up in support requests on a regular basis.

[bnfinet]

@jbwtan while testing #214 I came across this issue:
golang/go#36990

Do you have any thoughts on http.SameSiteDefaultMode vs http.SameSite(0)?

[jbwtan]

@bnfinet thanks for looking into the issue. good spot. I think the right approach for backwards compatibility would be to not write a same site attribute at all (and which should be how things are right now and handling of the cookie would be dependent on the browser). In that case if http.SameSite(0) doesn't write an attribute at all then I would opt for that even though it feels like a workaround for the implementation of SameSiteDefaultMode. I don't think that SameSiteDefaultMode should add an attribute key with no value

[aroden-crowdstrike]

Nginx hack to add SameSite=None to the tail of vouch.cookie.secure=True to make CORS work on latest chrome

    # in http context (only applies when $new_set_cookie is resolved
    map $upstream_http_set_cookie $new_set_cookie {
        ~(.*) '$1; SameSite=None';
    }

       # in server 
       location ^~ /vouch/ {
            proxy_pass http://127.0.0.1:9090/;
            add_header 'Set-Cookie' $new_set_cookie;
            proxy_hide_header 'Set-Cookie';
        }

[aroden-crowdstrike]

Recommendations based on the CHANGING operation of chrome/ff/safari in recent versions releasing ~this week.

Anyone using vouch to add auth to an API without same-origin front-ends (think localhost:80) is going to be annoyed by the defaults of SameSite

My recommendations for this project

• when secure=True add default of SameSite=None; most expected behavior from users but has existing risk of XSRF if CORS is poor i.e. just add the stupid cookie when hitting the site if otherwise allowed
• when secure=False add default of SameSite=Lax; NEW default of web-browsers i.e. only add the cookie when the nav bar is on the site OR following a href (window.location) to the site
• only when config opt-in to SameSite=Strict i.e. only add the cookie when already on the site

🤷‍♂ hope that helps. I already see a PR that would allow me to use this without hacks again

[bnfinet]

@aroden-crowdstrike what do you think of not adding an attribute at all by default. @jbwtan that's what you're suggesting, yes?

And, in order to not break existing setups would it be worth it to implement the logic detailed here..
https://www.chromium.org/updates/same-site/incompatible-clients

[jbwtan]

Hi @bnfinet. Yep thats what I'm suggesting as my understanding is that existing versions of vouch-proxy will not have a SameSite attribute defined in any way. So by setting sameSite = http.SameSite(0) as per the link golang/go#36990 you kindly shared, no attribute will be declared and therefore shouldnt break existing setups

[bnfinet]

#214 is merged

@jbwtan thanks for the contribution!