-
Notifications
You must be signed in to change notification settings - Fork 899
feat: Add option to enable hsts header #6147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
643f42a
bc89288
f216054
5e0a232
6389727
8221ab0
ac1fd5c
8f15025
602aa8f
895fece
c3c822c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,12 +224,21 @@ 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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user experience will be poor here if they enter a wrong value into options. Ideally we should verify this input before we start creating connections/starting the database in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can validate in cli/server.go. It is annoying the I'll just duplicate the valid check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍. I don’t think we use New in too many places, should be relatively easy to make it return an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I changed it so I validate in server.go.
|
||
} | ||
|
||
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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
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) | ||
}) | ||
|
||
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") | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to explain that the number sets the maxAge here.
We could also consider splitting this up into an enable bool and explicit maxage option that's required when enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that in the message. If I add another flag, then it's 3 flags to turn this thing fully on :(. I figure anything >0 make sense to enable it.