diff --git a/Dockerfile b/Dockerfile index be74db5f..73c50799 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,7 @@ # Copyright 2020 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -FROM golang:1.15.5 as build-env +FROM golang:1.15.6 as build-env WORKDIR /work # Get dependencies first so they can be cached as a layer diff --git a/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl index 9be04701..09f74c7c 100644 --- a/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index 5af68c1e..75d13c3e 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -462,7 +462,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== diff --git a/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } diff --git a/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index f0bca6a4..01a0c718 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -462,7 +462,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== diff --git a/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } diff --git a/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index ec9176fe..a61e1073 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -462,7 +462,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== diff --git a/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } diff --git a/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/internal/httputil/securityheader/securityheader.go b/internal/httputil/securityheader/securityheader.go index 42cf1e2f..2bb3af12 100644 --- a/internal/httputil/securityheader/securityheader.go +++ b/internal/httputil/securityheader/securityheader.go @@ -9,7 +9,6 @@ import "net/http" // Wrap the provided http.Handler so it sets appropriate security-related response headers. func Wrap(wrapped http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - wrapped.ServeHTTP(w, r) h := w.Header() h.Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'") h.Set("X-Frame-Options", "DENY") @@ -17,14 +16,9 @@ func Wrap(wrapped http.Handler) http.Handler { h.Set("X-Content-Type-Options", "nosniff") h.Set("Referrer-Policy", "no-referrer") h.Set("X-DNS-Prefetch-Control", "off") - - // first overwrite existing Cache-Control header with Set, then append more headers with Add - h.Set("Cache-Control", "no-cache") - h.Add("Cache-Control", "no-store") - h.Add("Cache-Control", "max-age=0") - h.Add("Cache-Control", "must-revalidate") - + h.Set("Cache-Control", "no-cache,no-store,max-age=0,must-revalidate") h.Set("Pragma", "no-cache") h.Set("Expires", "0") + wrapped.ServeHTTP(w, r) }) } diff --git a/internal/httputil/securityheader/securityheader_test.go b/internal/httputil/securityheader/securityheader_test.go index 715e7cd5..a0688c1a 100644 --- a/internal/httputil/securityheader/securityheader_test.go +++ b/internal/httputil/securityheader/securityheader_test.go @@ -4,22 +4,40 @@ package securityheader import ( + "context" + "io/ioutil" "net/http" "net/http/httptest" "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestWrap(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + testServer := httptest.NewServer(Wrap(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Test-Header", "test value") _, _ = w.Write([]byte("hello world")) - }) - rec := httptest.NewRecorder() - Wrap(handler).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) - require.Equal(t, http.StatusOK, rec.Code) - require.Equal(t, "hello world", rec.Body.String()) - require.EqualValues(t, http.Header{ + }))) + t.Cleanup(testServer.Close) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, testServer.URL, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + respBody, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "hello world", string(respBody)) + + expected := http.Header{ + "X-Test-Header": []string{"test value"}, "Content-Security-Policy": []string{"default-src 'none'; frame-ancestors 'none'"}, "Content-Type": []string{"text/plain; charset=utf-8"}, "Referrer-Policy": []string{"no-referrer"}, @@ -27,8 +45,11 @@ func TestWrap(t *testing.T) { "X-Frame-Options": []string{"DENY"}, "X-Xss-Protection": []string{"1; mode=block"}, "X-Dns-Prefetch-Control": []string{"off"}, - "Cache-Control": []string{"no-cache", "no-store", "max-age=0", "must-revalidate"}, + "Cache-Control": []string{"no-cache,no-store,max-age=0,must-revalidate"}, "Pragma": []string{"no-cache"}, "Expires": []string{"0"}, - }, rec.Header()) + } + for key, values := range expected { + assert.Equalf(t, values, resp.Header.Values(key), "unexpected values for header %s", key) + } } diff --git a/internal/testutil/assertions.go b/internal/testutil/assertions.go index b0c3018d..54fc8563 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -61,7 +61,9 @@ func RequireSecurityHeaders(t *testing.T, response *httptest.ResponseRecorder) { require.Equal(t, "nosniff", response.Header().Get("X-Content-Type-Options")) require.Equal(t, "no-referrer", response.Header().Get("Referrer-Policy")) require.Equal(t, "off", response.Header().Get("X-DNS-Prefetch-Control")) - require.ElementsMatch(t, []string{"no-cache", "no-store", "max-age=0", "must-revalidate"}, response.Header().Values("Cache-Control")) require.Equal(t, "no-cache", response.Header().Get("Pragma")) require.Equal(t, "0", response.Header().Get("Expires")) + + // This check is more relaxed since Fosite can override the base header we set. + require.Contains(t, response.Header().Get("Cache-Control"), "no-store") } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 4d92a452..5ae1bf0c 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -37,9 +37,13 @@ const ( // API operation. minIDTokenValidity = 10 * time.Minute - // refreshTimeout is the amount of time allotted for OAuth2 refresh operations. Since these don't involve any - // user interaction, they should always be roughly as fast as network latency. - refreshTimeout = 30 * time.Second + // httpRequestTimeout is the timeout for operations that involve one (or a few) non-interactive HTTPS requests. + // Since these don't involve any user interaction, they should always be roughly as fast as network latency. + httpRequestTimeout = 60 * time.Second + + // overallTimeout is the overall time that a login is allowed to take. This includes several user interactions, so + // we set this to be relatively long. + overallTimeout = 90 * time.Minute ) type handlerState struct { @@ -198,8 +202,13 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er } } + // Copy the configured HTTP client to set a request timeout (the Go default client has no timeout configured). + httpClientWithTimeout := *h.httpClient + httpClientWithTimeout.Timeout = httpRequestTimeout + h.httpClient = &httpClientWithTimeout + // Always set a long, but non-infinite timeout for this operation. - ctx, cancel := context.WithTimeout(h.ctx, 10*time.Minute) + ctx, cancel := context.WithTimeout(h.ctx, overallTimeout) defer cancel() ctx = oidc.ClientContext(ctx, h.httpClient) h.ctx = ctx @@ -404,8 +413,6 @@ func (h *handlerState) tokenExchangeRFC8693(baseToken *oidctypes.Token) (*oidcty } func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctypes.RefreshToken) (*oidctypes.Token, error) { - ctx, cancel := context.WithTimeout(ctx, refreshTimeout) - defer cancel() refreshSource := h.oauth2Config.TokenSource(ctx, &oauth2.Token{RefreshToken: refreshToken.Token}) refreshed, err := refreshSource.Token() @@ -473,7 +480,7 @@ func (h *handlerState) serve(listener net.Listener) func() { return func() { // Gracefully shut down the server, allowing up to 5 seconds for // clients to receive any in-flight responses. - shutdownCtx, cancel := context.WithTimeout(h.ctx, 1*time.Second) + shutdownCtx, cancel := context.WithTimeout(h.ctx, 5*time.Second) _ = srv.Shutdown(shutdownCtx) cancel() } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index ed4ca1eb..26b927ff 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -57,19 +57,25 @@ func TestSupervisorLogin(t *testing.T) { require.NoError(t, err) // Create an HTTP client that can reach the downstream discovery endpoint using the CA certs. - httpClient := &http.Client{Transport: &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: ca.Pool()}, - Proxy: func(req *http.Request) (*url.URL, error) { - if env.Proxy == "" { - t.Logf("passing request for %s with no proxy", req.URL) - return nil, nil - } - proxyURL, err := url.Parse(env.Proxy) - require.NoError(t, err) - t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) - return proxyURL, nil + httpClient := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{RootCAs: ca.Pool()}, + Proxy: func(req *http.Request) (*url.URL, error) { + if env.Proxy == "" { + t.Logf("passing request for %s with no proxy", req.URL) + return nil, nil + } + proxyURL, err := url.Parse(env.Proxy) + require.NoError(t, err) + t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) + return proxyURL, nil + }, }, - }} + // Don't follow redirects automatically. + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + } oidcHTTPClientContext := coreosoidc.ClientContext(ctx, httpClient) // Use the CA to issue a TLS server cert. @@ -144,6 +150,14 @@ func TestSupervisorLogin(t *testing.T) { pkceParam.Method(), ) + // Make the authorize request one "manually" so we can check its response headers. + authorizeRequest, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthorizeURL, nil) + require.NoError(t, err) + authorizeResp, err := httpClient.Do(authorizeRequest) + require.NoError(t, err) + require.NoError(t, authorizeResp.Body.Close()) + expectSecurityHeaders(t, authorizeResp) + // Open the web browser and navigate to the downstream authorize URL. page := browsertest.Open(t) t.Logf("opening browser to downstream authorize URL %s", library.MaskTokens(downstreamAuthorizeURL)) @@ -302,3 +316,16 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. require.NoError(t, err) t.Logf("exchanged token claims:\n%s", string(indentedClaims)) } + +func expectSecurityHeaders(t *testing.T, response *http.Response) { + h := response.Header + assert.Equal(t, "default-src 'none'; frame-ancestors 'none'", h.Get("Content-Security-Policy")) + assert.Equal(t, "DENY", h.Get("X-Frame-Options")) + assert.Equal(t, "1; mode=block", h.Get("X-XSS-Protection")) + assert.Equal(t, "nosniff", h.Get("X-Content-Type-Options")) + assert.Equal(t, "no-referrer", h.Get("Referrer-Policy")) + assert.Equal(t, "off", h.Get("X-DNS-Prefetch-Control")) + assert.Equal(t, "no-cache,no-store,max-age=0,must-revalidate", h.Get("Cache-Control")) + assert.Equal(t, "no-cache", h.Get("Pragma")) + assert.Equal(t, "0", h.Get("Expires")) +}