[Bug] Context not propagating to token generator

[someone1]

I'm testing the newly added context propagation feature of both Hydra + Fosite and noticed my traces are missing chunks where I generate my JWTs:

If I'm following this right (and believe me with all the interfaces and such I have no idea if I am), I think the context isn't propagated throughout the code correctly. Specifically, the jwt strategy handler doesn't seem to pass the context down to the jwt strategy generating the token.

This seems like a trivial fix but I'm not sure why it wasn't done beforehand. @aaslamin - was this just an oversight or is this more complicated than it looks? I'd be happy to put in a PR to fix it or leave it to you!

[aaslamin]

Hey @someone1,

These were left as is because those interfaces are not a part of a call path used in Hydra. Unless I am missing something obvious (totally possible 😅!)...

Based on the trace you provided, your call to POST /oauth2/token in Hydra is using https://github.com/ory/fosite/blob/master/token/jwt/jwt.go which is propagating context as expected.

The JWT strategy used by the oauth handler in Hydra: https://github.com/ory/hydra/blob/master/cmd/server/handler_oauth2_factory.go#L171

Constructor: https://github.com/ory/hydra/blob/master/jwk/jwt_strategy.go#L40 Which implements https://github.com/ory/fosite/blob/master/token/jwt/jwt.go#L40

With that said, it doesn't hurt to pass the context along there either. I wanted to minimize my footprint 👣

I'll leave it in the hands of @aeneasr 🚶

[someone1]

Thanks for the quick response!

I do think that the call path includes at least the jwt.Generate implementation of the JWTStrategy interface which the context should be passed to and isn't when using the JWT signing mechanism for AccessTokens in hydra.

I probably am the only one who noticed this so far because I bootstrap hydra to use my own database plugin and my own signing mechanism - the latter of which I'm tinkering with so I can possibly introduce a plugin mechanism for singing JWTs with external systems such as a KMS. (Check out my repo). I'm also using OpenCensus which is why I'm missing any other kind of tracing you may have added to hydra already.

That being said - I do think limiting the context propagation to the use-case of hydra isn't fair to fosite since its purpose isn't hydra-specific.

[aaslamin]

I do think that the call path includes at least the jwt.Generate implementation of the JWTStrategy interface which the context should be passed to and isn't when using the JWT signing mechanism for AccessTokens in hydra.

This is passed in here: https://github.com/ory/hydra/blob/master/jwk/jwt_strategy.go#L76

Notice the JWT strategy that is used create the DefaultStrategy: https://github.com/ory/hydra/blob/master/cmd/server/handler_oauth2_factory.go#L86

It is of type RS256JWTStrategy which implements both https://github.com/ory/hydra/blob/master/jwk/jwt_strategy.go#L40 & https://github.com/ory/fosite/blob/master/token/jwt/jwt.go - the former simply wraps the later and adds a new method GetPublicKeyID(ctx context.Context) (string, error).

So you shouldn't be missing any spans to the db if you have instrumented your driver correctly. Right? The context you need from what I can see has already been passed down for you.

That being said - I do think limiting the context propagation to the use-case of hydra isn't fair to fosite since its purpose isn't hydra-specific.

I agree. It's good practice to pass context along.

[aaslamin]

Please follow the call path from the token handler in Hydra down the rabbit hole.

There is quite a fair bit of interfaces to go through, but you should notice that context has been passed all the way down the chain. I just did this again to double check.

[someone1]

I actually don't use hydra's built-in signing mechanisms (I wrote my own here) which is why my spans aren't limited to db calls only and I need the propagation down to my custom JWTStrategy implementation.

(the following is how I think the call path works in hydra) That said, jwk.RS256JWTStrategy in hydra is a wrapper as you mentioned, but it isn't called from hydra directly as you linked, its passed in as a strategy for fosite when bootstrapping hydra, the wrapper having the express purpose of refreshing keys from the database so it's not a static implementation as it is in fosite (giving you the ability to rotate keys in hydra). So when you request a token in hydra, hydra calls upon the fosite strategy it has to generate the token, but fosite is dropping the context instead of passing it along to Generate. Hydra never calls upon the JWTStrategy from any of its handlers directly to generate tokens (well except here which is unrelated to the token exchange flow) and instead defers to fosite to handle that.

Well that's how I followed it anyway and it would explain why my traces are missing a span I know should be present. I'm sure @aeneasr can clear this up with a quick comment, haha.

I really appreciate you taking the time to talk through this with me! In the end I think we're both in agreement that the context shouldn't be dropped as it currently is.

[aeneasr]

This doesn't look right to me, it should use the context set by the "parent" methods as it's available there. The callpath ( @someone1 ) is correct by the way