From f0d120a6ca4155548e5d52fb6277f2146fd1b17c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 8 Jul 2021 09:44:02 -0700 Subject: [PATCH] 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(