Automatically set cookie `secure` attribute, remove COOKIE_SECURE option #26953

silverwind · 2023-09-06T22:48:59Z

The idea is to automatically set cookie Secure flag based on proxy headers or the connection, making it unnecessary to configure it by hand via previous COOKIE_SECURE option.

⚠️ BREAKING ⚠️

Gitea now requires a properly configured X-Forwarded-Proto header from reverse proxies, which should be set at the first user-facing reverse proxy and not be changed by any additional proxies in the chain.

silverwind · 2023-09-06T23:13:45Z

routers/common/middleware.go

+			})
+			handler(next).ServeHTTP(resp, req)
+		})
+	}


Not 100% sure this usage is correct, but it appears to work.

The "next" call is right, but the Sessioner is not designed to be used like this. eg: It starts a GC goroutine for every session.Sessioner() call.

The sessioner expects there should be only one instance for all requests.

If this is suboptimal, I think the only other option is to let sessioner accept a func for the Secure attribute as well, or add a autoSecure option. Or look for alternative sessioners that are more flexible.

It's not only suboptimal, it breaks the Sessioner's design and would cause bugs. The Sessioner also has many internal states, it expects the only one instance do "Init" (eg: connect to redis) and do session management (eg: Lock, GC).

Although we could make some changes to make a Sessioner work with dynamic Cookie Secure option, but I think it's too complex to do that, and "dynamic Cookie Secure" doesn't seem really bring enough benefit.

The mentioned "AppURL + COOKIE_SECURE option" should be simple enough and I don't see any problem by doing that.

It's not just cookie secure. If the backend wants for example to reconstruct the original URL a client used (for example for audit log), XFP is required for that as well.

Maybe I'll look into sessioner accepting a function (typecheck with Reflect), that seems the simplest option.

If the backend wants for example to reconstruct the original URL a client used (for example for audit log), XFP is required for that as well.

Just use AppURL, in many cases, the backend doesn't have the HTTP request context.

So, AppURL should always be correct.

Unless the app runs on multiple URLs :)

Unless the app runs on multiple URLs :)

Yup, but the AppURL should also be correct in this case, otherwise the nofication/mail/webhook would stop working.

As long as the AppURL should be always right, then just use it.

Opened https://gitea.com/go-chi/session/issues/7.

silverwind · 2023-09-06T23:14:51Z

Can confirm on a instance behind reverse proxy that all cookies now have the Secure attribute. I do see some duplicate set-cookie headers for session and csrf, need to check whether this is a regression from this PR.

modules/web/middleware/cookie.go

wxiaoguang · 2023-09-07T01:12:27Z

modules/web/middleware/cookie.go

+// GetCookieSecure returns whether the "Secure" attribute on a cookie should be set
+func GetCookieSecure(req *http.Request) bool {
+	forwardedProto := strings.ToLower(req.Header.Get("x-forwarded-proto"))
+	if forwardedProto != "" {
+		return forwardedProto == "https" || forwardedProto == "wss"
+	}
+	return req.TLS != nil
+}


Sorry but it's not right.

Many users might not pass these header when they use Gitea behind a reverse proxy.

Maybe we can just parse setting.AppURL.

Maybe we can just parse setting.AppURL.

Yes. And:

Keep the COOKIE_SECURE option, in case some people really would like to run Gitea on different domains

COOKIE_SECURE should be default to true if AppURL is "https"

Show a frontend warning if the "http" doesn't match COOKIE_SECURE=true, to tell users that "you might not be able to login"

Many users might not pass these header when they use Gitea behind a reverse proxy.

At least for nginx, we have been documenting for a long time to configure that header.

Yes, only "At least". None of others are documented.

And even there is a document, many users do not follow that, they just like to write the config by their experiences.

I would call it out in changelog and be done with it. It's the right way to check if connection is secure imho.

Thought I do see parsing AppURL as the more forgiving option. In an application that does not have such a setting, the current method is the only one that can work, but given that we have AppURL we might as well use it.

What only works with my method is to host the application on multiple domains and/or mixed http/https but I guess that is a seldomly used configuration.

I prefer the solution in comment #26953 (comment) .

According to MDN, the X-Forwarded-Proto is not standard and there are many alternatives (Front-End-Https / X-Forwarded-Protocol / X-Forwarded-Ssl / X-Url-Scheme). IMO using the strong assumption with X-Forwarded-Proto might cause many users unable to login and fail silently. Gitea should be able to be easy to setup and use out-of-box.

My suggestion also works well with "host the application on multiple domains and/or mixed http/https", the site admin could set COOKIE_SECURE=false in their config, keep trusted HTTP requests and redirect untrusted HTTP requests to HTTPS, still no security risk.

So in summary: Keep COOKIE_SECURE but the AppUrl is https, switch it to true? I'm not sure such "magic" is good to do. Maybe I should just scrap the entire PR.

silverwind · 2023-09-08T13:28:02Z

So unless we agree on taking this up as an acceptable change and likely break a few setups with misconfigured reverse proxies, I won't continue here. I still think it's right to require a properly configured X-Forwarded-Proto header from reverse proxies.

silverwind · 2023-09-08T13:30:25Z

Breaking label added.

silverwind · 2023-09-08T13:36:02Z

Actually I think it's non-breaking for the most common setup where Gitea is running on http with a reverse proxy terminating https. The Secure attribute will be absent in such a case, but users will still be able to login, just their cookie could be possibly transmitted over http as well.

wxiaoguang · 2023-09-08T13:45:56Z

