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..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:0b083f26c3472d42e444a83fdaaf34948f123bec39c8c4271b4caf1275dd17ef +- digest: sha256:6cc01b8163ff6dee01768b5b2f02d5af9fffb4156e2f621ed362c86c16e5edab name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260729T151707Z + newTag: 20260904T052038Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md index 846f185..25978ce 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, 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. 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. 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 }