Siyao Meng created HDDS-17196:
---------------------------------
Summary: Tenant admin request is rejected with PERMISSION_DENIED
while an admin revokes another accessId of the same user
Key: HDDS-17196
URL: https://issues.apache.org/jira/browse/HDDS-17196
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-6-is-tenant-admin-skip-missing-access-id.patch
h3. Mechanism
{{OMMultiTenantManagerImpl.isTenantAdmin(String, String, boolean)}} reads the
user's {{principalToAccessIdsTable}} row and then looks up each listed accessId
in {{tenantAccessIdTable}}, without a lock. When one of those lookups returns
null it returns {{false}} right away instead of checking the remaining
accessIds. The comment on that branch already says the row "could be null since
we may not have a lock on the tenant", and {{OzoneManager.tenantGetUserInfo}}
skips such an accessId with a warning, so skipping was the intent.
{{OMTenantRevokeUserAccessIdRequest.validateAndUpdateCache}} first updates the
user's principal row and then deletes the accessId row. A reader that reads the
principal row before the revoke and the accessId row after it gets null for the
revoked accessId. If that accessId is looked at before the one that makes the
user a tenant admin (the accessIds are a {{HashSet}}, so the order depends on
the names), the check fails.
h3. Trigger
A tenant admin check of a user reads the user's {{principalToAccessIdsTable}}
row while another accessId of the same user is being revoked: the check reads
the principal row before the revoke updates it and the {{tenantAccessIdTable}}
row after the revoke deletes it.
h3. Impact
* The user's request fails with {{PERMISSION_DENIED}}, although the user's
admin row of that tenant was never touched. Affected: preExecute of tenant user
assign and revoke and of tenant admin assign and revoke ({{checkTenantAdmin}}),
and {{listUsersInTenant}}.
* Stored state stays correct and a retry succeeds. Nothing in OM retries the
request.
* If a principal row permanently lists an accessId without a row (a dangling
reference, which a failed tenant user assign can leave behind, filed
separately), the same early return makes the denial permanent for that user
whenever the dangling accessId is looked at first. Argued from the code; the
regression test uses such a state, the full sequence was not run.
h3. Reproduction
The issue has no standalone reproduction. The regression test in the attached
patch covers it (see below).
h3. Patch
[^CR-6-is-tenant-admin-skip-missing-access-id.patch], against
a6b7bdb937109ba2893688b41a89470bc95c88cf. It also applies to master at
1753288f333.
A missing {{tenantAccessIdTable}} row now continues the loop instead of
returning {{false}}. This cannot make a user a tenant admin without an admin
row: the result is {{true}} only from an existing {{tenantAccessIdTable}} row
that names the requested tenant with the admin flag, and a user cannot hold two
accessIds in the same tenant. Covered by
{{testIsTenantAdminSkipsMissingAccessId}} in the existing
{{TestOMMultiTenantManagerImpl}}, which fails without the change and also
checks that, past the missing row, an admin without the delegated flag still
fails the delegated admin check. With it the tenant manager, tenant request and
{{TestS3GetSecretRequest}} unit suites pass and checkstyle is clean.
Found by code review of the OM multi-tenancy and Ranger sync paths, as part of
the TLA+ verification effort under HDDS-15926, on commit
a6b7bdb937109ba2893688b41a89470bc95c88cf. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. The attached patch is
a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
