Copilot commented on code in PR #10197:
URL: https://github.com/apache/ozone/pull/10197#discussion_r3386827594
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataReader.java:
##########
@@ -692,6 +687,22 @@ private RequestContext
maybeAttachSessionPolicyFromThreadLocal(RequestContext co
.build();
}
+ /**
+ * Attaches s3 action to RequestContext if an S3Authentication is found in
the Ozone Manager thread local,
+ * and it has an s3 action. Otherwise, returns the RequestContext as it was
before.
+ * @param context the original RequestContext
+ * @return RequestContext as before or with s3 action embedded
+ */
+ private RequestContext maybeAttachS3ActionFromThreadLocal(RequestContext
context) {
+ final S3Authentication s3Authentication = OzoneManager.getS3Auth();
+ if (s3Authentication == null || !s3Authentication.hasS3Action()) {
+ return context;
+ }
+ return context.toBuilder()
+ .setS3Action(s3Authentication.getS3Action())
+ .build();
Review Comment:
`maybeAttachS3ActionFromThreadLocal` only checks `hasS3Action()`. With
protobuf `optional string`, `hasS3Action()` can be true even when the value is
the empty string, which would then propagate an empty action into
`RequestContext` and potentially cause unexpected authorization behavior. It
should treat empty action the same as unset (similar to how sessionToken is
handled elsewhere).
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1103,22 +1107,25 @@ private CopyObjectResponse copyObject(OzoneVolume
volume,
throw ex;
}
- try (OzoneInputStream src = getClientProtocol().getKey(volume.getName(),
- sourceBucket, sourceKey)) {
+ try (OzoneInputStream src = runWithS3ActionString(
+ "GetObject", () -> getClientProtocol().getKey(volume.getName(),
sourceBucket, sourceKey));
+ DigestInputStream sourceDigestInputStream = new
DigestInputStream(src, md5Digest)) {
getMetrics().updateCopyKeyMetadataStats(startNanos);
- sourceDigestInputStream = new DigestInputStream(src,
getMD5DigestInstance());
- copy(volume, sourceDigestInputStream, sourceKeyLen, destkey,
destBucket, replicationConfig,
- customMetadata, perf, startNanos, tags, writeConditions);
- }
+ runWithS3ActionString("PutObject", () -> {
+ copy(volume, sourceDigestInputStream, sourceKeyLen, destkey,
destBucket,
+ replicationConfig, customMetadata, perf, startNanos, tags,
writeConditions);
+ return null;
+ });
- final OzoneKeyDetails destKeyDetails = getClientProtocol().getKeyDetails(
- volume.getName(), destBucket, destkey);
+ final OzoneKeyDetails destKeyDetails =
getClientProtocol().getKeyDetails(
+ volume.getName(), destBucket, destkey);
Review Comment:
In CopyObject/UploadPartCopy flow, `destKeyDetails` is fetched after the
`PutObject` phase but outside the `runWithS3ActionString("PutObject", ...)`
override. At this point the thread-local action will have reverted (likely to
`GetObject`), which can incorrectly require `GetObject` permission on the
destination object just to build the response (ETag/LastModified), breaking
CopyObject for STS policies that grant PutObject on the destination but not
GetObject.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -225,6 +228,39 @@ protected void init() {
// hook method
}
+ /**
+ * Sets the IAM S3 action on thread-local {@link S3Auth} for fine-grained
STS authorization.
+ * Called when the handler resolves the {@link S3GAction}.
+ */
+ protected void applyS3Action(S3GAction action) {
+ if (s3Auth != null) {
+ s3Auth.setS3Action(S3GActionIamMapper.toS3ActionString(action));
+ }
+ }
+
+ /**
+ * Temporarily override the S3 action string set on {@link S3Auth} for
authorization.
+ * <p>
+ * This does not change S3G auditing (which is based on {@link S3GAction}).
+ * The action string is the IAM-style S3 action name without the {@code s3:}
prefix (for example
+ * {@code GetObject}, {@code PutObject}, {@code GetObjectTagging}).
+ * This is used for special case APIs like CopyObject that don't have a 1-1
s3 action mapping, but
+ * requires GetObject on the source file and PutObject on the destination
file.
+ */
+ protected <T, E extends Exception> T runWithS3ActionString(String s3Action,
CheckedSupplier<T, E> checkedSupplier)
+ throws E {
+ if (s3Auth == null) {
+ return checkedSupplier.get();
+ }
+ final String originalS3Action = s3Auth.getS3Action();
+ s3Auth.setS3Action(s3Action);
+ try {
+ return checkedSupplier.get();
+ } finally {
+ s3Auth.setS3Action(originalS3Action);
+ }
+ }
+
protected OzoneVolume getVolume() throws IOException {
Review Comment:
There is trailing whitespace on an otherwise blank line, which can trip some
whitespace/lint checks and makes diffs noisier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
