Copilot commented on code in PR #10009:
URL: https://github.com/apache/ozone/pull/10009#discussion_r3271132719
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -662,7 +664,29 @@ public void close() {
*/
@VisibleForTesting
OMResponse runCommand(OMRequest request, TermIndex termIndex) {
+ boolean isStsThreadLocalSet = false;
try {
+ 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;
+ }
Review Comment:
The added STS apply-path behavior (invariant check + setting/clearing
STS_TOKEN ThreadLocal) isn’t covered by existing OzoneManagerStateMachine
tests. Please add a unit test that runs runCommand() with an OMRequest
containing S3Authentication+sessionToken+resolvedSts* fields and asserts the
ThreadLocal is set during handler.handleWriteRequest and cleared afterward,
plus a test that missing resolved fields yields an error response.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -112,15 +114,50 @@ public OMClientRequest(OMRequest omRequest) {
*/
public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
- LayoutVersion layoutVersion = LayoutVersion.newBuilder()
+ final LayoutVersion layoutVersion = LayoutVersion.newBuilder()
.setVersion(ozoneManager.getVersionManager().getMetadataLayoutVersion())
.build();
- omRequest = getOmRequest().toBuilder()
+
+ final OMRequest.Builder requestBuilder = getOmRequest().toBuilder()
.setUserInfo(getUserIfNotExists(ozoneManager))
- .setLayoutVersion(layoutVersion).build();
+ .setLayoutVersion(layoutVersion);
+
+ if (requestBuilder.hasS3Authentication()) {
+ requestBuilder.setS3Authentication(
+ resolveS3Authentication(requestBuilder.getS3Authentication(),
OzoneManager.getStsTokenIdentifier()));
+ }
+
Review Comment:
The new STS field resolution is only applied in
OMClientRequest.preExecute(). Any request class that overrides preExecute
without calling super will bypass resolveS3Authentication(), so STS requests
can reach Ratis apply without resolvedSts* fields and then fail
STSSecurityUtil.ensureResolvedStsFieldsInvariants() in
OzoneManagerStateMachine. To avoid breaking those request types, either enforce
this logic (eg make preExecute final and provide a hook) or update all
overriding preExecute implementations to call super.preExecute and build from
its returned request/builder (several existing bucket/ACL requests currently
don’t).
--
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]
