diff --git a/Cargo.lock b/Cargo.lock index 896e135..43def70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3352,9 +3352,9 @@ dependencies = [ [[package]] name = "rc-core" -version = "0.1.31" +version = "0.1.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "24cbba164a412d1fe595b1e5a6a58d02b9f26b47c5c1849f8fd31c468d913825" +checksum = "2c29f0b14dfa2b1b5df988baefc5717de4a07402a3af5b15540bfd5760291e3a" dependencies = [ "aes-gcm", "anyhow", @@ -3388,9 +3388,9 @@ dependencies = [ [[package]] name = "rc-s3" -version = "0.1.31" +version = "0.1.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ea41c3139e886ed61ed54341d272a141857cb9880108cb51d4fdf5e30b89372a" +checksum = "e91216df902251c29cd5116339247546cda51741782506ad6e568317842c5308" dependencies = [ "anyhow", "async-trait", @@ -3604,7 +3604,7 @@ dependencies = [ [[package]] name = "rustfs-operator" -version = "0.6.4" +version = "0.7.0" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index 2544a3a..4045c91 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustfs-operator" -version = "0.6.4" +version = "0.7.0" edition = "2024" rust-version = "1.92" license = "MIT OR Apache-2.0" @@ -8,8 +8,8 @@ description = "Kubernetes operator managing RustFS buckets, IAM users and polici [dependencies] # RustFS management-plane client -rc-core = "0.1.31" -rc-s3 = "0.1.31" +rc-core = "0.1.32" +rc-s3 = "0.1.32" # Kubernetes kube = { version = "4.0", default-features = false, features = ["client", "runtime", "derive", "rustls-tls", "ring"] } diff --git a/README.md b/README.md index f8dc40e..61819cc 100644 --- a/README.md +++ b/README.md @@ -30,14 +30,14 @@ User "spark" (policies attach here) The operator writes each generated `accessKey`/`secretKey`/`endpoint` into a Secret in the CR's namespace, owner-referenced so it is garbage-collected with the CR; if that Secret is lost the key is revoked and reissued. Keys -are issued while authenticated *as the owning user*, so an AccessKey needs -the user's password and the user's policies must allow -`admin:CreateServiceAccount`, `admin:ListServiceAccounts` and -`admin:RemoveServiceAccount`. +are issued by the operator's admin credential, which names the owner with +`targetUser`; the server accepts that only from an owner credential, so the +connection must hold RustFS root. **See [docs/iam-model.md](docs/iam-model.md)** for how parenting works, why -the password is required (a client-library gap, not a server one), and how -to inspect identities with `rc`. +root is required, and how to inspect identities with `rc`. Upgrading from +0.6.x is a breaking change — `AccessKey.passwordRef` / the chart's +`passwordFromUser` are gone. Namespaced resources select a RustFS server via `spec.connection`, in one of two mutually exclusive ways: diff --git a/charts/rustfs-operator-crds/Chart.yaml b/charts/rustfs-operator-crds/Chart.yaml index 943e773..f2be2a2 100644 --- a/charts/rustfs-operator-crds/Chart.yaml +++ b/charts/rustfs-operator-crds/Chart.yaml @@ -11,5 +11,5 @@ keywords: - operator - crds # version and appVersion are overwritten from the git tag by the release workflow -version: 0.6.4 -appVersion: "0.6.4" +version: 0.7.0 +appVersion: "0.7.0" diff --git a/charts/rustfs-operator-crds/templates/accesskeys.yaml b/charts/rustfs-operator-crds/templates/accesskeys.yaml index 1b1d040..d7158b7 100644 --- a/charts/rustfs-operator-crds/templates/accesskeys.yaml +++ b/charts/rustfs-operator-crds/templates/accesskeys.yaml @@ -86,7 +86,12 @@ spec: nullable: true type: string passwordRef: - description: Secret holding that user's password (key defaults to `password`). + description: |- + REMOVED in 0.7.0 and rejected if set. Keys are now issued by the admin + credential via `targetUser`, so the owning user's password is no longer + needed. Retained in the schema only so a stale manifest fails loudly + instead of being silently pruned; drop the field. + nullable: true properties: key: description: Key within the Secret; each consumer documents its default. @@ -112,11 +117,13 @@ spec: nullable: true type: string user: - description: Username of the owning RustFS user. + description: |- + Username of the owning RustFS user. The key is parented to this user + via `targetUser`, which the server honours only for an owner + credential — the connection must hold RustFS root. type: string required: - connection - - passwordRef - user type: object status: diff --git a/charts/rustfs-operator/Chart.yaml b/charts/rustfs-operator/Chart.yaml index 2a93a62..cadfd54 100644 --- a/charts/rustfs-operator/Chart.yaml +++ b/charts/rustfs-operator/Chart.yaml @@ -10,5 +10,5 @@ keywords: - s3 - operator # version and appVersion are overwritten from the git tag by the release workflow -version: 0.6.4 -appVersion: "0.6.4" +version: 0.7.0 +appVersion: "0.7.0" diff --git a/charts/rustfs-operator/crds/crds.yaml b/charts/rustfs-operator/crds/crds.yaml index e3b98f3..3af628d 100644 --- a/charts/rustfs-operator/crds/crds.yaml +++ b/charts/rustfs-operator/crds/crds.yaml @@ -376,7 +376,12 @@ spec: nullable: true type: string passwordRef: - description: Secret holding that user's password (key defaults to `password`). + description: |- + REMOVED in 0.7.0 and rejected if set. Keys are now issued by the admin + credential via `targetUser`, so the owning user's password is no longer + needed. Retained in the schema only so a stale manifest fails loudly + instead of being silently pruned; drop the field. + nullable: true properties: key: description: Key within the Secret; each consumer documents its default. @@ -402,11 +407,13 @@ spec: nullable: true type: string user: - description: Username of the owning RustFS user. + description: |- + Username of the owning RustFS user. The key is parented to this user + via `targetUser`, which the server honours only for an owner + credential — the connection must hold RustFS root. type: string required: - connection - - passwordRef - user type: object status: diff --git a/charts/rustfs-resources/Chart.yaml b/charts/rustfs-resources/Chart.yaml index 6ca30a2..1b0b1ad 100644 --- a/charts/rustfs-resources/Chart.yaml +++ b/charts/rustfs-resources/Chart.yaml @@ -10,5 +10,5 @@ keywords: - s3 - operator # version and appVersion are overwritten from the git tag by the release workflow -version: 0.6.4 -appVersion: "0.6.4" +version: 0.7.0 +appVersion: "0.7.0" diff --git a/charts/rustfs-resources/README.md b/charts/rustfs-resources/README.md index 05146c5..9c6a574 100644 --- a/charts/rustfs-resources/README.md +++ b/charts/rustfs-resources/README.md @@ -36,15 +36,13 @@ policies: users: - name: app-user - policies: ["app-data-rw"] # must include admin:*ServiceAccount actions + policies: ["app-data-rw"] passwordRef: # existing Secret with key `password` name: app-user-creds accessKeys: - name: app-key # operator writes AK/SK to Secret user: app-user # "app-key-credentials" in this namespace - passwordRef: - name: app-user-creds ``` ## Values @@ -55,7 +53,7 @@ accessKeys: | `buckets[]` | `name` (required), `bucketName`, `versioning`, `quotaBytes`, `deletionPolicy`, `connection` | | `policies[]` | `name` (required), `document` (required), `policyName`, `deletionPolicy`, `connection` | | `users[]` | `name` (required), `username`, `passwordRef` **or** inline `password`, `policies`, `enabled`, `deletionPolicy`, `connection` | -| `accessKeys[]` | `name`, `user` (required), `passwordRef` **or** `passwordFromUser`, `accessKey`, `description`, `policy`, `targetSecretName`, `deletionPolicy`, `connection` | +| `accessKeys[]` | `name`, `user` (required), `accessKey`, `description`, `policy`, `targetSecretName`, `deletionPolicy`, `connection` | Fields you omit stay unmanaged (e.g. no `versioning` key means the operator never touches versioning). `deletionPolicy` defaults to `Delete` — the @@ -75,12 +73,13 @@ entry is only needed for per-consumer or reduced-scope credentials `-credentials`). The key inherits that user's policies, so grant access via `users[].policies` rather than per key. -The password is needed because the operator authenticates as the user to -issue its keys: set `passwordRef`, or `passwordFromUser: ` to -reuse the password Secret this chart created for that user. For the same -reason the user's policies must allow `admin:CreateServiceAccount`, -`admin:ListServiceAccounts` and `admin:RemoveServiceAccount` over itself. -See [docs/iam-model.md](../../docs/iam-model.md). +Only `user` is needed: since chart 0.7.0 the operator issues keys with its own +admin credential, naming the owner via `targetUser`. `passwordFromUser` and +`passwordRef` were removed and are rejected at render time, and the owning +user no longer needs `admin:CreateServiceAccount` / +`admin:ListServiceAccounts` / `admin:RemoveServiceAccount`. This requires the +operator's connection to hold RustFS root — see +[docs/iam-model.md](../../docs/iam-model.md). ## Prerequisites diff --git a/charts/rustfs-resources/templates/accesskey.yaml b/charts/rustfs-resources/templates/accesskey.yaml index fd93a1f..44e8b90 100644 --- a/charts/rustfs-resources/templates/accesskey.yaml +++ b/charts/rustfs-resources/templates/accesskey.yaml @@ -6,14 +6,8 @@ {{- if eq $user "" }} {{- fail (printf "%s: 'user' (the owning RustFS username) is required" $id) }} {{- end }} -{{- $fromUser := .passwordFromUser | default "" }} -{{- $ref := .passwordRef | default dict }} -{{- $refName := $ref.name | default "" }} -{{- if and (ne $fromUser "") (ne $refName "") }} -{{- fail (printf "%s: set either passwordRef or passwordFromUser, not both" $id) }} -{{- end }} -{{- if and (eq $fromUser "") (eq $refName "") }} -{{- fail (printf "%s: a password source is required; set passwordRef or passwordFromUser (a users[] entry with inline password)" $id) }} +{{- if or (hasKey . "passwordFromUser") (hasKey . "passwordRef") }} +{{- fail (printf "%s: passwordFromUser/passwordRef were removed in chart 0.7.0. The operator now issues keys with its own admin credential using targetUser, so the owning user's password is no longer needed. Drop the field, and drop the admin:CreateServiceAccount / ListServiceAccounts / RemoveServiceAccount grants from that user's policy if nothing else needs them" $id) }} {{- end }} {{- $ak := .accessKey | default "" }} {{- if and (ne $ak "") (eq $ak $user) }} @@ -29,16 +23,6 @@ metadata: spec: {{- include "rustfs-resources.connection" (dict "root" $ "entry" . "id" $id) | nindent 2 }} user: {{ $user | quote }} - passwordRef: - {{- if ne $fromUser "" }} - # the Secret this chart creates for the referenced users[] entry - name: {{ printf "%s-user-%s" $.Release.Name $fromUser }} - {{- else }} - name: {{ $refName }} - {{- with $ref.key }} - key: {{ . }} - {{- end }} - {{- end }} {{- with .accessKey }} accessKey: {{ . | quote }} {{- end }} diff --git a/charts/rustfs-resources/values.yaml b/charts/rustfs-resources/values.yaml index 6e62dd8..e28b9b2 100644 --- a/charts/rustfs-resources/values.yaml +++ b/charts/rustfs-resources/values.yaml @@ -39,8 +39,6 @@ users: [] # # ... OR let the chart create one (WARNING: the password then lives # # in the Helm release values; prefer passwordRef in production): # password: "" -# # to issue access keys for this user, its policies must include the -# # admin:CreateServiceAccount / ListServiceAccounts / RemoveServiceAccount actions # policies: ["app-data-rw"] # enabled: true # optional # deletionPolicy: Delete @@ -50,10 +48,8 @@ accessKeys: [] # accessKeys: # - name: app-key # AK/SK pair for a user; credentials are # user: app-user # written by the operator to a Secret -# # password source for the owning user, EITHER: -# passwordFromUser: app-user # a users[] entry above with inline password -# # OR an explicit Secret reference: -# # passwordRef: {name: app-user-creds, key: password} +# # No password source: the operator issues the key with its own admin +# # credential via targetUser, which requires that credential to be root. # accessKey: "" # optional; generated when empty # description: "" # policy: {} # optional inline policy restricting the key diff --git a/deploy/crds.yaml b/deploy/crds.yaml index e3b98f3..3af628d 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -376,7 +376,12 @@ spec: nullable: true type: string passwordRef: - description: Secret holding that user's password (key defaults to `password`). + description: |- + REMOVED in 0.7.0 and rejected if set. Keys are now issued by the admin + credential via `targetUser`, so the owning user's password is no longer + needed. Retained in the schema only so a stale manifest fails loudly + instead of being silently pruned; drop the field. + nullable: true properties: key: description: Key within the Secret; each consumer documents its default. @@ -402,11 +407,13 @@ spec: nullable: true type: string user: - description: Username of the owning RustFS user. + description: |- + Username of the owning RustFS user. The key is parented to this user + via `targetUser`, which the server honours only for an owner + credential — the connection must hold RustFS root. type: string required: - connection - - passwordRef - user type: object status: diff --git a/deploy/example.yaml b/deploy/example.yaml index 113ebfc..9e60f46 100644 --- a/deploy/example.yaml +++ b/deploy/example.yaml @@ -85,12 +85,11 @@ spec: enabled: true --- # An AK/SK credential pair owned by app-user, inheriting its policies. The -# operator issues it in RustFS (authenticating as the user, hence the -# password) and writes accessKey/secretKey/endpoint into the Secret +# operator issues it with its own admin credential, naming the owner via +# targetUser, and writes accessKey/secretKey/endpoint into the Secret # "app-key-credentials" (owner-referenced, GC'd with the CR). -# NOTE: the user's policies must include admin:CreateServiceAccount, -# admin:ListServiceAccounts and admin:RemoveServiceAccount. -# See docs/iam-model.md for the full model. +# NOTE: the connection must hold RustFS root — the server only lets an owner +# parent a key to another user. See docs/iam-model.md for the full model. apiVersion: rustfs.com/v1alpha1 kind: AccessKey metadata: @@ -99,5 +98,3 @@ spec: connection: secretRef: rustfs-conn user: app-user - passwordRef: - name: app-user-creds diff --git a/docs/iam-model.md b/docs/iam-model.md index 35603ed..8aaf8c9 100644 --- a/docs/iam-model.md +++ b/docs/iam-model.md @@ -112,19 +112,38 @@ Rule of thumb: one workload per identity is fine on the user credential; several workloads, or anything needing per-consumer revocation or reduced scope, wants access keys. -## Why `AccessKey` needs the user's password +## How `AccessKey` issues keys -The operator authenticates **as the user** to issue that user's keys, which -is why `AccessKey` requires `passwordRef` (or `passwordFromUser`) and why -the owning user's policies must allow `admin:CreateServiceAccount`, -`admin:ListServiceAccounts` and `admin:RemoveServiceAccount` over itself. +The operator issues keys with its **admin credential**, naming the owner via +`targetUser`. The `AccessKey` CR therefore needs nothing but the username: -This is a limitation of the client library, not of RustFS. The server -accepts `targetUser`, so the admin credential alone would be enough — but -`targetUser` is not exposed by `rc-core`/`rc-s3`, so the operator cannot -send it. Tracked upstream in -[rustfs/cli#340](https://github.com/rustfs/cli/issues/340); once it lands, -the password requirement and the per-user admin actions can both be dropped. +```yaml +spec: + connection: { clusterRef: default } + user: spark # becomes targetUser +``` + +**The connection must hold RustFS root.** The server's guard is +`owner || target_user == req_user || target_user == req_parent_user`, so only +an owner credential may parent a key to somebody else; a scoped admin gets +`service account parent is outside requester scope`. This is deliberate — +`admin:CreateServiceAccount` controls *whether* a caller can create keys, not +*for whom*, and without the guard any holder of that action could mint a +root-parented key and escalate (GHSA-5354). + +Before 0.7.0 the operator authenticated *as the owning user*, which meant every +`AccessKey` carried `passwordRef`/`passwordFromUser` and every owning user +needed `admin:CreateServiceAccount`, `admin:ListServiceAccounts` and +`admin:RemoveServiceAccount` over itself. That was a client-library gap +([rustfs/cli#340](https://github.com/rustfs/cli/issues/340)), not a server one; +it was fixed by [#352](https://github.com/rustfs/cli/pull/352) and shipped in +rc-core 0.1.32, which the operator now requires. + +Migrating from 0.6.x: delete `passwordRef` / `passwordFromUser` from every +`AccessKey`, and drop the three `admin:*ServiceAccount` grants from the owning +user's policy unless something else needs them. A leftover `passwordRef` is +rejected as a spec error rather than ignored, and the chart fails at render +time, so a stale manifest cannot silently keep working. ## Reading the server directly @@ -139,9 +158,13 @@ rc admin service-account list --user spark # that user's keys rc admin service-account list # the alias's OWN keys ``` -The `parent:` shown in the listing is the owning user. A key created with -`rc admin service-account create` is always parented to the alias's -identity, since the CLI cannot send `targetUser` either. +The `parent:` shown in the listing is the owning user. Since rc 0.1.32, +`create` also takes `--user` to parent a new key to someone else (owner +credentials only); without it the key is parented to the alias's own identity: + +```sh +rc admin service-account create --user spark # rc >= 0.1.32 +``` ## Other server behaviours worth knowing diff --git a/scripts/chart-tests.sh b/scripts/chart-tests.sh index 8fba3f2..0410cd1 100755 --- a/scripts/chart-tests.sh +++ b/scripts/chart-tests.sh @@ -46,7 +46,9 @@ grep -q 'clusterRef: "prod"' <<<"$render" || fail "default connection not applie grep -q 'name: rel-user-ci-user' <<<"$render" || fail "chart-created user secret not rendered" grep -q 'password: "chart-e2e-password-123"' <<<"$render" || fail "user password not in chart secret" grep -q 'kind: AccessKey' <<<"$render" || fail "no AccessKey rendered" -grep -qE 'name: rel-user-ci-user' <<<"$render" || fail "passwordFromUser not resolved" +# User keeps passwordRef; AccessKey must not. Check only the AccessKey document. +akdoc=$(awk '/^kind: AccessKey$/{f=1} f{print} f&&/^---$/{exit}' <<<"$render") +grep -q 'passwordRef' <<<"$akdoc" && fail "AccessKey must no longer carry passwordRef" render=$(helm template rel charts/rustfs-resources --set-json 'buckets=[{"name":"b","connection":{"secretRef":"local"}}]') grep -q 'secretRef: "local"' <<<"$render" || fail "per-entry connection override not applied" grep -q 'versioning' <<<"$render" && fail "omitted versioning must not render" @@ -73,8 +75,9 @@ expect_fail $R 'buckets=[{"name":"a","connection":{"clusterRef":"p"},"deletionPo expect_fail $R 'policies=[{"name":"a","connection":{"clusterRef":"p"}}]' "'document' is required" expect_fail $R 'users=[{"name":"a","connection":{"clusterRef":"p"}}]' "password source is required" expect_fail $R 'users=[{"name":"a","connection":{"clusterRef":"p"},"password":"x","passwordRef":{"name":"y"}}]' "not both" -expect_fail $R 'accessKeys=[{"name":"a","connection":{"clusterRef":"p"},"passwordFromUser":"u"}]' "'user' (the owning RustFS username) is required" -expect_fail $R 'accessKeys=[{"name":"a","user":"u","connection":{"clusterRef":"p"}}]' "password source is required" -expect_fail $R 'accessKeys=[{"name":"a","user":"spark","accessKey":"spark","connection":{"clusterRef":"p"},"passwordFromUser":"spark"}]' "must differ from the owning username" +expect_fail $R 'accessKeys=[{"name":"a","connection":{"clusterRef":"p"}}]' "'user' (the owning RustFS username) is required" +expect_fail $R 'accessKeys=[{"name":"a","user":"u","connection":{"clusterRef":"p"},"passwordFromUser":"u"}]' "were removed in chart 0.7.0" +expect_fail $R 'accessKeys=[{"name":"a","user":"u","connection":{"clusterRef":"p"},"passwordRef":{"name":"s"}}]' "were removed in chart 0.7.0" +expect_fail $R 'accessKeys=[{"name":"a","user":"spark","accessKey":"spark","connection":{"clusterRef":"p"}}]' "must differ from the owning username" echo "chart-tests OK" diff --git a/scripts/testdata/resources-values.yaml b/scripts/testdata/resources-values.yaml index 868dfac..075d254 100644 --- a/scripts/testdata/resources-values.yaml +++ b/scripts/testdata/resources-values.yaml @@ -17,13 +17,6 @@ policies: - Effect: Allow Action: ["s3:GetObject", "s3:PutObject"] Resource: ["arn:aws:s3:::app-data/*"] - # required for the user to manage its own access keys - - Effect: Allow - Action: - - admin:CreateServiceAccount - - admin:ListServiceAccounts - - admin:RemoveServiceAccount - Resource: ["arn:aws:s3:::*"] users: - name: ci-user @@ -33,5 +26,4 @@ users: accessKeys: - name: ci-key user: ci-user - passwordFromUser: ci-user description: chart e2e key diff --git a/src/crd.rs b/src/crd.rs index cb90d48..321d282 100644 --- a/src/crd.rs +++ b/src/crd.rs @@ -253,10 +253,16 @@ impl User { #[serde(rename_all = "camelCase")] pub struct AccessKeySpec { pub connection: ConnectionRef, - /// Username of the owning RustFS user. + /// Username of the owning RustFS user. The key is parented to this user + /// via `targetUser`, which the server honours only for an owner + /// credential — the connection must hold RustFS root. pub user: String, - /// Secret holding that user's password (key defaults to `password`). - pub password_ref: SecretKeyRef, + /// REMOVED in 0.7.0 and rejected if set. Keys are now issued by the admin + /// credential via `targetUser`, so the owning user's password is no longer + /// needed. Retained in the schema only so a stale manifest fails loudly + /// instead of being silently pruned; drop the field. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub password_ref: Option, /// Explicit access key id; generated when omitted. #[serde(default, skip_serializing_if = "Option::is_none")] pub access_key: Option, diff --git a/src/provider.rs b/src/provider.rs index 2cfad2e..fd8d17d 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -72,33 +72,21 @@ pub trait RustFs: Send + Sync { async fn put_policy(&self, name: &str, document: &str) -> Result<()>; async fn delete_policy(&self, name: &str) -> Result<()>; - // Access keys (service accounts). RustFS only mints/manages service - // accounts for the calling identity, so these authenticate as the - // owning user (username + password) rather than as the admin. + // Access keys (service accounts). These run as the admin and name the + // owning user with `targetUser`, which the server honours only for an + // owner credential — see `docs/iam-model.md`. /// The S3 endpoint this provider talks to (stored in credential Secrets). fn endpoint(&self) -> String; - async fn get_access_key( - &self, - username: &str, - password: &str, - access_key: &str, - ) -> Result>; - #[allow(clippy::too_many_arguments)] + async fn get_access_key(&self, access_key: &str) -> Result>; async fn create_access_key( &self, username: &str, - password: &str, access_key: &str, secret_key: &str, description: Option, policy: Option, ) -> Result<()>; - async fn delete_access_key( - &self, - username: &str, - password: &str, - access_key: &str, - ) -> Result<()>; + async fn delete_access_key(&self, access_key: &str) -> Result<()>; } /// Real implementation backed by `rc-s3`. @@ -118,20 +106,10 @@ impl RustFsProvider { /// Log the equivalent rustfs-cli invocation for an API call. /// `$ALIAS` stands for an `rc alias` configured with the admin - /// credentials; `$USER_ALIAS` for one configured with the acting - /// user's credentials. Filter with RUST_LOG=rc_cli=info. + /// credentials. Filter with RUST_LOG=rc_cli=info. fn cli(cmd: &str) { tracing::info!(target: "rc_cli", "equivalent: rc {cmd}"); } - - /// Admin client authenticated as a regular user (for service-account - /// operations, which RustFS scopes to the calling identity). - fn client_as(&self, username: &str, password: &str) -> Result { - let mut info = self.info.clone(); - info.access_key = username.to_string(); - info.secret_key = password.to_string(); - Ok(AdminClient::new(&info.into_alias())?) - } } /// Treat "does not exist" responses as absence rather than failure. @@ -268,38 +246,28 @@ impl RustFs for RustFsProvider { self.info.endpoint.clone() } - async fn get_access_key( - &self, - username: &str, - password: &str, - access_key: &str, - ) -> Result> { - Self::cli(&format!( - "admin service-account info $USER_ALIAS {access_key} # $USER_ALIAS uses {username}'s credentials" - )); - let client = self.client_as(username, password)?; - optional(client.get_service_account(access_key).await) + async fn get_access_key(&self, access_key: &str) -> Result> { + Self::cli(&format!("admin service-account info $ALIAS {access_key}")); + optional(self.admin.get_service_account(access_key).await) } async fn create_access_key( &self, username: &str, - password: &str, access_key: &str, secret_key: &str, description: Option, policy: Option, ) -> Result<()> { Self::cli(&format!( - "admin service-account create $USER_ALIAS {access_key} ****{} # $USER_ALIAS uses {username}'s credentials", + "admin service-account create $ALIAS {access_key} **** --user {username}{}", if policy.is_some() { " --policy policy.json" } else { "" } )); - let client = self.client_as(username, password)?; - client + self.admin .create_service_account(CreateServiceAccountRequest { policy, expiry: None, @@ -307,32 +275,19 @@ impl RustFs for RustFsProvider { description, access_key: access_key.to_string(), secret_key: secret_key.to_string(), + // Parents the key to `username` instead of the admin. The + // server accepts this only from an owner credential. + target_user: Some(username.to_string()), }) .await?; Ok(()) } - async fn delete_access_key( - &self, - username: &str, - password: &str, - access_key: &str, - ) -> Result<()> { + async fn delete_access_key(&self, access_key: &str) -> Result<()> { Self::cli(&format!("admin service-account rm $ALIAS {access_key}")); - // Try as admin first (covers keys whose owner was already deleted), - // fall back to the owning user. match self.admin.delete_service_account(access_key).await { Ok(()) => Ok(()), - Err(admin_err) => { - if absent(admin_err).is_ok() { - return Ok(()); - } - let client = self.client_as(username, password)?; - match client.delete_service_account(access_key).await { - Ok(()) => Ok(()), - Err(e) => absent(e), - } - } + Err(e) => absent(e), } } } diff --git a/src/reconcile/access_key.rs b/src/reconcile/access_key.rs index 253fdbf..4fa8b6a 100644 --- a/src/reconcile/access_key.rs +++ b/src/reconcile/access_key.rs @@ -1,11 +1,11 @@ //! AccessKey (service account) reconciliation. //! -//! RustFS mints service accounts for the calling identity only, so the -//! operator authenticates as the owning user (username + password from the -//! spec) to issue keys. The generated AK/SK pair is written to a Secret in -//! the CR's namespace, owner-referenced for garbage collection. Secret keys -//! are only obtainable at creation time: if the target Secret disappears, -//! the key is revoked and reissued. +//! Keys are issued by the admin credential, which names the owning user with +//! `targetUser`; the server accepts that only from an owner credential, so the +//! operator's connection must hold root. The generated AK/SK pair is written +//! to a Secret in the CR's namespace, owner-referenced for garbage collection. +//! Secret keys are only obtainable at creation time: if the target Secret +//! disappears, the key is revoked and reissued. use std::sync::Arc; @@ -18,7 +18,7 @@ use kube::{Api, Resource, ResourceExt}; use rand::Rng; use super::{Context, FINALIZER, REQUEUE_OK, namespace_of, patch_status}; -use crate::connection::{provider_for, secret_key_value}; +use crate::connection::provider_for; use crate::crd::{AccessKey, AccessKeySpec, AccessKeyStatus, DeletionPolicy}; use crate::error::{Error, Result}; use crate::provider::RustFs; @@ -75,7 +75,6 @@ pub enum KeyOutcome { pub async fn ensure_access_key( fs: &dyn RustFs, username: &str, - password: &str, known_ak: Option<&str>, secret_intact: bool, spec: &AccessKeySpec, @@ -89,9 +88,20 @@ pub async fn ensure_access_key( }) .transpose()?; + if spec.password_ref.is_some() { + return Err(Error::Spec( + "passwordRef was removed in 0.7.0: access keys are now issued by the \ + operator's admin credential using targetUser, so the owning user's \ + password is no longer needed. Delete the field (and, if nothing else \ + uses them, the admin:CreateServiceAccount / ListServiceAccounts / \ + RemoveServiceAccount grants on that user)" + .into(), + )); + } + if let Some(ak) = known_ak { reject_username_collision(ak, username)?; - let exists = fs.get_access_key(username, password, ak).await?.is_some(); + let exists = fs.get_access_key(ak).await?.is_some(); if exists && secret_intact { return Ok(KeyOutcome::Kept { access_key: ak.to_string(), @@ -99,18 +109,11 @@ pub async fn ensure_access_key( } if exists { // Secret lost; the SK is unrecoverable — revoke and reissue. - fs.delete_access_key(username, password, ak).await?; + fs.delete_access_key(ak).await?; } let secret_key = generate_secret_key(); - fs.create_access_key( - username, - password, - ak, - &secret_key, - spec.description.clone(), - policy, - ) - .await?; + fs.create_access_key(username, ak, &secret_key, spec.description.clone(), policy) + .await?; return Ok(KeyOutcome::Issued { access_key: ak.to_string(), secret_key, @@ -121,7 +124,6 @@ pub async fn ensure_access_key( let secret_key = generate_secret_key(); fs.create_access_key( username, - password, &access_key, &secret_key, spec.description.clone(), @@ -137,7 +139,6 @@ pub async fn ensure_access_key( pub async fn cleanup_access_key( fs: &dyn RustFs, username: &str, - password: &str, access_key: Option<&str>, spec: &AccessKeySpec, ) -> Result<()> { @@ -145,7 +146,7 @@ pub async fn cleanup_access_key( // A colliding key was never created, and asking the server to revoke // an id that resolves to the user itself is not worth the risk. (DeletionPolicy::Delete, Some(ak)) if ak == username => Ok(()), - (DeletionPolicy::Delete, Some(ak)) => fs.delete_access_key(username, password, ak).await, + (DeletionPolicy::Delete, Some(ak)) => fs.delete_access_key(ak).await, _ => Ok(()), } } @@ -226,8 +227,6 @@ async fn apply(obj: Arc, ctx: &Context) -> Result { let secret_name = obj.target_secret_name(); let result: Result = async { - let password = - secret_key_value(&ctx.client, &ns, &obj.spec.password_ref, "password").await?; let fs = provider_for(&ctx.client, &ns, &obj.spec.connection).await?; let known_ak = obj @@ -240,15 +239,8 @@ async fn apply(obj: Arc, ctx: &Context) -> Result { None => false, }; - let outcome = ensure_access_key( - &fs, - &obj.spec.user, - &password, - known_ak.as_deref(), - intact, - &obj.spec, - ) - .await?; + let outcome = + ensure_access_key(&fs, &obj.spec.user, known_ak.as_deref(), intact, &obj.spec).await?; if let KeyOutcome::Issued { access_key, secret_key, @@ -301,22 +293,8 @@ async fn cleanup(obj: Arc, ctx: &Context) -> Result { if known_ak.is_none() { return Ok(Action::await_change()); } - // Best-effort password read: on `helm uninstall` the password Secret and - // this CR are often deleted together. Revocation tries the admin - // credentials first, so an empty password still succeeds; it is only - // needed for the user-scoped fallback. - let password = secret_key_value(&ctx.client, &ns, &obj.spec.password_ref, "password") - .await - .unwrap_or_default(); let fs = provider_for(&ctx.client, &ns, &obj.spec.connection).await?; - cleanup_access_key( - &fs, - &obj.spec.user, - &password, - known_ak.as_deref(), - &obj.spec, - ) - .await?; + cleanup_access_key(&fs, &obj.spec.user, known_ak.as_deref(), &obj.spec).await?; Ok(Action::await_change()) } @@ -331,10 +309,7 @@ mod tests { AccessKeySpec { connection: ConnectionRef::cluster("prod"), user: "spark".into(), - password_ref: SecretKeyRef { - name: "spark-password".into(), - key: None, - }, + password_ref: None, access_key: access_key.map(str::to_string), description: None, policy: None, @@ -347,12 +322,10 @@ mod tests { async fn issues_generated_key_when_none_known() { let mut fs = MockRustFs::new(); fs.expect_create_access_key() - .withf(|user, pwd, ak, sk, _, _| { - user == "spark" && pwd == "pw" && ak.len() == 20 && sk.len() == 40 - }) - .return_once(|_, _, _, _, _, _| Ok(())); + .withf(|user, ak, sk, _, _| user == "spark" && ak.len() == 20 && sk.len() == 40) + .return_once(|_, _, _, _, _| Ok(())); - match ensure_access_key(&fs, "spark", "pw", None, false, &spec(None)) + match ensure_access_key(&fs, "spark", None, false, &spec(None)) .await .unwrap() { @@ -365,10 +338,10 @@ mod tests { async fn existing_key_with_intact_secret_is_kept() { let mut fs = MockRustFs::new(); fs.expect_get_access_key() - .withf(|_, _, ak| ak == "AK1") - .return_once(|_, _, _| Ok(Some(ServiceAccount::new("AK1")))); + .withf(|ak| ak == "AK1") + .return_once(|_| Ok(Some(ServiceAccount::new("AK1")))); - let outcome = ensure_access_key(&fs, "spark", "pw", Some("AK1"), true, &spec(Some("AK1"))) + let outcome = ensure_access_key(&fs, "spark", Some("AK1"), true, &spec(Some("AK1"))) .await .unwrap(); assert_eq!( @@ -383,15 +356,15 @@ mod tests { async fn lost_secret_revokes_and_reissues() { let mut fs = MockRustFs::new(); fs.expect_get_access_key() - .return_once(|_, _, _| Ok(Some(ServiceAccount::new("AK1")))); + .return_once(|_| Ok(Some(ServiceAccount::new("AK1")))); fs.expect_delete_access_key() - .withf(|_, _, ak| ak == "AK1") - .return_once(|_, _, _| Ok(())); + .withf(|ak| ak == "AK1") + .return_once(|_| Ok(())); fs.expect_create_access_key() - .withf(|_, _, ak, _, _, _| ak == "AK1") - .return_once(|_, _, _, _, _, _| Ok(())); + .withf(|_, ak, _, _, _| ak == "AK1") + .return_once(|_, _, _, _, _| Ok(())); - match ensure_access_key(&fs, "spark", "pw", Some("AK1"), false, &spec(None)) + match ensure_access_key(&fs, "spark", Some("AK1"), false, &spec(None)) .await .unwrap() { @@ -403,12 +376,12 @@ mod tests { #[tokio::test] async fn key_deleted_serverside_is_recreated() { let mut fs = MockRustFs::new(); - fs.expect_get_access_key().return_once(|_, _, _| Ok(None)); + fs.expect_get_access_key().return_once(|_| Ok(None)); fs.expect_create_access_key() - .withf(|_, _, ak, _, _, _| ak == "AK1") - .return_once(|_, _, _, _, _, _| Ok(())); + .withf(|_, ak, _, _, _| ak == "AK1") + .return_once(|_, _, _, _, _| Ok(())); - match ensure_access_key(&fs, "spark", "pw", Some("AK1"), true, &spec(Some("AK1"))) + match ensure_access_key(&fs, "spark", Some("AK1"), true, &spec(Some("AK1"))) .await .unwrap() { @@ -420,16 +393,9 @@ mod tests { #[tokio::test] async fn access_key_equal_to_username_is_rejected_without_api_calls() { let fs = MockRustFs::new(); // any call panics - let err = ensure_access_key( - &fs, - "spark", - "pw", - Some("spark"), - false, - &spec(Some("spark")), - ) - .await - .expect_err("collision must be rejected"); + let err = ensure_access_key(&fs, "spark", Some("spark"), false, &spec(Some("spark"))) + .await + .expect_err("collision must be rejected"); assert!(err.is_config_error(), "should not be retried hot: {err}"); assert!( err.to_string() @@ -437,10 +403,25 @@ mod tests { ); } + #[tokio::test] + async fn password_ref_is_rejected_as_a_spec_error() { + let fs = MockRustFs::new(); // any call panics + let mut s = spec(Some("AK1")); + s.password_ref = Some(SecretKeyRef { + name: "spark-password".into(), + key: None, + }); + let err = ensure_access_key(&fs, "spark", Some("AK1"), true, &s) + .await + .expect_err("a stale passwordRef must be rejected"); + assert!(err.is_config_error(), "should not be retried hot: {err}"); + assert!(err.to_string().contains("passwordRef was removed in 0.7.0")); + } + #[tokio::test] async fn cleanup_skips_revoking_a_colliding_key() { let fs = MockRustFs::new(); // delete_access_key must not be called - cleanup_access_key(&fs, "spark", "pw", Some("spark"), &spec(Some("spark"))) + cleanup_access_key(&fs, "spark", Some("spark"), &spec(Some("spark"))) .await .unwrap(); } @@ -450,7 +431,7 @@ mod tests { let fs = MockRustFs::new(); // any call panics let mut s = spec(Some("AK1")); s.deletion_policy = DeletionPolicy::Retain; - cleanup_access_key(&fs, "spark", "pw", Some("AK1"), &s) + cleanup_access_key(&fs, "spark", Some("AK1"), &s) .await .unwrap(); } diff --git a/tests/e2e_k3s.rs b/tests/e2e_k3s.rs index 24d0759..4626b27 100644 --- a/tests/e2e_k3s.rs +++ b/tests/e2e_k3s.rs @@ -160,20 +160,13 @@ async fn operator_reconciles_crs_against_rustfs() { document: json!({ "Version": "2012-10-17", "Statement": [ + // No admin:*ServiceAccount grants: since 0.7.0 the + // operator issues keys with its own admin credential + // via targetUser, so the owning user needs none. { "Effect": "Allow", "Action": ["s3:GetObject"], "Resource": ["arn:aws:s3:::e2e-bucket/*"] - }, - // required for the user to manage its own access keys - { - "Effect": "Allow", - "Action": [ - "admin:CreateServiceAccount", - "admin:ListServiceAccounts", - "admin:RemoveServiceAccount" - ], - "Resource": ["arn:aws:s3:::*"] } ] }), @@ -289,10 +282,7 @@ async fn operator_reconciles_crs_against_rustfs() { AccessKeySpec { connection: conn.clone(), user: "e2e-user".into(), - password_ref: SecretKeyRef { - name: "e2e-user-creds".into(), - key: None, - }, + password_ref: None, access_key: None, description: Some("e2e".into()), policy: None, @@ -326,10 +316,7 @@ async fn operator_reconciles_crs_against_rustfs() { assert_eq!(get_key("endpoint"), endpoint); assert!(!get_key("secretKey").is_empty()); assert!( - fs.get_access_key("e2e-user", "e2e-password-123", &issued_ak) - .await - .unwrap() - .is_some(), + fs.get_access_key(&issued_ak).await.unwrap().is_some(), "issued key must exist in RustFS" ); @@ -339,11 +326,7 @@ async fn operator_reconciles_crs_against_rustfs() { .await .expect("delete AccessKey CR"); eventually("access key revoked in RustFS", 60, || async { - matches!( - fs.get_access_key("e2e-user", "e2e-password-123", &issued_ak) - .await, - Ok(None) - ) + matches!(fs.get_access_key(&issued_ak).await, Ok(None)) }) .await; eventually("credentials secret garbage-collected", 60, || async { diff --git a/tests/integration_rustfs.rs b/tests/integration_rustfs.rs index 3b91fd9..1ab9470 100644 --- a/tests/integration_rustfs.rs +++ b/tests/integration_rustfs.rs @@ -67,12 +67,13 @@ async fn provider_manages_buckets_policies_and_users() { let user = fs.get_user("it-user").await.unwrap().unwrap(); assert!(user.policies().contains(&"it-policy".to_string())); - // --- access keys (service accounts): issued as the user, usable for S3 --- + // --- access keys (service accounts): issued by the admin via targetUser --- + // No admin:*ServiceAccount grants here on purpose: since 0.7.0 the owning + // user needs none of them, so their absence is part of what this asserts. let allow_all = json!({ "Version": "2012-10-17", "Statement": [ - {"Effect": "Allow", "Action": ["s3:*"], "Resource": ["arn:aws:s3:::*"]}, - {"Effect": "Allow", "Action": ["admin:CreateServiceAccount", "admin:ListServiceAccounts", "admin:RemoveServiceAccount"], "Resource": ["arn:aws:s3:::*"]} + {"Effect": "Allow", "Action": ["s3:*"], "Resource": ["arn:aws:s3:::*"]} ] }); fs.put_policy("it-allow-all", &allow_all.to_string()) @@ -85,28 +86,14 @@ async fn provider_manages_buckets_policies_and_users() { "ITSAKEY1234567890ABC", "it-sa-secret-key-12345678901234567890", ); - assert!( - fs.get_access_key("it-user", "it-secret-key-123", sa_ak) - .await - .unwrap() - .is_none() - ); - fs.create_access_key( - "it-user", - "it-secret-key-123", - sa_ak, - sa_sk, - Some("integration".into()), - None, - ) - .await - .unwrap(); - assert!( - fs.get_access_key("it-user", "it-secret-key-123", sa_ak) - .await - .unwrap() - .is_some() - ); + assert!(fs.get_access_key(sa_ak).await.unwrap().is_none()); + fs.create_access_key("it-user", sa_ak, sa_sk, Some("integration".into()), None) + .await + .unwrap(); + // The key must be parented to it-user, not to the admin that created it — + // this is what `targetUser` buys and the reason no password is needed. + let sa = fs.get_access_key(sa_ak).await.unwrap().expect("key exists"); + assert_eq!(sa.parent_user.as_deref(), Some("it-user")); // the issued credentials authenticate and authorize real S3 calls let sa_provider = rustfs_operator::provider::RustFsProvider::connect( rustfs_operator::provider::ConnectionInfo { @@ -120,15 +107,8 @@ async fn provider_manages_buckets_policies_and_users() { .await .unwrap(); assert!(sa_provider.bucket_exists("it-bucket").await.unwrap()); - fs.delete_access_key("it-user", "it-secret-key-123", sa_ak) - .await - .unwrap(); - assert!( - fs.get_access_key("it-user", "it-secret-key-123", sa_ak) - .await - .unwrap() - .is_none() - ); + fs.delete_access_key(sa_ak).await.unwrap(); + assert!(fs.get_access_key(sa_ak).await.unwrap().is_none()); fs.set_user_status("it-user", false).await.unwrap(); // replace semantics: setting a different set drops the old attachment