Skip to content

fix(deploy): a provider's delete masks the secrets the deploy resolved (#2007) - #4862

Merged
go-to-k merged 2 commits into
mainfrom
fix/2007-delete-context-masker
Oct 13, 2026
Merged

go-to-k merged 2 commits into
mainfrom
fix/2007-delete-context-masker

Conversation

@go-to-k

@go-to-k go-to-k commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

Closes #2007

Problem

DeleteContext carried no maskSecrets, so a provider's delete() could not tell that a name or value it printed came from a secret:

  • The S3 Express directory bucket "not empty" refusal rendered a pasteable aws s3 rm s3://<name> --recursive, which the log's printing bag then turned into aws s3 rm s3://*** --recursive: a command that names no bucket. That refusal is reached on cdkd destroy, on a rollback, and on a deploy that drops the bucket from its template (a replacement of a directory bucket is refused up front as stateful, and with --force-stateful-recreation the bucket is emptied instead).
  • A deploy's replacement delete ran with no resolved value bound for the logger, so a provider's own delete lines could print one.
  • An in-process rollback deletes with the in-memory, RESOLVED bag, and its op re-resolution is of the PREVIOUS generation; a masker over only one of them misses a rotated secret.

Fix

  • DeleteContext extends SecretMaskingContext (src/provisioning/region-check.ts; type-only import, no runtime cycle).
  • Deploy engine (deploy-engine/{replacement,update-replace,update-in-place}.ts): each replacement delete (delete-first, create-first's delete of the old copy, --recreate-via-*, the update-not-supported fallback) passes maskSecrets over the bag this deploy resolved plus the resource's derived-name registry (fix(deployment): a physical name derived from a secret is masked only inside the provider operation that derived it #3869, one union pass, so a provider asking "is this name secret-derived?" sees what the logger sink sees) and runs under that bag via withPrintingSecrets, so the logger sink masks every provider's own delete lines, including the many providers that never read the context.
  • Rollback replay: the issue's design question ("which generation's bag wins") is answered with both. createOpMasker(logger, secrets, deployBag) now masks the op's re-resolution (previous generation) and the failed deploy's bag (RollbackExecutorContext.logOnlyNeedlesFor, already plumbed) in ONE union pass (createUnionSecretMasker, so a needle overlap across the two bags leaves no tail). Every replay delete (completed-CREATE, failed-CREATE, reverse-replacement new-first / after-recreate, the delete of what a failed re-create made, re-adopt of a retained old copy) passes that op mask as maskSecrets, and runs under both bags as printing bags (withReplayDeletePrinting). The replay's own failure line and event mask the deploy bag too. Redaction is unchanged: only the op's secrets positions it.
  • cdkd destroy and a template-removal delete (destroy-runner.ts, deploy-engine/delete.ts): both delete from the persisted record, which resolved nothing this run but can still be NAMED from a secret, so each passes a masker over the printing bag that delete already runs under (the record's secret-derived name spellings, fix(deployment): a physical name derived from a secret is masked only inside the provider operation that derived it #3869). Every cdkd delete site now passes one; the custom resource's no-masker refusal arm (the critic's counted EXEMPT row) is reached only by a direct caller, since the context stays optional.
  • Consumers: S3 Express directory bucket refusal (masked prose name; the shared renderer prints no command for a secret-derived name); custom resource (DELETE_PATH_UNMASKED retired for deletePathMasker, composing the NoEcho re-resolution's masker, the wider bag, first and then the caller's; the critic's EXEMPT row now covers only the no-masker arm, the two-arm refusal is kept for that reason, recorded on delete() has no secret masker, and the delete bag CAN carry plaintext on the in-process rollback path #2007; the bare parse catch keeps discarding error.message, now documented as load-bearing for that arm); Cloud Control delete's id scrub (now sees a plaintext leaf as evidence); IAM role / user / group delete sinks (unmaskedDeleteSinks() retired) and the replacement update() forwards its masker into the inner delete of the old role; IAM managed policy (its internal 6th mask parameter folded into DeleteContext.maskSecrets); DynamoDB global table delete waits (replica-gone, table-gone, both index waits, the protection-flip ACTIVE wait); Glue database / table / connection / crawler delete lines and failure wraps; the RDS DB proxy target group deregister and already-gone lines; the SNS subscription replacement now forwards its masker into its inner delete, which masks its lines and failure wrap (the ARN embeds the topic name). Internal deletes forward the operation's masker too: ACM certificate replacement, and Cloud Control's cleanup of a failed create's remnant.
  • Docs: docs/provider-rules.md's three sentences saying DeleteContext carries no masker are corrected.

Verification

  • Unit: tests/unit/deployment/rollback-delete-masker-2007.test.ts (new) covers every replay delete arm, the union pass and by-reference reads; noecho-delete-reresolve-engine-4682.test.ts gains every engine replacement delete plus the template-removal delete's registry-only masker; destroy-secret-name-needles-3869.test.ts pins destroy's masker; provider cases in the S3 Express, custom resource, Cloud Control, IAM, global table, Glue, SNS, ACM and RDS DB proxy suites.
  • Mutation probes (each site reverted alone, the matching suite re-run): every maskSecrets threading, every printing-bag wrap, the deploy-bag arm of createOpMasker in both loops, and each provider consumer went red; restored byte-exact after each.

Real-AWS arm

tests/integration/s3-directory-bucket Phase 5: a directory bucket named from a Secrets Manager secret (<secret>--<az>--x-s3, the only resource under S3DIR_SECRET_NAME) gets an object out of band and is then deleted through three sites in turn: (5a) cdkd destroy --verbose, (5b) cdkd rollback --verbose of a --no-rollback deploy whose RollbackBreaker failed after the bucket's create, (5c) a --verbose deploy that drops the bucket (S3DIR_DROP=true). Each refusal must say the name is secret-derived and print NO aws s3 rm (FAIL otherwise; on main each prints aws s3 rm s3://*** --recursive), and no log may name the secret. Each sub-phase then empties the bucket and finishes its delete; the secret goes through a 0600 file://, one trap cleans bucket, secret and state, and every object version under the stack prefix (state, journal, deployments/) is swept: all on the success path and whenever the trap's destroy succeeded, noncurrent when it failed (the state a recovery needs is kept, as the #2225 fence requires).

The first Phase 5 (a replacement of the bucket) could not reach the refusal and is gone: AWS::S3Express::DirectoryBucket is a stateful type, so its replacement is refused before any create, and --force-stateful-recreation empties it.

Why no in-process rollback arm in secret-derived-immutable-names: its rollback delete lines are already masked on main by the in-process replay's printing bag (replayPrintingBag, #3869), so an assertion there would pass with or without this PR. The in-process side is pinned by unit tests (rollback-delete-masker-2007.test.ts, including a rotated-secret case).

Not in this PR

  • Known limit (display only): a replay delete's maskSecrets is the op mask (the op's and the failed deploy's bags plus the derived-name spellings). A name known ONLY to a printing bag a caller bound around the replay (cdkd destroy's journaled-orphan batch, for a name the journal cannot re-derive, e.g. one generateResourceNameWithFallback rewrote) is not in it, so a provider's "is this name secret-derived?" decision can be narrower than the logger sink. Nothing prints in plaintext: the sink still masks the line. Composing the sink into the masker was tried and withdrawn: it can UNDER-mask when an outer needle is a substring of a derived spelling (the sink cuts it first). The correct fix is one union pass over every bound bag and the op bags under the derived-name masks, which needs a resource-secrets-scope API change.

  • The UPDATE revert's provider masker (rollback-executor.ts and replay-revert.ts) still masks with the op's bag only. It is not a printing-only channel: providers' update() DECIDE with it (unchangedBehindSecretReference and the immutable-name guards treat a value the masker changes as secret-derived), so adding the failed deploy's bag would change refuse/patch decisions on a rollback revert, not just what prints. Its lines are already masked by the in-process replay's printing bag.

@github-actions github-actions Bot added effort:large A new integ fixture must be written, or a behavior change / schema bump needing its own PR severity:medium Undone: a capability is missing but a workaround exists, or it only shows under one condition labels Oct 13, 2026
@go-to-k
go-to-k force-pushed the fix/2007-delete-context-masker branch 2 times, most recently from 9a5eea6 to 5eebe59 Compare October 13, 2026 06:58
@go-to-k
go-to-k force-pushed the fix/2007-delete-context-masker branch from bbc93ce to 594d838 Compare October 13, 2026 08:41
#2007)

DeleteContext now carries maskSecrets: engine replacement deletes, rollback replay deletes, cdkd destroy and template-dropped deletes pass a masker built from the bag each delete already runs under, so a provider can withhold a secret-derived name (the S3 Express "not empty" refusal no longer prints a command naming no bucket).
@go-to-k
go-to-k force-pushed the fix/2007-delete-context-masker branch from 93ce55f to 4f8c8fc Compare October 13, 2026 11:55
@go-to-k
go-to-k merged commit c148ebb into main Oct 13, 2026
14 checks passed
@go-to-k
go-to-k deleted the fix/2007-delete-context-masker branch October 13, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort:large A new integ fixture must be written, or a behavior change / schema bump needing its own PR severity:medium Undone: a capability is missing but a workaround exists, or it only shows under one condition

Projects

None yet

Development

Successfully merging this pull request may close these issues.

delete() has no secret masker, and the delete bag CAN carry plaintext on the in-process rollback path

1 participant