RemoteKeySet stores and reuses initial context

[zakcutner]

First off, thanks for maintaining this project! I recently updated go-oidc, and realised that #364 is actually a breaking change in some cases. After this change, go-oidc uses the deadline of the context you pass to NewRemoteKeySet for the whole lifetime of the RemoteKeySet.

In my case, I was passing NewRemoteKeySet a context with a short timeout as I believed the context was only used for the lifetime of that function. But after #364, that context's deadline started being used for future calls to oidc.IDTokenVerifier.Verify, causing those calls to now fail with a context timeout error.

In general, I think it's not recommended to store contexts in structs to avoid these kinds of confusions. I was wondering if instead of using this context when making requests to the jwksURL…

go-oidc/oidc/jwks.go

Line 236 in 22dfdca

resp, err := doRequest(r.ctx, req)

…it might be feasible to pass down and use the context from keysFromRemote?

go-oidc/oidc/jwks.go

Line 190 in 22dfdca

func (r *RemoteKeySet) keysFromRemote(ctx context.Context) ([]jose.JSONWebKey, error) {

I think that if this is possible, it would both mitigate the breaking nature of the change in #364, and result in a more expected behaviour for users.

[ericchiang]

Thanks! Sorry for taking a bit to get back. Been a busy week.

The main motivation for using the NewRemoteKeySet context rather than the Verify context is because many Verify calls can block on a single call to update the remote key set. I didn't want one client's context cancelation to be able to kill that update for all clients waiting on it.

Totally aware of the "context in structs" note. In hindsight, I wouldn't have use the context to pass things like the client HTTP config. This is a holdover from the way golang.org/x/oauth2 does things. Contexts also don't play well with background logic, as we're seeing here.

Generally I would generally be surprised if, for a method with a signature like:

NewFoo(ctx context.Context) (*Foo)

That you'd expect the returned Foo to be valid after the context was canceled.

Is it possible to increase the lifetime of the context you pass to NewRemoteKeySet instead?

[zakcutner]

No problem, thank you so much for getting back to me!

Generally I would generally be surprised if, for a method with a signature like:

NewFoo(ctx context.Context) (*Foo)

That you'd expect the returned Foo to be valid after the context was canceled.

Personally, I think I'd actually expect the context only to be used for the lifetime of the NewFoo call. Although an example like this is given in that blog as one that can be confusing, so I guess there are a few interpretations 😅

However, I think the more confusing thing here for me was actually that the semantics unintentionally changed between from v3.5.0 and v3.6.0.

The main motivation for using the NewRemoteKeySet context rather than the Verify context is because many Verify calls can block on a single call to update the remote key set. I didn't want one client's context cancelation to be able to kill that update for all clients waiting on it.

That makes sense! What would you think about creating a new context to update the remote key set for this that is cancelled only when the contexts from all the clients are (i.e. create a “union” of all the clients' contexts)?

I was thinking that something like this might prevent others from running into the same issue as me, and remove a bit of ambiguity. But no worries at all if you don't think this would work well, and I can absolutely just update my code to pass a different context as you suggest.