From 6189035e9899e8008a0ccfa27916dcafc9066d3d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Feb 2023 10:52:49 -0600 Subject: [PATCH] feat: Add option to enable hsts header (#6147) * feat: Add option to enable hsts header * Update golden files --- cli/deployment/config.go | 14 ++++ cli/server.go | 7 ++ cli/testdata/coder_server_--help.golden | 17 ++++ coderd/apidoc/docs.go | 6 ++ coderd/apidoc/swagger.json | 6 ++ coderd/coderd.go | 9 ++- coderd/httpmw/hsts.go | 72 +++++++++++++++++ coderd/httpmw/hsts_test.go | 103 ++++++++++++++++++++++++ codersdk/deployment.go | 2 + docs/api/general.md | 22 +++++ docs/api/schemas.md | 24 ++++++ docs/cli/coder_server.md | 4 + site/src/api/typesGenerated.ts | 2 + 13 files changed, 287 insertions(+), 1 deletion(-) 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 d5bb01db8e..41f53eb600 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -374,6 +374,20 @@ 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 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.", + 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 59121e8c23..41916eeabc 100644 --- a/cli/server.go +++ b/cli/server.go @@ -485,6 +485,13 @@ func Server(vip *viper.Viper, newAPI func(context.Context, *coderd.Options) (*co 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/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index ea59437409..4a05a68c0f 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 diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 36596fa706..06583721c9 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 b6452ba591..44a964ff43 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/coderd/coderd.go b/coderd/coderd.go index bfce5a5fb1..2a3627e4c4 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -103,6 +103,7 @@ type Options struct { OIDCConfig *OIDCConfig PrometheusRegistry *prometheus.Registry SecureAuthCookie bool + StrictTransportSecurityCfg httpmw.HSTSConfig SSHKeygenAlgorithm gitsshkey.Algorithm Telemetry telemetry.Reporter TracerProvider trace.TracerProvider @@ -222,12 +223,18 @@ 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 = httpmw.HSTS(staticHandler, options.StrictTransportSecurityCfg) + r := chi.NewRouter() api := &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, diff --git a/coderd/httpmw/hsts.go b/coderd/httpmw/hsts.go new file mode 100644 index 0000000000..52cfb79bbc --- /dev/null +++ b/coderd/httpmw/hsts.go @@ -0,0 +1,72 @@ +package httpmw + +import ( + "fmt" + "net/http" + "strings" + + "golang.org/x/xerrors" +) + +const ( + hstsHeader = "Strict-Transport-Security" +) + +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 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 HSTSConfig{}, 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 HSTSConfig{}, xerrors.Errorf("hsts: invalid option: %q. Must be 'preload' and/or 'includeSubDomains'", option) + } + _, err = str.WriteString("; " + option) + if err != nil { + 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, cfg.HeaderValue) + next.ServeHTTP(w, r) + }) +} diff --git a/coderd/httpmw/hsts_test.go b/coderd/httpmw/hsts_test.go new file mode 100644 index 0000000000..68f73e8adf --- /dev/null +++ b/coderd/httpmw/hsts_test.go @@ -0,0 +1,103 @@ +package httpmw_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpmw" +) + +func TestHSTS(t *testing.T) { + t.Parallel() + + 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) + }) + + 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") + }) + } +} diff --git a/codersdk/deployment.go b/codersdk/deployment.go index da4a68e675..ced725f43a 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"` diff --git a/docs/api/general.md b/docs/api/general.md index 423619d3c8..15854e7cfe 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 a8672795ce..c9d98afbc7 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 f6d42d755d..fd84472973 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 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 --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 471e2e5eb0..64ad8a3e5f 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