Actually I think it's non-breaking for the most common setup where Gitea is running on http with a reverse proxy terminating https. The Secure attribute will be absent in such a case, but users will still be able to login, just their cookie could be possibly transmitted over http as well.

It is ~~breaking~~ (see update below) for all instances which haven't set the "X-Forwarded-Proto" properly. And the Sessioner is another blocker.

Update: I mean, the users who do not have proper "X-Forwarded-Proto" header is under the risk of not having "secure cookie", but they couldn't know it. It leaves a new security problem.

silverwind · 2023-09-08T14:00:50Z

Yes, the sessioner topic needs to be properly fixed first: https://gitea.com/go-chi/session/issues/7.

silverwind · 2023-09-09T14:26:36Z

I lost motivation here, if someone wants to do the AppURL parsing, please raise a separate PR, I'm not really interested in doing that.

wxiaoguang · 2023-09-10T10:58:44Z

-> Use secure cookie for HTTPS sites #26999

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 6, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2023

silverwind marked this pull request as draft September 6, 2023 22:49

silverwind added the type/enhancement An improvement of existing functionality label Sep 6, 2023

silverwind force-pushed the autosecure branch from 35ee675 to c07c129 Compare September 6, 2023 22:57

Automatically set cookie secure attribute, remove COOKIE_SECURE option

c9cd3bf

silverwind force-pushed the autosecure branch from c07c129 to c9cd3bf Compare September 6, 2023 22:59

make it compile

b58bec7

silverwind commented Sep 6, 2023

View reviewed changes

restore previous order

ecebb8f

silverwind commented Sep 6, 2023

View reviewed changes

modules/web/middleware/cookie.go Outdated Show resolved Hide resolved

silverwind added 4 commits September 7, 2023 01:35

fix lint

f302954

Update modules/web/middleware/cookie.go

c625510

also accept wss

ac5dc18

lower

bcea3bd

silverwind marked this pull request as ready for review September 6, 2023 23:40

silverwind marked this pull request as draft September 6, 2023 23:41

wxiaoguang reviewed Sep 7, 2023

View reviewed changes

silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Sep 8, 2023

silverwind closed this Sep 9, 2023

silverwind deleted the autosecure branch September 9, 2023 14:26

wxiaoguang mentioned this pull request Sep 11, 2023

Use secure cookie for HTTPS sites #26999

Merged

GiteaBot added the kind/breaking label Oct 7, 2023

delvh removed the kind/breaking label Oct 7, 2023

GiteaBot added the kind/breaking label Oct 7, 2023

go-gitea locked as resolved and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically set cookie `secure` attribute, remove COOKIE_SECURE option #26953

Automatically set cookie `secure` attribute, remove COOKIE_SECURE option #26953

silverwind commented Sep 6, 2023 •

edited

Loading

silverwind Sep 6, 2023 •

edited

Loading

wxiaoguang Sep 7, 2023 •

edited

Loading

silverwind Sep 8, 2023 •

edited

Loading

wxiaoguang Sep 8, 2023

silverwind Sep 8, 2023

wxiaoguang Sep 8, 2023

silverwind Sep 8, 2023

wxiaoguang Sep 8, 2023

silverwind Sep 8, 2023

silverwind commented Sep 6, 2023

wxiaoguang Sep 7, 2023 •

edited

Loading

lunny Sep 7, 2023

wxiaoguang Sep 7, 2023

silverwind Sep 7, 2023

wxiaoguang Sep 7, 2023

silverwind Sep 7, 2023

silverwind Sep 7, 2023 •

edited

Loading

wxiaoguang Sep 7, 2023 •

edited

Loading

silverwind Sep 7, 2023

silverwind commented Sep 8, 2023 •

edited

Loading

silverwind commented Sep 8, 2023

silverwind commented Sep 8, 2023

wxiaoguang commented Sep 8, 2023 •

edited

Loading

silverwind commented Sep 8, 2023

silverwind commented Sep 9, 2023 •

edited

Loading

wxiaoguang commented Sep 10, 2023

Automatically set cookie secure attribute, remove COOKIE_SECURE option #26953

Automatically set cookie secure attribute, remove COOKIE_SECURE option #26953

Conversation

silverwind commented Sep 6, 2023 • edited Loading

⚠️ BREAKING ⚠️

silverwind Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

silverwind Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Sep 6, 2023

wxiaoguang Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Sep 8, 2023 • edited Loading

silverwind commented Sep 8, 2023

silverwind commented Sep 8, 2023

wxiaoguang commented Sep 8, 2023 • edited Loading

silverwind commented Sep 8, 2023

silverwind commented Sep 9, 2023 • edited Loading

wxiaoguang commented Sep 10, 2023

Automatically set cookie `secure` attribute, remove COOKIE_SECURE option #26953

Automatically set cookie `secure` attribute, remove COOKIE_SECURE option #26953

silverwind commented Sep 6, 2023 •

edited

Loading

silverwind Sep 6, 2023 •

edited

Loading

wxiaoguang Sep 7, 2023 •

edited

Loading

silverwind Sep 8, 2023 •

edited

Loading

wxiaoguang Sep 7, 2023 •

edited

Loading

silverwind Sep 7, 2023 •

edited

Loading

wxiaoguang Sep 7, 2023 •

edited

Loading

silverwind commented Sep 8, 2023 •

edited

Loading

wxiaoguang commented Sep 8, 2023 •

edited

Loading

silverwind commented Sep 9, 2023 •

edited

Loading