Support per-client signing algorithm

Question

Support per-client signing algorithm

mitar opened this issue 10 months ago · 8 comments

mitar commented 10 months ago

Preflight checklist

I could not find a solution in the existing issues, docs, nor discussions.
I agree to follow this project's Code of Conduct.
I have read and am following this repository's Contribution Guidelines.
I have joined the Ory Community Slack.
I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Describe your problem

Current implementation of Signer interface and DefaultSigner code assumes that there is only one private key used by Fosite and based on that private key an RS256 or ES256 signature is made for all tokens. This is problematic because it is hard to support anything besides RS256. RS256 is mandated by the spec that it should always be supported so if you want to support also ES256 you have a problem - clients cannot opt-in into ES256.

Self-registration supports id_token_signed_response_alg for clients to select the signing algorithm.

This has been discussed by @vivshankar and @james-d-elliott in this issue as well. You can also see pains implementing this currently (one has to pass client data through ctx).

Describe your ideal solution

I think another interface should be implemented, ClientSigner maybe, which extends Signer with the following method:

GenerateForClient(ctx context.Context, client ClientSigner, claims MapClaims, header Mapper) (string, string, error)

ClientSigner is another interface which extends Client to add:

GetIdTokenSignedResponseAlg() string
GetUserInfoSignedResponseAlg() string

GetUserInfoSignedResponseAlg I added for future when #581 is made.

Then DefaultSigner should implement GenerateForClient and GenerateForClient should be used instead of Generate when available.

I am open to different approach to addressing this. The important thing is that fosite would have to accept RSA and ECDSA keys which would then use them to sign tokens as picked by the client.

Workarounds or alternatives

You can try to implement Signer interface yourself and pass client data through ctx.

Version

latest master

Additional Context

No response

Answer 1 · 2024-01-29T03:07:14.000Z

I agree with the pain from not being able to figure out this easily. For reference I made my own Signer implementation which looks at the kid and alg header values and uses that to lookup the key to use.

Answer 2 · 2024-01-29T08:56:22.000Z

Oh, so you expose kid and alg through GetJWTHeader of your session struct based on client configuration? That is a nice trick and it seems better than my proposed API above. Can you share your implementation? Maybe we should add this into DefaultSigner implementation in fosite itself?

Answer 3 · 2024-01-29T09:30:20.000Z

More than willing. I'll put together a rough example. I believe it requires an alteration to multiple interface implementations (i.e. the implementations themselves, not the signature).

Answer 4 · 2024-02-05T21:09:39.000Z

I've tried to include as much context as possible. Some elements are domain logic and where possible have been excluded. It should be easy enough to extrapolate the required information if not I can clarify upon request. It should be noted that how I utilize this is I just set the kid, and the client when initialized determines what the kid should be based on the admins config of either the alg or the explicit kid.

For this to work I believe you must set the kid or alg header when generating the openid.Session before NewAuthorizeResponse:

		session := &openid.DefaultSession{
			Headers: &jwt.Headers{
				Extra: map[string]any{
					"kid": "abc123,
					"alg": "ES512",
				},
			},
		}

Signer Implementation example (not intended to be modified at runtime but could be relatively easily):

// The DynamicSigner type handles JWKs and signing operations.
type DynamicSigner struct {
	alg2kid map[string]string
	kids    map[string]*JWK
	algs    map[string]*JWK
}

// GetByHeader returns the JWK a JWT header with the appropriate kid value or returns an error.
func (s *DynamicSigner) GetByHeader(ctx context.Context, header fjwt.Mapper) (jwk *JWK, err error) {
	var (
		kid, alg string
		ok       bool
	)

	if header == nil {
		return nil, fmt.Errorf("jwt header was nil")
	}

	kid, _ = header.Get(JWTHeaderKeyIdentifier).(string)
	alg, _ = header.Get(JWTHeaderKeyAlgorithm).(string)

	if len(kid) != 0 {
		if jwk, ok = s.kids[kid]; ok {
			return jwk, nil
		}

		return nil, fmt.Errorf("jwt header '%s' with value '%s' does not match a managed jwk", JWTHeaderKeyIdentifier, kid)
	}

	if len(alg) != 0 {
		if jwk, ok = s.algs[alg]; ok {
			return jwk, nil
		}

		return nil, fmt.Errorf("jwt header '%s' with value '%s' does not match a managed jwk", JWTHeaderKeyAlgorithm, alg)
	}

	return nil, fmt.Errorf("jwt header did not match a known jwk")
}

// GetByTokenString does an invalidated decode of a token to get the  header, then calls GetByHeader.
func (s *DynamicSigner) GetByTokenString(ctx context.Context, tokenString string) (jwk *JWK, err error) {
	var (
		token *jwt.Token
	)

	if token, _, err = jwt.NewParser().ParseUnverified(tokenString, jwt.MapClaims{}); err != nil {
		return nil, err
	}

	return s.GetByHeader(ctx, &fjwt.Headers{Extra: token.Header})
}

// GetByKID returns the JWK given an key id or nil if it doesn't exist. If given a blank string it returns the default.
func (s *DynamicSigner) GetByKID(ctx context.Context, kid string) *JWK {
	if kid == "" {
		return s.kids[s.alg2kid[SigningAlgRSAUsingSHA256]]
	}

	if jwk, ok := s.kids[kid]; ok {
		return jwk
	}

	return nil
}

// Generate implements the fosite jwt.Signer interface and automatically maps the underlying keys based on the JWK Header kid.
func (s *DynamicSigner) Generate(ctx context.Context, claims fjwt.MapClaims, header fjwt.Mapper) (tokenString string, sig string, err error) {
	var jwk *JWK

	if jwk, err = s.GetByHeader(ctx, header); err != nil {
		return "", "", fmt.Errorf("error getting jwk from header: %w", err)
	}

	extra := header.ToMap()

	extra[JWTHeaderKeyIdentifier] = jwk.KeyID()
	extra[JWTHeaderKeyAlgorithm] = jwk.Algorithm()

	return jwk.Strategy().Generate(ctx, claims, &fjwt.Headers{Extra: extra})
}

// Validate implements the fosite jwt.Signer interface and automatically maps the underlying keys based on the JWK Header kid.
func (s *DynamicSigner) Validate(ctx context.Context, tokenString string) (sig string, err error) {
	var jwk *JWK

	if jwk, err = s.GetByTokenString(ctx, tokenString); err != nil {
		return "", fmt.Errorf("error getting jwk from token string: %w", err)
	}

	return jwk.Strategy().Validate(ctx, tokenString)
}

// Hash implements the fosite jwt.Signer interface.
func (s *DynamicSigner) Hash(ctx context.Context, in []byte) (sum []byte, err error) {
	return s.GetByKID(ctx, "").Strategy().Hash(ctx, in)
}

// Decode implements the fosite jwt.Signer interface and automatically maps the underlying keys based on the JWK Header kid.
func (s *DynamicSigner) Decode(ctx context.Context, tokenString string) (token *fjwt.Token, err error) {
	var jwk *JWK

	if jwk, err = s.GetByTokenString(ctx, tokenString); err != nil {
		return nil, fmt.Errorf("error getting jwk from token string: %w", err)
	}

	return jwk.Strategy().Decode(ctx, tokenString)
}

// GetSignature implements the fosite jwt.Signer interface.
func (s *DynamicSigner) GetSignature(ctx context.Context, tokenString string) (sig string, err error) {
	return getTokenSignature(tokenString)
}

// GetSigningMethodLength implements the fosite jwt.Signer interface.
func (s *DynamicSigner) GetSigningMethodLength(ctx context.Context) (size int) {
	return s.GetByKID(ctx, "").Strategy().GetSigningMethodLength(ctx)
}

// JWK is a representation layer over the *jose.JSONWebKey for convenience.
type JWK struct {
	kid  string
	use  string
	alg  jwt.SigningMethod
	hash crypto.Hash

	key            schema.CryptographicPrivateKey
	chain          schema.X509CertificateChain
	thumbprintsha1 []byte
	thumbprint     []byte
}

// GetSigningMethod returns the jwt.SigningMethod for this *JWK.
func (j *JWK) GetSigningMethod() jwt.SigningMethod {
	return j.alg
}

// GetPrivateKey returns the Private Key for this *JWK.
func (j *JWK) GetPrivateKey(ctx context.Context) (any, error) {
	return j.PrivateJWK(), nil
}

// KeyID returns the Key ID for this *JWK.
func (j *JWK) KeyID() string {
	return j.kid
}

// Algorithm returns the Algorithm for this *JWK.
func (j *JWK) Algorithm() string {
	return j.alg.Alg()
}

// DirectJWK directly returns the *JWK as a jose.JSONWebKey with the private key if appropriate.
func (j *JWK) DirectJWK() (jwk jose.JSONWebKey) {
	return jose.JSONWebKey{
		Key:                         j.key,
		KeyID:                       j.kid,
		Algorithm:                   j.alg.Alg(),
		Use:                         j.use,
		Certificates:                j.chain.Certificates(),
		CertificateThumbprintSHA1:   j.thumbprintsha1,
		CertificateThumbprintSHA256: j.thumbprint,
	}
}

// PrivateJWK directly returns the *JWK as a *jose.JSONWebKey with the private key if appropriate.
func (j *JWK) PrivateJWK() (jwk *jose.JSONWebKey) {
	value := j.DirectJWK()

	return &value
}

// JWK directly returns the *JWK as a jose.JSONWebKey specifically without the private key.
func (j *JWK) JWK() (jwk jose.JSONWebKey) {
	if jwk = j.DirectJWK(); jwk.IsPublic() {
		return jwk
	}

	return jwk.Public()
}

// Strategy returns the fosite jwt.Signer.
func (j *JWK) Strategy() (strategy fjwt.Signer) {
	return &Signer{
		hash:          j.hash,
		alg:           j.alg,
		GetPrivateKey: j.GetPrivateKey,
	}
}

Answer 5 · 2024-02-05T21:15:20.000Z

I see. Thanks. I think this could be probably made generic as part of default signer.

Answer 6 · 2024-02-05T21:36:30.000Z

If it could be added in a way which prevents breaking changes then I'd agree. My guess is that it may not be so simple. Not that a breaking change is a problem, but it may not be necessary in which case maybe adding a new optional implementation within fosite itself would be more appropriate.

Also not entirely sure the JWK struct is needed, I just made that struct to handle some domain logic I think.

But I would be more than happy for a direct copy paste of anything under the respective fosite license.

Answer 7 · 2024-02-05T21:42:14.000Z

My guess is that it may not be so simple.

Why not? If there are headers set, use that key, otherwise use default. It could work I think. But of course, we will see once somebody works on this for real. :-)

I will wait for now on others (e.g., @aeneasr) to confirm that this is something to be added to fosite.

Answer 8 · 2024-02-05T21:46:49.000Z

The way I implemented it assumes you will iteratively add the keys to the struct itself. The DefaultSigner if memory serves me correct takes a delegate which doesn't have adequate functionality to do what is needed.