HDDS-15137. [STS] Ensure each S3 API has an associated S3 Action by fmorg-git · Pull Request #10197 · apache/ozone

fmorg-git · 2026-05-05T23:54:27Z

Please describe your PR in detail:

In order to restrict STS tokens on a fine-grained basis by action, each S3 api needs to be associated with an S3 action. For example, both HeadObject and GetObject S3 apis are associated with "s3:GetObject" action. The actions will not have the s3: prefix when used internally in Ozone and Ranger.

Most apis have a 1-1 mapping with api and action like the HeadObject and GetObject apis mentioned previously. In order to avoid having to update multiple call sites, an S3GActionIamMapper is introduced to leverage the already existing api audit functionality and map api names to actions in a thread local (S3Authentication.getS3Action()). Then in OmMetadataReader.checkAcls(), the maybeAttachS3ActionFromThreadLocal function is used to read the thread local and set the s3Action in the RequestContext if it exists.

However, CopyObject and UploadPartCopy are special cases. There is no "s3:CopyObject" nor "s3:UploadPartCopy" action, but rather they both require "s3:GetObject" action on the source object and "s3:PutObject" action on the destination object. For these special cases, a helper method runWithS3ActionString is introduced in EndpointBase.java so the proper permissions can be checked on the source and on the destination by temporarily overriding the value in the thread local while the operation is invoked, then setting it back to the previous value when done.

Additionally, this ticket contains additional fixes found after testing with these new changes. For example, since we now have fine-grained actions, we no longer need to distinguish listing files permission vs download files permission with LIST acl vs READ acl. Both apis can use READ acl.

What is the link to the Apache JIRA

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

How was this patch tested?

unit tests, smoke tests

fmorg-git · 2026-05-27T20:41:22Z

hi @ChenSammi @sodonnel - this PR has been rebased and is ready for 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 15 out of 16 changed files in this pull request and generated 3 comments.

fmorg-git · 2026-06-11T19:54:18Z

+        final OzoneKeyDetails destKeyDetails = getClientProtocol().getKeyDetails(
+            volume.getName(), destBucket, destkey);


yes, the concern is correct, the suggested fix is incorrect. It is fixed in the follow-on PR https://github.com/apache/ozone/pull/10203/changes by removing the extra destKeyDetails call (which will help performance as well).

fmorg-git · 2026-06-11T23:24:13Z

+    final S3Authentication s3Authentication = OzoneManager.getS3Auth();
+    if (s3Authentication == null || !s3Authentication.hasS3Action()) {
+      return context;
+    }
+    return context.toBuilder()
+        .setS3Action(s3Authentication.getS3Action())
+        .build();


updated - a223e5c

fmorg-git · 2026-06-11T22:08:38Z

+  }
+
  protected OzoneVolume getVolume() throws IOException {


updated - 5868614

ChenSammi · 2026-06-10T08:56:48Z

 }

 message CommitKeyResponse {
-


This change is irrelevant, better reverted.

updated - 5868614

ChenSammi · 2026-06-10T08:56:57Z

      }
    }
  ]
-}


This change is irrelevant, better reverted.

updated - 5868614

ChenSammi · 2026-06-10T09:02:38Z

