Fix broken upstream OIDC discovery timeout added in previous commit

After noticing that the upstream OIDC discovery calls can hang
indefinitely, I had tried to impose a one minute timeout on them
by giving them a timeout context. However, I hadn't noticed that the
context also gets passed into the JWKS fetching object, which gets
added to our cache and used later. Therefore the timeout context
was added to the cache and timed out while sitting in the cache,
causing later JWKS fetchers to fail.

This commit is trying again to impose a reasonable timeout on these
discovery and JWKS calls, but this time by using http.Client's Timeout
field, which is documented to be a timeout for *each* request/response
cycle, so hopefully this is a more appropriate way to impose a timeout
for this use case. The http.Client instance ends up in the cache on
the JWKS fetcher object, so the timeout should apply to each JWKS
request as well.

Requests that can hang forever are effectively a server-side resource
leak, which could theoretically be taken advantage of in a denial of
service attempt, so it would be nice to avoid having them.

Dockerfile

@@ -1,7 +1,7 @@
 # Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 # SPDX-License-Identifier: Apache-2.0
 
-FROM golang:1.15.11 as build-env
+FROM golang:1.15.12 as build-env
 
 WORKDIR /work
 # Get dependencies first so they can be cached as a layer

deploy/supervisor/deployment.yaml

@@ -1,4 +1,4 @@
-#! Copyright 2020 the Pinniped contributors. All Rights Reserved.
+#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 #! SPDX-License-Identifier: Apache-2.0
 
 #@ load("@ytt:data", "data")
@@ -101,6 +101,15 @@ spec:
               protocol: TCP
             - containerPort: 8443
               protocol: TCP
+          env:
+            #@ if data.values.https_proxy:
+            - name: HTTPS_PROXY
+              value: #@ data.values.https_proxy
+            #@ end
+            #@ if data.values.no_proxy:
+            - name: NO_PROXY
+              value: #@ data.values.no_proxy
+            #@ end
           livenessProbe:
             httpGet:
               path: /healthz

deploy/supervisor/values.yaml

@@ -1,4 +1,4 @@
-#! Copyright 2020 the Pinniped contributors. All Rights Reserved.
+#! Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 #! SPDX-License-Identifier: Apache-2.0
 
 #@data/values
@@ -56,3 +56,11 @@ service_loadbalancer_ip: #! e.g. 1.2.3.4
 #! Specify the verbosity of logging: info ("nice to know" information), debug (developer
 #! information), trace (timing information), all (kitchen sink).
 log_level: #! By default, when this value is left unset, only warnings and errors are printed. There is no way to suppress warning and error logs.
+
+#! Set the standard golang HTTPS_PROXY and NO_PROXY environment variables on the Supervisor containers.
+#! These will be used when the Supervisor makes backend-to-backend calls to upstream identity providers using HTTPS,
+#! e.g. when the Supervisor fetches discovery documents, JWKS keys, and tokens from an upstream OIDC Provider.
+#! The Supervisor never makes insecure HTTP calls, so there is no reason to set HTTP_PROXY.
+#! Optional.
+https_proxy: #! e.g. http://proxy.example.com
+no_proxy: #! e.g. 127.0.0.1

go.mod

@@ -34,13 +34,13 @@ require (
 	gopkg.in/square/go-jose.v2 v2.5.1
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 // indirect
-	k8s.io/api v0.20.6
-	k8s.io/apimachinery v0.20.6
-	k8s.io/apiserver v0.20.6
-	k8s.io/client-go v0.20.6
-	k8s.io/component-base v0.20.6
+	k8s.io/api v0.20.7
+	k8s.io/apimachinery v0.20.7
+	k8s.io/apiserver v0.20.7
+	k8s.io/client-go v0.20.7
+	k8s.io/component-base v0.20.7
 	k8s.io/klog/v2 v2.4.0
-	k8s.io/kube-aggregator v0.20.6
+	k8s.io/kube-aggregator v0.20.7
 	k8s.io/utils v0.0.0-20201110183641-67b214c5f920
 	sigs.k8s.io/yaml v1.2.0
 )

go.sum

