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

Add SameSite cookie configuration value for session cookie #339

Merged
merged 4 commits into from Jan 20, 2020
Merged

Add SameSite cookie configuration value for session cookie #339

merged 4 commits into from Jan 20, 2020

Conversation

pgroudas
Copy link
Contributor

@pgroudas pgroudas commented Dec 20, 2019

Values of 'lax' and 'strict' can improve and mitigate
some categories of cross-site traffic tampering.

Given that the nature of this proxy is often to proxy
private tools, this is useful to take advantage of.

See: https://www.owasp.org/index.php/SameSite

Description

This extends the configuration options to add an additional field for COOKIE_SAMESITE
which will be propagated into options used to configure the authentication / session cookie.

If not provided, this will be set to the value of http.SameSiteDefaultMode, which is effectively
the same behavior as before this change.

However, this now allows a user to opt into using SameSiteLaxMode, SameSiteStrictMode,
or SameSiteNoneMode.

Motivation and Context

This change improves security of proxied services by preventing CSRF attacks.

See: See: https://www.owasp.org/index.php/SameSite

How Has This Been Tested?

Unit tests have been updated to verify the additional field on the session cookie.

The SameSite cookie attribute has been verified in debug tools in chrome.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@pgroudas pgroudas requested a review from a team December 20, 2019 16:30
@pgroudas pgroudas changed the title Pgroudas/add samesite cookie options Add SameSite cookie configuration value for session cookie Dec 20, 2019
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Hey @pgroudas, thanks for raising the PR! Looks good to me in general, very clean code!

I've added one potential improvement as a suggestion below, let me know what you think 😄

case "none":
return http.SameSiteNoneMode
default:
return http.SameSiteDefaultMode
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the default should be a panic to guard against future changes to the flag checking that might break the functionality?

    ...
    case "":
		return http.SameSiteDefaultMode
    default:
        panic(fmt.Sprintf("Invalid value for SameSite: %s", v))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done!

Paul Groudas added 2 commits January 6, 2020 12:21
Values of 'lax' and 'strict' can improve and mitigate
some categories of cross-site traffic tampering.

Given that the nature of this proxy is often to proxy
private tools, this is useful to take advantage of.

See: https://www.owasp.org/index.php/SameSite
@loshz loshz requested a review from JoelSpeed January 8, 2020 13:35
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @pgroudas!

@JoelSpeed JoelSpeed merged commit ec72ee8 into oauth2-proxy:master Jan 20, 2020
@idntfy
Copy link
Contributor

idntfy commented Jan 28, 2020

@JoelSpeed when do you think it can be expected to be released? the last released version 4.1.0 doesn't have this flag yet, I have to use latest docker build instead of pinned version which is unsafe (not because of changes since we put it to our own repository anyway, but rather because it's a non-released version)

@JoelSpeed
Copy link
Member

@idntfy We are planning to release soon, though we need to verify/fix #362 before we can make a release, as soon as that is done the release will happen. Hopefully that will be this week or next

@idntfy
Copy link
Contributor

idntfy commented Jan 28, 2020

however It works for me perfectly on v4.1.0-37-gd9362d3, will comment it in #362 as well

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 this pull request may close these issues.

None yet

3 participants