diff --git a/README.md b/README.md index f732e10..cfc6e0c 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ v1alpha1. Three drivers shipped: | --- | --- | --- | | `kadm` | Kafka-protocol brokers (Redpanda, Apache Kafka, Confluent) | Topic create/alter/delete. v1alpha1: no per-consumer SASL/SCRAM scoping. | | `s3` | S3-compatible (VersityGW, MinIO, AWS S3, Cloudflare R2, Hetzner, GCS interop) | Bucket create/delete. v1alpha1: all consumers receive the backend's root keys. | -| `gcs` | Google Cloud Storage via the native JSON API | Bucket create/update/delete with location, uniform bucket-level access, versioning and lifecycle parameters. Access Secrets carry a static HMAC pair (S3-protocol data path); all consumers receive the same pair. | +| `gcs` | Google Cloud Storage via the native JSON API | Bucket create/update/delete with location, uniform bucket-level access, versioning and lifecycle parameters. Access Secrets carry a static HMAC pair (S3-protocol data path); all consumers receive the same pair. Driver 0.2 adds opt-in per-bucket service accounts (`parameters.serviceAccount`): a bucket-scoped GCP SA whose key JSON lands in each access Secret for OAuth2 bearer-token auth (`examples/gcs/service-account/`). | e2e coverage in CI runs against Redpanda (`kadm`), VersityGW + MinIO (`s3`) and fake-gcs-server (`gcs`). The other listed S3 @@ -69,7 +69,8 @@ backends share the same client library and the same e2e shape; if you hit a compatibility issue with one of them, please file an issue. For `gcs`, behaviours the emulator cannot exercise (HMAC auth enforcement, the 90-day window for disabling uniform -bucket-level access) are documented rather than e2e-gated. +bucket-level access, the per-bucket serviceAccounts IAM surface) +are documented rather than e2e-gated. Use `gcs` (not `s3` interop) when the controller should provision GCS buckets: creation needs the project, and location / uniform @@ -192,6 +193,13 @@ backends: # mint out of band with: gcloud storage hmac create accessKeyID: ${GCS_HMAC_ACCESS_ID} secretAccessKey: ${GCS_HMAC_SECRET} + # Optional: per-bucket service accounts (parameters.serviceAccount). + # Use a DEDICATED identity project - key creation equals + # impersonation, so the controller's SA-admin grants must not + # extend to projects with unrelated identities. See + # examples/gcs/service-account/ for required grants. + # serviceAccounts: + # project: my-buckety-identities ``` Access Secrets carry the S3-interop `endpoint` (a bare host - the diff --git a/SPEC.md b/SPEC.md index fb1d078..8980e95 100644 --- a/SPEC.md +++ b/SPEC.md @@ -346,6 +346,33 @@ so `${backend.zone}` must reflect whichever backend a given `Buckety` resolves to. Drivers do not inspect `defaults`; the templating layer above does. +**Parameter templates.** A driver may additionally declare +individual `spec.parameters` keys as template-resolved (the gcs +driver declares `serviceAccount`). Declared keys resolve with a +RESTRICTED grammar: `${name}`, `${namespace}` and `${backend.X}` +only - no `${label[...]}`. The restriction is load-bearing: +`spec.name` may reference mutable labels because its resolution +freezes into `status.backendResourceName` at first reconcile, +but parameters re-resolve on every pass, so every input must be +immutable for the resolved value to be stable. Resolution runs +in admission and in the reconciler on the merged +defaults-under-CR view, which lets the cluster operator write a +convention once as a backend parameter default +(`serviceAccount: ${name}-${namespace}`) with tenants declaring +nothing. A CR overrides a default per key, and for +`serviceAccount` the empty string is the defined per-CR opt-out. +Undeclared keys never resolve; `${...}` in their backend +defaults stays a startup error. + +Be deliberate when ADDING a backend parameter default: defaults +merge into EXISTING resources' effective view on the next +reconcile after the controller rollout, without passing +admission - there is no recreate gate on this route, and a +default that resolves invalid for some existing resource (an +over-long namespace against the SA ID's 30-character cap, say) +freezes that resource's reconcile rather than failing its +creation. Audit the fleet before adding one. + ## Driver versioning Each driver carries a SemVer (`major.minor.patch`) advertised by @@ -751,8 +778,12 @@ data: bucket: # tenant1-orders (resource-type key) project: # the backend's GCP project region: # SigV4 signing region, derived with the endpoint (absent for multi-regions) - accessKeyID: + accessKeyID: # static backend-wide HMAC pair; omitted when parameters.hmac="false" secretAccessKey: + # With parameters.serviceAccount (driver >= 0.2, backend opt-in): + serviceAccountKey: # SA key JSON (client_email, private_key, ...) for OAuth2 bearer-token auth + serviceAccountEmail: # @.iam.gserviceaccount.com + serviceAccountKeyId: # private_key_id, for audit and rotation tooling ``` Unlike the s3 driver, whose `endpoint` passes the configured URL @@ -763,12 +794,74 @@ config can override both fields for emulators. The access keys are the backend's static HMAC pair (minted out of band via `gcloud storage hmac create`, copied identically to -every `BucketyAccess`). Driver-minted per-access credentials are -deliberately deferred to the v1alpha2 scoping design: GrantAccess -runs on every reconcile and its result rewrites the Secret, and a -GCS HMAC secret is only retrievable at creation, so per-access -minting cannot be idempotent until grant-once semantics exist. -Key names stay the same when scoped credentials land. +every `BucketyAccess`). `parameters.hmac="false"` (driver >= 0.2, +mutable, per CR or as a backend parameter default) omits the pair +- typically together with `serviceAccount`, so consumers hold +ONLY the bucket-scoped identity instead of having its +blast-radius win undone by the backend-wide pair riding along; +without `serviceAccount` it yields a coordinates-only Secret for +ambient-credential consumers. The DRIVER default stays +pair-included: it is the incumbent, family-portable contract, and +a minor bump must not remove Secret keys - a backend chooses the +opt-in posture by declaring `hmac: "false"` in its parameter +defaults. + +**Per-bucket service accounts (gcs driver 0.2, opt-in).** A gcs +backend that sets `serviceAccounts.project` in its config lets a +`Buckety` declare `parameters.serviceAccount` (a template-resolved +SA short name, immutable post-create). The driver then maintains a +GCP service account with `roles/storage.objectAdmin` on that +bucket only, and each `BucketyAccess` Secret additionally carries +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. + +The `serviceAccounts.project` SHOULD be a dedicated identity +project, separate from the bucket project: key creation equals +impersonation, and the project boundary is what confines the +controller's `roles/iam.serviceAccountAdmin` + +`roles/iam.serviceAccountKeyAdmin` grants to identities that exist +only to hold buckety-granted bucket bindings (the bucket project +additionally needs `storage.buckets.getIamPolicy/setIamPolicy`). +Ownership is stamped as JSON into each SA's description and +verified before every bind/mint/delete, so a tenant naming a +foreign SA in `parameters.serviceAccount` is refused rather than +handed its keys. (The marker is trusted as written: anyone +holding serviceAccountAdmin on the identity project can rewrite +descriptions, which is one more reason that project must be +dedicated to buckety-minted identities.) Role scoping is still +NOT implemented: all accesses share the bucket-scoped SA +regardless of role, and `ScopingNotImplemented` continues to +surface for non-ReadWrite roles. Scheduled key rotation is +deliberately deferred (see Non-goals); the reuse check plus +one-key-per-access keeps within GCP's 10-user-managed-keys-per-SA +limit, bounding a bucket at ~10 CONCURRENT accesses until +rotation lands - not cumulative lifecycle churn, because +credentials never outlive their access (next paragraph). + +**Retention semantics.** `retentionPolicy=Retain` retains the +backend data unit - the bucket, its IAM policy, and its (by then +keyless) service account - and NEVER credentials: keys are +revoked with each `BucketyAccess` under every policy, replaced +keys are revoked as soon as their successor is written, and the +implicit access is revoked before the `Buckety` itself lets go. +Keeping the SA is deliberate: the retained bucket's policy still +references it (deleting it would leave a dangling +`deleted:serviceAccount:` binding), and recreate-with-adoption - +the flow Retain exists for - finds a marker-matching keyless SA +instead of hitting GCP's 30-day tombstone on the reserved name. +Permanent teardowns that must reclaim SA quota delete the SA out +of band; the ownership marker identifies buckety's. If a leak +ever wedges an SA at the key cap, surplus `USER_MANAGED` keys on +a marker-verified SA are safe to delete server-side and the fleet +re-mints within one reconcile. ## Adoption @@ -836,7 +929,12 @@ deletion more conservative. - Both kinds carry a finalizer `buckety.yolean.se/cleanup`. - `BucketyAccess` deletion blocks on `RevokeAccess` succeeding - (in v1alpha1 the no-op revoke completes immediately). + (in v1alpha1 the no-op revoke completes immediately). When the + backend is missing from config, deletion blocks only for + principals the driver stamped revocable + (`status.principalRevocable`, from `GrantResult.Revocable` - + gcs SA keys); static shared principals release as before, so + backend renames do not wedge access teardown. - `Buckety` deletion blocks on (a) all referencing `BucketyAccess` being gone — controller does NOT cascade-delete them; it surfaces a `BlockedByAccesses` condition with the @@ -1034,7 +1132,7 @@ Required CI matrix for v1alpha1: | --- | --- | | `kadm` | redpanda | | `s3` | versitygw, minio | -| `gcs` | fakegcs (fake-gcs-server; covers the JSON-API control plane — real-GCS-only behaviours like HMAC auth enforcement and the 90-day UBLA disable window are documented, not e2e-gated) | +| `gcs` | fakegcs (fake-gcs-server; covers the JSON-API control plane — real-GCS-only behaviours like HMAC auth enforcement, the 90-day UBLA disable window and the serviceAccounts IAM surface (no iam.googleapis.com or bucket-IAM emulation; unit-tested against an httptest fake instead) are documented, not e2e-gated) | Adding an implementation later (e.g. AWS S3 once the project has credentials and a budget) requires no example or harness changes, @@ -1071,7 +1169,11 @@ and the corresponding GHA secret. - MySQL driver. - Per-consumer credential scoping (SASL/SCRAM for kafka, IAM users for S3). All `BucketyAccess` instances for the same - `Buckety` receive identical credentials. + `Buckety` receive identical credentials. Partial exception + since gcs driver 0.2: opt-in per-BUCKET service accounts give + each access its own key for a bucket-scoped identity (see + Secret output > gcs driver), but role scoping remains + unimplemented. - Cross-namespace `bucketyRef`. - Adopting backing resources that already exist outside Buckety. - Quota enforcement. @@ -1079,7 +1181,13 @@ and the corresponding GHA secret. startup-only; rotating credentials requires re-rolling the controller Pod). - Runtime credential rotation in issued Secrets without - `BucketyAccess` recreate. + `BucketyAccess` recreate. Deferred by intent, not omission: + for gcs per-bucket service accounts the reuse-or-mint grant + 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`). - Multi-cluster federation. - Admission webhook for cross-resource invariants. Per-resource parameter validation (against per-driver schemas) and diff --git a/deploy/kustomize/crd/bucketyaccess.yaml b/deploy/kustomize/crd/bucketyaccess.yaml index 328f268..0147d4d 100644 --- a/deploy/kustomize/crd/bucketyaccess.yaml +++ b/deploy/kustomize/crd/bucketyaccess.yaml @@ -91,6 +91,14 @@ spec: Backend-side identity granted access. In v1alpha1 with no per-consumer scoping this is typically the backend's root principal. + principalRevocable: + type: boolean + description: | + Whether principal names a credential the driver + minted for this access and must revoke + backend-side (a gcs SA key). Deletion with the + backend missing from config blocks only for + revocable principals. conditions: type: array items: diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index 25631ac..e34b5bb 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:85ec8d4746b37fae88387cc280631ca16269404fb60fcc83e6af5d59aa13c863 +- digest: sha256:1fd39caed0cb3b17d827ccff4e95c6e80c432a0317e9c70d7cd12839c21ccbc4 name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260717T064548Z + newTag: 20260729T082750Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md new file mode 100644 index 0000000..846f185 --- /dev/null +++ b/examples/gcs/service-account/README.md @@ -0,0 +1,78 @@ +# gcs per-bucket service account (opt-in, driver >= 0.2) + +A `Buckety` that declares `parameters.serviceAccount` gets a +dedicated GCP service account with `roles/storage.objectAdmin` on +its bucket only, and every access Secret additionally carries: + +| key | value | +| --- | --- | +| `serviceAccountKey` | SA key JSON — mount it and point `GOOGLE_APPLICATION_CREDENTIALS` at it for OAuth2 bearer-token auth with native GCS clients and V4 signed URLs | +| `serviceAccountEmail` | `orders-@.iam.gserviceaccount.com` | +| `serviceAccountKeyId` | the key's `private_key_id`, for audit and rotation tooling | + +The static HMAC pair stays in the Secret unchanged (additive keys +per SPEC §Secret output), so S3-interop consumers keep working; +the SA credential is what shrinks blast radius from +"every bucket on the backend" to "this bucket". Once no consumer +of a bucket needs the S3-interop path, add `hmac: "false"` +(mutable) and its Secrets drop the backend-wide pair entirely - +per CR, or as a backend parameter default to make HMAC opt-in +across the backend. A CR can conversely opt out of a +backend-default SA with `serviceAccount: ""`. + +## Backend prerequisites + +This example has no `assert.sh` deliberately: fake-gcs-server +implements neither `iam.googleapis.com` nor bucket IAM policies, +so the feature is unit-tested (`pkg/drivers/gcs/serviceaccount_test.go`) +and exercised against real GCS. The backend needs: + +```yaml +backends: +- name: gcs + driver: gcs + config: + project: my-bucket-project + accessKeyID: ${GCS_HMAC_ID} + secretAccessKey: ${GCS_HMAC_SECRET} + serviceAccounts: + # STRONGLY RECOMMENDED: a dedicated identity project. Key + # creation equals impersonation, so the controller's IAM + # grants must be confined to a project whose only identities + # are the ones buckety mints. Cross-project bucket bindings + # make the split free. + project: my-buckety-identities + # Or impose the naming convention for all buckets, letting CRs + # omit the parameter: + # parameters: + # serviceAccount: ${name}-${namespace} +``` + +Controller credential grants: + +- on the identity project: a custom role with + `iam.serviceAccounts.{create,get,delete}` and + `iam.serviceAccountKeys.{create,list,delete}` (the predefined + `roles/iam.serviceAccountAdmin` + `roles/iam.serviceAccountKeyAdmin` + work but carry more than needed) +- on the bucket project: `storage.buckets.getIamPolicy` + + `storage.buckets.setIamPolicy` on top of the existing bucket + CRUD grant + +Orgs that set `constraints/iam.disableServiceAccountKeyCreation` +block this feature by design; the grant fails with an actionable +`GrantFailed` condition. + +## Rotation (manual, until scheduled rotation lands) + +``` +gcloud iam service-accounts keys delete \ + --iam-account= +``` + +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. diff --git a/examples/gcs/service-account/buckety.yaml b/examples/gcs/service-account/buckety.yaml new file mode 100644 index 0000000..0150791 --- /dev/null +++ b/examples/gcs/service-account/buckety.yaml @@ -0,0 +1,20 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/schema/buckety-gcs.schema.json +apiVersion: buckety.yolean.se/v1alpha1 +kind: Buckety +metadata: + name: orders +spec: + backend: gcs + retentionPolicy: Delete + parameters: + uniformBucketLevelAccess: "true" + # Per-bucket GCP service account (backend must enable + # serviceAccounts in its config). Template-resolved; the + # -${namespace} suffix is the uniqueness convention since SA + # IDs are unique per project. Alternatively the cluster + # operator declares this once as a backend parameter default + # and CRs omit it entirely. Immutable post-create. + serviceAccount: ${name}-${namespace} + defaultAccess: + role: ReadWrite + credentialsSecretName: orders-bucket diff --git a/examples/gcs/service-account/kustomization.yaml b/examples/gcs/service-account/kustomization.yaml new file mode 100644 index 0000000..86932e6 --- /dev/null +++ b/examples/gcs/service-account/kustomization.yaml @@ -0,0 +1,4 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: +- buckety.yaml diff --git a/go.mod b/go.mod index cc0193a..1b17303 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,8 @@ module github.com/Yolean/buckety-controller go 1.26.1 +toolchain go1.26.5 + require ( cloud.google.com/go/storage v1.63.1 github.com/Yolean/y-cluster v0.4.6 diff --git a/pkg/api/v1alpha1/types.go b/pkg/api/v1alpha1/types.go index c2a97c7..7476458 100644 --- a/pkg/api/v1alpha1/types.go +++ b/pkg/api/v1alpha1/types.go @@ -240,6 +240,14 @@ type BucketyAccessStatus struct { // per-consumer scoping is not implemented. Principal string `json:"principal,omitempty"` + // PrincipalRevocable is whether Principal names a credential + // the driver minted for this access and must revoke + // backend-side (a gcs SA key), as opposed to a shared static + // principal whose revoke is a no-op. Deletion with the backend + // missing from config blocks only for revocable principals; + // static ones release as in v1alpha1. + PrincipalRevocable bool `json:"principalRevocable,omitempty"` + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` } diff --git a/pkg/config/config.go b/pkg/config/config.go index 8ba0e84..fcf3135 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -10,9 +10,13 @@ package config import ( "encoding/json" "fmt" + "slices" + "strings" "github.com/Yolean/buckety-controller/pkg/drivers/registry" + "github.com/Yolean/buckety-controller/pkg/template" yclconfig "github.com/Yolean/y-cluster/pkg/configfile" + yaml "sigs.k8s.io/yaml" ) // Filename is the conventional name inside the directory passed @@ -35,7 +39,15 @@ type rawBackend struct { // driver-specific knobs out of portable CRs (issue #17): a // gcs backend declares location/uniformBucketLevelAccess here // while the CR carries only family-common parameters. - Parameters map[string]string `json:"parameters,omitempty"` + // + // RawMessage, not map[string]string, for the same reason as + // Config: the loader's envsubst policy scan rejects ${...} in + // untagged strings, and defaults for driver-declared templated + // keys legitimately carry naming-template references + // (serviceAccount: ${name}-${namespace}) that resolve per + // resource, not against the environment. Load decodes and + // re-imposes the no-${...} policy on every OTHER key. + Parameters json.RawMessage `json:"parameters,omitempty"` } // Backend is a resolved backend after driver factory invocation. @@ -66,6 +78,25 @@ func (b Backend) EffectiveParameters(crParams map[string]string) map[string]stri return out } +// ResolvedParameters is EffectiveParameters followed by template +// resolution of the keys the driver declares templated, against +// the Buckety's metadata.name/namespace and this backend's +// defaults map. This is THE parameter view every driver call and +// validation operates on; reconcilers and the webhook must not +// hand-roll the resolution or they diverge on which keys resolve. +func (b Backend) ResolvedParameters(bkyName, bkyNamespace string, crParams map[string]string) (map[string]string, error) { + effective := b.EffectiveParameters(crParams) + keys := registry.TemplatedParameters(b.Driver) + if len(keys) == 0 { + return effective, nil + } + return template.ResolveParameters(effective, keys, template.Inputs{ + Name: bkyName, + Namespace: bkyNamespace, + BackendDefaults: b.Defaults, + }) +} + // Loaded is the result of a successful Load. type Loaded struct { // Backends keyed by name for O(1) lookup from reconcilers. @@ -100,22 +131,73 @@ func Load(dir string) (*Loaded, error) { if err != nil { return nil, fmt.Errorf("backends[%d] %q: %w", i, b.Name, err) } + var parameters map[string]string + if len(b.Parameters) > 0 { + if err := yaml.UnmarshalStrict(b.Parameters, ¶meters); err != nil { + return nil, fmt.Errorf("backends[%d] %q: parameters: %w", i, b.Name, err) + } + } // Backend parameter defaults are validated at startup so a // typo crash-loops with a config diagnostic instead of - // failing every resource at admission. - if err := drv.ValidateParameters(b.Parameters); err != nil { + // failing every resource at admission. Defaults for + // driver-declared templated keys carry unresolved templates + // (they resolve per resource), so those skip driver value + // validation here and get a template syntax check instead; + // every other key re-imposes the loader's envsubst policy + // that the RawMessage detour bypassed (see rawBackend). + params := parameters + tkeys := registry.TemplatedParameters(drv) + if len(params) > 0 { + for k, v := range params { + if !slices.Contains(tkeys, k) && strings.Contains(v, "${") { + return nil, fmt.Errorf("backends[%d] %q: parameters.%s: ${...} references are not supported here (driver %q does not declare %q as a templated parameter)", i, b.Name, k, b.Driver, k) + } + } + if len(tkeys) > 0 { + if _, err := template.ResolveParameters(params, tkeys, template.Inputs{ + Name: "startup-check", Namespace: "startup-check", BackendDefaults: b.Defaults, + }); err != nil { + return nil, fmt.Errorf("backends[%d] %q: parameters: %w", i, b.Name, err) + } + params = withoutKeys(params, tkeys) + } + } + if err := drv.ValidateParameters(params); err != nil { return nil, fmt.Errorf("backends[%d] %q: parameters: %w", i, b.Name, err) } out.Backends[b.Name] = Backend{ Name: b.Name, Driver: drv, Defaults: b.Defaults, - Parameters: b.Parameters, + Parameters: parameters, } } return out, nil } +// withoutKeys returns params minus the listed keys, copying only +// when something is actually dropped. +func withoutKeys(params map[string]string, keys []string) map[string]string { + drop := false + for _, k := range keys { + if _, ok := params[k]; ok { + drop = true + break + } + } + if !drop { + return params + } + out := make(map[string]string, len(params)) + for k, v := range params { + out[k] = v + } + for _, k := range keys { + delete(out, k) + } + return out +} + func validateOuter(c *rawConfig) error { if len(c.Backends) == 0 { return fmt.Errorf("no backends configured") diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index dbd3da7..4c61357 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -1,6 +1,16 @@ package config -import "testing" +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Yolean/buckety-controller/pkg/drivers/registry" +) func TestEffectiveParameters(t *testing.T) { b := Backend{Parameters: map[string]string{ @@ -21,3 +31,147 @@ func TestEffectiveParameters(t *testing.T) { t.Errorf("defaults only: %v", out) } } + +// stubDriver is a minimal registry.Driver for exercising the +// config layer; templated declares TemplatedParameters, and +// ValidateParameters rejects any declared-templated key it is +// handed, proving Load stripped them before validating backend +// parameter defaults. +type stubDriver struct{ templated []string } + +func (s stubDriver) Name() string { return "stub" } +func (s stubDriver) Version() string { return "0.0.1" } +func (s stubDriver) InspectBuckety(context.Context, string) (registry.Inspection, error) { + return registry.Inspection{}, nil +} +func (s stubDriver) EnsureBuckety(context.Context, registry.EnsureRequest) error { return nil } +func (s stubDriver) DeleteBuckety(context.Context, registry.DeleteRequest) error { return nil } +func (s stubDriver) GrantAccess(context.Context, registry.GrantRequest) (registry.GrantResult, error) { + return registry.GrantResult{}, nil +} +func (s stubDriver) RevokeAccess(context.Context, string) error { return nil } +func (s stubDriver) ValidateUpdateParameters(_, _ map[string]string) error { return nil } +func (s stubDriver) ValidateAccessParameters(map[string]string) error { return nil } +func (s stubDriver) ValidateResourceName(string) error { return nil } +func (s stubDriver) TemplatedParameters() []string { return s.templated } +func (s stubDriver) ValidateParameters(params map[string]string) error { + for _, k := range s.templated { + if _, ok := params[k]; ok { + return fmt.Errorf("declared-templated key %q reached driver validation unresolved", k) + } + } + return nil +} + +func TestResolvedParameters(t *testing.T) { + b := Backend{ + Driver: stubDriver{templated: []string{"serviceAccount"}}, + Defaults: map[string]string{"zone": "eu"}, + Parameters: map[string]string{ + "serviceAccount": "${name}-${namespace}", + "location": "EUROPE-WEST4", + }, + } + + got, err := b.ResolvedParameters("orders", "tenant1", nil) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got["serviceAccount"] != "orders-tenant1" || got["location"] != "EUROPE-WEST4" { + t.Errorf("resolved view: %v", got) + } + + // CR wins per key before resolution, so a literal CR value + // overrides a templated backend default. + got, err = b.ResolvedParameters("orders", "tenant1", map[string]string{"serviceAccount": "fixed-name"}) + if err != nil || got["serviceAccount"] != "fixed-name" { + t.Errorf("CR override: %v %v", got, err) + } + + // Backend defaults feed ${backend.X} in parameter templates. + b.Parameters["serviceAccount"] = "${backend.zone}-${name}" + if got, err = b.ResolvedParameters("orders", "tenant1", nil); err != nil || got["serviceAccount"] != "eu-orders" { + t.Errorf("backend ref: %v %v", got, err) + } + + // Label references are rejected (restricted grammar). + if _, err := b.ResolvedParameters("orders", "tenant1", map[string]string{"serviceAccount": "${label['site']}"}); err == nil { + t.Error("label reference accepted in parameter template") + } + + // A driver without templated keys passes the merged view + // through untouched, templates and all. + plain := Backend{Driver: stubDriver{}, Parameters: map[string]string{"serviceAccount": "${name}"}} + if got, err := plain.ResolvedParameters("orders", "t1", nil); err != nil || got["serviceAccount"] != "${name}" { + t.Errorf("no-capability passthrough: %v %v", got, err) + } +} + +// Load validates backend parameter defaults at startup, but +// driver-declared templated keys only resolve per resource: they +// get a template syntax check and skip driver value validation. +func TestLoadTemplatedParameterDefaults(t *testing.T) { + registry.Register("cfgstub", "0.0.1", func(json.RawMessage) (registry.Driver, error) { + return stubDriver{templated: []string{"serviceAccount"}}, nil + }) + + write := func(t *testing.T, yaml string) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, Filename), []byte(yaml), 0o644); err != nil { + t.Fatal(err) + } + return dir + } + + // Templated default passes startup despite the stub rejecting + // any unresolved templated key it sees. + dir := write(t, ` +backends: +- name: be + driver: cfgstub + parameters: + serviceAccount: ${name}-${namespace} + location: EU +`) + if _, err := Load(dir); err != nil { + t.Fatalf("templated default rejected at startup: %v", err) + } + + // Template syntax errors still crash-loop with a config + // diagnostic instead of failing every resource at admission. + dir = write(t, ` +backends: +- name: be + driver: cfgstub + parameters: + serviceAccount: ${label['site']}-x +`) + if _, err := Load(dir); err == nil { + t.Fatal("label reference in templated default accepted at startup") + } +} + +// The RawMessage detour that lets templated defaults through MUST +// NOT soften the loader's envsubst policy for everything else: a +// ${...} in a non-templated parameter default is still a startup +// error, not a value that travels to the backend verbatim. +func TestLoadRejectsRefsInNonTemplatedDefaults(t *testing.T) { + registry.Register("cfgstub2", "0.0.1", func(json.RawMessage) (registry.Driver, error) { + return stubDriver{templated: []string{"serviceAccount"}}, nil + }) + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, Filename), []byte(` +backends: +- name: be + driver: cfgstub2 + parameters: + location: ${REGION} +`), 0o644); err != nil { + t.Fatal(err) + } + _, err := Load(dir) + if err == nil || !strings.Contains(err.Error(), "parameters.location") { + t.Fatalf("non-templated ${...} default: %v", err) + } +} diff --git a/pkg/config/schema/buckety-controller.schema.json b/pkg/config/schema/buckety-controller.schema.json index fff0176..5c1915e 100644 --- a/pkg/config/schema/buckety-controller.schema.json +++ b/pkg/config/schema/buckety-controller.schema.json @@ -36,7 +36,7 @@ "parameters": { "type": "object", "additionalProperties": { "type": "string" }, - "description": "Cluster-operator-owned parameter defaults, merged under Buckety.spec.parameters (the CR wins per key). Lets driver-specific parameters stay out of portable CRs (SPEC: Driver families). Validated against the driver at startup." + "description": "Cluster-operator-owned parameter defaults, merged under Buckety.spec.parameters (the CR wins per key). Lets driver-specific parameters stay out of portable CRs (SPEC: Driver families). Validated against the driver at startup, except driver-declared templated keys (gcs serviceAccount), whose values may carry ${name}/${namespace}/${backend.X} references resolved per resource (SPEC: Naming templates > Parameter templates)." } } } diff --git a/pkg/controller/buckety/reconciler.go b/pkg/controller/buckety/reconciler.go index 125a6df..bb15ffc 100644 --- a/pkg/controller/buckety/reconciler.go +++ b/pkg/controller/buckety/reconciler.go @@ -227,10 +227,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // (docs/SCAFFOLDING.md "Webhook TLS"); this is the promised // fallback that surfaces invalid parameters on status instead of // letting them travel to the backend as an opaque driver error. - // Validation and Ensure both operate on the merged view of - // backend parameter defaults + CR parameters (CR wins per key, - // see config.Backend.EffectiveParameters). - effective := backend.EffectiveParameters(bky.Spec.Parameters) + // Validation and Ensure both operate on the merged + resolved + // view of backend parameter defaults + CR parameters (CR wins + // per key, driver-declared templated keys resolved; see + // config.Backend.ResolvedParameters). + effective, perr := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) + if perr != nil { + r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", + corev1.EventTypeWarning, "ParameterTemplate", perr.Error()) + setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), bky.Generation) + setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "ParameterTemplate", "spec change required", bky.Generation) + return ctrl.Result{}, r.Status().Patch(ctx, &bky, client.MergeFrom(baseBky)) + } if err := backend.Driver.ValidateParameters(effective); err != nil { r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "InvalidParameters", corev1.EventTypeWarning, "InvalidParameters", err.Error()) @@ -321,6 +329,38 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety return ctrl.Result{}, nil } + // The implicit access is not a deletion blocker, but it must + // be revoked while this Buckety can still resolve its backend: + // left to owner-ref GC it would be deleted AFTER the Buckety, + // and its finalizer then has no backend to revoke against - + // with gcs 0.2 per-access keys that orphans a live credential + // on a Retain-surviving service account. So its deletion is + // driven from here, and the Buckety waits for the access + // finalizer (which performs the revocation) to finish. + for i := range accesses.Items { + a := &accesses.Items[i] + if a.Spec.BucketyRef.Name != bky.Name || a.Labels[bucketyv1.LabelImplicit] != "true" { + continue + } + if a.DeletionTimestamp.IsZero() { + if err := r.Delete(ctx, a); err != nil && !apierrors.IsNotFound(err) { + return ctrl.Result{}, err + } + } + // The access deletion re-enqueues this Buckety via the + // access watch; the short requeue covers a lost event. + // Normally one pass - but a revocable principal with its + // backend missing blocks the access (with its own + // condition), so say what is being waited on. + setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokingAccesses", + fmt.Sprintf("waiting for implicit BucketyAccess %q to revoke before teardown", a.Name), + bky.Generation) + if err := r.Status().Patch(ctx, bky, client.MergeFrom(base)); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 2 * time.Second}, nil + } + // Adopted resources are never deleted from the backend (SPEC // §Adoption): the content predates this CR, or the CR never // verified otherwise, so retentionPolicy=Delete degrades to @@ -359,7 +399,24 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety } return ctrl.Result{}, nil } - if err := backend.Driver.DeleteBuckety(ctx, bky.Status.BackendResourceName); err != nil { + // The same resolved parameter view Ensure operated on, so + // the driver can find per-resource principals (gcs + // serviceAccount) at teardown. Resolution is deterministic + // (name/namespace/backend defaults only), so a failure here + // means the backend config changed underneath the resource; + // deletion blocks rather than orphaning the principal. + effective, perr := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) + if perr != nil { + r.eventIfTransition(bky, base.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", + corev1.EventTypeWarning, "DeleteFailed", perr.Error()) + setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), bky.Generation) + _ = r.Status().Patch(ctx, bky, client.MergeFrom(base)) + return ctrl.Result{}, perr + } + if err := backend.Driver.DeleteBuckety(ctx, registry.DeleteRequest{ + Name: bky.Status.BackendResourceName, + Parameters: effective, + }); err != nil { if registry.IsDeletionInProgress(err) { // Recursive contents deletion runs in bounded // slices; this is progress, not failure. The diff --git a/pkg/controller/buckety/reconciler_test.go b/pkg/controller/buckety/reconciler_test.go index f3630d5..472db68 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -1,9 +1,19 @@ package buckety import ( + "context" "testing" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" + "github.com/Yolean/buckety-controller/pkg/config" "github.com/Yolean/buckety-controller/pkg/drivers/registry" ) @@ -61,3 +71,86 @@ func TestDecideProvenance(t *testing.T) { } } } + +// Buckety deletion must delete the implicit access and WAIT for +// its finalizer before releasing its own: owner-ref GC would +// otherwise remove the access after the Buckety, whose absence +// makes the access finalizer skip RevokeAccess - orphaning a live +// key on a Retain-surviving service account (checkit review +// finding 3, sharpened). +func TestDeleteWaitsForImplicitAccessRevocation(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orders", Namespace: "t1", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketySpec{Backend: "be", RetentionPolicy: bucketyv1.RetentionRetain}, + } + implicit := &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orders", Namespace: "t1", + Labels: map[string]string{bucketyv1.LabelImplicit: "true"}, + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "orders-bucket", + }, + } + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky, implicit). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{Backends: map[string]config.Backend{}}} + if err := cl.Delete(ctx, bky); err != nil { + t.Fatal(err) + } + key := types.NamespacedName{Namespace: "t1", Name: "orders"} + req := reconcile.Request{NamespacedName: key} + + // First pass: implicit access gets deleted (terminating, held + // by its finalizer), Buckety finalizer stays. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("first delete pass: %v", err) + } + var acc bucketyv1.BucketyAccess + if err := cl.Get(ctx, key, &acc); err != nil { + t.Fatalf("implicit access should still exist while revoking: %v", err) + } + if acc.DeletionTimestamp.IsZero() { + t.Error("implicit access not deleted by the Buckety pass") + } + var stillHere bucketyv1.Buckety + if err := cl.Get(ctx, key, &stillHere); err != nil { + t.Fatalf("buckety released before implicit access was revoked: %v", err) + } + + // Second pass with the access still terminating: keep waiting. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("waiting pass: %v", err) + } + if err := cl.Get(ctx, key, &stillHere); err != nil { + t.Fatalf("buckety released while access still terminating: %v", err) + } + + // Access finalizer completes (its own reconciler would do this + // after RevokeAccess); the Buckety may then go. + acc.Finalizers = nil + if err := cl.Update(ctx, &acc); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("final delete pass: %v", err) + } + if err := cl.Get(ctx, key, &stillHere); !apierrors.IsNotFound(err) { + t.Errorf("buckety not released after implicit access completed: %v", err) + } +} diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index 5e1cd4f..575fac2 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/pkg/controller/bucketyaccess/reconciler.go @@ -125,16 +125,58 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu var bky bucketyv1.Buckety bkyErr := r.Get(ctx, types.NamespacedName{Namespace: access.Namespace, Name: access.Spec.BucketyRef.Name}, &bky) - // Deletion path. + // Deletion path. SPEC: deletion blocks on RevokeAccess + // succeeding - and since gcs 0.2 a principal can be a live + // key, so the skip paths here must never silently orphan one. if !access.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(&access, bucketyv1.FinalizerCleanup) { - if bkyErr == nil { - if backend, ok := r.Config.Lookup(bky.Spec.Backend); ok { + switch { + case bkyErr != nil && !apierrors.IsNotFound(bkyErr): + // Transient Buckety read failure: retry instead of + // falling through to a finalizer removal that would + // skip revocation. + return ctrl.Result{}, bkyErr + case bkyErr == nil: + backend, ok := r.Config.Lookup(bky.Spec.Backend) + if !ok && access.Status.PrincipalRevocable { + // A minted credential exists but no backend to + // revoke it against. Letting the finalizer go + // would orphan it, so deletion blocks with the + // same remedy as Buckety deletion under + // retentionPolicy=Delete: restore the backend in + // buckety-controller.yaml. Static shared + // principals (Revocable=false) release as in + // v1alpha1 - their revoke is a no-op, and + // blocking them would wedge scenarios like + // backend renames. + base := access.DeepCopy() + msg := fmt.Sprintf("cannot revoke principal %q: backend %q is not registered in buckety-controller.yaml; restore it to let this BucketyAccess go", access.Status.Principal, bky.Spec.Backend) + r.eventIfTransition(&access, base.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", + corev1.EventTypeWarning, "DeletionBlocked", msg) + setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", msg, access.Generation) + if err := r.Status().Patch(ctx, &access, client.MergeFrom(base)); err != nil { + return ctrl.Result{}, err + } + if r.RequeueAfter != nil { + return r.RequeueAfter(), nil + } + return ctrl.Result{}, nil + } + if ok { if err := backend.Driver.RevokeAccess(ctx, access.Status.Principal); err != nil { log.Error(err, "RevokeAccess failed") return ctrl.Result{}, err } } + default: + // Buckety already gone (NotFound): no backend can be + // resolved and no operator remedy would ever unblock, + // so the finalizer is released. The systematic route + // here - the implicit access, owner-ref-GC'd after + // its Buckety - is prevented by the Buckety + // reconciler deleting it BEFORE releasing its own + // finalizer, so revocation ran with the Buckety + // still present. } controllerutil.RemoveFinalizer(&access, bucketyv1.FinalizerCleanup) if err := r.Update(ctx, &access); err != nil { @@ -214,17 +256,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, r.Status().Patch(ctx, &access, client.MergeFrom(baseAccess)) } - res, err := backend.Driver.GrantAccess(ctx, registry.GrantRequest{ - BucketyName: bky.Status.BackendResourceName, - Role: string(access.Spec.Role), - Parameters: access.Spec.Parameters, - }) + // The Buckety's resolved parameter view rides along on the + // grant so drivers can find per-resource principals (gcs + // serviceAccount) without knowing about CRDs. + bkyParams, err := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) if err != nil { r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", corev1.EventTypeWarning, "GrantFailed", err.Error()) setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", err.Error(), access.Generation) - _ = r.Status().Patch(ctx, &access, client.MergeFrom(baseAccess)) - return ctrl.Result{}, err + return ctrl.Result{}, r.Status().Patch(ctx, &access, client.MergeFrom(baseAccess)) } // Refuse to touch a Secret this BucketyAccess does not control: @@ -234,6 +274,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // the conflict clears on a later requeue once the old Secret is // gone. Live read, not cache: foreign Secrets carry no // LabelOwnedSecret and are invisible to the scoped informer. + // + // The gate runs BEFORE GrantAccess: drivers may mint live + // credentials there (a GCS SA key), and minting for a Secret we + // then refuse to write would leak an unrecorded credential. The + // read also feeds ExistingSecretData, which is what lets such + // drivers return the already-minted credential unchanged + // instead of minting on every reconcile. var existing corev1.Secret getErr := r.liveReader().Get(ctx, types.NamespacedName{Namespace: access.Namespace, Name: access.Spec.CredentialsSecretName}, &existing) switch { @@ -253,6 +300,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu case getErr != nil && !apierrors.IsNotFound(getErr): return ctrl.Result{}, getErr } + var existingData map[string][]byte + if getErr == nil { + existingData = existing.Data + } + + res, err := backend.Driver.GrantAccess(ctx, registry.GrantRequest{ + BucketyName: bky.Status.BackendResourceName, + Role: string(access.Spec.Role), + Parameters: access.Spec.Parameters, + BucketyParameters: bkyParams, + ExistingSecretData: existingData, + }) + if err != nil { + r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", + corev1.EventTypeWarning, "GrantFailed", err.Error()) + setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", err.Error(), access.Generation) + _ = r.Status().Patch(ctx, &access, client.MergeFrom(baseAccess)) + return ctrl.Result{}, err + } // Mint/update the Secret with this BucketyAccess as owner. // Built on the live read above instead of @@ -277,7 +343,27 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, err } + // A changed principal means GrantAccess re-minted (the Secret + // was lost, hand-edited, or its key invalidated): revoke the + // replaced credential now that the Secret carries its + // successor. Without this, every re-mint orphans a live key + // on the gcs SA until GCP's 10-keys-per-SA cap wedges + // Keys.Create permanently (checkit review finding 1). + // Ordering makes it self-healing: status.principal keeps + // naming the old key until revocation succeeds, so a failure + // retries here while GrantAccess keeps returning the + // already-written replacement. + if old := access.Status.Principal; old != "" && old != res.Principal { + if rerr := backend.Driver.RevokeAccess(ctx, old); rerr != nil { + r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokeFailed", + corev1.EventTypeWarning, "RevokeFailed", rerr.Error()) + setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokeFailed", rerr.Error(), access.Generation) + _ = r.Status().Patch(ctx, &access, client.MergeFrom(baseAccess)) + return ctrl.Result{}, rerr + } + } access.Status.Principal = res.Principal + access.Status.PrincipalRevocable = res.Revocable // ScopingNotImplemented if the driver is not actually // scoping per role and the user asked for something other diff --git a/pkg/controller/bucketyaccess/reconciler_test.go b/pkg/controller/bucketyaccess/reconciler_test.go index 45a52ca..940364d 100644 --- a/pkg/controller/bucketyaccess/reconciler_test.go +++ b/pkg/controller/bucketyaccess/reconciler_test.go @@ -5,14 +5,19 @@ import ( "testing" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" + "github.com/Yolean/buckety-controller/pkg/config" + "github.com/Yolean/buckety-controller/pkg/drivers/registry" ) // The gate must treat a reason change within the same status as a @@ -148,3 +153,343 @@ func TestWriteSecret(t *testing.T) { t.Errorf("no-op reconcile bumped resourceVersion %s -> %s", before, after.ResourceVersion) } } + +// grantRecorder is a minimal registry.Driver that records what +// GrantAccess received and which principals were revoked. +type grantRecorder struct { + req *registry.GrantRequest + data map[string][]byte + principal string + revoked []string + revokeErr error +} + +func (g *grantRecorder) Name() string { return "rec" } +func (g *grantRecorder) Version() string { return "0.0.1" } +func (g *grantRecorder) InspectBuckety(context.Context, string) (registry.Inspection, error) { + return registry.Inspection{}, nil +} +func (g *grantRecorder) EnsureBuckety(context.Context, registry.EnsureRequest) error { return nil } +func (g *grantRecorder) DeleteBuckety(context.Context, registry.DeleteRequest) error { return nil } +func (g *grantRecorder) GrantAccess(_ context.Context, req registry.GrantRequest) (registry.GrantResult, error) { + g.req = &req + p := g.principal + if p == "" { + p = "rec-principal" + } + return registry.GrantResult{SecretData: g.data, Principal: p, Scoped: true}, nil +} +func (g *grantRecorder) RevokeAccess(_ context.Context, principal string) error { + g.revoked = append(g.revoked, principal) + return g.revokeErr +} +func (g *grantRecorder) ValidateParameters(map[string]string) error { return nil } +func (g *grantRecorder) ValidateUpdateParameters(_, _ map[string]string) error { return nil } +func (g *grantRecorder) ValidateAccessParameters(map[string]string) error { return nil } +func (g *grantRecorder) ValidateResourceName(string) error { return nil } + +// The Secret gate runs BEFORE GrantAccess: drivers may mint live +// credentials there (a GCS SA key), so a conflicting Secret must +// pre-empt minting entirely, and the existing owned Secret's data +// must ride along so create-only-retrievable credentials can be +// returned unchanged (registry.GrantRequest.ExistingSecretData). +func TestGrantGatedOnSecretAndFedExistingData(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + now := metav1.Now() + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{Name: "orders", Namespace: "t1"}, + Spec: bucketyv1.BucketySpec{Backend: "be", Parameters: map[string]string{"serviceAccount": "orders-t1"}}, + Status: bucketyv1.BucketyStatus{ + Backend: "be", + BackendResourceName: "t1-orders", + Conditions: []metav1.Condition{{ + Type: "Ready", Status: metav1.ConditionTrue, Reason: "EnsuredOnBackend", LastTransitionTime: now, + }}, + }, + } + newAccess := func() *bucketyv1.BucketyAccess { + return &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reader", Namespace: "t1", UID: "uid-a", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "reader-creds", + }, + } + } + reconcile := func(t *testing.T, rec *grantRecorder, extra ...client.Object) (client.Client, *bucketyv1.BucketyAccess) { + t.Helper() + access := newAccess() + objs := append([]client.Object{bky.DeepCopy(), access}, extra...) + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(objs...). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: rec}}, + }} + if _, err := r.Reconcile(context.Background(), reconcilerRequest("t1", "reader")); err != nil { + t.Fatalf("reconcile: %v", err) + } + var got bucketyv1.BucketyAccess + if err := cl.Get(context.Background(), types.NamespacedName{Namespace: "t1", Name: "reader"}, &got); err != nil { + t.Fatal(err) + } + return cl, &got + } + + // (a) No Secret: grant runs with nil ExistingSecretData and + // the Buckety's parameters, Secret gets minted. + rec := &grantRecorder{data: map[string][]byte{"bucket": []byte("t1-orders")}} + cl, got := reconcile(t, rec) + if rec.req == nil { + t.Fatal("grant not called") + } + if rec.req.ExistingSecretData != nil { + t.Errorf("ExistingSecretData on first mint: %v", rec.req.ExistingSecretData) + } + if rec.req.BucketyParameters["serviceAccount"] != "orders-t1" { + t.Errorf("BucketyParameters: %v", rec.req.BucketyParameters) + } + if rec.req.BucketyName != "t1-orders" { + t.Errorf("BucketyName: %q", rec.req.BucketyName) + } + var secret corev1.Secret + if err := cl.Get(context.Background(), types.NamespacedName{Namespace: "t1", Name: "reader-creds"}, &secret); err != nil { + t.Fatalf("minted secret: %v", err) + } + if got.Status.Principal != "rec-principal" { + t.Errorf("principal: %q", got.Status.Principal) + } + + // (b) Foreign Secret: SecretConflict pre-empts the grant - + // nothing is minted for a Secret we refuse to write. + rec = &grantRecorder{data: map[string][]byte{"bucket": []byte("x")}} + foreign := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "reader-creds", Namespace: "t1"}} + _, got = reconcile(t, rec, foreign) + if rec.req != nil { + t.Error("grant called despite SecretConflict") + } + conflicted := false + for _, c := range got.Status.Conditions { + if c.Type == "Ready" && c.Reason == "SecretConflict" { + conflicted = true + } + } + if !conflicted { + t.Errorf("SecretConflict not surfaced: %+v", got.Status.Conditions) + } + + // (c) Owned Secret: its current data feeds the grant. + rec = &grantRecorder{data: map[string][]byte{"bucket": []byte("t1-orders")}} + ctrl := true + owned := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reader-creds", Namespace: "t1", + Labels: map[string]string{bucketyv1.LabelOwnedSecret: "true"}, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: bucketyv1.GroupVersion.String(), Kind: "BucketyAccess", + Name: "reader", UID: "uid-a", Controller: &ctrl, + }}, + }, + Data: map[string][]byte{"serviceAccountKey": []byte("previously-minted")}, + } + _, _ = reconcile(t, rec, owned) + if rec.req == nil { + t.Fatal("grant not called for owned secret") + } + if string(rec.req.ExistingSecretData["serviceAccountKey"]) != "previously-minted" { + t.Errorf("ExistingSecretData: %v", rec.req.ExistingSecretData) + } +} + +func reconcilerRequest(ns, name string) reconcile.Request { + return reconcile.Request{NamespacedName: types.NamespacedName{Namespace: ns, Name: name}} +} + +// A re-mint that changes the principal must revoke the replaced +// one AFTER the Secret write, or every lost/hand-edited Secret +// orphans a live key until the SA's 10-key cap wedges Keys.Create +// (checkit review finding 1). status.principal advances only once +// revocation succeeds, so failures retry. +func TestReplacedPrincipalRevoked(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + now := metav1.Now() + newObjs := func() (*bucketyv1.Buckety, *bucketyv1.BucketyAccess) { + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{Name: "orders", Namespace: "t1"}, + Spec: bucketyv1.BucketySpec{Backend: "be"}, + Status: bucketyv1.BucketyStatus{ + Backend: "be", BackendResourceName: "t1-orders", + Conditions: []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue, Reason: "EnsuredOnBackend", LastTransitionTime: now}}, + }, + } + access := &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reader", Namespace: "t1", UID: "uid-a", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "reader-creds", + }, + Status: bucketyv1.BucketyAccessStatus{Principal: "projects/p/serviceAccounts/x/keys/old"}, + } + return bky, access + } + 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{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: rec}}, + }} + _, err := r.Reconcile(context.Background(), reconcilerRequest("t1", "reader")) + var got bucketyv1.BucketyAccess + if gerr := cl.Get(context.Background(), types.NamespacedName{Namespace: "t1", Name: "reader"}, &got); gerr != nil { + t.Fatal(gerr) + } + return cl, &got, err + } + + // Principal change: old revoked, status advances. + rec := &grantRecorder{ + data: map[string][]byte{"bucket": []byte("t1-orders")}, + principal: "projects/p/serviceAccounts/x/keys/new", + } + _, got, err := run(t, rec) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if len(rec.revoked) != 1 || rec.revoked[0] != "projects/p/serviceAccounts/x/keys/old" { + t.Errorf("revoked: %v", rec.revoked) + } + if got.Status.Principal != "projects/p/serviceAccounts/x/keys/new" { + t.Errorf("principal: %q", got.Status.Principal) + } + + // Unchanged principal: no revocation. + rec = &grantRecorder{ + data: map[string][]byte{"bucket": []byte("t1-orders")}, + principal: "projects/p/serviceAccounts/x/keys/old", + } + if _, _, err := run(t, rec); err != nil { + t.Fatal(err) + } + if len(rec.revoked) != 0 { + t.Errorf("steady state revoked: %v", rec.revoked) + } + + // Revocation failure: reconcile errors and status.principal + // still names the old key so the retry revokes it again. + rec = &grantRecorder{ + data: map[string][]byte{"bucket": []byte("t1-orders")}, + principal: "projects/p/serviceAccounts/x/keys/new", + revokeErr: context.DeadlineExceeded, + } + _, got, err = run(t, rec) + if err == nil { + t.Fatal("revoke failure swallowed") + } + if got.Status.Principal != "projects/p/serviceAccounts/x/keys/old" { + t.Errorf("principal advanced past failed revoke: %q", got.Status.Principal) + } +} + +// Deleting an access whose backend is missing from config must +// BLOCK while a REVOCABLE principal exists (releasing the +// finalizer would orphan the credential), and proceed for static +// shared principals - whose revoke is a no-op - exactly as in +// v1alpha1, or backend renames wedge every access teardown (seen +// as backend-stickiness e2e failures across all drivers). +func TestDeletionBlocksWithoutBackend(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + newAccess := func(principal string, revocable bool) (*bucketyv1.Buckety, *bucketyv1.BucketyAccess) { + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{Name: "orders", Namespace: "t1"}, + Spec: bucketyv1.BucketySpec{Backend: "gone"}, + } + return bky, &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reader", Namespace: "t1", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "reader-creds", + }, + Status: bucketyv1.BucketyAccessStatus{Principal: principal, PrincipalRevocable: revocable}, + } + } + + // Revocable principal: blocked with a condition. + bky, access := newAccess("projects/p/serviceAccounts/x/keys/k1", true) + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky, access). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{Backends: map[string]config.Backend{}}} + if err := cl.Delete(ctx, access); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(ctx, reconcilerRequest("t1", "reader")); err != nil { + t.Fatalf("reconcile: %v", err) + } + var got bucketyv1.BucketyAccess + if err := cl.Get(ctx, types.NamespacedName{Namespace: "t1", Name: "reader"}, &got); err != nil { + t.Fatalf("access should still exist (blocked): %v", err) + } + blocked := false + for _, c := range got.Status.Conditions { + if c.Type == "Ready" && c.Reason == "BackendUnavailable" { + blocked = true + } + } + if !blocked { + t.Errorf("no blocking condition: %+v", got.Status.Conditions) + } + + // Static shared principal (revoke is a no-op): released, as in + // v1alpha1 - this is what backend-stickiness scenarios do. + for _, principal := range []string{"gcs-static", ""} { + bky, access = newAccess(principal, false) + cl = fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky, access). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + r = &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{Backends: map[string]config.Backend{}}} + if err := cl.Delete(ctx, access); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(ctx, reconcilerRequest("t1", "reader")); err != nil { + t.Fatalf("reconcile: %v", err) + } + if err := cl.Get(ctx, types.NamespacedName{Namespace: "t1", Name: "reader"}, &got); !apierrors.IsNotFound(err) { + t.Errorf("access with principal %q not released: %v", principal, err) + } + } +} diff --git a/pkg/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index a3a1335..c80b4cd 100644 --- a/pkg/drivers/gcs/driver.go +++ b/pkg/drivers/gcs/driver.go @@ -25,13 +25,29 @@ // The static pair is copied identically to every BucketyAccess and // the reconciler surfaces ScopingNotImplemented for non-ReadWrite // roles - the same v1alpha1 posture as the s3 driver's root keys. -// Driver-minted per-access credentials (an HMAC key or a -// bucket-scoped service account per access) are deliberately NOT -// in v0.1: GrantAccess runs on every reconcile and rewrites the -// Secret from its result, and an HMAC secret is only retrievable -// at creation, so per-access minting needs the v1alpha2 scoping -// design (grant-once semantics, access identity in GrantRequest) -// before it can be idempotent. +// +// v0.2 adds opt-in per-bucket service accounts (see +// serviceaccount.go): a backend that enables `serviceAccounts` in +// its config lets a Buckety declare parameters.serviceAccount, and +// the driver then maintains a dedicated GCP service account bound +// to just that bucket (roles/storage.objectAdmin, bucket-level) +// plus one user-managed key per BucketyAccess, written to the +// Secret as serviceAccountKey/serviceAccountEmail/ +// 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 serviceAccounts config names the GCP project the SAs live +// in. A DEDICATED identity project (separate from the bucket +// project) is strongly recommended: key creation on an SA equals +// impersonating it, so the controller's serviceAccountAdmin/ +// serviceAccountKeyAdmin grants must be confined to a project +// whose only identities are the ones minted here. package gcs import ( @@ -50,7 +66,9 @@ import ( "cloud.google.com/go/storage" "golang.org/x/sync/errgroup" "google.golang.org/api/googleapi" + iam "google.golang.org/api/iam/v1" "google.golang.org/api/iterator" + "google.golang.org/api/option" "github.com/Yolean/buckety-controller/pkg/drivers/objectstore" "github.com/Yolean/buckety-controller/pkg/drivers/registry" @@ -63,10 +81,12 @@ const DriverName = "gcs" // version is the driver SemVer. Injected at build time via // -// -ldflags '-X github.com/Yolean/buckety-controller/pkg/drivers/gcs.version=0.1.0' +// -ldflags '-X github.com/Yolean/buckety-controller/pkg/drivers/gcs.version=0.2.0' // // per SPEC §Driver versioning. Default keeps tests building. -var version = "0.1.0" +// 0.2.0: additive serviceAccount parameter + Secret keys (minor +// bump per SPEC compatibility rules). +var version = "0.2.0" // globalEndpoint is the S3-interop host written to access // Secrets for buckets whose location has no locational endpoint @@ -81,7 +101,7 @@ func init() { } // Config is the typed shape of the `config:` block under a gcs -// backend. Mirrors pkg/drivers/gcs/schema/v0.1/config.schema.json. +// backend. Mirrors pkg/drivers/gcs/schema/v0.2/config.schema.json. // Credential fields carry envsubst:"true" so ${VAR} interpolation // works at controller startup. type Config struct { @@ -101,9 +121,37 @@ type Config struct { Region string `json:"region,omitempty"` // AccessKeyID / SecretAccessKey are the static HMAC pair copied // into every access Secret (see the package comment for why - // v0.1 does not mint per-access keys). + // v0.2 still carries them even with serviceAccounts enabled). AccessKeyID string `json:"accessKeyID" envsubst:"true"` SecretAccessKey string `json:"secretAccessKey" envsubst:"true"` + // ServiceAccounts opts this backend into per-bucket service + // accounts (parameters.serviceAccount). Nil disables the + // feature and rejects the parameter, keeping HMAC-only + // deployments free of IAM API calls and permissions. + ServiceAccounts *ServiceAccountsConfig `json:"serviceAccounts,omitempty"` +} + +// ServiceAccountsConfig gates and locates per-bucket service +// accounts. Mirrors schema/v0.2/config.schema.json. +type ServiceAccountsConfig struct { + // Project is the GCP project the per-bucket service accounts + // are created in. Strongly recommended to be a DEDICATED + // identity project separate from the bucket project: the + // controller needs roles/iam.serviceAccountAdmin + + // roles/iam.serviceAccountKeyAdmin here, and key creation + // equals impersonation, so this grant must not extend to + // projects holding unrelated service accounts. Cross-project + // bucket IAM bindings make the split free. + Project string `json:"project"` + // Endpoint overrides the IAM API endpoint (full URL), e.g. + // for Private Service Connect. Authentication stays on unless + // Insecure is also set; unset reaches iam.googleapis.com. + Endpoint string `json:"endpoint,omitempty"` + // Insecure disables authentication on Endpoint, for emulators + // and tests ONLY. A separate explicit flag so that an + // endpoint typo cannot silently turn credentials off + // (checkit review finding 4). Requires Endpoint. + Insecure bool `json:"insecure,omitempty"` } func factory(raw json.RawMessage) (registry.Driver, error) { @@ -136,13 +184,39 @@ func factory(raw json.RawMessage) (registry.Driver, error) { return nil, fmt.Errorf("gcs config: client init (is GOOGLE_APPLICATION_CREDENTIALS set?): %w", err) } - return &Driver{cfg: &c, client: cl}, nil + // The IAM admin client exists only when the backend opts in, + // so HMAC-only deployments need neither the iam.googleapis.com + // API enabled nor any IAM permissions. + var iamsvc *iam.Service + if c.ServiceAccounts != nil { + if c.ServiceAccounts.Project == "" { + return nil, fmt.Errorf("gcs config: serviceAccounts: missing required field %q (a dedicated identity project, separate from the bucket project, is strongly recommended)", "project") + } + if c.ServiceAccounts.Insecure && c.ServiceAccounts.Endpoint == "" { + return nil, fmt.Errorf("gcs config: serviceAccounts.insecure requires serviceAccounts.endpoint (it disables authentication towards that endpoint; emulators and tests only)") + } + var opts []option.ClientOption + if ep := c.ServiceAccounts.Endpoint; ep != "" { + opts = append(opts, option.WithEndpoint(ep)) + } + if c.ServiceAccounts.Insecure { + opts = append(opts, option.WithoutAuthentication()) + } + iamsvc, err = iam.NewService(context.Background(), opts...) + if err != nil { + return nil, fmt.Errorf("gcs config: IAM client init for serviceAccounts: %w", err) + } + } + + return &Driver{cfg: &c, client: cl, iamsvc: iamsvc}, nil } // Driver implements registry.Driver for Google Cloud Storage. type Driver struct { cfg *Config client *storage.Client + // iamsvc is non-nil iff cfg.ServiceAccounts is set. + iamsvc *iam.Service } func (d *Driver) Name() string { return DriverName } @@ -189,6 +263,16 @@ func (d *Driver) InspectBuckety(ctx context.Context, name string) (registry.Insp } func (d *Driver) EnsureBuckety(ctx context.Context, req registry.EnsureRequest) error { + if err := d.ensureBucket(ctx, req); err != nil { + return err + } + if sa := req.Parameters["serviceAccount"]; sa != "" { + return d.ensureServiceAccount(ctx, sa, req.Name) + } + return nil +} + +func (d *Driver) ensureBucket(ctx context.Context, req registry.EnsureRequest) error { bkt := d.client.Bucket(req.Name) attrs, err := bkt.Attrs(ctx) switch { @@ -253,7 +337,21 @@ func (d *Driver) reconcileExisting(ctx context.Context, bkt *storage.BucketHandl // obstacle to work around. Soft-deleted objects do not block // bucket deletion; with a soft delete policy the bucket itself // remains restorable for the configured window. -func (d *Driver) DeleteBuckety(ctx context.Context, name string) error { +func (d *Driver) DeleteBuckety(ctx context.Context, req registry.DeleteRequest) error { + if err := d.deleteBucket(ctx, req.Name); err != nil { + return err + } + // The per-bucket service account goes with the bucket. Only + // after the bucket is fully gone: earlier passes return + // ErrDeletionInProgress above, and a bucket that fails + // deletion keeps its data-plane identity intact. + if sa := req.Parameters["serviceAccount"]; sa != "" && d.iamsvc != nil { + return d.deleteServiceAccount(ctx, sa, req.Name) + } + return nil +} + +func (d *Driver) deleteBucket(ctx context.Context, name string) error { noList := false live, err := d.emptyBucketSlice(ctx, name) switch { @@ -364,20 +462,31 @@ func (d *Driver) emptyBucketSlice(ctx context.Context, name string) (int, error) } // GrantAccess returns the gcs Secret payload for a BucketyAccess: -// the backend's static HMAC pair, identical for all roles. -// Scoped=false signals the reconciler to surface -// ScopingNotImplemented for non-ReadWrite roles. +// the backend's static HMAC pair, identical for all roles, plus - +// when the Buckety opted into a per-bucket service account - one +// user-managed SA key minted for THIS access. Scoped stays false +// either way: the SA is scoped to the bucket, not to the role, so +// the reconciler still surfaces ScopingNotImplemented for +// non-ReadWrite roles. // // Secret keys per SPEC §Secret output > gcs driver: // // endpoint, bucket, project, region (when known), -// accessKeyID, secretAccessKey +// accessKeyID, secretAccessKey, +// serviceAccountKey, serviceAccountEmail, serviceAccountKeyId +// (the last three only with parameters.serviceAccount) // // `bucket` is the resource-type key per the SPEC's stable // per-driver convention. endpoint/region are derived from the // bucket's location unless the backend config overrides them // (issue #14: signing for a EUROPE-WEST4 bucket against the // global host breaks SigV4 and data residency). +// +// The SA key private material is only retrievable at creation, so +// the key found in ExistingSecretData is returned unchanged while +// keys.list still verifies it (see ensureAccessKey); Principal is +// the key's full resource name, which is what RevokeAccess +// deletes. func (d *Driver) GrantAccess(ctx context.Context, req registry.GrantRequest) (registry.GrantResult, error) { endpoint, region := d.cfg.Endpoint, d.cfg.Region if endpoint == "" { @@ -391,19 +500,57 @@ func (d *Driver) GrantAccess(ctx context.Context, req registry.GrantRequest) (re } } data := map[string][]byte{ - "endpoint": []byte(endpoint), - "bucket": []byte(req.BucketyName), - "project": []byte(d.cfg.Project), - "accessKeyID": []byte(d.cfg.AccessKeyID), - "secretAccessKey": []byte(d.cfg.SecretAccessKey), + "endpoint": []byte(endpoint), + "bucket": []byte(req.BucketyName), + "project": []byte(d.cfg.Project), + } + // hmac=false opts this bucket's Secrets out of the static + // backend-wide pair - typically together with serviceAccount, + // whose blast-radius win the shared pair would otherwise + // undo, but a coordinates-only Secret is also legitimate for + // consumers with ambient credentials (Workload Identity). The + // DRIVER default stays true: the pair is the incumbent + // contract (family-portable S3-interop Secrets, minor-bump + // key stability); a backend imposes the opt-in posture by + // declaring hmac "false" in its parameter defaults. + includeHMAC := true + if v, ok := req.BucketyParameters["hmac"]; ok { + if b, err := strconv.ParseBool(v); err == nil { + includeHMAC = b + } + } + if includeHMAC { + data["accessKeyID"] = []byte(d.cfg.AccessKeyID) + data["secretAccessKey"] = []byte(d.cfg.SecretAccessKey) } if region != "" { data["region"] = []byte(region) } + principal, revocable := "gcs-static", false + if sa := req.BucketyParameters["serviceAccount"]; sa != "" { + if d.iamsvc == nil { + // Validation rejects the parameter on non-enabled + // backends; erroring rather than silently dropping the + // SA keys covers a backend whose config lost the block + // after resources adopted it. + return registry.GrantResult{}, fmt.Errorf("gcs: bucket %q declares parameters.serviceAccount but this backend has serviceAccounts disabled", req.BucketyName) + } + email := d.saEmail(sa) + keyJSON, keyID, err := d.ensureAccessKey(ctx, email, req.ExistingSecretData) + if err != nil { + return registry.GrantResult{}, err + } + data["serviceAccountKey"] = keyJSON + data["serviceAccountEmail"] = []byte(email) + data["serviceAccountKeyId"] = []byte(keyID) + principal = d.saResource(email) + "/keys/" + keyID + revocable = true + } return registry.GrantResult{ SecretData: data, - Principal: "gcs-static", + Principal: principal, Scoped: false, + Revocable: revocable, }, nil } @@ -423,9 +570,24 @@ func locationEndpoint(location string) (endpoint, region string) { return globalEndpoint, "" } -// RevokeAccess is a no-op in v0.1 (nothing to remove since there -// is no per-access principal). -func (d *Driver) RevokeAccess(_ context.Context, _ string) error { return nil } +// RevokeAccess deletes the access's SA key when the principal is +// one (full key resource name, stamped by GrantAccess). The +// static-HMAC principal "gcs-static" has nothing backend-side to +// remove. Idempotent on NotFound - which also covers the key +// having gone with its already-deleted service account. +func (d *Driver) RevokeAccess(ctx context.Context, principal string) error { + if !strings.Contains(principal, "/keys/") { + return nil + } + if d.iamsvc == nil { + return fmt.Errorf("gcs: principal %q is a service account key but this backend has serviceAccounts disabled; re-enable it in the backend config so the key can be revoked", principal) + } + _, err := d.iamsvc.Projects.ServiceAccounts.Keys.Delete(principal).Context(ctx).Do() + if err == nil || isNotFound(err) { + return nil + } + return fmt.Errorf("gcs: delete service account key %q: %w", principal, err) +} // ValidateParameters accepts the driver-known keys. No internal // defaults per SPEC §Parameters: omitted keys leave the backend @@ -454,16 +616,40 @@ func (d *Driver) ValidateParameters(params map[string]string) error { if _, err := parseLabels(v); err != nil { return fmt.Errorf("parameters.labels: %w", err) } + case "serviceAccount": + if v == "" { + // Explicit opt-out: a CR clearing a backend + // parameter default. Valid regardless of the + // backend's serviceAccounts gate, since it asks for + // nothing. + continue + } + if d.cfg.ServiceAccounts == nil { + return fmt.Errorf("parameters.serviceAccount requires serviceAccounts to be enabled in this backend's config") + } + if !saNameRE.MatchString(v) { + return fmt.Errorf("parameters.serviceAccount %q must be a valid service account ID: 6-30 characters of lowercase letters, digits and hyphens, starting with a letter and ending alphanumeric (include the namespace, e.g. via the ${name}-${namespace} template, for project-wide uniqueness)", v) + } + case "hmac": + if _, err := strconv.ParseBool(v); err != nil { + return fmt.Errorf("parameters.%s: want \"true\" or \"false\", got %q", k, v) + } default: - return fmt.Errorf("unknown parameter %q (gcs v0.1 accepts: location, uniformBucketLevelAccess, versioning, lifecycle, softDeleteRetentionSeconds, labels)", k) + return fmt.Errorf("unknown parameter %q (gcs v0.2 accepts: location, uniformBucketLevelAccess, versioning, lifecycle, softDeleteRetentionSeconds, labels, hmac, and serviceAccount when serviceAccounts=enabled)", k) } } return nil } -// ValidateUpdateParameters: location is set-at-create and -// immutable; any change (including adding or removing the key) is -// a rejection, because the backend cannot move a bucket in place. +// ValidateUpdateParameters: location and serviceAccount are +// set-at-create and immutable; any change (including adding or +// removing the key) is a rejection. Location because the backend +// cannot move a bucket in place; serviceAccount because scoping +// out in-place transitions (SA rename orphaning bindings, add/ +// remove migrating live credentials under consumers) is what +// keeps the feature's lifecycle tractable - recreate the Buckety +// (retentionPolicy=Retain + adoption keeps the bucket) to change +// it. func (d *Driver) ValidateUpdateParameters(oldParams, newParams map[string]string) error { if err := d.ValidateParameters(newParams); err != nil { return err @@ -471,9 +657,29 @@ func (d *Driver) ValidateUpdateParameters(oldParams, newParams map[string]string if o, n := oldParams["location"], newParams["location"]; o != n { return fmt.Errorf("parameters.location is immutable post-create (current=%q, requested=%q)", o, n) } + if o, n := oldParams["serviceAccount"], newParams["serviceAccount"]; o != n { + return fmt.Errorf("parameters.serviceAccount is immutable post-create (current=%q, requested=%q); recreate the Buckety with retentionPolicy=Retain + adoption to change it", o, n) + } return nil } +// TemplatedParameters declares serviceAccount template-resolved +// (registry.TemplatedParameters), so the recommended uniqueness +// convention is written once as a backend parameter default: +// +// parameters: +// serviceAccount: ${name}-${namespace} +// +// Declared only when the backend enables the feature, so a +// serviceAccount default on a non-enabled backend fails startup +// validation instead of deferring to per-resource errors. +func (d *Driver) TemplatedParameters() []string { + if d.cfg.ServiceAccounts == nil { + return nil + } + return []string{"serviceAccount"} +} + func (d *Driver) ValidateAccessParameters(params map[string]string) error { if len(params) == 0 { return nil diff --git a/pkg/drivers/gcs/driver_test.go b/pkg/drivers/gcs/driver_test.go index f3849e6..7d6abf6 100644 --- a/pkg/drivers/gcs/driver_test.go +++ b/pkg/drivers/gcs/driver_test.go @@ -433,7 +433,7 @@ func keysOf(m map[string][]byte) []string { // whole-CR schemas (schema/) compose from this file, so this is // also their sync guard. func TestParametersSchemaInSync(t *testing.T) { - props := schemaProperties(t, "schema/v0.1/parameters.schema.json") + props := schemaProperties(t, "schema/v0.2/parameters.schema.json") d := &Driver{cfg: &Config{Project: "p"}} // Every schema property must be a code-known key: probing with @@ -450,7 +450,7 @@ func TestParametersSchemaInSync(t *testing.T) { // fails here. for _, key := range acceptedKeysFromError(t, d.ValidateParameters(map[string]string{"definitely-not-a-parameter": "x"})) { if _, ok := props[key]; !ok { - t.Errorf("ValidateParameters advertises %q but schema/v0.1/parameters.schema.json does not list it", key) + t.Errorf("ValidateParameters advertises %q but schema/v0.2/parameters.schema.json does not list it", key) } } diff --git a/pkg/drivers/gcs/schema/v0.1/config.schema.json b/pkg/drivers/gcs/schema/v0.2/config.schema.json similarity index 58% rename from pkg/drivers/gcs/schema/v0.1/config.schema.json rename to pkg/drivers/gcs/schema/v0.2/config.schema.json index 052388d..7acb9d7 100644 --- a/pkg/drivers/gcs/schema/v0.1/config.schema.json +++ b/pkg/drivers/gcs/schema/v0.2/config.schema.json @@ -28,6 +28,27 @@ "type": "string", "minLength": 1, "description": "Static HMAC secret copied into every access Secret. envsubst:\"true\" - supports ${VAR} interpolation at controller startup." + }, + "serviceAccounts": { + "type": "object", + "additionalProperties": false, + "required": ["project"], + "description": "Opts this backend into per-bucket GCP service accounts (parameters.serviceAccount). Omitted (the default) rejects the parameter and keeps the controller free of IAM API calls and permissions. When set, the controller additionally needs roles/iam.serviceAccountAdmin and roles/iam.serviceAccountKeyAdmin on the serviceAccounts project, and storage.buckets.getIamPolicy/setIamPolicy on the bucket project.", + "properties": { + "project": { + "type": "string", + "minLength": 1, + "description": "GCP project the per-bucket service accounts are created in. STRONGLY RECOMMENDED to be a dedicated identity project, separate from the bucket project: creating a key on an SA equals impersonating it, so the controller's serviceAccountAdmin/serviceAccountKeyAdmin grants must be confined to a project whose only identities are the ones buckety mints. Cross-project bucket IAM bindings make the split free - SAs here bind onto buckets in the main project without the controller holding IAM-admin rights there." + }, + "endpoint": { + "type": "string", + "description": "Overrides the IAM API endpoint (full URL), e.g. for Private Service Connect. Authentication stays on unless insecure is also set. Unset reaches iam.googleapis.com with ADC." + }, + "insecure": { + "type": "boolean", + "description": "Disables authentication towards endpoint - emulators and tests ONLY. A separate explicit flag so an endpoint typo cannot silently turn credentials off; requires endpoint." + } + } } } } diff --git a/pkg/drivers/gcs/schema/v0.1/parameters.schema.json b/pkg/drivers/gcs/schema/v0.2/parameters.schema.json similarity index 58% rename from pkg/drivers/gcs/schema/v0.1/parameters.schema.json rename to pkg/drivers/gcs/schema/v0.2/parameters.schema.json index 6047329..0729696 100644 --- a/pkg/drivers/gcs/schema/v0.1/parameters.schema.json +++ b/pkg/drivers/gcs/schema/v0.2/parameters.schema.json @@ -31,6 +31,17 @@ "labels": { "type": "string", "description": "Bucket labels as a JSON object of string values, e.g. {\"site\": \"tenant1\", \"managed-by\": \"buckety\"}, for cost attribution and ownership. Listed labels are converged to their declared values; labels absent from the parameter are unmanaged and never deleted (same posture as unlisted parameters). Key/value charset rules are enforced by the backend." + }, + "serviceAccount": { + "type": "string", + "anyOf": [{"const": ""}, {"minLength": 6, "maxLength": 30}], + "x-buckety-templated": true, + "description": "Opt-in per-bucket GCP service account, as the SA's short account ID: 6-30 characters of lowercase letters, digits and hyphens, starting with a letter and ending alphanumeric. Requires serviceAccounts enabled in the backend config. The driver maintains an SA of this name in the backend's identity project with roles/storage.objectAdmin on this bucket only, and each BucketyAccess Secret additionally carries serviceAccountKey (the SA key JSON for OAuth2 bearer-token auth), serviceAccountEmail and serviceAccountKeyId. Template-resolved with the restricted grammar (${name}, ${namespace}, ${backend.X} - no labels); include the namespace, e.g. ${name}-${namespace}, since SA IDs are unique per project. The empty string is an explicit per-CR opt-out of a backend parameter default. Set-at-create and immutable either way: recreate the Buckety (retentionPolicy=Retain + adoption keeps the bucket) to change it. NOTE a backend parameter default here applies to EXISTING Bucketys on the backend's next controller rollout - defaults merge on every reconcile and never re-pass admission, so audit resolved-name lengths fleet-wide before adding one." + }, + "hmac": { + "type": "string", + "enum": ["true", "false"], + "description": "Whether access Secrets carry the backend's static S3-interop HMAC pair (accessKeyID/secretAccessKey). Omitted means \"true\" - the incumbent contract; gcs Secrets are S3-protocol portable by default and a minor driver bump must not remove keys. Declare \"false\" to mint Secrets without the backend-wide pair, typically together with serviceAccount so consumers hold ONLY the bucket-scoped identity; without serviceAccount the Secret carries bucket coordinates only, for consumers with ambient credentials. A backend can impose the opt-in posture fleet-wide by declaring \"false\" in its parameter defaults; CRs override per key. Mutable: flipping it rewrites Secrets in place on the next reconcile (file-mounted consumers pick it up, env-based ones need a restart)." } } } diff --git a/pkg/drivers/gcs/serviceaccount.go b/pkg/drivers/gcs/serviceaccount.go new file mode 100644 index 0000000..de4e31a --- /dev/null +++ b/pkg/drivers/gcs/serviceaccount.go @@ -0,0 +1,227 @@ +// Per-bucket service accounts (Config.ServiceAccounts): the +// control-plane side of parameters.serviceAccount. EnsureBuckety +// maintains the SA and its bucket-level binding, GrantAccess +// mints one user-managed key per BucketyAccess (driver.go), and +// DeleteBuckety tears the SA down with the bucket. +// +// The ownership marker is the security boundary here: a tenant +// who names another Buckety's SA in parameters.serviceAccount +// must not get keys for it - a key equals impersonation, and the +// foreign SA holds bindings on the foreign bucket. Every +// mutating path therefore verifies the marker first and refuses +// SAs it did not create for exactly this bucket. +package gcs + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "regexp" + "strings" + "time" + + iam "google.golang.org/api/iam/v1" +) + +// saNameRE is GCP's service account ID rule: 6-30 characters, +// lowercase letters, digits and hyphens, starting with a letter, +// ending alphanumeric. +var saNameRE = regexp.MustCompile(`^[a-z][-a-z0-9]{4,28}[a-z0-9]$`) + +// saBucketRole is the bucket-level grant for the per-bucket SA. +// objectAdmin (get/list/create/delete objects), matching what the +// static HMAC pair's backing SA is documented to need; per-role +// scoping (ReadOnly -> objectViewer) is the per-access-SA design +// deferred with the rest of v1alpha2 scoping. +const saBucketRole = "roles/storage.objectAdmin" + +// saMarker is the ownership stamp serialized into the service +// account's description at creation. +type saMarker struct { + ManagedBy string `json:"managedBy"` + Bucket string `json:"bucket"` +} + +func (d *Driver) saEmail(shortName string) string { + return shortName + "@" + d.cfg.ServiceAccounts.Project + ".iam.gserviceaccount.com" +} + +// saResource is the IAM API resource name for the SA. The +// explicit project (not the "-" wildcard) keeps every call +// confined to the configured identity project. +func (d *Driver) saResource(email string) string { + return "projects/" + d.cfg.ServiceAccounts.Project + "/serviceAccounts/" + email +} + +// ensureServiceAccount creates-or-verifies the bucket's SA and +// re-asserts its bucket-level binding. Re-asserting every +// reconcile is what heals the binding after out-of-band edits and +// after SA delete+recreate (a recreated SA is a new identity to +// IAM; stale bindings never revive on their own). +func (d *Driver) ensureServiceAccount(ctx context.Context, shortName, bucket string) error { + if d.iamsvc == nil { + return fmt.Errorf("gcs: bucket %q declares parameters.serviceAccount but this backend has serviceAccounts disabled", bucket) + } + email := d.saEmail(shortName) + resource := d.saResource(email) + + sa, err := d.iamsvc.Projects.ServiceAccounts.Get(resource).Context(ctx).Do() + if isNotFound(err) { + marker, merr := json.Marshal(saMarker{ManagedBy: "buckety", Bucket: bucket}) + if merr != nil { + return merr + } + sa, err = d.iamsvc.Projects.ServiceAccounts.Create("projects/"+d.cfg.ServiceAccounts.Project, &iam.CreateServiceAccountRequest{ + AccountId: shortName, + ServiceAccount: &iam.ServiceAccount{ + DisplayName: "buckety bucket " + bucket, + Description: string(marker), + }, + }).Context(ctx).Do() + if isConflict(err) { + // Raced another creator; the re-fetched marker decides + // whether it is ours. + sa, err = d.iamsvc.Projects.ServiceAccounts.Get(resource).Context(ctx).Do() + if isNotFound(err) { + // Create conflicts while Get sees nothing: a + // soft-deleted SA holds the name. GCP reserves a + // deleted SA's ID for ~30 days, so this state does + // not converge on retries and the generic create + // error would hide the actual cause (checkit review + // finding 2). + return fmt.Errorf("gcs: service account ID %q is reserved by a recently deleted account; GCP holds deleted SA names for ~30 days. Wait out the window, undelete it if its numeric unique ID is known (gcloud iam service-accounts undelete), or use a different parameters.serviceAccount", email) + } + } + if err != nil { + return fmt.Errorf("gcs: create service account %q (needs roles/iam.serviceAccountAdmin on project %q): %w", email, d.cfg.ServiceAccounts.Project, err) + } + } else if err != nil { + return fmt.Errorf("gcs: get service account %q: %w", email, err) + } + if err := verifySAMarker(sa, bucket); err != nil { + return err + } + return d.ensureBucketBinding(ctx, bucket, email) +} + +// verifySAMarker enforces the ownership rule: the driver touches +// (binds, mints keys for, deletes) only SAs whose description +// carries its marker for exactly this bucket. +func verifySAMarker(sa *iam.ServiceAccount, bucket string) error { + var m saMarker + if json.Unmarshal([]byte(sa.Description), &m) != nil || m.ManagedBy != "buckety" || m.Bucket != bucket { + return fmt.Errorf("gcs: service account %q exists but was not created by buckety for bucket %q; refusing to touch it - choose another parameters.serviceAccount", sa.Email, bucket) + } + return nil +} + +// ensureBucketBinding grants the SA saBucketRole on the bucket, +// skipping the write when the binding is already present. +// SetPolicy is etag-guarded read-modify-write; a concurrent +// policy edit surfaces as a conflict error and the next reconcile +// retries. +func (d *Driver) ensureBucketBinding(ctx context.Context, bucket, email string) error { + handle := d.client.Bucket(bucket).IAM() + policy, err := handle.Policy(ctx) + if err != nil { + return fmt.Errorf("gcs: read IAM policy of bucket %q (needs storage.buckets.getIamPolicy): %w", bucket, err) + } + member := "serviceAccount:" + email + if policy.HasRole(member, saBucketRole) { + return nil + } + policy.Add(member, saBucketRole) + if err := handle.SetPolicy(ctx, policy); err != nil { + return fmt.Errorf("gcs: grant %s to %s on bucket %q (needs storage.buckets.setIamPolicy): %w", saBucketRole, member, bucket, err) + } + return nil +} + +// 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). +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 { + var k struct { + ClientEmail string `json:"client_email"` + PrivateKeyID string `json:"private_key_id"` + } + if json.Unmarshal(raw, &k) == nil && k.ClientEmail == email && k.PrivateKeyID != "" { + resp, err := d.iamsvc.Projects.ServiceAccounts.Keys.List(resource).KeyTypes("USER_MANAGED").Context(ctx).Do() + if err != nil { + return nil, "", fmt.Errorf("gcs: list keys of %q: %w", email, err) + } + for _, key := range resp.Keys { + if strings.HasSuffix(key.Name, "/keys/"+k.PrivateKeyID) && !keyExpired(key) { + 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. + } + key, err := d.iamsvc.Projects.ServiceAccounts.Keys.Create(resource, &iam.CreateServiceAccountKeyRequest{}).Context(ctx).Do() + if err != nil { + return nil, "", fmt.Errorf("gcs: create key for %q (needs roles/iam.serviceAccountKeyAdmin; the org policy constraints/iam.disableServiceAccountKeyCreation blocks user-managed keys entirely): %w", email, err) + } + keyJSON, err := base64.StdEncoding.DecodeString(key.PrivateKeyData) + if err != nil { + return nil, "", fmt.Errorf("gcs: decode created key for %q: %w", email, err) + } + id := key.Name[strings.LastIndex(key.Name, "/")+1:] + return keyJSON, id, nil +} + +// keyExpired reports whether the key's validity window has +// passed. Non-expiring keys carry a far-future (year 9999) +// validBeforeTime; an org-policy expiry +// (constraints/iam.serviceAccountKeyExpiryHours) shows up here, +// and treating such keys as invalid re-mints within one reconcile +// of expiry - degraded (the Secret holds a dead key for up to the +// requeue cadence) but converging. Proactive renewal is the +// scheduled-rotation roadmap item. +func keyExpired(key *iam.ServiceAccountKey) bool { + if key.ValidBeforeTime == "" { + return false + } + t, err := time.Parse(time.RFC3339, key.ValidBeforeTime) + if err != nil { + return false + } + return time.Now().After(t) +} + +// deleteServiceAccount removes the bucket's SA at bucket +// deletion. Idempotent on NotFound. An SA that fails the marker +// check is left alone WITHOUT blocking: it was never ours +// (ensureServiceAccount refused it too, so nothing was minted), +// and the immutable serviceAccount parameter would otherwise +// wedge the finalizer with no spec-side fix. +func (d *Driver) deleteServiceAccount(ctx context.Context, shortName, bucket string) error { + resource := d.saResource(d.saEmail(shortName)) + sa, err := d.iamsvc.Projects.ServiceAccounts.Get(resource).Context(ctx).Do() + if isNotFound(err) { + return nil + } + if err != nil { + return fmt.Errorf("gcs: get service account for deletion: %w", err) + } + if verifySAMarker(sa, bucket) != nil { + return nil + } + _, err = d.iamsvc.Projects.ServiceAccounts.Delete(resource).Context(ctx).Do() + if err == nil || isNotFound(err) { + return nil + } + return fmt.Errorf("gcs: delete service account %q: %w", sa.Email, err) +} diff --git a/pkg/drivers/gcs/serviceaccount_test.go b/pkg/drivers/gcs/serviceaccount_test.go new file mode 100644 index 0000000..d73b6b6 --- /dev/null +++ b/pkg/drivers/gcs/serviceaccount_test.go @@ -0,0 +1,707 @@ +package gcs + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + iam "google.golang.org/api/iam/v1" + + "github.com/Yolean/buckety-controller/pkg/drivers/registry" +) + +// fakeGCP fakes the two control-plane surfaces the serviceAccounts +// feature touches: iam.googleapis.com (SA + key lifecycle) and the +// storage JSON API's bucket IAM policy endpoints. fake-gcs-server +// implements neither, which is why this feature is unit-tested +// here and documented as not-e2e-gated (SPEC §E2E harness). +type fakeGCP struct { + mu sync.Mutex + project string + sas map[string]*iam.ServiceAccount // email -> SA + keys map[string]map[string]*iam.ServiceAccountKey // email -> key id -> key + keySeq int + policies map[string][]*policyBinding // bucket -> bindings + setPolicyCalls int + // tombstoned emails 404 on Get but still 409 on Create, + // GCP's ~30-day soft-deletion name reservation. + tombstoned map[string]bool +} + +type policyBinding struct { + Role string `json:"role"` + Members []string `json:"members"` +} + +func newFakeGCP(t *testing.T, project string) (*fakeGCP, *httptest.Server) { + t.Helper() + f := &fakeGCP{ + project: project, + sas: map[string]*iam.ServiceAccount{}, + keys: map[string]map[string]*iam.ServiceAccountKey{}, + policies: map[string][]*policyBinding{}, + tombstoned: map[string]bool{}, + } + mux := http.NewServeMux() + + writeJSON := func(w http.ResponseWriter, v any) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(v) + } + writeErr := func(w http.ResponseWriter, code int, msg string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(code) + _ = json.NewEncoder(w).Encode(map[string]any{ + "error": map[string]any{"code": code, "message": msg}, + }) + } + + mux.HandleFunc("POST /v1/projects/{proj}/serviceAccounts", func(w http.ResponseWriter, r *http.Request) { + var req iam.CreateServiceAccountRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + writeErr(w, 400, err.Error()) + return + } + email := req.AccountId + "@" + r.PathValue("proj") + ".iam.gserviceaccount.com" + f.mu.Lock() + defer f.mu.Unlock() + if _, exists := f.sas[email]; exists || f.tombstoned[email] { + writeErr(w, 409, "already exists") + return + } + sa := &iam.ServiceAccount{ + Name: "projects/" + r.PathValue("proj") + "/serviceAccounts/" + email, + Email: email, + } + if req.ServiceAccount != nil { + sa.DisplayName = req.ServiceAccount.DisplayName + sa.Description = req.ServiceAccount.Description + } + f.sas[email] = sa + writeJSON(w, sa) + }) + mux.HandleFunc("GET /v1/projects/{proj}/serviceAccounts/{email}", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + sa, ok := f.sas[r.PathValue("email")] + if !ok { + writeErr(w, 404, "no such service account") + return + } + writeJSON(w, sa) + }) + mux.HandleFunc("DELETE /v1/projects/{proj}/serviceAccounts/{email}", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + email := r.PathValue("email") + if _, ok := f.sas[email]; !ok { + writeErr(w, 404, "no such service account") + return + } + delete(f.sas, email) + delete(f.keys, email) + writeJSON(w, map[string]any{}) + }) + mux.HandleFunc("GET /v1/projects/{proj}/serviceAccounts/{email}/keys", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + email := r.PathValue("email") + if _, ok := f.sas[email]; !ok { + writeErr(w, 404, "no such service account") + return + } + resp := iam.ListServiceAccountKeysResponse{} + for _, k := range f.keys[email] { + resp.Keys = append(resp.Keys, k) + } + writeJSON(w, resp) + }) + mux.HandleFunc("POST /v1/projects/{proj}/serviceAccounts/{email}/keys", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + email := r.PathValue("email") + if _, ok := f.sas[email]; !ok { + writeErr(w, 404, "no such service account") + return + } + f.keySeq++ + id := fmt.Sprintf("key%d", f.keySeq) + keyfile, _ := json.Marshal(map[string]string{ + "type": "service_account", + "project_id": r.PathValue("proj"), + "private_key_id": id, + "private_key": "-----BEGIN PRIVATE KEY-----\nfake\n-----END PRIVATE KEY-----\n", + "client_email": email, + }) + key := &iam.ServiceAccountKey{ + Name: "projects/" + r.PathValue("proj") + "/serviceAccounts/" + email + "/keys/" + id, + PrivateKeyData: base64.StdEncoding.EncodeToString(keyfile), + ValidAfterTime: time.Now().UTC().Format(time.RFC3339), + ValidBeforeTime: "9999-12-31T23:59:59Z", + KeyType: "USER_MANAGED", + } + if f.keys[email] == nil { + f.keys[email] = map[string]*iam.ServiceAccountKey{} + } + f.keys[email][id] = key + writeJSON(w, key) + }) + mux.HandleFunc("DELETE /v1/projects/{proj}/serviceAccounts/{email}/keys/{id}", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + email, id := r.PathValue("email"), r.PathValue("id") + if _, ok := f.keys[email][id]; !ok { + writeErr(w, 404, "no such key") + return + } + delete(f.keys[email], id) + writeJSON(w, map[string]any{}) + }) + + mux.HandleFunc("GET /storage/v1/b/{bucket}/iam", func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + writeJSON(w, map[string]any{ + "kind": "storage#policy", + "resourceId": "projects/_/buckets/" + r.PathValue("bucket"), + "bindings": f.policies[r.PathValue("bucket")], + "etag": "CAE=", + }) + }) + mux.HandleFunc("PUT /storage/v1/b/{bucket}/iam", func(w http.ResponseWriter, r *http.Request) { + var body struct { + Bindings []*policyBinding `json:"bindings"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + writeErr(w, 400, err.Error()) + return + } + f.mu.Lock() + defer f.mu.Unlock() + f.policies[r.PathValue("bucket")] = body.Bindings + f.setPolicyCalls++ + writeJSON(w, map[string]any{ + "kind": "storage#policy", + "bindings": body.Bindings, + "etag": "CAI=", + }) + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return f, srv +} + +// saDriver builds a Driver through the factory, both clients +// pointed at the fake. The endpoint override keeps GrantAccess off +// the bucket-attrs lookup, matching TestGrantAccessPayload. +func saDriver(t *testing.T, srv *httptest.Server) *Driver { + t.Helper() + t.Setenv("STORAGE_EMULATOR_HOST", srv.URL) + raw := fmt.Sprintf(`{ + "project": "bucket-proj", "endpoint": "fake-gcs:8000", "region": "r1", + "accessKeyID": "id", "secretAccessKey": "sec", + "serviceAccounts": {"project": "id-proj", "endpoint": %q, "insecure": true} + }`, srv.URL) + drv, err := factory(json.RawMessage(raw)) + if err != nil { + t.Fatalf("factory: %v", err) + } + return drv.(*Driver) +} + +func (f *fakeGCP) hasBinding(bucket, role, member string) bool { + f.mu.Lock() + defer f.mu.Unlock() + for _, b := range f.policies[bucket] { + if b.Role != role { + continue + } + for _, m := range b.Members { + if m == member { + return true + } + } + } + return false +} + +func (f *fakeGCP) keyIDs(email string) []string { + f.mu.Lock() + defer f.mu.Unlock() + var out []string + for id := range f.keys[email] { + out = append(out, id) + } + return out +} + +func TestServiceAccountConfigValidation(t *testing.T) { + t.Setenv("STORAGE_EMULATOR_HOST", "127.0.0.1:1") + if _, err := factory(json.RawMessage(`{"project": "p", "accessKeyID": "a", "secretAccessKey": "s", "serviceAccounts": {}}`)); err == nil { + t.Error("serviceAccounts without project accepted") + } + // insecure is only meaningful together with endpoint; alone it + // is a config error, not a silent no-op. + if _, err := factory(json.RawMessage(`{"project": "p", "accessKeyID": "a", "secretAccessKey": "s", "serviceAccounts": {"project": "id-proj", "insecure": true}}`)); err == nil { + t.Error("serviceAccounts.insecure without endpoint accepted") + } + drv, err := factory(json.RawMessage(`{"project": "p", "accessKeyID": "a", "secretAccessKey": "s", "serviceAccounts": {"project": "id-proj", "endpoint": "http://127.0.0.1:1", "insecure": true}}`)) + if err != nil { + t.Fatalf("valid serviceAccounts config rejected: %v", err) + } + if got := registry.TemplatedParameters(drv); len(got) != 1 || got[0] != "serviceAccount" { + t.Errorf("TemplatedParameters with feature on: %v", got) + } + + // Feature off: parameter rejected, nothing declared templated. + off, err := factory(json.RawMessage(`{"project": "p", "accessKeyID": "a", "secretAccessKey": "s"}`)) + if err != nil { + t.Fatal(err) + } + if got := registry.TemplatedParameters(off); got != nil { + t.Errorf("TemplatedParameters with feature off: %v", got) + } + if err := off.ValidateParameters(map[string]string{"serviceAccount": "orders-t1"}); err == nil || !strings.Contains(err.Error(), "serviceAccounts") { + t.Errorf("parameter on non-enabled backend: %v", err) + } +} + +func TestValidateServiceAccountParameter(t *testing.T) { + d := &Driver{cfg: &Config{Project: "p", ServiceAccounts: &ServiceAccountsConfig{Project: "id-proj"}}} + + valid := []string{"orders", "orders-tenant1", "a-b-c-1", "a" + strings.Repeat("b", 29)} + for _, v := range valid { + if err := d.ValidateParameters(map[string]string{"serviceAccount": v}); err != nil { + t.Errorf("valid %q rejected: %v", v, err) + } + } + invalid := []string{ + "short", // 5 chars, below GCP's 6 minimum + strings.Repeat("a", 31), // above 30 + "1leading-digit", // must start with a letter + "trailing-dash-", // must end alphanumeric + "Upper-case", // charset + "under_score", // underscores illegal in SA IDs + "${name}-${namespace}", // unresolved template must not reach the driver + } + for _, v := range invalid { + if err := d.ValidateParameters(map[string]string{"serviceAccount": v}); err == nil { + t.Errorf("invalid %q accepted", v) + } + } +} + +func TestServiceAccountImmutable(t *testing.T) { + d := &Driver{cfg: &Config{Project: "p", ServiceAccounts: &ServiceAccountsConfig{Project: "id-proj"}}} + old := map[string]string{"serviceAccount": "orders-t1"} + + if err := d.ValidateUpdateParameters(old, map[string]string{"serviceAccount": "orders-t1", "versioning": "true"}); err != nil { + t.Fatalf("unchanged serviceAccount rejected: %v", err) + } + if err := d.ValidateUpdateParameters(old, map[string]string{"serviceAccount": "other-name"}); err == nil { + t.Error("serviceAccount change accepted") + } + if err := d.ValidateUpdateParameters(old, map[string]string{}); err == nil { + t.Error("serviceAccount removal accepted") + } + if err := d.ValidateUpdateParameters(map[string]string{}, old); err == nil { + t.Error("serviceAccount addition post-create accepted") + } +} + +func TestEnsureServiceAccount(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + ctx := context.Background() + email := "orders-t1@id-proj.iam.gserviceaccount.com" + member := "serviceAccount:" + email + + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatalf("first ensure: %v", err) + } + sa := f.sas[email] + if sa == nil { + t.Fatal("service account not created") + } + var m saMarker + if err := json.Unmarshal([]byte(sa.Description), &m); err != nil || m.ManagedBy != "buckety" || m.Bucket != "bucket-x" { + t.Errorf("ownership marker: %q", sa.Description) + } + if !f.hasBinding("bucket-x", saBucketRole, member) { + t.Errorf("bucket binding missing: %+v", f.policies["bucket-x"]) + } + if f.setPolicyCalls != 1 { + t.Errorf("setPolicy calls after create: %d", f.setPolicyCalls) + } + + // Steady state: no policy write. + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatalf("second ensure: %v", err) + } + if f.setPolicyCalls != 1 { + t.Errorf("steady-state ensure wrote the policy: %d calls", f.setPolicyCalls) + } + + // Out-of-band binding removal heals on the next pass. + f.mu.Lock() + f.policies["bucket-x"] = nil + f.mu.Unlock() + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatalf("heal ensure: %v", err) + } + if !f.hasBinding("bucket-x", saBucketRole, member) { + t.Error("binding not re-asserted after out-of-band removal") + } + + // Foreign SA (no buckety marker): refused. This is the + // cross-tenant hijack guard - a key for someone else's SA + // would carry their bucket grants. + f.mu.Lock() + f.sas["victim@id-proj.iam.gserviceaccount.com"] = &iam.ServiceAccount{ + Email: "victim@id-proj.iam.gserviceaccount.com", Description: "hand-made", + } + f.mu.Unlock() + if err := d.ensureServiceAccount(ctx, "victim", "bucket-x"); err == nil || !strings.Contains(err.Error(), "refusing") { + t.Errorf("foreign SA: %v", err) + } + + // Marker for a DIFFERENT bucket: also refused (two Bucketys + // racing for one SA name). + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-y"); err == nil || !strings.Contains(err.Error(), "refusing") { + t.Errorf("other bucket's SA: %v", err) + } +} + +func TestGrantAccessKeyLifecycle(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + ctx := context.Background() + email := "orders-t1@id-proj.iam.gserviceaccount.com" + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatal(err) + } + req := registry.GrantRequest{ + BucketyName: "bucket-x", + Role: "ReadWrite", + BucketyParameters: map[string]string{"serviceAccount": "orders-t1"}, + } + + // First grant mints. + res1, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("first grant: %v", err) + } + var keyfile struct { + ClientEmail string `json:"client_email"` + PrivateKeyID string `json:"private_key_id"` + Type string `json:"type"` + } + if err := json.Unmarshal(res1.SecretData["serviceAccountKey"], &keyfile); err != nil { + t.Fatalf("serviceAccountKey not JSON: %v", err) + } + if keyfile.ClientEmail != email || keyfile.Type != "service_account" { + t.Errorf("key file: %+v", keyfile) + } + if string(res1.SecretData["serviceAccountEmail"]) != email { + t.Errorf("serviceAccountEmail: %q", res1.SecretData["serviceAccountEmail"]) + } + keyID := string(res1.SecretData["serviceAccountKeyId"]) + if keyID != keyfile.PrivateKeyID { + t.Errorf("serviceAccountKeyId %q != key file id %q", keyID, keyfile.PrivateKeyID) + } + wantPrincipal := "projects/id-proj/serviceAccounts/" + email + "/keys/" + keyID + if res1.Principal != wantPrincipal { + t.Errorf("principal %q, want %q", res1.Principal, wantPrincipal) + } + if res1.Scoped { + t.Error("bucket-scoped SA must still report Scoped=false (role scoping is not implemented)") + } + if !res1.Revocable { + t.Error("minted key principal must report Revocable=true") + } + // HMAC pair still present (additive keys per SPEC). + if string(res1.SecretData["accessKeyID"]) != "id" || string(res1.SecretData["bucket"]) != "bucket-x" { + t.Errorf("base payload regressed: %v", keysOf(res1.SecretData)) + } + if n := len(f.keyIDs(email)); n != 1 { + t.Fatalf("keys after first grant: %d", n) + } + + // Steady state: the existing Secret's key verifies against + // keys.list and comes back byte-identical - no new key. + req.ExistingSecretData = res1.SecretData + res2, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("steady-state grant: %v", err) + } + if string(res2.SecretData["serviceAccountKey"]) != string(res1.SecretData["serviceAccountKey"]) || res2.Principal != res1.Principal { + t.Error("steady-state grant re-minted") + } + if n := len(f.keyIDs(email)); n != 1 { + 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. + f.mu.Lock() + delete(f.keys[email], keyID) + f.mu.Unlock() + res3, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("post-revocation grant: %v", err) + } + if string(res3.SecretData["serviceAccountKeyId"]) == keyID { + t.Error("revoked key reused") + } + if n := len(f.keyIDs(email)); n != 1 { + t.Fatalf("keys after re-mint: %d", n) + } + + // Expired key (org-policy iam.serviceAccountKeyExpiryHours): + // treated as invalid, 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) + f.mu.Unlock() + res4, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("post-expiry grant: %v", err) + } + if string(res4.SecretData["serviceAccountKeyId"]) == string(res3.SecretData["serviceAccountKeyId"]) { + t.Error("expired key reused") + } + + // 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) + } +} + +func TestRevokeAccessKey(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + ctx := context.Background() + email := "orders-t1@id-proj.iam.gserviceaccount.com" + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatal(err) + } + res, err := d.GrantAccess(ctx, registry.GrantRequest{ + BucketyName: "bucket-x", + BucketyParameters: map[string]string{"serviceAccount": "orders-t1"}, + }) + if err != nil { + t.Fatal(err) + } + + if err := d.RevokeAccess(ctx, res.Principal); err != nil { + t.Fatalf("revoke: %v", err) + } + if n := len(f.keyIDs(email)); n != 0 { + t.Errorf("keys after revoke: %d", n) + } + // Idempotent, and also covers the key having gone with its SA. + if err := d.RevokeAccess(ctx, res.Principal); err != nil { + t.Errorf("second revoke: %v", err) + } + // The static principal and empty principal are no-ops. + if err := d.RevokeAccess(ctx, "gcs-static"); err != nil { + t.Errorf("gcs-static revoke: %v", err) + } + if err := d.RevokeAccess(ctx, ""); err != nil { + t.Errorf("empty principal revoke: %v", err) + } + // A key principal on a feature-disabled backend is an error, + // not a silent leak: deletion blocks until the operator + // restores the serviceAccounts config. + off := &Driver{cfg: &Config{Project: "p"}} + if err := off.RevokeAccess(ctx, res.Principal); err == nil { + t.Error("key revoke with serviceAccounts disabled succeeded silently") + } +} + +func TestDeleteBucketyRemovesServiceAccount(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + ctx := context.Background() + email := "orders-t1@id-proj.iam.gserviceaccount.com" + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatal(err) + } + + // Owned SA goes with the bucket. + if err := d.deleteServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatalf("delete: %v", err) + } + if f.sas[email] != nil { + t.Error("service account not deleted") + } + // Idempotent on absent. + if err := d.deleteServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Errorf("second delete: %v", err) + } + + // Foreign SA is left alone WITHOUT blocking the finalizer. + f.mu.Lock() + f.sas["victim@id-proj.iam.gserviceaccount.com"] = &iam.ServiceAccount{ + Email: "victim@id-proj.iam.gserviceaccount.com", Description: "hand-made", + } + f.mu.Unlock() + if err := d.deleteServiceAccount(ctx, "victim", "bucket-x"); err != nil { + t.Errorf("foreign SA delete attempt errored: %v", err) + } + if f.sas["victim@id-proj.iam.gserviceaccount.com"] == nil { + t.Error("foreign SA deleted") + } +} + +// A soft-deleted SA holds its name for ~30 days: Create conflicts +// while Get sees nothing. That state never converges on retries, +// so the driver must name the tombstone instead of wrapping the +// misleading create/get error (checkit review finding 2). +func TestEnsureServiceAccountTombstone(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + f.mu.Lock() + f.tombstoned["orders-t1@id-proj.iam.gserviceaccount.com"] = true + f.mu.Unlock() + err := d.ensureServiceAccount(context.Background(), "orders-t1", "bucket-x") + if err == nil { + t.Fatal("tombstoned SA creation succeeded") + } + if !strings.Contains(err.Error(), "30 days") || !strings.Contains(err.Error(), "reserved") { + t.Errorf("tombstone diagnostic missing: %v", err) + } +} + +// serviceAccount works as a CR parameter, as a backend default, +// and - the piece this test pins - "" is the explicit per-CR +// opt-out of a backend default. Immutability covers the opt-out +// transition too. +func TestServiceAccountEmptyOptOut(t *testing.T) { + d := &Driver{cfg: &Config{Project: "p", ServiceAccounts: &ServiceAccountsConfig{Project: "id-proj"}}} + if err := d.ValidateParameters(map[string]string{"serviceAccount": ""}); err != nil { + t.Errorf("explicit opt-out rejected: %v", err) + } + // Also valid on a backend without the feature: it asks for + // nothing. + off := &Driver{cfg: &Config{Project: "p"}} + if err := off.ValidateParameters(map[string]string{"serviceAccount": ""}); err != nil { + t.Errorf("opt-out on non-enabled backend rejected: %v", err) + } + // Effective transition default->"" is still a post-create + // mutation and gets rejected like any other change. + if err := d.ValidateUpdateParameters( + map[string]string{"serviceAccount": "orders-t1"}, + map[string]string{"serviceAccount": ""}); err == nil { + t.Error("opt-out transition accepted post-create") + } + // GrantAccess treats "" exactly like absent: HMAC-only Secret. + // Endpoint override skips the bucket-attrs lookup, and the "" + // opt-out must never reach the (nil here) IAM client. + d = &Driver{cfg: &Config{ + Project: "p", Endpoint: "fake:8000", + AccessKeyID: "id", SecretAccessKey: "sec", + ServiceAccounts: &ServiceAccountsConfig{Project: "id-proj"}, + }} + res, err := d.GrantAccess(t.Context(), registry.GrantRequest{ + BucketyName: "bucket-x", + BucketyParameters: map[string]string{"serviceAccount": ""}, + }) + if err != nil { + t.Fatalf("grant with opt-out: %v", err) + } + if _, ok := res.SecretData["serviceAccountKey"]; ok { + t.Error("opt-out minted a key") + } + if res.Principal != "gcs-static" { + t.Errorf("principal: %q", res.Principal) + } + if res.Revocable { + t.Error("static principal must report Revocable=false") + } +} + +// hmac="false" omits the backend-wide static pair from the +// Secret; driver default stays "true" (incumbent contract). +func TestHMACOptOut(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + _ = f + ctx := context.Background() + + if err := d.ValidateParameters(map[string]string{"hmac": "false"}); err != nil { + t.Errorf("hmac false rejected: %v", err) + } + if err := d.ValidateParameters(map[string]string{"hmac": "sometimes"}); err == nil { + t.Error("bad hmac value accepted") + } + + // Default: pair present (compatibility). + res, err := d.GrantAccess(ctx, registry.GrantRequest{BucketyName: "bucket-x"}) + if err != nil { + t.Fatal(err) + } + if _, ok := res.SecretData["accessKeyID"]; !ok { + t.Error("default grant lost the HMAC pair") + } + + // Opt-out with a serviceAccount: SA keys only. + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatal(err) + } + res, err = d.GrantAccess(ctx, registry.GrantRequest{ + BucketyName: "bucket-x", + BucketyParameters: map[string]string{"hmac": "false", "serviceAccount": "orders-t1"}, + }) + if err != nil { + t.Fatal(err) + } + if _, ok := res.SecretData["accessKeyID"]; ok { + t.Error("hmac=false Secret still carries accessKeyID") + } + if _, ok := res.SecretData["secretAccessKey"]; ok { + t.Error("hmac=false Secret still carries secretAccessKey") + } + if _, ok := res.SecretData["serviceAccountKey"]; !ok { + t.Error("hmac=false Secret missing the SA key") + } + // Coordinates survive either way. + if string(res.SecretData["bucket"]) != "bucket-x" || string(res.SecretData["endpoint"]) == "" { + t.Errorf("coordinates missing: %v", keysOf(res.SecretData)) + } + + // Opt-out without serviceAccount: coordinates-only Secret for + // ambient-credential consumers. + res, err = d.GrantAccess(ctx, registry.GrantRequest{ + BucketyName: "bucket-x", + BucketyParameters: map[string]string{"hmac": "false"}, + }) + if err != nil { + t.Fatal(err) + } + for _, k := range []string{"accessKeyID", "secretAccessKey", "serviceAccountKey"} { + if _, ok := res.SecretData[k]; ok { + t.Errorf("coordinates-only Secret carries %s", k) + } + } +} diff --git a/pkg/drivers/kadm/driver.go b/pkg/drivers/kadm/driver.go index 0f5e243..6c93811 100644 --- a/pkg/drivers/kadm/driver.go +++ b/pkg/drivers/kadm/driver.go @@ -130,7 +130,8 @@ func (d *Driver) EnsureBuckety(ctx context.Context, req registry.EnsureRequest) } // DeleteBuckety removes the topic. Idempotent on NotFound. -func (d *Driver) DeleteBuckety(ctx context.Context, name string) error { +func (d *Driver) DeleteBuckety(ctx context.Context, req registry.DeleteRequest) error { + name := req.Name resp, err := d.aclient.DeleteTopics(ctx, name) if err != nil { return fmt.Errorf("kadm: delete topic %q: %w", name, err) diff --git a/pkg/drivers/registry/registry.go b/pkg/drivers/registry/registry.go index 3d8a550..c98d056 100644 --- a/pkg/drivers/registry/registry.go +++ b/pkg/drivers/registry/registry.go @@ -58,7 +58,7 @@ type Driver interface { // data plane placed on individual items (object holds, // retention) are honoured, not fought: deletion blocks with an // error naming the protected items until they are released. - DeleteBuckety(ctx context.Context, name string) error + DeleteBuckety(ctx context.Context, req DeleteRequest) error // GrantAccess returns the Secret payload to mint for a // BucketyAccess. v1alpha1 drivers may return the backend's @@ -114,8 +114,22 @@ type EnsureRequest struct { // name, S3 bucket name, ...). Pinned in // status.backendResourceName. Name string - // Parameters is spec.parameters with no controller-side - // transformation. + // Parameters is the effective parameter view (backend defaults + // merged under spec.parameters, declared templated keys + // resolved). + Parameters map[string]string +} + +// DeleteRequest carries what DeleteBuckety needs to tear down the +// backend resource and any per-resource principals the driver +// provisioned for it. +type DeleteRequest struct { + // Name is the resolved backend resource name, from + // status.backendResourceName. + Name string + // Parameters is the same effective view EnsureBuckety received; + // drivers that provisioned per-resource principals from + // parameters (gcs serviceAccount) find them here at teardown. Parameters map[string]string } @@ -129,6 +143,22 @@ type GrantRequest struct { Role string // Parameters is BucketyAccess.spec.parameters. Parameters map[string]string + // BucketyParameters is the referenced Buckety's effective + // parameter view, identical to what EnsureBuckety received. + // Drivers that mint per-resource principals read their knobs + // from here (gcs serviceAccount); drivers without such + // parameters ignore it. + BucketyParameters map[string]string + // ExistingSecretData is the current content of the access's + // credentials Secret, nil when it does not exist yet. This is + // what makes minting idempotent for credentials that are only + // 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). + ExistingSecretData map[string][]byte } // GrantResult is what the driver hands back for a BucketyAccess. @@ -147,6 +177,15 @@ type GrantResult struct { // surfaces ScopingNotImplemented=True when this is false and // Role != ReadWrite. Scoped bool + // Revocable reports whether Principal names a credential + // minted for THIS access that RevokeAccess must remove + // backend-side (a gcs SA key), as opposed to a shared static + // principal whose revoke is a no-op. Stamped into + // status.principalRevocable; the reconciler blocks deletion + // on a missing backend only for revocable principals, so + // static-credential accesses keep v1alpha1's release + // semantics. + Revocable bool } // Factory builds a Driver from its raw `config:` block as it @@ -155,6 +194,21 @@ type GrantResult struct { // types. type Factory func(rawConfig json.RawMessage) (Driver, error) +// TemplatedParameters returns the Buckety parameter keys the +// driver declares as template-resolved, or nil for drivers +// without the optional capability. Declared keys' values are run +// through the restricted parameter-template grammar +// (template.ResolveParameters) by the controller and webhook +// before validation and before every driver call; backend +// parameter defaults for declared keys skip driver validation at +// startup, since they only resolve per resource. +func TemplatedParameters(d Driver) []string { + if t, ok := d.(interface{ TemplatedParameters() []string }); ok { + return t.TemplatedParameters() + } + return nil +} + // ErrParameterDrift is the typed error EnsureBuckety returns when // it observes drift on the backend it cannot reconcile in place // (e.g. Kafka partition shrink). The controller maps this to a diff --git a/pkg/drivers/s3/driver.go b/pkg/drivers/s3/driver.go index d812720..b15457b 100644 --- a/pkg/drivers/s3/driver.go +++ b/pkg/drivers/s3/driver.go @@ -202,7 +202,8 @@ func (d *Driver) EnsureBuckety(ctx context.Context, req registry.EnsureRequest) // versioned buckets); ErrDeletionInProgress tells the controller // to requeue promptly. A bucket under sustained concurrent writes // is chased rather than declared failed. -func (d *Driver) DeleteBuckety(ctx context.Context, name string) error { +func (d *Driver) DeleteBuckety(ctx context.Context, req registry.DeleteRequest) error { + name := req.Name deleted, err := d.emptyBucketSlice(ctx, name) if err != nil { if isNotFound(err) { diff --git a/pkg/template/template.go b/pkg/template/template.go index 1e76c79..25320f8 100644 --- a/pkg/template/template.go +++ b/pkg/template/template.go @@ -29,6 +29,9 @@ type Inputs struct { Namespace string Labels map[string]string BackendDefaults map[string]string + // noLabels rejects ${label[...]} references; set only by + // ResolveParameters (see there for why). + noLabels bool } // Resolve substitutes the supported variables in tmpl against @@ -70,6 +73,49 @@ func Resolve(tmpl string, inputs Inputs) (string, error) { return b.String(), nil } +// ResolveParameters returns a copy of params with the values of +// the declared keys template-resolved; undeclared keys pass +// through untouched. Parameter templates use a restricted +// grammar: ${name}, ${namespace} and ${backend.X} only. +// ${label[...]} is rejected because parameters are re-resolved on +// every reconcile - a label edit would silently drift the +// resolved value (a service account name, say) - whereas +// spec.name gets away with label references only because its +// resolution is frozen into status.backendResourceName at first +// reconcile. +func ResolveParameters(params map[string]string, keys []string, in Inputs) (map[string]string, error) { + if len(params) == 0 || len(keys) == 0 { + return params, nil + } + in.Labels = nil + in.noLabels = true + var out map[string]string + for _, k := range keys { + tmpl, ok := params[k] + if !ok { + continue + } + resolved, err := Resolve(tmpl, in) + if err != nil { + return nil, fmt.Errorf("parameters.%s: %w", k, err) + } + if resolved == tmpl { + continue + } + if out == nil { + out = make(map[string]string, len(params)) + for pk, pv := range params { + out[pk] = pv + } + } + out[k] = resolved + } + if out == nil { + return params, nil + } + return out, nil +} + // labelRE matches the label['key'] form. The key may contain // the full K8s label-key shape (DNS-1123 names with an optional // `/` discriminator) but no closing bracket or quote. @@ -92,6 +138,9 @@ func resolveOne(expr string, in Inputs) (string, error) { return in.Namespace, nil } if m := labelRE.FindStringSubmatch(expr); m != nil { + if in.noLabels { + return "", fmt.Errorf("${%s}: parameter templates support ${name}, ${namespace} and ${backend.X} only; labels are mutable and would drift the re-resolved value", expr) + } key := m[1] v, ok := in.Labels[key] if !ok { diff --git a/pkg/template/template_test.go b/pkg/template/template_test.go index 404196d..dbae4eb 100644 --- a/pkg/template/template_test.go +++ b/pkg/template/template_test.go @@ -1,6 +1,9 @@ package template -import "testing" +import ( + "strings" + "testing" +) func TestResolve(t *testing.T) { cases := []struct { @@ -46,3 +49,65 @@ func TestResolve(t *testing.T) { }) } } + +// ResolveParameters is the parameter-template variant: declared +// keys only, restricted grammar (no labels - parameters re-resolve +// every reconcile, so mutable inputs would drift the value). +func TestResolveParameters(t *testing.T) { + in := Inputs{Name: "orders", Namespace: "tenant1", BackendDefaults: map[string]string{"zone": "eu"}} + + params := map[string]string{ + "serviceAccount": "${name}-${namespace}", + "lifecycle": `{"rule": []}`, + } + got, err := ResolveParameters(params, []string{"serviceAccount"}, in) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if got["serviceAccount"] != "orders-tenant1" { + t.Errorf("serviceAccount: %q", got["serviceAccount"]) + } + if got["lifecycle"] != `{"rule": []}` { + t.Errorf("undeclared key touched: %q", got["lifecycle"]) + } + // The input map is never mutated: callers pass the shared + // effective view. + if params["serviceAccount"] != "${name}-${namespace}" { + t.Errorf("input mutated: %q", params["serviceAccount"]) + } + + // Undeclared keys keep template-looking values verbatim. + if got, err := ResolveParameters(map[string]string{"lifecycle": "${name}"}, []string{"serviceAccount"}, in); err != nil || got["lifecycle"] != "${name}" { + t.Errorf("undeclared passthrough: %v %v", got, err) + } + + // Literal values: the same map comes back (no copy churn). + lit := map[string]string{"serviceAccount": "orders-static"} + if got, err := ResolveParameters(lit, []string{"serviceAccount"}, in); err != nil || got["serviceAccount"] != "orders-static" { + t.Errorf("literal: %v %v", got, err) + } + + // Backend defaults resolve; a missing default errors with the + // parameter key named. + if got, err := ResolveParameters(map[string]string{"serviceAccount": "${backend.zone}-${name}"}, []string{"serviceAccount"}, in); err != nil || got["serviceAccount"] != "eu-orders" { + t.Errorf("backend default: %v %v", got, err) + } + if _, err := ResolveParameters(map[string]string{"serviceAccount": "${backend.nope}"}, []string{"serviceAccount"}, in); err == nil { + t.Error("missing backend default accepted") + } + + // Labels are rejected in parameter templates even when the + // label exists - mutable inputs would drift the re-resolved + // value. + inWithLabels := in + inWithLabels.Labels = map[string]string{"site": "a"} + _, err = ResolveParameters(map[string]string{"serviceAccount": "${label['site']}-x"}, []string{"serviceAccount"}, inWithLabels) + if err == nil || !strings.Contains(err.Error(), "parameter templates") { + t.Errorf("label ref: %v", err) + } + + // $$ escaping still applies. + if got, _ := ResolveParameters(map[string]string{"serviceAccount": "a$$b"}, []string{"serviceAccount"}, in); got["serviceAccount"] != "a$b" { + t.Errorf("escape: %q", got["serviceAccount"]) + } +} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index e81c308..e426dae 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -87,6 +87,16 @@ func (v *Validator) validateBuckety(_ context.Context, req admission.Request) ad } } + // Merged + resolved views (config.Backend.ResolvedParameters): + // driver-declared templated keys resolve against immutable + // inputs only (name/namespace/backend defaults), so admission + // and the reconciler agree on the resolved value and label + // mutations cannot change it. + params, err := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) + if err != nil { + return admission.Denied(fmt.Sprintf("spec.parameters: %v", err)) + } + if req.Operation == admissionv1.Update && len(req.OldObject.Raw) > 0 { var old bucketyv1.Buckety if err := json.Unmarshal(req.OldObject.Raw, &old); err != nil { @@ -95,12 +105,18 @@ func (v *Validator) validateBuckety(_ context.Context, req admission.Request) ad // Merged views on both sides: dropping a CR key that a // backend default also defines falls back to the default // value, and that transition must pass immutability too. - if err := backend.Driver.ValidateUpdateParameters( - backend.EffectiveParameters(old.Spec.Parameters), - backend.EffectiveParameters(bky.Spec.Parameters)); err != nil { + oldParams, err := backend.ResolvedParameters(old.Name, old.Namespace, old.Spec.Parameters) + if err != nil { + // The stored object no longer resolves (a backend + // default it references was removed); comparing against + // the raw view still enforces immutability of the + // literal values without dead-ending every update. + oldParams = backend.EffectiveParameters(old.Spec.Parameters) + } + if err := backend.Driver.ValidateUpdateParameters(oldParams, params); err != nil { return admission.Denied(fmt.Sprintf("spec.parameters: %v", err)) } - } else if err := backend.Driver.ValidateParameters(backend.EffectiveParameters(bky.Spec.Parameters)); err != nil { + } else if err := backend.Driver.ValidateParameters(params); err != nil { return admission.Denied(fmt.Sprintf("spec.parameters: %v", err)) } diff --git a/schema/README.md b/schema/README.md index 804aae2..8e1ce6c 100644 --- a/schema/README.md +++ b/schema/README.md @@ -37,6 +37,7 @@ v0.1.1; the v0.1.0 tag predates this directory), or track `main`. Generated - do not edit by hand. Source of truth is the CRD yamls (CR shape) plus -`pkg/drivers//schema/v0.1/parameters.schema.json` -(parameters). Regenerate with `go run ./scripts/gen-cr-schemas`; +`pkg/drivers//schema//parameters.schema.json` +(parameters; the generator pins each driver's current schema +version). Regenerate with `go run ./scripts/gen-cr-schemas`; CI fails if the output is not committed. diff --git a/schema/buckety-gcs.schema.json b/schema/buckety-gcs.schema.json index 0158f3e..810e7ad 100644 --- a/schema/buckety-gcs.schema.json +++ b/schema/buckety-gcs.schema.json @@ -87,6 +87,14 @@ "additionalProperties": false, "description": "Schema for Buckety.spec.parameters when the resolved driver is gcs. All values are strings per the CRD contract. Omitted parameters are unmanaged: the driver never touches that knob on the backend and GCS's own defaults apply at creation (location US, uniform bucket-level access off, versioning off, no lifecycle rules).", "properties": { + "hmac": { + "description": "Whether access Secrets carry the backend's static S3-interop HMAC pair (accessKeyID/secretAccessKey). Omitted means \"true\" - the incumbent contract; gcs Secrets are S3-protocol portable by default and a minor driver bump must not remove keys. Declare \"false\" to mint Secrets without the backend-wide pair, typically together with serviceAccount so consumers hold ONLY the bucket-scoped identity; without serviceAccount the Secret carries bucket coordinates only, for consumers with ambient credentials. A backend can impose the opt-in posture fleet-wide by declaring \"false\" in its parameter defaults; CRs override per key. Mutable: flipping it rewrites Secrets in place on the next reconcile (file-mounted consumers pick it up, env-based ones need a restart).", + "enum": [ + "true", + "false" + ], + "type": "string" + }, "labels": { "description": "Bucket labels as a JSON object of string values, e.g. {\"site\": \"tenant1\", \"managed-by\": \"buckety\"}, for cost attribution and ownership. Listed labels are converged to their declared values; labels absent from the parameter are unmanaged and never deleted (same posture as unlisted parameters). Key/value charset rules are enforced by the backend.", "type": "string" @@ -100,6 +108,20 @@ "minLength": 1, "type": "string" }, + "serviceAccount": { + "anyOf": [ + { + "const": "" + }, + { + "maxLength": 30, + "minLength": 6 + } + ], + "description": "Opt-in per-bucket GCP service account, as the SA's short account ID: 6-30 characters of lowercase letters, digits and hyphens, starting with a letter and ending alphanumeric. Requires serviceAccounts enabled in the backend config. The driver maintains an SA of this name in the backend's identity project with roles/storage.objectAdmin on this bucket only, and each BucketyAccess Secret additionally carries serviceAccountKey (the SA key JSON for OAuth2 bearer-token auth), serviceAccountEmail and serviceAccountKeyId. Template-resolved with the restricted grammar (${name}, ${namespace}, ${backend.X} - no labels); include the namespace, e.g. ${name}-${namespace}, since SA IDs are unique per project. The empty string is an explicit per-CR opt-out of a backend parameter default. Set-at-create and immutable either way: recreate the Buckety (retentionPolicy=Retain + adoption keeps the bucket) to change it. NOTE a backend parameter default here applies to EXISTING Bucketys on the backend's next controller rollout - defaults merge on every reconcile and never re-pass admission, so audit resolved-name lengths fleet-wide before adding one.", + "type": "string", + "x-buckety-templated": true + }, "softDeleteRetentionSeconds": { "description": "Soft delete window (softDeletePolicy.retentionDurationSeconds): how long deleted or overwritten objects remain restorable. \"0\" disables; GCS accepts enabled windows of 604800 to 7776000 seconds (7 to 90 days) only. Note GCS defaults NEW buckets to 7 days on - declare \"0\" to opt out, or a value to pin it against console drift. Mutable; reconciled in place. Cheap insurance against mass-delete with a compromised data-plane key, at the cost of also retaining intentionally lifecycle-deleted objects for the window.", "type": "string" diff --git a/schema/bucketyaccess.schema.json b/schema/bucketyaccess.schema.json index 97d5f78..fc4fbcd 100644 --- a/schema/bucketyaccess.schema.json +++ b/schema/bucketyaccess.schema.json @@ -133,6 +133,10 @@ "principal": { "description": "Backend-side identity granted access. In v1alpha1\nwith no per-consumer scoping this is typically\nthe backend's root principal.\n", "type": "string" + }, + "principalRevocable": { + "description": "Whether principal names a credential the driver\nminted for this access and must revoke\nbackend-side (a gcs SA key). Deletion with the\nbackend missing from config blocks only for\nrevocable principals.\n", + "type": "boolean" } }, "type": "object" diff --git a/scripts/bump-release.sh b/scripts/bump-release.sh index 2eb3488..99da020 100755 --- a/scripts/bump-release.sh +++ b/scripts/bump-release.sh @@ -20,6 +20,19 @@ command -v jq >/dev/null || { echo "jq not on PATH" >&2; exit 1; } command -v kustomize >/dev/null || { echo "kustomize not on PATH" >&2; exit 1; } cd "$REPO" + +# The digest is a function of the compiler, and CI rebuilds with +# the go.mod toolchain directive (setup-go go-version-file). A bump +# built with any other toolchain pins a digest CI cannot reproduce, +# so drift fails here with the remedy instead of in the assertion. +WANT="$(go mod edit -json | jq -r '.Toolchain // empty')" +GOT="$(go env GOVERSION)" +if [[ -z "$WANT" || "$GOT" != "$WANT" ]]; then + echo "local toolchain ${GOT} does not match the go.mod toolchain directive '${WANT:-}'" >&2 + echo "align them first (go mod edit -toolchain=${GOT}) so CI reproduces the digest" >&2 + exit 1 +fi + TAG="$(date -u +%Y%m%dT%H%M%SZ)" rm -rf target/linux/amd64 oci diff --git a/scripts/gen-cr-schemas/main.go b/scripts/gen-cr-schemas/main.go index cfc5457..8f1dff8 100644 --- a/scripts/gen-cr-schemas/main.go +++ b/scripts/gen-cr-schemas/main.go @@ -48,7 +48,7 @@ func main() { // etc), while the family package keeps the SPEC term. {"blobstore", "pkg/drivers/objectstore/schema/v0.1/parameters.schema.json", "Standalone editor schema for a Buckety carrying only object-store family-common parameters, provisionable on any bucket backend (gcs, s3) - see SPEC.md \"Driver families\". SPECIALIZE to buckety-gcs or buckety-s3 for driver-specific parameters; GENERALIZE to buckety for no parameter constraints."}, - {"gcs", "pkg/drivers/gcs/schema/v0.1/parameters.schema.json", + {"gcs", "pkg/drivers/gcs/schema/v0.2/parameters.schema.json", "Standalone editor schema for a Buckety whose backend resolves to the gcs driver. GENERALIZE to buckety-blobstore to keep the resource portable across bucket backends, or to buckety for no parameter constraints."}, {"s3", "pkg/drivers/s3/schema/v0.1/parameters.schema.json", "Standalone editor schema for a Buckety whose backend resolves to the s3 driver. GENERALIZE to buckety-blobstore to keep the resource portable across bucket backends, or to buckety for no parameter constraints."},