Copilot commented on code in PR #11134:
URL: https://github.com/apache/ozone/pull/11134#discussion_r3920305357
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3AssumeRoleRequest.java:
##########
@@ -158,33 +207,15 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
Exception exception = null;
OMClientResponse omClientResponse;
try {
- // Validate duration
- S3STSUtils.validateDuration(durationSeconds);
-
- // Validate role session name
- S3STSUtils.validateRoleSessionName(roleSessionName);
-
- // Validate role ARN and extract role
- final String targetRoleName =
AwsRoleArnValidator.validateAndExtractRoleNameFromArn(roleArn);
-
- // Note: The IamSessionPolicyResolver validates the awsIamPolicy length
internally
-
- if (!omRequest.hasS3Authentication()) {
+ if (Strings.isNullOrEmpty(tempAccessKeyId) ||
Strings.isNullOrEmpty(secretAccessKey) ||
+ Strings.isNullOrEmpty(roleId) || Strings.isNullOrEmpty(sessionToken)
|| expirationEpochSeconds <= 0) {
throw new OMException(
- "S3AssumeRoleRequest does not have S3 authentication",
OMException.ResultCodes.INVALID_REQUEST);
+ "UpdateAssumeRoleRequest is missing leader-generated AssumeRole
fields",
+ OMException.ResultCodes.INVALID_REQUEST);
Review Comment:
validateAndUpdateCache rejects UpdateAssumeRoleRequest entries that don’t
have sessionToken/expirationEpochSeconds. During a rolling upgrade, an older OM
leader will replicate UpdateAssumeRoleRequest without these newly-added
optional proto fields, and newer OMs will start returning INVALID_REQUEST for
otherwise-valid entries.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ResolvedBucket.java:
##########
@@ -44,11 +46,18 @@ public class ResolvedBucket {
private final String realBucket;
private final String bucketOwner;
private final BucketLayout bucketLayout;
+ private final Collection<Pair<String, String>> linkChain;
public ResolvedBucket(String requestedVolume, String requestedBucket,
OmBucketInfo resolved) {
+ this(requestedVolume, requestedBucket, resolved, Collections.emptyList());
+ }
+
+ public ResolvedBucket(String requestedVolume, String requestedBucket,
+ OmBucketInfo resolved, Collection<Pair<String, String>> linkChain) {
this.requestedVolume = requestedVolume;
this.requestedBucket = requestedBucket;
+ this.linkChain = linkChain == null ? Collections.emptyList() : linkChain;
if (resolved != null) {
Review Comment:
ResolvedBucket stores the provided linkChain collection directly. Since
OzoneManager passes a mutable Set that is used during resolution, this allows
accidental mutation after construction and exposes internal state via
linkChain(). It should defensively copy and make the collection unmodifiable.
This issue also appears on line 81 of the same file.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3AssumeRoleRequest.java:
##########
@@ -274,13 +298,134 @@ String getSessionPolicy(OzoneManager ozoneManager,
String originalAccessKeyId, S
final Set<OzoneGrant> grants = Strings.isNullOrEmpty(awsIamPolicy) ?
null :
- IamSessionPolicyResolver.resolve(awsIamPolicy, volumeName,
IamSessionPolicyResolver.AuthorizerType.RANGER);
+ resolveGrantsAgainstBucketLinks(
+ IamSessionPolicyResolver.resolve(awsIamPolicy, volumeName,
IamSessionPolicyResolver.AuthorizerType.RANGER),
+ (linkVolume, linkBucket) ->
ozoneManager.resolveBucketLink(Pair.of(linkVolume, linkBucket), true, false));
return ozoneManager.getAccessAuthorizer().generateAssumeRoleSessionPolicy(
new org.apache.hadoop.ozone.security.acl.AssumeRoleRequest(
hostName, remoteIp, ugi, targetRoleName, grants));
}
+ /**
+ * Rewrites the resolved session-policy grants so that any bucket, key, or
prefix resource that names a
+ * bucket link is anchored to the link's source volume and bucket - the
resource paths the OM authorizes
+ * against once the link is resolved at request time. READ on each link
bucket in the chain (and, when the
+ * chain crosses volumes, READ on each distinct volume except the requested
one) is retained so OM can follow
+ * every hop at request time, which keeps the generated token as small as
possible.
+ * <p>
+ * The link target is resolved when the token is generated, so the token
grants access to whatever the link
+ * points to at that moment. If the link is later re-pointed, the token no
longer grants access to the new
+ * target.
+ *
+ * @param grants the grants produced by {@link
IamSessionPolicyResolver}, possibly {@code null}
+ * @param linkResolver resolves a (volume, bucket) pair to its link target
+ * @return the link-aware grants, or the input unchanged when there is
nothing to resolve
+ */
+ @VisibleForTesting
+ static Set<OzoneGrant> resolveGrantsAgainstBucketLinks(Set<OzoneGrant>
grants,
+ BucketLinkResolver linkResolver) throws IOException {
+ if (grants == null || grants.isEmpty()) {
+ return grants;
+ }
+
+ final Map<Pair<String, String>, ResolvedBucket> resolutionCache = new
HashMap<>();
+ final Set<IOzoneObj> linkFollowObjects = new LinkedHashSet<>();
+ final Set<OzoneGrant> resolvedGrants = new LinkedHashSet<>();
+
+ for (OzoneGrant grant : grants) {
+ final Set<IOzoneObj> resolvedObjects = new LinkedHashSet<>();
+ for (IOzoneObj object : grant.getObjects()) {
+ resolvedObjects.add(
+ resolveObjectAgainstBucketLink((OzoneObj) object, linkResolver,
resolutionCache, linkFollowObjects));
+ }
+ resolvedGrants.add(new OzoneGrant(resolvedObjects,
grant.getPermissions(), grant.getS3Actions()));
+ }
+
+ // Retain only the READ required to follow each link hop at request time.
+ if (!linkFollowObjects.isEmpty()) {
+ resolvedGrants.add(new OzoneGrant(linkFollowObjects,
EnumSet.of(ACLType.READ)));
+ }
+
+ return resolvedGrants;
+ }
+
+ /**
+ * Resolves a single grant object against its bucket link. Bucket, key, and
prefix objects that name a
+ * link bucket are rewritten to the link's source volume and bucket, and the
READ needed to follow each hop
+ * in the link chain is collected in {@code linkFollowObjects}. All other
objects (volume resources and
+ * wildcard buckets) are returned unchanged.
+ */
+ private static IOzoneObj resolveObjectAgainstBucketLink(OzoneObj object,
BucketLinkResolver linkResolver,
+ Map<Pair<String, String>, ResolvedBucket> resolutionCache,
Set<IOzoneObj> linkFollowObjects)
+ throws IOException {
+ final OzoneObj.ResourceType resourceType = object.getResourceType();
+ if (resourceType != OzoneObj.ResourceType.BUCKET
+ && resourceType != OzoneObj.ResourceType.KEY
+ && resourceType != OzoneObj.ResourceType.PREFIX) {
+ return object;
+ }
+
+ final String volumeName = object.getVolumeName();
+ final String bucketName = object.getBucketName();
+ // Wildcard or unspecified names cannot correspond to a concrete link
bucket.
+ if (StringUtils.isBlank(volumeName) || StringUtils.isBlank(bucketName) ||
hasWildcard(volumeName) ||
+ hasWildcard(bucketName)) {
+ return object;
+ }
+
+ final Pair<String, String> requested = Pair.of(volumeName, bucketName);
+ ResolvedBucket resolved = resolutionCache.get(requested);
+ if (resolved == null) {
+ resolved = linkResolver.resolve(volumeName, bucketName);
+ resolutionCache.put(requested, resolved);
+ }
Review Comment:
resolutionCache currently doesn’t actually cache a null resolution: when
linkResolver returns null, the map stores (key -> null) but a later get() still
returns null and triggers another resolve call. This can repeatedly call
resolveBucketLink for the same non-existent/dangling bucket within one token
generation.
##########
hadoop-hdds/docs/content/design/ozone-sts.md:
##########
@@ -117,6 +117,15 @@ team agreed that behavior is fine for actions, but does
not work for Conditions,
restrict calls by sourceIp, and if we silently ignore this, the client may
incorrectly think the temporary credentials
are restricted for use by that IP address, so the consensus was to reject the
request for that scenario.
+### 3.3.2 Additional Context on Linked Buckets
+
+In Ozone, one may configure a chain of bucket links. In the scenario where
one desires to call the AssumeRole api where the resource
+is a linked bucket, ensure the Ranger policies for the role have the proper
permissions for each link in the chain as well
Review Comment:
In the linked-bucket section, “AssumeRole api” should be capitalized as
“AssumeRole API” for consistency with the rest of the document.
--
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]
