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

Verify the JWT signature #109

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Verify the JWT signature #109

merged 1 commit into from
Oct 8, 2020

Conversation

jimmyjames
Copy link
Contributor

Changes

This change adds back the verification of the JWT's signature, which was removed in version 2.3.0.

This PR adds back signature verification for both HS256 and RS256 signed JWTs. Tests have been added to ensure that we are indeed verifying signatures, and doing so in a way that does not introduce breaking changes.

Note:
As this library only supports the Authorization Code flow, per the [OIDC Specification section 3.1.3.7] (https://openid.net/specs/openid-connect-core-1_0-final.html#IDTokenValidation), verifying the ID Token's signature is not strictly required:

If the ID Token is received via direct communication between the Client and the Token Endpoint (which it is in this flow), the TLS server validation MAY be used to validate the issuer in place of checking the token signature.

However, when we can verify the signature, we should; also removing the signature verification was a regression that this change addresses.

Testing

In addition to adding unit tests to verify the signature verification and claims validation, I also tested with a Quickstart sample, using both RS256 and HS256 signing algorithms, using a local version of this SDK with this fix included:

  • Valid JWTs were verified successfully

  • JWTs with invalid signatures are rejected

  • This change adds unit test coverage

  • This change has been tested on the latest version of the platform/language or why not

Checklist

@jimmyjames jimmyjames added this to the vNext milestone Oct 8, 2020
@jimmyjames jimmyjames requested a review from a team October 8, 2020 20:30
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jimmyjames jimmyjames merged commit 8999731 into master Oct 8, 2020
@jimmyjames jimmyjames modified the milestones: vNext, 2.4.1 Oct 8, 2020
@jimmyjames jimmyjames mentioned this pull request Oct 8, 2020
@davidpatrick davidpatrick deleted the signature-verification branch January 15, 2021 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants