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.
This commit is contained in:
parent
1f5480cd5c
commit
f0d120a6ca
@ -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(
|
||||
|
Loading…
Reference in New Issue
Block a user