Add CLI args for aws account id and aws principals (#178)

* Add command line options for aws_account_id and aws_principals

* Update AWS Account IDs in a test to be 12-digit numbers

* Update cli.md docs with aws-account-id and aws-principals args

cfripper/cli.py

@@ -1,8 +1,9 @@
 import logging
+import re
 import sys
 from io import TextIOWrapper
 from pathlib import Path
-from typing import Dict, Optional, Tuple
+from typing import Dict, List, Optional, Tuple
 
 import click
 import pycfmodel
@@ -30,10 +31,13 @@ def setup_logging(level: str) -> None:
 
 
 def init_cfripper(
-    rules_config_file: Optional[TextIOWrapper], rules_filters_folder: Optional[str]
+    rules_config_file: Optional[TextIOWrapper],
+    rules_filters_folder: Optional[str],
+    aws_account_id: Optional[str],
+    aws_principals: Optional[List[str]],
 ) -> Tuple[Config, RuleProcessor]:
     rules = get_all_rules()
-    config = Config(rules=rules.keys())
+    config = Config(rules=rules.keys(), aws_account_id=aws_account_id, aws_principals=aws_principals)
     if rules_config_file:
         config.load_rules_config_file(rules_config_file)
     if rules_filters_folder:
@@ -105,14 +109,16 @@ def process_template(
     output_format: str,
     rules_config_file: Optional[TextIOWrapper],
     rules_filters_folder: Optional[str],
+    aws_account_id: Optional[str],
+    aws_principals: Optional[List[str]],
 ) -> bool:
     logging.info(f"Analysing {template.name}...")
 
     cfmodel = get_cfmodel(template)
     if resolve:
         cfmodel = cfmodel.resolve(resolve_parameters)
 
-    config, rule_processor = init_cfripper(rules_config_file, rules_filters_folder)
+    config, rule_processor = init_cfripper(rules_config_file, rules_filters_folder, aws_account_id, aws_principals)
 
     result = analyse_template(cfmodel, rule_processor, config)
 
@@ -123,6 +129,21 @@ def process_template(
     return result.valid
 
 
+def validate_aws_account_id(ctx: click.Context, param: str, value: str) -> Optional[str]:
+    if value in [None, ""]:
+        return None
+    if re.match("^[0-9]{12}$", str(value)):
+        return str(value)
+    else:
+        raise click.BadParameter("AWS Account ID needs to be 12 digits – are you missing 0 prefixes?")
+
+
+def validate_aws_principals(ctx: click.Context, param: str, value: str) -> Optional[List[str]]:
+    if value in [None, ""]:
+        return None
+    return str(value).split(",")
+
+
 @click.command()
 @click.version_option(prog_name="cfripper", version=__version__)
 @click.argument("templates", type=click.File("r"), nargs=-1)
@@ -170,6 +191,19 @@ def process_template(
     type=click.Path(exists=True, resolve_path=True, readable=True, file_okay=False),
     help="All files in the folder must be of type: [.py, .pyc]",
 )
+@click.option(
+    "--aws-account-id",
+    type=click.STRING,
+    callback=validate_aws_account_id,
+    help="A 12-digit AWS account number eg. 123456789012",
+)
+@click.option(
+    "--aws-principals",
+    type=click.STRING,
+    callback=validate_aws_principals,
+    help="A comma separated list of AWS principals eg. arn:aws:iam::123456789012:root,234567890123,"
+    "arn:aws:iam::111222333444:user/user-name",
+)
 def cli(templates, logging_level, resolve_parameters, **kwargs):
     """
     Analyse AWS Cloudformation templates passed by parameter.

docs/cli.md

@@ -48,4 +48,65 @@ Result saved in /tmp/root.yaml.cfripper.results.json
 Analysing /tmp/root_bypass.json...
 Not adding CrossAccountTrustRule failure in rootRole because no AWS Account ID was found in the config.
 Result saved in /tmp/root_bypass.json.cfripper.results.json
+```
+
+### Using the "aws-account-id" and "aws-principals" arguments
+
+- `--aws-account-id` is used to specify the AWS Account you want to check the template against
+- `--aws-principals` is used to specify the expected/allowed principals to ignore when checking the template 
+
+See how the output is reduced as each option is added:
+
+Without either argument, 13 issues:
+```bash
+$ cfripper ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json --format txt --resolve
+Analysing ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json...
+Using `UNDEFINED_PARAM_S3Bucket` for S3Bucket. Original value wasn't available.
+Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess because no AWS Account ID was found in the config.
+Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess because no AWS Account ID was found in the config.
+Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess because no AWS Account ID was found in the config.
+Not adding S3CrossAccountTrustRule failure in S3BucketPolicyAccountAccess because no AWS Account ID was found in the config.
+Valid: False
+Issues found:
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 123456789012
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
+	- S3BucketPublicReadAclAndListStatementRule: S3 Bucket S3BucketPolicyAccountAccess should not have a public read acl and list bucket statement
+```
+
+With `--aws-account-id` argument, 6 issues:
+```bash
+$ cfripper ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json --format txt --aws-account-id 123456789012 --resolve
+Analysing ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json...
+Using `UNDEFINED_PARAM_S3Bucket` for S3Bucket. Original value wasn't available.
+Valid: False
+Issues found:
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
+	- S3BucketPolicyPrincipalRule: S3 Bucket S3BucketPolicyAccountAccess policy has non-allowed principals 666555444333
+	- S3BucketPublicReadAclAndListStatementRule: S3 Bucket S3BucketPolicyAccountAccess should not have a public read acl and list bucket statement
+	- S3CrossAccountTrustRule: S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444333:root for an S3 bucket.
+```
+
+With both arguments, 4 issues:
+```bash
+$ cfripper ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json --format txt --aws-account-id 123456789012 --aws-principals 666555444333 --resolve
+Analysing ./tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json...
+Using `UNDEFINED_PARAM_S3Bucket` for S3Bucket. Original value wasn't available.
+Valid: False
+Issues found:
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::123456789012:role/some-role/some-other-sub-role')
+	- PartialWildcardPrincipalRule: S3BucketPolicyAccountAccess should not allow wildcard in principals or account-wide principals (principal: 'arn:aws:iam::666555444333:root')
+	- S3BucketPublicReadAclAndListStatementRule: S3 Bucket S3BucketPolicyAccountAccess should not have a public read acl and list bucket statement
+	- S3CrossAccountTrustRule: S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444333:root for an S3 bucket.
 ```

tests/config/test_config.py

@@ -17,6 +17,8 @@ def template_two_roles_dict():
 def test_init_with_no_params():
     config = Config()
     assert config.rules is None
+    assert config.aws_account_id is None
+    assert config.aws_principals == []
 
 
 def test_init_with_nonexistent_params():
@@ -26,6 +28,16 @@ def test_init_with_nonexistent_params():
     assert set(config.rules) == set(default_rules)
 
 
+def test_init_with_existent_params():
+    expected_aws_account_id = "123456789012"
+    expected_aws_principals = ["234567890123", "345678901234"]
+
+    config = Config(aws_account_id=expected_aws_account_id, aws_principals=expected_aws_principals)
+
+    assert config.aws_account_id == expected_aws_account_id
+    assert config.aws_principals == expected_aws_principals
+
+
 def test_load_rules_config_file_success(test_files_location):
     mock_rules = ["RuleThatUsesResourceAllowlist", "SecurityGroupOpenToWorldRule"]
     config = Config(stack_name="test_stack", rules=mock_rules)

tests/rules/test_S3CrossAccountTrustRule.py

@@ -44,7 +44,7 @@ def test_s3_bucket_cross_account(s3_bucket_cross_account):
 
 
 def test_s3_bucket_cross_account_and_normal(s3_bucket_cross_account_and_normal):
-    rule = S3CrossAccountTrustRule(Config(aws_account_id="123456789"))
+    rule = S3CrossAccountTrustRule(Config(aws_account_id="123456789012"))
     result = rule.invoke(s3_bucket_cross_account_and_normal)
 
     assert not result.valid
@@ -53,7 +53,7 @@ def test_s3_bucket_cross_account_and_normal(s3_bucket_cross_account_and_normal):
         [
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444:root for an S3 bucket.",
+                reason="S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444333:root for an S3 bucket.",
                 risk_value=RuleRisk.MEDIUM,
                 rule="S3CrossAccountTrustRule",
                 rule_mode=RuleMode.BLOCKING,
@@ -65,7 +65,7 @@ def test_s3_bucket_cross_account_and_normal(s3_bucket_cross_account_and_normal):
 
 
 def test_s3_bucket_cross_account_and_normal_with_org_aws_account(s3_bucket_cross_account_and_normal):
-    rule = S3CrossAccountTrustRule(Config(aws_account_id="123456789", aws_principals=["666555444"]))
+    rule = S3CrossAccountTrustRule(Config(aws_account_id="123456789012", aws_principals=["666555444333"]))
     result = rule.invoke(s3_bucket_cross_account_and_normal)
 
     assert not result.valid
@@ -74,7 +74,7 @@ def test_s3_bucket_cross_account_and_normal_with_org_aws_account(s3_bucket_cross
         [
             Failure(
                 granularity=RuleGranularity.RESOURCE,
-                reason="S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444:root for an S3 bucket.",
+                reason="S3BucketPolicyAccountAccess has forbidden cross-account policy allow with arn:aws:iam::666555444333:root for an S3 bucket.",
                 risk_value=RuleRisk.MEDIUM,
                 rule="S3CrossAccountTrustRule",
                 rule_mode=RuleMode.BLOCKING,

tests/test_cli.py

@@ -0,0 +1,76 @@
+from unittest.mock import MagicMock, patch
+
+import click
+import pytest
+from click.testing import CliRunner
+
+import cfripper.cli as undertest
+from tests.utils import FIXTURE_ROOT_PATH
+
+
+@pytest.mark.parametrize(
+    "aws_account_id_arg, validation_result", [(None, None), ("", None), ("123456789012", "123456789012")],
+)
+def test_validate_aws_account_id(
+    aws_account_id_arg, validation_result,
+):
+    fake_command = click.Command("fake_command")
+    fake_context = click.Context(fake_command)
+    fake_param = "fake_param"
+    assert undertest.validate_aws_account_id(fake_context, fake_param, aws_account_id_arg) == validation_result
+
+
+def test_validate_aws_account_id_with_malformed_arg():
+    fake_command = click.Command("fake_command")
+    fake_context = click.Context(fake_command)
+    fake_param = "fake_param"
+
+    with pytest.raises(click.BadParameter):
+        undertest.validate_aws_account_id(fake_context, fake_param, "malformed aws account id")
+
+
+@pytest.mark.parametrize(
+    "aws_principals_arg, validation_result",
+    [
+        (None, None),
+        ("", None),
+        ("123456789012", ["123456789012"]),
+        (
+            "arn:aws:iam::123456789012:root,234567890123,arn:aws:iam::111222333444:user/user-name",
+            ["arn:aws:iam::123456789012:root", "234567890123", "arn:aws:iam::111222333444:user/user-name"],
+        ),
+    ],
+)
+def test_validate_aws_principals(
+    aws_principals_arg, validation_result,
+):
+    fake_command = click.Command("fake_command")
+    fake_context = click.Context(fake_command)
+    fake_param = "fake_param"
+    assert undertest.validate_aws_principals(fake_context, fake_param, aws_principals_arg) == validation_result
+
+
+@patch("cfripper.cli.process_template")
+def test_aws_account_id_cli_option(patched_process_template: MagicMock):
+    patched_process_template.return_value = True
+    test_template_path = str(FIXTURE_ROOT_PATH) + "/others/iam_role.json"
+    fake_aws_account_id = "123456789012"
+
+    runner = CliRunner()
+    result = runner.invoke(undertest.cli, ["--aws-account-id", fake_aws_account_id, test_template_path])
+    assert patched_process_template.call_count == 1
+    assert patched_process_template.call_args[1]["aws_account_id"] == fake_aws_account_id
+    assert result.exit_code == 0
+
+
+@patch("cfripper.cli.process_template")
+def test_aws_principles_cli_option(patched_process_template: MagicMock):
+    patched_process_template.return_value = True
+    test_template_path = str(FIXTURE_ROOT_PATH) + "/others/iam_role.json"
+    fake_aws_principals = ["123456789012", "234567890123"]
+
+    runner = CliRunner()
+    result = runner.invoke(undertest.cli, ["--aws-principals", ",".join(fake_aws_principals), test_template_path])
+    assert patched_process_template.call_count == 1
+    assert patched_process_template.call_args[1]["aws_principals"] == fake_aws_principals
+    assert result.exit_code == 0

tests/test_templates/rules/S3CrossAccountTrustRule/s3_bucket_cross_account_and_normal.json

@@ -83,7 +83,7 @@
             {
               "Effect": "Allow",
               "Principal": {
-                "AWS": "arn:aws:iam::123456789:role/some-role/some-sub-role"
+                "AWS": "arn:aws:iam::123456789012:role/some-role/some-sub-role"
               },
               "Action": "s3:DeleteObjectVersion",
               "Resource": "arn:aws:s3:::company-prod-a-bucket-of-some-sort/*"
@@ -92,10 +92,10 @@
               "Effect": "Deny",
               "NotPrincipal": {
                 "AWS": [
-                  "arn:aws:sts::123456789:assumed-role/employee/employee-a",
-                  "arn:aws:iam::123456789:root",
-                  "arn:aws:sts::123456789:assumed-role/employee/employee-b",
-                  "arn:aws:iam::123456789:role/some-role/some-sub-role"
+                  "arn:aws:sts::123456789012:assumed-role/employee/employee-a",
+                  "arn:aws:iam::123456789012:root",
+                  "arn:aws:sts::123456789012:assumed-role/employee/employee-b",
+                  "arn:aws:iam::123456789012:role/some-role/some-sub-role"
                 ]
               },
               "Action": "s3:DeleteObjectVersion",
@@ -104,7 +104,7 @@
             {
               "Effect": "Allow",
               "Principal": {
-                "AWS": "arn:aws:iam::123456789:role/some-role/some-sub-role"
+                "AWS": "arn:aws:iam::123456789012:role/some-role/some-sub-role"
               },
               "Action": [
                 "s3:DeleteObject",
@@ -116,8 +116,8 @@
               "Effect": "Allow",
               "Principal": {
                 "AWS": [
-                  "arn:aws:iam::123456789:role/some-role/some-other-sub-role",
-                  "arn:aws:iam::666555444:root"
+                  "arn:aws:iam::123456789012:role/some-role/some-other-sub-role",
+                  "arn:aws:iam::666555444333:root"
                 ]
               },
               "Action": [
@@ -133,8 +133,8 @@
               "Effect": "Deny",
               "Principal": {
                 "AWS": [
-                  "arn:aws:iam::123456789:role/some-role/some-other-sub-role",
-                  "arn:aws:iam::666555444:root"
+                  "arn:aws:iam::123456789012:role/some-role/some-other-sub-role",
+                  "arn:aws:iam::666555444333:root"
                 ]
               },
               "Action": [