From f0d120a6ca4155548e5d52fb6277f2146fd1b17c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 8 Jul 2021 09:44:02 -0700 Subject: [PATCH 1/3] 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. --- .../oidcupstreamwatcher/oidc_upstream_watcher.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index d5a8d302..889d66fc 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -264,12 +264,15 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } - httpClient = &http.Client{Transport: &http.Transport{Proxy: http.ProxyFromEnvironment, TLSClientConfig: tlsConfig}} + httpClient = &http.Client{ + Timeout: time.Minute, + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + TLSClientConfig: tlsConfig, + }, + } - timeoutCtx, cancelFunc := context.WithTimeout(oidc.ClientContext(ctx, httpClient), time.Minute) - defer cancelFunc() - - discoveredProvider, err = oidc.NewProvider(timeoutCtx, upstream.Spec.Issuer) + discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) if err != nil { const klogLevelTrace = 6 c.log.V(klogLevelTrace).WithValues( From 2f7dbed321d0f31985dc03d9f2ba2b1073de5ea7 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 8 Jul 2021 11:17:22 -0700 Subject: [PATCH 2/3] Try increasing the "eventually" timeouts in one integration test There were 10 second timeouts in `TestAPIServingCertificateAutoCreationAndRotation` which fail often on CI. Maybe increasing the timeouts will help? --- test/integration/concierge_api_serving_certs_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index 3689ed7a..2bb02f26 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -111,7 +111,7 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { var err error secret, err = kubeClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, defaultServingCertResourceName, metav1.GetOptions{}) requireEventually.NoError(err) - }, 10*time.Second, 250*time.Millisecond) + }, time.Minute, 250*time.Millisecond) regeneratedCACert := secret.Data["caCertificate"] regeneratedPrivateKey := secret.Data["tlsPrivateKey"] regeneratedCertChain := secret.Data["tlsCertificateChain"] @@ -131,7 +131,7 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { apiService, err := aggregatedClient.ApiregistrationV1().APIServices().Get(ctx, apiServiceName, metav1.GetOptions{}) requireEventually.NoErrorf(err, "get for APIService %q returned error", apiServiceName) requireEventually.Equalf(regeneratedCACert, apiService.Spec.CABundle, "CA bundle in APIService %q does not yet have the expected value", apiServiceName) - }, 10*time.Second, 250*time.Millisecond, "never saw CA certificate rotate to expected value") + }, time.Minute, 250*time.Millisecond, "never saw CA certificate rotate to expected value") // Check that we can still make requests to the aggregated API through the kube API server, // because the kube API server uses these certs when proxying requests to the aggregated API server, @@ -150,7 +150,7 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { }, metav1.CreateOptions{}) requireEventually.NoError(err, "dynamiccertificates.Notifier broken?") } - }, 30*time.Second, 250*time.Millisecond) + }, time.Minute, 250*time.Millisecond) }) } } From e130da6daa592c544fce9399cf6b69f75c061f81 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 8 Jul 2021 11:47:49 -0700 Subject: [PATCH 3/3] Add unit test assertion for new OIDC client request timeout --- .../oidcupstreamwatcher/oidc_upstream_watcher_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 24cb9b14..8712af26 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -807,6 +807,8 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs actualTransportProxyFunction := reflect.ValueOf(actualTransport.Proxy).Pointer() require.Equal(t, httpProxyFromEnvFunction, actualTransportProxyFunction, "Transport should have used http.ProxyFromEnvironment as its Proxy func") + // We also want a reasonable timeout on each request/response cycle for OIDC discovery and JWKS. + require.Equal(t, time.Minute, actualIDP.Client.Timeout) } actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().OIDCIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{})