plan-skeptic v0.1.1: close two silent misses, improve Action UX

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,23 +51,24 @@ 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
 
 | Id | Severity | Flags |
 |---|---|---|
 | <a id="ps001"></a>PS001 | high | A data-holding resource (database, bucket, table, volume, key, PVC) is destroyed or replaced, and why it is being replaced |
 | <a id="ps002"></a>PS002 | medium | Any other resource is deleted |
-| <a id="ps003"></a>PS003 | high | An IAM policy newly grants `*` / `service:*`, or uses `NotAction` in an Allow |
+| <a id="ps003"></a>PS003 | high | An IAM policy (including role `inline_policy`) newly grants `*` / `service:*`, or uses `NotAction` in an Allow |
 | <a id="ps004"></a>PS004 | high | AdministratorAccess, PowerUserAccess or IAMFullAccess is attached |
 | <a id="ps005"></a>PS005 | high | A trust policy lets any principal assume the role without a Condition |
 | <a id="ps006"></a>PS006 | high | Ingress opened to 0.0.0.0/0 or ::/0 (medium for ports 80 and 443) |
 | <a id="ps007"></a>PS007 | high | An S3 bucket made public by ACL, bucket policy or public access block |
 | <a id="ps008"></a>PS008 | high | A database given `publicly_accessible = true` |
 | <a id="ps009"></a>PS009 | medium | Encryption at rest set to false |
 | <a id="ps010"></a>PS010 | medium | `deletion_protection` switched off, or `skip_final_snapshot` / `force_destroy` switched on |
+| <a id="ps011"></a>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

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

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,

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": {}
+      }
+    }
+  ]
+}

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"

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"

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
     ]

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,21 +349,28 @@ 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]:
     if rc.type == "aws_s3_bucket_public_access_block":
         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))