Added Token Validations #1682

noah-paige · 2024-11-04T16:10:22Z

Feature or Bugfix

Bugfix

Detail

This PR does the following w.r.t Cognito IdP and Auth Flow
- Changes Authorizer from built-in Cognito Authorizer to Custom Authorizer to validate token signature, issuer, and expiry time, etc.
- Adds aditional step to execute GET API on Cognito's /oauth/userInfo/ endpoint to ensure access Token validity

Allows data.all API request to execute if the above criteria are met

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features?
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users?
- Have you used the least-privilege principle? How?

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

noah-paige · 2024-11-04T22:52:33Z

deploy/stacks/lambda_api.py

petrkalos · 2024-11-05T12:58:14Z

deploy/stacks/lambda_api.py

+                'LOG_LEVEL': 'DEBUG',
+                'user_info_url': f'https://{user_pool_domain.domain_name}.auth.{self.region}.amazoncognito.com/oauth2/userInfo',
+                'custom_auth_provider': 'Cognito',
+                'custom_auth_url': f'https://cognito-idp.{self.region}.amazonaws.com/{user_pool.user_pool_id}',


instead of passing the user_info_url explicitly we can use https://{custom_auth_url}/.well-known/openid-configuration and read the userinfo_endpoint field.

using https://{custom_auth_url}/.well-known/openid-configuration for userinfo URL now

should we remove the custom_auth_jwks_url since you are pulling this from openid-configuration endpoint?

removed in latest

TejasRGitHub · 2024-11-06T15:27:20Z

frontend/src/authentication/hooks/useToken.js

@@ -21,14 +23,18 @@ export const useToken = () => {
              await auth.signinSilent();
            }
            const t = auth.user.id_token;
+            const at = auth.user.access_token;


Are we now using both Access and Identity token ?

Do we need any particular token for any specific use ? Like a specific gql endpoint needs an identity token and cannot work with access token ?

debating whether we should just send access token and forget about ID

currently we send both which may be overkill

ID token contains headers like email that we do need. However when we run a request with access token against userinfo endpoint we get that information (such as email) in response that we can add to context

Additionally, access tokens (for cognito at least) contain user and group information - not sure if that is standard

At a high level the token uses are:

the access token is needed to invoke the /userinfo endpoint

the verified claims of the ID Token are needed to populate context information (email, user, groups) for downstream lambda (this theoretically could be sourced elsewhere)

dlpzx · 2024-11-06T15:32:53Z

deploy/stacks/backend_stack.py

