Skip to content

fix: let container and daemon roles find ACM certificates by their import tag - #972

Merged
alongubkin merged 2 commits into
mainfrom
alon/alien-1372-acm-list-permissions
Oct 11, 2026
Merged

alongubkin merged 2 commits into
mainfrom
alon/alien-1372-acm-list-permissions

Conversation

@alongubkin

@alongubkin alongubkin commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Summary

ImportCertificate without a CertificateArn makes a new certificate on every call and takes no idempotency token (API reference). When ACM accepts an import but the response is lost, or the caller stops right after the call, a retry cannot tell whether a certificate already exists and imports a second one. The first is then owned by nothing.

The way to find it is to tag each import with a token recorded before the call, then list certificates and read their tags (find_imported_certificates_by_tag, added in #935). The worker and Kubernetes public endpoint permission sets already grant this. The container and daemon sets did not, so a controller for those resources cannot look up a lost import.

This PR adds the same two grants to container/management, container/provision and daemon/provision:

  • acm:ListTagsForCertificate in the existing tag-conditioned certificate statement (stack, resource and managed-by tags). A certificate without our tags answers AccessDenied, and the lookup skips it.
  • acm:ListCertificates in a new list-tls-certificates statement on *. AWS evaluates this action only against * (ACM API permissions). It returns certificate ARNs and domain names, nothing else.

Safety

  • Grants only; no code path changes. Shipping this before any controller uses it changes nothing.
  • A role set up before this change still works: a controller that gets AccessDenied on ListCertificates imports as before. Rerunning setup grants it.

Why this over alternatives

  • Deterministic certificate names: ACM certificates have no caller-chosen name or idempotency token.
  • Adopt any certificate with our ownership tags: that could adopt a certificate from another import (for example a previous issuance), which holds a different key. A per-import token tag matches only the import that made it.

History

Same grants and wording as the worker / Kubernetes public endpoint change (#935).

Tests

CI: Rust crates (including the permission-set ABAC test and the IAM dataset check, which validates both action names) and the setup snapshot tests pass. Container and daemon sets are not in the setup snapshots, so no snapshot changes.

Linear: ALIEN-1372

🤖 Generated with Claude Code

@enclave-ai

enclave-ai Bot commented Oct 10, 2026

Copy link
Copy Markdown

Enclave skipped this draft pull request. It will run automatically when you mark the PR as Ready for review.

@alongubkin

Copy link
Copy Markdown
Member Author

@greptileai review

@greptile-apps

greptile-apps Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Critical impact] The PR appears safe to merge; the new grants preserve the existing certificate ownership limits.

Summary

Adds certificate recovery permissions to container management, container provisioning, and daemon provisioning.

  • Container and daemon roles can find imported certificates by their tags.

Reviews (4) · Last reviewed commit: "chore: drop pending snapshot outputs com..." · Reviewed by Greptile

@alongubkin

Copy link
Copy Markdown
Member Author

@greptileai review

alongubkin and others added 2 commits October 10, 2026 22:03
…port tag

ImportCertificate without an ARN creates a new certificate on every call and
takes no idempotency token. To find a certificate whose import response was
lost, a controller lists certificates and reads their tags. Grant
acm:ListTagsForCertificate (tag-conditioned) and acm:ListCertificates (on *,
the only resource AWS accepts for it) to container/management,
container/provision and daemon/provision, as worker sets already have.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alongubkin
alongubkin force-pushed the alon/alien-1372-acm-list-permissions branch from a8d1c59 to 802cc5d Compare October 10, 2026 19:07
@alongubkin

Copy link
Copy Markdown
Member Author

@greptileai review

@alongubkin
alongubkin marked this pull request as ready for review October 10, 2026 20:59
@alongubkin
alongubkin merged commit 502c0ad into main Oct 11, 2026
24 checks passed
@alongubkin
alongubkin deleted the alon/alien-1372-acm-list-permissions branch October 11, 2026 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant