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
Conversation
report.resource_arn = pool.arn | ||
if ( | ||
pool.advanced_security_mode == "ENFORCED" | ||
and pool.risk_configuration.account_takeover_risk_configuration.get( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Codecov ReportAttention: Patch coverage is
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. |
@@ -0,0 +1,30 @@ | |||
{ | |||
"Provider": "aws", | |||
"CheckID": "cognito_identity_pool_guest_access_enabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"CheckID": "cognito_identity_pool_guest_access_enabled", | |
"CheckID": "cognito_identity_pool_guest_access_disabled", |
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
].describe_user_pool_client( | ||
UserPoolId=user_pool.id, | ||
ClientId=self.regional_clients[ | ||
user_pool.region | ||
].list_user_pool_clients(UserPoolId=user_pool.id)[ | ||
"UserPoolClients" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f"User pool {pool.id} has advanced security audit enabled." | |
f"User pool {pool.id} has advanced security enabled but with audit-only mode." |
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f"User pool {pool.id} has advanced security enforced." | |
f"User pool {pool.id} has advanced security enforced with full-function mode." |
There was a problem hiding this comment.
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": |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"CheckID": "cognito_user_pool_prevent_reveal_user_existence", | |
"CheckID": "cognito_user_pool_client_prevent_user_existence_errors", |
There was a problem hiding this comment.
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)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)}" |
There was a problem hiding this comment.
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 = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
associated_identity_pools = [] | |
associated_identity_pool_authenticated_roles = [] |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"CheckID": "cognito_user_pool_self_registration_enabled", | |
"CheckID": "cognito_user_pool_self_registration_disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"CheckID": "cognito_user_pool_advanced_security_block_sign_in_compromised_credentials", | |
"CheckID": "cognito_user_pool_blocks_compromised_credentials_sign_in_attempts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
"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" | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(pool.risk_configuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Please, remove all |
== "BLOCK" | ||
): | ||
report.status = "PASS" | ||
report.status_extended = f"User pool {pool.name} block sign-in attempts with suspected compromised credentials." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
report.region = pool.region | ||
report.resource_id = pool_client.id | ||
report.resource_arn = pool.arn | ||
report.resource_tags = pool.tags |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😀
There was a problem hiding this 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
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.