Fix double-minted secret at first provision

SPEC.md

@@ -816,12 +816,18 @@ one user-managed key for it — native GCS auth with bucket-scoped
 blast radius, alongside the (still backend-wide) HMAC pair. The
 create-only-retrievable-key problem that deferred per-access
 minting is solved by `GrantRequest.ExistingSecretData`: GrantAccess
-returns the Secret's current key unchanged while it still verifies
-against `keys.list`, and mints only when the key is absent, revoked
-out of band, or expired — which makes server-side key deletion the
-manual rotation runbook. `status.principal` is the key's full
-resource name and `RevokeAccess` deletes it, so BucketyAccess
-deletion performs real revocation for these Secrets.
+returns the Secret's current key unchanged, and mints only when
+the Secret has none or the key is positively unusable (expired
+per `keys.list`, or not this SA's). Absence from `keys.list` is
+deliberately NOT a mint trigger: the listing is eventually
+consistent and misses a key minted a second earlier, which is
+when the follow-up reconcile runs - and a mint revokes the
+replaced key, so it must rest on evidence rather than on a
+missing entry. The manual rotation runbook is therefore to delete
+the Secret: the next reconcile mints a fresh key and revokes the
+previous one. `status.principal` is the key's full resource name
+and `RevokeAccess` deletes it, so BucketyAccess deletion performs
+real revocation for these Secrets.
 
 The `serviceAccounts.project` SHOULD be a dedicated identity
 project, separate from the bucket project: key creation equals
@@ -1186,8 +1192,8 @@ and the corresponding GHA secret.
   machinery is already rotation-shaped, and scheduled key
   rotation (mint new, overlap one period, garbage-collect the
   previous key) is the planned follow-up; until then rotation is
-  operator-driven - delete the key server-side and the next
-  reconcile re-mints (`gcloud iam service-accounts keys delete`).
+  operator-driven - delete the access Secret and the next
+  reconcile mints a fresh key and revokes the previous one.
 - Multi-cluster federation.
 - Admission webhook for cross-resource invariants. Per-resource
   parameter validation (against per-driver schemas) and

deploy/kustomize/release/kustomization.yaml

@@ -7,7 +7,7 @@ kind: Kustomization
 resources:
 - ../base
 images:
-- digest: sha256:0b083f26c3472d42e444a83fdaaf34948f123bec39c8c4271b4caf1275dd17ef
+- digest: sha256:6cc01b8163ff6dee01768b5b2f02d5af9fffb4156e2f621ed362c86c16e5edab
   name: ghcr.io/yolean/buckety-controller
   newName: ghcr.io/yolean/buckety-controller
-  newTag: 20260729T151707Z
+  newTag: 20260904T052038Z

examples/gcs/service-account/README.md

@@ -66,13 +66,23 @@ block this feature by design; the grant fails with an actionable
 ## Rotation (manual, until scheduled rotation lands)
 
 ```
-gcloud iam service-accounts keys delete <serviceAccountKeyId> \
-  --iam-account=<serviceAccountEmail>
+kubectl -n <namespace> delete secret <credentialsSecretName>
 ```
 
-The next reconcile (within the requeue cadence) detects the
-revoked key via `keys.list` and mints a fresh one into the Secret
-in place. Deleting the `BucketyAccess` revokes its key
-(`status.principal` is the key's resource name); deleting the
-`Buckety` with `retentionPolicy=Delete` removes the service
-account with the bucket.
+The controller reconciles the `BucketyAccess` on the Secret's
+deletion: it mints a fresh key into a new Secret and then revokes
+the previous key, recording a `PrincipalReplaced` event. Pods that
+loaded the key once (a mounted file read at boot) keep using the
+revoked one until restarted, so restart them after the new Secret
+appears.
+
+Deleting the key server-side instead (`gcloud iam service-accounts
+keys delete`) does NOT trigger a re-mint: the controller cannot
+tell a deleted key from one that `keys.list` has not caught up
+with yet, and it never replaces a key on missing evidence. Delete
+the Secret afterwards to re-mint.
+
+Deleting the `BucketyAccess` revokes its key (`status.principal`
+is the key's resource name); deleting the `Buckety` with
+`retentionPolicy=Delete` removes the service account with the
+bucket.

pkg/controller/bucketyaccess/reconciler.go

@@ -361,6 +361,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			_ = r.patchStatus(ctx, &access, baseAccess)
 			return ctrl.Result{}, rerr
 		}
+		// A replaced credential is invisible from the resource
+		// otherwise: Ready stays True and the Secret looks fine,
+		// while a consumer that loaded the old key once is now
+		// broken. The event is what tells an operator to restart
+		// such consumers, or that an unrequested rotation happened.
+		if r.Recorder != nil {
+			r.Recorder.Event(&access, corev1.EventTypeNormal, "PrincipalReplaced",
+				fmt.Sprintf("credential re-minted: %q revoked, secret %q now carries %q; restart consumers that loaded the secret once", old, access.Spec.CredentialsSecretName, res.Principal))
+		}
 	}
 	access.Status.Principal = res.Principal
 	access.Status.PrincipalRevocable = res.Revocable

pkg/controller/bucketyaccess/reconciler_test.go

@@ -2,6 +2,7 @@ package bucketyaccess
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	corev1 "k8s.io/api/core/v1"
@@ -351,14 +352,15 @@ func TestReplacedPrincipalRevoked(t *testing.T) {
 		}
 		return bky, access
 	}
+	events := record.NewFakeRecorder(10)
 	run := func(t *testing.T, rec *grantRecorder) (client.Client, *bucketyv1.BucketyAccess, error) {
 		t.Helper()
 		bky, access := newObjs()
 		cl := fake.NewClientBuilder().WithScheme(scheme).
 			WithObjects(bky, access).
 			WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}).
 			Build()
-		r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{
+		r := &Reconciler{Client: cl, Scheme: scheme, Recorder: events, Config: &config.Loaded{
 			Backends: map[string]config.Backend{"be": {Name: "be", Driver: rec}},
 		}}
 		_, err := r.Reconcile(context.Background(), reconcilerRequest("t1", "reader"))
@@ -368,8 +370,30 @@ func TestReplacedPrincipalRevoked(t *testing.T) {
 		}
 		return cl, &got, err
 	}
+	drainEvents := func() []string {
+		var out []string
+		for {
+			select {
+			case e := <-events.Events:
+				out = append(out, e)
+			default:
+				return out
+			}
+		}
+	}
+	hasEvent := func(reason string) bool {
+		for _, e := range drainEvents() {
+			if strings.Contains(e, " "+reason+" ") {
+				return true
+			}
+		}
+		return false
+	}
 
-	// Principal change: old revoked, status advances.
+	// Principal change: old revoked, status advances, and the
+	// replacement is announced - it is otherwise invisible on a
+	// resource that stays Ready=True while consumers holding the
+	// old key are broken.
 	rec := &grantRecorder{
 		data:      map[string][]byte{"bucket": []byte("t1-orders")},
 		principal: "projects/p/serviceAccounts/x/keys/new",
@@ -384,8 +408,11 @@ func TestReplacedPrincipalRevoked(t *testing.T) {
 	if got.Status.Principal != "projects/p/serviceAccounts/x/keys/new" {
 		t.Errorf("principal: %q", got.Status.Principal)
 	}
+	if !hasEvent("PrincipalReplaced") {
+		t.Error("principal replacement emitted no PrincipalReplaced event")
+	}
 
-	// Unchanged principal: no revocation.
+	// Unchanged principal: no revocation, no announcement.
 	rec = &grantRecorder{
 		data:      map[string][]byte{"bucket": []byte("t1-orders")},
 		principal: "projects/p/serviceAccounts/x/keys/old",
@@ -396,6 +423,9 @@ func TestReplacedPrincipalRevoked(t *testing.T) {
 	if len(rec.revoked) != 0 {
 		t.Errorf("steady state revoked: %v", rec.revoked)
 	}
+	if hasEvent("PrincipalReplaced") {
+		t.Error("steady state emitted PrincipalReplaced")
+	}
 
 	// Revocation failure: reconcile errors and status.principal
 	// still names the old key so the retry revokes it again.

pkg/drivers/gcs/driver.go

@@ -36,11 +36,13 @@
 // serviceAccountKeyId alongside the static HMAC pair. Keys are
 // only retrievable at creation, so GrantAccess - which runs on
 // every reconcile and rewrites the Secret from its result - reuses
-// the key found in GrantRequest.ExistingSecretData while it still
-// verifies against keys.list, and mints only when absent, revoked
-// out of band, or expired. Scheduled key rotation is a roadmap
-// item (SPEC §Roadmap); until then rotation is operator-driven:
-// delete the key server-side and the next reconcile re-mints.
+// the key found in GrantRequest.ExistingSecretData, and mints only
+// when the Secret has none or the key is positively unusable
+// (expired per keys.list, or not this SA's); see ensureAccessKey
+// for why absence from keys.list does not count. Scheduled key
+// rotation is a roadmap item (SPEC §Roadmap); until then rotation
+// is operator-driven: delete the Secret and the next reconcile
+// mints a fresh key and revokes the previous one.
 //
 // The serviceAccounts config names the GCP project the SAs live
 // in. A DEDICATED identity project (separate from the bucket

pkg/drivers/gcs/serviceaccount.go

@@ -165,14 +165,27 @@ func isMemberNotPropagated(err error) bool {
 	return gapiCode(err, 400) && strings.Contains(err.Error(), "does not exist")
 }
 
-// ensureAccessKey returns the access's SA key JSON and key id,
-// reusing the key carried in the existing Secret while it still
-// verifies against the backend. The private material is only
-// retrievable at creation, so reuse is what makes GrantAccess
-// idempotent; the keys.list check is what makes it self-healing
-// (out-of-band revocation -> fresh key on the next reconcile,
-// which is also the documented manual rotation runbook until
-// scheduled rotation lands).
+// ensureAccessKey returns the access's SA key JSON and key id.
+// The private material is only retrievable at creation, so the
+// key carried in the existing Secret is reused, and a re-mint
+// happens only on POSITIVE evidence that it is unusable: the JSON
+// does not parse, it names another identity, or keys.list shows
+// it expired (constraints/iam.serviceAccountKeyExpiryHours).
+//
+// Absence from keys.list is NOT such evidence. The listing is
+// eventually consistent, and a key created a second earlier is
+// routinely missing from it - which is exactly when the second
+// reconcile runs, since the Secret write and the status patch of
+// the first one both re-enqueue the access. Reading absence as
+// "revoked out of band" made that second reconcile mint a
+// replacement and revoke the key a consumer had just read from
+// the Secret (11 of 19 first provisionings in one project;
+// ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md).
+// A revoke is irreversible, so it needs evidence, not the lack of
+// it. The cost is that a key deleted server-side is no longer
+// replaced on its own; rotation is triggered by deleting the
+// Secret instead, which mints a fresh key and (in the reconciler)
+// revokes the previous one.
 func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map[string][]byte) ([]byte, string, error) {
 	resource := d.saResource(email)
 	if raw, ok := existing["serviceAccountKey"]; ok {
@@ -185,17 +198,22 @@ func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map
 			if err != nil {
 				return nil, "", fmt.Errorf("gcs: list keys of %q: %w", email, err)
 			}
+			expired := false
 			for _, key := range resp.Keys {
-				if strings.HasSuffix(key.Name, "/keys/"+k.PrivateKeyID) && !keyExpired(key) {
-					return raw, k.PrivateKeyID, nil
+				if strings.HasSuffix(key.Name, "/keys/"+k.PrivateKeyID) {
+					expired = keyExpired(key)
+					break
 				}
 			}
+			if !expired {
+				return raw, k.PrivateKeyID, nil
+			}
 		}
-		// Mismatched email, revoked out of band, or expired
-		// (constraints/iam.serviceAccountKeyExpiryHours): mint
-		// fresh. The one key each access holds means nothing else
-		// needs garbage collection; GCP caps user-managed keys at
-		// 10 per SA, bounding accesses per bucket accordingly.
+		// Unparseable, mismatched email, or expired: mint fresh.
+		// The reconciler revokes the replaced key once the Secret
+		// carries its successor, so each access holds one key; GCP
+		// caps user-managed keys at 10 per SA, bounding accesses
+		// per bucket accordingly.
 	}
 	key, err := d.iamsvc.Projects.ServiceAccounts.Keys.Create(resource, &iam.CreateServiceAccountKeyRequest{}).Context(ctx).Do()
 	if err != nil {

pkg/drivers/gcs/serviceaccount_test.go

@@ -38,6 +38,10 @@ type fakeGCP struct {
 	// bindForbidden with a 403.
 	bindRefusals  int
 	bindForbidden int
+	// unlisted key ids exist (Delete finds them) but are missing
+	// from keys.list: the eventually consistent listing lagging
+	// behind a create.
+	unlisted map[string]bool
 }
 
 type policyBinding struct {
@@ -53,6 +57,7 @@ func newFakeGCP(t *testing.T, project string) (*fakeGCP, *httptest.Server) {
 		keys:       map[string]map[string]*iam.ServiceAccountKey{},
 		policies:   map[string][]*policyBinding{},
 		tombstoned: map[string]bool{},
+		unlisted:   map[string]bool{},
 	}
 	mux := http.NewServeMux()
 
@@ -123,7 +128,10 @@ func newFakeGCP(t *testing.T, project string) (*fakeGCP, *httptest.Server) {
 			return
 		}
 		resp := iam.ListServiceAccountKeysResponse{}
-		for _, k := range f.keys[email] {
+		for id, k := range f.keys[email] {
+			if f.unlisted[id] {
+				continue
+			}
 			resp.Keys = append(resp.Keys, k)
 		}
 		writeJSON(w, resp)
@@ -464,24 +472,60 @@ func TestGrantAccessKeyLifecycle(t *testing.T) {
 		t.Fatalf("keys after steady-state grant: %d", n)
 	}
 
-	// Out-of-band revocation (also the manual rotation runbook):
-	// next grant self-heals with a fresh key.
+	// keys.list lagging behind the create (the second reconcile
+	// after a first mint runs within a second of it): absence from
+	// the listing is not evidence of revocation, so the key is
+	// reused and nothing is minted or replaced. This is the
+	// regression test for the first-provisioning double mint
+	// (ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md).
 	f.mu.Lock()
-	delete(f.keys[email], keyID)
+	f.unlisted[keyID] = true
 	f.mu.Unlock()
 	res3, err := d.GrantAccess(ctx, req)
 	if err != nil {
-		t.Fatalf("post-revocation grant: %v", err)
+		t.Fatalf("grant during listing lag: %v", err)
+	}
+	if res3.Principal != res1.Principal {
+		t.Errorf("listing lag re-minted: principal %q, want %q", res3.Principal, res1.Principal)
+	}
+	if n := len(f.keyIDs(email)); n != 1 {
+		t.Fatalf("keys after listing-lag grant: %d", n)
+	}
+	f.mu.Lock()
+	delete(f.unlisted, keyID)
+	f.mu.Unlock()
+
+	// Deleted server-side: indistinguishable from the lag above,
+	// so the Secret keeps the key. Rotation is a Secret deletion
+	// (next block), not a key deletion.
+	f.mu.Lock()
+	delete(f.keys[email], keyID)
+	f.mu.Unlock()
+	res3, err = d.GrantAccess(ctx, req)
+	if err != nil {
+		t.Fatalf("grant after server-side deletion: %v", err)
+	}
+	if res3.Principal != res1.Principal || len(f.keyIDs(email)) != 0 {
+		t.Errorf("server-side deletion re-minted: principal %q, keys %v", res3.Principal, f.keyIDs(email))
+	}
+
+	// Secret deleted (the rotation runbook): fresh key. The
+	// reconciler revokes the replaced principal once the new
+	// Secret is written (TestReplacedPrincipalRevoked).
+	req.ExistingSecretData = nil
+	res3, err = d.GrantAccess(ctx, req)
+	if err != nil {
+		t.Fatalf("grant after Secret deletion: %v", err)
 	}
-	if string(res3.SecretData["serviceAccountKeyId"]) == keyID {
-		t.Error("revoked key reused")
+	if res3.Principal == res1.Principal {
+		t.Error("Secret deletion did not mint a fresh key")
 	}
 	if n := len(f.keyIDs(email)); n != 1 {
-		t.Fatalf("keys after re-mint: %d", n)
+		t.Fatalf("keys after rotation: %d", n)
 	}
 
 	// Expired key (org-policy iam.serviceAccountKeyExpiryHours):
-	// treated as invalid, re-minted.
+	// positive evidence from the listing, re-minted.
 	req.ExistingSecretData = res3.SecretData
 	f.mu.Lock()
 	f.keys[email][string(res3.SecretData["serviceAccountKeyId"])].ValidBeforeTime = time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)
@@ -495,20 +539,22 @@ func TestGrantAccessKeyLifecycle(t *testing.T) {
 	}
 
 	// A key file for some OTHER identity in the Secret (email
-	// mismatch) is never trusted.
-	req.ExistingSecretData = map[string][]byte{
-		"serviceAccountKey": []byte(`{"type":"service_account","client_email":"other@id-proj.iam.gserviceaccount.com","private_key_id":"keyX"}`),
-	}
-	res5, err := d.GrantAccess(ctx, req)
-	if err != nil {
-		t.Fatalf("mismatched-email grant: %v", err)
-	}
-	var got struct {
-		ClientEmail string `json:"client_email"`
-	}
-	_ = json.Unmarshal(res5.SecretData["serviceAccountKey"], &got)
-	if got.ClientEmail != email {
-		t.Errorf("mismatched-email grant kept foreign key: %q", got.ClientEmail)
+	// mismatch), or one that does not parse, is never trusted.
+	for name, raw := range map[string]string{
+		"foreign email": `{"type":"service_account","client_email":"other@id-proj.iam.gserviceaccount.com","private_key_id":"keyX"}`,
+		"garbage":       `not json`,
+	} {
+		req.ExistingSecretData = map[string][]byte{"serviceAccountKey": []byte(raw)}
+		res5, err := d.GrantAccess(ctx, req)
+		if err != nil {
+			t.Fatalf("%s grant: %v", name, err)
+		}
+		var got struct {
+			ClientEmail string `json:"client_email"`
+		}
+		if json.Unmarshal(res5.SecretData["serviceAccountKey"], &got) != nil || got.ClientEmail != email {
+			t.Errorf("%s grant kept the Secret's key: %s", name, res5.SecretData["serviceAccountKey"])
+		}
 	}
 }