From 3b4c9ce25c5a81e5f6aff231ff7155d08624a05d Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:19:53 +0200 Subject: [PATCH 1/2] gcs: a key missing from keys.list is not evidence to re-mint it First provisioning of a serviceAccount-enabled access minted two keys about a second apart and revoked the first, stranding any consumer that had read the Secret in between (11 of 19 provisionings in one project; a gateway pod served a lapsed token for hours with the access reporting Ready=True). The reconciler's Secret write and status patch both re-enqueue the access, so a second reconcile always follows the first mint within a second. ensureAccessKey then verified the Secret's key against keys.list, and the listing - eventually consistent - had not caught up yet. The code read that as "revoked out of band", minted a replacement, and the reconciler dutifully revoked the key it had just handed out. A revoke is irreversible, so it must rest on positive evidence. The Secret's key is now reused unless it does not parse, names another identity, or the listing shows it expired. Absence from the listing no longer triggers anything. The consequence is that a key deleted server-side is no longer replaced automatically; the rotation runbook becomes "delete the Secret", which mints a fresh key and, through the existing replaced-principal path, revokes the previous one. SPEC, the driver docs and the example README are updated accordingly. The fake IAM gains an "unlisted" set so the lifecycle test covers the lag; it fails on the previous code with the second key minted. Release pin bumped. Co-Authored-By: Claude Fable 5.1 --- SPEC.md | 22 +++-- deploy/kustomize/release/kustomization.yaml | 4 +- examples/gcs/service-account/README.md | 26 ++++-- pkg/drivers/gcs/driver.go | 12 +-- pkg/drivers/gcs/serviceaccount.go | 48 +++++++---- pkg/drivers/gcs/serviceaccount_test.go | 92 +++++++++++++++------ pkg/drivers/registry/registry.go | 8 +- 7 files changed, 148 insertions(+), 64 deletions(-) diff --git a/SPEC.md b/SPEC.md index 8980e95..f9c4f29 100644 --- a/SPEC.md +++ b/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 diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index e643889..9352352 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:0b083f26c3472d42e444a83fdaaf34948f123bec39c8c4271b4caf1275dd17ef +- digest: sha256:b59c48e1f02bda32f76fe49791363f260e8464cdd4ccf1b8d8f47006e1f533cf name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260729T151707Z + newTag: 20260904T051923Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md index 846f185..fe32ece 100644 --- a/examples/gcs/service-account/README.md +++ b/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 \ - --iam-account= +kubectl -n delete secret ``` -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. 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. diff --git a/pkg/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index c80b4cd..694f579 100644 --- a/pkg/drivers/gcs/driver.go +++ b/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 diff --git a/pkg/drivers/gcs/serviceaccount.go b/pkg/drivers/gcs/serviceaccount.go index 99c375d..85323ae 100644 --- a/pkg/drivers/gcs/serviceaccount.go +++ b/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 { diff --git a/pkg/drivers/gcs/serviceaccount_test.go b/pkg/drivers/gcs/serviceaccount_test.go index c4bddf8..273c532 100644 --- a/pkg/drivers/gcs/serviceaccount_test.go +++ b/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"]) + } } } diff --git a/pkg/drivers/registry/registry.go b/pkg/drivers/registry/registry.go index b112134..d2e0987 100644 --- a/pkg/drivers/registry/registry.go +++ b/pkg/drivers/registry/registry.go @@ -155,9 +155,11 @@ type GrantRequest struct { // retrievable at creation (GCS SA keys, HMAC secrets): // GrantAccess runs on every reconcile and its result rewrites // the Secret, so such a driver MUST return the existing data - // unchanged while it still verifies against the backend, and - // mint only when it is absent or invalid (which doubles as - // self-healing after out-of-band revocation). + // unchanged and mint only when it is absent or positively + // invalid. A changed Principal makes the reconciler revoke the + // previous one, which is irreversible - so "not found" from an + // eventually consistent backend listing is not grounds to + // mint: it is what a just-minted credential looks like. ExistingSecretData map[string][]byte } From a94170c5553866ec5b2eed502f96220b6354ff24 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:20:46 +0200 Subject: [PATCH 2/2] bucketyaccess: announce a replaced principal with an event When GrantAccess returns a new principal the reconciler revokes the previous one, which is the right thing for a lost or rotated Secret - and invisible: the access stays Ready=True, the Secret holds a valid key, and nothing in the controller log names the namespace. The stranded gateway pod in ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md was only found through the IAM audit log. A Normal PrincipalReplaced event now names the revoked and the current principal and tells the operator to restart consumers that loaded the Secret once. Steady-state reconciles emit nothing; the test covers both. Release pin bumped. Co-Authored-By: Claude Fable 5.1 --- deploy/kustomize/release/kustomization.yaml | 4 +-- examples/gcs/service-account/README.md | 2 +- pkg/controller/bucketyaccess/reconciler.go | 9 +++++ .../bucketyaccess/reconciler_test.go | 36 +++++++++++++++++-- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index 9352352..6c6dce0 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:b59c48e1f02bda32f76fe49791363f260e8464cdd4ccf1b8d8f47006e1f533cf +- digest: sha256:6cc01b8163ff6dee01768b5b2f02d5af9fffb4156e2f621ed362c86c16e5edab name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260904T051923Z + newTag: 20260904T052038Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md index fe32ece..25978ce 100644 --- a/examples/gcs/service-account/README.md +++ b/examples/gcs/service-account/README.md @@ -71,7 +71,7 @@ kubectl -n delete secret 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. Pods that +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. diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index 190097d..fba0325 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/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 diff --git a/pkg/controller/bucketyaccess/reconciler_test.go b/pkg/controller/bucketyaccess/reconciler_test.go index 940364d..f372e43 100644 --- a/pkg/controller/bucketyaccess/reconciler_test.go +++ b/pkg/controller/bucketyaccess/reconciler_test.go @@ -2,6 +2,7 @@ package bucketyaccess import ( "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -351,6 +352,7 @@ 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() @@ -358,7 +360,7 @@ func TestReplacedPrincipalRevoked(t *testing.T) { 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.