feat(core): validate if access and id tokens are valid cognito tokens #13385

ashwinkumar6 · 2024-05-15T17:57:57Z

Description of changes

Add additional validations:
Add isValidCognitoToken util to core to validate if the given access and id tokens are valid using the aws-jwt-verify library on server side

Description of how you validated changes

Tested locally in Next.js sample app by passing valid and invalid tokens in cookie storage

Checklist

PR description included
yarn test passes
Tests are changed or added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Samaritan1011001 · 2024-05-15T23:24:48Z

packages/core/src/utils/isValidCognitoToken.ts

+
+		return true;
+	} catch (error) {
+		return false;


Do we want to debug log a generic message here?

We could, though if we really want a log to provide insight of the check failure, we may actually need production log.

Also can we add a TODO item here to remind us that this error should be surfaced eventually.

thanks added

Samaritan1011001 · 2024-05-15T23:32:50Z

packages/auth/src/providers/cognito/tokenProvider/TokenStore.ts

+	): Promise<boolean> {
+		const userPoolId = this.authConfig?.Cognito?.userPoolId;
+		const clientId = this.authConfig?.Cognito?.userPoolClientId;
+		if (!userPoolId || !clientId) return false;


We may want to throw a different error here? If not we will just throw InvalidToken and is not appropriate I think.

Discussed offline, throwing any error here would result in it being swallowed by the token store. Hence we just return null if validation fails when getting an item from the store.

The reason the token store swallows the error and returns null is it intends to mock the Storage mechanism in the browser which also returns null if a key isn't found. Adding a TODO to revisit this, we could also clear the tokens if invalid

israx

The usage of the aws-jwt-verify library to validate the signature of the tokens make sense to me. Based on their code base I see that they have a solid approach to validate Cognito tokens. However we shouldn't validate the tokens at the TokenStore level because it can be overwritten by a custom token provider.

Blocking PR until we make sure there is not a security gap left. It might be worth validating tokens in the server fetchAuthSession API directly. That way we isolate any extra bundle size that is affecting other APIs.

israx · 2024-05-16T14:43:08Z

packages/core/src/utils/isValidCognitoToken.ts

+			tokenUse: tokenType,
+			clientId,
+		});
+		await verifier.verify(token);


Probably this util might not be needed. The verify method returns the same JWT if the token is valid, otherwise it throws. I think we should rely on that error and let the customer handle it.

Discussed async, the error thrown by aws-jwt-verify may contain sensitive information. It's best to not surface the actual error generated by aws-jwt-verify

ashwinkumar6 · 2024-05-17T18:19:50Z

nit cleanups in progress

…ito tokens

packages/adapter-nextjs/src/utils/validator.ts

packages/aws-amplify/package.json

israx

Thanks @ashwinkumar6 for fixing this issue!

HuiSF

Thanks for adding this improvement.

packages/core/src/adapterCore/serverContext/types/KeyValueStorageValidator.ts

HuiSF · 2024-05-17T23:38:47Z

packages/core/src/utils/isValidCognitoToken.ts

