[jwt-verifier] feat: Add verifyIdToken() #951

denysoblohin-okta · 2020-12-02T12:10:29Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Only access tokens can be validated.

Issue Number: N/A

What is the new behavior?

ID tokens can be validated.

Does this PR introduce a breaking change?

Yes
No

Verifier will throw the error "No KID specified" if no KID is present in the JWT header.

Other information

Validation spec: https://github.com/okta/oss-technical-designs/blob/master/technical_designs/jwt-validation-libraries.md#id-token-verification
Internal ref: OKTA-234446

Reviewers

packages/jwt-verifier/CHANGELOG.md

packages/jwt-verifier/README.md

packages/jwt-verifier/lib.js

aarongranick-okta

Please bump the version in jwt-verifier package.json to 1.1.0. In this repo we must manage versions manually.

aarongranick-okta · 2020-12-13T21:44:11Z

packages/jwt-verifier/README.md

@@ -3,9 +3,9 @@
 [![npm version](https://img.shields.io/npm/v/@okta/jwt-verifier.svg?style=flat-square)](https://www.npmjs.com/package/@okta/jwt-verifier)
 [![build status](https://img.shields.io/travis/okta/okta-oidc-js/master.svg?style=flat-square)](https://travis-ci.org/okta/okta-oidc-js)

-This library verifies Okta access tokens (issued by [Okta Custom Authorization servers](https://developer.okta.com/docs/concepts/auth-servers/) by fetching the public keys from the JWKS endpoint of the authorization server. If the access token is valid it will be converted to a JSON object and returned to your code. 
+This library verifies Okta access tokens (issued by [Okta Custom Authorization servers](https://developer.okta.com/docs/concepts/auth-servers/) and ID tokens by fetching the public keys from the JWKS endpoint of the authorization server. If the access token is valid it will be converted to a JSON object and returned to your code. 


This section is talking about id and access tokens together, but there are some important and subtle distinctions between them. the ID token is part of OIDC, (establishing identity) and the access token is part of OAuth (scoping permissions). I think we will want to add more links to the access token section for api access management and how to use scopes. And we will want to add more to id token section about how to use claims within an id token. So I think it would be good to separate the current text into the two sections, "Access Tokens", "ID Tokens"

Access Tokens

This library verifies Okta access tokens .....

You can learn about access tokens....

custom auth servers warning...

ID Tokens

Library verifies ID tokens issued by custom auth servers OR okta org

How ID tokens are verified by this SDK

Learn more about ID tokens and OIDC

aarongranick-okta · 2020-12-13T21:51:10Z

packages/jwt-verifier/lib.js

@@ -115,15 +139,19 @@ class OktaJwtVerifier {
    });
    this.verifier = nJwt.createVerifier().setSigningAlgorithm('RS256').withKeyResolver((kid, cb) => {
      this.jwksClient.getSigningKey(kid, (err, key) => {
+        // If kid is undefined, but there is 1 signing key, `jwks-rsa` will not throw error
+        if (!kid)


is there a JIRA issue related to this undefined kid condition? This seems like its fixing (or working around) a bug. If the SDK has to check this condition, would it be better to check before calling getSigningKey ?

There is a test for this condition.
But currently, it depends on the configuration of the authorization server.
If there are 2 public keys in JWKS, jwks-rsa will throw the error, which will result in error from njwt: "Error while resolving signing key for kid undefined"
If there is 1 key in JWKS, no error will be thrown and tests will fail (which happens currently)

Maybe remove code change and skip test should fail if no kid is present in the JWT header?

If "kid" is required, why don't we throw before calling getSigningKey? Is there another condition we are missing here? Should it be

if (!kid && !err) // do not override error if it exists

or

if (!id && key) // this seems to match the comment? (kid is undefined, but there is 1 signing key)

The other question is if you think this is a bug in jwk-rsa or njwt? If so we can include the fix here but in a comment reference another issue to fix it in the other library.

In latest commit I've changed behavior to throw an error if KID is not specified , before calling getSigningKey

Looks like it's expected behaviour of jwks-rsa rather than a bug.

aarongranick-okta · 2021-01-05T01:25:27Z

@denysoblohin-okta do you think any of the changes here are "breaking". Should we bump the version to 2.0.0 ?

denysoblohin-okta · 2021-01-11T14:50:29Z

@denysoblohin-okta do you think any of the changes here are "breaking". Should we bump the version to 2.0.0 ?

@aarongranick-okta I've bumped to 2.0.0 and added "Verifier will throw error "No KID specified" if no KID is present in the JWT header" in changelog

aarongranick-okta

Looks good!

If possible, could you squash to 2 commits:

1 which contains the feature, all code changes and README (add verifyId token)
1 which has package.json, CHANGELOG (release 2.0.0)

Add verifyIdToken() functionality to @okta/jwt-verifier (OKTA-234446) Verifier will throw error "No KID specified" if no KID is present in the JWT header

denysoblohin-okta force-pushed the add-verifyIdToken-OKTA-234446 branch from 6d97e61 to b72e0e9 Compare December 4, 2020 09:13

denysoblohin-okta marked this pull request as ready for review December 4, 2020 09:17

oleksandrpravosudko-okta self-requested a review December 4, 2020 15:59

oleksandrpravosudko-okta approved these changes Dec 4, 2020

View reviewed changes

shuowu reviewed Dec 7, 2020

View reviewed changes

packages/jwt-verifier/CHANGELOG.md Outdated Show resolved Hide resolved

shuowu reviewed Dec 7, 2020

View reviewed changes

packages/jwt-verifier/README.md Outdated Show resolved Hide resolved

shuowu reviewed Dec 7, 2020

View reviewed changes

packages/jwt-verifier/lib.js Outdated Show resolved Hide resolved

shuowu approved these changes Dec 7, 2020

View reviewed changes

aarongranick-okta suggested changes Dec 7, 2020

View reviewed changes

denysoblohin-okta force-pushed the add-verifyIdToken-OKTA-234446 branch from f795fda to cb38eca Compare December 7, 2020 20:26

denysoblohin-okta requested a review from aarongranick-okta December 7, 2020 20:37

aarongranick-okta reviewed Dec 13, 2020

View reviewed changes

denysoblohin-okta force-pushed the add-verifyIdToken-OKTA-234446 branch 2 times, most recently from 6d5e7b2 to 3823b59 Compare December 14, 2020 16:49

denysoblohin-okta force-pushed the add-verifyIdToken-OKTA-234446 branch 2 times, most recently from c9f1c9e to d7ff3a1 Compare January 11, 2021 14:48

aarongranick-okta approved these changes Jan 11, 2021

View reviewed changes

denysoblohin-okta added 2 commits January 11, 2021 21:45

feat[jwt-verifier]: Add verifyIdToken()

5c2f688

Add verifyIdToken() functionality to @okta/jwt-verifier (OKTA-234446) Verifier will throw error "No KID specified" if no KID is present in the JWT header

[jwt-verifier] release 2.0.0

a355039

denysoblohin-okta force-pushed the add-verifyIdToken-OKTA-234446 branch from d7ff3a1 to a355039 Compare January 11, 2021 19:47

denysoblohin-okta merged commit 6b1c36a into master Jan 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jwt-verifier] feat: Add verifyIdToken() #951

[jwt-verifier] feat: Add verifyIdToken() #951

denysoblohin-okta commented Dec 2, 2020 •

edited

Loading

aarongranick-okta left a comment

aarongranick-okta Dec 13, 2020 •

edited

Loading

aarongranick-okta Dec 13, 2020 •

edited

Loading

denysoblohin-okta Dec 14, 2020

denysoblohin-okta Dec 14, 2020

aarongranick-okta Jan 5, 2021 •

edited

Loading

denysoblohin-okta Jan 11, 2021 •

edited

Loading

aarongranick-okta commented Jan 5, 2021

denysoblohin-okta commented Jan 11, 2021 •

edited

Loading

aarongranick-okta left a comment

[jwt-verifier] feat: Add verifyIdToken() #951

[jwt-verifier] feat: Add verifyIdToken() #951

Conversation

denysoblohin-okta commented Dec 2, 2020 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Reviewers

aarongranick-okta left a comment

Choose a reason for hiding this comment

aarongranick-okta Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

aarongranick-okta Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

denysoblohin-okta Dec 14, 2020

Choose a reason for hiding this comment

denysoblohin-okta Dec 14, 2020

Choose a reason for hiding this comment

aarongranick-okta Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

denysoblohin-okta Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

aarongranick-okta commented Jan 5, 2021

denysoblohin-okta commented Jan 11, 2021 • edited Loading

aarongranick-okta left a comment

Choose a reason for hiding this comment

denysoblohin-okta commented Dec 2, 2020 •

edited

Loading

aarongranick-okta Dec 13, 2020 •

edited

Loading

aarongranick-okta Dec 13, 2020 •

edited

Loading

aarongranick-okta Jan 5, 2021 •

edited

Loading

denysoblohin-okta Jan 11, 2021 •

edited

Loading

denysoblohin-okta commented Jan 11, 2021 •

edited

Loading