Skip to content

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

Open
rich7420 wants to merge 1 commit into
apache:masterfrom
rich7420:HDDS-15467
Open

HDDS-15467. Do not fall back to the OM starter user in OMClientRequest#10469
rich7420 wants to merge 1 commit 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().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant