Merge pull request #94 from Skyscanner/add-more-docs

Add more documentation to CFRipper rules

CHANGELOG.md

@@ -1,6 +1,12 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
+## [0.12.2] - 2020-01-10
+### Improvements
+- Documentation updated to show the risk of rules and possible fixes where available, 
+as well as a large set of updates to the content. The macros for parsing the documentation
+have also been updated.
+
 ## [0.12.1] - 2020-01-09
 ### Fixes
 - Fix for `CrossAccountCheckingRule` was adding errors when the principal was sts when it shouldn't. 

cfripper/__version__.py

@@ -1,3 +1,3 @@
-VERSION = (0, 12, 1)
+VERSION = (0, 12, 2)
 
 __version__ = ".".join(map(str, VERSION))

cfripper/rules/cloudformation_authentication.py

@@ -19,7 +19,34 @@
 
 
 class CloudFormationAuthenticationRule(Rule):
-    """This rule checks for hardcoded credentials"""
+    """
+    Checks that any `AWS::CloudFormation::Authentication` resource does not contain plain text credentials.
+
+    Risk:
+        Secrets are stored in clear text and printed in clear text in the AWS console.
+
+    Fix:
+        Do not store credentials in CloudFormation files, use parameters.
+
+    Code for fix:
+        ````yml
+        Parameters:
+          PasswordAuth:
+            NoEcho: true
+            Description: Some cool password
+            MinLength: 8
+            Type: String
+
+        ...
+
+        Resources:
+          AWS::CloudFormation::Authentication:
+            ...
+            password:
+              Ref: "PasswordAuth"
+            ...
+        ````
+    """
 
     REASON = "Hardcoded credentials in {}"
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/cross_account_trust.py

@@ -38,8 +38,8 @@
 
 class CrossAccountCheckingRule(PrincipalCheckingRule):
     """
-    Base class not intended to be instantiated, but inherited from
-    This class provides common methods used to detect access permissions from other accounts
+    Base class not intended to be instantiated, but inherited from.
+    This class provides common methods used to detect access permissions from other accounts.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
@@ -90,7 +90,15 @@ def _do_statement_check(self, logical_id, statement):
 
 class CrossAccountTrustRule(CrossAccountCheckingRule):
     """
-    This rule checks for permissions granted to principals from other accounts
+    Checks if the trust policy of a role grants permissions to principals from other accounts.
+    Do not use whole accounts as principals.
+
+    Risk:
+        It might allow other AWS identities to escalate privileges.
+
+    Fix:
+        If cross account permissions are required, the stack should be added to the whitelist for this rule.
+        Otherwise, the access should be removed from the CloudFormation definition.
     """
 
     REASON = "{} has forbidden cross-account trust relationship with {}"
@@ -105,7 +113,14 @@ def invoke(self, cfmodel):
 
 class S3CrossAccountTrustRule(CrossAccountCheckingRule):
     """
-    This rule checks for permissions granted to principals from other accounts in S3 Buckets
+    Check for cross account access in S3 bucket policies. Cross account access by default should not be allowed.
+
+    Risk:
+        It might allow other AWS identities to access/modify content of the bucket.
+
+    Fix:
+        If cross account permissions are required for S3 access, the stack should be added to the whitelist for this rule.
+        Otherwise, the access should be removed from the CloudFormation definition.
     """
 
     REASON = "{} has forbidden cross-account policy allow with {} for an S3 bucket."
@@ -119,7 +134,14 @@ def invoke(self, cfmodel):
 
 class KMSKeyCrossAccountTrustRule(CrossAccountCheckingRule):
     """
-    This rule checks for KMS keys that allow cross-account principals to get access to the key
+    Checks for KMS keys that allow cross-account principals to get access to the key.
+
+    Risk:
+        It might allow other AWS identities to read/modify the secrets.
+
+    Fix:
+        If cross account permissions are required for KMS access, the stack should be added to the whitelist for this rule.
+        Otherwise, the access should be removed from the CloudFormation definition.
     """
 
     REASON = "{} has forbidden cross-account policy allow with {} for an KMS Key Policy"

cfripper/rules/ebs_volume_has_sse.py

@@ -19,7 +19,27 @@
 
 class EBSVolumeHasSSERule(Rule):
     """
-    Check that server side encryption is enabled for all EBS volumes.
+    Checks that server side encryption is enabled for all EBS volumes.
+
+    Risk:
+        Data that is not encrypted at rest could breach regulatory compliance
+        and allow easier access for an attacker to view any instace storage data
+        of your EC2 instance.
+
+    Fix:
+        Enable server-side encryption on EBS volumes.
+
+    Code for fix:
+        ````json
+        {
+            "Type" : "AWS::EC2::Volume",
+            "Properties" : {
+                ...
+                "Encrypted" : true,
+                ...
+            }
+        }
+        ````
     """
 
     REASON = "EBS volume {} should have server-side encryption enabled"

cfripper/rules/hardcoded_RDS_password.py

@@ -21,7 +21,39 @@
 
 class HardcodedRDSPasswordRule(Rule):
     """
-    This rule checks that RDS clusters and instances don't expose their passwords
+    Checks that any RDS clusters or instances aren't exposing their passwords.
+    The rule forbids default password parameters and any missing `NoEcho` for RDS passwords.
+
+    Risk:
+        Not setting this correctly can lead to malicious agents attempting to gain access to your
+        RDS instaces with a default password, or by reading the value that will be printed in plain
+        text in the AWS console and logs if `NoEcho` is not set.
+
+    Fix:
+        When defining a password **do not use** the default value.
+        If you specify a default password and you don’t provide a parameter, it will use the default
+        which can be found clear text in the CloudFormation file.
+
+    Code for fix:
+        ````yml
+        Parameters:
+          MasterUserPassword:
+            NoEcho: true
+            Description: The database admin account password
+            MinLength: 8
+            Type: String
+
+        ...
+
+        Resources:
+          RDSCluster:
+            Type: AWS::RDS::DBCluster
+            DeletionPolicy: "Snapshot"
+            Properties:
+              ...
+              MasterUserPassword: !Ref 'MasterUserPassword'
+              ...
+        ````
     """
 
     REASON_DEFAULT = "Default RDS {} password parameter (readable in plain-text) for {}."

cfripper/rules/iam_managed_policy_wildcard_action.py

@@ -21,8 +21,9 @@
 
 
 class IAMManagedPolicyWildcardActionRule(Rule):
+    #  TODO: update this description below and make it 100% accurate.
     """
-    This rule checks for wildcards in IAM Managed policies
+    Rule currently not in use.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/iam_roles.py

@@ -26,7 +26,7 @@
 
 class IAMRolesOverprivilegedRule(Rule):
     """
-    Rule that checks for wildcards in resources for a set of actions and restricts managed policies
+    Rule that checks for wildcards in resources for a set of actions and restricts managed policies.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
@@ -71,11 +71,15 @@ def check_inline_policies(self, logical_id, role):
 
 class IAMRoleWildcardActionOnPermissionsPolicyRule(Rule):
     """
-    Rule that checks for wildcards in actions in IAM role policies
+    Checks if any IAM role is using a `*` character in any of its permissions policy.
+
+    Risk:
+        The principle of least privilege (POLP), an important concept in computer security, is the practice of
+        limiting access rights for users to the bare minimum permissions they need to perform their work.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "IAM role {} should not allow * action on its permissions policy {}"
+    REASON = "IAM role {} should not allow any * action on its permissions policy {}"
 
     def invoke(self, cfmodel):
         for logical_id, resource in cfmodel.Resources.items():
@@ -91,11 +95,15 @@ def invoke(self, cfmodel):
 
 class IAMRoleWildcardActionOnTrustPolicyRule(Rule):
     """
-    Rule that checks for wildcards in actions in IAM role assume role policy documents
+    Checks if any IAM role is using a `*` character in its trust policy.
+
+    Risk:
+        Do not allow any principal to assume your role.
+        This is a security risk as it allow a third party account to assume your role and escalate privileges in our account.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
-    REASON = "IAM role {} should not allow * action on its trust policy"
+    REASON = "IAM role {} should not allow any * action on its trust policy"
 
     def invoke(self, cfmodel):
         for logical_id, resource in cfmodel.Resources.items():

cfripper/rules/kms_key_wildcard_principal.py

@@ -27,7 +27,7 @@
 
 class KMSKeyWildcardPrincipal(Rule):
     """
-    This rule checks for KMS keys that contain wildcards in the key policies
+    Check for wildcards in principals in KMS Policies.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/managed_policy_on_user.py

@@ -22,7 +22,7 @@
 
 class ManagedPolicyOnUserRule(Rule):
     """
-    Rule that checks for IAM managed policies attached directly to users
+    Checks if any IAM managed policy is applied to a group and not a user.
     """
 
     REASON = "IAM managed policy {} should not apply directly to users. Should be on group"

cfripper/rules/policy_on_user.py

@@ -22,7 +22,7 @@
 
 class PolicyOnUserRule(Rule):
     """
-    Rule that checks for IAM policies attached directly to users
+    Checks if any IAM policy is applied to a group and not a user.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/privilege_escalation.py

@@ -21,7 +21,13 @@
 
 class PrivilegeEscalationRule(Rule):
     """
-    Rule that checks for actions that allow privilege escalation in IAM policies
+    Checks for any dangerous IAM actions that could allow privilege escalation and potentially
+    represent a large security risk.
+    See [current blacklisted IAM actions](https://github.com/Skyscanner/cfripper/blob/master/cfripper/rules/privilege_escalation.py#L29).
+
+    Fix:
+        Unless strictly necessary, do not use actions in the IAM action blacklist. CloudFormation files that do require these
+        actions should be added to the whitelist.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/s3_bucket_policy.py

@@ -28,8 +28,14 @@
 
 class S3BucketPolicyPrincipalRule(PrincipalCheckingRule):
     """
-    Rule that checks for non-whitelisted principals in S3 bucket policies.
-    This is designed to block unintended access from third party accounts to your buckets
+    Checks for non-whitelisted principals in S3 bucket policies.
+
+    Risk:
+        This is designed to block unintended access from third party accounts to your buckets.
+
+    Fix:
+        All principals connected to S3 Bucket Policies should be known. CFRipper checks that **all** principals meet
+        the requirements expected. The list of valid accounts is defined in `valid_principals`, which is set in the config.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
@@ -61,7 +67,7 @@ def invoke(self, cfmodel):
 
 class S3BucketPolicyWildcardActionRule(Rule):
     """
-    Rule that checks for wildcard actions in S3 bucket policies
+    Checks if any S3 bucket policies contain wildcard actions.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE

cfripper/rules/s3_public_access.py

@@ -26,8 +26,13 @@
 
 
 class S3BucketPublicReadAclAndListStatementRule(Rule):
+    # TODO: refactor regex to regex file.
     """
-    Rule that checks for public read access to S3 bucket policies
+    Checks if any S3 bucket policy has a public read ACL and `List` permission in the bucket policy.
+
+    Fix:
+        Unless the bucket is hosting static content and needs to be accessed publicly,
+        these bucket policies should be locked down.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE
@@ -49,7 +54,7 @@ def invoke(self, cfmodel):
 
 class S3BucketPublicReadWriteAclRule(Rule):
     """
-    Rule that checks for public read access to S3 buckets
+    Checks if any S3 bucket policy has access control set to `PublicReadWrite`.
     """
 
     GRANULARITY = RuleGranularity.RESOURCE