Verify hsts options earlier

coder · Emyrk · Feb 10, 2023 · Feb 10, 2023 · Feb 10, 2023 · Feb 10, 2023
commit ac1fd5c99bdf3e6f26112f9919e8a2fe5e06be09
diff --git 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
@@ -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
@@ -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
@@ -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")
 		})
 	}