Repository navigation
fix(idle-shutdown): give DescribeInstanceStatus Resource * in wake policy - #138
Conversation
…e policy EC2 Describe* actions do not support resource-level permissions. They match only "Resource": "*". The wake Lambda policy gave ec2:DescribeInstanceStatus only on the instance ARN, so the call in wake-lambda/index.mjs got AccessDenied before StartInstances ran. Move ec2:DescribeInstanceStatus to its own statement with Resource "*". Keep ec2:StartInstances limited to the instance ARN. Add tests/test-idle-wake-policy.sh, which reads the policy from install-idle.sh and checks both statements. Fixes #129 Crew: Drew (Kiro Crew Issue Radar)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e0d7b031a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # - each statement with an ec2:Describe* action has "Resource": "*" | ||
| # - ec2:StartInstances stays limited to the one instance ARN | ||
| # | ||
| # No AWS required. Run standalone or via CI (discovered by pack-tests.yml). |
There was a problem hiding this comment.
Although the new test is discovered when the workflow runs, .github/workflows/pack-tests.yml lines 6-20 do not include bootstraps/** in either path filter. A future PR that changes only bootstraps/optional/idle-shutdown/install-idle.sh therefore will not start this workflow, so the contract test cannot catch the policy regression it was added to prevent; add the relevant bootstrap path to both workflow filters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct: pack-tests.yml does not have bootstraps/** in its path filters, so a change to install-idle.sh alone does not start this test. I did not change the workflow, because changes to CI configuration are a maintainer decision and not part of a fix for #129. In d767a4b I added a note to the test header about this. Recommendation for a maintainer: add bootstraps/** to both the push and pull_request path filters of pack-tests.yml (tests/test-idle-check.sh has the same gap for idle-check.py).
| out(resources == [instance_arn], | ||
| f"ec2:StartInstances is limited to the instance ARN (got {resources})") | ||
|
|
||
| out(describe_seen, "policy allows ec2:DescribeInstanceStatus") |
There was a problem hiding this comment.
This assertion labels any ec2:Describe* action as proof that ec2:DescribeInstanceStatus is allowed because describe_seen is set from the prefix match above. If the required action is later deleted or replaced with another describe action such as ec2:DescribeInstances, the test still passes even though wake-lambda/index.mjs calls DescribeInstanceStatusCommand and wake requests will fail; track exact membership of ec2:DescribeInstanceStatus instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d767a4b. The test now sets the "allows ec2:DescribeInstanceStatus" result only when that exact action is in a statement. I checked it with a mutation: when I change the action to ec2:DescribeInstances, the test fails (exit 1) on that check. It also still fails on main and passes with the fix.
1 participant
Fixes #129
Root cause
EC2
Describe*actions do not support resource-level permissions. They match only"Resource": "*". Inbootstraps/optional/idle-shutdown/install-idle.sh, the wake Lambda inline policy gaveec2:DescribeInstanceStatusonly on the instance ARN. Thus theDescribeInstanceStatusCommandcall inwake-lambda/index.mjs(line 82) gets AccessDenied, and the handler goes to its error path (line 87) beforeStartInstances(line 110). The wake link can fail before it starts the instance.Change
install-idle.sh: moveec2:DescribeInstanceStatusto its own statement with"Resource": "*".ec2:StartInstancesstays limited to the one instance ARN. This is the same pattern that the notify Lambda policy in the same file already uses for its Describe actions.tests/test-idle-wake-policy.sh(new): reads theWAKE_INLINEheredoc frominstall-idle.sh, substitutes the variables, parses the JSON, and checks that eachec2:Describe*statement uses"Resource": "*"and thatec2:StartInstancesis limited to the instance ARN. No AWS is needed.pack-tests.ymlfinds it by its name.Existing installs get the new policy when
install-idle.shruns again (put-role-policyreplaces the inline policy).Verification
CI=true bash tests/test-idle-wake-policy.shonmain(before the fix): exit 1, 5 passed, 1 failed (ec2:DescribeInstanceStatus uses Resource "*").CI=true bash tests/test-idle-check.sh: exit 0.bash -n install-idle.sh: exit 0.tests/.Crew: Drew (Kiro Crew Issue Radar)