oauth2: abstract oauth2/handler JWT Strategies

[someone1]

I mentioned this in the chat a week or two ago and I believe @arekkas responded with leaning towards introducing an interface to implement this.

Basically, I'd like to use an external JWT signing mechanism. I'd like to bootstrap Hydra on my own in order to do this and for the most part I can, however, the introduction of OpenIDJWTStrategy *jwk.RS256JWTStrategy and AccessTokenJWTStrategy *jwk.RS256JWTStrategy in the oauth2/handler struct is preventing me from doing so.

I know there was some talk about introducing a plugin system for external key management/signing services though I think this is a good first step in implementing such a system. If we can introduce a basic interface for these two I can start devoting time to coming up with a system by hacking in my own deployment before proposing something here.

From what I can tell, the oauth2/handler only expects the following for both of these:

import (
	jwt2 "github.com/dgrijalva/jwt-go"
	"github.com/ory/fosite/token/jwt"
)

type JWTStrategyer interface {
	GetPublicKeyID() (string, error)
	Generate(claims jwt2.Claims, header jwt.Mapper) (string, string, error)
}

AccessTokenJWTStrategy only appears to be used when its not nil, meaning hydra was bootstrapped to utilize JWT for Access Tokens. The oauth2/handler only cares about the PublicKeyId this has to return which is passed along the Token and Auth handlers.

OpenIDJWTStrategy expects to be present so hydra can use it's PublicKeyId in the Auth handler response, and if a client is using signed responses, the UserInfo handler will sign it's response with this strategy.

If it's alright with you, I can submit a simple PR to introduce the interface and update the handler accordingly.

[aeneasr]

Sounds good! I think Generate(claims jwt2.Claims, header jwt.Mapper) (string, string, error) is only used in fosite, if I am not mistaken.

[someone1]

It's actually used once when signing userinfo responses - maybe this was not intended?

[someone1]

Does it make more sense to just add GetPublicKeyID() (string, error) to the JWTStrategy interface in fosite and just use that here? Looks like the implementation in hydra just layers ontop of fosite to refresh the keys.

[aeneasr]

Ah right, userinfo uses it. Makes sense regarding extending the fosite interface!

…

On 3. Aug 2018, at 16:31, Prateek Malhotra ***@***.***> wrote: Does it make more sense to just add GetPublicKeyID() (string, error) to the JWTStrategy interface in fosite and just use that here? Looks like the implementation in hydra just layers ontop of fosite to refresh the keys. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[someone1]

I opened an issue on fosite but maybe I was mistaken in doing so. Should we introduce a new interface in hydra, extending the fosite interface with the GetPublicKeyID() (string, error) function? Reason being is that this feature is dependent on key management which may be out of scope for the fosite interface?

The only way I see this being implemented in fosite is by adding an exported PublicKeyID string property to the existing RS256JWTStrategy implementation so it can be set/updated along with the exported PrivateKey value?

I think it makes more sense to extend the fosite interface by introducing a new interface in hydra. If this is what you meant then I'll go ahead with a PR and close the issue I opened in fosite. I'm open to discussing this further.

[someone1]

It was fairly quick to get this one done, you can preview my hydra changes and let me know what you think!

[aeneasr]

Yes, I think this should be introduced in Hydra as the GetPublicKeyID() (which basically allows refreshing the public/private keys) is currently a Hydra-only feature.

[aeneasr]

The changes look solid!