From cffa353ffb76983dc172a9d211158ddbaf0155df Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 5 May 2022 13:12:06 -0700 Subject: [PATCH] Login page styling/structure for users, screen readers, passwd managers Also: - Add CSS to login page - Refactor login page HTML and CSS into a new package - New custom CSP headers for the login page, because the requirements are different from the form_post page --- internal/oidc/auth/auth_handler_test.go | 2 +- .../oidc/callback/callback_handler_test.go | 2 +- internal/oidc/login/get_login_handler.go | 60 ++++------ internal/oidc/login/get_login_handler_test.go | 106 ++++++------------ internal/oidc/login/login_form.gohtml | 39 ------- internal/oidc/login/login_handler.go | 3 +- internal/oidc/login/login_handler_test.go | 4 +- internal/oidc/login/loginhtml/login_form.css | 94 ++++++++++++++++ .../oidc/login/loginhtml/login_form.gohtml | 40 +++++++ internal/oidc/login/loginhtml/loginhtml.go | 65 +++++++++++ .../oidc/login/loginhtml/loginhtml_test.go | 68 +++++++++++ internal/oidc/provider/csp/csp.go | 15 +++ internal/oidc/provider/csp/csp_test.go | 15 +++ .../provider/formposthtml/formposthtml.go | 15 +-- .../formposthtml/formposthtml_test.go | 4 - internal/oidc/provider/manager/manager.go | 2 +- internal/testutil/assertions.go | 20 +++- internal/testutil/loginhtml.go | 68 +++++++++++ 18 files changed, 449 insertions(+), 173 deletions(-) delete mode 100644 internal/oidc/login/login_form.gohtml create mode 100644 internal/oidc/login/loginhtml/login_form.css create mode 100644 internal/oidc/login/loginhtml/login_form.gohtml create mode 100644 internal/oidc/login/loginhtml/loginhtml.go create mode 100644 internal/oidc/login/loginhtml/loginhtml_test.go create mode 100644 internal/oidc/provider/csp/csp.go create mode 100644 internal/oidc/provider/csp/csp_test.go create mode 100644 internal/testutil/loginhtml.go diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 058cb70c..e2b3f000 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -2566,7 +2566,7 @@ func TestAuthorizationEndpoint(t *testing.T) { require.Equal(t, test.wantStatus, rsp.Code) testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType) - testutil.RequireSecurityHeadersWithoutFormPostCSPs(t, rsp) + testutil.RequireSecurityHeadersWithoutCustomCSPs(t, rsp) if test.wantPasswordGrantCall != nil { test.wantPasswordGrantCall.args.Ctx = reqContext diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index e92974d9..d8f08822 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -1034,7 +1034,7 @@ func TestCallbackEndpoint(t *testing.T) { t.Logf("response: %#v", rsp) t.Logf("response body: %q", rsp.Body.String()) - testutil.RequireSecurityHeadersWithFormPostCSPs(t, rsp) + testutil.RequireSecurityHeadersWithFormPostPageCSPs(t, rsp) if test.wantAuthcodeExchangeCall != nil { test.wantAuthcodeExchangeCall.args.Ctx = reqContext diff --git a/internal/oidc/login/get_login_handler.go b/internal/oidc/login/get_login_handler.go index a34f487c..3e33c937 100644 --- a/internal/oidc/login/get_login_handler.go +++ b/internal/oidc/login/get_login_handler.go @@ -4,53 +4,39 @@ package login import ( - _ "embed" - "html/template" "net/http" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/login/loginhtml" ) -const defaultErrorMessage = "An internal error occurred. Please contact your administrator for help." - -var ( - //go:embed login_form.gohtml - rawHTMLTemplate string - - errorMappings = map[string]string{ - "login_error": "Incorrect username or password.", - } +const ( + internalErrorMessage = "An internal error occurred. Please contact your administrator for help." + incorrectUsernameOrPasswordErrorMessage = "Incorrect username or password." ) -type PageData struct { - State string - IDPName string - HasAlertError bool - AlertMessage string - Title string - PostPath string -} - -func NewGetHandler(upstreamIDPs oidc.UpstreamIdentityProvidersLister) HandlerFunc { - var parsedHTMLTemplate = template.Must(template.New("login_post.gohtml").Parse(rawHTMLTemplate)) +func NewGetHandler() HandlerFunc { return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error { - alertError := r.URL.Query().Get("err") - message := errorMappings[alertError] - if message == "" { - message = defaultErrorMessage - } - err := parsedHTMLTemplate.Execute(w, &PageData{ + alertMessage, hasAlert := getAlert(r) + + pageInputs := &loginhtml.PageData{ + PostPath: r.URL.Path, // the path for POST is the same as for GET State: encodedState, IDPName: decodedState.UpstreamName, - HasAlertError: alertError != "", - AlertMessage: message, - Title: "Pinniped", - PostPath: r.URL.Path, // the path for POST is the same as for GET - }) - if err != nil { - return err + HasAlertError: hasAlert, + AlertMessage: alertMessage, } - - return nil + return loginhtml.Template().Execute(w, pageInputs) } } + +func getAlert(r *http.Request) (string, bool) { + errorParamValue := r.URL.Query().Get(errParamName) + + message := internalErrorMessage + if errorParamValue == string(ShowBadUserPassErr) { + message = incorrectUsernameOrPasswordErrorMessage + } + + return message, errorParamValue != "" +} diff --git a/internal/oidc/login/get_login_handler_test.go b/internal/oidc/login/get_login_handler_test.go index 484ee450..472148d5 100644 --- a/internal/oidc/login/get_login_handler_test.go +++ b/internal/oidc/login/get_login_handler_test.go @@ -4,21 +4,23 @@ package login import ( - "fmt" "net/http" "net/http/httptest" "testing" - "go.pinniped.dev/internal/testutil" - "github.com/stretchr/testify/require" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/login/loginhtml" + "go.pinniped.dev/internal/testutil" ) func TestGetLogin(t *testing.T) { const ( - happyLdapIDPName = "some-ldap-idp" + testPath = "/some/path/login" + testUpstreamName = "some-ldap-idp" + testUpstreamType = "ldap" + testEncodedState = "fake-encoded-state-value" ) tests := []struct { @@ -34,63 +36,57 @@ func TestGetLogin(t *testing.T) { { name: "Happy path ldap", decodedState: &oidc.UpstreamStateParamData{ - UpstreamName: happyLdapIDPName, - UpstreamType: "ldap", + UpstreamName: testUpstreamName, + UpstreamType: testUpstreamType, }, - encodedState: "foo", // the encoded and decoded state don't match, but that verification is handled one level up. + encodedState: testEncodedState, // the encoded and decoded state don't match, but that verification is handled one level up. wantStatus: http.StatusOK, wantContentType: htmlContentType, - wantBody: getHTMLResult(""), + wantBody: testutil.ExpectedLoginPageHTML(loginhtml.CSS(), testUpstreamName, testPath, testEncodedState, ""), // no alert message }, { name: "displays error banner when err=login_error param is sent", decodedState: &oidc.UpstreamStateParamData{ - UpstreamName: happyLdapIDPName, - UpstreamType: "ldap", + UpstreamName: testUpstreamName, + UpstreamType: testUpstreamType, }, - encodedState: "foo", + encodedState: testEncodedState, errParam: "login_error", wantStatus: http.StatusOK, wantContentType: htmlContentType, - wantBody: getHTMLResult(` -
- Incorrect username or password. -
-`), + wantBody: testutil.ExpectedLoginPageHTML(loginhtml.CSS(), testUpstreamName, testPath, testEncodedState, + "Incorrect username or password.", + ), }, { name: "displays error banner when err=internal_error param is sent", decodedState: &oidc.UpstreamStateParamData{ - UpstreamName: happyLdapIDPName, - UpstreamType: "ldap", + UpstreamName: testUpstreamName, + UpstreamType: testUpstreamType, }, - encodedState: "foo", + encodedState: testEncodedState, errParam: "internal_error", wantStatus: http.StatusOK, wantContentType: htmlContentType, - wantBody: getHTMLResult(` -
- An internal error occurred. Please contact your administrator for help. -
-`), + wantBody: testutil.ExpectedLoginPageHTML(loginhtml.CSS(), testUpstreamName, testPath, testEncodedState, + "An internal error occurred. Please contact your administrator for help.", + ), }, - // If we get an error that we don't recognize, that's also an error, so we - // should probably just tell you to contact your administrator... { + // If we get an error that we don't recognize, that's also an error, so we + // should probably just tell you to contact your administrator... name: "displays generic error banner when unrecognized err param is sent", decodedState: &oidc.UpstreamStateParamData{ - UpstreamName: happyLdapIDPName, - UpstreamType: "ldap", + UpstreamName: testUpstreamName, + UpstreamType: testUpstreamType, }, - encodedState: "foo", + encodedState: testEncodedState, errParam: "some_other_error", wantStatus: http.StatusOK, wantContentType: htmlContentType, - wantBody: getHTMLResult(` -
- An internal error occurred. Please contact your administrator for help. -
-`), + wantBody: testutil.ExpectedLoginPageHTML(loginhtml.CSS(), testUpstreamName, testPath, testEncodedState, + "An internal error occurred. Please contact your administrator for help.", + ), }, } @@ -100,8 +96,8 @@ func TestGetLogin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - handler := NewGetHandler(tt.idps) - target := "/some/path/login?state=" + tt.encodedState + handler := NewGetHandler() + target := testPath + "?state=" + tt.encodedState if tt.errParam != "" { target += "&err=" + tt.errParam } @@ -113,44 +109,8 @@ func TestGetLogin(t *testing.T) { require.Equal(t, tt.wantStatus, rsp.Code) testutil.RequireEqualContentType(t, rsp.Header().Get("Content-Type"), tt.wantContentType) body := rsp.Body.String() + // t.Log("actual body:", body) // useful when updating expected values require.Equal(t, tt.wantBody, body) }) } } - -func getHTMLResult(errorBanner string) string { - happyGetResult := ` - - - Pinniped - - - -

Pinniped

-

some-ldap-idp

-%s -
- -
- - -
- -
- - -
- -
- -
- - - -
- - - -` - return fmt.Sprintf(happyGetResult, errorBanner) -} diff --git a/internal/oidc/login/login_form.gohtml b/internal/oidc/login/login_form.gohtml deleted file mode 100644 index 6dd4819d..00000000 --- a/internal/oidc/login/login_form.gohtml +++ /dev/null @@ -1,39 +0,0 @@ - - - - {{.Title}} - - - -

Pinniped

-

{{ .IDPName }}

-{{if .HasAlertError}} -
- {{.AlertMessage}} -
-{{end}} -
- -
- - -
- -
- - -
- -
- -
- - - -
- - - diff --git a/internal/oidc/login/login_handler.go b/internal/oidc/login/login_handler.go index eb9d8251..06444bc1 100644 --- a/internal/oidc/login/login_handler.go +++ b/internal/oidc/login/login_handler.go @@ -11,6 +11,7 @@ import ( "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/login/loginhtml" "go.pinniped.dev/internal/oidc/provider/formposthtml" "go.pinniped.dev/internal/plog" ) @@ -84,7 +85,7 @@ func NewHandler( func wrapSecurityHeaders(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - wrapped := securityheader.Wrap(handler) + wrapped := securityheader.WrapWithCustomCSP(handler, loginhtml.ContentSecurityPolicy()) if r.Method == http.MethodPost { // POST requests can result in the form_post html page, so allow it with CSP headers. wrapped = securityheader.WrapWithCustomCSP(handler, formposthtml.ContentSecurityPolicy()) diff --git a/internal/oidc/login/login_handler_test.go b/internal/oidc/login/login_handler_test.go index 79a0ee65..11380950 100644 --- a/internal/oidc/login/login_handler_test.go +++ b/internal/oidc/login/login_handler_test.go @@ -417,9 +417,9 @@ func TestLoginEndpoint(t *testing.T) { subject.ServeHTTP(rsp, req) if tt.method == http.MethodPost { - testutil.RequireSecurityHeadersWithFormPostCSPs(t, rsp) + testutil.RequireSecurityHeadersWithFormPostPageCSPs(t, rsp) } else { - testutil.RequireSecurityHeadersWithoutFormPostCSPs(t, rsp) + testutil.RequireSecurityHeadersWithLoginPageCSPs(t, rsp) } require.Equal(t, tt.wantStatus, rsp.Code) diff --git a/internal/oidc/login/loginhtml/login_form.css b/internal/oidc/login/loginhtml/login_form.css new file mode 100644 index 00000000..5eba47e0 --- /dev/null +++ b/internal/oidc/login/loginhtml/login_form.css @@ -0,0 +1,94 @@ +/* Copyright 2022 the Pinniped contributors. All Rights Reserved. */ +/* SPDX-License-Identifier: Apache-2.0 */ + +html { + height: 100%; +} + +body { + font-family: "Metropolis-Light", Helvetica, sans-serif; + display: flex; + flex-flow: column wrap; + justify-content: flex-start; + align-items: center; + /* subtle gradient make the login box stand out */ + background: linear-gradient(to top, #f8f8f8, white); + min-height: 100%; +} + +h1 { + font-size: 20px; + margin: 0; +} + +.box { + display: flex; + flex-direction: column; + flex-wrap: nowrap; + border-radius: 4px; + border-color: #ddd; + border-width: 1px; + border-style: solid; + width: 400px; + padding:30px 30px 0; + margin: 60px 20px 0; + background: white; + font-size: 14px; +} + +input { + color: inherit; + font: inherit; + border: 0; + margin: 0; + outline: 0; + padding: 0; +} + +.form-field { + display: flex; + margin-bottom: 30px; +} + +.form-field input[type="password"], .form-field input[type="text"], .form-field input[type="submit"] { + width: 100%; + padding: 1em; +} + +.form-field input[type="password"], .form-field input[type="text"] { + border-radius: 3px; + border-width: 1px; + border-style: solid; + border-color: #a6a6a6; +} + +.form-field input[type="submit"] { + background-color: #218fcf; /* this is a color from the Pinniped logo :) */ + color: #eee; + font-weight: bold; + cursor: pointer; + transition: all .3s; +} + +.form-field input[type="submit"]:focus, .form-field input[type="submit"]:hover { + background-color: #1abfd3; /* this is a color from the Pinniped logo :) */ +} + +.form-field input[type="submit"]:active { + transform: scale(.99); +} + +.hidden { + border: 0; + clip: rect(0 0 0 0); + height: 1px; + margin: -1px; + overflow: hidden; + padding: 0; + position: absolute; + width: 1px; +} + +.alert { + color: crimson; +} diff --git a/internal/oidc/login/loginhtml/login_form.gohtml b/internal/oidc/login/loginhtml/login_form.gohtml new file mode 100644 index 00000000..a92e406f --- /dev/null +++ b/internal/oidc/login/loginhtml/login_form.gohtml @@ -0,0 +1,40 @@ + + + + Pinniped + + + + + +
+
+

Log in to {{.IDPName}}

+
+ {{if .HasAlertError}} +
+ {{.AlertMessage}} +
+ {{end}} +
+ +
+ + +
+
+ + +
+
+ +
+
+
+ + diff --git a/internal/oidc/login/loginhtml/loginhtml.go b/internal/oidc/login/loginhtml/loginhtml.go new file mode 100644 index 00000000..1493979f --- /dev/null +++ b/internal/oidc/login/loginhtml/loginhtml.go @@ -0,0 +1,65 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package loginhtml defines HTML templates used by the Supervisor. +//nolint: gochecknoglobals // This package uses globals to ensure that all parsing and minifying happens at init. +package loginhtml + +import ( + _ "embed" // Needed to trigger //go:embed directives below. + "html/template" + "strings" + + "github.com/tdewolff/minify/v2/minify" + + "go.pinniped.dev/internal/oidc/provider/csp" +) + +var ( + //go:embed login_form.css + rawCSS string + minifiedCSS = mustMinify(minify.CSS(rawCSS)) + + //go:embed login_form.gohtml + rawHTMLTemplate string +) + +// Parse the Go templated HTML and inject functions providing the minified inline CSS and JS. +var parsedHTMLTemplate = template.Must(template.New("login_form.gohtml").Funcs(template.FuncMap{ + "minifiedCSS": func() template.CSS { return template.CSS(minifiedCSS) }, +}).Parse(rawHTMLTemplate)) + +// Generate the CSP header value once since it's effectively constant. +var cspValue = strings.Join([]string{ + `default-src 'none'`, + `style-src '` + csp.Hash(minifiedCSS) + `'`, + `frame-ancestors 'none'`, +}, "; ") + +func mustMinify(s string, err error) string { + if err != nil { + panic(err) + } + return s +} + +// ContentSecurityPolicy returns the Content-Security-Policy header value to make the Template() operate correctly. +// +// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy. +func ContentSecurityPolicy() string { return cspValue } + +// Template returns the html/template.Template for rendering the login page. +func Template() *template.Template { return parsedHTMLTemplate } + +// CSS returns the minified CSS that will be embedded into the page template. +func CSS() string { return minifiedCSS } + +// PageData represents the inputs to the template. +type PageData struct { + State string + IDPName string + HasAlertError bool + AlertMessage string + MinifiedCSS template.CSS + PostPath string +} diff --git a/internal/oidc/login/loginhtml/loginhtml_test.go b/internal/oidc/login/loginhtml/loginhtml_test.go new file mode 100644 index 00000000..a2e91ed1 --- /dev/null +++ b/internal/oidc/login/loginhtml/loginhtml_test.go @@ -0,0 +1,68 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package loginhtml + +import ( + "bytes" + "fmt" + "testing" + + "go.pinniped.dev/internal/testutil" + + "github.com/stretchr/testify/require" +) + +var ( + testExpectedCSS = `html{height:100%}body{font-family:metropolis-light,Helvetica,sans-serif;display:flex;flex-flow:column wrap;justify-content:flex-start;align-items:center;background:linear-gradient(to top,#f8f8f8,white);min-height:100%}h1{font-size:20px;margin:0}.box{display:flex;flex-direction:column;flex-wrap:nowrap;border-radius:4px;border-color:#ddd;border-width:1px;border-style:solid;width:400px;padding:30px 30px 0;margin:60px 20px 0;background:#fff;font-size:14px}input{color:inherit;font:inherit;border:0;margin:0;outline:0;padding:0}.form-field{display:flex;margin-bottom:30px}.form-field input[type=password],.form-field input[type=text],.form-field input[type=submit]{width:100%;padding:1em}.form-field input[type=password],.form-field input[type=text]{border-radius:3px;border-width:1px;border-style:solid;border-color:#a6a6a6}.form-field input[type=submit]{background-color:#218fcf;color:#eee;font-weight:700;cursor:pointer;transition:all .3s}.form-field input[type=submit]:focus,.form-field input[type=submit]:hover{background-color:#1abfd3}.form-field input[type=submit]:active{transform:scale(.99)}.hidden{border:0;clip:rect(0 0 0 0);height:1px;margin:-1px;overflow:hidden;padding:0;position:absolute;width:1px}.alert{color:crimson}` + + // It's okay if this changes in the future, but this gives us a chance to eyeball the formatting. + // Our browser-based integration tests should find any incompatibilities. + testExpectedCSP = `default-src 'none'; ` + + `style-src 'sha256-QC9ckaUFAdcN0Ysmu8q8iqCazYFgrJSQDJPa/przPXU='; ` + + `frame-ancestors 'none'` +) + +func TestTemplate(t *testing.T) { + const ( + testUpstreamName = "test-idp-name" + testPath = "test-post-path" + testEncodedState = "test-encoded-state" + testAlert = "test-alert-message" + ) + + var buf bytes.Buffer + pageInputs := &PageData{ + PostPath: testPath, + State: testEncodedState, + IDPName: testUpstreamName, + HasAlertError: true, + AlertMessage: testAlert, + } + + // Render with an alert. + expectedHTMLWithAlert := testutil.ExpectedLoginPageHTML(testExpectedCSS, testUpstreamName, testPath, testEncodedState, testAlert) + require.NoError(t, Template().Execute(&buf, pageInputs)) + // t.Logf("actual value:\n%s", buf.String()) // useful when updating minify library causes new output + require.Equal(t, expectedHTMLWithAlert, buf.String()) + + // Render again without an alert. + pageInputs.HasAlertError = false + expectedHTMLWithoutAlert := testutil.ExpectedLoginPageHTML(testExpectedCSS, testUpstreamName, testPath, testEncodedState, "") + buf = bytes.Buffer{} // clear previous result from buffer + require.NoError(t, Template().Execute(&buf, pageInputs)) + require.Equal(t, expectedHTMLWithoutAlert, buf.String()) +} + +func TestContentSecurityPolicy(t *testing.T) { + require.Equal(t, testExpectedCSP, ContentSecurityPolicy()) +} + +func TestCSS(t *testing.T) { + require.Equal(t, testExpectedCSS, CSS()) +} + +func TestHelpers(t *testing.T) { + require.Equal(t, "test", mustMinify("test", nil)) + require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) }) +} diff --git a/internal/oidc/provider/csp/csp.go b/internal/oidc/provider/csp/csp.go new file mode 100644 index 00000000..d3f97e50 --- /dev/null +++ b/internal/oidc/provider/csp/csp.go @@ -0,0 +1,15 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package csp defines helpers related to HTML Content Security Policies. +package csp + +import ( + "crypto/sha256" + "encoding/base64" +) + +func Hash(s string) string { + hashBytes := sha256.Sum256([]byte(s)) + return "sha256-" + base64.StdEncoding.EncodeToString(hashBytes[:]) +} diff --git a/internal/oidc/provider/csp/csp_test.go b/internal/oidc/provider/csp/csp_test.go new file mode 100644 index 00000000..746d5822 --- /dev/null +++ b/internal/oidc/provider/csp/csp_test.go @@ -0,0 +1,15 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package csp + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHash(t *testing.T) { + // Example test vector from https://content-security-policy.com/hash/. + require.Equal(t, "sha256-RFWPLDbv2BY+rCkDzsE+0fr8ylGr2R2faWMhq4lfEQc=", Hash("doSomething();")) +} diff --git a/internal/oidc/provider/formposthtml/formposthtml.go b/internal/oidc/provider/formposthtml/formposthtml.go index 6552c9a1..b96f0d5d 100644 --- a/internal/oidc/provider/formposthtml/formposthtml.go +++ b/internal/oidc/provider/formposthtml/formposthtml.go @@ -6,13 +6,13 @@ package formposthtml import ( - "crypto/sha256" _ "embed" // Needed to trigger //go:embed directives below. - "encoding/base64" "html/template" "strings" "github.com/tdewolff/minify/v2/minify" + + "go.pinniped.dev/internal/oidc/provider/csp" ) var ( @@ -37,8 +37,8 @@ var parsedHTMLTemplate = template.Must(template.New("form_post.gohtml").Funcs(te // Generate the CSP header value once since it's effectively constant. var cspValue = strings.Join([]string{ `default-src 'none'`, - `script-src '` + cspHash(minifiedJS) + `'`, - `style-src '` + cspHash(minifiedCSS) + `'`, + `script-src '` + csp.Hash(minifiedJS) + `'`, + `style-src '` + csp.Hash(minifiedCSS) + `'`, `img-src data:`, `connect-src *`, `frame-ancestors 'none'`, @@ -51,14 +51,9 @@ func mustMinify(s string, err error) string { return s } -func cspHash(s string) string { - hashBytes := sha256.Sum256([]byte(s)) - return "sha256-" + base64.StdEncoding.EncodeToString(hashBytes[:]) -} - // ContentSecurityPolicy returns the Content-Security-Policy header value to make the Template() operate correctly. // -// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src#:~:text=%27%3Chash-algorithm%3E-%3Cbase64-value%3E%27. +// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy. func ContentSecurityPolicy() string { return cspValue } // Template returns the html/template.Template for rendering the response_type=form_post response page. diff --git a/internal/oidc/provider/formposthtml/formposthtml_test.go b/internal/oidc/provider/formposthtml/formposthtml_test.go index 07fb508a..d5d69c9d 100644 --- a/internal/oidc/provider/formposthtml/formposthtml_test.go +++ b/internal/oidc/provider/formposthtml/formposthtml_test.go @@ -93,10 +93,6 @@ func TestContentSecurityPolicyHashes(t *testing.T) { } func TestHelpers(t *testing.T) { - // These are silly tests but it's easy to we might as well have them. require.Equal(t, "test", mustMinify("test", nil)) require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) }) - - // Example test vector from https://content-security-policy.com/hash/. - require.Equal(t, "sha256-RFWPLDbv2BY+rCkDzsE+0fr8ylGr2R2faWMhq4lfEQc=", cspHash("doSomething();")) } diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 3da0c2c3..ffa33139 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -139,7 +139,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs m.providerHandlers[(issuerHostWithPath + oidc.PinnipedLoginPath)] = login.NewHandler( upstreamStateEncoder, csrfCookieEncoder, - login.NewGetHandler(m.upstreamIDPs), + login.NewGetHandler(), login.NewPostHandler(issuer, m.upstreamIDPs, oauthHelperWithKubeStorage), ) diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index b592e07e..ee7bc2ed 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -54,7 +54,7 @@ func RequireNumberOfSecretsMatchingLabelSelector(t *testing.T, secrets v1.Secret require.Len(t, storedAuthcodeSecrets.Items, expectedNumberOfSecrets) } -func RequireSecurityHeadersWithFormPostCSPs(t *testing.T, response *httptest.ResponseRecorder) { +func RequireSecurityHeadersWithFormPostPageCSPs(t *testing.T, response *httptest.ResponseRecorder) { // Loosely confirm that the unique CSPs needed for the form_post page were used. cspHeader := response.Header().Get("Content-Security-Policy") require.Contains(t, cspHeader, "script-src '") // loose assertion @@ -66,8 +66,20 @@ func RequireSecurityHeadersWithFormPostCSPs(t *testing.T, response *httptest.Res requireSecurityHeaders(t, response) } -func RequireSecurityHeadersWithoutFormPostCSPs(t *testing.T, response *httptest.ResponseRecorder) { - // Confirm that the unique CSPs needed for the form_post page were NOT used. +func RequireSecurityHeadersWithLoginPageCSPs(t *testing.T, response *httptest.ResponseRecorder) { + // Loosely confirm that the unique CSPs needed for the login page were used. + cspHeader := response.Header().Get("Content-Security-Policy") + require.Contains(t, cspHeader, "style-src '") // loose assertion + require.NotContains(t, cspHeader, "script-src") // only needed by form_post page + require.NotContains(t, cspHeader, "img-src data:") // only needed by form_post page + require.NotContains(t, cspHeader, "connect-src *") // only needed by form_post page + + // Also require all the usual security headers. + requireSecurityHeaders(t, response) +} + +func RequireSecurityHeadersWithoutCustomCSPs(t *testing.T, response *httptest.ResponseRecorder) { + // Confirm that the unique CSPs needed for the form_post or login page were NOT used. cspHeader := response.Header().Get("Content-Security-Policy") require.NotContains(t, cspHeader, "script-src") require.NotContains(t, cspHeader, "style-src") @@ -79,7 +91,7 @@ func RequireSecurityHeadersWithoutFormPostCSPs(t *testing.T, response *httptest. } func requireSecurityHeaders(t *testing.T, response *httptest.ResponseRecorder) { - // Loosely confirm that the generic CSPs were used. + // Loosely confirm that the generic default CSPs were used. cspHeader := response.Header().Get("Content-Security-Policy") require.Contains(t, cspHeader, "default-src 'none'") require.Contains(t, cspHeader, "frame-ancestors 'none'") diff --git a/internal/testutil/loginhtml.go b/internal/testutil/loginhtml.go new file mode 100644 index 00000000..431f708d --- /dev/null +++ b/internal/testutil/loginhtml.go @@ -0,0 +1,68 @@ +// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "fmt" + + "go.pinniped.dev/internal/here" +) + +func ExpectedLoginPageHTML(wantCSS, wantIDPName, wantPostPath, wantEncodedState, wantAlert string) string { + alertHTML := "" + if wantAlert != "" { + alertHTML = fmt.Sprintf("\n"+ + "
\n"+ + " %s\n"+ + "
\n ", + wantAlert, + ) + } + + // Note that "role", "aria-*", and "alert" attributes are hints to screen readers. + // Also note that some structure and attributes used here are hints to password managers, + // see https://support.1password.com/compatible-website-design/. + // Please take care when changing the HTML of this form, + // and test with a screen reader and password manager after changes. + return here.Docf(` + + + Pinniped + + + + + +
+
+

Log in to %s

+
+ %s +
+ +
+ + +
+
+ + +
+
+ +
+
+
+ + + `, + wantCSS, + wantIDPName, + alertHTML, + wantPostPath, + wantEncodedState, + ) +}