diff --git a/security-review/iam/CROSS-REVIEW-PACKET.md b/security-review/iam/CROSS-REVIEW-PACKET.md index 6a0062e..f30ef3e 100644 --- a/security-review/iam/CROSS-REVIEW-PACKET.md +++ b/security-review/iam/CROSS-REVIEW-PACKET.md @@ -1,10 +1,17 @@ # Cross-review packet — R720 aws-posture IAM (step-ca → Roles Anywhere → read-only AWS role) +> **GPT-4.1 cross-review 2026-06-18: APPROVE, no BLOCKs; FIXes applied** +> (`aws:SourceAccount` added to the trust policy; `ec2:DescribeImages` removed from the +> permission policy). NIT answers recorded in `aws-posture-readonly-policy.rationale.md`: +> snapshots = account-owned idle-spend signal (kept); `s3:ListAllMyBuckets` = names-only +> inventory, no object data (kept); no `logs:*` needed; `aws:RequestedRegion` deliberately +> SKIPPED (global-endpoint `ce:*`/`s3:ListAllMyBuckets` could be DENYed by a blanket region pin). + **Audience:** the mandatory GPT-4.1 IAM cross-review + Adam. -**Status:** these are AUTHORED FILES, nothing is applied to AWS. aws-posture (the checker that -*uses* this role) is **hard-gated behind this review** (design `docs/r720-agent-team-design.md` -§7, B3) and is NOT built in this change. Approving this packet unblocks provisioning + building -aws-posture; it does not itself change AWS. +**Status:** these are AUTHORED FILES, nothing is applied to AWS. The review has now PASSED +(APPROVE, no BLOCKs), which unblocks **building** aws-posture (done in this Phase-3 change set, +PROVISIONING-GATED — the checker makes no AWS call until step-ca + Roles Anywhere are stood up). +Approving this packet unblocks provisioning; it does not itself change AWS. **Account:** 328440206208 · **Region:** us-east-1 · **Box:** the always-on R720 secrev VM (single-user, unattended, currently holds a long-lived read-only GitHub PAT). @@ -52,6 +59,7 @@ sts:AssumeRole on role/r720-aws-posture-readonly │ trust policy (aws-posture-trust-policy.json) requires ALL of: │ (1) Principal = rolesanywhere.amazonaws.com (came via Roles Anywhere) │ (2) aws:SourceArn = THIS trust anchor (not some other anchor) + │ (2b) aws:SourceAccount = 328440206208 (confused-deputy guard, added in cross-review) │ (3) x509Subject/CN = "r720-aws-posture" AND x509Issuer/CN = the internal CA ▼ 1-hour STS session, permissions = aws-posture-readonly-policy.json (read-only cost + idle inventory) @@ -61,8 +69,8 @@ read-only AWS APIs: ce:Get*, cloudwatch:GetMetric*/DescribeAlarms, ec2/elb/rds:D ``` Three independent conditions must ALL hold to assume the role: via Roles Anywhere, from THIS -anchor, presenting a leaf with the pinned subject CN + issuer CN. Any one missing → AssumeRole -denied. +anchor (in THIS account, via the `aws:SourceAccount` guard added in cross-review), presenting a +leaf with the pinned subject CN + issuer CN. Any one missing → AssumeRole denied. ## Least-privilege rationale (summary; full table in the rationale .md) @@ -137,21 +145,34 @@ snapshot-restore away from gone. > gone. Paste the transcript here. Until this is filled in, the rollback is "written, not > exercised" and the phase is NOT accepted (design §7). -## Specific things for the cross-reviewer to scrutinize +## Specific things for the cross-reviewer to scrutinize (with resolutions) 1. **Trust-policy condition completeness.** Are `aws:PrincipalTag/x509Subject/CN` + `aws:PrincipalTag/x509Issuer/CN` + `ArnEquals aws:SourceArn` (the trust anchor) sufficient to prevent any other cert (or another trust anchor in the account) from assuming the role? Is there a confused-deputy gap I should also pin (e.g. should I add `aws:SourceAccount`)? + → **RESOLVED (FIX applied):** added `aws:SourceAccount = 328440206208` to the StringEquals + condition. The trust now pins anchor (SourceArn) **and** account (SourceAccount) plus the + cert CN/issuer — closing the confused-deputy gap the reviewer raised. 2. **`Resource: "*"` statements.** Confirm each is an API that genuinely has no resource-level support, and that no statement could be tightened with a condition (e.g. `aws:RequestedRegion` = us-east-1) without breaking the checker. + → **RESOLVED (NIT, SKIPPED with rationale):** every `Resource:"*"` statement is an + API family without resource-level ARNs (ce, the cloudwatch metric-data calls, ec2/elb/rds + `Describe*`). `aws:RequestedRegion` is **NOT** applied — `ce:*` and `s3:ListAllMyBuckets` are + global-endpoint services that a blanket region condition could DENY. Full reasoning in + `aws-posture-readonly-policy.rationale.md` ("Cross-review NIT answers"). 3. **Action allow-list.** Anything in here that is NOT needed for idle/anomalous-spend (i.e. over-grant), or any read verb that leaks data we don't want (the intent is pricing + inventory, no object/secret/log data). + → **RESOLVED (FIX applied):** removed `ec2:DescribeImages` (AMIs are not an idle-spend signal). + `ec2:DescribeSnapshots` kept (orphan-snapshot waste, account-owned metadata only); + `s3:ListAllMyBuckets` kept (bucket *names* only, no `s3:GetObject`); no `logs:*` granted. 4. **Session duration vs leaf lifetime.** 1h STS session + ~24h leaf — acceptable blast window? + → Accepted as-is (no change requested). 5. **Rollback ordering.** Is disabling the profile/anchor first the correct fastest-cut order, and does step 2/3 leave any orphaned grant? + → Accepted as-is (no change requested). ## Process note diff --git a/security-review/iam/aws-posture-readonly-policy.json b/security-review/iam/aws-posture-readonly-policy.json index 3756bbb..482fec6 100644 --- a/security-review/iam/aws-posture-readonly-policy.json +++ b/security-review/iam/aws-posture-readonly-policy.json @@ -40,7 +40,6 @@ "ec2:DescribeAddresses", "ec2:DescribeNatGateways", "ec2:DescribeSnapshots", - "ec2:DescribeImages", "ec2:DescribeRegions" ], "Resource": "*" diff --git a/security-review/iam/aws-posture-readonly-policy.rationale.md b/security-review/iam/aws-posture-readonly-policy.rationale.md index e41b9d6..94c4b09 100644 --- a/security-review/iam/aws-posture-readonly-policy.rationale.md +++ b/security-review/iam/aws-posture-readonly-policy.rationale.md @@ -5,8 +5,17 @@ is kept strictly valid (no inline `Comment` keys — IAM rejects those), so all here. This policy is the permission set for the **aws-posture** checker (design D5 / §4): idle / anomalous-spend watch on the Sea Haven AWS account (328440206208, us-east-1). -**aws-posture itself is NOT built in this change** — it is hard-gated behind the mandatory -GPT-4.1 IAM cross-review (design §7, B3). This file + the policy are the review inputs. +**Cross-review status (2026-06-18):** GPT-4.1 IAM cross-review returned **APPROVE, no BLOCKs**. +FIXes applied to the policy as a result: +- **Removed `ec2:DescribeImages`** (data minimization — AMIs are not part of the idle-spend + signal; orphan EBS snapshots already cover the storage-waste case via `ec2:DescribeSnapshots`). +- The trust policy (`aws-posture-trust-policy.json`) gained **`aws:SourceAccount` = + `328440206208`** as an extra confused-deputy guard alongside the existing `aws:SourceArn` + trust-anchor pin (see that file). + +**aws-posture itself is built in Phase-3 (this change set) but stays PROVISIONING-GATED** — the +checker never calls AWS until step-ca + Roles Anywhere (this packet) are stood up. This file + the +policy are the IAM cross-review inputs (design §7, B3). ## Design principle @@ -23,7 +32,7 @@ allow-list** (only the specific read verbs), not by narrowing `Resource`. |---|---|---| | `CostAndUsageReadOnly` | The core idle/anomalous-spend signal (the design flags ≈$330/mo). `GetCostAndUsage`, forecasts, dimensions, and the native CE anomaly detectors. | Cost Explorer is an account-scoped service; its API has no resource-level ARNs, so `Resource:*` is the only valid form. Only `Get*` verbs — no `ce:Update*/Create*/Delete*`, no budget mutation. | | `CloudWatchMetricsReadOnly` | Correlate spend with utilization (an instance billing but at ~0% CPU is idle). `GetMetricData`/`GetMetricStatistics`/`ListMetrics`; `DescribeAlarms*` to see whether an idle resource is already alarmed. | These metric-read APIs do not support resource-level permissions. **No `PutMetricData`, no alarm create/modify/delete.** | -| `Ec2DescribeReadOnly` | The classic idle-spend inventory: stopped instances still paying for EBS, unattached volumes, unassociated Elastic IPs, idle NAT gateways, orphan snapshots/AMIs. | `Describe*` is read-only; these list calls don't take resource ARNs. **No `Run*/Start*/Stop*/Terminate*/Modify*/Create*/Delete*`.** | +| `Ec2DescribeReadOnly` | The classic idle-spend inventory: stopped instances still paying for EBS, unattached volumes, unassociated Elastic IPs, idle NAT gateways, orphan snapshots. (`ec2:DescribeImages` was **removed** in cross-review — AMIs are not an idle-spend signal aws-posture acts on.) | `Describe*` is read-only; these list calls don't take resource ARNs. **No `Run*/Start*/Stop*/Terminate*/Modify*/Create*/Delete*`.** | | `ElbAndRdsDescribeReadOnly` | Idle load balancers (no healthy targets) and idle/oversized RDS are frequent waste. `Describe*` only. | List APIs without resource-level ARNs. **No `rds:Modify*/Delete*/Reboot*`, no ELB mutation.** | | `LambdaAndStorageInventoryReadOnly` | Inventory functions + buckets to correlate against CloudWatch idle metrics. | **Deliberately excludes `s3:GetObject`** — the role never reads object *data*, only `ListAllMyBuckets` + `GetBucketLocation` (existence/region). **No `lambda:InvokeFunction`, no Lambda mutation.** This is the tightest the inventory can be while still seeing what exists. | @@ -38,6 +47,29 @@ allow-list** (only the specific read verbs), not by narrowing `Resource`. A leaked session from this role can **enumerate and price the account, and nothing more** — it cannot read application data, secrets, or change a single resource. +## Cross-review NIT answers (2026-06-18) + +- **`ec2:DescribeSnapshots` kept (NIT: is it needed?)** — yes. Orphan EBS snapshots are a common + idle-spend line item (snapshots of long-deleted volumes keep billing); the checker lists them + to flag that waste. It returns only account-owned metadata (we query with `OwnerIds=["self"]`), + no snapshot data. `ec2:DescribeImages` (AMIs) was the over-grant and was **removed**. +- **`s3:ListAllMyBuckets` kept (NIT: data exposure?)** — it returns only bucket *names* you own, + no object data and no bucket contents; `s3:GetBucketLocation` returns only the region. Both are + account-owned inventory queries needed to correlate idle buckets/regions against cost. **No + `s3:GetObject`** anywhere, so there is no data-plane read path. +- **No `logs:*` (NIT: do we need CloudWatch Logs?)** — no. aws-posture reasons over *metrics* + (`cloudwatch:GetMetric*`) and the cost/inventory describe calls; it never needs log *events*. + Omitting `logs:GetLogEvents`/`logs:FilterLogEvents` keeps the role off the log-exfil path. +- **`aws:RequestedRegion` condition (NIT: optional region pin) — SKIPPED, deliberately.** The + reviewer flagged this as optional. It is **NOT applied** because Cost Explorer (`ce:*`) and + `s3:ListAllMyBuckets` are **global-endpoint services** that resolve to us-east-1 with request + contexts where `aws:RequestedRegion` does not reliably equal `us-east-1` — a blanket region + condition risks **DENYing the core cost signal**. Scoping it to a separate statement covering + only the regional `Describe*` calls (ec2/rds/elb/cloudwatch) would add a fourth+ statement for + marginal benefit (the action allow-list already bounds blast radius, and the box only ever runs + in us-east-1). Per the task's guidance, we prefer SKIP over a region pin that could break the + global-service statements. + ## Comparison to the AWS-managed alternatives `ReadOnlyAccess` / `ViewOnlyAccess` are far broader (they include `s3:GetObject`, diff --git a/security-review/iam/aws-posture-trust-policy.json b/security-review/iam/aws-posture-trust-policy.json index 64bd454..ffb812f 100644 --- a/security-review/iam/aws-posture-trust-policy.json +++ b/security-review/iam/aws-posture-trust-policy.json @@ -15,7 +15,8 @@ "Condition": { "StringEquals": { "aws:PrincipalTag/x509Subject/CN": "r720-aws-posture", - "aws:PrincipalTag/x509Issuer/CN": "Sea Haven Internal CA - R720 Roles Anywhere" + "aws:PrincipalTag/x509Issuer/CN": "Sea Haven Internal CA - R720 Roles Anywhere", + "aws:SourceAccount": "328440206208" }, "ArnEquals": { "aws:SourceArn": "arn:aws:rolesanywhere:us-east-1:328440206208:trust-anchor/REPLACE_WITH_TRUST_ANCHOR_ID"