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

Security: Use same cookie settings for all cookies #19787

Merged
merged 15 commits into from
Jan 10, 2020
Merged

Security: Use same cookie settings for all cookies #19787

merged 15 commits into from
Jan 10, 2020

Conversation

jeffdesc
Copy link
Contributor

What this PR does / why we need it: For security reasons the Secure flag in the redirect_to cookie should be enforced together when the cookie_secure is enabled.

Which issue(s) this PR fixes: #19744

Fixes #19744

@torkelo torkelo requested a review from marefr October 14, 2019 07:00
marefr
marefr suggested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Reasonable changes, but please see comments.

pkg/api/login.go Outdated Show resolved Hide resolved
pkg/api/login_oauth.go Outdated Show resolved Hide resolved
@jeffdesc jeffdesc requested a review from marefr October 14, 2019 09:49
marefr
marefr suggested changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good. Some minor request for changes, see comments.

pkg/api/login.go Outdated Show resolved Hide resolved
pkg/api/login_oauth.go Outdated Show resolved Hide resolved
@jeffdesc jeffdesc requested a review from marefr October 15, 2019 21:11
torkelo
torkelo reviewed Oct 22, 2019
View reviewed changes
pkg/api/login.go Outdated Show resolved Hide resolved
@papagian papagian added the pr/external This PR is from external contributor label Oct 31, 2019
@marefr
Copy link
Contributor

marefr commented Nov 12, 2019

@jeffdesc are you planning to continue working with the last changes requested in #19787 (review)?

@stale
Copy link

stale bot commented Dec 2, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label Dec 2, 2019
@papagian papagian mentioned this pull request Dec 12, 2019
Merged
@papagian papagian force-pushed the feature/redirect-to-secure branch from c78d6ed to 905f8fb Compare December 13, 2019 12:19
@stale stale bot removed the stale Issue with no recent activity label Dec 13, 2019
@papagian papagian requested review from a team and removed request for a team December 13, 2019 13:16
aknuds1
aknuds1 approved these changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

marefr
marefr suggested changes Dec 13, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Some minor things. See comments.

pkg/middleware/auth.go Outdated Show resolved Hide resolved
pkg/api/login.go Outdated Show resolved Hide resolved
@papagian papagian requested a review from marefr December 13, 2019 16:57
marefr
marefr suggested changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Great. Much better 👍 Some minor things left to address, see comments.

pkg/middleware/middleware.go Outdated Show resolved Hide resolved
pkg/middleware/auth.go Outdated Show resolved Hide resolved
pkg/middleware/auth.go Outdated Show resolved Hide resolved
@aknuds1 aknuds1 requested a review from marefr December 18, 2019 08:01
marefr
marefr suggested changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM. Great work! 👍

Left a comment though about one problem.

pkg/api/login_oauth.go
@@ -82,7 +83,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *m.ReqContext) {

// delete cookie
ctx.Resp.Header().Del("Set-Cookie")
hs.deleteCookie(ctx.Resp, OauthStateCookieName, hs.Cfg.CookieSameSite)
middleware.DeleteCookie(ctx.Resp, OauthStateCookieName, hs.cookieOptionsFromCfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this cookie is never deleted. Maybe an old problem? But think we could resolve that by removing all ctx.Resp.Header().Del("Set-Cookie"). As far as I understand it you can have multiple headers with "Set-Cookie" as long as they don't operate on the same cookie name. Can't find any reference to why ctx.Resp.Header().Del("Set-Cookie") was added in the first throughout the code.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will have a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marefr
I have tested also in the master and the cookie is not deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I still think it should be deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, should I delete also this one then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah

@stale
Copy link

stale bot commented Jan 1, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added stale Issue with no recent activity and removed stale Issue with no recent activity labels Jan 1, 2020
@papagian papagian requested a review from marefr January 10, 2020 11:04
marefr
marefr approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@papagian papagian added this to the 6.6 milestone Jan 10, 2020
@papagian papagian merged commit c5f906f into grafana:master Jan 10, 2020
@marefr marefr changed the title Security: refactor 'redirect_to' cookie to use 'Secure' flag Security: Use same cookie settings for all cookies Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@papagian papagian papagian left review comments

@torkelo torkelo torkelo left review comments

@marefr marefr marefr approved these changes

@aknuds1 aknuds1 aknuds1 approved these changes

Assignees

@papagian papagian

Labels
add to changelog area/backend pr/external This PR is from external contributor type/bug
Projects
None yet
Milestone
6.6.0-beta1
Development

Successfully merging this pull request may close these issues.

'cookie_secure' has no impact on 'redirect_to' cookie
6 participants
@jeffdesc @marefr @torkelo @aknuds1 @papagian @marcusolsson