feat: Add strict transport security and secure cookie options #741

f0ssel · 2022-03-30T19:47:50Z

No description provided.

codecov · 2022-03-30T19:49:57Z

Codecov Report

Merging #741 (49f2e75) into main (6ab1a68) will decrease coverage by 0.37%.
The diff coverage is 100.00%.

@@            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

Flag	Coverage Δ
unittest-go-	`63.03% <100.00%> (-0.61%)`	⬇️
unittest-go-macos-latest	`58.98% <100.00%> (-0.05%)`	⬇️
unittest-go-ubuntu-latest	`61.79% <100.00%> (-0.21%)`	⬇️
unittest-go-windows-2022	`58.07% <100.00%> (-0.23%)`	⬇️
unittest-js	`62.63% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/coderd.go	`96.47% <ø> (ø)`
cli/start.go	`66.00% <100.00%> (-0.16%)`	⬇️
coderd/users.go	`57.41% <100.00%> (+0.05%)`	⬆️
coderd/database/db.go	`55.17% <0.00%> (-13.80%)`	⬇️
coderd/provisionerdaemons.go	`59.52% <0.00%> (-4.56%)`	⬇️
coderd/parameter/compute.go	`74.07% <0.00%> (-4.45%)`	⬇️
cli/ssh.go	`38.88% <0.00%> (-4.45%)`	⬇️
agent/conn.go	`65.38% <0.00%> (-3.59%)`	⬇️
agent/agent.go	`67.30% <0.00%> (-1.09%)`	⬇️
peer/conn.go	`75.38% <0.00%> (-1.02%)`	⬇️
... and 15 more

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 6ab1a68...49f2e75. Read the comment docs.

Emyrk · 2022-03-30T20:08:53Z

coderd/httpmw/hsts.go

+	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)))


The time should be in seconds. Golang times are int64 in nanoseconds

Should we make the value of this header configurable by the flag? With "true" defaulting to our max-age? Seems like there's a swath of options that we could set, but might not have the best idea when it's appropriate.

Idk, HSTS should be applied to a long time. Like 1-2 years. Anything shorter is strange. Idk why you'd set it short.

I'd just stick with on/off

There's an option includeSubdomains 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.

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 to CODER_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;"

I think your argument is fair. It's weird to offer something nobody is asking for, and this is easy to change later too!

coderd/httpmw/hsts.go

Emyrk · 2022-03-30T20:09:36Z

coderd/httpmw/hsts_test.go

+		t.Parallel()
+
+		res := setup(true)
+		require.Contains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge)))


I'd rather us hard code the seconds integer, given that this value is likely massively too high atm.

Emyrk · 2022-03-30T20:10:47Z

coderd/httpmw/hsts_test.go

+		t.Parallel()
+
+		res := setup(false)
+		require.NotContains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge)))


The false case should probably check it contains the empty string ""

kylecarbs · 2022-03-30T21:17:21Z

coderd/httpmw/hsts.go

+	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)))


Should we make the value of this header configurable by the flag? With "true" defaulting to our max-age? Seems like there's a swath of options that we could set, but might not have the best idea when it's appropriate.

kylecarbs · 2022-03-30T21:18:18Z

cli/start.go

@@ -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")


HSTS and struct-transport-security confuse me a bit. With my other comment, could we call this:

CODER_STRICT_TRANSPORT_SECURITY

I had this originally and thought it may be too long and changed it lol.

kylecarbs · 2022-03-30T21:18:58Z

cli/start.go

@@ -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")


Our description would be more helpful if it linked to MDN or something. As a user, I'd just Google that right away!

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.

I personally kind of like just letting people google the header? It's super easy to grok

In that case, I'd adjust these to use " like other parameter help sections do for consistency. I wouldn't call this Set either. In other commands we use Specifies, which I think more accurately describes it (because it's a bool).

SecureCookie might be worth explicitly stating SecureAuthCookie instead. That way there's no ambiguity.

Emyrk · 2022-03-30T22:05:05Z

coderd/httpmw/hsts_test.go

+
+	"github.com/coder/coder/coderd/httpmw"


close this import gap

f0ssel · 2022-03-30T22:22:21Z

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.

Emyrk · 2022-03-30T22:38:34Z

coderd/httpmw/hsts_test.go

+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/coderd/httpmw"


Wait still need to close this gap right? Github imports have no newline gaps?

Nah, they do for internal imports.

kylecarbs

Make sure to rename the hsts*.go files before merge. I added another comment about renaming secureCookie to secureAuthCookie instead, which I think makes it a bit more explicit.

I don't think we should export objects just for testing. Apart from that, looks good to me!

kylecarbs · 2022-03-30T22:47:31Z

coderd/httpmw/hsts.go

+
+const (
+	StrictTransportSecurityHeader = "Strict-Transport-Security"
+	StrictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year


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.

We have a lint rule that requires that tests use httmw_test package which then forces you to export. I'll figure out how to disable that linter?

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.

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.

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.

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 an option 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.

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.

Ah we can do XXX_internal_test.go. So I'm good with the linter if we have the option to go around it 👍.

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.

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.

kylecarbs · 2022-03-31T14:23:24Z

coderd/httpmw/stricttransportsecurity.go

+const (
+	strictTransportSecurityHeader = "Strict-Transport-Security"
+	strictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year
+)
+


Since these are just used in the one place, I don't think they should be constants.

cmon https://en.wikipedia.org/wiki/Magic_number_(programming)

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.

I did it anyways to get this pr merged....

We can leave em' as consts that's just a minor nit.

kylecarbs · 2022-03-31T14:43:16Z

coderd/httpmw/stricttransportsecurity_test.go

+const (
+	strictTransportSecurityHeader = "Strict-Transport-Security"
+	strictTransportSecurityMaxAge = time.Hour * 24 * 365
+)


These should probably be in the test function scope. If they aren't used globally between tests, it could be confusing.

kylecarbs

LGTM! One comment is just a question, not a change.

kylecarbs · 2022-03-31T15:43:17Z

coderd/coderd.go

-		r.Use(chitrace.Middleware())
+		r.Use(
+			chitrace.Middleware(),
+			httpmw.StrictTransportSecurity(api.StrictTransportSecurity),


Is this needed for static assets as well?

@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.

HSTS should 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

We should probably move it outside of there then, just to confirm properly.

kylecarbs · 2022-03-31T15:49:01Z

We should update the title too- since we renamed HSTS to StrictTransportSecurity.

A discussion (linked below) was had that touched on why this linter is enabled. To avoid losing that history, adding the comment inline with our linting rules can avoid duplicating this discussion! #741 (comment)

ammario · 2022-03-31T17:00:45Z

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.

ammario

(See my comment)

f0ssel · 2022-03-31T17:07:13Z

@ammario can I link this comment in 6 months when an enterprise customer / our PM asks for this again like they did in v1?

ammario · 2022-03-31T17:17:55Z

@f0ssel of course :)

deleted the code

A discussion (linked below) was had that touched on why this linter is enabled. To avoid losing that history, adding the comment inline with our linting rules can avoid duplicating this discussion! #741 (comment)

feat: Add HSTS and secure cookie options

d6c404c

f0ssel requested review from kylecarbs and Emyrk March 30, 2022 19:47

Emyrk previously requested changes Mar 30, 2022

View reviewed changes

Fix tests

c6534f6

kylecarbs requested changes Mar 30, 2022

View reviewed changes

Emyrk reviewed Mar 30, 2022

View reviewed changes

coderd/httpmw/hsts_test.go Outdated

Comment on lines 11 to 12

"github.com/coder/coder/coderd/httpmw"

Copy link

Member

Emyrk Mar 30, 2022

Choose a reason for hiding this comment

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

close this import gap

pr comments

197b163

f0ssel requested review from Emyrk and kylecarbs March 30, 2022 22:24

f0ssel force-pushed the f0ssel/security branch 3 times, most recently from b7eda3e to 803313d Compare March 30, 2022 22:31

Emyrk reviewed Mar 30, 2022

View reviewed changes

kylecarbs approved these changes Mar 30, 2022

View reviewed changes

f0ssel force-pushed the f0ssel/security branch 2 times, most recently from 96bcf77 to 368ead5 Compare March 31, 2022 02:13

pr comments

c874f35

f0ssel force-pushed the f0ssel/security branch from 368ead5 to c874f35 Compare March 31, 2022 02:14

kylecarbs requested changes Mar 31, 2022

View reviewed changes

pr comments

fccf4bb

f0ssel force-pushed the f0ssel/security branch from 05993dc to fccf4bb Compare March 31, 2022 15:15

f0ssel requested a review from kylecarbs March 31, 2022 15:17

kylecarbs approved these changes Mar 31, 2022

View reviewed changes

f0ssel changed the title ~~feat: Add HSTS and secure cookie options~~ feat: Add strict transport security and secure cookie options Mar 31, 2022

kylecarbs mentioned this pull request Mar 31, 2022

chore: Add comment explaining why testpackage is enabled #774

Merged

ammario previously requested changes Mar 31, 2022

View reviewed changes

rip out hsts

49f2e75

f0ssel requested review from Emyrk, kylecarbs and ammario March 31, 2022 17:16

f0ssel merged commit 0d53795 into main Mar 31, 2022

f0ssel deleted the f0ssel/security branch March 31, 2022 17:31

misskniss added the V2 BETA label May 15, 2022

misskniss added this to the V2 Beta milestone May 15, 2022

		"github.com/stretchr/testify/require"

		"github.com/coder/coder/coderd/httpmw"

feat: Add strict transport security and secure cookie options #741

feat: Add strict transport security and secure cookie options #741

Uh oh!

Conversation

f0ssel commented Mar 30, 2022

Uh oh!

codecov bot commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f0ssel commented Mar 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f0ssel Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f0ssel Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

codecov bot commented Mar 30, 2022 •

edited

Loading

f0ssel Mar 31, 2022 •

edited

Loading

f0ssel Mar 31, 2022 •

edited

Loading

kylecarbs Mar 31, 2022 •

edited

Loading