Skip to content

HDDS-11732. Skip ACL check on bucket resolution while reading from snapshot #7446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
swamirishi merged 1 commit into from
Nov 17, 2024
Merged

HDDS-11732. Skip ACL check on bucket resolution while reading from snapshot #7446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ public ReferenceCounted<IOmMetadataReader> getActiveFsMetadataOrSnapshot(
// Updating the volumeName & bucketName in case the bucket is a linked bucket. We need to do this before a
// permission check, since linked bucket permissions and source bucket permissions could be different.
ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(Pair.of(volumeName,
bucketName), false);
bucketName), false, false);
Comment on lines 628 to +631
Copy link
Contributor

@smengcl smengcl Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @swamirishi for the patch.

As long as we make sure getActiveFsMetadataOrSnapshot() can ONLY be called by a privileged user that has admin permission, I am good with this. According to your updated PR description that seems to be the case here.

Because otherwise it becomes a potential security hole (info leak).

Copy link
Contributor

@hemantk-12 hemantk-12 Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the above comment since we are skipping ACL check now.

volumeName = resolvedBucket.realVolume();
bucketName = resolvedBucket.realBucket();
return (ReferenceCounted<IOmMetadataReader>) (ReferenceCounted<?>)
Expand Down
32 changes: 25 additions & 7 deletions hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4402,10 +4402,16 @@ public ResolvedBucket resolveBucketLink(Pair<String, String> requested,
}

public ResolvedBucket resolveBucketLink(Pair<String, String> requested,
boolean allowDanglingBuckets)
boolean allowDanglingBuckets) throws IOException {
return resolveBucketLink(requested, allowDanglingBuckets, isAclEnabled);
}

public ResolvedBucket resolveBucketLink(Pair<String, String> requested,
boolean allowDanglingBuckets,
boolean aclEnabled)
throws IOException {
OmBucketInfo resolved;
if (isAclEnabled) {
if (aclEnabled) {
UserGroupInformation ugi = getRemoteUser();
if (getS3Auth() != null) {
ugi = UserGroupInformation.createRemoteUser(
Expand All @@ -4416,15 +4422,26 @@ public ResolvedBucket resolveBucketLink(Pair<String, String> requested,
ugi,
remoteIp != null ? remoteIp : omRpcAddress.getAddress(),
remoteIp != null ? remoteIp.getHostName() :
omRpcAddress.getHostName(), allowDanglingBuckets);
omRpcAddress.getHostName(), allowDanglingBuckets, aclEnabled);
} else {
resolved = resolveBucketLink(requested, new HashSet<>(),
null, null, null, allowDanglingBuckets);
null, null, null, allowDanglingBuckets, aclEnabled);
}
return new ResolvedBucket(requested.getLeft(), requested.getRight(),
resolved);
}

private OmBucketInfo resolveBucketLink(
Pair<String, String> volumeAndBucket,
Set<Pair<String, String>> visited,
UserGroupInformation userGroupInformation,
InetAddress remoteAddress,
String hostName,
boolean allowDanglingBuckets) throws IOException {
return resolveBucketLink(volumeAndBucket, visited, userGroupInformation, remoteAddress, hostName,
allowDanglingBuckets, isAclEnabled);
}

/**
* Resolves bucket symlinks. Read permission is required for following links.
*
Expand All @@ -4442,7 +4459,8 @@ private OmBucketInfo resolveBucketLink(
UserGroupInformation userGroupInformation,
InetAddress remoteAddress,
String hostName,
boolean allowDanglingBuckets) throws IOException {
boolean allowDanglingBuckets,
boolean aclEnabled) throws IOException {
Copy link
Contributor

@hemantk-12 hemantk-12 Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: aclEnabled and other parameters are not present in the java-doc comment. Please update it accordingly.


String volumeName = volumeAndBucket.getLeft();
String bucketName = volumeAndBucket.getRight();
Expand All @@ -4465,7 +4483,7 @@ private OmBucketInfo resolveBucketLink(
DETECTED_LOOP_IN_BUCKET_LINKS);
}

if (isAclEnabled) {
if (aclEnabled) {
final ACLType type = ACLType.READ;
checkAcls(ResourceType.BUCKET, StoreType.OZONE, type,
volumeName, bucketName, null, userGroupInformation,
Expand All @@ -4476,7 +4494,7 @@ private OmBucketInfo resolveBucketLink(
return resolveBucketLink(
Pair.of(info.getSourceVolume(), info.getSourceBucket()),
visited, userGroupInformation, remoteAddress, hostName,
allowDanglingBuckets);
allowDanglingBuckets, aclEnabled);
}

@VisibleForTesting
Expand Down