@@ -1504,27 +1504,27 @@ honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt
 honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 howett.net/plist v0.0.0-20181124034731-591f970eefbb/go.mod h1:vMygbs4qMhSZSc4lCUl2OEE+rDiIIJAIdR4m7MiMcm0=
 k8s.io/api v0.20.1/go.mod h1:KqwcCVogGxQY3nBlRpwt+wpAMF/KjaCc7RpywacvqUo=
-k8s.io/api v0.20.6 h1:bgdZrW++LqgrLikWYNruIKAtltXbSCX2l5mJu11hrVE=
-k8s.io/api v0.20.6/go.mod h1:X9e8Qag6JV/bL5G6bU8sdVRltWKmdHsFUGS3eVndqE8=
+k8s.io/api v0.20.7 h1:wOEPJ3NoimUfR9v9sAO2JosPiEP9IGFNplf7zZvYzPU=
+k8s.io/api v0.20.7/go.mod h1:4x0yErUkcEWYG+O0S4QdrYa2+PLEeY2M7aeQe++2nmk=
 k8s.io/apimachinery v0.20.1/go.mod h1:WlLqWAHZGg07AeltaI0MV5uk1Omp8xaN0JGLY6gkRpU=
-k8s.io/apimachinery v0.20.6 h1:R5p3SlhaABYShQSO6LpPsYHjV05Q+79eBUR0Ut/f4tk=
-k8s.io/apimachinery v0.20.6/go.mod h1:ejZXtW1Ra6V1O5H8xPBGz+T3+4gfkTCeExAHKU57MAc=
-k8s.io/apiserver v0.20.6 h1:NnVriMMOpqQX+dshbDoZixqmBhfgrPk2uOh2fzp9vHE=
-k8s.io/apiserver v0.20.6/go.mod h1:QIJXNt6i6JB+0YQRNcS0hdRHJlMhflFmsBDeSgT1r8Q=
+k8s.io/apimachinery v0.20.7 h1:tBfhql7OggSCahvASeEpLRzvxc7FK77wNivi1uXCQWM=
+k8s.io/apimachinery v0.20.7/go.mod h1:ejZXtW1Ra6V1O5H8xPBGz+T3+4gfkTCeExAHKU57MAc=
+k8s.io/apiserver v0.20.7 h1:kmj4lX5evfdm8h07jRjuSANvRH0kPlXTq6LOSGT6n/k=
+k8s.io/apiserver v0.20.7/go.mod h1:7gbB7UjDdP1/epYBGnIUE6jWY4Wpz99cZ7igfDa9rv4=
 k8s.io/client-go v0.20.1/go.mod h1:/zcHdt1TeWSd5HoUe6elJmHSQ6uLLgp4bIJHVEuy+/Y=
-k8s.io/client-go v0.20.6 h1:nJZOfolnsVtDtbGJNCxzOtKUAu7zvXjB8+pMo9UNxZo=
-k8s.io/client-go v0.20.6/go.mod h1:nNQMnOvEUEsOzRRFIIkdmYOjAZrC8bgq0ExboWSU1I0=
-k8s.io/code-generator v0.20.6/go.mod h1:i6FmG+QxaLxvJsezvZp0q/gAEzzOz3U53KFibghWToU=
-k8s.io/component-base v0.20.6 h1:G0inASS5vAqCpzs7M4Sp9dv9d0aElpz39zDHbSB4f4g=
-k8s.io/component-base v0.20.6/go.mod h1:6f1MPBAeI+mvuts3sIdtpjljHWBQ2cIy38oBIWMYnrM=
+k8s.io/client-go v0.20.7 h1:Ot22456XfYAWrCWddw/quevMrFHqP7s1qT499FoumVU=
+k8s.io/client-go v0.20.7/go.mod h1:uGl3qh/Jy3cTF1nDoIKBqUZlRWnj/EM+/leAXETKRuA=
+k8s.io/code-generator v0.20.7/go.mod h1:i6FmG+QxaLxvJsezvZp0q/gAEzzOz3U53KFibghWToU=
+k8s.io/component-base v0.20.7 h1:TdRMMGxxxhcArvkem+FVqBljPOczs9j+tVGpYRM6TM8=
+k8s.io/component-base v0.20.7/go.mod h1:878UWprXC07P2CWFg+jjvTfxJSlkHp1v2m1MTkNQnJY=
 k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/gengo v0.0.0-20201113003025-83324d819ded/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=
 k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
 k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
 k8s.io/klog/v2 v2.4.0 h1:7+X0fUguPyrKEC4WjH8iGDg3laWgMo5tMnRTIGTTxGQ=
 k8s.io/klog/v2 v2.4.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y=
-k8s.io/kube-aggregator v0.20.6 h1:8auOC2n9sdqxc/pVaUZaX13U/ITgPYTzLsxGbVMrEuc=
-k8s.io/kube-aggregator v0.20.6/go.mod h1:YYhRn55SGsIyUjJc95hYfnlIKMJ8RWqCVBYIh5GkUqw=
+k8s.io/kube-aggregator v0.20.7 h1:gMTDj5zDAg0IYjo5wSNeMei0KzLvE+xGcMSFlH42DW8=
+k8s.io/kube-aggregator v0.20.7/go.mod h1:jItPWEHry5RdBf0MKbeIp/r4nEwkYn4LcuSzO/mg1Yw=
 k8s.io/kube-openapi v0.0.0-20201113171705-d219536bb9fd h1:sOHNzJIkytDF6qadMNKhhDRpc6ODik8lVC6nOur7B2c=
 k8s.io/kube-openapi v0.0.0-20201113171705-d219536bb9fd/go.mod h1:WOJ3KddDSol4tAGcJo0Tvi+dK12EcqSLqcWsryKMpfM=
 k8s.io/utils v0.0.0-20201110183641-67b214c5f920 h1:CbnUZsM497iRC5QMVkHwyl8s2tB3g7yaSHkYPkpgelw=

internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go

@@ -265,7 +265,14 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.OIDC
 				Message: err.Error(),
 			}
 		}
-		httpClient = &http.Client{Transport: &http.Transport{TLSClientConfig: tlsConfig}}
+
+		httpClient = &http.Client{
+			Timeout: time.Minute,
+			Transport: &http.Transport{
+				Proxy:           http.ProxyFromEnvironment,
+				TLSClientConfig: tlsConfig,
+			},
+		}
 
 		discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer)
 		if err != nil {

internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/url"
+	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -650,6 +651,15 @@ func TestController(t *testing.T) {
 				require.Equal(t, tt.wantResultingCache[i].GetUsernameClaim(), actualIDP.GetUsernameClaim())
 				require.Equal(t, tt.wantResultingCache[i].GetGroupsClaim(), actualIDP.GetGroupsClaim())
 				require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes())
+
+				// We always want to use the proxy from env on these clients, so although the following assertions
+				// are a little hacky, this is a cheap way to test that we are using it.
+				actualTransport, ok := actualIDP.Client.Transport.(*http.Transport)
+				require.True(t, ok, "expected cached provider to have client with Transport of type *http.Transport")
+				httpProxyFromEnvFunction := reflect.ValueOf(http.ProxyFromEnvironment).Pointer()
+				actualTransportProxyFunction := reflect.ValueOf(actualTransport.Proxy).Pointer()
+				require.Equal(t, httpProxyFromEnvFunction, actualTransportProxyFunction,
+					"Transport should have used http.ProxyFromEnvironment as its Proxy func")
 			}
 
 			actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().OIDCIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{})

internal/oidc/oidc.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package oidc contains common OIDC functionality needed by Pinniped.
@@ -199,7 +199,7 @@ func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration {
 		AuthorizationCodeSessionStorageLifetime: authorizationCodeLifespan + refreshTokenLifespan,
 		PKCESessionStorageLifetime:              authorizationCodeLifespan + (1 * time.Minute),
 		OIDCSessionStorageLifetime:              authorizationCodeLifespan + (1 * time.Minute),
-		AccessTokenSessionStorageLifetime:       accessTokenLifespan + (1 * time.Minute),
+		AccessTokenSessionStorageLifetime:       refreshTokenLifespan + accessTokenLifespan,
 		RefreshTokenSessionStorageLifetime:      refreshTokenLifespan + accessTokenLifespan,
 	}
 }