Copilot commented on code in PR #11103:
URL: https://github.com/apache/ozone/pull/11103#discussion_r3879330896
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/iam/IamSessionPolicyResolver.java:
##########
@@ -141,14 +148,20 @@ public static Set<AssumeRoleRequest.OzoneGrant>
resolve(String policyJson, Strin
final Set<JsonNode> statements =
parseJsonAndRetrieveStatements(policyJson);
for (JsonNode stmt : statements) {
+ validateSupportedStatementFields(stmt);
validateEffectInJsonStatement(stmt);
- final Set<String> actions = readStringOrArray(stmt.get("Action"));
- final Set<String> resources = readStringOrArray(stmt.get("Resource"));
+ final Set<String> actions =
readRequiredStringOrArray(stmt.get("Action"), "Action");
+ final Set<String> resources =
readRequiredStringOrArray(stmt.get("Resource"), "Resource");
// Parse prefixes from conditions, if any
final Condition condition = parsePrefixesFromConditions(stmt);
+ // An empty s3:prefix array matches no prefixes and therefore grants no
access (AWS behavior).
+ if (condition != null && condition.prefixes.isEmpty()) {
Review Comment:
`condition.prefixes.isEmpty()` assumes `prefixes` is always non-null, but
other code in this class still guards `condition.prefixes != null` (e.g., later
resource processing). If `Condition` can ever be constructed with a null
`prefixes`, this will throw an NPE. Either (mandatory) enforce a non-null
invariant for `Condition.prefixes` (e.g., via constructor validation/default to
`Collections.emptySet()`), or (simpler) guard here with `condition.prefixes !=
null && condition.prefixes.isEmpty()`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3AssumeRoleRequest.java:
##########
@@ -491,6 +490,40 @@ public void testAssumeRoleWithSessionPolicyPresent()
throws IOException {
assertMarkForAuditCalled(requestWithCredentials);
}
+ @Test
+ public void testMalformedSessionPolicyDoesNotIssueCredentials() throws
IOException {
+ final String sessionPolicy = "{\n" +
+ " \"Statement\": [{\n" +
+ " \"Effect\": \"Allow\",\n" +
+ " \"Action\": \"s3:GetObject\",\n" +
+ " \"Action\": \"s3:*\",\n" +
+ " \"Resource\": \"arn:aws:s3:::bucket1/*\"\n" +
+ " }]\n" +
+ "}";
+ final OMRequest omRequest = baseOmRequestBuilder()
+ .setAssumeRoleRequest(
+ AssumeRoleRequest.newBuilder()
+ .setRoleArn(ROLE_ARN_1)
+ .setRoleSessionName(SESSION_NAME)
+ .setDurationSeconds(3600)
+ .setAwsIamSessionPolicy(sessionPolicy)
+ .setRequestId(REQUEST_ID)
+ ).build();
+
+ final S3AssumeRoleRequest request = new S3AssumeRoleRequest(omRequest,
CLOCK);
+ final OMRequest preExecutedRequest = request.preExecute(ozoneManager);
+ final S3AssumeRoleRequest requestWithCredentials = new
S3AssumeRoleRequest(preExecutedRequest, CLOCK);
+ final OMClientResponse response =
requestWithCredentials.validateAndUpdateCache(ozoneManager, context);
+ final OMResponse omResponse = response.getOMResponse();
+
+
assertThat(omResponse.getStatus()).isEqualTo(Status.MALFORMED_POLICY_DOCUMENT);
+ assertThat(omResponse.getMessage()).isEqualTo("IAM session policy:
Duplicate field 'Action' in session policy");
Review Comment:
This test asserts an exact, fully formatted error message string, which
tends to be brittle (minor wording/punctuation changes will break it even if
behavior is correct). Consider (mandatory if message text is not part of the
public contract) asserting on the status code plus a stable substring/key
(e.g., contains `Duplicate field 'Action'`) rather than full equality.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/security/acl/iam/IamSessionPolicyResolver.java:
##########
@@ -203,6 +216,10 @@ private static void validateInputParameters(String
policyJson, String volumeName
* Parses IAM session policy and retrieve the statement(s).
*/
private static Set<JsonNode> parseJsonAndRetrieveStatements(String
policyJson) throws OMException {
+ // Jackson's tree model silently collapses duplicate keys (last value
wins), which could let a caller smuggle
+ // broader permissions than intended. Detect them up front so we can
reject them with the exact field name.
+ checkForDuplicateFields(policyJson);
Review Comment:
Duplicate-key detection currently does an additional full streaming pass
over `policyJson` and then parses again into a tree. If policy documents can be
large or frequently evaluated, this adds avoidable overhead. Consider
(optional) enabling Jackson's strict duplicate detection on the parser used for
`readTree` (so duplicates are detected during the single parse) and translating
that parser exception into the desired `OMException` message.
--
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]
