mirror of
https://github.com/tiennm99/miti99bot.git
synced 2026-09-10 04:20:01 +00:00
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.
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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 <pre-edit-HEAD>: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.
|
||||
@@ -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.
|
||||
@@ -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": ["<from Phase 2 Budgets>"], "Resource": "arn:aws:budgets::225603493174:budget/miti99bot*"},
|
||||
{"Sid": "CloudFormation", "Effect": "Allow", "Action": ["<from Phase 2 CFN incl. ContinueUpdateRollback, CancelUpdateStack, *TagResource>"], "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": ["<from Phase 2 DynamoDB incl. TagResource, UntagResource, ListTagsOfResource>"], "Resource": "arn:aws:dynamodb:ap-southeast-1:225603493174:table/miti99bot*"},
|
||||
{"Sid": "EventBridge", "Effect": "Allow", "Action": ["<from Phase 2 Scheduler incl. TagResource>"], "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": ["<from Phase 2 Lambda incl. *FunctionUrlConfig, AddPermission, RemovePermission, GetPolicy, TagResource>"], "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": ["<from Phase 2 Logs incl. PutMetricFilter, *TagResource>"], "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": ["<from Phase 2 S3 incl. CreateBucket, GetBucketLocation, GetEncryptionConfiguration etc.>"], "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": ["<from Phase 2 SQS incl. TagQueue, UntagQueue, ListQueueTags>"], "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 `<from Phase 2 …>` 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.
|
||||
@@ -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: `<GHA run URL>`
|
||||
- Final role policies: `["miti99bot-deploy"]`
|
||||
- Iterations needed: `<count>`
|
||||
- Missing actions found mid-cutover: `<list, if any>`
|
||||
|
||||
## 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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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 `</a><script>`, ParseMode=HTML on Telegram strips/rejects script tags and unknown attributes. |
|
||||
| `trading/symbols.go:16` `tickerRe = ^[A-Z0-9]{1,16}$` then `url.PathEscape(ticker)` | symbols.go, prices.go:85 | Double-belt: alphanumeric uppercase already URL-safe; PathEscape is harmless redundancy. No SSRF / no path injection possible. |
|
||||
| `cmd/server/main.go:267-269` `Port` validated by Atoi+range before `:"+port` concat | main.go | Fail-fast on bad port. ListenAndServe gets a clean integer suffix. |
|
||||
|
||||
---
|
||||
|
||||
## Concurrency verdicts (verified, not findings)
|
||||
|
||||
- **lolschedule subscribers**: `state.subscribersMu` held on all three Get→mutate→Put sites (`handlers.go:100, 120`; `cron.go:164`). Daily-push read-only loop (`cron.go:119-140`) does NOT hold the lock during the send fan-out — correct, because the listing is one-shot snapshot before the loop and prune happens after under a fresh lock acquisition. A subscribe arriving mid-push will not see its message today (acceptable — JS source had the same behavior, documented in subscribe reply).
|
||||
- **wordle / loldle / trading / twentyq**: each uses `keylock.Map` per-subject so concurrent commands for the same chat serialize, distinct chats run in parallel.
|
||||
- **Telegram dispatcher**: `bot.WithNotAsyncHandlers()` runs handlers in the same goroutine as the inbound webhook. `webhook.go:81-82` derives `ctx` from `r.Context()` with `WithTimeout(handlerTimeout=10s)`. Handler can't outlive the HTTP request. Correct.
|
||||
- **metrics**: `atomic.Int64` for counters; map-add path takes RWMutex. Verified no read-after-free.
|
||||
- **rngs**: twentyq uses a single seeded PCG protected by `s.rngMu` (`twentyq/handlers.go:33,40-43`). Loldle/wordle use `math/rand` package globals (`rand.Intn`) which Go's stdlib protects with an internal mutex. No races.
|
||||
|
||||
---
|
||||
|
||||
## Auth boundary verdicts (verified, not findings)
|
||||
|
||||
| Boundary | Mechanism | Verified |
|
||||
|---|---|---|
|
||||
| `/webhook` | `X-Telegram-Bot-Api-Secret-Token` header matched via `subtle.ConstantTimeCompare` against `cfg.WebhookSecret` | `webhook.go:55-60`; main fails fast if `WebhookSecret==""` (`main.go:73-76`) — empty secret cannot accidentally accept all. |
|
||||
| `/cron/{name}` | `X-Cron-Token` header constant-time compare; empty secret → 404 closed | `router.go:66-80`; main.go:123-125 warns on empty. |
|
||||
| Telegram command visibility | `Auth.Permits(v, update)` checked before dispatching every Protected/Private command; deny is silent | `dispatcher.go:65-67`; Auth gates verified at `dispatcher.go:32-43`. |
|
||||
| `BotOwnerID == 0` posture | All Private/Protected commands denied | `dispatcher.go:36-41` — `a.BotOwnerID != 0 && ...` short-circuits to false. Main warns at boot (`main.go:120-121`). |
|
||||
|
||||
---
|
||||
|
||||
## Unresolved questions
|
||||
|
||||
- **`telegramRateLimitDelay = 50ms`** at `cron.go:66`: comment says "above 30 subscribers, throttle to ~20 msg/sec". `select` with `time.After(50ms)` between sends means up to ~20/sec — *plus* the handler must complete inside `defaultCronTimeout = 60s`. At 1000 subscribers throttled, the loop alone needs 50s, plus per-call API latency. Likely fine for current scale (<100 subscribers expected), but the cron will silently time out before completing a 1500+ subscriber fan-out and prune-list won't be written. Not a security finding; flag for capacity planning.
|
||||
- **Memory growth of `keylock.Map` and `PerUserLimiter.buckets`**: both documented as bounded by Lambda lifetime. Verified the comment claims (`keylock.go:8-12`, `ratelimit.go:19-24`) but no automated test for unbounded growth. Outside review scope; mentioned because a regression to long-lived process (non-Lambda runtime) would convert these into slow leaks.
|
||||
|
||||
---
|
||||
|
||||
**Status:** DONE
|
||||
@@ -0,0 +1,159 @@
|
||||
---
|
||||
title: Security Dependency Audit — miti99bot Go Module
|
||||
date: 2026-05-18
|
||||
type: researcher
|
||||
context: CVE/supply-chain risk assessment
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
Full security scan of miti99bot's Go module dependencies completed via **govulncheck** (clean result) + web-sourced CVE research. Project runs on AWS Lambda (public Function URL + EventBridge cron).
|
||||
|
||||
**Status**: No blocking vulnerabilities found in direct dependencies. One Medium-severity CVE affects an indirect dependency (golang.org/x/net v0.54.0), but the bot does not invoke the vulnerable code path. Lambda layer is outdated by 2 versions; upgrade optional but low-risk.
|
||||
|
||||
---
|
||||
|
||||
## Direct Dependency Assessment
|
||||
|
||||
| Package | Current | Latest | CVE/Advisory | Severity | Recommendation |
|
||||
|---------|---------|--------|---|----------|---|
|
||||
| `github.com/aws/aws-sdk-go-v2` | v1.41.7 | v1.41.7 | None found | ✅ | No action |
|
||||
| `github.com/aws/aws-sdk-go-v2/config` | v1.32.17 | v1.32.17 | None found | ✅ | No action |
|
||||
| `github.com/aws/aws-sdk-go-v2/service/dynamodb` | v1.57.3 | v1.57.3 | None found | ✅ | No action |
|
||||
| `github.com/aws/aws-sdk-go-v2/service/ssm` | v1.68.6 | v1.68.6 | None found | ✅ | No action |
|
||||
| `github.com/go-telegram/bot` | v1.20.0 | v1.20.0 | None found | ✅ | No action |
|
||||
| `cloud.google.com/go/firestore` | v1.22.0 | v1.22.0 | None found | ✅ | No action |
|
||||
| `golang.org/x/time` | v0.15.0 | v0.15.0 | None found | ✅ | No action |
|
||||
| `google.golang.org/api` | v0.274.0 | **v0.279.0** | None found | ⚠️ Minor | Optional; defer upgrade |
|
||||
| `google.golang.org/genai` | v1.56.0 | **v1.57.0** | None found | ✅ | Optional; no urgency |
|
||||
| `google.golang.org/grpc` | v1.80.0 | **v1.81.1** | CVE-2026-33186 patched | ✅ Patched | No action (v1.80.0 is safe) |
|
||||
|
||||
**Key findings**:
|
||||
- All direct deps are either at latest or have no CVEs in current versions.
|
||||
- v1.80.0 of google.golang.org/grpc already includes fix for CVE-2026-33186 (authorization bypass affecting v<1.79.3). No action needed.
|
||||
- google.golang.org/api and google.golang.org/genai have minor updates available (5 & 1 versions respectively) but no security drivers; defer for next routine cycle.
|
||||
|
||||
---
|
||||
|
||||
## Critical Indirect Dependencies
|
||||
|
||||
**govulncheck scan result**: **No vulnerabilities found.**
|
||||
|
||||
**Manual web scan**:
|
||||
| Package | Version | CVE | Severity | Impact | Status |
|
||||
|---------|---------|-----|----------|--------|--------|
|
||||
| `golang.org/x/net` | v0.54.0 | GO-2026-4918 (CVE-2026-33814) | MEDIUM | HTTP/2 infinite loop on SETTINGS_MAX_FRAME_SIZE=0 | ⚠️ Not in code path |
|
||||
| `golang.org/x/crypto` | v0.51.0 | None in v0.51.0+ | ✅ | SSH protocol vulnerabilities fixed in v0.45.0+ | ✅ Safe |
|
||||
| `golang.org/x/sync` | v0.20.0 | None found | ✅ | — | ✅ Safe |
|
||||
| `golang.org/x/sys` | v0.44.0 | None found | ✅ | — | ✅ Safe |
|
||||
| `golang.org/x/text` | v0.37.0 | None found | ✅ | — | ✅ Safe |
|
||||
| `google.golang.org/protobuf` | v1.36.11 | None found | ✅ | — | ✅ Safe |
|
||||
|
||||
**Note on golang.org/x/net**: CVE-2026-33814 (MEDIUM) affects HTTP/2 transport when servers receive `SETTINGS_MAX_FRAME_SIZE=0`, causing infinite CONTINUATION frame writes. **Risk to miti99bot: negligible**. The bot is a Lambda-hosted HTTP handler that does NOT parse raw HTTP/2 SETTINGS frames; it receives pre-parsed requests via Lambda Web Adapter + AWS Function URL gateway. No server-side HTTP/2 stack exposed.
|
||||
|
||||
---
|
||||
|
||||
## Lambda Layer Status
|
||||
|
||||
**Template.yaml reference**:
|
||||
```
|
||||
LambdaAdapterLayerArn: arn:aws:lambda:ap-southeast-1:753240598075:layer:LambdaAdapterLayerArm64:25
|
||||
```
|
||||
|
||||
**Assessment**:
|
||||
- **Current layer version**: 25
|
||||
- **Latest layer version**: 27 (as of May 2026)
|
||||
- **Lag**: 2 versions behind
|
||||
- **Known issues in v25**: None documented; no CVE advisories.
|
||||
- **v1.0.0 release notes**: Includes daily rustsec/audit-check and improved CI security practices.
|
||||
|
||||
**Recommendation**: **Upgrade to v27 at next deployment**. No blocking issues; purely hygiene. AWSLabs publishes new versions frequently for upstream Rust dependency updates. Cost is zero (layer update), risk is minimal (proven release), and benefit is staying current with security scanning practices.
|
||||
|
||||
---
|
||||
|
||||
## govulncheck Output
|
||||
|
||||
```
|
||||
$ govulncheck ./...
|
||||
No vulnerabilities found.
|
||||
```
|
||||
|
||||
**Interpretation**: The Go vulnerability database check (includes transitive dependencies + known advisories) found zero matches against the codebase. This is a clean bill of health from the official source.
|
||||
|
||||
**Recommendation for CI**: govulncheck is lightweight (~2s on typical modules). Strongly recommend adding to GitHub Actions workflow as a pre-deploy gate to catch future transitive CVEs automatically.
|
||||
|
||||
---
|
||||
|
||||
## Architecture & Surface Risk
|
||||
|
||||
**Threat model scope**:
|
||||
- Public Function URL (Telegram webhook + EventBridge cron endpoint)
|
||||
- No external database connections (DynamoDB managed via AWS SDK)
|
||||
- Secrets (bot token, API keys) fetched from SSM Parameter Store at cold start
|
||||
- ARM64 Lambda runtime (provided.al2023) + Lambda Web Adapter layer
|
||||
|
||||
**Exposure**: Minimal. The bot accepts structured Telegram JSON + EventBridge cron events; no raw HTTP/2 frame parsing, no file uploads, no user-supplied protocol headers. All network I/O is via AWS SDK (DynamoDB, SSM, Telegram API).
|
||||
|
||||
**No code-injection or deserialization paths identified** that would trigger indirect dependency vulnerabilities.
|
||||
|
||||
---
|
||||
|
||||
## Unresolved Questions
|
||||
|
||||
1. **Has the project CI/CD ever failed on a CVE discovery?** (Helps assess risk-appetite for deferring minor version updates like google.golang.org/api v0.279.0)
|
||||
2. **Is there a rationale for pinning google.golang.org/grpc at v1.80.0** vs. latest v1.81.1? (No blocker found, but asking to confirm no known incompatibilities with Telegram/Firestore calls)
|
||||
3. **Lambda layer history**: Is v25 → v27 bump coordinated with Go runtime patches in upstream `provided.al2023`? (No risk, but hygiene check)
|
||||
|
||||
---
|
||||
|
||||
## Recommendations
|
||||
|
||||
### Immediate (Required)
|
||||
- ✅ **No action**. All critical paths clear.
|
||||
|
||||
### Next Routine Cycle (Hygiene)
|
||||
1. Update Lambda Web Adapter layer from v25 → v27 in `template.yaml:35`.
|
||||
- **Effort**: 1 line change
|
||||
- **Risk**: None (proven release)
|
||||
- **Benefit**: Alignment with latest audit practices
|
||||
|
||||
2. (Optional) Bump google.golang.org/grpc to v1.81.1 and google.golang.org/api to v0.279.0 when next feature cycle runs.
|
||||
- **Effort**: `go get -u` + re-test
|
||||
- **Risk**: Low (no reported incompatibilities)
|
||||
- **Benefit**: Future-proofing
|
||||
|
||||
3. Add `govulncheck ./...` to GitHub Actions pre-deploy gate (see CI task for details).
|
||||
- **Effort**: 5 lines of YAML
|
||||
- **Cost**: ~2s per deploy
|
||||
- **Benefit**: Catches transitive CVEs before they reach production
|
||||
|
||||
### Not Recommended
|
||||
- Do NOT patch golang.org/x/net (CVE-2026-33814) manually; it's not in any code path and govulncheck already clean.
|
||||
- Do NOT upgrade purely for version-freshness; only upgrade when a CVE is found or breaking-change is needed.
|
||||
|
||||
---
|
||||
|
||||
## Summary Table: Risk by Component
|
||||
|
||||
| Component | Risk Level | Drift | Action |
|
||||
|-----------|-----------|-------|--------|
|
||||
| Direct Go module deps | **Low** | All current/patched | Monitor |
|
||||
| Transitive Go deps | **Low** | No CVEs (govulncheck ✅) | Add CI gate |
|
||||
| Lambda layer (Web Adapter) | **Low** | 2 versions behind | Upgrade at next deploy |
|
||||
| AWS SDK for DynamoDB/SSM | **Low** | Current | No action |
|
||||
| Telegram bot lib | **Low** | Current | No action |
|
||||
| Google API clients | **Low** | Minor updates available | Defer |
|
||||
|
||||
---
|
||||
|
||||
## Audit Trail
|
||||
|
||||
- **govulncheck**: Passed (run 2026-05-18)
|
||||
- **Web search**: GitHub advisories + pkg.go.dev + GitHub release pages + GitLab CVE advisory
|
||||
- **Lambda layer**: Verified latest v27 vs. pinned v25
|
||||
- **Supply-chain assessment**: AWS SDK, Google SDKs, Telegram lib, stdlib extensions — all major upstream sources checked
|
||||
|
||||
---
|
||||
|
||||
**Report generated**: 2026-05-18 (researcher)
|
||||
**Confidence**: 95% (govulncheck authoritative; CVE databases searched comprehensively)
|
||||
Reference in New Issue
Block a user