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

WIP: Support hsts #7423

Closed
wants to merge 3 commits into from
Closed

WIP: Support hsts #7423

wants to merge 3 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jul 11, 2019

As title. Since a gitea service may behind a reverse proxy, we don't know if it's a HTTPS request or a HTTP one. So users could enable it if he knows the service will always use HTTPS.

@techknowlogick Just remember you have implemented the middleware after I wrote the PR. :(

should fix #3788

@lunny lunny added the type/enhancement An improvement of existing functionality label Jul 11, 2019
modules/setting/hsts.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 11, 2019
@lunny lunny force-pushed the lunny/support_hsts branch from db50212 to c0cda33 Compare July 11, 2019 14:46
func createHeaderValueNew(maxAge time.Duration, sendPreloadDirective bool) string {
buf := bytes.NewBufferString("max-age=")
buf.WriteString(strconv.Itoa(int(maxAge.Seconds())))
buf.WriteString("; includeSubDomains")
Copy link
Member

Choose a reason for hiding this comment

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

That means if somebody runs gitea on example.com HSTS policy would apply to *.example.com and their other subdomains will not load in a browser that uses HSTS if they don't have SSL configured. So if they run another service on wiki.example.com that has no SSL it would stop working.

I think we should move this line to ifsendPreloadDirective check since it is required for preload but not required otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

If use preload then it has to meet the requirements for the other values:
https://hstspreload.org
https://wiki.mozilla.org/SecurityEngineering/HTTP_Strict_Transport_Security_(HSTS)_Preload_List

So Firefox bases their list from Google which requires maxAge >= 1 year and includeSubDomains, but then has separate rules to change it after. I think if a user sets SEND_PRELOAD_DIRECTIVE = true then we should force the other settings to match what Google requires and it will work in all browsers then.

Maybe like this:

if sendPreloadDirective {
    if len int(maxAge.Seconds()) < 31536000 {
      maxAge = time.Hour * 24 * 365
    }

    buf := bytes.NewBufferString("max-age=")
    buf.WriteString(strconv.Itoa(int(maxAge.Seconds())))
    buf.WriteString("; includeSubDomains")
    buf.WriteString("; preload")
} else {
    buf := bytes.NewBufferString("max-age=")
    buf.WriteString(strconv.Itoa(int(maxAge.Seconds())))
    if includeSubDomains {
        buf.WriteString("; includeSubDomains")
    }
}

@lunny lunny force-pushed the lunny/support_hsts branch from c0cda33 to e6d772f Compare July 12, 2019 01:55
@codecov-io
Copy link

Codecov Report

Merging #7423 into master will decrease coverage by <.01%.
The diff coverage is 23.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7423      +/-   ##
==========================================
- Coverage   41.19%   41.18%   -0.01%     
==========================================
  Files         469      470       +1     
  Lines       63544    63570      +26     
==========================================
+ Hits        26174    26183       +9     
- Misses      33948    33965      +17     
  Partials     3422     3422
Impacted Files Coverage Δ
routers/routes/routes.go 80.75% <0%> (-1.61%) ⬇️
modules/setting/setting.go 49.13% <100%> (+0.08%) ⬆️
modules/setting/hsts.go 55.55% <55.55%> (ø)
models/repo_list.go 72.08% <0%> (-1.02%) ⬇️
modules/log/event.go 65.64% <0%> (+1.02%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 072bdfa...e6d772f. Read the comment docs.

@silverwind
Copy link
Member

silverwind commented Jul 12, 2019

Instead of adding a configuration (and useless code) for static HTTP headers, would it be instead possible to just let the user configure static HTTP headers in a separate config section using key-value mapping? Something like:

[http.headers]
Strict-Transport-Security = max-age=31536000; includeSubDomains
X-Frame-Options = DENY

@mrsdizzie
Copy link
Member

Instead of adding a configuration (and useless code) for static HTTP headers, would it be instead possible to just let the user configure static HTTP headers in a separate config section using key-value mapping?

I agree, I almost suggested in last response that it would be easier to just let people enter the entire header string rather than try and have different options.

It could default to empty and the app.ini sample / config cheat sheet could list some values people might want to enter (like those above).

@silverwind
Copy link
Member

Thinking about above header format, we could require the keys/header names to be uppercase for consistency with our .ini format and then lowercase them when sending them out (because HTTP headers are case-insensitive).

@sapk sapk mentioned this pull request Aug 13, 2019
@stale
Copy link

stale bot commented Sep 14, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Sep 14, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Sep 15, 2019
@stale stale bot removed the issue/stale label Sep 15, 2019
@silverwind
Copy link
Member

Since a gitea service may behind a reverse proxy, we don't know if it's a HTTPS request or a HTTP one

Not really helping for this PR (I think HSTS should always be a concious user choice) but there's a header for that specific issue.

@lunny lunny changed the title Support hsts WIP: Support hsts Dec 23, 2020
@lunny lunny closed this Nov 4, 2022
@lunny lunny deleted the lunny/support_hsts branch November 4, 2022 09:10
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set HSTS header
6 participants