From 3b74aa3abb19be5ce9387dbfb20fb670f8a59b45 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Mon, 18 May 2026 16:50:38 +0700 Subject: [PATCH] docs(security): audit reports + IAM least-privilege plan + project policy Captures the 2026-05-18 security review session output: - plans/reports/code-reviewer-260518-1019-security-aws-infra.md - plans/reports/code-reviewer-260518-1019-security-go-app.md - plans/reports/researcher-260518-1019-security-dependencies.md - docs/deploy-aws-free-tier-guide.md (adds free-tier hard rule + accepted security trade-offs as project standards) Plan for the two HIGH-severity findings (F1, F2) targeting github-deploy-miti99bot OIDC role: plans/260518-1019-iam-least-privilege/. Plan was red-team-reviewed (15 findings applied) and validate-interviewed (4 decisions recorded). Zero unresolved contradictions. Implementation not yet started; phase 1 is standalone and lowest risk. Other audit findings (F3 CORS, F4 root handler, F5-F16) deferred to future commits; rationale in audit report. --- docs/deploy-aws-free-tier-guide.md | 28 +++ .../phase-01-narrow-oidc-trust-f2.md | 130 ++++++++++ .../phase-02-discover-required-actions.md | 193 +++++++++++++++ .../phase-03-draft-custom-policy.md | 137 ++++++++++ .../phase-04-cutover-validate.md | 233 ++++++++++++++++++ .../phase-05-update-bootstrap-docs.md | 148 +++++++++++ plans/260518-1019-iam-least-privilege/plan.md | 142 +++++++++++ ...reviewer-260518-1019-security-aws-infra.md | 99 ++++++++ ...de-reviewer-260518-1019-security-go-app.md | 101 ++++++++ ...rcher-260518-1019-security-dependencies.md | 159 ++++++++++++ 10 files changed, 1370 insertions(+) create mode 100644 plans/260518-1019-iam-least-privilege/phase-01-narrow-oidc-trust-f2.md create mode 100644 plans/260518-1019-iam-least-privilege/phase-02-discover-required-actions.md create mode 100644 plans/260518-1019-iam-least-privilege/phase-03-draft-custom-policy.md create mode 100644 plans/260518-1019-iam-least-privilege/phase-04-cutover-validate.md create mode 100644 plans/260518-1019-iam-least-privilege/phase-05-update-bootstrap-docs.md create mode 100644 plans/260518-1019-iam-least-privilege/plan.md create mode 100644 plans/reports/code-reviewer-260518-1019-security-aws-infra.md create mode 100644 plans/reports/code-reviewer-260518-1019-security-go-app.md create mode 100644 plans/reports/researcher-260518-1019-security-dependencies.md diff --git a/docs/deploy-aws-free-tier-guide.md b/docs/deploy-aws-free-tier-guide.md index 9d47e48..3c66d29 100644 --- a/docs/deploy-aws-free-tier-guide.md +++ b/docs/deploy-aws-free-tier-guide.md @@ -8,6 +8,34 @@ Related docs: --- +## Project Standards (Non-Negotiables) + +### Standard 1: Free Tier Is a Hard Requirement + +Any AWS line item that incurs a cost is disqualified, even if monthly usage rounds the bill to $0.00. The principle: **no per-invocation or per-request charges**, period. Free-tier qualification is evaluated per service (not aggregate). + +**Disqualified resources:** EventBridge ApiDestination invocations ($0.20/M, no free tier). + +**Free-tier-covered resources:** Lambda (1M req + 400k GB-s/mo), DynamoDB on-demand (2.5M read + 1M write units/mo), EventBridge Scheduler (14M invocations/mo), SQS (1M req/mo), CloudWatch Logs (5 GB ingest/mo), CloudFormation, IAM, Budgets, SSM Parameter Store Standard tier. + +When evaluating new AWS resources, explicitly verify per-service free-tier coverage in the AWS pricing docs. The `MonthlyBudget` resource (see below) is a safety net, but design must not depend on it. If you hit the $1 threshold, something violated this standard. + +### Standard 2: Security Trade-Offs Accepted + +Non-negotiable security boundary: AWS resources must not be publicly reachable except via the designed surface — Lambda Function URL serving `/webhook` (Telegram) and `/cron/{name}` (EventBridge Scheduler). Everything else is private or IAM-gated. + +Trade-offs explicitly accepted in this project (do NOT flag as "harden this" unless they breach the boundary above): +- Secrets in CloudWatch Logs / Lambda environment (visible to anyone with logs:GetLogEvents — IAM gates that). +- Secrets embedded in EventBridge Scheduler Target.Input (visible to anyone with scheduler:GetSchedule — IAM gates that). +- NoEcho CloudFormation parameters instead of Secrets Manager (simplifies deployment, avoids storage cost). +- SSM SecureString fetched at Lambda cold start instead of Secrets Manager with rotation (faster, no API call overhead, acceptable risk given Lambda is not a long-lived server). + +Rationale: in this project, **free-tier compliance outweighs secret-storage maturity**. Architectural change (Secrets Manager, rotation, KMS key rotation) would either cost money or require additional infrastructure to stay free-tier. Rejected in favor of free tier: EventBridge Connection's service-linked Secrets Manager secret (would cost). + +**Red flags that should trigger review:** Any change that (a) makes a non-Function-URL AWS resource publicly reachable, (b) changes Function URL's AuthType from NONE (mitigated by app-level token check), (c) adds a second public-facing Lambda endpoint, (d) ships `cronDisabled=true` due to empty CronSharedSecret. + +--- + ## What you get (all free-tier) | Resource | Free quota | This bot's usage | diff --git a/plans/260518-1019-iam-least-privilege/phase-01-narrow-oidc-trust-f2.md b/plans/260518-1019-iam-least-privilege/phase-01-narrow-oidc-trust-f2.md new file mode 100644 index 0000000..0291806 --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/phase-01-narrow-oidc-trust-f2.md @@ -0,0 +1,130 @@ +--- +phase: 1 +title: "Narrow OIDC trust (F2)" +status: pending +priority: P1 +effort: "30m" +dependencies: [] +--- + +# Phase 1: Narrow OIDC trust (F2) + +## Overview + +Remove the `pull_request` claim (and `refs/heads/dev` if unused) from the OIDC trust policy on `github-deploy-miti99bot`. After this lands, only pushes to `main` can assume the deploy role. + +## Requirements + +**Functional** +- The `Condition.StringLike.token.actions.githubusercontent.com:sub` array on the role's trust policy must contain only branch refs that are actually used to deploy. +- Current set: `refs/heads/main`, `refs/heads/dev`, `pull_request`. +- Target set: `refs/heads/main` (plus `refs/heads/dev` ONLY if it's used; default-drop otherwise). + +**Non-functional** +- Apply out-of-band via maintainer's local `admin` AWS profile — not through the workflow being modified. +- Rollback: re-apply the previous `iam-github-oidc-trust.json` snapshot via the same `aws iam update-assume-role-policy` call. + +## Architecture + +Single JSON file (`aws/iam-github-oidc-trust.json`) is the source of truth committed to the repo; AWS-side trust policy is updated via `aws iam update-assume-role-policy`. No CloudFormation involvement. + +## Related Code Files + +- Modify: `aws/iam-github-oidc-trust.json` (drop 1-2 lines from `sub` allowlist) +- Read-only: `.github/workflows/deploy.yml` (confirms only `main` is on the `push` trigger) + +## Implementation Steps + +0. **Verify admin profile is reachable** (RT-4). `aws/README.md:119` recommends deleting admin keys as hardening posture, so before proceeding confirm the operator can authenticate: + ```sh + aws sts get-caller-identity --profile admin + ``` + If this fails with `InvalidClientTokenId` / `Unable to locate credentials`: recreate admin access keys via console (root → IAM → Users → admin → Security credentials → Create access key), or perform every step in this phase via the AWS Console fallback below. + + **Console fallback for Phase 1:** IAM → Roles → `github-deploy-miti99bot` → Trust relationships → Edit trust policy → paste the JSON from step 3 → Update policy. + +1. **Confirm `dev` is not used.** Inspect `.github/workflows/deploy.yml:5` — `on.push.branches` is `[main]`. Search every workflow file: + ```sh + rg -l 'dev|id-token' .github/workflows/ + ``` + Also check for any workflow with `permissions: id-token: write` (only OIDC-capable workflows matter). Current state (verified 2026-05-18): only `deploy.yml` has `id-token: write`. `ci.yml` has `permissions: contents: read` only — physically cannot mint OIDC. So `refs/heads/dev` is dormant; drop it. The procedure to re-add for a future preview env is documented in Phase 5 (RT-15). + +2. **No /tmp snapshot needed** (RT-8). `aws/iam-github-oidc-trust.json` is git-tracked. Rollback = `git show HEAD:aws/iam-github-oidc-trust.json | aws iam update-assume-role-policy --role-name github-deploy-miti99bot --policy-document file:///dev/stdin --profile admin`. Capture the pre-edit `HEAD` commit hash for explicit rollback: + ```sh + git rev-parse HEAD # save this — recovery uses it + ``` + +3. **Edit + commit FIRST, then apply** (V-4 decision). The repo file is the source of truth. Commit the edit before invoking `aws iam update-assume-role-policy` so: + - `git show HEAD^:aws/iam-github-oidc-trust.json` always recovers the previous state. + - If the apply fails / hangs, the repo file matches the intended target — no AWS/repo drift. + + Edit `aws/iam-github-oidc-trust.json` to its final shape: + ```json + { + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Principal": {"Federated": "arn:aws:iam::225603493174:oidc-provider/token.actions.githubusercontent.com"}, + "Action": "sts:AssumeRoleWithWebIdentity", + "Condition": { + "StringEquals": {"token.actions.githubusercontent.com:aud": "sts.amazonaws.com"}, + "StringLike": {"token.actions.githubusercontent.com:sub": [ + "repo:tiennm99/miti99bot:ref:refs/heads/main" + ]} + } + }] + } + ``` + +4. **Commit the edit**, then **apply** out-of-band (V-4 — commit-first): + ```sh + git add aws/iam-github-oidc-trust.json + git commit -m "fix(security): narrow OIDC trust to main only (F2)" + + aws iam update-assume-role-policy \ + --role-name github-deploy-miti99bot \ + --policy-document file://aws/iam-github-oidc-trust.json \ + --profile admin + ``` + If the `aws iam` call fails, the commit is harmless on its own (workflows still use the live AWS-side trust). Reverse with `git revert HEAD` if abandoning the change. + +5. **Smoke test (positive path):** trigger `workflow_dispatch` from GitHub Actions on `main`. Expect the `configure-aws-credentials` step to succeed and the deploy to proceed exactly as before. + +6. **Smoke test (positive verifies narrowing):** the trust narrowing is enforced by AWS IAM at `sts:AssumeRoleWithWebIdentity` time, not by workflow trigger configuration. Step 5's successful `workflow_dispatch` on main proves the trust still permits the intended caller. No PR-triggered workflow currently has `id-token: write`, so a negative-path test would require provisioning a throwaway workflow — out of scope here. If you want belt-and-braces, see Phase 5's "Trust policy invariants" subsection for how to add a synthetic OIDC token verification step. + +7. **Already committed in step 4** (V-4). Nothing to do here. + +## Todo List + +- [ ] **Step 0:** Verify `aws sts get-caller-identity --profile admin` succeeds (RT-4) +- [ ] Verify `dev` branch and OIDC `id-token: write` usage with `rg -l 'dev|id-token' .github/workflows/` +- [ ] Capture pre-edit HEAD via `git rev-parse HEAD` (RT-8 — git is the snapshot) +- [ ] Edit `aws/iam-github-oidc-trust.json` +- [ ] Apply via `aws iam update-assume-role-policy` +- [ ] workflow_dispatch deploy succeeds on main +- [ ] Commit JSON edit +- [ ] Mark phase complete via `ck plan check 1` + +## Success Criteria + +- [ ] `aws iam get-role --role-name github-deploy-miti99bot --query 'Role.AssumeRolePolicyDocument.Statement[0].Condition.StringLike."token.actions.githubusercontent.com:sub"'` returns only `["repo:tiennm99/miti99bot:ref:refs/heads/main"]`. +- [ ] `workflow_dispatch` deploy on `main` succeeds end-to-end. +- [ ] Commit `aws/iam-github-oidc-trust.json` is on `main`. + +## Risk Assessment + +| Risk | Likelihood | Mitigation | +|---|---|---| +| Rollback needed (e.g. forgot dev branch is used by something) | Low | `git show :aws/iam-github-oidc-trust.json \| aws iam update-assume-role-policy --policy-document file:///dev/stdin --profile admin` — git is the snapshot (RT-8). | +| OIDC-claim format typo locks out all deploys | Very Low | JSON file is small + reviewable; AWS rejects malformed `sub` patterns at update time. | +| User has no `admin` profile / local AWS creds | Med | Step 0 verifies admin reachability before edits. Console fallback documented inline in step 0 (RT-4). | + +## Security Considerations + +- This phase REDUCES attack surface; no new privileges introduced. +- After this lands, a leaked GitHub PR-context OIDC token cannot assume this role even if other findings remain unfixed. +- Pairs with Phase 4 (F1 cutover) — together they reduce blast radius from "any PR = account takeover" to "any push to main = scoped deploy only". + +## Next Steps + +Phase 1 is standalone. Phase 2 (Discover required actions) can start in parallel or after — no dependency. diff --git a/plans/260518-1019-iam-least-privilege/phase-02-discover-required-actions.md b/plans/260518-1019-iam-least-privilege/phase-02-discover-required-actions.md new file mode 100644 index 0000000..3a60054 --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/phase-02-discover-required-actions.md @@ -0,0 +1,193 @@ +--- +phase: 2 +title: "Discover required actions" +status: pending +priority: P1 +effort: "1-2h" +dependencies: [] +--- + +# Phase 2: Discover required actions + +## Overview + +Enumerate every IAM action `sam deploy` invokes for the miti99bot stack, scoped to the specific resources in `template.yaml`. Output is a documented action × resource table that Phase 3 translates into JSON policy statements. + +A missing action here = pipeline broken on next push (per F1 risk highlight). A too-broad action = doesn't satisfy least-privilege intent. Bias toward enumerating real call-sites over generic SAM-deploy guides. + +**Inventory MUST be fully populated before Phase 3 begins** (RT-1). The "Action Inventory" section below cannot ship with `TBD` placeholders. Phase 3's blocking dependency on this phase requires a complete table. + +### Categories that are easy to miss — explicit mandatory checks (RT-10, RT-13) + +For each AWS service in `template.yaml`, you MUST enumerate: + +1. **Lifecycle:** Create / Update / Delete / Get / List actions on each resource ARN. +2. **Tagging:** `*:TagResource` / `*:UntagResource` / `*:ListTagsForResource` (or service-specific equivalents — `dynamodb:TagResource`, `lambda:TagResource`, `logs:TagLogGroup`/`TagResource`, `sqs:TagQueue`, `scheduler:TagResource`, `iam:TagRole`, `iam:UntagRole`, `iam:ListRoleTags`). CFN applies tags on every CREATE and many UPDATE paths — missing one = guaranteed UPDATE failure. +3. **Sub-resources** for Lambda specifically: `lambda:CreateFunctionUrlConfig`, `lambda:UpdateFunctionUrlConfig`, `lambda:DeleteFunctionUrlConfig`, `lambda:GetFunctionUrlConfig`, `lambda:AddPermission`, `lambda:RemovePermission`, `lambda:GetPolicy`. +4. **Rollback path:** `cloudformation:ContinueUpdateRollback`, `cloudformation:CancelUpdateStack`, `cloudformation:RollbackStack` (RT-3 — without these, a stuck stack cannot be recovered without re-attaching FullAccess). +5. **SAM-managed S3 bucket bootstrap** (RT-13): `s3:CreateBucket`, `s3:GetBucketLocation`, `s3:GetBucketVersioning`, `s3:PutBucketVersioning`, `s3:GetEncryptionConfiguration`, `s3:PutEncryptionConfiguration`, `s3:PutBucketPolicy`, `s3:ListBucket`, `s3:PutObject`, `s3:GetObject`, `s3:DeleteObject`. SAM creates the bucket on first deploy when `resolve_s3 = true` (`samconfig.toml:13`). + +## Requirements + +**Functional** +- Cover every AWS API call the deploy workflow makes from start (`actions/checkout`) to end (`setMyCommands`). +- Group calls by service. For each: action name, resource ARN pattern, optional Conditions. +- Cover both happy-path (CREATE) and update-path (UPDATE_IN_PROGRESS → COMPLETE) and rollback (UPDATE_ROLLBACK_*) — IAM checks all three on a failed deploy. + +**Non-functional** +- Output lives in the phase file's "Action inventory" section so Phase 3 reads directly from here. +- No AWS calls in this phase — pure code reading. + +## Architecture + +Read `template.yaml` resource-by-resource; for each `Type: AWS::*::*`, look up which IAM actions CFN issues. Cross-reference with the workflow's explicit `aws` CLI calls (`aws ssm get-parameter`, `aws cloudformation describe-stacks`). + +## Related Code Files + +- Read: `template.yaml` (every Resources entry + Outputs) +- Read: `.github/workflows/deploy.yml` (steps post-`configure-aws-credentials` that issue AWS calls) +- Read: `samconfig.toml` (`resolve_s3 = true` → SAM manages an artifact bucket) +- No files modified in this phase. Output is appended to this phase doc. + +## Implementation Steps + +1. **Service inventory.** From `template.yaml` resource types: + - `AWS::DynamoDB::Table` + - `AWS::Logs::LogGroup`, `AWS::Logs::MetricFilter` + - `AWS::Serverless::Function` (expands to `AWS::Lambda::Function` + `AWS::IAM::Role` + `AWS::Lambda::Url` + permissions) + - `AWS::SQS::Queue` + - `AWS::IAM::Role` + - `AWS::Scheduler::Schedule` + - `AWS::Budgets::Budget` (Conditional) + - Plus framework: CloudFormation (changesets), S3 (artifact bucket), STS (caller identity). + +2. **For each resource, enumerate the IAM actions CFN calls on UPDATE+ROLLBACK paths.** Sources: AWS CFN per-resource documentation (the IAM permissions table at the top of each page). Don't trust memory — verify against docs. + +3. **Workflow-explicit calls:** + - `aws cloudformation describe-stacks` → `cloudformation:DescribeStacks` + - `aws ssm get-parameter --with-decryption` → `ssm:GetParameter` + the SSM service-managed KMS key has implicit access (no extra IAM action required for the AWS-owned key path) + - SAM internal: `cloudformation:CreateChangeSet` / `DescribeChangeSet` / `ExecuteChangeSet` / `DeleteChangeSet`, `cloudformation:DescribeStackEvents` / `ListStackResources` / `GetTemplateSummary`, S3 multipart upload, `sts:GetCallerIdentity` (SAM probes account/region on startup). + +4. **iam:PassRole identification.** SAM creates an execution role for `BotFunction` and an inline role for the SchedulerExecutionRole. Both need `iam:PassRole` so CFN can attach them to the Lambda / Scheduler. Scope: roles whose path or name match `miti99bot*`. + +5. **Resource ARN patterns.** Use `miti99bot*` (not `miti99bot`) on stack-scoped resources so a future `miti99bot-dev` parallel stack works (RT-14). For each action, write the tightest ARN pattern still passing on a fresh deploy: + - Stack: `arn:aws:cloudformation:ap-southeast-1:225603493174:stack/miti99bot*/*` + - ChangeSet: `arn:aws:cloudformation:ap-southeast-1:225603493174:changeSet/*/miti99bot*/*` + - DynamoDB: `arn:aws:dynamodb:ap-southeast-1:225603493174:table/miti99bot*` (covers `miti99bot-data` + future dev `miti99bot-dev-data`) + - Lambda function: `arn:aws:lambda:ap-southeast-1:225603493174:function:miti99bot*` + - Lambda layer (read-only ref to AWSLabs adapter): `arn:aws:lambda:ap-southeast-1:753240598075:layer:LambdaAdapterLayerArm64:*` + - SQS queue: `arn:aws:sqs:ap-southeast-1:225603493174:miti99bot*` + - IAM roles created by stack: `arn:aws:iam::225603493174:role/miti99bot*` (NOTE: the deploy role itself is `github-deploy-miti99bot` — does NOT match because IAM globs are left-anchored) + - Log group: `arn:aws:logs:ap-southeast-1:225603493174:log-group:/aws/lambda/miti99bot*` + - SSM parameter: `arn:aws:ssm:ap-southeast-1:225603493174:parameter/miti99bot/*/*` (`/prod/*` AND `/dev/*` — covers both envs without widening to other apps) + - Budget: `arn:aws:budgets::225603493174:budget/miti99bot*` + - Scheduler: `arn:aws:scheduler:ap-southeast-1:225603493174:schedule/*/miti99bot*` (wildcard the GroupName segment — `template.yaml:217-228` does not set `GroupName`; EventBridge defaults to `default` today but pinning to `default` is undocumented contract → RT-11) + - SAM S3 bucket: **discover at execution time** — see step 5a below (RT-13) + +5a. **Discover the actual SAM artifact bucket** (RT-13). Do not assume the `aws-sam-cli-managed-default-samclisourcebucket-*` convention is stable. Run: + ```sh + aws s3 ls --profile admin | grep -E 'sam|miti99bot' || \ + aws cloudformation describe-stacks --stack-name aws-sam-cli-managed-default \ + --query "Stacks[0].Outputs[?OutputKey=='SourceBucket'].OutputValue" --output text --profile admin + ``` + Use the discovered name verbatim in the Phase 3 ARN list. If empty (fresh account), the policy must include `s3:CreateBucket` on `arn:aws:s3:::aws-sam-cli-managed-default-*` so SAM can create it on first deploy. + +6. **Wildcard-required actions.** Some IAM actions have no resource-level support (must use `Resource: "*"`). Document each: + - `sts:GetCallerIdentity` — always `*` + - `cloudformation:ListStacks` (used by SAM during a deploy if it scans) — `*` + - `s3:ListAllMyBuckets` (SAM uses this to find the managed bucket if not configured) — `*` (low risk: read-only across all buckets, no data exposed) + +7. **Output: Action × Resource inventory.** Append a table to this phase doc with columns: Service · Action · Resource ARN · Notes. Phase 3 consumes this verbatim. + +## Action Inventory + +> Filled during execution. Phase 3 reads from here. **No TBD allowed** — Phase 3 is blocked until every category below has actual actions listed (RT-1). + +Each category MUST include: lifecycle actions, tagging actions, sub-resource actions (if applicable), rollback-path actions (if applicable). See "Categories that are easy to miss" in Overview above. + +### CloudFormation +- Stack lifecycle: `cloudformation:CreateStack`, `UpdateStack`, `DeleteStack`, `DescribeStacks`, `DescribeStackEvents`, `DescribeStackResources`, `ListStackResources`, `GetTemplate`, `GetTemplateSummary`, `ValidateTemplate` +- ChangeSet: `cloudformation:CreateChangeSet`, `ExecuteChangeSet`, `DescribeChangeSet`, `DeleteChangeSet`, `ListChangeSets` +- Rollback (RT-3): `cloudformation:ContinueUpdateRollback`, `CancelUpdateStack`, `RollbackStack` +- Tagging: `cloudformation:TagResource`, `UntagResource`, `ListStackResources` +- Global read: `cloudformation:ListStacks` (no resource-level support) + +### S3 (SAM artifact bucket) +- Bucket lifecycle (RT-13): `s3:CreateBucket`, `GetBucketLocation`, `GetBucketVersioning`, `PutBucketVersioning`, `GetEncryptionConfiguration`, `PutEncryptionConfiguration`, `GetBucketPolicy`, `PutBucketPolicy` +- Objects: `s3:ListBucket`, `PutObject`, `GetObject`, `DeleteObject`, `PutObjectTagging` +- Global read (justified): `s3:ListAllMyBuckets` (no resource-level support; needed by SAM CLI to find the managed bucket on first run) + +### IAM +- Role lifecycle: `iam:CreateRole`, `DeleteRole`, `GetRole`, `ListRoles`, `PutRolePolicy`, `DeleteRolePolicy`, `GetRolePolicy`, `ListRolePolicies`, `AttachRolePolicy`, `DetachRolePolicy`, `ListAttachedRolePolicies` +- **NOT included** (RT-2): `iam:UpdateAssumeRolePolicy` — CFN never invokes this on stack-managed roles (trust changes go via Delete+Create). Including it enables trust-rewrite escalation. +- PassRole: `iam:PassRole` (with `iam:PassedToService` Condition — see Phase 3, RT-7) +- AttachRolePolicy Condition (RT-6): scope `iam:PolicyARN` to AWS-managed policies the stack actually attaches (currently none — Lambda execution role uses SAM macros that inline policies, not attach managed; if SAM ever changes, add specific ARNs). Best path: omit `AttachRolePolicy` entirely until proven necessary. +- Tagging: `iam:TagRole`, `UntagRole`, `ListRoleTags` + +### Lambda +- Function lifecycle: `lambda:CreateFunction`, `UpdateFunctionCode`, `UpdateFunctionConfiguration`, `GetFunction`, `GetFunctionConfiguration`, `DeleteFunction`, `PublishVersion`, `ListVersionsByFunction` +- Function URL sub-resource (RT-10): `lambda:CreateFunctionUrlConfig`, `UpdateFunctionUrlConfig`, `DeleteFunctionUrlConfig`, `GetFunctionUrlConfig` +- Resource-based policy: `lambda:AddPermission`, `RemovePermission`, `GetPolicy` +- Layer read (cross-account, AWSLabs): `lambda:GetLayerVersion` +- Tagging: `lambda:TagResource`, `UntagResource`, `ListTags` + +### DynamoDB +- Table lifecycle: `dynamodb:CreateTable`, `UpdateTable`, `DescribeTable`, `DeleteTable`, `ListTables` +- Tagging: `dynamodb:TagResource`, `UntagResource`, `ListTagsOfResource` +- (No data-plane actions for deploy role; Lambda execution role has those separately.) + +### EventBridge Scheduler +- Schedule lifecycle: `scheduler:CreateSchedule`, `UpdateSchedule`, `GetSchedule`, `DeleteSchedule`, `ListSchedules` +- Tagging: `scheduler:TagResource`, `UntagResource`, `ListTagsForResource` + +### SQS +- Queue lifecycle: `sqs:CreateQueue`, `DeleteQueue`, `GetQueueAttributes`, `SetQueueAttributes`, `GetQueueUrl`, `ListQueues` +- Tagging: `sqs:TagQueue`, `UntagQueue`, `ListQueueTags` + +### CloudWatch Logs +- Log group lifecycle: `logs:CreateLogGroup`, `DeleteLogGroup`, `DescribeLogGroups`, `PutRetentionPolicy`, `DeleteRetentionPolicy` +- Metric filter: `logs:PutMetricFilter`, `DeleteMetricFilter`, `DescribeMetricFilters` +- Tagging: `logs:TagResource`, `UntagResource`, `ListTagsForResource` + +### Budgets +- Budget lifecycle: `budgets:CreateBudget`, `ModifyBudget`, `DescribeBudget`, `DeleteBudget` +- Notification: `budgets:CreateNotification`, `DeleteNotification`, `DescribeNotificationsForBudget`, `CreateSubscriber`, `DeleteSubscriber` + +### SSM (workflow-explicit) +- `ssm:GetParameter`, `ssm:GetParameters` — used by `.github/workflows/deploy.yml:53,76,80,84,108` (cron secret + telegram token + webhook secret fetches) and by Lambda cold start; scope to `parameter/miti99bot/*/*` + +### STS +- `sts:GetCallerIdentity` — used by SAM at deploy start (no resource-level support; wildcard required) + +## Todo List + +- [ ] Service-by-service walk of template.yaml +- [ ] Cross-reference each AWS::* type against CFN per-resource IAM requirements +- [ ] Enumerate workflow-explicit `aws` CLI calls +- [ ] Identify `iam:PassRole` targets +- [ ] Build action × resource × ARN-pattern table in this doc +- [ ] Identify wildcard-required actions and justify each +- [ ] Mark phase complete via `ck plan check 2` + +## Success Criteria + +- [ ] Every CFN resource type in `template.yaml` has an entry in the action inventory. +- [ ] Every wildcard `Resource: "*"` has a 1-line "why not scoped" justification. +- [ ] Phase 3 can write the policy file by transcribing the inventory; no further AWS docs lookup needed in Phase 3. + +## Risk Assessment + +| Risk | Likelihood | Mitigation | +|---|---|---| +| Miss a CFN action (e.g. tagging permission, drift detection) | Med | Test by attaching the eventual policy in dual-mode (Phase 4) before detaching FullAccess. CFN failures surface in CloudFormation events; map to missing action and re-add. | +| AWS adds new required actions after this work | Low | Documented in `aws/README.md` (Phase 5): on `sam deploy` UPDATE failure with AccessDenied, check CloudTrail event → identify missing action → patch policy. | +| Over-tight ARN pattern (e.g. forgot `/index/*` for a future GSI) | Low | Phase 3 will use globs (`miti99bot*` not `miti99bot`) where the stack might extend. | + +## Security Considerations + +- This phase is read-only — no security implications. +- Output drives Phase 3's security boundary. + +## Next Steps + +Phase 3 starts after this phase's action inventory is complete. diff --git a/plans/260518-1019-iam-least-privilege/phase-03-draft-custom-policy.md b/plans/260518-1019-iam-least-privilege/phase-03-draft-custom-policy.md new file mode 100644 index 0000000..1d70cd6 --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/phase-03-draft-custom-policy.md @@ -0,0 +1,137 @@ +--- +phase: 3 +title: "Draft custom policy" +status: pending +priority: P1 +effort: "1h" +dependencies: [2] +--- + +# Phase 3: Draft custom policy + +## Overview + +Translate Phase 2's action inventory into a single JSON IAM policy document committed to the repo at `aws/iam-github-deploy-policy.json`. Validate JSON syntax and run AWS IAM Policy Simulator dry-run. + +## Requirements + +**Functional** +- Single `Version: "2012-10-17"` policy with multiple `Statement` entries, grouped by service. +- Each statement: `Effect: Allow`, action list, resource ARN list (with `${AWS::AccountId}` / `${AWS::Region}` interpolated to the actual account `225603493174` and region `ap-southeast-1`). +- `iam:PassRole` statement scoped via `Condition.ForAllValues:StringEquals.iam:PassedToService` to `lambda.amazonaws.com` and `scheduler.amazonaws.com` only (RT-7). Adding a new service later = explicit policy update via documented procedure in Phase 5. +- `iam:UpdateAssumeRolePolicy` deliberately EXCLUDED (RT-2) — CFN does not invoke it on stack-managed roles; including it enables trust-rewrite escalation. +- `iam:AttachRolePolicy` either omitted entirely OR scoped via `Condition.ArnEquals.iam:PolicyARN` to a documented allowlist (RT-6). Current SAM macros use inline `PutRolePolicy` only — start with omission, add only if a deploy fails AccessDenied on this action. +- Wildcard `Resource: "*"` only where the action has no resource-level support (Phase 2 enumerated these). + +**Empirical verification (RT-7) before applying:** +- After Phase 2 inventory complete, before Phase 4 starts, run `aws iam simulate-principal-policy` against the draft policy with each Phase-2-enumerated action × target ARN. Pay special attention to `iam:PassRole` on each stack-managed role: the simulator's "ImplicitDeny" result for `iam:PassedToService` mismatches surfaces here, not at deploy time. + +**Non-functional** +- Total policy size must stay under 6,144 chars (AWS managed-policy hard limit) OR be split into two inline policies on the same role. +- File committed to repo so future bootstrap reads from version control. +- JSON formatted with 2-space indent; trailing newline; sorted statements by service alphabetically for diff readability. + +## Architecture + +``` +aws/ +├── iam-github-deploy-policy.json ← NEW (this phase) +├── iam-github-oidc-trust.json ← Phase 1 narrowed this +└── README.md ← Phase 5 updates to reference new file +``` + +Single artifact, version-controlled, applied via `aws iam put-role-policy --policy-name miti99bot-deploy --role-name github-deploy-miti99bot --policy-document file://aws/iam-github-deploy-policy.json` (inline policy, not managed — keeps the policy with the role lifecycle). + +Inline vs managed: +- Inline: scoped to role lifecycle, no separate ARN, deleted with role. +- Managed: separate ARN, reusable across roles, has 6,144-char hard limit (same as inline) + 10-policies-per-role limit. +- Choice: **inline.** Single role, no reuse needed, simpler lifecycle. + +## Related Code Files + +- Create: `aws/iam-github-deploy-policy.json` +- Read-only: `plans/260518-1019-iam-least-privilege/phase-02-discover-required-actions.md` (source of truth for actions × resources) + +## Implementation Steps + +1. **Build the JSON from the Phase 2 inventory.** No `/* ... */` placeholders — every Action array fully populated by transcribing Phase 2 (RT-1). Statement skeleton (Sids + ARNs ready; transcribe full action lists from Phase 2): + ```json + { + "Version": "2012-10-17", + "Statement": [ + {"Sid": "Budgets", "Effect": "Allow", "Action": [""], "Resource": "arn:aws:budgets::225603493174:budget/miti99bot*"}, + {"Sid": "CloudFormation", "Effect": "Allow", "Action": [""], "Resource": ["arn:aws:cloudformation:ap-southeast-1:225603493174:stack/miti99bot*/*", "arn:aws:cloudformation:ap-southeast-1:225603493174:changeSet/*/miti99bot*/*"]}, + {"Sid": "CloudFormationGlobalRead", "Effect": "Allow", "Action": ["cloudformation:ListStacks", "cloudformation:ValidateTemplate"], "Resource": "*"}, + {"Sid": "DynamoDB", "Effect": "Allow", "Action": [""], "Resource": "arn:aws:dynamodb:ap-southeast-1:225603493174:table/miti99bot*"}, + {"Sid": "EventBridge", "Effect": "Allow", "Action": [""], "Resource": "arn:aws:scheduler:ap-southeast-1:225603493174:schedule/*/miti99bot*"}, + {"Sid": "IAMRolesScoped", "Effect": "Allow", "Action": ["iam:CreateRole","iam:DeleteRole","iam:GetRole","iam:ListRoles","iam:PutRolePolicy","iam:DeleteRolePolicy","iam:GetRolePolicy","iam:ListRolePolicies","iam:ListAttachedRolePolicies","iam:TagRole","iam:UntagRole","iam:ListRoleTags"], "Resource": "arn:aws:iam::225603493174:role/miti99bot*"}, + {"Sid": "IAMPassRole", "Effect": "Allow", "Action": "iam:PassRole", "Resource": "arn:aws:iam::225603493174:role/miti99bot*", "Condition": {"ForAllValues:StringEquals": {"iam:PassedToService": ["lambda.amazonaws.com", "scheduler.amazonaws.com"]}}}, + {"Sid": "Lambda", "Effect": "Allow", "Action": [""], "Resource": "arn:aws:lambda:ap-southeast-1:225603493174:function:miti99bot*"}, + {"Sid": "LambdaLayerRead","Effect": "Allow", "Action": "lambda:GetLayerVersion", "Resource": "arn:aws:lambda:ap-southeast-1:753240598075:layer:LambdaAdapterLayerArm64:*"}, + {"Sid": "Logs", "Effect": "Allow", "Action": [""], "Resource": ["arn:aws:logs:ap-southeast-1:225603493174:log-group:/aws/lambda/miti99bot*", "arn:aws:logs:ap-southeast-1:225603493174:log-group:/aws/lambda/miti99bot*:*"]}, + {"Sid": "S3SamArtifacts", "Effect": "Allow", "Action": [""], "Resource": ["arn:aws:s3:::aws-sam-cli-managed-default-*", "arn:aws:s3:::aws-sam-cli-managed-default-*/*"]}, + {"Sid": "S3GlobalList", "Effect": "Allow", "Action": "s3:ListAllMyBuckets", "Resource": "*"}, + {"Sid": "SQS", "Effect": "Allow", "Action": [""], "Resource": "arn:aws:sqs:ap-southeast-1:225603493174:miti99bot*"}, + {"Sid": "SSMRead", "Effect": "Allow", "Action": ["ssm:GetParameter","ssm:GetParameters"], "Resource": "arn:aws:ssm:ap-southeast-1:225603493174:parameter/miti99bot/*/*"}, + {"Sid": "STS", "Effect": "Allow", "Action": "sts:GetCallerIdentity", "Resource": "*"} + ] + } + ``` + + **NOT in the policy** (intentional, RT-2 + RT-6): + - `iam:UpdateAssumeRolePolicy` — CFN doesn't need it; including it enables trust-rewrite escalation. + - `iam:AttachRolePolicy` / `iam:DetachRolePolicy` — current SAM transforms use inline `PutRolePolicy` only. Add later with `Condition.ArnEquals.iam:PolicyARN` to a specific allowlist IF a real deploy fails on it; do not add prophylactically. + +2. **Fill action lists** from Phase 2 inventory verbatim. Replace every `` placeholder with the actual action array. Sort alphabetically within each `Action` array. + +3. **Validate JSON syntax:** + ```sh + jq . aws/iam-github-deploy-policy.json > /dev/null && echo OK + ``` + +4. **Verify byte count** under 6,144: + ```sh + wc -c aws/iam-github-deploy-policy.json + ``` + If over: split S3 + Lambda + IAM statements into a second inline policy `miti99bot-deploy-2`. + +5. **IAM Policy Simulator dry-run** (optional but recommended — free). Use AWS Console: IAM → Policies → "Simulate" → paste the JSON → select each Phase-2 action with its target ARN → confirm "Allowed" for every legitimate operation. Note any "Implicit Deny" results and patch. + +6. **Commit** the JSON file. Do NOT yet apply to the role (Phase 4). + +## Todo List + +- [ ] Build JSON skeleton with all statement Sids +- [ ] Fill each Action array from Phase 2 inventory +- [ ] Apply alphabetical sort within Actions for diff readability +- [ ] `jq .` validates +- [ ] Byte count under 6,144 (split if not) +- [ ] (Optional) IAM Policy Simulator dry-run passes for every Phase-2 action +- [ ] Commit `aws/iam-github-deploy-policy.json` +- [ ] Mark phase complete via `ck plan check 3` + +## Success Criteria + +- [ ] `aws/iam-github-deploy-policy.json` exists, valid JSON, under 6,144 bytes (single-policy form). +- [ ] Every action enumerated in Phase 2 appears in exactly one statement. +- [ ] `iam:PassRole` constrained by `iam:PassedToService` to the two services that need it. +- [ ] Every `Resource: "*"` has a justification comment outside the JSON (in Phase 2 inventory). +- [ ] Commit on `main`. Role NOT yet modified (Phase 4 applies). + +## Risk Assessment + +| Risk | Likelihood | Mitigation | +|---|---|---| +| Exceed 6,144 char policy limit | Low | Split into 2 inline policies (miti99bot-deploy-cfn-lambda + miti99bot-deploy-data-iam). | +| Typo in action name | Low | `jq .` catches JSON syntax; IAM Policy Simulator catches unknown action names. | +| Forgot a CFN resource-tagging action | Med | Phase 4 dual-attach validate catches at first deploy; iterate. | + +## Security Considerations + +- No AWS state changes in this phase — policy is on disk only. +- File contains no secrets — safe to commit. +- ARN patterns hardcode account ID `225603493174` and region `ap-southeast-1`; documented as project constants; rotating either invalidates the file but the project's `aws/README.md` already pins them. + +## Next Steps + +Phase 4 (cutover) consumes this file. Do not start Phase 4 until commit lands. diff --git a/plans/260518-1019-iam-least-privilege/phase-04-cutover-validate.md b/plans/260518-1019-iam-least-privilege/phase-04-cutover-validate.md new file mode 100644 index 0000000..b569d6d --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/phase-04-cutover-validate.md @@ -0,0 +1,233 @@ +--- +phase: 4 +title: "Cutover + validate" +status: pending +priority: P1 +effort: "1-2h" +dependencies: [3] +--- + +# Phase 4: Cutover + validate + +## Overview + +Replace the 10× `*FullAccess` managed policies on `github-deploy-miti99bot` with the inline custom policy from Phase 3. Validate via `workflow_dispatch`. This is the highest-risk phase — a wrong policy locks the deploy pipeline. + +**Revised strategy after red-team (RT-3):** two-stage cutover with a dual-attach trial deploy first. + +- **Stage 4a (Trial):** attach the new inline policy ALONGSIDE the existing FullAccess set. Trigger a deploy. The deploy succeeds because IAM evaluates the UNION — but CloudTrail records which policy authorized each action. This surfaces *missing actions* in the new policy WITHOUT the deploy actually failing. We don't claim sufficiency from this; we use it as a syntax-and-coverage smoke test before risking the cutover. +- **Stage 4b (Cutover):** disable the deploy workflow, detach the 10 FullAccess policies, re-enable the workflow, trigger validation deploy. Rollback path: re-attach FullAccess from a committed script. + +The "AccessDenied during ROLLBACK" risk requires `cloudformation:ContinueUpdateRollback` (already in Phase 3 policy per RT-3). Also requires the deploy workflow itself to be DISABLED during 4b so a concurrent push doesn't run mid-cutover (RT-9). + +## Requirements + +**Functional** +- Custom policy attached as inline policy `miti99bot-deploy` on role. +- All 10 `*FullAccess` managed policies detached from role. +- Subsequent `workflow_dispatch` deploy succeeds end-to-end (includes the smoke test + Telegram webhook setup steps already in the workflow). +- On AccessDenied during deploy: instant rollback re-attaches all 10 FullAccess policies; iterate on the inline policy. + +**Non-functional** +- All IAM mutations applied out-of-band via maintainer's local `admin` profile (chicken-and-egg per F1). +- Rollback script prepared and dry-tested BEFORE cutover starts. +- Maintain a < 15-min window where deploys can be re-enabled if cutover fails. + +## Architecture + +``` +Before: During: After: +[10× *FullAccess managed] → [10× FullAccess] → [inline: miti99bot-deploy] + [inline: miti99bot-deploy] + ^^^ never the steady state ^^^ +``` + +The middle "both attached" state exists only as a transient — used for the snapshot moment. We do not validate from there; we validate after the FullAccess detach. + +## Related Code Files + +- Read: `aws/iam-github-deploy-policy.json` (from Phase 3) +- Read: `.github/workflows/deploy.yml` (target for workflow_dispatch) +- No code changes in this phase — only AWS state changes. + +## Implementation Steps + +### Pre-flight gates (RT-4, RT-8) + +0. **Verify admin profile reachable.** `aws/README.md:119` recommends deleting admin keys as hardening posture. Before any cutover: + ```sh + aws sts get-caller-identity --profile admin + ``` + If fails: recreate admin access keys via console (root login → IAM → Users → admin → Security credentials → Create access key). DO NOT proceed until this succeeds. Console-only path is documented but unwieldy for the 11+ IAM calls below. + +1. **Commit the rollback script to the repo** (RT-8) at `aws/iam-rollback-fullaccess.sh`. Per-call retry on throttling + post-loop verification: + ```sh + #!/bin/sh + # Re-attaches the 10 FullAccess managed policies to github-deploy-miti99bot. + # Idempotent: attach-role-policy succeeds even if policy already attached. + ROLE=github-deploy-miti99bot + POLICIES=" + arn:aws:iam::aws:policy/AWSCloudFormationFullAccess + arn:aws:iam::aws:policy/AWSLambda_FullAccess + arn:aws:iam::aws:policy/AmazonDynamoDBFullAccess + arn:aws:iam::aws:policy/AmazonEventBridgeFullAccess + arn:aws:iam::aws:policy/AmazonSQSFullAccess + arn:aws:iam::aws:policy/AmazonSSMFullAccess + arn:aws:iam::aws:policy/CloudWatchLogsFullAccess + arn:aws:iam::aws:policy/AWSBudgetsActionsWithAWSResourceControlAccess + arn:aws:iam::aws:policy/IAMFullAccess + arn:aws:iam::aws:policy/AmazonS3FullAccess + " + for arn in $POLICIES; do + for try in 1 2 3 4 5; do + if aws iam attach-role-policy --role-name "$ROLE" --policy-arn "$arn" --profile admin 2>&1; then + break + fi + echo "retry $try for $arn after throttle…"; sleep $((try * 2)) + done + done + # Verify final state — exit non-zero if anything is missing + ATTACHED=$(aws iam list-attached-role-policies --role-name "$ROLE" --profile admin --query 'AttachedPolicies[].PolicyArn' --output text) + MISSING=0 + for arn in $POLICIES; do + echo "$ATTACHED" | grep -q "$arn" || { echo "MISSING: $arn"; MISSING=1; } + done + [ "$MISSING" = 0 ] && echo "Rollback complete — all 10 FullAccess policies attached." || { echo "Rollback INCOMPLETE — see MISSING lines above. Re-run or attach via console."; exit 1; } + ``` + `chmod +x aws/iam-rollback-fullaccess.sh`. Commit alongside `aws/iam-github-deploy-policy.json`. Any teammate can recover, not only the operator. + +2. **Verify Phase 3 prerequisites:** + - `aws/iam-github-deploy-policy.json` exists on `main`; `jq .` validates. + - No deploy currently in progress (`aws cloudformation describe-stacks --stack-name miti99bot --query 'Stacks[0].StackStatus' --profile admin` returns `*_COMPLETE`). + - Coordinate with collaborators: announce a deploy freeze in the team channel for the cutover window (~30 min). + +### Stage 4a — Dual-attach trial (RT-3) + +3a. **Attach the new inline policy alongside existing FullAccess** (does not detach anything yet): + ```sh + aws iam put-role-policy \ + --role-name github-deploy-miti99bot \ + --policy-name miti99bot-deploy \ + --policy-document file://aws/iam-github-deploy-policy.json \ + --profile admin + ``` + +3b. **Trial deploy:** trigger `workflow_dispatch` on `main`. With BOTH policy sets attached, the deploy MUST succeed (FullAccess covers any gap in the new policy). Confirm success of every workflow step including smoke test + Telegram webhook + Telegram commands. + +3c. **CloudTrail sanity check (optional but recommended):** for the trial-deploy invocation, query CloudTrail for `userIdentity.arn` matching the deploy role and look at the `requestParameters` — events authorized only by the FullAccess managed policies (and not by the inline policy) signal a coverage gap in `miti99bot-deploy`. Patch the inline policy + redo step 3a before proceeding to 4b. (This step uses console UI; CLI access not required.) + +### Stage 4b — Cutover (the actual narrowing) + +4. **Disable the deploy workflow** to prevent concurrent runs (RT-9): + ```sh + gh workflow disable deploy-aws.yml + ``` + Or in GitHub UI: Actions → deploy-aws → "Disable workflow". Re-enabled in step 7. + +5. **Detach the 10 FullAccess policies with retry-on-throttle:** + ```sh + ROLE=github-deploy-miti99bot + for arn in \ + arn:aws:iam::aws:policy/AWSCloudFormationFullAccess \ + arn:aws:iam::aws:policy/AWSLambda_FullAccess \ + arn:aws:iam::aws:policy/AmazonDynamoDBFullAccess \ + arn:aws:iam::aws:policy/AmazonEventBridgeFullAccess \ + arn:aws:iam::aws:policy/AmazonSQSFullAccess \ + arn:aws:iam::aws:policy/AmazonSSMFullAccess \ + arn:aws:iam::aws:policy/CloudWatchLogsFullAccess \ + arn:aws:iam::aws:policy/AWSBudgetsActionsWithAWSResourceControlAccess \ + arn:aws:iam::aws:policy/IAMFullAccess \ + arn:aws:iam::aws:policy/AmazonS3FullAccess; do + for try in 1 2 3 4 5; do + aws iam detach-role-policy --role-name "$ROLE" --policy-arn "$arn" --profile admin && break + sleep $((try * 2)) + done + done + ``` + +6. **Verify role state:** + ```sh + aws iam list-attached-role-policies --role-name github-deploy-miti99bot --profile admin + # Expect: empty AttachedPolicies list + aws iam list-role-policies --role-name github-deploy-miti99bot --profile admin + # Expect: ["miti99bot-deploy"] + ``` + +7. **Re-enable the workflow:** + ```sh + gh workflow enable deploy-aws.yml + ``` + +8. **Trigger validation deploy:** `workflow_dispatch` on `main` from the GitHub Actions UI. This is the FIRST deploy with ONLY the new inline policy. If it succeeds end-to-end → cutover complete. If AccessDenied appears anywhere: + - **Immediately run** `bash aws/iam-rollback-fullaccess.sh --profile admin`. The script handles throttling + verifies all 10 re-attached. + - **If the stack ended in `UPDATE_ROLLBACK_FAILED`** (RT-3): after re-attaching FullAccess, run: + ```sh + aws cloudformation continue-update-rollback --stack-name miti99bot --profile admin + ``` + Wait for `UPDATE_ROLLBACK_COMPLETE`. Then push a fresh build (or `workflow_dispatch`) to re-establish baseline. + - Capture the failing action from CloudTrail. Update `aws/iam-github-deploy-policy.json`, commit, and re-attempt from step 3a (trial again before cutover). + +9. **Smoke test post-deploy:** + - Workflow's built-in steps (`Smoke test`, `Register Telegram webhook`, `Register Telegram command menu`) must all succeed. + - Manually trigger the EventBridge Scheduler "Run now" from the AWS Console to confirm the cron pathway still functions end-to-end with the new role. + +10. **Final state:** role has only `miti99bot-deploy` inline policy. Document the cutover commit hash + timestamp in the "Cutover Record" section below. + +## Cutover Record + +> Filled in during execution. + +- Cutover started: `YYYY-MM-DD HH:MM:SS UTC` +- Cutover finished: `YYYY-MM-DD HH:MM:SS UTC` +- Validating deploy run ID: `` +- Final role policies: `["miti99bot-deploy"]` +- Iterations needed: `` +- Missing actions found mid-cutover: `` + +## Todo List + +- [ ] **Step 0:** `aws sts get-caller-identity --profile admin` succeeds (RT-4) +- [ ] **Step 1:** `aws/iam-rollback-fullaccess.sh` committed with retry+verify logic (RT-8) +- [ ] **Step 2:** Phase 3 prerequisites verified; deploy freeze announced +- [ ] **Stage 4a step 3a:** new inline policy attached alongside FullAccess +- [ ] **Stage 4a step 3b:** trial `workflow_dispatch` deploy succeeds end-to-end +- [ ] **Stage 4a step 3c:** (optional) CloudTrail confirms no actions authorized solely by FullAccess +- [ ] **Stage 4b step 4:** `gh workflow disable deploy-aws.yml` (RT-9) +- [ ] **Stage 4b step 5:** All 10 FullAccess policies detached (retry-on-throttle) +- [ ] **Stage 4b step 6:** Role state verified (only `miti99bot-deploy` policy listed) +- [ ] **Stage 4b step 7:** `gh workflow enable deploy-aws.yml` +- [ ] **Stage 4b step 8:** Validation `workflow_dispatch` deploy succeeds end-to-end with ONLY the new inline policy +- [ ] **Stage 4b step 9:** EventBridge Scheduler "Run now" succeeds +- [ ] Cutover record filled in +- [ ] Mark phase complete via `ck plan check 4` + +## Success Criteria + +- [ ] `aws iam list-attached-role-policies --role-name github-deploy-miti99bot` returns empty. +- [ ] `aws iam list-role-policies --role-name github-deploy-miti99bot` returns `["miti99bot-deploy"]`. +- [ ] **Stage 4a trial deploy** succeeds with both policy sets attached (proves new policy syntax is valid and doesn't break anything). +- [ ] **Stage 4b validation deploy** succeeds with ONLY the new inline policy — zero rollbacks needed during cutover. +- [ ] EventBridge Scheduler manual fire succeeds within 60s of "Run now". +- [ ] Cutover record section above is filled in. +- [ ] `aws/iam-rollback-fullaccess.sh` is committed to the repo (RT-8). + +## Risk Assessment + +| Risk | Likelihood | Severity | Mitigation | +|---|---|---|---| +| Missing IAM action — deploy fails mid-CFN-update | Med | High | Rollback script ready (step 1). Iteration loop documented in step 7. CFN UPDATE_ROLLBACK_COMPLETE state is recoverable — re-attach FullAccess, the next deploy fixes any drift. | +| Missing action AFTER CFN_COMPLETE (e.g. Telegram webhook step uses ssm:GetParameter on a parameter not in scope) | Med | Med | Run rollback. The CFN state is fine; only the post-deploy steps failed. Patch policy and re-run workflow_dispatch — no CFN churn. | +| AccessDenied during ROLLBACK path (worst case) | Low | Critical | If CFN can't roll back due to missing IAM action, run rollback script immediately and let CFN retry with FullAccess. Then patch the missing action and re-run. | +| Maintainer loses local `admin` creds mid-cutover | Low | High | All steps idempotent — re-running from any point produces the same end state. AWS Console works as alternate path for every step. | +| Concurrent push to main during cutover window | Low | Med | `concurrency: deploy-prod` group in workflow prevents overlapping runs. Cutover window <15min; coordinate with anyone else on the repo. | + +## Security Considerations + +- During the cutover window, the deploy role's permissions ARE temporarily over-broad (both old and new attached). This is < 1 minute. +- After cutover: blast radius reduced from "10× FullAccess incl. account takeover via IAMFullAccess" to "stack-scoped CRUD on `miti99bot*` resources only". Paired with Phase 1 narrowing OIDC trust, the combined reduction is what F1+F2 set out to achieve. +- `iam:PassRole` Condition keeps the role from being able to pass arbitrary roles to Lambda/Scheduler — only `miti99bot-*` roles. +- The new inline policy is committed to git (Phase 3) — auditable, drift-detectable by comparing `aws iam get-role-policy` output to `aws/iam-github-deploy-policy.json`. + +## Next Steps + +After this phase: Phase 5 updates `aws/README.md` to reflect the new bootstrap. The plan is complete after Phase 5. diff --git a/plans/260518-1019-iam-least-privilege/phase-05-update-bootstrap-docs.md b/plans/260518-1019-iam-least-privilege/phase-05-update-bootstrap-docs.md new file mode 100644 index 0000000..7dcd488 --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/phase-05-update-bootstrap-docs.md @@ -0,0 +1,148 @@ +--- +phase: 5 +title: "Update bootstrap docs" +status: pending +priority: P2 +effort: "30m" +dependencies: [4] +--- + +# Phase 5: Update bootstrap docs + +## Overview + +Update `aws/README.md` step 4 to reflect the new least-privilege bootstrap: a single `aws iam put-role-policy` call from a committed JSON file instead of attaching 10× `*FullAccess` managed policies. Add a "drift detection" note + rollback procedure. + +## Requirements + +**Functional** +- `aws/README.md` step 4 replaced with new bootstrap flow. +- New step references the committed `aws/iam-github-deploy-policy.json`. +- Section 7 ("Tighten — optional but recommended") updated: bullet 3 (replacing broad managed policies) is now redundant — mark as DONE or remove. +- Add a brief "Updating the deploy policy" subsection explaining: edit the JSON in repo → `aws iam put-role-policy` from `admin` profile (NOT through the workflow) → commit. + +**Non-functional** +- Docs explain WHY (link to F1 finding + this plan dir) so future maintainers don't reattach FullAccess for convenience. +- Keep README concise — defer rationale to plan + audit report. + +## Architecture + +No architecture change. Pure documentation. + +## Related Code Files + +- Modify: `aws/README.md` (steps 4, 7) +- Read-only: `aws/iam-github-deploy-policy.json` (referenced from README) +- Read-only: `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` (link target) + +## Implementation Steps + +1. **Read current `aws/README.md`** to confirm section anchors (especially step 4 and section 7). + +2. **Rewrite step 4** ("Deploy IAM role for GitHub Actions"): + - Keep the `aws iam create-role` call (trust policy unchanged from Phase 1 narrowing). + - Replace the `for arn in ... do attach ... done` loop with: + ```sh + aws iam put-role-policy \ + --role-name github-deploy-miti99bot \ + --policy-name miti99bot-deploy \ + --policy-document file://aws/iam-github-deploy-policy.json \ + --profile admin + ``` + - Add 1-line note: "Scoped to stacks/resources named `miti99bot*`. See [security audit](../plans/reports/code-reviewer-260518-1019-security-aws-infra.md) F1 for rationale and [plan](../plans/260518-1019-iam-least-privilege/) for the cutover record." + +3. **Update section 7** ("Tighten — optional but recommended"): + - Bullet 3 ("Replace the broad managed policies on `github-deploy-miti99bot` with stack-scoped custom policies") is now done — remove or mark `[done in 2026-05]`. + - Bullet 1 (rotate admin keys) and bullet 2 (workflow_dispatch confirmation) remain. + +4. **Add new subsection "Updating the deploy policy"** at the end of section 4: + ```md + ### Updating the deploy policy + + When `template.yaml` adds a new resource type, the deploy role may need new IAM + actions. Workflow: + + 1. Edit `aws/iam-github-deploy-policy.json` — add the action(s) + ARN pattern. + 2. Apply out-of-band from a maintainer's `admin` profile (NOT via the workflow): + ```sh + aws iam put-role-policy --role-name github-deploy-miti99bot \ + --policy-name miti99bot-deploy \ + --policy-document file://aws/iam-github-deploy-policy.json --profile admin + ``` + 3. Commit the JSON. Next deploy uses the new permissions. + + Drift check — structural compare, not byte-diff (RT-12). `aws iam get-role-policy` returns JSON that differs from the local file in key ordering / whitespace but may be semantically identical. Compare normalized: + ```sh + diff <(aws iam get-role-policy --role-name github-deploy-miti99bot \ + --policy-name miti99bot-deploy --profile admin --query PolicyDocument | jq -S .) \ + <(jq -S . aws/iam-github-deploy-policy.json) + ``` + Non-empty output = INVESTIGATE before reapplying. AWS-side may have been intentionally patched during an outage; blindly re-applying overwrites that fix. + ``` + + ### Trust policy invariants (RT-15) + + `aws/iam-github-oidc-trust.json` constrains which GitHub Actions contexts can + assume `github-deploy-miti99bot`. The current allowlist is intentionally + narrow: only pushes to `main` can deploy. + + **To add a new branch / context** (e.g., a future `dev` preview deploy): + + 1. Edit `aws/iam-github-oidc-trust.json` — add the new `sub` claim to the + `StringLike` array. Examples: + - `repo:tiennm99/miti99bot:ref:refs/heads/dev` — pushes to `dev` branch + - `repo:tiennm99/miti99bot:environment:preview` — workflows scoped to a + GitHub Environment named `preview` (requires `permissions: id-token: write`) + 2. Apply out-of-band: + ```sh + aws iam update-assume-role-policy --role-name github-deploy-miti99bot \ + --policy-document file://aws/iam-github-oidc-trust.json --profile admin + ``` + 3. Commit. Test by triggering the new workflow path. + + **Reasons `pull_request` is NOT in the allowlist** (do not re-add without + reviewing): PR-context OIDC tokens are derivable from any contributor's + PR. Granting the deploy role to PRs is equivalent to granting deploy access + to every contributor. Combined with the inline policy's IAM/Lambda/DynamoDB + actions, an attacker-controlled PR could exfiltrate or alter prod state. + +5. **Commit** the README edit + the policy JSON file (if not already committed in Phase 4) on the same branch / PR. + +## Todo List + +- [ ] Read current `aws/README.md` to map anchors +- [ ] Rewrite step 4 with `put-role-policy` flow + link to `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` (RT-5 — file now exists) +- [ ] Add "Updating the deploy policy" subsection with `jq -S` structural-diff drift check (RT-12) +- [ ] Add "Trust policy invariants" subsection documenting how to re-add sub claims safely (RT-15) +- [ ] Update section 7 — mark broad-policy-replacement as done +- [ ] Add "Updating the deploy policy" subsection with drift-check command +- [ ] Commit `aws/README.md` change +- [ ] Mark phase complete via `ck plan check 5` + +## Success Criteria + +- [ ] `aws/README.md` step 4 no longer references `*FullAccess` managed policies. +- [ ] `aws/README.md` references `aws/iam-github-deploy-policy.json` as the canonical bootstrap source. +- [ ] `aws/README.md` links to `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` for F1/F2 rationale. +- [ ] Section 7 no longer lists "tighten policies" as a TODO. +- [ ] New "Updating the deploy policy" subsection includes the `jq -S` structural-diff drift command (not plain `diff`). +- [ ] New "Trust policy invariants" subsection documents the procedure to re-add a `sub` claim and explains why `pull_request` is excluded. + +## Risk Assessment + +| Risk | Likelihood | Mitigation | +|---|---|---| +| README drifts from actual role state | Med | "Updating the deploy policy" subsection includes a drift-check command. Future maintainers run it before assuming the README is accurate. | +| Future maintainer re-attaches FullAccess "just to ship a hotfix" | Med | README explicitly references the security audit finding F1 — explanation of why this is bad. Plan dir `260518-1019-iam-least-privilege/` provides full history. | + +## Security Considerations + +- Docs-only phase; no AWS state changes. +- Preserves the security work done in phases 1-4 by making the new bootstrap discoverable to future maintainers. + +## Next Steps + +Plan complete. After Phase 5 lands: +- `/ck:journal` — write a session journal entry recording the cutover lesson (in particular, the dual-attach-strategy investigation in Phase 4). +- Archive the plan with `/ck:plan archive`. +- Address remaining audit findings (F3, F4, F5-F16) — out of scope here. diff --git a/plans/260518-1019-iam-least-privilege/plan.md b/plans/260518-1019-iam-least-privilege/plan.md new file mode 100644 index 0000000..85ccc8a --- /dev/null +++ b/plans/260518-1019-iam-least-privilege/plan.md @@ -0,0 +1,142 @@ +--- +title: "IAM least-privilege + OIDC trust narrowing (F1, F2)" +description: "Replace 10× *FullAccess managed policies on github-deploy-miti99bot with a single stack-scoped custom inline policy; remove pull_request claim from OIDC trust." +status: pending +priority: P1 +branch: "main" +tags: [security, iam, deploy] +blockedBy: [] +blocks: [] +created: "2026-05-18T09:06:20.846Z" +createdBy: "ck:plan" +source: skill +--- + +# IAM least-privilege + OIDC trust narrowing (F1, F2) + +## Overview + +Two HIGH-severity findings from the 2026-05-18 security audit. Both target the +GitHub Actions OIDC deploy role `github-deploy-miti99bot`. + +Defence-in-depth framing (revised after red-team review): +- F2 today is dormant — no PR-trigger workflow currently has `id-token: write` (verified: `.github/workflows/ci.yml` has `permissions: contents: read` only; `deploy.yml` is the only OIDC consumer and triggers on `push: main` + `workflow_dispatch`). A future workflow addition would make it live. Removing the claim closes the latent path. +- F1 is the bigger lever: combined with the dormant F2 path, the 10× `*FullAccess` set (incl. `IAMFullAccess`) means any future OIDC-loosening + workflow compromise = account takeover. + +- **F2 (trivial, 1-line):** drop `repo:tiennm99/miti99bot:pull_request` (and `:ref:refs/heads/dev` if unused) from the OIDC trust `sub` allowlist. +- **F1 (careful):** replace 10× `*FullAccess` managed policies with one stack-scoped inline custom policy. Must enumerate every IAM action `sam deploy` actually invokes for every CFN resource in `template.yaml` — missing one = pipeline broken on next push. + +## References + +- `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` — finding details (F1, F2) +- `aws/iam-github-oidc-trust.json` — current trust policy +- `aws/README.md` step 4 — current broad-policy provisioning loop (to be replaced) +- `.github/workflows/deploy.yml` — ground truth for what the role needs +- `template.yaml` — every CFN resource sam deploy manages +- `docs/deploy-aws-free-tier-guide.md:11-37` — accepted security trade-off envelope + +## Phases + +| Phase | Name | Status | Risk | +|-------|------|--------|------| +| 1 | [Narrow OIDC trust (F2)](./phase-01-narrow-oidc-trust-f2.md) | Pending | Low — JSON edit + 1 aws-iam call; rollback via `git show HEAD^:aws/iam-github-oidc-trust.json` (RT-8) | +| 2 | [Discover required actions](./phase-02-discover-required-actions.md) | Pending | Low — read-only enumeration; output is a documented action × resource table | +| 3 | [Draft custom policy](./phase-03-draft-custom-policy.md) | Pending | Low — file creation + JSON validation; no AWS calls | +| 4 | [Cutover + validate](./phase-04-cutover-validate.md) | Pending | **High** — wrong policy = pipeline locked. Two-stage (4a dual-attach trial + 4b cutover) with committed rollback script + workflow-disable gate + `ContinueUpdateRollback` recovery (RT-3, RT-8, RT-9). | +| 5 | [Update bootstrap docs](./phase-05-update-bootstrap-docs.md) | Pending | Low — `aws/README.md` only | + +Phase 1 is independent of 2-5 and can land standalone. +Phases 2 → 3 → 4 → 5 are strictly sequential. + +## Constraints (locked from project memory) + +- **Free tier hard:** no Secrets Manager, no KMS CMK, no Config rules, no IAM Access Analyzer (paid features). Use IAM Policy Simulator only (free). +- **Security envelope:** secret-in-logs / secret-in-Input acceptable; designed public surface = Function URL only; documented at `docs/deploy-aws-free-tier-guide.md:11-37`. +- **Bootstrap chicken-and-egg:** F1 + F2 modify the very role the pipeline uses. All IAM mutations must be applied out-of-band (maintainer local creds with the original `admin` profile or AWS Console), NOT via the workflow being modified. + +## Dependencies + +None — both findings are repo-internal. F2 has no upstream / downstream dependency. + +## Red Team Review + +### Session — 2026-05-18 + +**Findings:** 15 of 30 surviving deduplication (10 accepted-applied, 5 cut as duplicate-of-fix or speculative) +**Severity breakdown:** 4 Critical · 8 High · 3 Medium +**Reviewers:** Security Adversary · Failure Mode Analyst · Assumption Destroyer + +| # | Sev | Finding | Disposition | Applied To | +|---|---|---|---|---| +| 1 | CRIT | Phase 2 inventory placeholders + Phase 3 designs against TBD | Accept | Phase 2, Phase 3 (rewrites) | +| 2 | CRIT | `iam:UpdateAssumeRolePolicy` enables trust-rewrite escalation | Accept | Phase 3 (action dropped) | +| 3 | CRIT | UPDATE_ROLLBACK_FAILED unrecoverable; `cloudformation:ContinueUpdateRollback` missing; dual-attach trial rejected too early | Accept | Phase 3 + Phase 4 (re-architect) | +| 4 | CRIT | `--profile admin` everywhere conflicts with `aws/README.md:119` "delete admin keys" | Accept | Phase 1 + 4 + 5 (admin-gate + console fallback) | +| 5 | HIGH | Audit report file `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` did not exist | Accept | Audit file written; references valid | +| 6 | HIGH | `iam:AttachRolePolicy` without `iam:PolicyARN` Condition → admin-policy attach escalation | Accept | Phase 3 (Condition added or action dropped) | +| 7 | HIGH | `iam:PassedToService` `StringEquals` brittle; CFN-internals + future services not covered | Accept | Phase 3 (empirical verification + extensibility note) | +| 8 | HIGH | Rollback script in `/tmp` not repo; `set -e` aborts mid-loop on throttle | Accept | Phase 4 (script committed at `aws/iam-rollback-fullaccess.sh` with retry + verify) | +| 9 | HIGH | `concurrency: deploy-prod` does not gate external IAM mutations | Accept | Phase 4 (workflow-disable during cutover) | +| 10 | HIGH | `*:TagResource` / `*:UntagResource` / `*:ListTagsForResource` not enumerated | Accept | Phase 2 (mandatory categories) + Phase 3 (added) | +| 11 | HIGH | Schedule ARN `schedule/default/miti99bot-*` depends on undocumented default-group folklore | Accept | Phase 3 (`schedule/*/miti99bot-*`) | +| 12 | HIGH | Drift `diff` produces false positives — AWS normalizes JSON server-side | Accept | Phase 5 (`jq -S` structural compare) | +| 13 | MED | SAM bucket prefix is convention not contract; bucket-bootstrap actions missing | Accept | Phase 2 (discovery step) + Phase 3 (broader S3 actions) | +| 14 | MED | Stack ARN hardcodes `miti99bot` literal — future `miti99bot-dev` locked out | Accept | Phase 3 (`miti99bot*` globs) | +| 15 | MED | Dropping `refs/heads/dev` without re-add procedure | Accept | Phase 5 ("Trust policy invariants" section) | + +**Cut as duplicate-of-fix or speculative:** +- Function URL config actions (subsumed by Finding 1 inventory rewrite) +- F2 threat narrative inflation (addressed by Overview reframe above) +- Cross-account layer assumption (low actionable impact) +- `CAPABILITY_NAMED_IAM` future need (speculative forward-look) + +**Reports written:** +- `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` (was missing; produced from inline audit content) + +### Whole-Plan Consistency Sweep — 2026-05-18 + +Re-read `plan.md` + 5 phase files after edits. Reconciled: + +- ✅ ARN patterns match across Phase 2 (discovery) and Phase 3 (policy skeleton): `stack/miti99bot*/*`, `table/miti99bot*`, `function:miti99bot*`, `schedule/*/miti99bot*`, `role/miti99bot*`, `parameter/miti99bot/*/*`. +- ✅ Phase 1 step 0 (admin pre-flight, RT-4) reflected in Todo List + Risk Assessment. +- ✅ Phase 4 two-stage restructure (4a trial + 4b cutover) reflected in Implementation Steps, Todo List, Success Criteria, plan.md risk column. +- ✅ Audit report `plans/reports/code-reviewer-260518-1019-security-aws-infra.md` exists; Phase 5 Success Criteria references it. +- ✅ `aws/iam-rollback-fullaccess.sh` (committed script, RT-8) referenced consistently across Phase 4 step 1, Phase 4 step 8, Success Criteria. +- ✅ `jq -S` structural-diff (RT-12) replaces `diff` in both phase doc and Success Criteria. +- ✅ `iam:UpdateAssumeRolePolicy` (RT-2) and `iam:AttachRolePolicy` (RT-6) marked as deliberately-excluded in Phase 2 + Phase 3, with rationale for re-adding. +- ✅ "F2 dormant today, defence-in-depth fix" framing (RT cuts) in plan.md Overview matches Phase 1 step 6 rewrite. + +**Unresolved contradictions:** none. Plan ready for implementation. + +## Validation Log + +### Session — 2026-05-18 (post-red-team) + +Critical-questions interview after red-team. 4 questions; 4 decisions recorded. + +| # | Question | Decision | Applied To | +|---|---|---|---| +| V-1 | Phase 1 dev-branch: does user push to `dev` from local? | **No, never push to dev** → drop `refs/heads/dev` from OIDC trust as Phase 1 already proposes. Decisive narrowing. | Phase 1 (already reflected) | +| V-2 | Phase 4 admin pre-flight: add multi-item preconditions checklist? | **No** — "make this workflow simple, just work first, then we will solve problems later." Existing single Step 0 (`aws sts get-caller-identity`) is enough. Don't add MFA/network/console-access checklist. | Phase 4 (no change — minimal step 0 retained) | +| V-3 | Phase 4a trial: make CloudTrail coverage check mandatory? | **No** — same simplicity preference as V-2. Stays optional. Dual-attach trial succeeding + cutover deploy succeeding are the two coverage signals. | Phase 4 step 3c (no change — stays "optional but recommended") | +| V-4 | Phase 1: commit `iam-github-oidc-trust.json` edit BEFORE or AFTER `aws iam update-assume-role-policy`? | **Commit FIRST, then apply.** Repo is source of truth; `git show HEAD^:...` always recovers previous state. AWS/repo drift avoided. | Phase 1 step 3 + 4 + 7 (commit step folded into apply step) | + +**Memory captured:** [[simplicity-over-defensive-checklists]] — durable preference for minimum workflow on ops/deploy plans for this project. + +### Whole-Plan Consistency Sweep (validation) + +After V-4 edit: +- ✅ Phase 1 step 3 now includes commit-before-edit guidance + rationale. +- ✅ Phase 1 step 4 includes both `git commit` and `aws iam update-assume-role-policy` together. +- ✅ Phase 1 step 7 redirected to step 4 (no double-commit). +- ✅ Todo List unchanged — "Commit JSON edit" item still maps to step 4 (just consolidated, not removed). +- ✅ Plan.md risk column for Phase 1 still accurate: "rollback via `git show HEAD^:aws/iam-github-oidc-trust.json`" works because the commit is on `HEAD`. + +**Unresolved contradictions:** none. Plan ready for implementation. + +## Non-goals (explicit cuts) + +- F3 (CORS), F4 (root handler audit), F5-F16 — captured in audit report, separate fixes. +- Moving secrets to Secrets Manager — violates free-tier rule. +- Adding `govulncheck` to CI — separate hygiene work. +- Rotating the existing CronSharedSecret — out of scope. diff --git a/plans/reports/code-reviewer-260518-1019-security-aws-infra.md b/plans/reports/code-reviewer-260518-1019-security-aws-infra.md new file mode 100644 index 0000000..ca96f44 --- /dev/null +++ b/plans/reports/code-reviewer-260518-1019-security-aws-infra.md @@ -0,0 +1,99 @@ +--- +type: code-reviewer +scope: infrastructure +date: 2026-05-18 +slug: security-aws-infra +status: complete +related: + - plans/reports/code-reviewer-260518-1019-security-go-app.md + - plans/reports/researcher-260518-1019-security-dependencies.md + - plans/260518-1019-iam-least-privilege/ (consumes findings F1, F2) +--- + +# Security audit — AWS infrastructure (miti99bot) + +Adversarial review of `template.yaml`, `.github/workflows/deploy.yml`, `aws/`, `samconfig.toml`. Findings calibrated to project policy: free-tier hard; accepted trade-offs at `docs/deploy-aws-free-tier-guide.md:11-37` not raised. + +## Findings + +| # | Sev | Location | Title | +|---|---|---|---| +| F1 | **HIGH** | `aws/README.md:67-82` | GHA OIDC role `github-deploy-miti99bot` has 10× `*FullAccess` managed policies incl. `IAMFullAccess` → account takeover from any compromise of trusted ref. | +| F2 | **HIGH** | `aws/iam-github-oidc-trust.json:14-19` | OIDC trust accepts `repo:tiennm99/miti99bot:pull_request` — dormant today (no PR-trigger workflow has `id-token: write`), but any future workflow adding that permission opens a takeover path combined with F1. | +| F3 | **MEDIUM** | `template.yaml:138-144` | Function URL `Cors.AllowOrigins: ["*"]` + `AllowHeaders: ["*"]`. Telegram + Scheduler are server-to-server; CORS is dead code that turns the bot into a browser-side replay target. | +| F4 | **MEDIUM** | `template.yaml:138-140` | `FunctionUrlConfig.AuthType: NONE` requires every Go route to reject unauthenticated callers. Smoke test `curl "$URL/"` at `deploy.yml:73` confirms `/` returns a JSON body to unauthenticated callers — verify body leaks no version/build/env data. Handoff to Go reviewer. | +| F5 | **LOW** | `template.yaml:62-77` | `BotTable` no explicit `SSESpecification`. Default SSE-S3 is implicit since 2018; set explicit for intent + drift resistance. | +| F6 | **MEDIUM** | `template.yaml:163-167` | `CronDLQ` SQS has no resource policy. May hold cron payload with token on failure. Add resource policy: Scheduler `SendMessage` + explicit operator role `ReceiveMessage`; deny non-TLS. | +| F7 | **LOW** | `template.yaml:33-36, 119` | `LambdaAdapterLayerArn` is third-party-published (AWSLabs account `753240598075`). Pinned-version mitigation acknowledged; supply-chain risk documented + accepted. | +| F8 | **LOW** | `template.yaml:91-95` | Log retention 7d acceptable; policy allows secrets in logs. Confirm Go code does not log secret env values (handoff to Go reviewer). | +| F9 | **LOW** | `template.yaml:102-153` | No `ReservedConcurrentExecutions` cap on BotFunction. Add `: 10` as free DoS / cost-amplification guard. | +| F10 | **LOW** | `template.yaml:217-235` | Scheduler `Target.Input` carries the cron secret plain. IAM-gated by `scheduler:GetSchedule`. F1 fix collapses the blast radius (deploy role currently has `AmazonEventBridgeFullAccess`). | +| F11 | **LOW** | `template.yaml:138-140` | `InvokeMode: BUFFERED` — limits responses to 6 MB. Intentional; no issue. | +| F12 | **LOW** | `template.yaml:264-276` | `BotFunctionUrl` in Outputs — discoverable via `cloudformation:DescribeStacks`. URL is public by design; no action. | +| F13 | **INFO** | `template.yaml:62` | `Tracing: Active` enables X-Ray. No public surface; within free tier. | +| F14 | **INFO** | Function URL spec | AWS guarantees HTTPS-only. No HTTP listener exists. | +| F15 | **INFO** | `template.yaml:239-263` | `MonthlyBudget` triggers alerts at 80% / 100% of $1; does not block spend. Working as designed. | +| F16 | **MEDIUM** | `samconfig.toml:16` | Hardcoded `BotOwnerID`, `AdminUserIDs`, `AlertEmail` committed. Move to GHA `--parameter-overrides`. | + +## IAM principal-of-least-privilege summary + +### Lambda execution role (SAM-auto-generated) +- DynamoDBCrudPolicy: table-scoped — OK +- Inline `ssm:GetParameter*` on `parameter/miti99bot/${StackEnv}/*` — OK +- AWSLambdaBasicExecutionRole, AWSXrayWriteOnlyAccess — OK + +### `SchedulerExecutionRole` (in stack) +- Trust on `scheduler.amazonaws.com` (no `aws:SourceAccount` Condition — minor; consider adding for defence in depth) +- `lambda:InvokeFunction` on `BotFunction.Arn` — OK +- `sqs:SendMessage` on `CronDLQ.Arn` — OK + +### `github-deploy-miti99bot` — **OVER-PRIVILEGED (F1)** +10× managed full-access policies. Effective: all stacks, all functions, all tables, **all SSM parameters in account**, all queues, all log groups, **all IAM** (incl. `iam:CreateUser`, `iam:AttachUserPolicy` — account takeover), all S3 buckets, all budgets. + +Blast radius if OIDC trust is loosened or trusted ref compromised: full AWS account takeover. + +### OIDC trust (`aws/iam-github-oidc-trust.json`) +- Federated provider: OK +- `aud`: `sts.amazonaws.com` — OK +- `sub` allowlist: `refs/heads/main`, `refs/heads/dev`, **`pull_request`** — F2 + +## Public-surface inventory + +| Resource | Reachability | Policy alignment | +|---|---|---| +| `BotFunctionUrl` | **Public HTTPS**, `AuthType: NONE`, app-layer token gate | ✅ Designed public surface | +| `BotTable` | IAM-gated | ✅ Not public | +| `BotFunctionLogGroup` | IAM-gated | ✅ Not public | +| `BotFunction` (direct invoke) | IAM-gated | ✅ Only Scheduler role can invoke | +| `CronDLQ` | IAM-gated same-account | ✅ Not public; see F6 | +| `SchedulerExecutionRole` | Assumable by `scheduler.amazonaws.com` only | ✅ | +| `LolscheduleDailyPushSchedule` | IAM-gated control plane | ✅ | +| SSM SecureString params | IAM-gated, AWS-managed KMS | ✅ | +| X-Ray traces | Outbound only | ✅ | + +Only Function URL is internet-public. Matches policy. + +## Verified non-issues (policy-accepted; do not re-flag) + +- Secret in Scheduler `Target.Input` — `docs/deploy-aws-free-tier-guide.md:29` +- Function URL `AuthType: NONE` with app-layer auth — `docs/deploy-aws-free-tier-guide.md:31` +- NoEcho CFN param + CI SSM fetch — `docs/deploy-aws-free-tier-guide.md:30` +- SSM SecureString at cold start (not Secrets Manager) — `docs/deploy-aws-free-tier-guide.md:31` +- PITR disabled on DynamoDB — free-tier rule (`template.yaml:73-74`) +- 7-day log retention — cost/visibility trade-off +- Third-party Lambda layer — pinned-version mitigation, supply-chain accepted + +## Recommended action order + +1. **F2** (1-line) — drop `pull_request` from OIDC trust. +2. **F1** (significant) — replace `*FullAccess` with stack-scoped custom inline policy. See `plans/260518-1019-iam-least-privilege/` for the implementation plan. +3. **F3** — drop CORS. +4. **F6** — DLQ resource policy. +5. **F9** — `ReservedConcurrentExecutions: 10`. +6. **F5** — explicit `SSESpecification`. +7. **F16** — move samconfig hardcodes to overrides. +8. **F4** — Go reviewer confirms root handler safety (separate handoff). + +## Status + +DONE — F1 + F2 captured in `plans/260518-1019-iam-least-privilege/`. F3-F16 await separate work. diff --git a/plans/reports/code-reviewer-260518-1019-security-go-app.md b/plans/reports/code-reviewer-260518-1019-security-go-app.md new file mode 100644 index 0000000..8376877 --- /dev/null +++ b/plans/reports/code-reviewer-260518-1019-security-go-app.md @@ -0,0 +1,101 @@ +--- +type: code-review +date: 2026-05-18 +slug: security-go-app +status: final +scope: Go application code only (cmd/, internal/) — infra excluded +threat-model: Public-internet attacker + Telegram user; no AWS IAM creds, no DNS control +--- + +# Adversarial security review — miti99bot Go app + +## TL;DR + +**No Critical or High findings.** Auth boundaries at `/webhook` and `/cron/{name}` are tight: constant-time secret compare, body bounded only after auth, path regex enforced after auth (no fingerprinting). Telegram update path HTML-escapes all user-controlled strings before ParseMode=HTML. No `exec.Command`, no SSRF (only hardcoded outbound endpoints). One Medium (float input validation in `/trade_topup`) and a handful of Low/Info items below. + +--- + +## Findings + +| # | Severity | File:line | Issue | Fix | +|---|---|---|---|---| +| 1 | Medium | `internal/modules/trading/handlers.go:83-86`, `handlers.go:96`, `portfolio.go:80` | `/trade_topup +Inf` and `/trade_topup NaN` both pass the `amount <= 0` guard (NaN compares false to everything; +Inf is finite-vs-zero passable). `p.AddCurrency("VND", amount)` and `p.Meta.Invested += amount` poison the in-memory portfolio. `SavePortfolio` then errors out at `json.Marshal` (`json: unsupported value: +Inf`), so KV is NOT corrupted — but the user-facing reply is the generic "Could not save portfolio" instead of "amount must be finite". Operator may also incorrectly conclude KV is failing. | Add `math.IsInf(amount,0) \|\| math.IsNaN(amount)` check next to the `amount <= 0` guard in `handleTopup` (line 84). Same defense in `DeductCurrency` / `AddCurrency` would be belt-and-suspenders. | +| 2 | Low | `internal/modules/lolschedule/api_client.go:164` | `io.ReadAll(resp.Body)` is unbounded. A malicious upstream (`esports-api.lolesports.com`) could return a multi-GB body that OOMs the 256 MB Lambda. Threat requires controlling Riot or DNS/TLS MitM (out of stated threat model), but defense-in-depth is cheap. | Wrap with `io.LimitReader(resp.Body, 4<<20)` (4 MiB; typical page is ~50 KB). | +| 3 | Low | `internal/modules/lolschedule/format.go:13-40` | Two-key drift: `leagueOrder` (slice, line 13) and `majorLeagueSlugs` (map, line 29) duplicate the major-league list. Currently in sync; adding a league to one and forgetting the other silently drops it from the filter or from the ordering. Not a security finding per se, but a class of "silent fail" bug that's easy to introduce. | Derive one from the other (e.g. build `majorLeagueSlugs` from `leagueOrder` at init). | +| 4 | Low | `internal/modules/twentyq/parser.go:101-110` (`redactSecret`) | The defense regex `(?i)\bTARGET\b` only catches whole-word ASCII matches. The model could trivially defeat it by hyphenating ("gui-tar"), inserting zero-width chars, or rendering with surrounding non-`\W` Unicode. The prompt also forbids leaking the secret, so this is a defense-in-depth backstop — but its strength is much weaker than the comment suggests. Worst case: a chatty model gives the answer away faster, ruining game UX. No safety impact. | Either widen to handle simple obfuscations (strip `-_ ` from hint before matching) or document the limitation honestly. | +| 5 | Low | `internal/modules/util/info.go:32-44` | `/info` exposes `chat_id`, `thread_id`, `sender_id`. Guarded by `VisibilityProtected`. Correct gating — non-admin sees nothing. BUT: a denied non-admin gets *no reply at all* (silent deny per dispatcher.go:66). This means anyone in the group who knows `/info` exists can confirm the bot is present but learns nothing else. Acceptable; flagging because the visibility-vs-silence pairing is a project invariant worth keeping explicit. | No action. Verify in PR review that any future `/info`-like helper inherits both the Protected visibility AND the silent-deny default. | +| 6 | Info | `internal/server/router.go:66-69` | `cronDisabled=true` short-circuits to 404 *before* method check and *before* path-regex check. An attacker probing `/cron/anything` while the bot is misconfigured (empty secret) gets uniform 404 — no fingerprinting. This is the intended posture per policy "Red flag (d): ships cronDisabled=true". Note: the warn log "CRON_SHARED_SECRET unset" fires once at boot (`main.go:123-125`), so operator visibility relies on log scraping. | No action; verified correct. | +| 7 | Info | `internal/telegram/webhook.go:50-60` | Auth check order: method → header read → constant-time compare → body read+bound. Correct. An unauthenticated caller does NOT cause a 1 MiB read; `MaxBytesReader` is applied only after secret matches. No DoS via large-body floods from non-Telegram callers (modulo the slow-loris angle, mitigated by `ReadHeaderTimeout=10s` and `ReadTimeout=30s` in main.go:137-138). | No action; verified. | +| 8 | Info | `internal/server/router.go:75-79` | Constant-time secret compare for `/cron/{name}`. `subtle.ConstantTimeCompare` returns 0 immediately when lengths differ — *which is a timing oracle for length*. Standard caveat for `subtle.ConstantTimeCompare`. Mitigated because the secret is a fixed-length operator-chosen token; an attacker learning "secret length is 32" gains nothing actionable. | No action. | +| 9 | Info | `internal/modules/dispatcher.go:60-75`, `module.go:26-44` | `auth.Permits` denies silently (no reply) for `Visibility{Protected,Private}` to keep gated command existence private. Correct, but `Install` registers the *handler match* even for Private commands — so the dispatcher *does* run the matcher on every public message looking for `/fortytwo`. That's a tiny CPU cost, not a finding; recording it because a future change to "skip matcher entirely if not permitted" would need to preserve the silent-deny invariant. | No action. | +| 10 | Info | `internal/modules/trading/handlers.go:115`, `handlers.go:172` | `strconv.ParseInt(args[0], 10, 64)` then `qty <= 0` guard. `math.MaxInt64 * price` could overflow `float64` to `+Inf` for very large qty; `DeductCurrency(VND, +Inf)` then returns `(false, balance)` because `balance < +Inf`, so the user just sees "Insufficient VND. Need +Inf". No corruption. | No action. | +| 11 | Info | `internal/modules/loldle/handlers.go:272-279` (`/loldle_setmax`) | `VisibilityPrivate` (owner only). Argument is `Atoi` → range-check `[1, MaxGuessesCap=10]` → KV write. No risk. Verified. | No action. | +| 12 | Info | `internal/modules/lolschedule/subscribers.go:38-53` | Per-subject lock `state.subscribersMu` serializes Get→mutate→Put. Verified at `handlers.go:100-101, 120-121` and `cron.go:164-165` (pruneDeadSubscribers). All three callers acquire the lock. Concurrent `/lolschedule_subscribe` from same chat → second call sees the updated list and replies "Already subscribed". | No action; concurrency verified. | +| 13 | Info | `internal/server/router.go:82-87` | Path regex `^[a-z0-9_]{1,32}$` is enforced *after* successful auth. So an unauthenticated probe of `/cron/foo` returns 401 (same as `/cron/bar`); only an authenticated caller can enumerate valid cron names via 200/404 differences. Replay attacks (re-submitting a captured `X-Cron-Token`) are possible — no nonce/timestamp — but the policy correctly notes this is acceptable given IAM-gated caller. | No action. | +| 14 | Info | `cmd/server/main.go:319-321` | `awsconfig.WithHTTPClient(&http.Client{Timeout: ssmInitTimeout})` sets a 5s timeout on the AWS SDK HTTP client used for SSM GetParameters. Good — guarantees cold start doesn't hang on a stuck SSM endpoint. | No action; verified. | + +--- + +## Red-team verdicts on accepted trade-offs + +For each trade-off documented in `docs/deploy-aws-free-tier-guide.md:23-33`, confirm it does NOT enable an attack vector beyond what's documented: + +| Trade-off | Verdict | Notes | +|---|---|---| +| Secrets in CloudWatch / Lambda env | **Confirmed scoped to IAM** | Code does not log secret *values* anywhere I could find. `webhook.go:74` logs JSON decode errors which don't echo body content. `main.go:343` logs `"count", len(out.Parameters)` not values. No echo of `TelegramBotToken` / `WebhookSecret` / `CronSecret` anywhere via grep. | +| Secret in Scheduler Target.Input | **Confirmed scoped to IAM** | Lambda receives the header from Scheduler invocation; the bot does not re-publish or store the secret. | +| NoEcho CFN param for cron secret | **No app-level impact** | Resolved into `cfg.CronSecret` at boot. Closure-captured. Process restart re-reads. | +| SSM SecureString fetched at cold start | **Confirmed safe** | `resolveSSMSecrets` at `main.go:290-345`: `WithDecryption: true`, 5s timeout, fails fast on `InvalidParameters`. No retry-storm risk. Secret never re-fetched during the warm container's life — fine because Lambda's max lifetime caps exposure. | + +**No new findings escalated from trade-offs.** + +--- + +## Verified non-issues (look suspicious, are fine) + +| Pattern | File:line | Why it's fine | +|---|---|---| +| `apiKey = "0TvQnueqKa5mxJntVWt0w4LpLfEkrV1Ta8rQBb9Z"` hardcoded | `internal/modules/lolschedule/api_client.go:35` | Public web-client key embedded in lolesports.com's own JS bundle. Comment explicitly marks `#nosec G101`. Not a credential. | +| `math/rand` (not `crypto/rand`) for picking targets | `internal/modules/loldle/handlers.go:31`, `wordle/pick_random.go:25`, `loldle/stickers.go:35` | Picks game answers / sticker file_ids — not a security primitive. Goroutine-safe via stdlib's internal mutex. Twentyq uses `crypto/rand` to seed a per-state PCG (`twentyq.go:46-52`) — appropriate stronger choice for the seed. | +| `subtle.ConstantTimeCompare` length-leak | `router.go:76`, `webhook.go:56` | Length of secrets is fixed by operator config; no observable advantage to an attacker. Standard pattern. | +| `cmd/server/main.go:124` warns "cronDisabled" but cron handler still returns 404 (not 503) | `router.go:66-68` | Intended: 404 makes the route indistinguishable from a non-existent endpoint to scanners. Operator must read logs to know cron is disabled. | +| `webhook.go:103-105` suppresses trailing 200 on panic | webhook.go | Correct: a panicked handler may have already written a response; double-WriteHeader emits Go's "superfluous response.WriteHeader" warning. LogRequests middleware's recover-path tags status 500 from its side. | +| `dispatcher.go:60-75` matches every command on every text message | dispatcher.go | Acceptable: `matchCommand` is O(entities in message) which is bounded by Telegram. No regex compilation per call. | +| `firestore_kv.go:52-69` rejects keys with `/`, `.`, `..`, `__*__` | firestore_kv.go | Defense-in-depth: Firestore document-id constraints. Keys are module-constructed (e.g. `"user:%d"` → never user-controlled string portion); defensive check catches future regressions where a module concats user input into a key without sanitization. | +| `chathelper.SubjectFor` returns "" for channels | chathelper.go:26-39 | Correct: channels have no `From` user; modules then reply "Cannot identify chat" rather than scope state under a sentinel. | +| `lolschedule cron.go:127-131` calls `b.SendMessage(... ParseMode: ParseModeHTML, Text: text)` where `text=RenderToday(...)` | cron.go, format.go | `RenderToday` HTML-escapes every interpolated string (team labels, league names, BlockName, day labels). Verified at `format.go:95-99, 197, 203, 219-220, 256-258`. | +| `trongtruonghop` user-arg substitution into `tg://user?id=%d` link | misc.go:117, 134 | `arg` HTML-escaped on line 134; `u.ID` is an `int64` formatted with `%d` (no injection vector). `senderMention` escapes `name`. Username path uses `@%s` — Telegram client validates username chars server-side; even if a maliciously-named user could craft `