automate bootstrapping of integrations tests #1289

petrkalos · 2024-05-20T14:57:11Z

Feature or Bugfix

Feature

Detail

introduce an SSM param that holds the test userdata
migrate ConfigureCognito step from a frontend codebuild step to a TriggerFunction that runs after the deployment of the CognitoClient
add logic in ConfigureCognito to create the test users and assigned them to groups if with_approval_tests=True (reads info from SSM param)
- minor: change prints to logging
tests are reading the userdata from the SSM param
if with_approval_tests=True Cognito allows ALLOW_USER_PASSWORD_AUTH auth flow

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.

* introduce a secret that holds the test userdata * move ConfigureCognito step from frontend to backend * add logic in ConfigureCognito to create the test users and assigned them to groups if `with_approval_tests=True` (reads info from the secret) * tests are reading the userdata from the secret

SofiaSazonova · 2024-05-20T15:34:55Z

tests_new/integration_tests/README.md

@@ -3,7 +3,18 @@ The purpose of these tests is to automatically validate functionalities of data.

 ## Pre-requisites
 - A real deployment of data.all in AWS
- 4 Cognito users (at the moment only Cognito is supported) like the ones defined in `conftest`(e.g. `testUser1` with password `Pass1Word!`)
+- A SecretManager secret (`'{self.resource_prefix}-{target_env["envname"]}-cognito-test-users'`) with the followings contents


Do we need SecretManager here? We don't need auto rotation or so, I think SSM Parameter store is enough, also it's free.

That's a good point, I am not an expert but my go-to is that when a password is involved I use SecretsManager (even if it's just for tests). Apart from the pricing aspect I don't see a downside.

We had issues in the past with checkov flagging some secrets in SecretsManager because some best practices dictate that encrypted Secrets in secret manager should be rotated at least annually and encrypted with a KMS key. So if we add a secret, it needs to be encrypted with KMS and with rotation enabled. So as @SofiaSazonova was saying if it does not rotate, we might go for SSM. The question is, do we need to rotate them? what can happen if the password gets exposed?

In the scenario where a malicious user:

knows that a company uses data.all. Tries to guess the cloudfront domain that might be something like .dataall.com

tries to hit the domain with the default test user password (company is careless and did not change it)

can overflood a company CICD pipeline with requests?

After discussing with @dlpzx offline we decided to use SSM params for simplicity. Thanks for the spot on commend Sofia :)

SofiaSazonova · 2024-05-20T15:36:11Z

tests_new/integration_tests/README.md

@@ -3,7 +3,18 @@ The purpose of these tests is to automatically validate functionalities of data.

 ## Pre-requisites
 - A real deployment of data.all in AWS
- 4 Cognito users (at the moment only Cognito is supported) like the ones defined in `conftest`(e.g. `testUser1` with password `Pass1Word!`)
+- A SecretManager secret (`'{self.resource_prefix}-{target_env["envname"]}-cognito-test-users'`) with the followings contents
+    ```


I know, that's for test purposes, but why expose it in code (especially opensource)? May be it's better just to leave comment like: userdata with username, password and groups.

I just gave an example password, real deployments should use a different one. Is your concern that people will just copy and paste the example and hence make themselves vulnerable?

dlpzx · 2024-05-21T08:22:43Z

tests_new/integration_tests/README.md

+        "testUser4": {"password": "yourPassword", "groups": ["testGroup4"]}
+    }
+    ```
+- If you are not using Cognito then you must manually create the users/groups


If not using Cognito, they still need to create the secret to store the passwords right?

Indeed, if they are not using Cognito they must create the users manually and then create the secret for the CodeBuild job to read from. Will fix

dlpzx · 2024-05-21T08:32:10Z

deploy/configs/cognito_urls_config.py

+        else:
+            raise e
+
+    print('Updating Canaries user password...')


nit: prints logs referencing CW canaries. Can we replace prints by logs?

dlpzx · 2024-05-21T12:07:47Z

deploy/configs/cognito_urls_config.py


    cognito = boto3.client('cognito-idp', region_name=region)
-    try:
-        user_pool = cognito.describe_user_pool_client(UserPoolId=user_pool_id, ClientId=app_client)
+    user_pool = cognito.describe_user_pool_client(UserPoolId=user_pool_id, ClientId=app_client)


You removed the general try/except because if it fails then the stack will rollback right?

Nope, I removed it because it was only catching the exception printing and throwing it again which isn't required as the exception will currently bubble up and print the exact same information...

except ClientError as e: print(f'Failed to setup cognito due to: {e}') raise e

dlpzx

looks good!

) ### Feature or Bugfix - Bugfix ### Detail Integration tests are running from a CodeBuild job in the tooling account but the `cognito-test-users` param exists in the environment account. ### Relates Fixes an issue introduced #1289 ### 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.

petrkalos requested a review from dlpzx May 20, 2024 14:57

SofiaSazonova requested changes May 20, 2024

View reviewed changes

dlpzx reviewed May 21, 2024

View reviewed changes

petrkalos added 3 commits May 21, 2024 14:43

convert print to log

56867f6

move to SSM

42b458b

codebuild to trigger

5dfeedd

dlpzx reviewed May 21, 2024

View reviewed changes

dlpzx approved these changes May 21, 2024

View reviewed changes

SofiaSazonova approved these changes May 21, 2024

View reviewed changes

petrkalos merged commit e2b70cb into main May 22, 2024
10 checks passed

petrkalos deleted the feature/test_users branch May 22, 2024 07:15

petrkalos mentioned this pull request May 22, 2024

codebuild reads cognito-test-users param from environment account #1295

Merged

petrkalos mentioned this pull request Jun 20, 2024

new deployments fail to configure Cognito #1353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

automate bootstrapping of integrations tests #1289

automate bootstrapping of integrations tests #1289

petrkalos commented May 20, 2024 •

edited

Loading

SofiaSazonova May 20, 2024

petrkalos May 20, 2024

dlpzx May 21, 2024

petrkalos May 21, 2024 •

edited

Loading

SofiaSazonova May 20, 2024

petrkalos May 20, 2024

dlpzx May 21, 2024

petrkalos May 21, 2024

dlpzx May 21, 2024

dlpzx May 21, 2024

petrkalos May 21, 2024

dlpzx left a comment

automate bootstrapping of integrations tests #1289

automate bootstrapping of integrations tests #1289

Conversation

petrkalos commented May 20, 2024 • edited Loading

Feature or Bugfix

Detail

Security

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrkalos May 21, 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

dlpzx left a comment

Choose a reason for hiding this comment

petrkalos commented May 20, 2024 •

edited

Loading

petrkalos May 21, 2024 •

edited

Loading