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.

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

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