Skip to content

fix(cli): delete the minted GCP key when the credentials upload panics - #2233

Merged
cristim merged 1 commit into
mainfrom
fix/gcp-key-cleanup-hardening
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/gcp-key-cleanup-hardening

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Refs #2117. This PR covers items 1 and 2. Item 3 (the second Ctrl-C during the remote delete) and item 4 (reporting a possibly orphaned key when CreateKey is interrupted) follow in separate PRs.

Summary

  • Item 2: a panic during the credentials upload left the minted key active in GCP. The upload's defer only deleted the key remotely when err != nil. A panic leaves the named err at nil, so the delete was skipped.
    • A done flag is now set just before the success return. The defer treats err == nil && !done as aborted, sets a synthetic error ("upload aborted before completion"), and runs the remote delete and local removal.
    • In that case only, it prints the result to stderr, including the gcloud delete command if the remote delete failed.
    • There is no recover, so the panic still propagates. A normal error return prints nothing extra.
  • Item 1: test gap. RemovalFailureReported now asserts deleteCalled is false. Without it, a change that deleted a successfully stored key when only the local unlink failed passed every test.

Evidence (local, macOS, GOTOOLCHAIN=go1.26.9, gcpKeyProvisioner fakes, no GCP calls)

  • Fail before / pass after: with the fix stashed, TestWriteServiceAccountKey_PanicDeletesRemoteKey fails (deleteCalled false), and _PanicReportsFailedRemoteDelete fails (stderr empty instead of the gcloud command). With the fix, both pass. assert.Panics keeps the test binary alive.
  • Mutation probes:
    • Deleting the key remotely on a local removal failure fails RemovalFailureReported.
    • Dropping done = true fails MintedKeyRemovedAfterSuccess and RemovalFailureReported.
    • Removing the stderr print fails PanicReportsFailedRemoteDelete.
  • Checks:
    • make build, go vet ./... and go test -race ./cmd/... pass.
    • The GCP tests pass: 77 tests, from the reviewer's run.
    • golangci-lint v2.10.1 reports 0 issues.
    • scripts/test-git-secrets-allowlist.sh passes 44/44.
  • Review: the plan for all four items had two adversarial passes by an independent reviewer (sonnet, high). This diff then had two passes by a second non-author reviewer, who found no actionable findings at ceb773fa.

🤖 Generated with Claude Code

The deferred cleanup in uploadGCPCredentialsFile only deleted the remote
key when err was non-nil; during a panic the named err is still nil, so
the local file was removed while the key stayed active in GCP. Track
completion and treat a non-completed nil-error exit as an abort: delete
the key, report the outcome on stderr (the return value is lost) and let
the panic keep propagating.

Also assert that a failed local removal after a successful upload does
not delete the stored key remotely.

Refs #2117

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/xs Trivial / one-liner type/chore Maintenance / non-user-visible triaged Item has been triaged labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 8 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 6497c988-941f-4c5f-a2b1-b19af473942f
📥 Commits

Reviewing files that changed from the base of the PR and between 700ff43 and 73d15bd.

📒 Files selected for processing: 2
  • cmd/configure_gcp.go
  • cmd/configure_gcp_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 73d15bd

Reviewer: fresh Claude agent (Sonnet, high effort), not the author. Coordinator: claude-cc-reviewer.

What was checked:

  • Defer behaviour: the defer (cmd/configure_gcp.go ~195-213) has no recover(), so it runs during panic unwinding and the panic keeps propagating. Goexit is covered too, and the tests confirm propagation with assert.Panics.
  • done flag: set only at the single success return (~225), so every error return leaves it false. The defer is registered only when mintedKeyName != "", so nothing tries to delete a key that was never minted (operator-supplied files pass "").
  • Item 1: RemovalFailureReported now asserts !deleteCalled.
  • Tests: PanicDeletesRemoteKey asserts the delete call, the key name and that the local key file is removed. PanicReportsFailedRemoteDelete asserts the gcloud command on stderr. Fail-before: with the non-test change reverted, both panic tests fail.
  • go vet ./cmd/... is clean, and go test -race -count=1 ./cmd/... passes.

Non-blocking: the remote delete uses a fresh context with a timeout. If deleteKey itself panicked, the original panic would still appear first in the trace.

Gate:

@cristim
cristim merged commit fd0af9f into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the fix/gcp-key-cleanup-hardening branch October 16, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant