Add support for S3 request signing #2280

adutra · 2025-08-06T17:26:49Z

Fixes #32.

Design doc:

https://docs.google.com/document/d/1ygdia7u4bUHUt6n8XhZo48aKoIyyrCvKqan3XP25iB8/edit?usp=sharing

singhpk234 · 2025-08-06T23:21:18Z

Thank you so much for this @adutra !

when ready please let us know (would be really helpful if you can share some description to walk us through your thought process), happy to help with reviews :) !

snazy

This looks like good 1st step.

As all signing requests will have to access the backend for every little data file chunk being accessed by clients, this implementation will cause quite a slow experience for users. Until we don't have a faster implementation (signed access-rules), I think we should label this feature as "experimental".

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

snazy · 2025-08-07T08:33:58Z

...service/src/main/java/org/apache/polaris/service/storage/aws/signer/S3RequestSignerImpl.java

+ * S3 request signer that creates presigned URLs for S3 operations using AWS credentials obtained
+ * from the storage integration.
+ */
+public class S3RequestSignerImpl implements S3RequestSigner {


Suggested change

public class S3RequestSignerImpl implements S3RequestSigner {

class S3RequestSignerImpl implements S3RequestSigner {

snazy · 2025-08-07T08:34:08Z

...service/src/main/java/org/apache/polaris/service/storage/aws/signer/S3RequestSignerImpl.java

+  private final AwsCredentialsStorageIntegration storageIntegration;
+  private final StorageCredentialCache storageCredentialCache;
+
+  public S3RequestSignerImpl(


Suggested change

public S3RequestSignerImpl(

S3RequestSignerImpl(

snazy · 2025-08-07T08:47:34Z

...service/src/main/java/org/apache/polaris/service/storage/aws/signer/S3RequestSignerImpl.java

+            .method(method)
+            .headers(signingRequest.headers());
+
+    // FIXME is this correct?


nope.
See ListObjects + ListObjectsV2 REST spec.

Be careful: Not all GET /{bucket/ requests are list-objects requests!
The "v1" list-objects has no required parameter, so you cannot just check for the presence of a req param. It could also be a get-bucket-cors or get-bucket-encryption or ...-policy or ...-website or ......
Nice, no?

Maybe it's okay to assume that every client uses the v2 api, which has a unique, required request param.

...service/src/main/java/org/apache/polaris/service/storage/aws/signer/S3RequestSignerImpl.java

adutra · 2025-08-08T14:14:55Z

Ready for review. The test failures are unrelated to this change.

\cc @singhpk234 @snazy @dimas-b @metadaddy

adutra · 2025-08-08T14:24:01Z

api/iceberg-aws-sign-service/build.gradle.kts

+    mapOf(
+      "IcebergErrorResponse" to "org.apache.iceberg.rest.responses.ErrorResponse",
+      "S3SignRequest" to "org.apache.polaris.service.aws.sign.model.PolarisS3SignRequest",
+      "SignS3Request200Response" to


FYI The name of the logical response type is a bit weird because it is inlined, instead of being a $ref in the OpanAPI spec.

adutra · 2025-08-08T14:25:44Z

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java

@@ -146,11 +148,13 @@ public abstract class PolarisRestCatalogIntegrationBase extends CatalogTests<RES
  private RESTCatalog restCatalog;
  private String currentCatalogName;
  private Map<String, String> restCatalogConfig;
-  private URI externalCatalogBase;
+  private URI externalCatalogBaseLocation;


FYI I refactored this class a bit, notably to facilitate subclassing, but there is no functional change in this class.

Minor request here, but you could pull this and the new IntegerationTest Helpers out into a separate PR and reduce the size of this PR by a bit. I think that would also be a very fast review

Yup good idea!

#2384

.../src/main/java/org/apache/polaris/service/it/test/PolarisS3RemoteSigningIntegrationTest.java

adutra · 2025-08-08T14:29:04Z

runtime/server/build.gradle.kts

@@ -39,9 +39,6 @@ val distributionElements by
  }

 dependencies {
-  implementation(project(":polaris-core"))


Not directly referenced in this module.

This is another change I would try to pull out to another PR, it's not related to this changeset correct?

Correct:

#2385

adutra · 2025-08-08T14:31:06Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

@@ -297,12 +298,6 @@ public CreateNamespaceResponse createNamespace(CreateNamespaceRequest request) {
    }
  }

-  private static boolean isStaticFacade(CatalogEntity catalog) {


Refactored to become a method of CatalogEntity.

@RussellSpitzer FYI:

#2386
#2387

Thanks! It definitely helps to keep this sort of refactor out of the larger functional change

adutra · 2025-08-08T14:32:49Z

runtime/service/src/main/java/org/apache/polaris/service/storage/StorageConfiguration.java

+   * <p>The returned provider is not meant to be vended directly to clients, but rather used with
+   * STS, unless credential subscoping is disabled.
+   */
+  default AwsCredentialsProvider awsSystemCredentials() {


Renamed to awsSystemCredentials because these are not "just" credentials for STS, they are server credentials.

adutra · 2025-08-08T14:35:30Z

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogAdapter.java

+        prefix,
+        catalog -> {
+          S3SignResponse response = catalog.signS3Request(s3SignRequest, tableIdentifier);
+          return Response.status(Response.Status.OK).entity(response).build();


In theory the response should include Cache-Control headers. This is left for a later improvement.

... but that would require clients to respect those 🤷

adutra · 2025-08-11T16:02:26Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/io/DefaultFileIOFactory.java

@@ -101,6 +110,32 @@ public FileIO loadFileIO(
      properties.putAll(accessConfig.get().credentials());
      properties.putAll(accessConfig.get().extraProperties());
      properties.putAll(accessConfig.get().internalProperties());
+    } else {
+      // If no subscoped creds were produced, use system-wide AWS or GCP credentials if available.


This bit is not directly related to remote signing, but I realized that, when credentials subscoping is disabled, the server is unable to use the credentials configured in application.properties.

This change allows that.

That being said, the server would still be unable to use e.g. a custom S3 endpoint, simply because this property is not exposed in application.properties.

IMHO each property that can be configured in AccessConfig for a specific catalog should also be configurable by a system-wide property in application.properties.

metadaddy · 2025-08-11T17:14:24Z

Looks good. When I get an opportunity, I'll try setting this up with Backblaze B2 as S3-compatible storage and Trino as the Iceberg REST catalog client. Thanks!

snazy

Overall LGTM.
Not an "always boring" change" though - therefore a few more comments.

...gn-service/src/main/java/org/apache/polaris/service/aws/sign/model/PolarisS3SignRequest.java

.../src/main/java/org/apache/polaris/service/it/test/PolarisS3RemoteSigningIntegrationTest.java

polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java

polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java

polaris-core/src/main/java/org/apache/polaris/core/rest/PolarisEndpoints.java

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

snazy · 2025-08-13T10:04:35Z

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogAdapter.java

+        prefix,
+        catalog -> {
+          S3SignResponse response = catalog.signS3Request(s3SignRequest, tableIdentifier);
+          return Response.status(Response.Status.OK).entity(response).build();


... but that would require clients to respect those 🤷

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogHandler.java

snazy · 2025-08-13T10:07:28Z

...c/main/java/org/apache/polaris/service/context/catalog/PolarisCallContextCatalogFactory.java

+            polarisEventListener,
+            storageIntegrationProvider,
+            prefixParser,
+            uriInfo);


(Not related to this PR)
The consistently increasing amount of parameters passed to these types worries me.

Agreed. We could look into making it a CDI bean.

snazy

I think, it's very very close :)

snazy · 2025-08-14T14:35:48Z

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogHandler.java

+    // TODO authorize based on the request's method?
+
+    try {
+      authorizeBasicTableLikeOperationOrThrow(


hm - probably worth to mention in the changelog

singhpk234 · 2025-08-14T15:22:46Z

Hey @adutra thank you the change, i was reading the description and it turns out we are introducing new priviledges, IMHO this requires broader set of eyes and feedbacks as its just no longer implementation of the specification of IRC which can be contained in the PR.

Can you please send your Proposal in the dev-list (if you have already have one, can you please link this to the PR)
I only see this thread presently - https://lists.apache.org/thread/qvzwc3qxlfrk9vr7yfbx6zxfhz9lhlbc

singhpk234 · 2025-08-14T14:17:55Z

...gn-service/src/main/java/org/apache/polaris/service/aws/sign/model/PolarisS3SignRequest.java

+import org.immutables.value.Value;
+
+/**
+ * Request for S3 signing requests.


Suggested change

* Request for S3 signing requests.

* Request for S3 signing.

singhpk234 · 2025-08-14T20:24:02Z

...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java

+    if (storageConfig.getAwsPartition().equals("aws-us-gov") && region == null) {
+      throw new IllegalArgumentException(
+          String.format(
+              "AWS region must be set when using partition %s", storageConfig.getAwsPartition()));
+    }
+
+    AccessConfig.Builder accessConfig = AccessConfig.builder();
+    if (region != null) {
+      accessConfig.put(StorageAccessProperty.CLIENT_REGION, region);
+    }
+
+    URI endpointUri = storageConfig.getEndpointUri();
+    if (endpointUri != null) {
+      accessConfig.put(StorageAccessProperty.AWS_ENDPOINT, endpointUri.toString());
+    }
+    if (Boolean.TRUE.equals(storageConfig.getPathStyleAccess())) {
+      accessConfig.put(StorageAccessProperty.AWS_PATH_STYLE_ACCESS, Boolean.TRUE.toString());
+    }


can we put these in a helper, to be used here and the method above ?

singhpk234 · 2025-08-14T20:59:55Z

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogHandler.java

+    // no need to materialize the catalog here, as we only need the catalog entity
+  }
+
+  public PolarisS3SignResponse signS3Request(


can i have a sign priviledge without table read priviledge ? a client may directly contact the sign endpoint

Not anymore, I refactored this bit to comply with the design doc.

For a read request you need TABLE_REMOTE_SIGN + TABLE_READ_DATA.

For a write request you need TABLE_REMOTE_SIGN + TABLE_WRITE_DATA.

singhpk234 · 2025-08-14T21:07:15Z

...c/main/java/org/apache/polaris/service/storage/aws/signer/S3RemoteSigningCatalogHandler.java

+
+    LOGGER.debug("Requesting s3 signing for {}: {}", tableIdentifier, s3SignRequest);
+
+    // TODO authorize based on the request's method?


shouldn't we ? SIGN_S3_REQUEST shouldn't be a blanket approval for deletes ?

See my previous comment. For a DELETE method you now need TABLE_REMOTE_SIGN + TABLE_WRITE_DATA.

singhpk234 · 2025-08-14T21:13:19Z

...service/src/main/java/org/apache/polaris/service/storage/aws/signer/S3RequestSignerImpl.java

+            .method(method)
+            .headers(signingRequest.headers());
+
+    AwsCredentials credentials = storageConfiguration.awsSystemCredentials().resolveCredentials();


How are the credentials refreshed ?

These are the server credentials – they must not need refreshing.

adutra · 2025-08-18T15:54:55Z

FYI: I was requested to produce a design doc for this feature:

https://docs.google.com/document/d/1ygdia7u4bUHUt6n8XhZo48aKoIyyrCvKqan3XP25iB8/edit?usp=sharing

This change promotes CatalogConfig and RestCatalogConfig to top-level, public annotations and introduces a few "hooks" in PolarisRestCatalogIntegrationBase that can be overridden by subclasses. This change is a preparatory work for apache#2280 (S3 remote signing).

adutra · 2025-08-19T09:11:02Z

.../src/main/java/org/apache/polaris/service/storage/s3/sign/S3RemoteSigningCatalogHandler.java

+    // Must be done after the authorization check, as the auth check creates the catalog entity
+    throwIfRemoteSigningNotEnabled(callContext.getRealmConfig(), catalogEntity);
+
+    // TODO S3 location access checks


FYI, this is left for a follow-up PR.

adutra · 2025-08-19T09:14:17Z

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java

+    }
+
+    String prefix = prefixParser.catalogNameToPrefix(callContext.getRealmContext(), catalogName);
+    URI signerUri = uriInfo.getBaseUri().resolve("api/");


FYI, resolving the signer URI in case of HTTP proxies is left for a follow-up PR.

This change promotes `CatalogConfig` and `RestCatalogConfig` to top-level, public annotations and introduces a few "hooks" in `PolarisRestCatalogIntegrationBase` that can be overridden by subclasses. This change is a preparatory work for #2280 (S3 remote signing).

Fixes apache#32.

adutra · 2025-08-19T15:35:10Z

FYI I just rebased and synchronized this PR with the design doc.

This change promotes `CatalogConfig` and `RestCatalogConfig` to top-level, public annotations and introduces a few "hooks" in `PolarisRestCatalogIntegrationBase` that can be overridden by subclasses. This change is a preparatory work for apache#2280 (S3 remote signing).

github-project-automation bot added this to Basic Kanban Board Aug 6, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 6, 2025

adutra force-pushed the request-signing branch from 48aa8ef to 9640b7e Compare August 6, 2025 17:29

snazy reviewed Aug 7, 2025

View reviewed changes

adutra force-pushed the request-signing branch 14 times, most recently from 6bf40a9 to 8d1f354 Compare August 8, 2025 13:08

adutra marked this pull request as ready for review August 8, 2025 13:09

adutra force-pushed the request-signing branch 2 times, most recently from df10af7 to 6a26337 Compare August 8, 2025 14:14

adutra commented Aug 8, 2025

View reviewed changes

.../src/main/java/org/apache/polaris/service/it/test/PolarisS3RemoteSigningIntegrationTest.java Show resolved Hide resolved

adutra commented Aug 8, 2025

View reviewed changes

adutra commented Aug 11, 2025

View reviewed changes

adutra force-pushed the request-signing branch from 125f079 to e382e83 Compare August 11, 2025 16:07

snazy reviewed Aug 13, 2025

View reviewed changes

adutra mentioned this pull request Aug 14, 2025

Register table endpoint should honor access delegation #2351

Open

adutra force-pushed the request-signing branch from e382e83 to 6ee282e Compare August 14, 2025 13:34

dimas-b mentioned this pull request Aug 14, 2025

[ENHANCEMENT] Support non-AWS S3 storage that does not have STS #2207

Open

snazy reviewed Aug 14, 2025

View reviewed changes

snazy previously approved these changes Aug 14, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Aug 14, 2025

singhpk234 reviewed Aug 14, 2025

View reviewed changes

adutra dismissed snazy’s stale review via 86ce418 August 18, 2025 10:15

adutra force-pushed the request-signing branch from 0c7f905 to 86ce418 Compare August 18, 2025 10:15

adutra force-pushed the request-signing branch 2 times, most recently from 55c0ce9 to 1bc4526 Compare August 19, 2025 08:40

adutra mentioned this pull request Aug 19, 2025

Minor refactor of integration test classes #2384

Merged

adutra commented Aug 19, 2025

View reviewed changes

adutra force-pushed the request-signing branch 4 times, most recently from 22a07f6 to 7fefc85 Compare August 19, 2025 09:43

Add support for S3 request signing

ad0f0d0

Fixes apache#32.

adutra force-pushed the request-signing branch from 7fefc85 to ad0f0d0 Compare August 19, 2025 14:28

	public class S3RequestSignerImpl implements S3RequestSigner {
	class S3RequestSignerImpl implements S3RequestSigner {


		LOGGER.debug("Requesting s3 signing for {}: {}", tableIdentifier, s3SignRequest);

		// TODO authorize based on the request's method?

Add support for S3 request signing #2280

Are you sure you want to change the base?

Add support for S3 request signing #2280

Conversation

adutra commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singhpk234 commented Aug 6, 2025

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adutra commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metadaddy commented Aug 11, 2025

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 commented Aug 14, 2025

adutra commented Aug 6, 2025 •

edited

Loading

adutra Aug 8, 2025 •

edited

Loading