Skip to content

feat: Add strict transport security and secure cookie options #741

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 31, 2022
Merged

feat: Add strict transport security and secure cookie options #741

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6c404c
feat: Add HSTS and secure cookie options
f0ssel Mar 30, 2022
c6534f6
Fix tests
f0ssel Mar 30, 2022
197b163
pr comments
f0ssel Mar 30, 2022
c874f35
pr comments
f0ssel Mar 30, 2022
fccf4bb
pr comments
f0ssel Mar 31, 2022
49f2e75
rip out hsts
f0ssel Mar 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 20 additions & 14 deletions cli/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +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
provisionerDaemonCount uint8
tlsCertFile string
tlsClientCAFile string
tlsClientAuth string
tlsEnable bool
tlsKeyFile string
tlsMinVersion string
useTunnel bool
traceDatadog bool
strictTransportSecurity bool
secureAuthCookie bool
)
root := &cobra.Command{
Use: "start",
Expand Down Expand Up @@ -127,11 +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,
AccessURL: accessURLParsed,
Logger: logger.Named("coderd"),
Database: databasefake.New(),
Pubsub: database.NewPubsubInMemory(),
GoogleTokenValidator: validator,
StrictTransportSecurity: strictTransportSecurity,
SecureAuthCookie: secureAuthCookie,
}

if !dev {
Expand Down Expand Up @@ -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(), &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
}
Expand Down
8 changes: 7 additions & 1 deletion coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ type Options struct {

AWSCertificates awsidentity.Certificates
GoogleTokenValidator *idtoken.Validator

StrictTransportSecurity bool
SecureAuthCookie bool
}

// New constructs the Coder API into an HTTP handler.
Expand All @@ -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.StrictTransportSecurity(api.StrictTransportSecurity),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed for static assets as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Emyrk ? I don't know but my guess is "no" because it will already be set on the browser after the first api call and also static assets would never have sensitive data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HSTS should be on static, but I think it might not really matter as you only need to hit 1 HSTS header for it "take effect". Every HSTS header after is redundant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably move it outside of there then, just to confirm properly.

)
r.Get("/", func(w http.ResponseWriter, r *http.Request) {
httpapi.Write(w, http.StatusOK, httpapi.Response{
Message: "👋",
Expand Down
34 changes: 34 additions & 0 deletions coderd/httpmw/stricttransportsecurity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package httpmw

import (
"fmt"
"net/http"
"time"
)

const (
strictTransportSecurityHeader = "Strict-Transport-Security"
strictTransportSecurityMaxAge = time.Hour * 24 * 365 // 1 year
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are just used in the one place, I don't think they should be constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmon https://en.wikipedia.org/wiki/Magic_number_(programming)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consts are more self documenting (variable name) and cleaner code to read imo. I feel like I was told never to use magic numbers like day 1 of programming and have never looked back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it anyways to get this pr merged....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave em' as consts that's just a minor nit.

// 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(w http.ResponseWriter, r *http.Request) {
if enable {
w.Header().Set(strictTransportSecurityHeader, fmt.Sprintf("max-age=%d", int64(strictTransportSecurityMaxAge.Seconds())))
}

next.ServeHTTP(w, r)
})
}
}
52 changes: 52 additions & 0 deletions coderd/httpmw/stricttransportsecurity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
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"
)

const (
strictTransportSecurityHeader = "Strict-Transport-Security"
strictTransportSecurityMaxAge = time.Hour * 24 * 365
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably be in the test function scope. If they aren't used globally between tests, it could be confusing.


func TestStrictTransportSecurity(t *testing.T) {
t.Parallel()

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), "")
})
}
1 change: 1 addition & 0 deletions coderd/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down