feat(cli): add hyperframes lambda policies role/user/validate#912
feat(cli): add hyperframes lambda policies role/user/validate#912jrusso1020 wants to merge 3 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
69ee7ba to
f5512ff
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Solid addition. CI is green. Here's what I checked:
IAM policy correctness
The action list per service is accurate and appropriate for SAM-based Lambda + Step Functions + S3 + CloudWatch deployment. Resource: "*" is the right call for bootstrap — the PR description and docs both explain the tradeoff honestly and tell adopters to narrow it post-first-deploy. No unnecessary IAM superpowers (no iam:*, no AdministratorAccess). iam:PassRole is included, which is required when CloudFormation creates execution roles. The samArtifactBucket group for the aws-sam-cli-* bucket is a thoughtful edge case.
CLI command structure
Follows the existing lambda subcommand pattern exactly — positional target maps to role|user|validate, positional extra maps to the validate path argument, and parsePrincipal matches the existing parseFormat/parseQuality enum-parsing convention. The policies case is dispatched correctly in the switch block. policies is already added to help.ts in the base PR (#910) so it will surface in hyperframes --help.
Security
No hardcoded credentials. No ARN injection vectors — validatePolicy reads a file path and parses JSON; no shell exec involved. actionMatches wildcard expansion is conservative: only *, service:*, and prefix* patterns, which matches the IAM wildcard spec. Deny statements are correctly ignored in the static Allow-set analysis (with a comment explaining why IAM evaluation order is out of scope).
Code quality
Types are clean — separate PolicyStatement and TrustPolicyStatement shapes prevent mixing action-policy and trust-policy fields. allRequiredActions() is a pure function that dedupes and sorts, making it safe to call from both buildPolicyDocument and validatePolicy. Error paths use process.exitCode = 1 (not process.exit(1)) in the JSON branch so the JSON output still flushes before exit — correct pattern. 10 unit tests cover all wildcard cases, the single-Statement edge case, and Deny-statement handling.
One non-blocking note
The validate command's inputPath is sourced from args.extra (the second positional), meaning hyperframes lambda policies validate ./policy.json works, but the UX is a bit implicit — the file path is the second positional, not the first (args.target is "validate"). This is consistent with how lambda sites create <projectDir> works, so it's fine within the existing convention; just worth documenting in --help output if a describe block gets added later.
vanceingalls
left a comment
There was a problem hiding this comment.
Phase-6b IAM bootstrap. The required-action registry, doc shapes, and validatePolicy semantics are mostly right; the test surface is the strongest part of the diff (10 cases, including wildcard expansion + the Deny semantics carve-out). Findings below center on (a) action-set gaps that will surface as failed first deploys, (b) a couple of validate-path correctness edges, and (c) UX/discoverability misses.
Calibrated strengths
validatePolicyseparation: required / granted / missing as a structured result (policies.ts:264-308) is the right shape for both CLI and CI consumers.- Deny-statement scope note is explicit (
policies.ts:289, testpolicies.test.ts:158-176) — "Allow set sufficiency only, evaluation order out of scope" is documented and tested. Good intentional limit. - Trust-policy split into its own type (
policies.ts:44-53) instead of pollutingPolicyStatementwith an optionalPrincipalfield — small but right.
Findings
blocker — Missing required actions for first sam deploy --resolve-s3. The samDeploy driver passes --resolve-s3 (sam.ts samDeploy()), which on first run calls s3:ListAllMyBuckets (a.k.a. s3:ListBuckets) to discover/create the aws-sam-cli-managed-default-* artifact bucket. That action is not in any group in REQUIRED_ACTIONS (policies.ts:59-150). A first-deploy adopter with the generated policy will hit AccessDenied: ListAllMyBuckets before SAM ever uploads the ZIP — which is exactly the failure mode this PR is trying to eliminate. Add s3:ListAllMyBuckets to s3Bucket. Likely also cloudformation:ValidateTemplate (CFN template validation during change-set creation) — verify by smoke-running the generated policy against a fresh account.
important — Wildcard matcher silently misses NotAction / NotResource statements. validatePolicy (policies.ts:288-296) only inspects stmt.Action. A policy authored as { Effect: "Allow", NotAction: ["iam:DeleteUser"] } grants everything except iam:DeleteUser. The validator will report all required actions as missing, producing a false negative. Either (a) detect NotAction and treat as * (true-but-overscoped grant), or (b) raise an "unsupported policy shape" error so users don't trust a misleading green/red. Same applies to NotResource if resource-scoping is added later. Pin this with a test.
important — subcommand positional description in citty schema is stale. packages/cli/src/commands/lambda.ts:62 still reads "deploy | sites | render | progress | destroy" — no policies. This shows up directly in hyperframes lambda --help and in citty-generated usage. Fix the string and add an example in examples (top of file) so policies validate shows up in the auto-rendered help.
important — Duplicate samArtifactBucket group is dead code. REQUIRED_ACTIONS.samArtifactBucket = ["s3:CreateBucket", "s3:GetObject", "s3:ListBucket", "s3:PutObject"] (policies.ts:147-150) is fully subsumed by s3Bucket + s3Object and gets deduped by the Set in allRequiredActions. The comment claims "adopters who set --s3-bucket can drop these" but there is no toggle that lets them drop a subset; the registry is flat and unioned. Either remove the group (it's misleading) or convert the structure into a real per-feature toggle so the comment is true.
important — policies role --principal=lambda is a footgun. The CLI's deploy/destroy/render flow never asks an adopter to assemble a Lambda execution role by hand — the SAM template creates it. Exposing lambda as a --principal value produces a trust-policy for lambda.amazonaws.com plus a full deploy-superset inline policy, which is a confusingly-overscoped Lambda execution role no human should attach. Either drop the lambda option (CFN-only) or, if you want to support standalone runtime-role inspection, scope the inline policy to a runtime-only subset (S3 + Step Functions read/write + lambda:InvokeFunction), not the full deploy set.
important — Wildcard expansion is end-anchored only. actionMatches (policies.ts:311-324) handles *, service:*, and prefix* but not mid-string wildcards (s3:Get*Object) or ? single-char wildcards. IAM supports both. This is acceptable for V1 — but the validator should say so. Right now a policy with s3:Get*Object will be silently reported as not granting s3:GetObject, which a user will read as "missing permission" and overscope to fix. Either implement glob-style matching (cheap with a regex), or print a one-time warning when a non-end-anchored * appears in Action.
important — Missing negative-path tests on the validate path. The test suite has zero coverage for: missing/non-existent file (ENOENT bubble-up unfriendly to users), unparseable JSON (raw SyntaxError), Statement field absent entirely, NotAction / NotResource shapes. These are the inputs CI graders will throw at this — pin them.
nit — --json flag semantics on policies user are surprising. user always emits JSON to stdout; the --json flag only suppresses the trailing # Attach … comment on stderr (policies.ts:215-221). Anyone reading hyperframes lambda policies user --json will assume "without --json I get text format" — but there is no non-JSON format. Rename to --quiet, or in --help, document the flag's actual effect (suppresses the explanatory comment).
nit — validate --json only exits non-zero when missing.length > 0, swallowing other failures. If validatePolicy itself throws (bad JSON, missing file), the unhandled rejection produces a non-zero exit but with a noisy stack trace; CI consumers will scrape stderr. Wrap the throw site in runPolicies with a friendly error envelope ({ ok: false, error: "..." }) when --json is set, so CI integrators have one parse shape.
nit — extra positional is doing double duty for sites create <dir> and policies validate <path>. Reusable today, but the description string (lambda.ts:72) still says "e.g. sites create <projectDir>" — update to mention policies validate <policy.json> too, so a user grepping --help finds the right slot.
nit — buildPolicyDocument() returns the same actions for both role and user. Mostly fine, but it means a CloudFormation service role gets iam:PassRole on * — which lets that role hand any other role to any service. CFN typically needs iam:PassRole only for the roles SAM creates inside this stack. Scoping is per the PR body deferred to the adopter; just ensure the docs callout in cli.mdx mentions this specific Resource-tightening priority (CFN role's iam:PassRole is the highest-risk grant in the bundle).
Cross-reference with hf#907 SAM template (HEAD fff432d): the action set matches what template.yaml provisions for the runtime/state-machine roles (X-Ray put, S3 CRUD, SFN logging, alarms), and the Tags-on-Globals + per-service *:TagResource actions are correctly present in the registry. So the static template surface is covered; the gaps are around the invocation surface (s3:ListAllMyBuckets, possibly cloudformation:ValidateTemplate).
Verdict: REQUEST CHANGES
Reasoning: One blocker (s3:ListAllMyBuckets missing — defeats the stated purpose of the PR on first deploy) plus four importants (NotAction blind-spot, stale subcommand help, dead samArtifactBucket group, --principal=lambda footgun). Each is a small fix; once those land + the validate negative tests pin behavior, this is APPROVE.
Review by Vai
c701224 to
d49753e
Compare
fff432d to
8a006ca
Compare
IAM bootstrap subcommand for the lambda CLI. Closes the "first run hits
'User is not authorized to perform iam:CreateRole'" gap that adopters
otherwise have to figure out by hand.
hyperframes lambda policies user
→ prints an inline-policy doc to attach to the IAM user that runs
the CLI
hyperframes lambda policies role --principal=cloudformation
→ prints { TrustRelationship, InlinePolicy } for a service role
cloudformation can assume
hyperframes lambda policies validate ./infra/policy.json
→ diffs a checked-in policy against the CLI's required action set,
expanding s3:* / s3:Get* / * wildcards, exits non-zero on missing
actions (wire it into CI to catch drift before deploys fail)
The required-actions list is derived from what the SAM template at
examples/aws-lambda/template.yaml needs to create plus what
renderToLambda/getRenderProgress call against S3 + Step Functions at
runtime. Sorted alphabetically per-service so diffs stay readable.
Resource is "*" by design — CloudFormation creates new function /
state-machine / bucket ARNs on every adopter's first deploy. The
generated policy is documented as a starting point; adopters with
stricter postures narrow Resource to the deployed ARNs after the
first successful run.
Tests: 10 unit tests covering the action set, doc shape, trust policy
service principal, and validate() against valid / missing / wildcard /
single-Statement / Deny-statement inputs.
Adds a typed TrustPolicyDocument / TrustPolicyStatement pair so
buildRoleTrustPolicy can return a real type instead of unknown. The
trust-policy shape has a Principal field that the generic
PolicyStatement doesn't model, but it was previously punted via a
return unknown rather than a parallel type.
Test cleanup: drop the `as {...}` casts that the previous return-
unknown signature forced.
d49753e to
30deb7b
Compare
One blocker + four importants from Vai's review:
- REQUIRED_ACTIONS was missing `s3:ListAllMyBuckets` (called by
`sam deploy --resolve-s3` on first run to discover/create the
`aws-sam-cli-managed-default-*` artifact bucket) and
`cloudformation:ValidateTemplate` (CFN template validation
during change-set creation). Without these, a first-deploy
adopter with the generated policy hits AccessDenied on the
very call the PR was meant to unblock. Added both.
- `policies role --principal=lambda` was a footgun — it produced
a `lambda.amazonaws.com` trust paired with the full deploy
superset, i.e. a confusingly-overscoped Lambda execution role
no human should attach (the SAM template creates its own
scoped execution role automatically). Dropped `lambda` as a
principal option; `policies role` now always emits a
CloudFormation service-role doc.
- `validatePolicy` silently misreported NotAction/NotResource
statements (treating them as zero grants), producing false
negatives. Detect both shapes and surface them via a new
`warnings: string[]` field; NotAction statements are skipped
(rather than producing a false negative), NotResource is
treated as full action grant + a warning.
- Mid-string wildcards (`s3:Get*Object`, `?`) silently failed
the matcher. End-anchored wildcards still work; mid-string
patterns now warn so users know the validator can't expand
them.
- Dropped the dead `samArtifactBucket` action group (fully
subsumed by `s3Bucket` + `s3Object`).
- `validate --json` now wraps errors in a friendly envelope
(`{ ok: false, error: "..." }`) so CI consumers have one
parse shape regardless of failure mode.
- lambda.ts subcommand description and examples updated to
include `policies`.
Tests: 5 new negative-path tests cover NotAction warning,
NotResource warning, mid-string wildcard warning, missing file
(ENOENT), malformed JSON (SyntaxError), and absent Statement
field. All 21 policies tests pass.
8a006ca to
5c99590
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Blocker from the previous review is addressed:
REQUIRED_ACTIONS missing s3:ListAllMyBuckets and cloudformation:ValidateTemplate — Fixed. Both actions are present in the REQUIRED_ACTIONS map in policies.ts:
s3:ListAllMyBucketsis in thes3Bucketgroup (verified in diff)cloudformation:ValidateTemplateis in thecloudformationgroup (verified in diff)
The allRequiredActions() test also asserts coverage of key touchpoints, and the validatePolicy tests verify the full required set is used for diffing. Solid test coverage with wildcard expansion, NotAction/NotResource warning paths, and edge cases.
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review at HEAD 5c99590. Flipping CHANGES_REQUESTED → APPROVE — blocker + all 4 importants addressed.
Status of prior findings
- Blocker (missing
s3:ListAllMyBuckets) — ADDRESSED.policies.ts:126lists it;:75addscloudformation:ValidateTemplatetoo.sam deploy --resolve-s3won't hit the first-deploy access-denied anymore. - Important (
validatePolicyignoresNotAction/NotResource) — ADDRESSED.:340-348now emits explicit warnings on each shape rather than silently ignoring; user knows the validator's coverage gap on those statements. - Important (
subcommandhelp description missingpolicies) — ADDRESSED.lambda.ts:67now reads"deploy | sites | render | progress | destroy | policies". - Important (
REQUIRED_ACTIONS.samArtifactBucketdead code) — ADDRESSED. The dead key is removed (grep at HEAD returns no references). - Important (
policies role --principal=lambdafootgun) — ADDRESSED.policies.ts:184-187adds an explicit comment that lambda-execution trust is intentionally out of scope ("the SAM template creates its own scoped execution role, and emitting alambda.amazonaws.comtrust paired with the full deploy-superset inline policy below would…") — the comment naming the failure mode prevents future-reader confusion.
Notes
The CFN-superset Resource: "*" is documented as deliberate (policies.ts:160-167 — adopters can't predict ARNs before first deploy) with an explicit narrow-after-deploy recommendation. Reasonable trade-off for the bootstrap-IAM use case.
Verdict: APPROVE.
Review by Vai (re-review)

What
Adds
hyperframes lambda policies role|user|validateso adopters runninghyperframes lambda deployfor the first time don't have to figure out the IAM policy by hand. Without this, the typical first attempt fails withUser is not authorized to perform iam:CreateRole on resource ...and a 30-minute detour.Why
Per
DISTRIBUTED-RENDERING-PLAN.md§ 11 Phase 6c P1: "IAM bootstrap — we currently assume the user can already deploy a CFN stack with Lambda+SFN+IAM+CW." This PR removes that assumption.validateis the CI-friendly variant: wire it into a pre-deploy check that fails loudly if a checked-in IAM policy stops covering what the CLI needs (e.g. after the SAM template adds a new resource).How
packages/cli/src/commands/lambda/policies.tswith the required-actions registry, the policy-doc builder, andvalidatePolicy(path).policiessubcommand dispatched frompackages/cli/src/commands/lambda.ts.validateexpands the common wildcard shapes (s3:*,s3:Get*,*) when checking whether a checked-in policy covers a required action. IAM evaluation order (Deny over Allow) is out of scope —validateonly confirms the Allow set is sufficient.Notable design call:
Resource: "*"in the generated docs is intentional. CloudFormation creates new ARNs on every adopter's first deploy; scoping the Resource by name would require the adopter to already have deployed, which is exactly what they're trying to do. The CLI's help text + cli.mdx both call out that adopters should narrow Resource to the deployed ARNs after the first successful run.Test plan
validatePolicyagainst valid / missing / wildcard / single-Statement / Deny-statement inputs)docs/packages/cli.mdxStacks on #909 and #910.
🤖 Generated with Claude Code