+   * @param context the original RequestContext
+   * @return RequestContext as before or with s3 action embedded
+   */
+  private RequestContext maybeAttachS3ActionFromThreadLocal(RequestContext context) {


Can maybeAttachS3ActionFromThreadLocal and maybeAttachSessionPolicyFromThreadLocal merge into one function?

BTW, can we move setS3Action and setSessionPolicy to where context is created?

Cannot found where context.getS3Action() is used. Is this a complete patch, or there will be following patch which requires this get function?

updated to combine the maybeAttach methods and move the setters to where the context is created: a223e5c

Cannot found where context.getS3Action() is used. Is this a complete patch, or there will be following patch which requires this get function?

This is used in theRangerOzoneAuthorizer to help authorize against the specific action. See apache/ranger#995 and in particular https://github.com/fmorg-git/ranger/blob/e6327b6bd039ef52d8c82e203be4d8c034214f09/plugin-ozone/src/main/java/org/apache/ranger/authorization/ozone/authorizer/RangerOzoneAuthorizer.java#L168-L173

BTW, can we move setS3Action and setSessionPolicy to where context is created?

originally, I implemented this suggestion but this broke some of my unit tests in TestOMMetadataReader. Now I remember why I did it the way I did, is because there are many sites where the RequestContext is created but they all funnel down to this method in OmMetadataReader:

public boolean checkAcls(OzoneObj obj, RequestContext context, boolean throwIfPermissionDenied)

So it's safer to have it in the one central location that everyone uses, because if we try to rely on adding to all the sites where RequestContext is created, if a new overloaded call site is added in future, someone would need to remember to update that one as well or else there would be a bug until it is realized, which is brittle. The second issue is that someone might call this method directly like the unit test does, and if we don't set the values here, it would break, just like the unit test. So if we need to set the values in this method anyway, we might as well just set it once. I understand it is an extra object allocation, but at least now the two maybeAttach* methods are combined into one, so it's one less allocation and the code is more maintainable with the one central location.

ChenSammi · 2026-06-10T10:04:12Z

-        long putLength;
-        try (OzoneOutputStream ozoneOutputStream = getClientProtocol()
+        final long putLength;
+        final OzoneOutputStream ozoneOutputStream = getClientProtocol()


runWithS3ActionString("PutObject" is missed here. ozoneOutputStream is not closed

Actually, here is the reason "PutObject" is not missed here. In this else branch of the code, the request action is set to S3GAction.CREATE_MULTIPART_KEY and this has a mapping in S3GActionIamMapper to "PutObject", so it's covered. In the if branch of the code, the request action is set to S3GAction.CREATE_MULTIPART_KEY_BY_COPY which is mapped to null in S3GActionIamMapper (by design, since it needs "GetObject" on the source and "PutObject" on the destination.)

Also, the ozoneOutputStream is closed on line 920 in the try-with-resources block.

I added a clarification comment about the runWithS3ActionString - e344aae

ChenSammi · 2026-06-10T10:04:31Z

+          final long finalLength = length;
+          final long bytesToCopy = length;
+          omMultipartCommitUploadPartInfo = runWithS3ActionString("PutObject", () -> {
+            final OzoneOutputStream ozoneOutputStream = getClientProtocol().createMultipartKey(


ozoneOutputStream is not closed.

the ozoneOutputStream is closed on line 898 in the try-with-resources block

…icy and s3Action and set them at the call sites Generated-By: Claude Code with manual updates by me

…ction

ChenSammi · 2026-06-12T04:58:07Z

@@ -628,8 +628,12 @@ public boolean checkAcls(ResourceType resType, StoreType storeType,
  public boolean checkAcls(OzoneObj obj, RequestContext context,


You can pass RequestContext.Builder in instead of RequestContext here.

yep, that's a much better idea. Updated - 77cfc5e

…Acl method

fmorg-git · 2026-06-12T10:00:18Z

Thanks for the review and merge.

fmorg-git marked this pull request as draft May 5, 2026 23:55

fmorg-git force-pushed the HDDS-15137 branch 2 times, most recently from cfd028f to 7b59609 Compare May 6, 2026 01:36

This was referenced May 6, 2026

HDDS-15182. [STS] Remove latent extra read on CopyObject api - make CopyPart work similarly #10203

Open

HDDS-15194. [STS] Update IamSessionPolicyResolver to return S3 Actions #10204

Draft

fmorg-git mentioned this pull request May 14, 2026

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

Draft

fmorg-git force-pushed the HDDS-15137 branch from 7b59609 to b634fda Compare May 15, 2026 23:08

ensure each S3 API has an associated S3 Action

c467b3b

fmorg-git force-pushed the HDDS-15137 branch from b634fda to c467b3b Compare May 27, 2026 17:05

fmorg-git marked this pull request as ready for review May 27, 2026 17:06

remove unnecessary method

a0b33cb

ChenSammi requested a review from Copilot June 10, 2026 08:28

Copilot started reviewing on behalf of ChenSammi June 10, 2026 08:29 View session

Copilot AI reviewed Jun 10, 2026

ChenSammi requested a review from Copilot June 10, 2026 08:51

Copilot AI reviewed Jun 10, 2026

View reviewed changes

Copilot started reviewing on behalf of ChenSammi June 10, 2026 08:51 View session

ChenSammi reviewed Jun 10, 2026

View reviewed changes

Fabian Morgan added 2 commits June 11, 2026 14:24

pr review comments - whitespace fixes

5868614

pr review comments for ChenSammi - combine methods that setSessionPol…

2daec4f

…icy and s3Action and set them at the call sites Generated-By: Claude Code with manual updates by me

fmorg-git force-pushed the HDDS-15137 branch from a223e5c to 2daec4f Compare June 11, 2026 23:12

pr review comments - add clarification comment for runWithS3ActionString

e344aae

fmorg-git requested a review from ChenSammi June 11, 2026 23:53

ChenSammi approved these changes Jun 12, 2026

View reviewed changes

need to have centralized location for setting session policy and s3 a…

c8044fc

…ction

fmorg-git requested a review from ChenSammi June 12, 2026 04:54

ChenSammi reviewed Jun 12, 2026

View reviewed changes

pr review comment for ChenSammi - use RequestContext.Builder in check…

77cfc5e

…Acl method

ChenSammi merged commit fd0ae5c into apache:HDDS-13323-sts Jun 12, 2026
47 checks passed

		final OzoneKeyDetails destKeyDetails = getClientProtocol().getKeyDetails(
		volume.getName(), destBucket, destkey);

		@@ -628,8 +628,12 @@ public boolean checkAcls(ResourceType resType, StoreType storeType,
		public boolean checkAcls(OzoneObj obj, RequestContext context,

Conversation

fmorg-git commented May 5, 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!

fmorg-git commented May 27, 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!

ChenSammi Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

fmorg-git commented Jun 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fmorg-git commented May 5, 2026 •

edited

Loading

ChenSammi Jun 10, 2026 •

edited

Loading

fmorg-git Jun 12, 2026 •

edited

Loading

ChenSammi Jun 10, 2026 •

edited

Loading

fmorg-git Jun 11, 2026 •

edited

Loading