Skip to content

feat(backup): prove the shared destination refuses the dedicated Wedding credential - #4364

Merged
devantler merged 6 commits into
mainfrom
claude/platform-wedding-backup-denial-3253
Oct 2, 2026
Merged

devantler merged 6 commits into
mainfrom
claude/platform-wedding-backup-denial-3253

Conversation

@devantler

Copy link
Copy Markdown
Contributor

🤖 Generated by the Agentic Engineer

Why

The wedding database now backs up with its own dedicated key, but the old shared backup access can only be switched off once we have actually watched the shared backup store turn that dedicated key away. Until now nothing could check that against the key that is really deployed, so the isolation rested on how the key was set up rather than on what the store enforces.

What

Adds a manually started production check that tries to list, read and write the shared backup store with the dedicated key and passes only when every attempt is explicitly refused for lack of permission. It first proves the key works on its own backups and that the shared target really exists, so a typo or a network fault can never pass as a refusal. The credential-rotation runbook now requires this check for every new wedding backup key.

Part of #3253

👉 After merge/promotion: dispatch "Verify Wedding Backup Denial" on main; its result decides whether shared access can be retired.

…ing credential

Add an operator-dispatched proof that runs in wedding-app with the deployed
credentials: the dedicated credential must reach its own catalogue, the shared
credential must name an object in the shared catalogue, and the shared
destination must refuse the dedicated credential's list, read and write with
AccessDenied. Any accepted access is broken isolation; any other error is
unproven. The rotation runbook requires the proof for every new dedicated token.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A refusal now needs a non-zero exit, an AccessDenied report and no trace: no
listed entry, no local copy, and no probe object when the shared credential
lists the bucket after the write (the probe prefix must be absent before it).
The runtime test runs the pinned mc against a stub that refuses every request
as R2 does and feeds that output through the proof. The pod is created rather
than applied so an earlier pod is never read as this run's result, local runs
get a unique name, and redacted client output reaches the operator with the
bare endpoint host removed too.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The runtime test showed how the pinned client reports a refused access: the
listing carries the AccessDenied code, the copy only mc's own
insufficient-permissions error, and the upload a success record before its
refusal. Replay those records as a fixture so they are checked without the
runtime image, and say in the proof why neither decides the verdict.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 56 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 82f26c7e-06c7-456b-9333-aad0c2c993db

📥 Commits

Reviewing files that changed from the base of the PR and between 84b87e0 and 53e8ade.

📒 Files selected for processing (10)
  • .github/workflows/ci.yaml
  • .github/workflows/verify-wedding-backup-denial.yaml
  • docs/dr/restore-drill.md
  • docs/dr/runbook.md
  • docs/dr/velero-cnpg.md
  • scripts/tests/s3-access-denied-stub/main.go
  • scripts/tests/test-mirror-wedding-backup-runtime.sh
  • scripts/tests/test-verify-wedding-backup-denial.sh
  • scripts/verify-wedding-backup-denial-pod.sh
  • scripts/verify-wedding-backup-denial.sh
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Redact IPv4 addresses from the operator-visible client output, which reaches a
public workflow log. Delete only the objects this run created, so a same-named
pod is refused rather than removed. Name the probe object when the pod does
not finish. The refusing stub records each request, and the runtime test
requires the listing, the object read and the probe PUT to have reached it and
fails if the pinned-client case did not run. Describe the refusal as the client
reports it and the workflow's single probe write as it is.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…real listing

The stub runs as the container user, so its request log must be readable by
the runner that checks it. It now records each full request URI, and the test
requires a list-type=2 listing rather than the bucket-location request every
mc process sends. Redact long hex tokens before IPv4 addresses, keep the probe
prefix identical in the runner and the pod, and cover a pre-existing
ConfigMap, which this run neither uses nor deletes.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…glued addresses

Require the real client's listing request to name the shared catalogue prefix,
so no other listing satisfies the check, and stage it through a variable rather
than a pipe that pipefail could fail spuriously. Redact an IPv4 address that
directly follows a long hex token. Fail the probe-prefix check when neither
script declares one.

Part of #3253

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@devantler devantler left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)

