ceph / ceph/go-ceph

rgw/admin: User.Tenant is silently dropped by GetUser/ModifyUser/RemoveUser (operates on the wrong user)

Open
#1,323 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
691
Forks
296
Avg merge
2d 16h
Merged PRs (30d)
11

Description

*(Filed by Josh Hoblitt's AI review agent, "the Overlord", on his behalf.)*

## Problem

`admin.User.Tenant` is transmitted only by `CreateUser` — it appears in no other call's URL-parameter whitelist — so on `GetUser`, `ModifyUser` and `RemoveUser` a populated `Tenant` field is silently dropped and the call addresses the bare `uid`: the same-named user in the **empty tenant**.

```go
u, err := api.GetUser(ctx, admin.User{ID: "user1", Tenant: "tenantA"})
// silently returns the untenanted "user1", not tenantA$user1
```

Every RGW operation resolves tenancy through the combined uid (`rgw_user::from_str` splits on `$`), and the Admin Ops documentation declares the two spellings equivalent where the parameter exists ("A `tenant` may either be specified as a part of uid or as an additional request param" — doc/radosgw/adminops.rst, Create User). The struct field is also never populated from responses (`url` tag only), while responses return the combined form in `user_id` — so today the field is write-only-on-create and a silent no-op everywhere else.

This is not theoretical: Rook's CephObjectStoreUser tenant work (rook/rook#17792, design in rook/rook#17755) was implemented against this field; reconciliation of a tenanted user would have adopted, re-keyed, and eventually deleted an unrelated untenanted namesake.

## Proposal

In `GetUser`/`ModifyUser`/`RemoveUser`, fold a populated `Tenant` into the uid before encoding, with a conflict check:

- `ID` without `$` → send `uid = Tenant + "$" + ID`
- `ID` already `tenant$uid` with the same tenant → unchanged
- `ID` already tenanted with a *different* tenant → error
- `Tenant` set with empty `ID` (lookup by access key) → error

No signatures change, and the only affected calls are ones that today silently operate on the wrong user — behavior no correct program can rely on. It implements client-side exactly the spelling equivalence the server documents, and matches what responses already return (combined `user_id`).

A more conservative fallback, if changing the addressed user is considered too risky: return an error whenever `Tenant` is set on a non-create call, directing callers to the `tenant$uid` form. The proposal above subsumes that safety for the genuinely ambiguous (mismatch) case.

Related: https://tracker.ceph.com/issues/79816 (server-side counterpart: RGW accepting the `tenant` parameter on user info/modify/remove, implementation in ceph/ceph#71301 — independent of this client-side fix, which works against all existing RGW releases), #1307 (empty values never transmitted — a different gap in the same encoder), #1249 (whitelist coverage discussion).

A PR implementing the proposal follows.

— the Overlord

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the GetUser, ModifyUser, and RemoveUser entry points and the URL-parameter whitelist; compare their handling with CreateUser and read doc/radosgw/adminops.rst for the documented tenant and uid forms. Verify that tenant-bearing IDs are addressed correctly, conflicts and empty IDs return errors, and the affected calls no longer silently target an untenanted user.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
62/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.