Merge remote-tracking branch 'upstream/main' into secret-generation

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

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"`
 }

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:

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".
 |===
 
 

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"`
 }

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:

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".
 |===
 
 

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"`
 }

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:

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".
 |===
 
 

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"`
 }

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:

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")
-
+		h.Set("Cache-Control", "no-cache,no-store,max-age=0,must-revalidate")
-		// 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("Pragma", "no-cache")
 		h.Set("Expires", "0")
+		wrapped.ServeHTTP(w, r)
 	})
 }

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()
+	t.Cleanup(testServer.Close)
-	Wrap(handler).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil))
+
-	require.Equal(t, http.StatusOK, rec.Code)
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
-	require.Equal(t, "hello world", rec.Body.String())
+	defer cancel()
-	require.EqualValues(t, http.Header{
+
+	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)
+	}
 }

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")
 }

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
+	// httpRequestTimeout is the timeout for operations that involve one (or a few) non-interactive HTTPS requests.
-	// 	user interaction, they should always be roughly as fast as network latency.
+	// Since these don't involve any user interaction, they should always be roughly as fast as network latency.
-	refreshTimeout = 30 * time.Second
+	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()
 	}

test/integration/supervisor_login_test.go

@@ -57,7 +57,8 @@ 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{
+	httpClient := &http.Client{
+		Transport: &http.Transport{
 			TLSClientConfig: &tls.Config{RootCAs: ca.Pool()},
 			Proxy: func(req *http.Request) (*url.URL, error) {
 				if env.Proxy == "" {
@@ -69,7 +70,12 @@ func TestSupervisorLogin(t *testing.T) {
 				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"))
+}