Skip to content

HDDS-15467. Do not fall back to the OM starter user in OMClientRequest#10469

Merged
adoroszlai merged 2 commits into
apache:masterfrom
rich7420:HDDS-15467
Jun 26, 2026
Merged

HDDS-15467. Do not fall back to the OM starter user in OMClientRequest#10469
adoroszlai merged 2 commits into
apache:masterfrom
rich7420:HDDS-15467

Conversation

@rich7420

@rich7420 rich7420 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

OMClientRequest#getUserIfNotExists() rebuilt the request UserInfo from
UserGroupInformation.getCurrentUser() — the OM starter/login user — whenever
the derived UserInfo was missing a username or a remote address. Because the
result feeds createUGI() and the ACL check, this is a fail-open to an
often-privileged identity: any request reaching preExecute() without complete
user info would silently run as the OM service user. The escalation is latent
today (real RPC and gRPC clients always carry user info), but a future change in
getUserInfo() that dropped the username or remote address for a client path
would silently grant the OM identity.

The only caller that relies on this fallback is the Trash emptier
(TrashOzoneFileSystem), and it already populates a complete UserInfo
(service user + OM address) on the request it builds. The fallback merely
re-derived the same values, because getUserInfo() did not carry the
caller-supplied host/address over when there was no RPC/gRPC context.

Dataflow

Before — every write request, on missing user info, is rebuilt as the OM
starter user:

preExecute() -> setUserInfo(getUserIfNotExists(om))
                  getUserInfo()                      // may be incomplete
                  if (!hasRemoteAddress() || !hasUserName())   // OR
                      userName = getCurrentUser()    // OM starter (admin)
                      address  = OM node address
                  -> createUGI() -> UGI(admin) -> checkAcls    // fail-open

After — no getCurrentUser() fallback; identity is either derived from the
client context, preserved from a request that already carries it (Trash), or
the request fails closed:

client request                         internal service (Trash)
  preExecute()                           builds request with its own UserInfo
   -> setUserInfo(getUserInfo())          (service user + OM address)
        userName <- RPC user / S3 id       userName <- omRequest.UserInfo
        address  <- RPC ip / gRPC ctx      address  <- omRequest.UserInfo (new branch)
   -> createUGI()                        -> createUGI() -> UGI(service user)
        userName present -> UGI(user)
        userName missing -> UNAUTHORIZED  // fail-closed, no admin escalation

So this PR:

  • makes getUserInfo() preserve the host/address already present on the request
    when neither an RPC nor a gRPC client context is available (mirroring how the
    username is already carried over for gRPC s3g requests);
  • removes the now-redundant getUserIfNotExists() and the
    getCurrentUser()/admin fallback;
  • points preExecute() and the OMKeyDeleteRequest / OMKeyRenameRequest /
    OMAllocateBlockRequest callers at getUserInfo().

Requests that genuinely have no identity now fail closed in createUGI()
(UNAUTHORIZED) instead of being granted the OM starter user. Real client
paths are unchanged (they always carry user info), and the Trash emptier keeps
its explicit service identity.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15467

How was this patch tested?

https://github.com/rich7420/ozone/actions/runs/27190426330

getUserIfNotExists() rebuilt the request UserInfo from
UserGroupInformation.getCurrentUser() (the OM starter/login user) whenever the
derived UserInfo was missing a username or remote address. This is a fail-open
to an often-privileged identity: any request reaching preExecute without
complete user info would silently run as the OM service user.

Internal callers that need this (the Trash emptier) already populate their own
UserInfo on the request, so getUserInfo() only needs to preserve a
caller-supplied host/address when there is no RPC or gRPC context. With that,
the getCurrentUser() fallback is redundant and is removed; requests with no
identity now fail closed in createUGI().

@adoroszlai adoroszlai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rich7420 for the patch. Few nits to reduce unnecessary processing.

// one uses direct omclient we might be in trouble.

UserInfo userInfo = getUserIfNotExists(ozoneManager);
UserInfo userInfo = getUserInfo();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to super.preExecute has already calculated and set userInfo on omRequest.

Suggested change
UserInfo userInfo = getUserInfo();
UserInfo userInfo = getOmRequest().getUserInfo();

We can also remove setUserInfo(userInfo) below:

.setDeleteKeyRequest(deleteKeyRequest.toBuilder()
.setKeyArgs(resolvedArgs))
.setUserInfo(getUserIfNotExists(ozoneManager)).build();
.setUserInfo(getUserInfo()).build();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setUserInfo(getUserInfo()).build();
.build();

.setRenameKeyRequest(renameKeyRequest.toBuilder().setToKeyName(dstKey)
.setKeyArgs(resolvedArgs))
.setUserInfo(getUserIfNotExists(ozoneManager)).build();
.setUserInfo(getUserInfo()).build();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setUserInfo(getUserInfo()).build();
.build();

@ivandika3 ivandika3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rich7420 . LGTM +1 after @adoroszlai review is addressed.

@adoroszlai adoroszlai added the om label Jun 25, 2026
@rich7420

Copy link
Copy Markdown
Contributor Author

@adoroszlai and @ivandika3 thanks for the review

@adoroszlai adoroszlai merged commit 6f0b5a3 into apache:master Jun 26, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants