updateKeys for remoteKeySet uses wrong context

[ekiyanov]

Im trying to verify token with Verifier created by provider.

verifier := provider.Verifier(&oidc.Config{ClientID: myClientID})
rawIDToken, ok := oauth2Token.Extra("id_token").(string)
ctx,_ = context.WithTimeout(context.Background(), time.Second*10)
idToken, err:= verifier.Verify(ctx, rawIDToken)

less than in a second i got the error saying context deadline exceeded
Trying to figure out what happened I changed ctx to context.Background()
but issue still persisted.

Digging into Verify code i find out that keys are actually fetched by function
func (r *remoteKeySet)keysFromRemote(ctx context.Context)([]jose.JSONWebKey, error)
and inside goroutine there is call r.updateKeys() which does not receive a context and inside the updateKeys actually is used another context r.ctx which one assigned by Provider.

Provided is created at start of the application, and due http request for openid-configuration Im not updating it often. and due same http request I set a context for provider with a deadline for 10 seconds, just in case if there is no connection to provider.

And what happens is Verify is actually would work during first 10 seconds after provider has been created, but not considering context passed to Verify

[ericchiang]

Provider contains a long term context to query the remote keys. Since may Verify calls share a single remote key, the context from the provider is used to fetch the keys, not the Verify call. For example, even if a Verify call hits its deadline, that shouldn't cancel an outbound request for keys for the concurrent Verify call.

I can see this being confusing if you've set a deadline for the context passed to NewProvider. I don't see a great way of fixing this given the current API.

On another note, I've never really understood how long lived structures should handle contexts. For example, NewRemoteKeySet which needs to make subsequent calls in the background. If you've got any thoughts here they'd be appreciated :)

https://godoc.org/github.com/coreos/go-oidc#NewRemoteKeySet

[swdunlop]

I ran into this issue and was surprised as well, since I had assumed this context controlled the first fetch of the remote key set. I expected the context passed to the Verify method to control subsequent fetches of the remote key set, if needed.

Functions generally should not retain contexts passed to them as arguments. The calling functions are expected to call any cancellation functions they produced, such as context.WithCancel or context.WithTimeout before they return.

Since Go-OIDC has to deal with periodically fetching information in the background, probably the best path is to add configuration for timeouts for these requests.

[misberner]

I also found myself pretty annoyed that I effectively can't bound the execution time of NewProvider via a context, but have to invoke it in a goroutine and pass the result back via a channel.

I opened #229 as a suggestion on how to improve things in a minimally disruptive way.

[ericchiang]

This is now fixed with #262.

RemoteKeySet no longer uses a cancelable context in the background, it just uses it for its bag of values. The NewProvider() context can be canceled freely without impacting the verifier.

[johejo]

IMO: I felt that the detached context (golang.org/x/tools/internal/xcontext) approach was better than the #260. how is it?
https://github.com/golang/tools/blob/master/internal/xcontext/xcontext.go

[ericchiang]

gah, I ment to link #260

That does basically the same thing as the detached context, so sounds like we're in agreement :)