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

Add support for setting SameSite and Secure attribute for auth cookie we set #5248

Merged
merged 21 commits into from
Mar 27, 2022
Merged

Add support for setting SameSite and Secure attribute for auth cookie we set #5248

merged 21 commits into from
Mar 27, 2022

Conversation

Kami
Copy link
Member

@Kami Kami commented Apr 22, 2021 •
edited

This pull request includes a small "security hardening" change.

It allows operator to configure value for SameSite attribute (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite, https://web.dev/samesite-cookies-explained/) which is set with the auth-token cookie we set in some situations (e.g. when authenticating via st2web and similar).

The value defaults to Lax which should work as a good secure default (defining it to Strict may break some in some situations, see the link above).

TODO

@Kami
Add support for setting custom value for SameSite attribute for
44d4381 
"auth-token" cookie we set when authentication against st2api from
st2web.

For backward compatibility reasons it defaults to none.
@Kami Kami added this to the 3.5.0 milestone Apr 22, 2021
@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Apr 22, 2021
@Kami Kami changed the title Add support for setting SameSite attribute for auth cookie we set Add support for setting SameSite and Secure attribute for auth cookie we set Apr 23, 2021
@Kami
Copy link
Member Author

Kami commented Apr 23, 2021

I noticed we also don't set secure flag for the cookie.

I will also add an option for that and default it to True since it's a best security practice.

In case someone doesn't run StackStorm over https (bad idea), they will need to set it to False. I will open st2docs upgrade notes entry which documents how to do that.

Also keep in mind that this cookie is pretty much only used when logging via token / api key in query parameters (which pretty much only means st2web for our official stuff).

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Apr 23, 2021
@Kami
Use better options name, also allow user to set "secure" value for auth
70d32b5 
cookie we set and default it to True for security reasons.

Also default SameSite attribute to Lax.
@Kami
Revert "Fix lint."
f8369bb 
This reverts commit dde0617.
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Apr 23, 2021
@Kami Kami mentioned this pull request Apr 24, 2021
Merged
@amanda11 amanda11 added this to In progress in StackStorm v3.5.0 via automation Jun 14, 2021
@amanda11 amanda11 removed this from In progress in StackStorm v3.5.0 Jun 14, 2021
@amanda11 amanda11 added this to To Do in StackStorm v3.6.0 via automation Jun 14, 2021
@amanda11 amanda11 modified the milestones: 3.5.0, 3.6.0 Jun 14, 2021
@amanda11 amanda11 moved this from To Do to In Progress in StackStorm v3.6.0 Jul 1, 2021
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

cognifloyd
cognifloyd reviewed Oct 2, 2021
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment •
edited

Choose a reason for hiding this comment

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

Ooh. nice.
I have a doc wording suggestion committed, and a question about none vs None.

conf/st2.conf.sample Outdated Show resolved Hide resolved
st2api/st2api/validation.py Outdated Show resolved Hide resolved
st2common/st2common/config.py Outdated Show resolved Hide resolved
@arm4b arm4b removed this from In Progress in StackStorm v3.6.0 Oct 5, 2021
@arm4b arm4b removed this from the 3.6.0 milestone Oct 5, 2021
@Kami
Copy link
Member Author

Kami commented Nov 11, 2021

@cognifloyd I pushed a change which renames None to unset (which I also think it's better).

Hopefully CI and tests will pass since my local dev environment is totally toast and I don't have multiple hours to spend to try to fix it at this point.

@Kami Kami modified the milestones: 3.6.0, 3.7.0 Nov 11, 2021
cognifloyd
cognifloyd approved these changes Nov 11, 2021
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Awesome. I'm not merging right now since we're in code freeze for the 3.6 release.

@Kami
Copy link
Member Author

Kami commented Mar 27, 2022

Now that v3.6.0 merge freeze is over, I will go ahead and merge it into master.

@Kami Kami merged commit 56005db into master Mar 27, 2022
@Kami Kami deleted the samesite_cookie_support branch March 27, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cognifloyd cognifloyd cognifloyd approved these changes

Assignees
No one assigned
Labels
security service: api size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Kami @CLAassistant @cognifloyd @arm4b @amanda11