+
+		return true;
+	} catch (error) {
+		return false;


Also can we add a TODO item here to remind us that this error should be surfaced eventually.

...__tests__/adapterCore/storageFactories/createKeyValueStorageFromCookieStorageAdapter.test.ts

HuiSF · 2024-05-17T23:46:02Z

packages/adapter-nextjs/src/utils/createTokenValidator.ts

+
+			// TODO: is this correct ?
+			// make sure userPoolId && clientId are present if token is access/id
+			if (!userPoolId || !clientId) return false;


nit: we can require these two fields from createTokenValidator's consumer.

If we make userPoolId and clientId required fields in this function, we would need make sure these are passed by it's consumer createRunWithAmplifyServerContext.

It's a bit tricky because if we add assertions in createRunWithAmplifyServerContext, we won't be able to get other keys from Storage if userPoolId or clientId are missing.

packages/adapter-nextjs/__tests__/utils/createTokenValidator.test.ts

…ageValidator.ts Co-authored-by: Hui Zhao <[email protected]>

…est.ts Co-authored-by: Hui Zhao <[email protected]>

…ageValidator.ts Co-authored-by: Hui Zhao <[email protected]>

packages/adapter-nextjs/src/utils/createTokenValidator.ts

israx · 2024-05-20T21:07:47Z

...s-amplify/src/adapter-core/storageFactories/createKeyValueStorageFromCookieStorageAdapter.ts

+			if (value && validatorMap?.getItem) {
+				const isValid = await validatorMap.getItem(key, value);
+				if (!isValid) return null;
+			}
+
+			return value;


I wonder if it make sense to move this logic inside the cookieStorageAdapter directly and return null if it is not able to find the cookie

We had initially moved validation logic inside packages/adapter-nextjs/src/utils/createCookieStorageAdapterFromNextServerContext.ts but this isn't an async function. So in case we throw an error it'll become unhandledRejection.

Hence we made the validationMap optionally injectable into the KeyValueStorage

israx · 2024-05-20T21:12:31Z

packages/adapter-nextjs/src/utils/createTokenValidator.ts

+}): KeyValueStorageMethodValidator => {
+	return {
+		// validate access, id tokens
+		getItem: async (key: string, value: string): Promise<boolean> => {


The getItem method implies that it will get something but the return type is a boolean. We could either return the actual item/token and return null if it is invalid. The other option is to change the method name to something more deterministic that implies the validation of the token.

That's a good point 🤔,
This function returns a validationMap where the key is name of the function in KeyValueStorage that we want to add validation for (getItem in this case) and the value is the validation function that returns a bool if the validation was successful or now

We could maybe improve the function name to clarity that

israx

Thanks @ashwinkumar6 for putting this together only some minor comments. Let me know what you think

Co-authored-by: israx <[email protected]>

jimblanc

Looking good, few questions & nits

jimblanc · 2024-05-20T21:49:53Z

...s-amplify/src/adapter-core/storageFactories/createKeyValueStorageFromCookieStorageAdapter.ts

@@ -18,6 +21,7 @@ const ONE_YEAR_IN_MS = 365 * 24 * 60 * 60 * 1000;
 */
 export const createKeyValueStorageFromCookieStorageAdapter = (
 	cookieStorageAdapter: CookieStorage.Adapter,
+	validatorMap?: KeyValueStorageMethodValidator,


Given that we have control over this interface & it isn't customer facing, I wonder why even need createTokenValidator with the interface with getItem? Couldn't this just be a function predicate we pass in that ultimately calls isValidCognitoToken and returns true/false?

This would also update a public interface. If customers are creating a custom KeyValueStorageFromCookieStorage they can now pass in a validator also optionally to it (2nd parameter). Since it's a custom KeyValueStorage it's up to them on how they handle the validation.

jimblanc · 2024-05-20T21:51:50Z

packages/core/package.json

@@ -54,6 +54,7 @@
 		"@aws-sdk/types": "3.398.0",
 		"@smithy/util-hex-encoding": "2.0.0",
 		"@types/uuid": "^9.0.0",
+		"aws-jwt-verify": "^4.0.1",


Sanity check: No concerns with RN or anything (polyfills, etc) here right as this code should only execute on SSR?

Yep that's right, the isValidCognitoToken util is added to coreUtils and is used only on the server side for validation. No change on the client side

…o tokens (aws-amplify#13385)" This reverts commit 0b72b32.

…o tokens (#13385)" (#13410) This reverts commit 0b72b32.

ashwinkumar6 marked this pull request as ready for review May 15, 2024 22:34

ashwinkumar6 requested review from a team as code owners May 15, 2024 22:34

Samaritan1011001 reviewed May 15, 2024

View reviewed changes

israx suggested changes May 16, 2024

View reviewed changes

israx reviewed May 16, 2024

View reviewed changes

ashwinkumar6 force-pushed the next-jwt-verifier branch from 395ca9a to c3dc5b6 Compare May 17, 2024 17:23

feat(adapter-nextjs): validate if access and id tokens are valid cogn…

8bad71e

…ito tokens

ashwinkumar6 force-pushed the next-jwt-verifier branch from c3dc5b6 to 8bad71e Compare May 17, 2024 18:24

HuiSF reviewed May 17, 2024

View reviewed changes

packages/adapter-nextjs/src/utils/validator.ts Outdated Show resolved Hide resolved

packages/adapter-nextjs/src/utils/validator.ts Outdated Show resolved Hide resolved

packages/adapter-nextjs/src/utils/validator.ts Outdated Show resolved Hide resolved

Ashwin Kumar added 4 commits May 17, 2024 13:50

add unit tests and cleanups

6c053e0

increase bundle size

f007450

resolve conflicts

703394f

increase bundle size

e741faa

israx reviewed May 17, 2024

View reviewed changes

packages/aws-amplify/package.json Show resolved Hide resolved

israx reviewed May 17, 2024

View reviewed changes

israx self-requested a review May 17, 2024 22:00

HuiSF reviewed May 17, 2024

View reviewed changes

ashwinkumar6 and others added 5 commits May 20, 2024 08:25

Update packages/core/src/adapterCore/serverContext/types/KeyValueStor…

d149005

…ageValidator.ts Co-authored-by: Hui Zhao <[email protected]>

Update packages/adapter-nextjs/__tests__/utils/createTokenValidator.t…

7970a41

…est.ts Co-authored-by: Hui Zhao <[email protected]>

Update packages/core/src/adapterCore/serverContext/types/KeyValueStor…

2b300b4

…ageValidator.ts Co-authored-by: Hui Zhao <[email protected]>

address feedback

afb3ba2

address feedback

dffa3cb

ashwinkumar6 requested review from Samaritan1011001 and HuiSF May 20, 2024 17:09

israx reviewed May 20, 2024

View reviewed changes

packages/adapter-nextjs/src/utils/createTokenValidator.ts Outdated Show resolved Hide resolved

israx reviewed May 20, 2024

View reviewed changes

israx self-requested a review May 20, 2024 21:13

ashwinkumar6 and others added 2 commits May 20, 2024 14:26

Update packages/adapter-nextjs/src/utils/createTokenValidator.ts

27f25d7

Co-authored-by: israx <[email protected]>

address feedback

008eb27

jimblanc reviewed May 20, 2024

View reviewed changes

jimblanc approved these changes May 21, 2024

View reviewed changes

israx approved these changes May 21, 2024

View reviewed changes

ashwinkumar6 merged commit 0b72b32 into aws-amplify:main May 21, 2024
30 checks passed

jimblanc added a commit to jimblanc/amplify-js that referenced this pull request May 22, 2024

Revert "feat(core): validate if access and id tokens are valid cognit…

6ff3beb

…o tokens (aws-amplify#13385)" This reverts commit 0b72b32.

jimblanc added a commit that referenced this pull request May 22, 2024

Revert "feat(core): validate if access and id tokens are valid cognit…

82d53fa

…o tokens (#13385)" (#13410) This reverts commit 0b72b32.

ashwinkumar6 mentioned this pull request May 22, 2024

release(required): adapter-nextjs validate if access and id tokens are valid cognito tokens #13415

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): validate if access and id tokens are valid cognito tokens #13385

feat(core): validate if access and id tokens are valid cognito tokens #13385

ashwinkumar6 commented May 15, 2024 •

edited

Loading

Samaritan1011001 May 15, 2024

HuiSF May 17, 2024

HuiSF May 17, 2024

ashwinkumar6 May 20, 2024

Samaritan1011001 May 15, 2024

ashwinkumar6 May 20, 2024 •

edited

Loading

israx left a comment •

edited

Loading

israx May 16, 2024 •

edited

Loading

ashwinkumar6 May 20, 2024

ashwinkumar6 commented May 17, 2024

israx left a comment

HuiSF left a comment

HuiSF May 17, 2024

HuiSF May 17, 2024

ashwinkumar6 May 20, 2024

israx May 20, 2024

ashwinkumar6 May 20, 2024

israx May 20, 2024

ashwinkumar6 May 20, 2024

israx left a comment

jimblanc left a comment

jimblanc May 20, 2024

ashwinkumar6 May 20, 2024

jimblanc May 20, 2024

ashwinkumar6 May 20, 2024 •

edited

Loading

feat(core): validate if access and id tokens are valid cognito tokens #13385

feat(core): validate if access and id tokens are valid cognito tokens #13385

Conversation

ashwinkumar6 commented May 15, 2024 • edited Loading

Description of changes

Description of how you validated changes

Checklist

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

ashwinkumar6 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

israx left a comment • edited Loading

Choose a reason for hiding this comment

israx May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwinkumar6 commented May 17, 2024

israx left a comment

Choose a reason for hiding this comment

HuiSF 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

israx left a comment

Choose a reason for hiding this comment

jimblanc 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

ashwinkumar6 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

ashwinkumar6 commented May 15, 2024 •

edited

Loading

ashwinkumar6 May 20, 2024 •

edited

Loading

israx left a comment •

edited

Loading

israx May 16, 2024 •

edited

Loading

ashwinkumar6 May 20, 2024 •

edited

Loading