From d6c404cf90b0308d08e9282a6150570ef5ac4f2f Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 30 Mar 2022 19:46:25 +0000 Subject: [PATCH 1/6] feat: Add HSTS and secure cookie options --- cli/start.go | 6 ++++++ coderd/coderd.go | 8 ++++++- coderd/httpmw/hsts.go | 33 +++++++++++++++++++++++++++++ coderd/httpmw/hsts_test.go | 43 ++++++++++++++++++++++++++++++++++++++ coderd/users.go | 1 + 5 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 coderd/httpmw/hsts.go create mode 100644 coderd/httpmw/hsts_test.go diff --git a/cli/start.go b/cli/start.go index 1c89bf8e20b7a..4f5716c807a95 100644 --- a/cli/start.go +++ b/cli/start.go @@ -56,6 +56,8 @@ func start() *cobra.Command { tlsMinVersion string useTunnel bool traceDatadog bool + hsts bool + secureCookie bool ) root := &cobra.Command{ Use: "start", @@ -132,6 +134,8 @@ func start() *cobra.Command { Database: databasefake.New(), Pubsub: database.NewPubsubInMemory(), GoogleTokenValidator: validator, + HSTS: hsts, + SecureCookie: secureCookie, } if !dev { @@ -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") return root } diff --git a/coderd/coderd.go b/coderd/coderd.go index e8836eeef7232..59a8daa5e509c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -29,6 +29,9 @@ type Options struct { AWSCertificates awsidentity.Certificates GoogleTokenValidator *idtoken.Validator + + HSTS bool + SecureCookie bool } // New constructs the Coder API into an HTTP handler. @@ -45,7 +48,10 @@ func New(options *Options) (http.Handler, func()) { r := chi.NewRouter() r.Route("/api/v2", func(r chi.Router) { - r.Use(chitrace.Middleware()) + r.Use( + chitrace.Middleware(), + httpmw.HSTS(api.HSTS), + ) r.Get("/", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(w, http.StatusOK, httpapi.Response{ Message: "👋", diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go new file mode 100644 index 0000000000000..5dbc88c8bc185 --- /dev/null +++ b/coderd/httpmw/hsts.go @@ -0,0 +1,33 @@ +package httpmw + +import ( + "fmt" + "net/http" + "time" +) + +const ( + HSTSHeader = "Strict-Transport-Security" + HSTSMaxAge = time.Hour * 24 * 365 // 1 year +) + +// HSTS will add the strict-transport-security header if enabled. +// This header forces a browser to always use https for the domain after it loads https +// once. +// Meaning: On first load of product.coder.com, they are redirected to https. +// On all subsequent loads, the client's local browser forces https. This prevents man in the middle. +// +// This header only makes sense if the app is using tls. +// Full header example +// Strict-Transport-Security: max-age=63072000; includeSubDomains; preload +func HSTS(hsts bool) func(next http.Handler) http.Handler { + 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))) + } + + next.ServeHTTP(w, r) + }) + } +} diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go new file mode 100644 index 0000000000000..8cca191e8b7b6 --- /dev/null +++ b/coderd/httpmw/hsts_test.go @@ -0,0 +1,43 @@ +package httpmw_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/coder/coder/coderd/httpmw" + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/require" +) + +func TestHSTS(t *testing.T) { + t.Parallel() + setup := func(hsts bool) *http.Response { + rw := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/", nil) + + rtr := chi.NewRouter() + rtr.Use(httpmw.HSTS(hsts)) + rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("hello!")) + }) + rtr.ServeHTTP(rw, r) + res := rw.Result() + defer res.Body.Close() + return res + } + + t.Run("True", func(t *testing.T) { + t.Parallel() + + res := setup(true) + require.Contains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) + }) + t.Run("False", func(t *testing.T) { + t.Parallel() + + res := setup(false) + require.NotContains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) + }) +} diff --git a/coderd/users.go b/coderd/users.go index e5db601ad2828..a3b72002ca68a 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -417,6 +417,7 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) { Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, + Secure: api.SecureCookie, }) render.Status(r, http.StatusCreated) From c6534f6e9291fd3f70a6e4945fda55b340774c0a Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 30 Mar 2022 20:12:32 +0000 Subject: [PATCH 2/6] Fix tests --- coderd/httpmw/hsts.go | 5 +++-- coderd/httpmw/hsts_test.go | 14 ++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go index 5dbc88c8bc185..2b7f6a77aa892 100644 --- a/coderd/httpmw/hsts.go +++ b/coderd/httpmw/hsts.go @@ -20,10 +20,11 @@ const ( // This header only makes sense if the app is using tls. // Full header example // Strict-Transport-Security: max-age=63072000; includeSubDomains; preload -func HSTS(hsts bool) func(next http.Handler) http.Handler { +// nolint:revive +func HSTS(enable bool) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if hsts { + if enable { w.Header().Set(HSTSHeader, fmt.Sprintf("max-age=%d", int64(HSTSMaxAge))) } diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go index 8cca191e8b7b6..32ea0fe706cd1 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/hsts_test.go @@ -6,38 +6,40 @@ import ( "net/http/httptest" "testing" - "github.com/coder/coder/coderd/httpmw" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpmw" ) func TestHSTS(t *testing.T) { t.Parallel() - setup := func(hsts bool) *http.Response { + + setup := func(enable bool) *http.Response { rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/", nil) rtr := chi.NewRouter() - rtr.Use(httpmw.HSTS(hsts)) + rtr.Use(httpmw.HSTS(enable)) rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("hello!")) }) rtr.ServeHTTP(rw, r) - res := rw.Result() - defer res.Body.Close() - return res + return rw.Result() } t.Run("True", func(t *testing.T) { t.Parallel() res := setup(true) + defer res.Body.Close() require.Contains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) }) t.Run("False", func(t *testing.T) { t.Parallel() res := setup(false) + defer res.Body.Close() require.NotContains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) }) } From 197b163365578cb1e9950b5759407e20f16cbd9e Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 30 Mar 2022 22:21:25 +0000 Subject: [PATCH 3/6] pr comments --- cli/start.go | 26 +++++++++++++------------- coderd/httpmw/hsts.go | 12 ++++++------ coderd/httpmw/hsts_test.go | 12 ++++++------ 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/cli/start.go b/cli/start.go index 4f5716c807a95..128d5fa1b6154 100644 --- a/cli/start.go +++ b/cli/start.go @@ -47,17 +47,17 @@ func start() *cobra.Command { dev bool postgresURL string // provisionerDaemonCount is a uint8 to ensure a number > 0. - provisionerDaemonCount uint8 - tlsCertFile string - tlsClientCAFile string - tlsClientAuth string - tlsEnable bool - tlsKeyFile string - tlsMinVersion string - useTunnel bool - traceDatadog bool - hsts bool - secureCookie bool + provisionerDaemonCount uint8 + tlsCertFile string + tlsClientCAFile string + tlsClientAuth string + tlsEnable bool + tlsKeyFile string + tlsMinVersion string + useTunnel bool + traceDatadog bool + strictTransportSecurity bool + secureCookie bool ) root := &cobra.Command{ Use: "start", @@ -134,7 +134,7 @@ func start() *cobra.Command { Database: databasefake.New(), Pubsub: database.NewPubsubInMemory(), GoogleTokenValidator: validator, - HSTS: hsts, + HSTS: strictTransportSecurity, SecureCookie: secureCookie, } @@ -338,7 +338,7 @@ 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(), &strictTransportSecurity, "strict-transport-security", "", "CODER_STRICT_TRANSPORT_SECURITY", 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") return root diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go index 2b7f6a77aa892..07b5d7c0463a1 100644 --- a/coderd/httpmw/hsts.go +++ b/coderd/httpmw/hsts.go @@ -7,11 +7,11 @@ import ( ) const ( - HSTSHeader = "Strict-Transport-Security" - HSTSMaxAge = time.Hour * 24 * 365 // 1 year + StrictTransportSecurityHeader = "Strict-Transport-Security" + StrictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year ) -// HSTS will add the strict-transport-security header if enabled. +// StrictTransportSecurity will add the strict-transport-security header if enabled. // This header forces a browser to always use https for the domain after it loads https // once. // Meaning: On first load of product.coder.com, they are redirected to https. @@ -19,13 +19,13 @@ const ( // // This header only makes sense if the app is using tls. // Full header example -// Strict-Transport-Security: max-age=63072000; includeSubDomains; preload +// Strict-Transport-Security: max-age=63072000; // nolint:revive -func HSTS(enable bool) func(next http.Handler) http.Handler { +func StrictTransportSecurity(enable bool) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if enable { - w.Header().Set(HSTSHeader, fmt.Sprintf("max-age=%d", int64(HSTSMaxAge))) + w.Header().Set(StrictTransportSecurityHeader, fmt.Sprintf("max-age=%d", int64(StrictTransportSecurityMaxAge.Seconds()))) } next.ServeHTTP(w, r) diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go index 32ea0fe706cd1..3a9905b2a3552 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/hsts_test.go @@ -6,13 +6,12 @@ import ( "net/http/httptest" "testing" + "github.com/coder/coder/coderd/httpmw" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/httpmw" ) -func TestHSTS(t *testing.T) { +func TestStrictTransportSecurity(t *testing.T) { t.Parallel() setup := func(enable bool) *http.Response { @@ -20,7 +19,7 @@ func TestHSTS(t *testing.T) { r := httptest.NewRequest("GET", "/", nil) rtr := chi.NewRouter() - rtr.Use(httpmw.HSTS(enable)) + rtr.Use(httpmw.StrictTransportSecurity(enable)) rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("hello!")) }) @@ -33,13 +32,14 @@ func TestHSTS(t *testing.T) { res := setup(true) defer res.Body.Close() - require.Contains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) + require.Contains(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(httpmw.StrictTransportSecurityMaxAge))) }) t.Run("False", func(t *testing.T) { t.Parallel() res := setup(false) defer res.Body.Close() - require.NotContains(t, res.Header.Get(httpmw.HSTSHeader), fmt.Sprintf("max-age=%d", int64(httpmw.HSTSMaxAge))) + require.NotContains(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(httpmw.StrictTransportSecurityMaxAge))) + require.Equal(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), "") }) } From c874f3534ae0f54f37f7e77daece51de396e5776 Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 30 Mar 2022 22:23:31 +0000 Subject: [PATCH 4/6] pr comments --- cli/start.go | 20 +++++++++---------- coderd/coderd.go | 6 +++--- .../{hsts.go => stricttransportsecurity.go} | 6 +++--- ...est.go => stricttransportsecurity_test.go} | 15 ++++++++++---- 4 files changed, 27 insertions(+), 20 deletions(-) rename coderd/httpmw/{hsts.go => stricttransportsecurity.go} (76%) rename coderd/httpmw/{hsts_test.go => stricttransportsecurity_test.go} (62%) diff --git a/cli/start.go b/cli/start.go index 128d5fa1b6154..7a1e0f4e1e685 100644 --- a/cli/start.go +++ b/cli/start.go @@ -57,7 +57,7 @@ func start() *cobra.Command { useTunnel bool traceDatadog bool strictTransportSecurity bool - secureCookie bool + secureAuthCookie bool ) root := &cobra.Command{ Use: "start", @@ -129,13 +129,13 @@ func start() *cobra.Command { } logger := slog.Make(sloghuman.Sink(os.Stderr)) options := &coderd.Options{ - AccessURL: accessURLParsed, - Logger: logger.Named("coderd"), - Database: databasefake.New(), - Pubsub: database.NewPubsubInMemory(), - GoogleTokenValidator: validator, - HSTS: strictTransportSecurity, - SecureCookie: secureCookie, + AccessURL: accessURLParsed, + Logger: logger.Named("coderd"), + Database: databasefake.New(), + Pubsub: database.NewPubsubInMemory(), + GoogleTokenValidator: validator, + StrictTransportSecurity: strictTransportSecurity, + SecureAuthCookie: secureAuthCookie, } if !dev { @@ -338,8 +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(), &strictTransportSecurity, "strict-transport-security", "", "CODER_STRICT_TRANSPORT_SECURITY", 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") + cliflag.BoolVarP(root.Flags(), &strictTransportSecurity, "strict-transport-security", "", "CODER_STRICT_TRANSPORT_SECURITY", false, `Specifies if the "strict-transport-security" header is set on http responses`) + cliflag.BoolVarP(root.Flags(), &secureAuthCookie, "secure-auth-cookie", "", "CODER_SECURE_AUTH_COOKIE", false, "Specifies if the 'Secure' property is set on browser session cookies") return root } diff --git a/coderd/coderd.go b/coderd/coderd.go index 59a8daa5e509c..aee79f4708ced 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -30,8 +30,8 @@ type Options struct { AWSCertificates awsidentity.Certificates GoogleTokenValidator *idtoken.Validator - HSTS bool - SecureCookie bool + StrictTransportSecurity bool + SecureAuthCookie bool } // New constructs the Coder API into an HTTP handler. @@ -50,7 +50,7 @@ func New(options *Options) (http.Handler, func()) { r.Route("/api/v2", func(r chi.Router) { r.Use( chitrace.Middleware(), - httpmw.HSTS(api.HSTS), + httpmw.StrictTransportSecurity(api.StrictTransportSecurity), ) r.Get("/", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(w, http.StatusOK, httpapi.Response{ diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/stricttransportsecurity.go similarity index 76% rename from coderd/httpmw/hsts.go rename to coderd/httpmw/stricttransportsecurity.go index 07b5d7c0463a1..647f1e05396e7 100644 --- a/coderd/httpmw/hsts.go +++ b/coderd/httpmw/stricttransportsecurity.go @@ -7,8 +7,8 @@ import ( ) const ( - StrictTransportSecurityHeader = "Strict-Transport-Security" - StrictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year + strictTransportSecurityHeader = "Strict-Transport-Security" + strictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year ) // StrictTransportSecurity will add the strict-transport-security header if enabled. @@ -25,7 +25,7 @@ func StrictTransportSecurity(enable bool) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if enable { - w.Header().Set(StrictTransportSecurityHeader, fmt.Sprintf("max-age=%d", int64(StrictTransportSecurityMaxAge.Seconds()))) + w.Header().Set(strictTransportSecurityHeader, fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) } next.ServeHTTP(w, r) diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/stricttransportsecurity_test.go similarity index 62% rename from coderd/httpmw/hsts_test.go rename to coderd/httpmw/stricttransportsecurity_test.go index 3a9905b2a3552..88c25789bb543 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/stricttransportsecurity_test.go @@ -5,10 +5,17 @@ import ( "net/http" "net/http/httptest" "testing" + "time" - "github.com/coder/coder/coderd/httpmw" "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpmw" +) + +const ( + strictTransportSecurityHeader = "Strict-Transport-Security" + strictTransportSecurityMaxAge = time.Hour * 24 * 365 ) func TestStrictTransportSecurity(t *testing.T) { @@ -32,14 +39,14 @@ func TestStrictTransportSecurity(t *testing.T) { res := setup(true) defer res.Body.Close() - require.Contains(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(httpmw.StrictTransportSecurityMaxAge))) + require.Contains(t, res.Header.Get(strictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) }) t.Run("False", func(t *testing.T) { t.Parallel() res := setup(false) defer res.Body.Close() - require.NotContains(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(httpmw.StrictTransportSecurityMaxAge))) - require.Equal(t, res.Header.Get(httpmw.StrictTransportSecurityHeader), "") + require.NotContains(t, res.Header.Get(strictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) + require.Equal(t, res.Header.Get(strictTransportSecurityHeader), "") }) } From fccf4bba440e537e4c71394b15c94e251e144c01 Mon Sep 17 00:00:00 2001 From: Garrett Date: Thu, 31 Mar 2022 15:05:01 +0000 Subject: [PATCH 5/6] pr comments --- coderd/httpmw/stricttransportsecurity.go | 12 ++++-------- coderd/httpmw/stricttransportsecurity_test.go | 8 +++----- coderd/users.go | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/coderd/httpmw/stricttransportsecurity.go b/coderd/httpmw/stricttransportsecurity.go index 647f1e05396e7..93c2d2dc465f3 100644 --- a/coderd/httpmw/stricttransportsecurity.go +++ b/coderd/httpmw/stricttransportsecurity.go @@ -6,11 +6,6 @@ import ( "time" ) -const ( - strictTransportSecurityHeader = "Strict-Transport-Security" - strictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year -) - // StrictTransportSecurity will add the strict-transport-security header if enabled. // This header forces a browser to always use https for the domain after it loads https // once. @@ -23,12 +18,13 @@ const ( // nolint:revive func StrictTransportSecurity(enable bool) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if enable { - w.Header().Set(strictTransportSecurityHeader, fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) + age := time.Hour * 24 * 365 // 1 year + rw.Header().Set("Strict-Transport-Security", fmt.Sprintf("max-age=%d", int64(age.Seconds()))) } - next.ServeHTTP(w, r) + next.ServeHTTP(rw, r) }) } } diff --git a/coderd/httpmw/stricttransportsecurity_test.go b/coderd/httpmw/stricttransportsecurity_test.go index 88c25789bb543..0cd04d1ecf9ef 100644 --- a/coderd/httpmw/stricttransportsecurity_test.go +++ b/coderd/httpmw/stricttransportsecurity_test.go @@ -13,14 +13,12 @@ import ( "github.com/coder/coder/coderd/httpmw" ) -const ( - strictTransportSecurityHeader = "Strict-Transport-Security" - strictTransportSecurityMaxAge = time.Hour * 24 * 365 -) - func TestStrictTransportSecurity(t *testing.T) { t.Parallel() + strictTransportSecurityHeader := "Strict-Transport-Security" + strictTransportSecurityMaxAge := time.Hour * 24 * 365 + setup := func(enable bool) *http.Response { rw := httptest.NewRecorder() r := httptest.NewRequest("GET", "/", nil) diff --git a/coderd/users.go b/coderd/users.go index a3b72002ca68a..8d413b9cdc05a 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -417,7 +417,7 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) { Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, - Secure: api.SecureCookie, + Secure: api.SecureAuthCookie, }) render.Status(r, http.StatusCreated) From 49f2e754b67e46f7c8b4c4008c28079ce8321b47 Mon Sep 17 00:00:00 2001 From: Garrett Date: Thu, 31 Mar 2022 17:15:20 +0000 Subject: [PATCH 6/6] rip out hsts --- cli/start.go | 35 ++++++------- coderd/coderd.go | 8 +-- coderd/httpmw/stricttransportsecurity.go | 30 ----------- coderd/httpmw/stricttransportsecurity_test.go | 50 ------------------- 4 files changed, 18 insertions(+), 105 deletions(-) delete mode 100644 coderd/httpmw/stricttransportsecurity.go delete mode 100644 coderd/httpmw/stricttransportsecurity_test.go diff --git a/cli/start.go b/cli/start.go index 7a1e0f4e1e685..47a2032f15ff1 100644 --- a/cli/start.go +++ b/cli/start.go @@ -47,17 +47,16 @@ func start() *cobra.Command { dev bool postgresURL string // provisionerDaemonCount is a uint8 to ensure a number > 0. - provisionerDaemonCount uint8 - tlsCertFile string - tlsClientCAFile string - tlsClientAuth string - tlsEnable bool - tlsKeyFile string - tlsMinVersion string - useTunnel bool - traceDatadog bool - strictTransportSecurity bool - secureAuthCookie bool + provisionerDaemonCount uint8 + tlsCertFile string + tlsClientCAFile string + tlsClientAuth string + tlsEnable bool + tlsKeyFile string + tlsMinVersion string + useTunnel bool + traceDatadog bool + secureAuthCookie bool ) root := &cobra.Command{ Use: "start", @@ -129,13 +128,12 @@ func start() *cobra.Command { } logger := slog.Make(sloghuman.Sink(os.Stderr)) options := &coderd.Options{ - AccessURL: accessURLParsed, - Logger: logger.Named("coderd"), - Database: databasefake.New(), - Pubsub: database.NewPubsubInMemory(), - GoogleTokenValidator: validator, - StrictTransportSecurity: strictTransportSecurity, - SecureAuthCookie: secureAuthCookie, + AccessURL: accessURLParsed, + Logger: logger.Named("coderd"), + Database: databasefake.New(), + Pubsub: database.NewPubsubInMemory(), + GoogleTokenValidator: validator, + SecureAuthCookie: secureAuthCookie, } if !dev { @@ -338,7 +336,6 @@ 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(), &strictTransportSecurity, "strict-transport-security", "", "CODER_STRICT_TRANSPORT_SECURITY", false, `Specifies if the "strict-transport-security" header is set on http responses`) cliflag.BoolVarP(root.Flags(), &secureAuthCookie, "secure-auth-cookie", "", "CODER_SECURE_AUTH_COOKIE", false, "Specifies if the 'Secure' property is set on browser session cookies") return root diff --git a/coderd/coderd.go b/coderd/coderd.go index aee79f4708ced..0eac01793eb76 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -30,8 +30,7 @@ type Options struct { AWSCertificates awsidentity.Certificates GoogleTokenValidator *idtoken.Validator - StrictTransportSecurity bool - SecureAuthCookie bool + SecureAuthCookie bool } // New constructs the Coder API into an HTTP handler. @@ -48,10 +47,7 @@ func New(options *Options) (http.Handler, func()) { r := chi.NewRouter() r.Route("/api/v2", func(r chi.Router) { - r.Use( - chitrace.Middleware(), - httpmw.StrictTransportSecurity(api.StrictTransportSecurity), - ) + r.Use(chitrace.Middleware()) r.Get("/", func(w http.ResponseWriter, r *http.Request) { httpapi.Write(w, http.StatusOK, httpapi.Response{ Message: "👋", diff --git a/coderd/httpmw/stricttransportsecurity.go b/coderd/httpmw/stricttransportsecurity.go deleted file mode 100644 index 93c2d2dc465f3..0000000000000 --- a/coderd/httpmw/stricttransportsecurity.go +++ /dev/null @@ -1,30 +0,0 @@ -package httpmw - -import ( - "fmt" - "net/http" - "time" -) - -// StrictTransportSecurity will add the strict-transport-security header if enabled. -// This header forces a browser to always use https for the domain after it loads https -// once. -// Meaning: On first load of product.coder.com, they are redirected to https. -// On all subsequent loads, the client's local browser forces https. This prevents man in the middle. -// -// This header only makes sense if the app is using tls. -// Full header example -// Strict-Transport-Security: max-age=63072000; -// nolint:revive -func StrictTransportSecurity(enable bool) func(next http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - if enable { - age := time.Hour * 24 * 365 // 1 year - rw.Header().Set("Strict-Transport-Security", fmt.Sprintf("max-age=%d", int64(age.Seconds()))) - } - - next.ServeHTTP(rw, r) - }) - } -} diff --git a/coderd/httpmw/stricttransportsecurity_test.go b/coderd/httpmw/stricttransportsecurity_test.go deleted file mode 100644 index 0cd04d1ecf9ef..0000000000000 --- a/coderd/httpmw/stricttransportsecurity_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package httpmw_test - -import ( - "fmt" - "net/http" - "net/http/httptest" - "testing" - "time" - - "github.com/go-chi/chi/v5" - "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/httpmw" -) - -func TestStrictTransportSecurity(t *testing.T) { - t.Parallel() - - strictTransportSecurityHeader := "Strict-Transport-Security" - strictTransportSecurityMaxAge := time.Hour * 24 * 365 - - setup := func(enable bool) *http.Response { - rw := httptest.NewRecorder() - r := httptest.NewRequest("GET", "/", nil) - - rtr := chi.NewRouter() - rtr.Use(httpmw.StrictTransportSecurity(enable)) - rtr.Get("/", func(w http.ResponseWriter, r *http.Request) { - _, _ = w.Write([]byte("hello!")) - }) - rtr.ServeHTTP(rw, r) - return rw.Result() - } - - t.Run("True", func(t *testing.T) { - t.Parallel() - - res := setup(true) - defer res.Body.Close() - require.Contains(t, res.Header.Get(strictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) - }) - t.Run("False", func(t *testing.T) { - t.Parallel() - - res := setup(false) - defer res.Body.Close() - require.NotContains(t, res.Header.Get(strictTransportSecurityHeader), fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds()))) - require.Equal(t, res.Header.Get(strictTransportSecurityHeader), "") - }) -}