From 404ff93102a2813010dfac641c6d6bdd34e4dd39 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 15 Dec 2020 21:52:07 -0600 Subject: [PATCH 1/7] Fix documentation comment for the UpstreamOIDCProvider's spec.client.secretName type. The value is correctly validated as `secrets.pinniped.dev/oidc-client` elsewhere, only this comment was wrong. Signed-off-by: Matt Moyer --- apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl | 2 +- .../idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 2 +- generated/1.17/README.adoc | 2 +- .../apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 2 +- .../crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 2 +- generated/1.18/README.adoc | 2 +- .../apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 2 +- .../crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 2 +- generated/1.19/README.adoc | 2 +- .../apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go | 2 +- .../crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml | 2 +- 11 files changed, 11 insertions(+), 11 deletions(-) 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 0ccdd1df..1303b2a4 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -442,7 +442,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 97042b25..7c3dda8f 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -442,7 +442,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 edda33b8..d4d0c8b1 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -442,7 +442,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: From a33dace80bfc4c555e7bc3bc0a4425a07407001d Mon Sep 17 00:00:00 2001 From: Aram Price Date: Wed, 16 Dec 2020 13:31:54 -0500 Subject: [PATCH 2/7] Upgrade golang (1.15.5 -> 1.15.6) Signed-off-by: Andrew Keesler --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 602f3c59babdb7090771c4b61fe8d9db3cd1eed1 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 15 Dec 2020 21:34:34 -0600 Subject: [PATCH 3/7] Fix a regression in securityheader package. The bug itself has to do with when headers are streamed to the client. Once a wrapped handler has sent any bytes to the `http.ResponseWriter`, the value of the map returned from `w.Header()` no longer matters for the response. The fix is fairly trivial, which is to add those response headers before invoking the wrapped handler. The existing unit test didn't catch this due to limitations in `httptest.NewRecorder()`. It is now replaced with a new test that runs a full HTTP test server, which catches the previous bug. Signed-off-by: Matt Moyer --- .../httputil/securityheader/securityheader.go | 3 +- .../securityheader/securityheader_test.go | 37 +++++++++++++++---- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/internal/httputil/securityheader/securityheader.go b/internal/httputil/securityheader/securityheader.go index 42cf1e2f..2def7ede 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") @@ -26,5 +25,7 @@ func Wrap(wrapped http.Handler) http.Handler { 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..e8772f51 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"}, @@ -30,5 +48,8 @@ func TestWrap(t *testing.T) { "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) + } } From 74e52187a3a0bc0579ffc259191ffa9c589c7a02 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 15 Dec 2020 21:38:55 -0600 Subject: [PATCH 4/7] Simplify securityheader package by merging header fields. From RFC2616 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2): > It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, > without changing the semantics of the message, by appending each subsequent field-value to the first, > each separated by a comma. This was correct before, but this simplifes a bit and shaves off a few bytes from the response. Signed-off-by: Matt Moyer --- internal/httputil/securityheader/securityheader.go | 9 +-------- internal/httputil/securityheader/securityheader_test.go | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/internal/httputil/securityheader/securityheader.go b/internal/httputil/securityheader/securityheader.go index 2def7ede..2bb3af12 100644 --- a/internal/httputil/securityheader/securityheader.go +++ b/internal/httputil/securityheader/securityheader.go @@ -16,16 +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 e8772f51..a0688c1a 100644 --- a/internal/httputil/securityheader/securityheader_test.go +++ b/internal/httputil/securityheader/securityheader_test.go @@ -45,7 +45,7 @@ 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"}, } From 24c01d3e5491cffff69fa3cdbc10762d2ca0f3b9 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 15 Dec 2020 21:42:11 -0600 Subject: [PATCH 5/7] Add an integration test to verify security headers on the supervisor authorize endpoint. It would be great to do this for the supervisor's callback endpoint as well, but it's difficult to get at those since the request happens inside the spawned browser. Signed-off-by: Matt Moyer --- test/integration/supervisor_login_test.go | 51 +++++++++++++++++------ 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 6fa7875f..6fc03a7d 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)) @@ -306,3 +320,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")) +} From 3948bb76d870d6cb07d72ca10ccceb153255dece Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 16 Dec 2020 13:15:38 -0600 Subject: [PATCH 6/7] Be more lax in some of our test assertions. Fosite overrides the `Cache-Control` header we set, which is basically fine even though it's not exactly what we want. Signed-off-by: Matt Moyer --- internal/testutil/assertions.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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") } From 01b6bf7850d580fcc44d1ef9c49a43388866b6d2 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Tue, 15 Dec 2020 22:11:49 -0600 Subject: [PATCH 7/7] Tweak timeouts in oidcclient package. - The overall timeout for logins is increased to 90 minutes. - The timeout for token refresh is increased from 30 seconds to 60 seconds to be a bit more tolerant of extremely slow networks. - A new, matching timeout of 60 seconds has been added for the OIDC discovery, auth code exchange, and RFC8693 token exchange operations. The new code uses the `http.Client.Timeout` field rather than managing contexts on individual requests. This is easier because the OIDC package stores a context at creation time and tries to use it later when performing key refresh operations. Signed-off-by: Matt Moyer --- pkg/oidcclient/login.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) 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() }