-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [#631] JWT Encryption support for client authentication and ID Token generation #764
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good from my perspective. I am wondering if we want to add an interface for client ID Token encryption options similar to the OpenIDConnectClient interface (specifically the GetRequestObjectSigningAlgorithm implementation). But I'm not entirely sure this is the best way (just sharing ideas as I have no concrete opinion).
@aeneasr Before I go too far down this rabbit hole, I wanted your opinion on replacing the current mechanism for validating JWTs that effectively relies on a KeyFunc returning a key, typically from jwks/jwks_uri config. In the implementation I am introducing here, starting with client authentication, I am expanding support to do the following -
I have a few cases where the approach I am introducing here applies or will apply -
In all cases, I am trying to preserve the current behavior (unit tests confirm it) while adding this extra option to validate the JWT but it may effectively negate the need for some functions that are in use today. It also introduces decryption as part of the same set of changes. Note here that I am specifically referring to incoming JWTs. The PR also carries a mechanism to encrypt outgoing JWTs for id_tokens, JARM, userinfo as JWT etc. I know some of these aren't yet in place but this sets the foundation to add those capabilities. I am, by no means, done with this PR though it is ready for review. |
I believe the following is a spec compliant way to derive symmetric key types: package example
import (
"crypto/aes"
"crypto/sha256"
"crypto/sha512"
"fmt"
"hash"
"github.com/go-jose/go-jose/v4"
)
func DeriveSymmetricKey(secret []byte, kid, alg, enc, use string) (jwk *jose.JSONWebKey, err error) {
if len(secret) == 0 {
return nil, fmt.Errorf("error occurred deriving symmetric jwk: client secret is not configured")
}
switch use {
case "sig":
return &jose.JSONWebKey{
Key: secret,
KeyID: kid,
Algorithm: alg,
Use: use,
}, nil
case "enc":
var (
hasher hash.Hash
bits int
)
keyAlg := jose.KeyAlgorithm(alg)
switch keyAlg {
case jose.A128KW, jose.A128GCMKW, jose.A192KW, jose.A192GCMKW, jose.A256KW, jose.A256GCMKW, jose.PBES2_HS256_A128KW:
hasher = sha256.New()
case jose.PBES2_HS384_A192KW:
hasher = sha512.New384()
case jose.PBES2_HS512_A256KW, jose.DIRECT:
hasher = sha512.New()
default:
return nil, fmt.Errorf("error occurred deriving symmetric jwk: the encryption key algorithm '%s' is not supported", enc)
}
switch keyAlg {
case jose.A128KW, jose.A128GCMKW, jose.PBES2_HS256_A128KW:
bits = aes.BlockSize
case jose.A192KW, jose.A192GCMKW, jose.PBES2_HS384_A192KW:
bits = aes.BlockSize * 1.5
case jose.A256KW, jose.A256GCMKW, jose.PBES2_HS512_A256KW:
bits = aes.BlockSize * 2
case jose.DIRECT:
switch jose.ContentEncryption(enc) {
case jose.A128CBC_HS256, "":
bits = aes.BlockSize * 2
case jose.A192CBC_HS384:
bits = aes.BlockSize * 3
case jose.A256CBC_HS512:
bits = aes.BlockSize * 4
default:
return nil, fmt.Errorf("error occurred deriving symmetric jwk: the encryption key algortihm '%s' does not support content encryption '%s'", alg, enc)
}
}
if _, err = hasher.Write(secret); err != nil {
return nil, fmt.Errorf("error occurred deriving symmetric jwk: error occurred hasing the secret: %w", err)
}
return &jose.JSONWebKey{
Key: hasher.Sum(nil)[:bits],
KeyID: kid,
Algorithm: alg,
Use: use,
}, nil
default:
return nil, fmt.Errorf("error occurred deriving symmetric jwk: the use '%s' is not supported", use)
}
} |
See #631
Related Issue or Design Document
See #631
Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact security@ory.sh) from the maintainers to push the changes.
Further comments