- Notifications
You must be signed in to change notification settings - Fork928
feat: Add strict transport security and secure cookie options#741
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMar 30, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #741 +/- ##==========================================- Coverage 64.31% 63.93% -0.38%========================================== Files 198 199 +1 Lines 11635 11830 +195 Branches 87 87 ==========================================+ Hits 7483 7564 +81- Misses 3351 3441 +90- Partials 801 825 +24
Continue to review full report at Codecov.
|
coderd/httpmw/hsts.go Outdated
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if hsts { | ||
| w.Header().Set(HSTSHeader, fmt.Sprintf("max-age=%d", int64(HSTSMaxAge))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The time should be in seconds. Golang times are int64 innanoseconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we make the value of this header configurable by the flag? With "true" defaulting to ourmax-age? Seems like there's a swath of options that wecould set, but might not have the best idea when it's appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Idk, HSTSshould be applied to a long time. Like 1-2 years. Anything shorter is strange. Idk why you'd set it short.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd just stick withon/off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There's an optionincludeSubdomains you can add too. What happens when a customer asks for it? Unless we're confident they won't, it's really simple for us to add now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think adding things that aren't required is a big part of what helped get v1 where it is today. No one asking for this is a bet I would take 10/10 times right now.
I could see an argument for changing this toCODER_STRICT_SECURITY_HEADER with the default value"" in which we don't set it, and otherwise they provide the value for the header - like"max-age=63072000;"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think your argument is fair. It's weird to offer something nobody is asking for, and this is easy to change later too!
Uh oh!
There was an error while loading.Please reload this page.
coderd/httpmw/hsts_test.go Outdated
| t.Parallel() | ||
| res := setup(true) | ||
| require.Contains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd rather us hard code the seconds integer, given that this value is likely massively too high atm.
coderd/httpmw/hsts_test.go Outdated
| t.Parallel() | ||
| res := setup(false) | ||
| require.NotContains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The false case should probably check it contains the empty string ""
coderd/httpmw/hsts.go Outdated
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if hsts { | ||
| w.Header().Set(HSTSHeader, fmt.Sprintf("max-age=%d", int64(HSTSMaxAge))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we make the value of this header configurable by the flag? With "true" defaulting to ourmax-age? Seems like there's a swath of options that wecould set, but might not have the best idea when it's appropriate.
cli/start.go Outdated
| @@ -334,6 +338,8 @@ func start() *cobra.Command { | |||
| cliflag.BoolVarP(root.Flags(), &useTunnel, "tunnel", "", "CODER_DEV_TUNNEL", true, "Serve dev mode through a Cloudflare Tunnel for easy setup") | |||
| _ = root.Flags().MarkHidden("tunnel") | |||
| cliflag.BoolVarP(root.Flags(), &traceDatadog, "trace-datadog", "", "CODER_TRACE_DATADOG", false, "Send tracing data to a datadog agent") | |||
| cliflag.BoolVarP(root.Flags(), &hsts, "hsts", "", "CODER_HSTS", false, "Set the 'strict-transport-security' header on http responses") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
HSTS andstruct-transport-security confuse me a bit. With my other comment, could we call this:
CODER_STRICT_TRANSPORT_SECURITYThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had this originally and thought it may be too long and changed it lol.
cli/start.go Outdated
| @@ -334,6 +338,8 @@ func start() *cobra.Command { | |||
| cliflag.BoolVarP(root.Flags(), &useTunnel, "tunnel", "", "CODER_DEV_TUNNEL", true, "Serve dev mode through a Cloudflare Tunnel for easy setup") | |||
| _ = root.Flags().MarkHidden("tunnel") | |||
| cliflag.BoolVarP(root.Flags(), &traceDatadog, "trace-datadog", "", "CODER_TRACE_DATADOG", false, "Send tracing data to a datadog agent") | |||
| cliflag.BoolVarP(root.Flags(), &hsts, "hsts", "", "CODER_HSTS", false, "Set the 'strict-transport-security' header on http responses") | |||
| cliflag.BoolVarP(root.Flags(), &secureCookie, "secure-cookie", "", "CODER_SECURE_COOKIE", false, "Set the 'Secure' property on browser session cookies") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Our description would be more helpful if it linked to MDN or something. As a user, I'd just Google that right away!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We do in the dash but I wanted to keep these help docs slim. But I'm down to add it if we don't mind the verbosity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I personally kind of like just letting people google the header? It's super easy to grok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In that case, I'd adjust these to use" like other parameter help sections do for consistency. I wouldn't call thisSet either. In other commands we useSpecifies, which I think more accurately describes it (because it's a bool).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SecureCookie might be worth explicitly statingSecureAuthCookie instead. That way there's no ambiguity.
coderd/httpmw/hsts_test.go Outdated
| "github.com/coder/coder/coderd/httpmw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
close this import gap
Updated with the feedback. I think we should keep max-age unconfigurable for now, it's really easy to add it later as a envflag and I'm not convinced anyone will ever ask for it. |
b7eda3e to803313dComparecoderd/httpmw/hsts_test.go Outdated
| "github.com/stretchr/testify/require" | ||
| "github.com/coder/coder/coderd/httpmw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wait still need to close this gap right? Github imports have no newline gaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nah, they do forinternal imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Make sure to rename thehsts*.go files before merge. I added another comment about renamingsecureCookie tosecureAuthCookie instead, which I think makes it a bit more explicit.
I don't think we should export objectsjust for testing. Apart from that, looks good to me!
coderd/httpmw/hsts.go Outdated
| const ( | ||
| StrictTransportSecurityHeader = "Strict-Transport-Security" | ||
| StrictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If we just check for the existence of the header, we can remove these as exports.
Exporting just for tests is bad practice from my perspective. If a customer were to depend on these constant values and they changed randomly, it would break their stuff. Tests should run against the API just like a customer would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We have a lint rule that requires that tests usehttmw_test package which then forces you to export. I'll figure out how to disable that linter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should disable that lint rule, as it prevents us from creating good package scopes.
I think it's totally reasonable to test unexported vars/consts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we should disable that linter, I think we should duplicate the values!
Customers testing against our APIs likely won't use our constants, they'll probably use the values.
I intentionally enabled that linter to avoid package bloat and poor decomposition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://github.com/maratori/testpackage
I recommend you read the attached posts (there are like four of them) describing its intention. If you still disagree, I'm happy to reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I personally see the value, and get the philosophy forcing tests to use the public api. Also I'm behind the duplicate values thing too, I considered that solution as well.
I do however think when it comes to testing I'd rather have both public and private functional testing as anoption if we want it. I see value in the forcing function but think it may be too heavy handed to say "no private function testing".
I'll dupe the values for this PR and keep the lint rule for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the "export_test" example (the 4th link) is a pretty bad pattern. I wouldn't want to have a test file dedicated to bridging the gap and avoid the linter, seems super confusing and fighting the toolchain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah we can doXXX_internal_test.go. So I'm good with the linter if we have the option to go around it 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the "export_test" example (the 4th link) is a pretty bad pattern. I wouldn't want to have a test file dedicated to bridging the gap and avoid the linter, seems super confusing and fighting the toolchain.
Yea, I was not a fan of that either. I think with the_internal_ option for naming though, we can still write internal tests. I am fine with that.
kylecarbsMar 31, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Exactly! There are of course edge-cases, but the linting rule optimizes for 95% of cases. In the 5% where it doesn't fit, the_internal_ pattern provides well.
96bcf77 to368ead5Compare| const ( | ||
| strictTransportSecurityHeader = "Strict-Transport-Security" | ||
| strictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since these are just used in the one place, I don't think they should be constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
consts are more self documenting (variable name) and cleaner code to read imo. I feel like I was told never to use magic numbers like day 1 of programming and have never looked back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I did it anyways to get this pr merged....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We can leave em' as consts that's just a minor nit.
| const ( | ||
| strictTransportSecurityHeader = "Strict-Transport-Security" | ||
| strictTransportSecurityMaxAge = time.Hour * 24 * 365 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These should probably be in the test function scope. If they aren't used globally between tests, it could be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM! One comment is just a question, not a change.
coderd/coderd.go Outdated
| r.Use(chitrace.Middleware()) | ||
| r.Use( | ||
| chitrace.Middleware(), | ||
| httpmw.StrictTransportSecurity(api.StrictTransportSecurity), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this needed for static assets as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Emyrk ? I don't know but my guess is "no" because it will already be set on the browser after the first api call and also static assets would never have sensitive data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
HSTSshould be on static, but I think it might not really matter as you only need to hit 1 HSTS header for it "take effect". Every HSTS header after is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should probably move it outside of there then, just to confirm properly.
We should update the title too- since we renamed |
A discussion (linked below) was had that touched on why this linteris enabled. To avoid losing that history, adding the comment inline withour linting rules can avoid duplicating this discussion!#741 (comment)
HSTS should be configured by a load balancer or proxy, not the end application. It's a niche requirement and readily configurable by almost every reverse proxy. Not even GitLab has built in HSTS. Instead, they suggest users configure nginx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(See my comment)
@ammario can I link this comment in 6 months when an enterprise customer / our PM asks for this again like they did in v1? |
@f0ssel of course :) |
A discussion (linked below) was had that touched on why this linteris enabled. To avoid losing that history, adding the comment inline withour linting rules can avoid duplicating this discussion!#741 (comment)
No description provided.