@@ -35,6 +35,7 @@ def __init__(
        id,
        envname: str = 'dev',
        resource_prefix='dataall',
+        tooling_region=None,


tooling_region is not used anywhere

TejasRGitHub · 2024-11-06T15:58:47Z

deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py

    effect = 'Allow'
    policy = AuthServices.generate_policy(verified_claims, effect, incoming_event['methodArn'])
-    logger.debug('Generated policy is ', policy)
+    logger.debug('Generated policy is ', json.dumps(policy))


logger.debug(f'Generated policy is json.dumps(policy)').

using f-string,. I think this logger.debug('Generated policy is ', json.dumps(policy)) doesn't work

yup I made this change earlier and just ran into this - updating now

logger.debug('Generated policy is ', policy) was also not showing policy so opting for f string approach

dlpzx · 2024-11-06T15:59:05Z

deploy/custom_resources/custom_authorizer/jwt_services.py

            return payload
-        except ExpiredSignatureError:
+        except jwt.exceptions.ExpiredSignatureError:


nit: I would directly raise these Exceptions here instead of passing the None and then raising the general exception

TejasRGitHub · 2024-11-06T16:09:09Z

deploy/custom_resources/custom_authorizer/jwt_services.py

 import logging

 logger = logging.getLogger()
 logger.setLevel(os.environ.get('LOG_LEVEL', 'INFO'))

-# Configs required to fetch public keys from JWKS


We don't need this because pyJWT does it with some inbuilt method ?

yes - https://pyjwt.readthedocs.io/en/latest/usage.html (re: PyJWKClient())

TejasRGitHub · 2024-11-06T17:09:17Z

deploy/stacks/lambda_api.py

-                'custom_auth_provider': custom_auth.get('provider'),
-                'custom_auth_url': custom_auth.get('url'),
-                'custom_auth_client': custom_auth.get('client_id'),
-                'custom_auth_jwks_url': custom_auth.get('jwks_url'),


Are we removing the custom_auth_jwks_url URL completely ? and using the .well-known/openid-configuration instead ?

TejasRGitHub · 2024-11-06T17:18:30Z

frontend/src/authentication/hooks/useToken.js

@@ -45,5 +51,5 @@ export const useToken = () => {
      );


In the if (!token ) should it be instead if ( !token and !accessToken ) ?

updated only using access token now

TejasRGitHub · 2024-11-06T17:22:47Z

backend/dataall/base/utils/api_handler_utils.py



 def redact_creds(event):
-    if 'headers' in event and 'Authorization' in event['headers']:
+    if event.get('headers', {}).get('Authorization'):


Can anyone please tell why are we redacting these ?

user creds information does not need to be logged and is no longer relevant for the remaining request lifecycle - opting to redact that info

Instead of redacting , can we just extract useful information and then pass it onto our lambda ?

TejasRGitHub · 2024-11-06T21:57:28Z

deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py

+    user_info = jwt_service.validate_access_token(auth_token)
+
+    # Validate JWT
+    verified_claims = jwt_service.validate_jwt_token(auth_token[7:])
    if not verified_claims:


Since the method validate_jwt_token is now raising exception this if block can be removed

petrkalos · 2024-11-07T09:19:37Z

deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py

@@ -16,21 +17,33 @@
 Custom Lambda Authorizer is attached to the API Gateway. Check the deploy/stacks/lambda_api.py for more details on deployment
 """

+OPENID_CONFIG_PATH = os.path.join(os.environ.get('custom_auth_url', ''), '.well-known', 'openid-configuration')


Should we use os.environ['custom_auth_url'] and let it throw if auth_url isn't specified? otherwise it will construct a wrong url and later on will fail with an obscure message.

sure that is a good call out - just made that change to os.environ['custom_auth_url']

petrkalos · 2024-11-07T09:25:23Z

tests_new/integration_tests/client.py

+        token_url = os.path.join(idp_domain_url, 'oauth2', 'token')
+        login_url = os.path.join(idp_domain_url, 'login')
+
+        client_id = os.environ['COGNITO_CLIENT']


This is now a quite generic OAuth2 client, should we rename this to CLIENT_ID?

there are still some particularities that makes integration tests only work with cognito. I am inclined to make it fully generic in a separate PR out of scope from here and leave it as it was inherited in this PR for now if that is okay

petrkalos · 2024-11-07T09:30:14Z

tests_new/integration_tests/client.py

+        token = uuid.uuid4()
+        scope = 'aws.cognito.signin.user.admin openid'
+
+        idp_domain_url = os.environ['IDP_DOMAIN_URL']


You must amend the env variables in the pipeline step

believe I already do that in the deploy/stacks/pipeline.py changes in this PR - will tag you on those lines for ref

petrkalos · 2024-11-07T09:31:19Z

tests_new/integration_tests/client.py

@TejasRGitHub, take a look at this change as I think it will enable support for any IdP

Thanks @petrkalos for pointing at this. Will take a look

petrkalos · 2024-11-07T09:32:16Z

deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py

@@ -16,21 +17,33 @@
 Custom Lambda Authorizer is attached to the API Gateway. Check the deploy/stacks/lambda_api.py for more details on deployment
 """

+OPENID_CONFIG_PATH = os.path.join(os.environ.get('custom_auth_url', ''), '.well-known', 'openid-configuration')
+jwt_service = JWTServices(OPENID_CONFIG_PATH)


nit: I would capitalize global variables

wasn't sure for global class instance - capitalizing now

noah-paige · 2024-11-07T13:37:23Z

deploy/stacks/pipeline.py

@@ -716,6 +716,8 @@ def set_approval_tests_stage(
                                    'aws sts get-caller-identity --profile buildprofile',
                                    f'export COGNITO_CLIENT=$(aws ssm get-parameter --name /dataall/{target_env["envname"]}/cognito/appclient --profile buildprofile --output text --query "Parameter.Value")',
                                    f'export API_ENDPOINT=$(aws ssm get-parameter --name /dataall/{target_env["envname"]}/apiGateway/backendUrl --profile buildprofile --output text --query "Parameter.Value")',
+                                    f'export IDP_DOMAIN_URL=https://$(aws ssm get-parameter --name /dataall/{target_env["envname"]}/cognito/domain --profile buildprofile --output text --query "Parameter.Value").auth.us-east-1.amazoncognito.com',


noah-paige · 2024-11-07T13:40:12Z

deploy/custom_resources/custom_authorizer/custom_authorizer_lambda.py

@@ -16,21 +17,33 @@
 Custom Lambda Authorizer is attached to the API Gateway. Check the deploy/stacks/lambda_api.py for more details on deployment
 """

+OPENID_CONFIG_PATH = os.path.join(os.environ.get('custom_auth_url', ''), '.well-known', 'openid-configuration')


sure that is a good call out - just made that change to os.environ['custom_auth_url']

dlpzx · 2024-11-07T14:58:05Z

deploy/stacks/lambda_api.py

+
+        # Initialize Klayers
+        runtime = _lambda.Runtime.PYTHON_3_9
+        klayers = Klayers(self, python_version=runtime, region=self.region)


- Bugfix - This PR does the following w.r.t Cognito IdP and Auth Flow - Changes Authorizer from built-in Cognito Authorizer to Custom Authorizer to validate token signature, issuer, and expiry time, etc. - Adds aditional step to execute GET API on Cognito's `/oauth/userInfo/` endpoint to ensure access Token validity Allows data.all API request to execute if the above criteria are met - <URL or Ticket> Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

### Feature or Bugfix - Security ### Detail * get-parameter CloudfrontDistributionDomainName from us-east-1 (#1687 ) * Added Token Validations (#1682) * add warning to untrust data.all account when removing an environment (#1685) * add custom domain support for apigw (#1679) * Lambda Event Logs Handling (#1678) * Upgrade Spark version to 3.3 (#1675) - a0c63a4 * ES Search Query Collect All Response (#1631) * Extend Tenant Perms Coverage (#1630) * Limit Response info dataset queries (#1665) * Add Removal Policy Retain to Bucket Policy IaC (#1660) * log API handler response only for LOG_LEVEL DEBUG. Set log level INFO for prod deployments (#1662) * Add permission checks to markNotificationAsRead + deleteNotification (#1654) * Added error view and unified utility to check tenant user (#1657 * Userguide signout flow (#1629) ### Relates - Security release ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com> Co-authored-by: Petros Kalos <kalosp@amazon.com>

### Feature or Bugfix  - Enhancement ### Detail - Add explicit token duration (60 min) over default 60 min ### Relates - #1682 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

noah-paige added 7 commits November 4, 2024 11:07

Add checks to ensure access tokenn validity

155db7a

store access token non cognito

b70b55c

Enforce validation on Custom Auth Lambda

88c03e5

clean up

5d1c2c9

env var user info endpoint

02b5d36

fix lambda API CDK

7117d55

remove unused import

cba99e9

noah-paige added 4 commits November 4, 2024 18:30

clean up and lint

09caacb

handle cognito groups header

0e77776

update custom auth logic

e6fcf8f

fix

58a5440

petrkalos reviewed Nov 5, 2024

View reviewed changes

deploy/stacks/lambda_api.py Show resolved Hide resolved

petrkalos reviewed Nov 5, 2024

View reviewed changes

noah-paige added 5 commits November 5, 2024 09:43

fix custom Auth logic

c04760a

fix cognito groups list handling

b5c0c08

lint

25334bc

update checkov baseline

fca14f2

use well-known/openid-config

64fa2f1

noah-paige marked this pull request as ready for review November 5, 2024 17:29

noah-paige added 4 commits November 5, 2024 17:43

Update deps custom authorizer

d416a55

clean up requirements

913c1ce

update cryptography version

a8cfe08

Add cryptography lambda layer

647b689

noah-paige requested review from dlpzx and petrkalos November 6, 2024 13:24

noah-paige self-assigned this Nov 6, 2024

Reformat

eddcb86

TejasRGitHub reviewed Nov 6, 2024

View reviewed changes

dlpzx reviewed Nov 6, 2024

View reviewed changes

TejasRGitHub reviewed Nov 6, 2024

View reviewed changes

dlpzx reviewed Nov 6, 2024

View reviewed changes

TejasRGitHub reviewed Nov 6, 2024

View reviewed changes

Fix Integration Tests to fetch ID and Access Tokens

f30dd4f

TejasRGitHub reviewed Nov 6, 2024

View reviewed changes

noah-paige added 5 commits November 6, 2024 12:28

use cdk klayers

320f652

refactor jwt services

ff910c7

fix string formatting

9b9cbad

raise exceptions

451b0d4

only use access token, remove id token

0e329a3

TejasRGitHub reviewed Nov 6, 2024

View reviewed changes

noah-paige added 3 commits November 6, 2024 16:59

update exception str

5a0893b

Clean Up

8b8eb1a

merge latest os main

26bd79c

petrkalos requested changes Nov 7, 2024

View reviewed changes

petrkalos reviewed Nov 7, 2024

View reviewed changes

PR comments

86db3e8

noah-paige commented Nov 7, 2024

View reviewed changes

fix IDP_DOMAIN_URL setting

8db81a1

petrkalos approved these changes Nov 7, 2024

View reviewed changes

dlpzx reviewed Nov 7, 2024

View reviewed changes

noah-paige merged commit 5069bf8 into main Nov 7, 2024
10 checks passed

dlpzx mentioned this pull request Nov 8, 2024

2.6.1 Security features #1686

Merged

noah-paige mentioned this pull request Nov 14, 2024

add explicit token duration config for both JWTs #1698

Merged

Added Token Validations #1682

Added Token Validations #1682

Conversation

noah-paige commented Nov 4, 2024 • edited Loading

Feature or Bugfix

Detail

Relates

Security

noah-paige commented Nov 4, 2024 • edited Loading

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

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

petrkalos Nov 7, 2024 • edited Loading

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

noah-paige commented Nov 4, 2024 •

edited

Loading

noah-paige commented Nov 4, 2024 •

edited

Loading

petrkalos Nov 7, 2024 •

edited

Loading