feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec #481

swiftone · 2019-06-24T22:49:19Z

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?

access token validation was only checking signature and expiration

Issue Number: N/A

What is the new behavior?

Checks for issuer match for iss claim
Optionally checks audience for aud claim if passed to verifyAccessToken()

Does this PR introduce a breaking change?

Yes
No

Issuer is now a required claim. This should break nothing for existing production code where issuer was always included. Nonetheless, this will be part of the 1.0 release so will be a major version change.

Other information

Reviewers

vijetmahabaleshwar-okta · 2019-06-25T20:54:35Z

packages/oidc-middleware/package.json

@@ -19,7 +19,7 @@
    "test": "yarn test:unit && yarn test:e2e && yarn test:integration",
    "pretest:e2e": "../../scripts/updateSeDrivers.sh",
    "test:e2e": "protractor test/e2e/protractor.conf.js",
-    "test:integration": "../../scripts/tck.sh",
+    "test:integration": "../../scripts/tck.sh 0.4.0-SNAPSHOT",


Still using older version of TCK in oidc-middleware. Tests failing with latest version.
Created this JIRA to track and fix it - https://oktainc.atlassian.net/browse/OKTA-234895

vijetmahabaleshwar-okta · 2019-06-25T20:56:18Z

scripts/tck.sh

-TCK_VERSION=0.4.0-SNAPSHOT
+if [ -z "$1" ]
+  then
+    TCK_VERSION=0.5.4-SNAPSHOT


If no version is passed, use the latest.

robertjd

A few questions to resolve, looks great otherwise.

robertjd · 2019-06-26T15:42:41Z

packages/jwt-verifier/lib.js

        resolve(jwt);
      });
    });
  }
+
+  async verifyAccessToken(accessTokenString, expectedAudience) {


Are we converting this to async becuase we want to be ready for a situation where we support remote validation via introspect?

It's not really a conversion - it previously returned a promise, so the async is just explicit.

robertjd · 2019-06-26T15:43:56Z

packages/jwt-verifier/lib.js

+    // - issuer claim
+    // - any custom claims passed in
+
+    const jwt = await this.verifyAsPromise(accessTokenString);


Is it useful to break out this verifyAsPromise logic to its own function? Asking because I only see it being used once.

I'm a proponent of having each function do one job (organizing calls to other functions is "one job").

Originally I had the verifyAsPromise in here, and it made this function a little confusing - it was juggling a callback, a promise from that callback, and calling some other functions, so I refactored it to this.

robertjd · 2019-06-26T15:45:51Z

packages/jwt-verifier/test/integration-test/resource-server.js

@@ -94,7 +96,7 @@ app.get('/api/messages', authenticationRequired, (req, res) => {
      },
      {
        date:  new Date(new Date().getTime() - 1000 * 60 * 60),
-        text: 'Hello, word!'
+        text: 'Hello, world!'


robertjd · 2019-06-26T15:46:43Z

packages/jwt-verifier/test/integration-test/resource-server.js


-  return oktaJwtVerifier.verifyAccessToken(accessToken)
+  return oktaJwtVerifier.verifyAccessToken(accessToken, expectedAud)


I don't see a test that asserts a failure if the expected audience doesn't match the provided audience?

Oops nevermind this one, I found it later

Doesn't line 373 in verifier.spec.js cover this?
it('fails with a invalid audience when given a valid expectation', () => {

robertjd · 2019-06-26T15:57:03Z

packages/jwt-verifier/test/spec/verifier.spec.js

+      mockKidAsKeyFetch(verifier);
+
+      return verifier.verifyAccessToken(token, 'http://myapp.com/')
+        .then( () => { throw new Error('Invalid Signature was accepted'); } )


robertjd · 2019-06-26T15:58:07Z

packages/jwt-verifier/test/spec/verifier.spec.js

+      mockKidAsKeyFetch(verifier);
+
+      return verifier.verifyAccessToken(token)
+        .catch( err => {


Add add a then() handler here, and do a "this shouldn't have passed" exception like you did above for signature?

Can do - I left it out since every other test is implicitly doing this.

robertjd · 2019-06-26T15:58:42Z

packages/jwt-verifier/test/spec/verifier.spec.js

+        });
+    });
+
+    it('passes when no audience expectation is passed (Legacy)', () => {


Since we are doing a major rev, can we remove the legacy behavior?

Yes. Is that a request to do so?

Yeah, let's remove

robertjd · 2019-06-26T16:00:54Z

packages/jwt-verifier/test/spec/verifier.spec.js

+      });
+      mockKidAsKeyFetch(verifier);
+
+      return verifier.verifyAccessToken(token, ['one', 'http://myapp.com/', 'three'])


What happens if i do verifyAccessToken(token, []) ? Seems like test we should have

What happens is a guaranteed failure, since the actual audience won't (can't) match any entry in the empty array, but it's a reasonable enough test to add.

I'd advocate for adding a test, so that in the future we can refactor internals and know we're catching this case. I've been bitten by internal logic that allows an empty value to bypass validations 😬

aarongranick-okta

LGTM

feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec

6ccc264

swiftone requested review from aarongranick-okta and robertjd June 24, 2019 22:49

Merge branch 'master' into br-hat-jwt-access-toke-to-spec-OKTA-233219

c3768fd

vijetmahabaleshwar-okta self-requested a review June 25, 2019 19:48

Updates TCK version to 0.5.4 to get tests passing!

d518530

vijetmahabaleshwar-okta force-pushed the br-hat-jwt-access-toke-to-spec-OKTA-233219 branch from 79a3372 to d518530 Compare June 25, 2019 19:49

vijetmahabaleshwar-okta reviewed Jun 25, 2019

View reviewed changes

Uses TCK version 0.4.0 in oidc-middleware

4cfa955

vijetmahabaleshwar-okta force-pushed the br-hat-jwt-access-toke-to-spec-OKTA-233219 branch from 762bb33 to 4cfa955 Compare June 25, 2019 20:55

vijetmahabaleshwar-okta reviewed Jun 25, 2019

View reviewed changes

robertjd reviewed Jun 26, 2019

View reviewed changes

aarongranick-okta approved these changes Jun 26, 2019

View reviewed changes

Removes legacy verifyAccessToken w/o expectedAud

0d63de4

swiftone merged commit 9d76c9f into master Jun 27, 2019

swiftone deleted the br-hat-jwt-access-toke-to-spec-OKTA-233219 branch July 26, 2019 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec #481

feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec #481

swiftone commented Jun 24, 2019

vijetmahabaleshwar-okta Jun 25, 2019

vijetmahabaleshwar-okta Jun 25, 2019

robertjd left a comment

robertjd Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019

robertjd Jun 26, 2019

robertjd Jun 26, 2019

robertjd Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019

robertjd Jun 26, 2019

swiftone Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019

robertjd Jun 26, 2019

swiftone Jun 26, 2019

robertjd Jun 26, 2019 •

edited

Loading

swiftone Jun 26, 2019

aarongranick-okta left a comment


		return oktaJwtVerifier.verifyAccessToken(accessToken)
		return oktaJwtVerifier.verifyAccessToken(accessToken, expectedAud)

feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec #481

feat(jwt-verifier): Adds verifications to verifyAccessToken() per spec #481

Conversation

swiftone commented Jun 24, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertjd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertjd Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarongranick-okta left a comment

Choose a reason for hiding this comment

robertjd Jun 26, 2019 •

edited

Loading