Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

vivshankar
Copy link
Contributor

See #631

Related Issue or Design Document

See #631

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    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.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

@vivshankar vivshankar changed the title feat: [#631] JWT Encryption support for client authentication feat: [#631] JWT Encryption support for client authentication and ID Token generation Aug 5, 2023
Copy link
Contributor

@james-d-elliott james-d-elliott left a 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).

@vivshankar
Copy link
Contributor Author

@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 -

  • Preserve current behavior to support jwks/jwks_uri based validation
  • Decryption for JWEs
  • Custom validation strategy for JWTs that might, for example, leverage a trust store or a key management service that offers crypto APIs etc.
  • Introducing some security controls around "allowed key IDs" and "allowed algs". The former is important IMO as seen in a recent hack. The latter is useful to enforce requirements for PS256 and ES256 type algorithms required by FAPI and other high assurance flows.

I have a few cases where the approach I am introducing here applies or will apply -

  1. Request object
  2. DCR with SSA validation and DCR client_metadata as application/jose (AU-CDR, for example, uses this)
  3. Token exchange for custom JWT type as a subject or actor token
  4. JWT bearer grant flow (would be an enhancement on top of what is available today)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants