Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cognito): Add new checks related with cognito service #3898

Merged
merged 14 commits into from May 8, 2024

Conversation

pedrooot
Copy link
Member

@pedrooot pedrooot commented Apr 30, 2024

Description

New checks related with cognito service for aws are added:

  • cognito_user_pool_temporary_password_expiration -> Cognito user pools temporary passwords set by administrators expire in 7 days or less

  • cognito_user_pool_password_policy_lowercase -> Ensure Cognito User Pool has password policy to require at least one lowercase letter

  • cognito_user_pool_password_policy_minimum_length_14 -> Ensure that the password policy for your user pools require a minimum length of 14 or greater

  • cognito_user_pool_password_policy_number -> Ensure that the password policy for your user pool requires a number

  • cognito_user_pool_password_policy_symbol -> Ensure that the password policy for your Amazon Cognito user pool requires at least one symbol.

  • cognito_user_pool_password_policy_uppercase -> Ensure that the password policy for your user pool requires at least one uppercase letter

  • cognito_user_pool_mfa_enabled -> Cognito user pools require multi factor authentication (MFA)

  • cognito_user_pool_web_acl_associated -> Cognito user pools should be protected by AWS Web Application Firewall

  • cognito_user_pool_prevent_reveal_user_existence -> Cognito user pools should prevent error messages that reveal user existence

  • cognito_user_pool_token_revocation_enabled -> Cognito user pools should have enabled token revocation

  • cognito_user_pool_advanced_security_enabled -> Cognito user pools has advanced security enabled with full-function

  • cognito_user_pool_advanced_security_block_sign_in_compromised_credentials -> Cognito user pools has advanced security enabled with full-function to block sign-in by users with suspected compromised credentials

  • cognito_user_pool_advanced_security_adaptative_authentication_block_sign_in -> Cognito user pools has advanced security enabled with full-function with adaptive authentication automatic risk response as block sign-in

  • cognito_user_pool_deletion_protection_enabled -> Cognito user pools deletion protection enabled to prevent accidental deletion

  • cognito_user_pool_self_registration_enabled -> Self-registration is enabled on the user pools and it is associated with an identity pool (and show the roles that can be asssumed)

  • cognito_identity_pool_guest_access_enabled -> Guest access is enabled on the identity pool(and show the roles that can be assumed)

License

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