Reviewed commit: 09f6a45d72da3f8ece531a6923f884719b63e0b2

  • CodeRabbit: rate limited on this PR at 2026-10-02T15:15:40Z ("Review rate limited"; its summary states "Next included review available in 56 minutes"). Lane health reads LIMITED until 2026-10-02T16:17:48Z.
  • Codex: usage limit, "You have reached your Codex usage limits for code reviews" (platform#4327, 2026-10-01T18:54:43Z). No retry window; only the maintainer can lift it.
  • Cursor Bugbot: usage limit, "Bugbot couldn't run - usage limit reached" (platform#4327, 2026-10-01T18:56:08Z). No retry window.

Scope. Independent review passes covered the whole diff, and every finding was fixed at the root cause before this head:

  • Full diff at 335f59b0: 2 P1. Verdicts trusted mc's text over what happened, and the classifier was only tested on hand-written output. Fixed: write judged at the destination, read by local copy, exit 0 always granted; pinned mc run against a refusing stub in CI.
  • Full diff at 53e8adef: 0 P0/P1, 6 P2. Fixed: IP redaction, created-object-only cleanup, probe named on timeout, wording, stub request log, required real-mc case.
  • Deltas 53e8adef..4eb2eeaa and 4eb2eeaa..f37da93f: 0 P0/P1. P2s fixed: stub log readable by the runner, listing check requires a real list-type=2 request.
  • f37da93f..09f6a45d72da3f8ece531a6923f884719b63e0b2 (reviewed here): the listing check now also requires the catalogue prefix and no longer pipes under pipefail; an IPv4 address glued to a hex token is redacted; the probe-prefix check fails when empty. Verified against the request lines the stub recorded in CI and against sample redaction inputs.

Evidence from the pinned client (runtime test at f37da93f, real mc against a stub answering every request with R2's 403 AccessDenied): the stub received the catalogue listing (list-type=2&prefix=cnpg%2Fwedding-db%2F), a HEAD for the object and a PUT of the probe key. mc exited 1 for all three, and the proof classified them as refusals. mc reports the copy's refusal without an error code, and prints an upload success record before the refusal; the proof relies on neither.

Safety. With the dedicated credential, the only write is the run-owned probe key. With the shared credential, the proof only lists, plus a non-recursive removal of that key when it was not refused. Operator-visible output is redacted (endpoint host, IPv4, long hex), and only the exact receipt and marker as the final two lines pass. The workflow is dispatch-only, main-only, prod environment, prod-deploy lock, contents: read.

Verdict: no P0/P1 findings

@devantler

Copy link
Copy Markdown
Contributor Author

🤖 Generated by the Agentic Engineer

Readiness at 09f6a45d72da3f8ece531a6923f884719b63e0b2.

  • Tested: CI green at this head (rollup SUCCESS). The denial suite runs twice: on the host, and inside the pinned mc and BusyBox images, where the real pinned mc also runs list, read and write against a stub that answers every request with R2's 403 AccessDenied. The stub's request log shows the catalogue listing (list-type=2, catalogue prefix), a HEAD for the object and a PUT of the probe key, and the proof classifies all three refusals as denied. More than 30 mutations of the proof and runner were each caught by the suite.
  • Reviewed: local review round GREEN self@09f6a45d (local-review-verdict.sh). CodeRabbit was rate limited on this PR, and Codex and Bugbot are on usage limits; the review records the evidence for each lane. Four independent review passes found 2 P1 and several P2, all fixed at the root cause; 0 unresolved threads.
  • Tried as a user: the workflow runs only from main, and starting its pod before merge would change production, so the pre-merge trial was read-only. The runner's own preconditions hold on live prod: the Cluster archives through wedding-db-dedicated, both ObjectStores match the reviewed destination, Secret and committed endpoint, and no denial-proof pod or ConfigMap exists.
  • After merge: dispatch Verify Wedding Backup Denial on main with confirm=verify-wedding-backup-denial. Its receipt and DENIAL OBSERVED are the denial gate in Verify the dedicated wedding backup identity, retire shared access, and update the DR runbooks #3253.

@devantler
devantler marked this pull request as ready for review October 2, 2026 16:17
@devantler
devantler added this pull request to the merge queue Oct 2, 2026
Merged via the queue into main with commit 23f3e9b Oct 2, 2026
32 checks passed
@devantler
devantler deleted the claude/platform-wedding-backup-denial-3253 branch October 2, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

1 participant