From b5e36ddf8a8ec1a51e3d919a4618f1e6926ed8d6 Mon Sep 17 00:00:00 2001 From: Robert Stupp Date: Mon, 11 Nov 2024 12:48:12 +0100 Subject: [PATCH] adopt S3 + GCS iam policies to new object-storage layout --- .../catalog/files/gcs/GcsStorageSupplier.java | 4 +++- .../catalog/files/s3/S3IamPolicies.java | 13 ++++++++++--- .../catalog/files/s3/TestS3IamPolicies.java | 19 ++++++++++++------- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/gcs/GcsStorageSupplier.java b/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/gcs/GcsStorageSupplier.java index 8c12ec91497..91cf0bce0f5 100644 --- a/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/gcs/GcsStorageSupplier.java +++ b/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/gcs/GcsStorageSupplier.java @@ -52,7 +52,9 @@ public final class GcsStorageSupplier { private static final Logger LOGGER = LoggerFactory.getLogger(GcsStorageSupplier.class); - static final String RANDOMIZED_PART = "([A-Za-z0-9=]+/)?"; + + // Suitable for both old object-storage layout (before Iceberg 1.7.0) and new (since 1.7.0) + static final String RANDOMIZED_PART = "([A-Za-z0-9=]+/|[01]{4}/[01]{4}/[01]{4}/[01]{8}/)?"; private final HttpTransportFactory httpTransportFactory; private final GcsConfig gcsConfig; diff --git a/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/s3/S3IamPolicies.java b/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/s3/S3IamPolicies.java index 75d63441a57..27ec1c86299 100644 --- a/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/s3/S3IamPolicies.java +++ b/catalog/files/impl/src/main/java/org/projectnessie/catalog/files/s3/S3IamPolicies.java @@ -74,9 +74,12 @@ static String locationDependentPolicy(S3ClientIam clientIam, StorageLocations lo ArrayNode s3Prefix = stringLike.withArray("s3:prefix"); s3Prefix.add(path); s3Prefix.add(path + "/*"); - // For write.object-storage.enabled=true + // For write.object-storage.enabled=true / before Iceberg 1.7.0 s3Prefix.add("*/" + path); s3Prefix.add("*/" + path + "/*"); + // For write.object-storage.enabled=true / after Iceberg 1.7.0 + s3Prefix.add("*/*/*/*/" + path); + s3Prefix.add("*/*/*/*/" + path + "/*"); } // "Allow Write" for all remaining S3 actions on the bucket+path. @@ -94,8 +97,10 @@ static String locationDependentPolicy(S3ClientIam clientIam, StorageLocations lo String bucket = iamEscapeString(location.requiredAuthority()); String path = iamEscapeString(location.pathWithoutLeadingTrailingSlash()); resources.add(format("arn:aws:s3:::%s/%s/*", bucket, path)); - // For write.object-storage.enabled=true + // For write.object-storage.enabled=true / before Iceberg 1.7.0 resources.add(format("arn:aws:s3:::%s/*/%s/*", bucket, path)); + // For write.object-storage.enabled=true / since Iceberg 1.7.0 + resources.add(format("arn:aws:s3:::%s/*/*/*/*/%s/*", bucket, path)); } } @@ -112,8 +117,10 @@ static String locationDependentPolicy(S3ClientIam clientIam, StorageLocations lo String bucket = iamEscapeString(location.requiredAuthority()); String path = iamEscapeString(location.pathWithoutLeadingTrailingSlash()); resources.add(format("arn:aws:s3:::%s%s/*", bucket, path)); - // For write.object-storage.enabled=true + // For write.object-storage.enabled=true / before Iceberg 1.7.0 resources.add(format("arn:aws:s3:::%s/*/%s/*", bucket, path)); + // For write.object-storage.enabled=true / since Iceberg 1.7.0 + resources.add(format("arn:aws:s3:::%s/*/*/*/*/%s/*", bucket, path)); } } diff --git a/catalog/files/impl/src/test/java/org/projectnessie/catalog/files/s3/TestS3IamPolicies.java b/catalog/files/impl/src/test/java/org/projectnessie/catalog/files/s3/TestS3IamPolicies.java index 7fe8e4ff002..eace6a88b47 100644 --- a/catalog/files/impl/src/test/java/org/projectnessie/catalog/files/s3/TestS3IamPolicies.java +++ b/catalog/files/impl/src/test/java/org/projectnessie/catalog/files/s3/TestS3IamPolicies.java @@ -78,7 +78,7 @@ void multipleStorageLocations() throws Exception { + " \"Resource\" : \"arn:aws:s3:::bucket1\",\n" + " \"Condition\" : {\n" + " \"StringLike\" : {\n" - + " \"s3:prefix\" : [ \"my/path/bar\", \"my/path/bar/*\", \"*/my/path/bar\", \"*/my/path/bar/*\" ]\n" + + " \"s3:prefix\" : [ \"my/path/bar\", \"my/path/bar/*\", \"*/my/path/bar\", \"*/my/path/bar/*\", \"*/*/*/*/my/path/bar\", \"*/*/*/*/my/path/bar/*\" ]\n" + " }\n" + " }\n" + " }, {\n" @@ -87,7 +87,7 @@ void multipleStorageLocations() throws Exception { + " \"Resource\" : \"arn:aws:s3:::bucket2\",\n" + " \"Condition\" : {\n" + " \"StringLike\" : {\n" - + " \"s3:prefix\" : [ \"my/other/bar\", \"my/other/bar/*\", \"*/my/other/bar\", \"*/my/other/bar/*\" ]\n" + + " \"s3:prefix\" : [ \"my/other/bar\", \"my/other/bar/*\", \"*/my/other/bar\", \"*/my/other/bar/*\", \"*/*/*/*/my/other/bar\", \"*/*/*/*/my/other/bar/*\" ]\n" + " }\n" + " }\n" + " }, {\n" @@ -96,7 +96,7 @@ void multipleStorageLocations() throws Exception { + " \"Resource\" : \"arn:aws:s3:::bucket3\",\n" + " \"Condition\" : {\n" + " \"StringLike\" : {\n" - + " \"s3:prefix\" : [ \"read/path/bar\", \"read/path/bar/*\", \"*/read/path/bar\", \"*/read/path/bar/*\" ]\n" + + " \"s3:prefix\" : [ \"read/path/bar\", \"read/path/bar/*\", \"*/read/path/bar\", \"*/read/path/bar/*\", \"*/*/*/*/read/path/bar\", \"*/*/*/*/read/path/bar/*\" ]\n" + " }\n" + " }\n" + " }, {\n" @@ -105,17 +105,17 @@ void multipleStorageLocations() throws Exception { + " \"Resource\" : \"arn:aws:s3:::bucket4\",\n" + " \"Condition\" : {\n" + " \"StringLike\" : {\n" - + " \"s3:prefix\" : [ \"read/other/bar\", \"read/other/bar/*\", \"*/read/other/bar\", \"*/read/other/bar/*\" ]\n" + + " \"s3:prefix\" : [ \"read/other/bar\", \"read/other/bar/*\", \"*/read/other/bar\", \"*/read/other/bar/*\", \"*/*/*/*/read/other/bar\", \"*/*/*/*/read/other/bar/*\" ]\n" + " }\n" + " }\n" + " }, {\n" + " \"Effect\" : \"Allow\",\n" + " \"Action\" : [ \"s3:GetObject\", \"s3:GetObjectVersion\", \"s3:PutObject\", \"s3:DeleteObject\" ],\n" - + " \"Resource\" : [ \"arn:aws:s3:::bucket1/my/path/bar/*\", \"arn:aws:s3:::bucket1/*/my/path/bar/*\", \"arn:aws:s3:::bucket2/my/other/bar/*\", \"arn:aws:s3:::bucket2/*/my/other/bar/*\" ]\n" + + " \"Resource\" : [ \"arn:aws:s3:::bucket1/my/path/bar/*\", \"arn:aws:s3:::bucket1/*/my/path/bar/*\", \"arn:aws:s3:::bucket1/*/*/*/*/my/path/bar/*\", \"arn:aws:s3:::bucket2/my/other/bar/*\", \"arn:aws:s3:::bucket2/*/my/other/bar/*\", \"arn:aws:s3:::bucket2/*/*/*/*/my/other/bar/*\" ]\n" + " }, {\n" + " \"Effect\" : \"Allow\",\n" + " \"Action\" : [ \"s3:GetObject\", \"s3:GetObjectVersion\" ],\n" - + " \"Resource\" : [ \"arn:aws:s3:::bucket3read/path/bar/*\", \"arn:aws:s3:::bucket3/*/read/path/bar/*\", \"arn:aws:s3:::bucket4read/other/bar/*\", \"arn:aws:s3:::bucket4/*/read/other/bar/*\" ]\n" + + " \"Resource\" : [ \"arn:aws:s3:::bucket3read/path/bar/*\", \"arn:aws:s3:::bucket3/*/read/path/bar/*\", \"arn:aws:s3:::bucket3/*/*/*/*/read/path/bar/*\", \"arn:aws:s3:::bucket4read/other/bar/*\", \"arn:aws:s3:::bucket4/*/read/other/bar/*\", \"arn:aws:s3:::bucket4/*/*/*/*/read/other/bar/*\" ]\n" + " }, {\n" + " \"Effect\" : \"Deny\",\n" + " \"Action\" : \"s3:*\",\n" @@ -156,7 +156,11 @@ static Stream locationDependentPolicy() { return Stream.of( arguments( ImmutableS3ClientIam.builder().enabled(true).build(), - List.of("arn:aws:s3:::foo", "arn:aws:s3:::foo/b\"ar/*", "arn:aws:s3:::foo/*/b\"ar/*")), + List.of( + "arn:aws:s3:::foo", + "arn:aws:s3:::foo/b\"ar/*", + "arn:aws:s3:::foo/*/b\"ar/*", + "arn:aws:s3:::foo/*/*/*/*/b\"ar/*")), arguments( ImmutableS3ClientIam.builder() .enabled(true) @@ -168,6 +172,7 @@ static Stream locationDependentPolicy() { "arn:aws:s3:::foo", "arn:aws:s3:::foo/b\"ar/*", "arn:aws:s3:::foo/*/b\"ar/*", + "arn:aws:s3:::foo/*/*/*/*/b\"ar/*", "arn:aws:s3:::*/blocked\"Namespace/*"))); }