@pedrooot pedrooot added the no-merge Please, DO NOT MERGE this PR. label Apr 30, 2024
@pedrooot pedrooot requested review from a team as code owners April 30, 2024 07:41
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label Apr 30, 2024
report.resource_arn = pool.arn
if (
pool.advanced_security_mode == "ENFORCED"
and pool.risk_configuration.account_takeover_risk_configuration.get(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to map this into an object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point

== "BLOCK"
):
report.status = "PASS"
report.status_extended = f"User pool {pool.id} has advanced security enforced with adaptative authentication sign-in blocked."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get the tags in the service, check other AWS services for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 92.99363% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 86.44%. Comparing base (a54a0dd) to head (8e2fb36).
Report is 18 commits behind head on master.

Files Patch % Lines
.../providers/aws/services/cognito/cognito_service.py 79.85% 28 Missing ⚠️
...cognito_user_pool_temporary_password_expiration.py 90.47% 2 Missing ⚠️
...wler/providers/aws/services/wafv2/wafv2_service.py 33.33% 2 Missing ⚠️
...ed/cognito_user_pool_self_registration_disabled.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3898      +/-   ##
==========================================
+ Coverage   86.37%   86.44%   +0.06%     
==========================================
  Files         748      767      +19     
  Lines       23333    23856     +523     
==========================================
+ Hits        20155    20622     +467     
- Misses       3178     3234      +56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sergargar sergargar removed the no-merge Please, DO NOT MERGE this PR. label May 6, 2024
@@ -0,0 +1,30 @@
{
"Provider": "aws",
"CheckID": "cognito_identity_pool_guest_access_enabled",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"CheckID": "cognito_identity_pool_guest_access_enabled",
"CheckID": "cognito_identity_pool_guest_access_disabled",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

f"Identity pool {identity_pool.id} has guest access enabled."
)
if identity_pool.roles.unauthenticated:
report.status_extended = f"Identity pool {identity_pool.id} has guest access enabled and unauthenticated role {identity_pool.roles.unauthenticated} defined."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"Identity pool {identity_pool.id} has guest access enabled and unauthenticated role {identity_pool.roles.unauthenticated} defined."
report.status_extended = f"Identity pool {identity_pool.id} has guest access enabled assuming the role {identity_pool.roles.unauthenticated}."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 99 to 104
].describe_user_pool_client(
UserPoolId=user_pool.id,
ClientId=self.regional_clients[
user_pool.region
].list_user_pool_clients(UserPoolId=user_pool.id)[
"UserPoolClients"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, create two different functions for the describe and list calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

tags: Optional[list] = []
tags: Optional[dict]
account_recovery_settings: Optional[dict]
user_pool_client: Optional[UserPoolClient]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, create a list of UserPoolClient since there can be more than one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

account_takeover_risk_configuration: Optional[AccountTakeoverRiskConfiguration]


class UserPoolClient(BaseModel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the ID and Name of the client when doing the list

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

elif pool.advanced_security_mode == "AUDIT":
report.status = "FAIL"
report.status_extended = (
f"User pool {pool.id} has advanced security audit enabled."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"User pool {pool.id} has advanced security audit enabled."
f"User pool {pool.id} has advanced security enabled but with audit-only mode."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if pool.advanced_security_mode == "ENFORCED":
report.status = "PASS"
report.status_extended = (
f"User pool {pool.id} has advanced security enforced."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"User pool {pool.id} has advanced security enforced."
f"User pool {pool.id} has advanced security enforced with full-function mode."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.resource_id = pool.id
report.resource_arn = pool.arn
report.resource_tags = pool.tags
if pool.user_pool_client.prevent_user_existence_errors == "ENABLED":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, iterate for each client and add a finding for each one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resource ID and name must be the user_pool_client one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,30 @@
{
"Provider": "aws",
"CheckID": "cognito_user_pool_prevent_reveal_user_existence",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"CheckID": "cognito_user_pool_prevent_reveal_user_existence",
"CheckID": "cognito_user_pool_client_prevent_user_existence_errors",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

else:
associated_identity_pools.append(identity_pool.name)
if associated_identity_pools:
report.status_extended = f"User pool {user_pool.id} has self registration enabled and is associated with the following identity pools: {(', ').join(associated_identity_pools)}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {user_pool.id} has self registration enabled and is associated with the following identity pools: {(', ').join(associated_identity_pools)}"
report.status_extended = f"User pool {user_pool.id} has self registration enabled assuming the role(s): {(', ').join(associated_identity_pools)}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status_extended = (
f"User pool {user_pool.id} has self registration enabled."
)
associated_identity_pools = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
associated_identity_pools = []
associated_identity_pool_authenticated_roles = []

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,30 @@
{
"Provider": "aws",
"CheckID": "cognito_user_pool_self_registration_enabled",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"CheckID": "cognito_user_pool_self_registration_enabled",
"CheckID": "cognito_user_pool_self_registration_disabled",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 4 to 23
"CheckTitle": "Ensure cognito user pools has advanced security enabled with full-function with adaptive authentication automatic risk response as block sign-in",
"CheckType": [],
"ServiceName": "cognito",
"SubServiceName": "",
"ResourceIdTemplate": "arn:aws:cognito-idp:region:account:userpool/userpool-id",
"Severity": "medium",
"ResourceType": "AwsCognitoUserPool",
"Description": "Amazon Cognito advanced security features provide adaptive authentication and protection against compromised credentials. Adaptive authentication helps protect your applications from malicious actors and compromised credentials by evaluating the risk associated with each user login and providing the appropriate level of security to mitigate that risk. Adaptive authentication is a feature of advanced security that uses contextual data and intelligent mechanisms to evaluate the risk of each authentication attempt and respond with the appropriate level of security. Adaptive authentication can be configured to block sign-in when the risk is too high.",
"Risk": "If adaptive authentication is not enabled, your user pool is more vulnerable to attacks that use compromised credentials. Adaptive authentication helps protect your applications from malicious actors and compromised credentials by evaluating the risk associated with each user login and providing the appropriate level of security to mitigate that risk.",
"RelatedUrl": "https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-advanced-security.html",
"Remediation": {
"Code": {
"CLI": "",
"NativeIaC": "",
"Other": "",
"Terraform": ""
},
"Recommendation": {
"Text": "To enable adaptive authentication with automatic risk response as block sign-in, follow the Amazon Cognito documentation",
"Url": "https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-advanced-security.html"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review this again with the new check name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

]
):
report.status = "PASS"
report.status_extended = f"User pool {pool.id} has advanced security enforced with adaptative authentication sign-in blocked."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {pool.id} has advanced security enforced with adaptative authentication sign-in blocked."
report.status_extended = f"User pool {pool.id} blocks all potential malicious sign-in attempts."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if pool.advanced_security_mode == "ENFORCED" and all(
[
pool.risk_configuration.account_takeover_risk_configuration.low_action
and pool.risk_configuration.account_takeover_risk_configuration.low_action.get(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omit the EventAction attribute and add it in the service to the action

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status_extended = f"User pool {pool.id} has advanced security enforced with adaptative authentication sign-in blocked."
else:
report.status = "FAIL"
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with adaptative authentication sign-in blocked."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with adaptative authentication sign-in blocked."
report.status_extended = f"User pool {pool.id} does not block all potential malicious sign-in attempts."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,30 @@
{
"Provider": "aws",
"CheckID": "cognito_user_pool_advanced_security_block_sign_in_compromised_credentials",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"CheckID": "cognito_user_pool_advanced_security_block_sign_in_compromised_credentials",
"CheckID": "cognito_user_pool_blocks_compromised_credentials_sign_in_attempts",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 4 to 24
"CheckTitle": "Ensure cognito user pools has advanced security enabled with full-function to block sign-in by users with suspected compromised credentials",
"CheckType": [],
"ServiceName": "cognito",
"SubServiceName": "",
"ResourceIdTemplate": "arn:aws:cognito-idp:region:account:userpool/userpool-id",
"Severity": "medium",
"ResourceType": "AwsCognitoUserPool",
"Description": "Checks whether advanced security features are enabled for an Amazon Cognito User Pool to block sign-in by users with suspected compromised credentials.",
"Risk": "If advanced security features are not enabled for an Amazon Cognito User Pool, the user pool will not be able to block sign-in by users with suspected compromised credentials.",
"RelatedUrl": "https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-advanced-security.html",
"Remediation": {
"Code": {
"CLI": "",
"NativeIaC": "",
"Other": "",
"Terraform": ""
},
"Recommendation": {
"Text": "To enable advanced security features for an Amazon Cognito User Pool, follow the steps in the Amazon Cognito Developer Guide.",
"Url": "https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pool-settings-advanced-security.html"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review this again with the new check name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

):
print(pool.risk_configuration)
report.status = "PASS"
report.status_extended = f"User pool {pool.id} has advanced security enforced with compromised credentials sign-in blocked."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {pool.id} has advanced security enforced with compromised credentials sign-in blocked."
report.status_extended = f"User pool {pool.id} block sign-in attempts with suspected compromised credentials."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status_extended = f"User pool {pool.id} has advanced security enforced with compromised credentials sign-in blocked."
else:
report.status = "FAIL"
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with compromised credentials sign-in blocked."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with compromised credentials sign-in blocked."
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with compromised credentials sign-in blocked."
Suggested change
report.status_extended = f"User pool {pool.id} does not have advanced security enforced with compromised credentials sign-in blocked."
report.status_extended = f"User pool {pool.id} does not block sign-in attempts with suspected compromised credentials."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@sergargar sergargar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, review my comments. Good job 🔝

)
== "BLOCK"
):
print(pool.risk_configuration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(pool.risk_configuration)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sergargar
Copy link
Member

Please, remove all report.resource_name and add the name of the resource to the status extended.

@pedrooot pedrooot requested a review from sergargar May 7, 2024 16:07
== "BLOCK"
):
report.status = "PASS"
report.status_extended = f"User pool {pool.name} block sign-in attempts with suspected compromised credentials."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool {pool.name} block sign-in attempts with suspected compromised credentials."
report.status_extended = f"User pool {pool.name} blocks sign-in attempts with suspected compromised credentials."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report = Check_Report_AWS(self.metadata())
report.region = pool.region
report.resource_id = user_pool_client.id
report.resource_arn = pool.arn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would not be the user_pool_client.arn and user_pool_client.region?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.resource_tags = pool.tags
if user_pool_client.prevent_user_existence_errors == "ENABLED":
report.status = "PASS"
report.status_extended = f"User pool client {user_pool_client.name} has prevent user existence reveal enabled."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool client {user_pool_client.name} has prevent user existence reveal enabled."
report.status_extended = f"User pool client {user_pool_client.name} prevents revealing users in existence errors."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status_extended = f"User pool client {user_pool_client.name} has prevent user existence reveal enabled."
else:
report.status = "FAIL"
report.status_extended = f"User pool client {user_pool_client.name} has prevent user existence reveal disabled."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
report.status_extended = f"User pool client {user_pool_client.name} has prevent user existence reveal disabled."
report.status_extended = f"User pool client {user_pool_client.name} does not prevent revealing users in existence errors."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 11 to 14
report.region = pool.region
report.resource_id = pool_client.id
report.resource_arn = pool.arn
report.resource_tags = pool.tags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would not it be with the pool_client attributes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for all but not for tags since user pool clients don't have tags 😀

Copy link
Member

@sergargar sergargar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Last few changes left :)

pool.advanced_security_mode == "ENFORCED"
and "SIGN_IN"
in pool.risk_configuration.compromised_credentials_risk_configuration.event_filter
and pool.risk_configuration.compromised_credentials_risk_configuration.actions.get(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you store this into the class object instead of doing a get in the map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.resource_arn = pool.arn
report.resource_tags = pool.tags
if pool.password_policy:
if pool.password_policy.get("RequireLowercase", False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you store this into the class object instead of doing a get in the map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

report.status_extended = (
f"User pool {user_pool.id} has self registration disabled."
)
if not user_pool.admin_create_user_config.get(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you store this into the class object instead of doing a get in the map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jfagoagas jfagoagas self-requested a review May 8, 2024 15:24
Copy link
Member

@jfagoagas jfagoagas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@jfagoagas jfagoagas merged commit 225e12b into master May 8, 2024
11 checks passed
@jfagoagas jfagoagas deleted the cognito-checks branch May 8, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/aws Issues/PRs related with the AWS provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants