From 643f42ac1a5b2296aae4bc9be41dd604ab0772f5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 21:14:59 -0600 Subject: [PATCH 01/11] feat: Add option to enable hsts header --- cli/deployment/config.go | 13 +++++ cli/server.go | 48 +++++++++--------- coderd/coderd.go | 11 ++++ coderd/httpmw/hsts.go | 58 +++++++++++++++++++++ coderd/httpmw/hsts_test.go | 100 +++++++++++++++++++++++++++++++++++++ codersdk/deployment.go | 2 + 6 files changed, 209 insertions(+), 23 deletions(-) create mode 100644 coderd/httpmw/hsts.go create mode 100644 coderd/httpmw/hsts_test.go diff --git a/cli/deployment/config.go b/cli/deployment/config.go index d5bb01db8e6ba..a433253cd3399 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -374,6 +374,19 @@ func newConfig() *codersdk.DeploymentConfig { Usage: "Controls if the 'Secure' property is set on browser session cookies.", Flag: "secure-auth-cookie", }, + StrictTransportSecurity: &codersdk.DeploymentConfigField[int]{ + Name: "Strict-Transport-Security", + Usage: "Controls if the 'Strict-Transport-Security' header is set on all responses. " + + "This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds.", + Default: 0, + Flag: "strict-transport-security", + }, + StrictTransportSecurityOptions: &codersdk.DeploymentConfigField[[]string]{ + Name: "Strict-Transport-Security Options", + Usage: "Two optional fields can be set in the Strict-Transport-Security header; 'includeSubDomains' and 'preload'. " + + "The 'strict-transport-security' flag must be set to a non-zero value for these options to be used.", + Flag: "strict-transport-security-options", + }, SSHKeygenAlgorithm: &codersdk.DeploymentConfigField[string]{ Name: "SSH Keygen Algorithm", Usage: "The algorithm to use for generating ssh keys. Accepted values are \"ed25519\", \"ecdsa\", or \"rsa4096\".", diff --git a/cli/server.go b/cli/server.go index 59121e8c23d81..a2f8444ff4cd0 100644 --- a/cli/server.go +++ b/cli/server.go @@ -457,29 +457,31 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co } options := &coderd.Options{ - AccessURL: accessURLParsed, - AppHostname: appHostname, - AppHostnameRegex: appHostnameRegex, - Logger: logger.Named("coderd"), - Database: dbfake.New(), - DERPMap: derpMap, - Pubsub: database.NewPubsubInMemory(), - CacheDir: cacheDir, - GoogleTokenValidator: googleTokenValidator, - GitAuthConfigs: gitAuthConfigs, - RealIPConfig: realIPConfig, - SecureAuthCookie: cfg.SecureAuthCookie.Value, - SSHKeygenAlgorithm: sshKeygenAlgorithm, - TracerProvider: tracerProvider, - Telemetry: telemetry.NewNoop(), - MetricsCacheRefreshInterval: cfg.MetricsCacheRefreshInterval.Value, - AgentStatsRefreshInterval: cfg.AgentStatRefreshInterval.Value, - DeploymentConfig: cfg, - PrometheusRegistry: prometheus.NewRegistry(), - APIRateLimit: cfg.RateLimit.API.Value, - LoginRateLimit: loginRateLimit, - FilesRateLimit: filesRateLimit, - HTTPClient: httpClient, + AccessURL: accessURLParsed, + AppHostname: appHostname, + AppHostnameRegex: appHostnameRegex, + Logger: logger.Named("coderd"), + Database: dbfake.New(), + DERPMap: derpMap, + Pubsub: database.NewPubsubInMemory(), + CacheDir: cacheDir, + GoogleTokenValidator: googleTokenValidator, + GitAuthConfigs: gitAuthConfigs, + RealIPConfig: realIPConfig, + SecureAuthCookie: cfg.SecureAuthCookie.Value, + StrictTransportSecurityAge: cfg.StrictTransportSecurity.Value, + StrictTransportSecurityOptions: cfg.StrictTransportSecurityOptions.Value, + SSHKeygenAlgorithm: sshKeygenAlgorithm, + TracerProvider: tracerProvider, + Telemetry: telemetry.NewNoop(), + MetricsCacheRefreshInterval: cfg.MetricsCacheRefreshInterval.Value, + AgentStatsRefreshInterval: cfg.AgentStatRefreshInterval.Value, + DeploymentConfig: cfg, + PrometheusRegistry: prometheus.NewRegistry(), + APIRateLimit: cfg.RateLimit.API.Value, + LoginRateLimit: loginRateLimit, + FilesRateLimit: filesRateLimit, + HTTPClient: httpClient, } if tlsConfig != nil { options.TLSCertificates = tlsConfig.Certificates diff --git a/coderd/coderd.go b/coderd/coderd.go index bfce5a5fb1a88..4f3252d8ecf0f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -103,6 +103,8 @@ type Options struct { OIDCConfig *OIDCConfig PrometheusRegistry *prometheus.Registry SecureAuthCookie bool + StrictTransportSecurityAge int + StrictTransportSecurityOptions []string SSHKeygenAlgorithm gitsshkey.Algorithm Telemetry telemetry.Reporter TracerProvider trace.TracerProvider @@ -222,6 +224,15 @@ func New(options *Options) *API { options.MetricsCacheRefreshInterval, ) + staticHandler := site.Handler(site.FS(), binFS, binHashes) + // Static file handler must be wrapped with HSTS handler if the + // StrictTransportSecurityAge is set. We only need to set this header on + // static files since it only affects browsers. + staticHandler, err = httpmw.HSTS(staticHandler, options.StrictTransportSecurityAge, options.StrictTransportSecurityOptions) + if err != nil { + panic(xerrors.Errorf("coderd: setting hsts header failed (options: %v): %w", options.StrictTransportSecurityOptions, err)) + } + r := chi.NewRouter() api := &API{ ID: uuid.New(), diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go new file mode 100644 index 0000000000000..64da25cdfbedb --- /dev/null +++ b/coderd/httpmw/hsts.go @@ -0,0 +1,58 @@ +package httpmw + +import ( + "fmt" + "net/http" + "strings" + + "golang.org/x/xerrors" +) + +const ( + hstsHeader = "Strict-Transport-Security" +) + +// 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(next http.Handler, maxAge int, options []string) (http.Handler, error) { + if maxAge <= 0 { + // No header, so no need to wrap the handler + return next, nil + } + + // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security + var str strings.Builder + _, err := str.WriteString(fmt.Sprintf("max-age=%d", maxAge)) + if err != nil { + return nil, xerrors.Errorf("hsts: write max-age: %w", err) + } + + for _, option := range options { + switch { + // Only allow valid options and fix any casing mistakes + case strings.EqualFold(option, "includeSubDomains"): + option = "includeSubDomains" + case strings.EqualFold(option, "preload"): + option = "preload" + default: + return nil, xerrors.Errorf("hsts: invalid option: %q. Must be 'preload' and/or 'includeSubDomains'", option) + } + _, err = str.WriteString("; " + option) + if err != nil { + return nil, xerrors.Errorf("hsts: write option: %w", err) + } + } + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set(hstsHeader, str.String()) + next.ServeHTTP(w, r) + }), nil +} diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go new file mode 100644 index 0000000000000..22f3b9e98588d --- /dev/null +++ b/coderd/httpmw/hsts_test.go @@ -0,0 +1,100 @@ +package httpmw_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/coder/coder/coderd/httpmw" + + "github.com/stretchr/testify/require" +) + +func TestHSTS(t *testing.T) { + tests := []struct { + Name string + MaxAge int + Options []string + + wantErr bool + expectHeader string + }{ + { + Name: "Empty", + MaxAge: 0, + Options: nil, + }, + { + Name: "NoAge", + MaxAge: 0, + Options: []string{"includeSubDomains"}, + }, + { + Name: "NegativeAge", + MaxAge: -100, + Options: []string{"includeSubDomains"}, + }, + { + Name: "Age", + MaxAge: 1000, + Options: []string{}, + expectHeader: "max-age=1000", + }, + { + Name: "AgeSubDomains", + MaxAge: 1000, + // Mess with casing + Options: []string{"INCLUDESUBDOMAINS"}, + expectHeader: "max-age=1000; includeSubDomains", + }, + { + Name: "AgePreload", + MaxAge: 1000, + Options: []string{"Preload"}, + expectHeader: "max-age=1000; preload", + }, + { + Name: "AllOptions", + MaxAge: 1000, + Options: []string{"preload", "includeSubDomains"}, + expectHeader: "max-age=1000; preload; includeSubDomains", + }, + + // Error values + { + Name: "BadOption", + MaxAge: 100, + Options: []string{"not-valid"}, + wantErr: true, + }, + { + Name: "BadOptions", + MaxAge: 100, + Options: []string{"includeSubDomains", "not-valid", "still-not-valid"}, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.Name, func(t *testing.T) { + t.Parallel() + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + got, err := httpmw.HSTS(handler, tt.MaxAge, tt.Options) + if tt.wantErr { + require.Error(t, err, "Expect error, HSTS(%v, %v)", tt.MaxAge, tt.Options) + return + } + + require.NoError(t, err, "Expect no error, HSTS(%v, %v)", tt.MaxAge, tt.Options) + req := httptest.NewRequest("GET", "/", nil) + res := httptest.NewRecorder() + + got.ServeHTTP(res, req) + require.Equal(t, tt.expectHeader, res.Header().Get("Strict-Transport-Security"), "expected header value") + }) + } +} diff --git a/codersdk/deployment.go b/codersdk/deployment.go index da4a68e6753a8..ced725f43a38a 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -126,6 +126,8 @@ type DeploymentConfig struct { TLS *TLSConfig `json:"tls" typescript:",notnull"` Trace *TraceConfig `json:"trace" typescript:",notnull"` SecureAuthCookie *DeploymentConfigField[bool] `json:"secure_auth_cookie" typescript:",notnull"` + StrictTransportSecurity *DeploymentConfigField[int] `json:"strict_transport_security" typescript:",notnull"` + StrictTransportSecurityOptions *DeploymentConfigField[[]string] `json:"strict_transport_security_options" typescript:",notnull"` SSHKeygenAlgorithm *DeploymentConfigField[string] `json:"ssh_keygen_algorithm" typescript:",notnull"` MetricsCacheRefreshInterval *DeploymentConfigField[time.Duration] `json:"metrics_cache_refresh_interval" typescript:",notnull"` AgentStatRefreshInterval *DeploymentConfigField[time.Duration] `json:"agent_stat_refresh_interval" typescript:",notnull"` From bc892888048f2af8519bfb8e572ffb33f8bda67d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 21:16:07 -0600 Subject: [PATCH 02/11] Import sorting --- coderd/httpmw/hsts_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go index 22f3b9e98588d..e19b6cc6dd08c 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/hsts_test.go @@ -5,9 +5,9 @@ import ( "net/http/httptest" "testing" - "github.com/coder/coder/coderd/httpmw" - "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpmw" ) func TestHSTS(t *testing.T) { From f2160540f48542711de77a327955dcbe64d7debf Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 21:17:56 -0600 Subject: [PATCH 03/11] Make gen --- coderd/apidoc/docs.go | 6 ++++++ coderd/apidoc/swagger.json | 6 ++++++ docs/api/general.md | 22 ++++++++++++++++++++++ docs/api/schemas.md | 24 ++++++++++++++++++++++++ docs/cli/coder_server.md | 4 ++++ site/src/api/typesGenerated.ts | 2 ++ 6 files changed, 64 insertions(+) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 36596fa706242..06583721c9679 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6146,6 +6146,12 @@ const docTemplate = `{ "ssh_keygen_algorithm": { "$ref": "#/definitions/codersdk.DeploymentConfigField-string" }, + "strict_transport_security": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-int" + }, + "strict_transport_security_options": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-array_string" + }, "swagger": { "$ref": "#/definitions/codersdk.SwaggerConfig" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b6452ba5911d9..44a964ff435eb 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -5470,6 +5470,12 @@ "ssh_keygen_algorithm": { "$ref": "#/definitions/codersdk.DeploymentConfigField-string" }, + "strict_transport_security": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-int" + }, + "strict_transport_security_options": { + "$ref": "#/definitions/codersdk.DeploymentConfigField-array_string" + }, "swagger": { "$ref": "#/definitions/codersdk.SwaggerConfig" }, diff --git a/docs/api/general.md b/docs/api/general.md index 423619d3c8265..15854e7cfec86 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -857,6 +857,28 @@ curl -X GET http://coder-server:8080/api/v2/config/deployment \ "usage": "string", "value": "string" }, + "strict_transport_security": { + "default": 0, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": 0 + }, + "strict_transport_security_options": { + "default": ["string"], + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": ["string"] + }, "swagger": { "enable": { "default": true, diff --git a/docs/api/schemas.md b/docs/api/schemas.md index a8672795ce132..c9d98afbc7e07 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -2256,6 +2256,28 @@ CreateParameterRequest is a structure used to create a new parameter value for a "usage": "string", "value": "string" }, + "strict_transport_security": { + "default": 0, + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": 0 + }, + "strict_transport_security_options": { + "default": ["string"], + "enterprise": true, + "flag": "string", + "hidden": true, + "name": "string", + "secret": true, + "shorthand": "string", + "usage": "string", + "value": ["string"] + }, "swagger": { "enable": { "default": true, @@ -2515,6 +2537,8 @@ CreateParameterRequest is a structure used to create a new parameter value for a | `scim_api_key` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | | `secure_auth_cookie` | [codersdk.DeploymentConfigField-bool](#codersdkdeploymentconfigfield-bool) | false | | | | `ssh_keygen_algorithm` | [codersdk.DeploymentConfigField-string](#codersdkdeploymentconfigfield-string) | false | | | +| `strict_transport_security` | [codersdk.DeploymentConfigField-int](#codersdkdeploymentconfigfield-int) | false | | | +| `strict_transport_security_options` | [codersdk.DeploymentConfigField-array_string](#codersdkdeploymentconfigfield-array_string) | false | | | | `swagger` | [codersdk.SwaggerConfig](#codersdkswaggerconfig) | false | | | | `telemetry` | [codersdk.TelemetryConfig](#codersdktelemetryconfig) | false | | | | `tls` | [codersdk.TLSConfig](#codersdktlsconfig) | false | | | diff --git a/docs/cli/coder_server.md b/docs/cli/coder_server.md index f6d42d755d755..406a8ed0ad332 100644 --- a/docs/cli/coder_server.md +++ b/docs/cli/coder_server.md @@ -118,6 +118,10 @@ coder server [flags] Consumes $CODER_MAX_SESSION_EXPIRY (default 24h0m0s) --ssh-keygen-algorithm string The algorithm to use for generating ssh keys. Accepted values are "ed25519", "ecdsa", or "rsa4096". Consumes $CODER_SSH_KEYGEN_ALGORITHM (default "ed25519") + --strict-transport-security int Controls if the 'Strict-Transport-Security' header is set on all responses. This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds. + Consumes $CODER_STRICT_TRANSPORT_SECURITY + --strict-transport-security-options strings Two optional fields can be set in the Strict-Transport-Security header; 'includeSubDomains' and 'preload'. The 'strict-transport-security' flag must be set to a non-zero value for these options to be used. + Consumes $CODER_STRICT_TRANSPORT_SECURITY_OPTIONS --swagger-enable Expose the swagger endpoint via /swagger. Consumes $CODER_SWAGGER_ENABLE --telemetry Whether telemetry is enabled or not. Coder collects anonymized usage data to help improve our product. diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 471e2e5eb07cf..64ad8a3e5f8b8 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -309,6 +309,8 @@ export interface DeploymentConfig { readonly tls: TLSConfig readonly trace: TraceConfig readonly secure_auth_cookie: DeploymentConfigField + readonly strict_transport_security: DeploymentConfigField + readonly strict_transport_security_options: DeploymentConfigField readonly ssh_keygen_algorithm: DeploymentConfigField readonly metrics_cache_refresh_interval: DeploymentConfigField readonly agent_stat_refresh_interval: DeploymentConfigField From 5e0a23254241677c8314dfee717f3feee074d701 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 21:35:29 -0600 Subject: [PATCH 04/11] Actually use the right handler --- cli/deployment/config.go | 2 +- coderd/coderd.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index a433253cd3399..1d34714c0c77d 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -376,7 +376,7 @@ func newConfig() *codersdk.DeploymentConfig { }, StrictTransportSecurity: &codersdk.DeploymentConfigField[int]{ Name: "Strict-Transport-Security", - Usage: "Controls if the 'Strict-Transport-Security' header is set on all responses. " + + Usage: "Controls if the 'Strict-Transport-Security' header is set on all static file responses. " + "This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds.", Default: 0, Flag: "strict-transport-security", diff --git a/coderd/coderd.go b/coderd/coderd.go index 4f3252d8ecf0f..df9f4374c9ed3 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -238,7 +238,7 @@ func New(options *Options) *API { ID: uuid.New(), Options: options, RootHandler: r, - siteHandler: site.Handler(site.FS(), binFS, binHashes), + siteHandler: staticHandler, HTTPAuth: &HTTPAuthorizer{ Authorizer: options.Authorizer, Logger: options.Logger, From 6389727b446d4cd7832d7f9161e29cb7a0ad56a3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 22:44:13 -0600 Subject: [PATCH 05/11] Add parallel unit test --- coderd/httpmw/hsts_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go index e19b6cc6dd08c..8c6284089f515 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/hsts_test.go @@ -11,6 +11,8 @@ import ( ) func TestHSTS(t *testing.T) { + t.Parallel() + tests := []struct { Name string MaxAge int From 8221ab0ca273db2789251b7898c5daea1eb10d8a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Feb 2023 22:45:43 -0600 Subject: [PATCH 06/11] make gen --- docs/cli/coder_server.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cli/coder_server.md b/docs/cli/coder_server.md index 406a8ed0ad332..9ea3d72d233bc 100644 --- a/docs/cli/coder_server.md +++ b/docs/cli/coder_server.md @@ -118,7 +118,7 @@ coder server [flags] Consumes $CODER_MAX_SESSION_EXPIRY (default 24h0m0s) --ssh-keygen-algorithm string The algorithm to use for generating ssh keys. Accepted values are "ed25519", "ecdsa", or "rsa4096". Consumes $CODER_SSH_KEYGEN_ALGORITHM (default "ed25519") - --strict-transport-security int Controls if the 'Strict-Transport-Security' header is set on all responses. This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds. + --strict-transport-security int Controls if the 'Strict-Transport-Security' header is set on all static file responses. This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds. Consumes $CODER_STRICT_TRANSPORT_SECURITY --strict-transport-security-options strings Two optional fields can be set in the Strict-Transport-Security header; 'includeSubDomains' and 'preload'. The 'strict-transport-security' flag must be set to a non-zero value for these options to be used. Consumes $CODER_STRICT_TRANSPORT_SECURITY_OPTIONS From ac1fd5c99bdf3e6f26112f9919e8a2fe5e06be09 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 08:56:50 -0600 Subject: [PATCH 07/11] Verify hsts options earlier --- cli/server.go | 55 +++++++++++++++++++++----------------- coderd/coderd.go | 8 ++---- coderd/httpmw/hsts.go | 50 +++++++++++++++++++++------------- coderd/httpmw/hsts_test.go | 7 ++--- 4 files changed, 68 insertions(+), 52 deletions(-) diff --git a/cli/server.go b/cli/server.go index a2f8444ff4cd0..41916eeabceac 100644 --- a/cli/server.go +++ b/cli/server.go @@ -457,36 +457,41 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co } options := &coderd.Options{ - AccessURL: accessURLParsed, - AppHostname: appHostname, - AppHostnameRegex: appHostnameRegex, - Logger: logger.Named("coderd"), - Database: dbfake.New(), - DERPMap: derpMap, - Pubsub: database.NewPubsubInMemory(), - CacheDir: cacheDir, - GoogleTokenValidator: googleTokenValidator, - GitAuthConfigs: gitAuthConfigs, - RealIPConfig: realIPConfig, - SecureAuthCookie: cfg.SecureAuthCookie.Value, - StrictTransportSecurityAge: cfg.StrictTransportSecurity.Value, - StrictTransportSecurityOptions: cfg.StrictTransportSecurityOptions.Value, - SSHKeygenAlgorithm: sshKeygenAlgorithm, - TracerProvider: tracerProvider, - Telemetry: telemetry.NewNoop(), - MetricsCacheRefreshInterval: cfg.MetricsCacheRefreshInterval.Value, - AgentStatsRefreshInterval: cfg.AgentStatRefreshInterval.Value, - DeploymentConfig: cfg, - PrometheusRegistry: prometheus.NewRegistry(), - APIRateLimit: cfg.RateLimit.API.Value, - LoginRateLimit: loginRateLimit, - FilesRateLimit: filesRateLimit, - HTTPClient: httpClient, + AccessURL: accessURLParsed, + AppHostname: appHostname, + AppHostnameRegex: appHostnameRegex, + Logger: logger.Named("coderd"), + Database: dbfake.New(), + DERPMap: derpMap, + Pubsub: database.NewPubsubInMemory(), + CacheDir: cacheDir, + GoogleTokenValidator: googleTokenValidator, + GitAuthConfigs: gitAuthConfigs, + RealIPConfig: realIPConfig, + SecureAuthCookie: cfg.SecureAuthCookie.Value, + SSHKeygenAlgorithm: sshKeygenAlgorithm, + TracerProvider: tracerProvider, + Telemetry: telemetry.NewNoop(), + MetricsCacheRefreshInterval: cfg.MetricsCacheRefreshInterval.Value, + AgentStatsRefreshInterval: cfg.AgentStatRefreshInterval.Value, + DeploymentConfig: cfg, + PrometheusRegistry: prometheus.NewRegistry(), + APIRateLimit: cfg.RateLimit.API.Value, + LoginRateLimit: loginRateLimit, + FilesRateLimit: filesRateLimit, + HTTPClient: httpClient, } if tlsConfig != nil { options.TLSCertificates = tlsConfig.Certificates } + if cfg.StrictTransportSecurity.Value > 0 { + options.StrictTransportSecurityCfg, err = httpmw.HSTSConfigOptions(cfg.StrictTransportSecurity.Value, cfg.StrictTransportSecurityOptions.Value) + if err != nil { + return xerrors.Errorf("coderd: setting hsts header failed (options: %v): %w", cfg.StrictTransportSecurityOptions.Value, err) + } + } + if cfg.UpdateCheck.Value { options.UpdateCheckOptions = &updatecheck.Options{ // Avoid spamming GitHub API checking for updates. diff --git a/coderd/coderd.go b/coderd/coderd.go index df9f4374c9ed3..2a3627e4c4cce 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -103,8 +103,7 @@ type Options struct { OIDCConfig *OIDCConfig PrometheusRegistry *prometheus.Registry SecureAuthCookie bool - StrictTransportSecurityAge int - StrictTransportSecurityOptions []string + StrictTransportSecurityCfg httpmw.HSTSConfig SSHKeygenAlgorithm gitsshkey.Algorithm Telemetry telemetry.Reporter TracerProvider trace.TracerProvider @@ -228,10 +227,7 @@ func New(options *Options) *API { // Static file handler must be wrapped with HSTS handler if the // StrictTransportSecurityAge is set. We only need to set this header on // static files since it only affects browsers. - staticHandler, err = httpmw.HSTS(staticHandler, options.StrictTransportSecurityAge, options.StrictTransportSecurityOptions) - if err != nil { - panic(xerrors.Errorf("coderd: setting hsts header failed (options: %v): %w", options.StrictTransportSecurityOptions, err)) - } + staticHandler = httpmw.HSTS(staticHandler, options.StrictTransportSecurityCfg) r := chi.NewRouter() api := &API{ diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go index 64da25cdfbedb..52cfb79bbc403 100644 --- a/coderd/httpmw/hsts.go +++ b/coderd/httpmw/hsts.go @@ -12,27 +12,22 @@ const ( hstsHeader = "Strict-Transport-Security" ) -// 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(next http.Handler, maxAge int, options []string) (http.Handler, error) { +type HSTSConfig struct { + // HeaderValue is an empty string if hsts header is disabled. + HeaderValue string +} + +func HSTSConfigOptions(maxAge int, options []string) (HSTSConfig, error) { if maxAge <= 0 { - // No header, so no need to wrap the handler - return next, nil + // No header, so no need to build the header string. + return HSTSConfig{}, nil } // https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security var str strings.Builder _, err := str.WriteString(fmt.Sprintf("max-age=%d", maxAge)) if err != nil { - return nil, xerrors.Errorf("hsts: write max-age: %w", err) + return HSTSConfig{}, xerrors.Errorf("hsts: write max-age: %w", err) } for _, option := range options { @@ -43,16 +38,35 @@ func HSTS(next http.Handler, maxAge int, options []string) (http.Handler, error) case strings.EqualFold(option, "preload"): option = "preload" default: - return nil, xerrors.Errorf("hsts: invalid option: %q. Must be 'preload' and/or 'includeSubDomains'", option) + return HSTSConfig{}, xerrors.Errorf("hsts: invalid option: %q. Must be 'preload' and/or 'includeSubDomains'", option) } _, err = str.WriteString("; " + option) if err != nil { - return nil, xerrors.Errorf("hsts: write option: %w", err) + return HSTSConfig{}, xerrors.Errorf("hsts: write option: %w", err) } } + return HSTSConfig{ + HeaderValue: str.String(), + }, nil +} +// 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(next http.Handler, cfg HSTSConfig) http.Handler { + if cfg.HeaderValue == "" { + // No header, so no need to wrap the handler. + return next + } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set(hstsHeader, str.String()) + w.Header().Set(hstsHeader, cfg.HeaderValue) next.ServeHTTP(w, r) - }), nil + }) } diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go index 8c6284089f515..68f73e8adf506 100644 --- a/coderd/httpmw/hsts_test.go +++ b/coderd/httpmw/hsts_test.go @@ -85,17 +85,18 @@ func TestHSTS(t *testing.T) { w.WriteHeader(http.StatusOK) }) - got, err := httpmw.HSTS(handler, tt.MaxAge, tt.Options) + cfg, err := httpmw.HSTSConfigOptions(tt.MaxAge, tt.Options) if tt.wantErr { require.Error(t, err, "Expect error, HSTS(%v, %v)", tt.MaxAge, tt.Options) return } - require.NoError(t, err, "Expect no error, HSTS(%v, %v)", tt.MaxAge, tt.Options) + + got := httpmw.HSTS(handler, cfg) req := httptest.NewRequest("GET", "/", nil) res := httptest.NewRecorder() - got.ServeHTTP(res, req) + require.Equal(t, tt.expectHeader, res.Header().Get("Strict-Transport-Security"), "expected header value") }) } From 8f15025a117a4b257795a21e92170dd3c2e11a11 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 08:58:20 -0600 Subject: [PATCH 08/11] Clarity of what the value is --- cli/deployment/config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 1d34714c0c77d..fa801f4005ecf 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -377,7 +377,8 @@ func newConfig() *codersdk.DeploymentConfig { StrictTransportSecurity: &codersdk.DeploymentConfigField[int]{ Name: "Strict-Transport-Security", Usage: "Controls if the 'Strict-Transport-Security' header is set on all static file responses. " + - "This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds.", + "This header should only be set if the server is accessed via HTTPS. This value is the MaxAge in seconds of " + + "the header." + Default: 0, Flag: "strict-transport-security", }, From 602aa8f84ee9a8b95c0a3f159674c47acf0c631f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 08:59:25 -0600 Subject: [PATCH 09/11] Fix syntax --- cli/deployment/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index fa801f4005ecf..41f53eb600fd2 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -378,7 +378,7 @@ func newConfig() *codersdk.DeploymentConfig { Name: "Strict-Transport-Security", Usage: "Controls if the 'Strict-Transport-Security' header is set on all static file responses. " + "This header should only be set if the server is accessed via HTTPS. This value is the MaxAge in seconds of " + - "the header." + + "the header.", Default: 0, Flag: "strict-transport-security", }, From 895feced04ec4eef2735e0d9fa65f5814e2922b5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 09:22:31 -0600 Subject: [PATCH 10/11] Make gen to update msg --- docs/cli/coder_server.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cli/coder_server.md b/docs/cli/coder_server.md index 9ea3d72d233bc..fd844729734e4 100644 --- a/docs/cli/coder_server.md +++ b/docs/cli/coder_server.md @@ -118,7 +118,7 @@ coder server [flags] Consumes $CODER_MAX_SESSION_EXPIRY (default 24h0m0s) --ssh-keygen-algorithm string The algorithm to use for generating ssh keys. Accepted values are "ed25519", "ecdsa", or "rsa4096". Consumes $CODER_SSH_KEYGEN_ALGORITHM (default "ed25519") - --strict-transport-security int Controls if the 'Strict-Transport-Security' header is set on all static file responses. This header should only be set if the server is accessed via HTTPS. The value should be a whole number in seconds. + --strict-transport-security int Controls if the 'Strict-Transport-Security' header is set on all static file responses. This header should only be set if the server is accessed via HTTPS. This value is the MaxAge in seconds of the header. Consumes $CODER_STRICT_TRANSPORT_SECURITY --strict-transport-security-options strings Two optional fields can be set in the Strict-Transport-Security header; 'includeSubDomains' and 'preload'. The 'strict-transport-security' flag must be set to a non-zero value for these options to be used. Consumes $CODER_STRICT_TRANSPORT_SECURITY_OPTIONS From c3c822c669f953632028c203e4487953801dcf3f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 09:58:54 -0600 Subject: [PATCH 11/11] Update golden files --- cli/testdata/coder_server_--help.golden | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index ea59437409b5d..4a05a68c0faba 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -280,6 +280,23 @@ Flags: "ed25519", "ecdsa", or "rsa4096". Consumes $CODER_SSH_KEYGEN_ALGORITHM (default "ed25519") + --strict-transport-security int Controls if the + 'Strict-Transport-Security' header + is set on all static file responses. + This header should only be set if + the server is accessed via HTTPS. + This value is the MaxAge in seconds + of the header. + Consumes $CODER_STRICT_TRANSPORT_SECURITY + --strict-transport-security-options strings Two optional fields can be set in + the Strict-Transport-Security + header; 'includeSubDomains' and + 'preload'. The + 'strict-transport-security' flag + must be set to a non-zero value for + these options to be used. + Consumes + $CODER_STRICT_TRANSPORT_SECURITY_OPTIONS --swagger-enable Expose the swagger endpoint via /swagger. Consumes $CODER_SWAGGER_ENABLE