HDDS-14935. [STS] Handle Latent Inconsistencies in S3 API Acl Checks by fmorg-git · Pull Request #10009 · apache/ozone

fmorg-git · 2026-03-31T01:07:27Z

Please describe your PR in detail:

Currently, S3 APIs are not consistent in how ACL checks are applied. For example, PutObject (i.e. OMKeyCreateRequest, OMAllocateBlockRequest, OMKeyCommitRequest), DeleteObject (i.e. OMKeyDeleteRequest), PutObjectTagging (i.e. S3PutObjectTaggingRequest), etc. perform their ACL checks in preExecute() which is on the OM leader RPC thread.

However, APIs like DeleteBucket (i.e. OMBucketDeleteRequest), PutBucketAcl (i.e. OMBucketSetAclRequest), etc. perform their ACL checks in validateAndUpdateCache() which is on the Ratis apply thread. This affects STS in that the STSTokenIdentifier ThreadLocal currently is not available on the Ratis apply thread, so if the STS token has an inline session policy, some ACL checks that should pass would fail. This ticket addresses the inconsistency by ensuring the ThreadLocal is always available on the Ratis apply thread via updates to OzoneManagerStateMachine.

A separate PR is already open to move the checks to the correct place (#9653 and https://issues.apache.org/jira/browse/HDDS-13855), but this ticket is a fallback in case any future API has the check in the incorrect place, so it won't break STS.

What is the link to the Apache JIRA

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

How was this patch tested?

unit tests, smoke tests

github-actions · 2026-04-22T00:12:36Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

fmorg-git · 2026-04-22T00:22:28Z

adding comment to remove stale label

fmorg-git · 2026-05-18T15:47:13Z

hi @ChenSammi @sodonnel - just a quick reminder that this PR is ready to review. Thanks!

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

fmorg-git · 2026-05-21T02:36:58Z

+    final OMRequest.Builder requestBuilder = getOmRequest().toBuilder()
        .setUserInfo(getUserIfNotExists(ozoneManager))
-        .setLayoutVersion(layoutVersion).build();
+        .setLayoutVersion(layoutVersion);
+
+    if (requestBuilder.hasS3Authentication()) {
+      requestBuilder.setS3Authentication(
+          resolveS3Authentication(requestBuilder.getS3Authentication(), OzoneManager.getStsTokenIdentifier()));
+    }
+


updated to fix all overriding preExecute implementations and to add a checkstyle rule for future ones - 6d43f39

fmorg-git · 2026-05-21T02:35:40Z

+      if (ozoneManager.isSecurityEnabled() && request.hasS3Authentication()) {
+        // STS token verification runs on the leader RPC path so we don't need to recheck here on the apply
+        // after the log is committed
+        STSSecurityUtil.ensureResolvedStsFieldsInvariants(request);
+
+        final OzoneManagerProtocolProtos.S3Authentication s3Auth = request.getS3Authentication();
+        if (s3Auth.hasSessionToken() && !s3Auth.getSessionToken().isEmpty()) {
+          // ThreadLocal carries session policy for OmMetadataReader
+          final STSTokenIdentifier rehydratedTokenIdentifier = new STSTokenIdentifier(
+                  s3Auth.hasResolvedStsTempAccessKeyId() ? s3Auth.getResolvedStsTempAccessKeyId() : "",
+                  s3Auth.hasResolvedStsOriginalAccessKeyId() ? s3Auth.getResolvedStsOriginalAccessKeyId() : "",
+                  s3Auth.hasResolvedStsRoleArn() ? s3Auth.getResolvedStsRoleArn() : "",
+                  java.time.Instant.MAX, // ensure it deterministically is not expired
+                  "", // no secretAccessKey needed
+                  s3Auth.hasResolvedStsSessionPolicy() ? s3Auth.getResolvedStsSessionPolicy() : "",
+                  null // no encryption key needed
+              );
+          OzoneManager.setStsTokenIdentifier(rehydratedTokenIdentifier);
+          isStsThreadLocalSet = true;
+        }


updated - c483af5

ChenSammi · 2026-05-20T04:51:08Z


-    return getOmRequest().toBuilder()
+    // super.preExecute resolves S3Authentication (STS) for Ratis apply.  Merge SetAclRequest changes on top.
+    final OMRequest request = super.preExecute(ozoneManager);


You mentioned OMBucketDeleteRequest in the JIRA description. Shall we call the super.preExecute in OMBucketDeleteRequest too?

There are two other requests, OMBucketAddAclRequest, OMBucketRemoveAclRequest. So we should consider move this to OMBucketAclRequest.

Agreed, we should call super.preExecute() in OMBucketDeleteRequest as well.

You mentioned OMBucketDeleteRequest in the JIRA description. Shall we call the super.preExecute in OMBucketDeleteRequest too?

OMBucketDeleteRequest doesn't override preExecute() method, so it automatically gets the resolved sts fields behavior. The issue happens when a subclass overrides preExecute() but does not call super.preExecute().

There are two other requests, OMBucketAddAclRequest, OMBucketRemoveAclRequest. So we should consider move this to OMBucketAclRequest.

updated as part of 6d43f39. I changed the subclasses directly.

ChenSammi · 2026-05-20T04:51:46Z

+      OzoneManagerProtocolProtos.S3Authentication s3Auth, STSTokenIdentifier stsTokenIdentifier) {
+    final OzoneManagerProtocolProtos.S3Authentication.Builder s3AuthBuilder = s3Auth.toBuilder();
+
+    if (s3Auth.hasSessionToken() && !s3Auth.getSessionToken().isEmpty() && stsTokenIdentifier != null) {


Is "s3Auth.hasSessionToken() && !s3Auth.getSessionToken().isEmpty()" and null stsTokenIdentifier allowed?

no this is not allowed. Updated to throw an error if this happens and a unit test coverage - b1ed2d2

…gerStateMachine

…ss that overrides preExecute calls super.preExecute. Add checkstyle rule for this as well

…hat stsTokenIdentifier is not null

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

ChenSammi

The last patch LGTM.

This reverts commit ca2a33a.

fmorg-git · 2026-05-27T16:06:17Z

Thanks all for the review and merge.

fmorg-git marked this pull request as draft March 31, 2026 01:07

kerneltime requested a review from sumitagrawl March 31, 2026 05:00

github-actions Bot added the stale label Apr 22, 2026

github-actions Bot removed the stale label Apr 23, 2026

fmorg-git force-pushed the HDDS-14935 branch from 8f9e629 to 5bbfa24 Compare May 5, 2026 23:32

fmorg-git marked this pull request as ready for review May 5, 2026 23:33

fmorg-git mentioned this pull request May 5, 2026

HDDS-15137. [STS] Ensure each S3 API has an associated S3 Action #10197

Open

fmorg-git mentioned this pull request May 14, 2026

HDDS-14810. [STS] Part 2 - STS Ranger Smoke Tests #9902

Draft

handle latent inconsistencies in s3 api acl checks

d1cc9b2

fmorg-git force-pushed the HDDS-14935 branch from 5bbfa24 to d1cc9b2 Compare May 15, 2026 15:58

ChenSammi requested a review from Copilot May 19, 2026 09:42

Copilot started reviewing on behalf of ChenSammi May 19, 2026 09:43 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

ChenSammi requested a review from Copilot May 20, 2026 03:49

Copilot started reviewing on behalf of ChenSammi May 20, 2026 03:49 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

ChenSammi reviewed May 20, 2026

View reviewed changes

Fabian Morgan added 3 commits May 20, 2026 19:14

pr review update for Copilot: add additional unit tests for OzoneMana…

c483af5

…gerStateMachine

pr review comments for Copilot and ChenSammi - ensure any request cla…

6d43f39

…ss that overrides preExecute calls super.preExecute. Add checkstyle rule for this as well

pr review update for ChenSammi - ensure if sessionToken is present, t…

b1ed2d2

…hat stsTokenIdentifier is not null

fmorg-git requested review from ChenSammi and Russole May 21, 2026 02:40

update unit tests after CI failures

fd5f08a

ChenSammi requested a review from Copilot May 26, 2026 08:36

Copilot started reviewing on behalf of ChenSammi May 26, 2026 08:36 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

ChenSammi requested a review from Copilot May 27, 2026 08:03

Copilot started reviewing on behalf of ChenSammi May 27, 2026 08:03 View session

Copilot AI reviewed May 27, 2026

View reviewed changes

Comment thread hadoop-hdds/dev-support/checkstyle/checkstyle.xml

Potential fix for pull request finding

ca2a33a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

ChenSammi approved these changes May 27, 2026

View reviewed changes

Revert "Potential fix for pull request finding"

af93237

This reverts commit ca2a33a.

ChenSammi merged commit 5665462 into apache:HDDS-13323-sts May 27, 2026
90 of 91 checks passed

Conversation

fmorg-git commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

github-actions Bot commented Apr 22, 2026

Uh oh!

fmorg-git commented Apr 22, 2026

Uh oh!

fmorg-git commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

ChenSammi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fmorg-git commented May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fmorg-git commented Mar 31, 2026 •

edited

Loading