diff --git a/README.md b/README.md index b1ca7e9..7e709b6 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ to `s3:*` to make an error go away, SSH opened "temporarily". The diff reads fine. The plan says what will actually happen. `plan-skeptic` reads the plan and points at the lines a reviewer must not skim. -It is not a policy engine and does not try to replace one. It is ten +It is not a policy engine and does not try to replace one. It is eleven opinionated checks about **what a change introduces**, tuned so that the output is short enough to be read on every pull request. @@ -41,7 +41,7 @@ PYTHONPATH=src python3 -m plan_skeptic plan.json - run: | terraform plan -out=plan.out terraform show -json plan.out > plan.json -- uses: TellersTechOrg/plan-skeptic@v0.1.0 +- uses: TellersTechOrg/plan-skeptic@v0.1.1 with: plan-json: plan.json fail-on: high # high | medium | low | never @@ -51,8 +51,8 @@ PYTHONPATH=src python3 -m plan_skeptic plan.json sarif_file: plan-skeptic.sarif ``` -Findings appear in the Security tab and inline on the pull request. Uploading -SARIF needs `security-events: write`. +Findings appear in the job summary, the Security tab (when SARIF is uploaded), +and inline on the pull request. Uploading SARIF needs `security-events: write`. ## Rules @@ -60,7 +60,7 @@ SARIF needs `security-events: write`. |---|---|---| | PS001 | high | A data-holding resource (database, bucket, table, volume, key, PVC) is destroyed or replaced, and why it is being replaced | | PS002 | medium | Any other resource is deleted | -| PS003 | high | An IAM policy newly grants `*` / `service:*`, or uses `NotAction` in an Allow | +| PS003 | high | An IAM policy (including role `inline_policy`) newly grants `*` / `service:*`, or uses `NotAction` in an Allow | | PS004 | high | AdministratorAccess, PowerUserAccess or IAMFullAccess is attached | | PS005 | high | A trust policy lets any principal assume the role without a Condition | | PS006 | high | Ingress opened to 0.0.0.0/0 or ::/0 (medium for ports 80 and 443) | @@ -68,6 +68,7 @@ SARIF needs `security-events: write`. | PS008 | high | A database given `publicly_accessible = true` | | PS009 | medium | Encryption at rest set to false | | PS010 | medium | `deletion_protection` switched off, or `skip_final_snapshot` / `force_destroy` switched on | +| PS011 | high | A KMS key policy lets any principal use the key without a Condition | A delete paired with a create of the same type and name gets a hint to use a `moved` block, since that is what an unfinished refactor looks like. @@ -96,7 +97,7 @@ refused with exit 2 rather than reported clean. ## Fixtures -[`fixtures/`](fixtures/) holds seven flawed plans, each paired with the request +[`fixtures/`](fixtures/) holds eight flawed plans, each paired with the request that produced it, plus a clean control. They are the exercises for the [Confidently Wrong workshop](https://www.tellerstech.com/workshops/ai-era-infrastructure-risk-workshop/) and come from the same material as the book diff --git a/action.yml b/action.yml index 62cb22f..8b4b0fd 100644 --- a/action.yml +++ b/action.yml @@ -35,6 +35,13 @@ runs: PS_DISABLE: ${{ inputs.disable }} PYTHONPATH: ${{ github.action_path }}/src run: | + set -euo pipefail args=(--format sarif --output "$PS_SARIF" --fail-on "$PS_FAIL_ON") for rule in $PS_DISABLE; do args+=(--disable "$rule"); done - python3 -m plan_skeptic "${args[@]}" "$PS_PLAN" + # Text summary always goes to stdout when SARIF is written to a file; + # tee it into the job summary so findings are visible without code scanning. + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + python3 -m plan_skeptic "${args[@]}" "$PS_PLAN" | tee -a "$GITHUB_STEP_SUMMARY" + else + python3 -m plan_skeptic "${args[@]}" "$PS_PLAN" + fi diff --git a/fixtures/README.md b/fixtures/README.md index 7e580e3..26b0067 100644 --- a/fixtures/README.md +++ b/fixtures/README.md @@ -20,6 +20,7 @@ the same shape, trimmed to the attributes the rules read. | `05-public-bucket-for-static-site` | "Serve these assets as a static site" | Public ACL, public bucket policy, and the public access block relaxed, where CloudFront with OAC needed none of it | PS007 | | `06-admin-to-unblock-ci` | "CI fails with AccessDenied on deploy" | AdministratorAccess on the CI role, and a trust statement that lets any AWS account assume it | PS004 PS005 | | `07-refactor-without-moved-block` | "Move the orders resources into a module" | Destroys the table and log group and creates new ones at the module address, because nobody wrote a `moved` block | PS001 PS002 | +| `08-kms-open-to-unblock-encrypt` | "The app gets AccessDeniedException on Decrypt, fix the key policy" | Adds a statement granting `kms:*` to Principal `*` with no Condition | PS011 | `clean/01-tags-only` is the control: a tags-only change on resources that already carry a wildcard policy and a public 443 rule. It must produce nothing, diff --git a/fixtures/flawed/08-kms-open-to-unblock-encrypt.json b/fixtures/flawed/08-kms-open-to-unblock-encrypt.json new file mode 100644 index 0000000..21b7100 --- /dev/null +++ b/fixtures/flawed/08-kms-open-to-unblock-encrypt.json @@ -0,0 +1,25 @@ +{ + "format_version": "1.2", + "terraform_version": "1.12.5", + "resource_changes": [ + { + "address": "aws_kms_key.app", + "mode": "managed", + "type": "aws_kms_key", + "name": "app", + "provider_name": "registry.opentofu.org/hashicorp/aws", + "change": { + "actions": ["update"], + "before": { + "description": "app data key", + "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"Root\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::111122223333:root\"},\"Action\":\"kms:*\",\"Resource\":\"*\"},{\"Sid\":\"App\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::111122223333:role/app\"},\"Action\":[\"kms:Decrypt\",\"kms:GenerateDataKey\"],\"Resource\":\"*\"}]}" + }, + "after": { + "description": "app data key", + "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"Root\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::111122223333:root\"},\"Action\":\"kms:*\",\"Resource\":\"*\"},{\"Sid\":\"App\",\"Effect\":\"Allow\",\"Principal\":{\"AWS\":\"arn:aws:iam::111122223333:role/app\"},\"Action\":[\"kms:Decrypt\",\"kms:GenerateDataKey\"],\"Resource\":\"*\"},{\"Sid\":\"Anyone\",\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":\"kms:*\",\"Resource\":\"*\"}]}" + }, + "after_unknown": {} + } + } + ] +} diff --git a/pyproject.toml b/pyproject.toml index 90dcc36..89aa15d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "plan-skeptic" -version = "0.1.0" +version = "0.1.1" description = "Flag the parts of a Terraform/OpenTofu plan that need a human." readme = "README.md" requires-python = ">=3.9" diff --git a/src/plan_skeptic/__init__.py b/src/plan_skeptic/__init__.py index c49e831..69c388e 100644 --- a/src/plan_skeptic/__init__.py +++ b/src/plan_skeptic/__init__.py @@ -1,3 +1,3 @@ """plan-skeptic: flag the parts of a Terraform/OpenTofu plan that need a human.""" -__version__ = "0.1.0" +__version__ = "0.1.1" diff --git a/src/plan_skeptic/report.py b/src/plan_skeptic/report.py index 0765e06..fcdd776 100644 --- a/src/plan_skeptic/report.py +++ b/src/plan_skeptic/report.py @@ -77,7 +77,9 @@ def render_sarif(findings: list, plan_uri: str) -> str: "logicalLocations": [{"fullyQualifiedName": f.address, "kind": "resource"}], } ], - "partialFingerprints": {"resourceRule": f"{f.rule_id}:{f.address}:{f.message}"}, + "partialFingerprints": { + "resourceRule": f"{f.rule_id}:{f.address}" + (f":{f.key}" if f.key else ""), + }, } for f in findings ] diff --git a/src/plan_skeptic/rules.py b/src/plan_skeptic/rules.py index cbde31f..9983c86 100644 --- a/src/plan_skeptic/rules.py +++ b/src/plan_skeptic/rules.py @@ -40,6 +40,10 @@ class Finding: severity: str address: str message: str + # Distinguishes multiple findings of the same rule on one address in SARIF + # fingerprints. Must not be the free-text message: wording changes would + # reopen alerts as new. + key: str = "" RULES = { @@ -110,6 +114,12 @@ class Finding: "the next mistake recoverable. Turning one off is often the first step " "of a plan that deletes something in a later apply.", ), + Rule( + "PS011", "kms-key-policy-any-principal", HIGH, + "A KMS key policy allows any principal without a Condition.", + "A Principal of `*` with no Condition lets any AWS account use the key. " + "Generated policies open this to make encrypt/decrypt errors go away.", + ), ) } @@ -132,6 +142,8 @@ class Finding: IAM_POLICY_TYPES = frozenset({ "aws_iam_policy", "aws_iam_role_policy", "aws_iam_user_policy", "aws_iam_group_policy", }) +# aws_iam_role carries nested inline_policy blocks; same wildcard rules apply. +IAM_WILDCARD_TYPES = IAM_POLICY_TYPES | {"aws_iam_role"} POLICY_ATTACHMENT_TYPES = frozenset({ "aws_iam_role_policy_attachment", "aws_iam_user_policy_attachment", "aws_iam_group_policy_attachment", "aws_iam_policy_attachment", @@ -175,8 +187,10 @@ def _new_items(rc: ResourceChange, extract: Callable[[dict], set]) -> list: return sorted(after - before, key=str) -def _finding(rule_id: str, rc: ResourceChange, message: str, severity: str = "") -> Finding: - return Finding(rule_id, severity or RULES[rule_id].severity, rc.address, message) +def _finding( + rule_id: str, rc: ResourceChange, message: str, severity: str = "", key: str = "", +) -> Finding: + return Finding(rule_id, severity or RULES[rule_id].severity, rc.address, message, key) def _as_list(value) -> list: @@ -208,31 +222,46 @@ def _principal_is_anyone(principal) -> bool: return False +def _policy_allows_anyone(doc) -> bool: + return any( + _allows(s) and _principal_is_anyone(s.get("Principal")) and not s.get("Condition") + for s in _policy_statements(doc) + ) + + +def _policy_docs(values: dict) -> list: + """Policy documents on a resource: top-level `policy` plus role inline_policy blocks.""" + docs = [values.get("policy")] + for block in _as_list(values.get("inline_policy")): + if isinstance(block, dict): + docs.append(block.get("policy")) + return docs + + def _wildcard_grants(values: dict) -> set: grants = set() - for stmt in _policy_statements(values.get("policy")): - if not _allows(stmt): - continue - for action in _as_list(stmt.get("Action")): - if isinstance(action, str) and (action == "*" or action.endswith(":*")): - grants.add(f"Action {action}") - if stmt.get("NotAction") is not None: - grants.add("NotAction in an Allow statement") + for doc in _policy_docs(values): + for stmt in _policy_statements(doc): + if not _allows(stmt): + continue + for action in _as_list(stmt.get("Action")): + if isinstance(action, str) and (action == "*" or action.endswith(":*")): + grants.add(f"Action {action}") + if stmt.get("NotAction") is not None: + grants.add("NotAction in an Allow statement") return grants def _anyone_can_assume(values: dict) -> bool: - return any( - _allows(s) and _principal_is_anyone(s.get("Principal")) and not s.get("Condition") - for s in _policy_statements(values.get("assume_role_policy")) - ) + return _policy_allows_anyone(values.get("assume_role_policy")) def _bucket_policy_public(values: dict) -> bool: - return any( - _allows(s) and _principal_is_anyone(s.get("Principal")) and not s.get("Condition") - for s in _policy_statements(values.get("policy")) - ) + return _policy_allows_anyone(values.get("policy")) + + +def _kms_policy_anyone(values: dict) -> bool: + return _policy_allows_anyone(values.get("policy")) def _port_label(from_port, to_port) -> str: @@ -290,10 +319,10 @@ def check_deleted(rc: ResourceChange) -> Iterable[Finding]: def check_iam_wildcard(rc: ResourceChange) -> Iterable[Finding]: - if rc.type not in IAM_POLICY_TYPES: + if rc.type not in IAM_WILDCARD_TYPES: return for grant in _new_items(rc, _wildcard_grants): - yield _finding("PS003", rc, f"Policy now grants {grant}.") + yield _finding("PS003", rc, f"Policy now grants {grant}.", key=grant) def check_broad_attachment(rc: ResourceChange) -> Iterable[Finding]: @@ -309,7 +338,7 @@ def broad(values: dict) -> set: } for name in _new_items(rc, broad): - yield _finding("PS004", rc, f"Attaches the AWS managed policy {name}.") + yield _finding("PS004", rc, f"Attaches the AWS managed policy {name}.", key=name) def check_trust_anyone(rc: ResourceChange) -> Iterable[Finding]: @@ -320,7 +349,11 @@ def check_trust_anyone(rc: ResourceChange) -> Iterable[Finding]: def check_open_ingress(rc: ResourceChange) -> Iterable[Finding]: for from_port, to_port, cidr in _new_items(rc, lambda v: _world_ingress(rc.type, v)): severity = MEDIUM if _is_web_only(from_port, to_port) else HIGH - yield _finding("PS006", rc, f"Opens {_port_label(from_port, to_port)} to {cidr}.", severity) + label = _port_label(from_port, to_port) + yield _finding( + "PS006", rc, f"Opens {label} to {cidr}.", severity, + key=f"{from_port}-{to_port}-{cidr}", + ) def check_s3_public(rc: ResourceChange) -> Iterable[Finding]: @@ -328,13 +361,16 @@ def check_s3_public(rc: ResourceChange) -> Iterable[Finding]: flags = ("block_public_acls", "block_public_policy", "ignore_public_acls", "restrict_public_buckets") off = _new_items(rc, lambda v: {f for f in flags if v.get(f) is False}) if off: - yield _finding("PS007", rc, "Public access block disables " + ", ".join(off) + ".") + yield _finding( + "PS007", rc, "Public access block disables " + ", ".join(off) + ".", + key=",".join(off), + ) elif rc.type in ("aws_s3_bucket_acl", "aws_s3_bucket"): if _introduced(rc, lambda v: v.get("acl") in PUBLIC_ACLS): - yield _finding("PS007", rc, f"Bucket ACL set to {rc.after.get('acl')}.") + yield _finding("PS007", rc, f"Bucket ACL set to {rc.after.get('acl')}.", key="acl") elif rc.type == "aws_s3_bucket_policy": if _introduced(rc, _bucket_policy_public): - yield _finding("PS007", rc, "Bucket policy allows Principal * with no Condition.") + yield _finding("PS007", rc, "Bucket policy allows Principal * with no Condition.", key="policy") def check_public_database(rc: ResourceChange) -> Iterable[Finding]: @@ -345,7 +381,7 @@ def check_public_database(rc: ResourceChange) -> Iterable[Finding]: def check_encryption_disabled(rc: ResourceChange) -> Iterable[Finding]: attr = ENCRYPTION_FLAGS.get(rc.type) if attr and _introduced(rc, lambda v: v.get(attr) is False): - yield _finding("PS009", rc, f"{attr} = false.") + yield _finding("PS009", rc, f"{attr} = false.", key=attr) def check_recovery_guards(rc: ResourceChange) -> Iterable[Finding]: @@ -358,9 +394,20 @@ def check_recovery_guards(rc: ResourceChange) -> Iterable[Finding]: # Defaults on a new resource are not a change a reviewer can weigh, # except skip_final_snapshot/force_destroy, which are opt-in. if attr in ("skip_final_snapshot", "force_destroy") and rc.after.get(attr) is unsafe: - yield _finding("PS010", rc, f"{attr} = {str(unsafe).lower()} on a new data store.") + yield _finding( + "PS010", rc, f"{attr} = {str(unsafe).lower()} on a new data store.", key=attr, + ) elif rc.after.get(attr) is unsafe and rc.before.get(attr) is (not unsafe): - yield _finding("PS010", rc, f"{attr} changes {str(not unsafe).lower()} -> {str(unsafe).lower()}.") + yield _finding( + "PS010", rc, + f"{attr} changes {str(not unsafe).lower()} -> {str(unsafe).lower()}.", + key=attr, + ) + + +def check_kms_anyone(rc: ResourceChange) -> Iterable[Finding]: + if rc.type == "aws_kms_key" and _introduced(rc, _kms_policy_anyone): + yield _finding("PS011", rc, "Key policy allows Principal * with no Condition.") CHECKS = ( @@ -374,6 +421,7 @@ def check_recovery_guards(rc: ResourceChange) -> Iterable[Finding]: check_public_database, check_encryption_disabled, check_recovery_guards, + check_kms_anyone, ) @@ -412,6 +460,7 @@ def review(changes: Iterable[ResourceChange], disabled: Iterable[str] = ()) -> l f.rule_id, f.severity, f.address, f"{f.message} The same object is created at {moved[rc.address]}; " "if this is a refactor, add a `moved` block instead.", + f.key, ) findings.append(f) findings.sort(key=lambda f: (-SEVERITY_ORDER[f.severity], f.rule_id, f.address)) diff --git a/tests/test_plan_skeptic.py b/tests/test_plan_skeptic.py index 4c8278a..a7e65f9 100644 --- a/tests/test_plan_skeptic.py +++ b/tests/test_plan_skeptic.py @@ -126,6 +126,11 @@ def test_refactor_without_moved_block(self): for f in found: self.assertIn("moved", f.message) + def test_kms_open_to_unblock_encrypt(self): + self.assertEqual(findings_for("flawed/08-kms-open-to-unblock-encrypt.json"), { + ("PS011", "high", "aws_kms_key.app"), + }) + def test_clean_plan_has_no_findings(self): self.assertEqual(findings_for("clean/01-tags-only.json"), set()) @@ -153,6 +158,58 @@ def test_deny_wildcard_is_not_a_grant(self): plan = plan_with(change("aws_iam_policy.x", "aws_iam_policy", ["create"], after={"policy": policy})) self.assertEqual(review(load_plan(plan)), []) + def test_role_inline_policy_wildcard(self): + # Nested inline_policy is what generated Terraform often emits instead of + # a separate aws_iam_role_policy; missing it was a silent false negative. + role = { + "name": "app", + "assume_role_policy": json.dumps({ + "Statement": [{"Effect": "Allow", "Principal": {"Service": "ec2.amazonaws.com"}, + "Action": "sts:AssumeRole"}], + }), + "inline_policy": [{ + "name": "wide", + "policy": json.dumps({ + "Statement": [{"Effect": "Allow", "Action": "*", "Resource": "*"}], + }), + }], + } + plan = plan_with(change("aws_iam_role.x", "aws_iam_role", ["create"], after=role)) + found = review(load_plan(plan)) + self.assertEqual([(f.rule_id, f.message, f.key) for f in found], [ + ("PS003", "Policy now grants Action *.", "Action *"), + ]) + + def test_role_inline_policy_existing_wildcard_not_reported_again(self): + before = { + "inline_policy": [{ + "name": "wide", + "policy": json.dumps({"Statement": [{"Effect": "Allow", "Action": "s3:*", "Resource": "*"}]}), + }], + } + after = { + "inline_policy": [{ + "name": "wide", + "policy": json.dumps({"Statement": [ + {"Effect": "Allow", "Action": "s3:*", "Resource": "*"}, + {"Effect": "Allow", "Action": "kms:*", "Resource": "*"}, + ]}), + }], + } + plan = plan_with(change("aws_iam_role.x", "aws_iam_role", ["update"], before=before, after=after)) + found = review(load_plan(plan)) + self.assertEqual([f.message for f in found], ["Policy now grants Action kms:*."]) + + def test_kms_policy_with_condition_is_not_anyone(self): + policy = json.dumps({ + "Statement": [{ + "Effect": "Allow", "Principal": "*", "Action": "kms:Decrypt", "Resource": "*", + "Condition": {"StringEquals": {"kms:CallerAccount": "111122223333"}}, + }], + }) + plan = plan_with(change("aws_kms_key.x", "aws_kms_key", ["create"], after={"policy": policy})) + self.assertEqual(review(load_plan(plan)), []) + def test_trust_policy_with_condition_is_not_anyone(self): policy = json.dumps({"Statement": [{"Effect": "Allow", "Principal": {"AWS": "*"}, "Action": "sts:AssumeRole", "Condition": {"StringEquals": {"aws:PrincipalOrgID": "o-123"}}}]}) @@ -178,6 +235,16 @@ def test_findings_sort_high_first(self): self.assertEqual([f.severity for f in found][:3], ["high"] * 3) self.assertEqual(found[-1].severity, "medium") + def test_fingerprint_ignores_message_wording(self): + found = review(load_plan(fixture("flawed/03-iam-wildcard-creep.json"))) + keys = {(f.rule_id, f.address, f.key) for f in found} + self.assertEqual(keys, { + ("PS003", "aws_iam_policy.exporter", "Action kms:*"), + ("PS003", "aws_iam_policy.exporter", "Action s3:*"), + ("PS003", "aws_iam_role_policy.ci_deploy", "NotAction in an Allow statement"), + }) + for f in found: + self.assertNotEqual(f.key, f.message) class PlanLoading(unittest.TestCase): def test_binary_plan_is_refused(self): @@ -243,6 +310,9 @@ def test_sarif_is_valid_shape(self): loc = result["locations"][0] self.assertIn("physicalLocation", loc, "code scanning drops results without one") self.assertTrue(loc["logicalLocations"][0]["fullyQualifiedName"].startswith("aws_s3_")) + fp = result["partialFingerprints"]["resourceRule"] + self.assertTrue(fp.startswith("PS007:"), fp) + self.assertNotIn("Bucket", fp, "message wording must not enter the fingerprint") def test_output_file_plus_text_summary(self): with tempfile.TemporaryDirectory() as tmp: