Refactor CredentialsController, add RemoteConnection plugin #190

mosiac1 · 2025-04-01T17:19:46Z

This PR moves to a more dynamic approach to resolving remote credentials, where the whole Request object can be used to determine the endpoint it should be proxied to, the remote credentials to use and STS mechanism.

RemoteS3ConnectionProvider returns remote credentials, optional STS role and optional RemoteS3Facade configs (used to provide and endpoint but also change the path-style if needed). It takes as arguments the ParsedRequest, Identity and SigningMetadata.

The request flow with this change goes:

Client request to aws-proxy
-> CredentialsProvider to get emulated secret key and Identity
-> Signature verification (nothing changed)
-> SecurityFacade (nothing changed)
-> S3RequestRewriter (nothing changed)
-> RemoteS3ConnectionProvider to get remote credentials, role and remote configs
-> (Optional) Assume Remote Role
-> Sing request to remote (nothing changed)
-> Send request to remote (nothing changed)

This PR moves to a more dynamic approach to resolving remote credentials, where the whole Request object can be used to determine the endpoint it should be proxied to, the remote credentials to use and STS mechanism. RemoteS3ConnectionProvider returns remote credentials, optional STS role and optional RemoteS3Facade configs (used to provide and endpoint but also change the path-style if needed). It takes as arguments the ParsedRequest, Identity and SigningMetadata. The request flow with this change goes: Client request to aws-proxy -> CredentialsProvider to get emulated secret key and Identity -> SecurityFacade -> S3RequestRewriter -> RemoteS3ConnectionProvider to get remote credentials, role and remote configs -> (Optional) Assume Remote Role -> Sing request to remote -> Send request to remote

Randgalt

This is a large change and I want to spend some time on it. Blocking for that reason.

Randgalt · 2025-04-08T06:26:52Z

@Laonel please review this to see how it affects our codebase

Randgalt · 2025-04-08T06:30:33Z

trino-aws-proxy-glue/src/main/java/io/trino/aws/proxy/glue/rest/TrinoGlueResource.java

@@ -59,7 +59,7 @@ public TrinoGlueResource(ObjectMapper objectMapper, GlueRequestHandler requestHa
    @Produces(MediaType.APPLICATION_JSON)
    public Response gluePost(@Context Request request, @Context SigningMetadata signingMetadata, @Context RequestLoggingSession requestLoggingSession)
    {
-        requestLoggingSession.logProperty("request.glue.emulated.key", signingMetadata.credentials().emulated().secretKey());
+        requestLoggingSession.logProperty("request.glue.emulated.key", signingMetadata.credential().secretKey());


Please put this in a separate commit.

Do you mean changes to the Glue pieces? I can't really split it, interfaces and records changed so classes wouldn't compile.

In this line specifically, the change is due to the fact that SigningMetadata doesn't have a Credentials field anymore (since Credentials was deleted) but only a simple Credential which in the emulated credential. So no behaviour change.

Laonel

Overall, it looks like the right direction, and it's also consistent with what we are doing on our side, which I think is an additional validation :) But I think there are some changes required around RemoteS3Facade plugin to avoid regressions.

Laonel · 2025-04-10T06:04:50Z

trino-aws-proxy-spi/src/main/java/io/trino/aws/proxy/spi/credentials/IdentityCredential.java

+        this(emulatedCredential, Optional.empty());
+    }
+
+    public static IdentityCredential of(Credential emulatedCredential)


nit: I would decide on a single way for public clients to create instances. At the moment, there is a public constructor and a factory method with the same signature.

Laonel · 2025-04-10T06:05:24Z

trino-aws-proxy-spi/src/main/java/io/trino/aws/proxy/spi/credentials/EmulatedAssumedRole.java

-        requireNonNull(emulatedCredential, "emulatedCredential is null");
+        requireNonNull(emulatedCredential, "credential is null");


Laonel · 2025-04-10T06:07:09Z

trino-aws-proxy-spi/src/main/java/io/trino/aws/proxy/spi/remote/RemoteS3Connection.java

+    {
+        requireNonNull(remoteCredential, "remoteCredential is null");
+        requireNonNull(remoteSessionRole, "remoteSessionRole is null");
+        remoteS3FacadeConfiguration = requireNonNull(remoteS3FacadeConfiguration, "remoteS3FacadeConfiguration is null").map(ImmutableMap::copyOf);


nit: redundant rnn

...o-aws-proxy/src/main/java/io/trino/aws/proxy/server/remote/RemoteS3ConnectionController.java

Laonel · 2025-04-10T06:39:15Z

...o-aws-proxy/src/main/java/io/trino/aws/proxy/server/remote/RemoteS3ConnectionController.java

+                    RemoteS3Facade contextRemoteS3Facade = remoteConnection.remoteS3FacadeConfiguration().map(config -> {
+                        // TODO: This should respect the plugin installed for the RemoteS3Facade somehow
+                        Injector subInjector = new Bootstrap(new DefaultRemoteS3Module()).doNotInitializeLogging().quiet().setRequiredConfigurationProperties(config).initialize();
+                        return subInjector.getInstance(RemoteS3Facade.class);
+                    }).orElse(defaultS3Facade);


How this will work without implementing the TODO? If there is config provided, we will initialize a new RemoteS3Facade provided by the DefaultRemoteS3Module. Otherwise, we are using defaultS3Facade instance, which is provided by the installed RemoteS3Facade plugin.

I see two possible solutions:

change remote S3 plugin to provide RemoteS3FacadeProvider that can consume config and return RemoteS3Facade instance

change the plugin to provide a RemoteS3Module supplier that we would use to create an injector here. Seems a bit hacky, but that would allow to utilize Airlift for injecting remoteS3FacadeConfiguration into that instance.

Right now this would work, but only with the default path-style and virtual-host stye RemoteS3Facade implementations.

I was looking for some inspiration on how to do include plugins, so thanks for the suggestions!

1 makes sense, it might just be the cleanest, but I am not sure how we would resolve what configs to pass to the RemoteS3FacadeFactory (calling it a factory here instead of a Provider to match Trino naming which I'm more used to). We can:

Have the configs for this in a separate file like remote-s3-facade.properties (like Trino does). This would make this plugin stand out from the other that don't have this approach, but I don't think that would be an issue

We grab all the configs from the main config.properties that start with remote-s3. and feed them to the factory. This feels more hacky tbh.

for 2, I had an idea in a similar direction, but I am not sure that would work. A Plugin instance returns a Module that may be a combination of multiple modules, we don't an easy was to find RemoteS3Module instances among those.

So now I am thinking if they following would work:

A plugin binds RemoteS3FacadeFactory, and can bind additional config it needs to inject to the factory. That config would then be defined in config.properties.

On each call, RemoteS3FacadeFactory is creating a new instance of RemoteS3Facade using the config defined in the implementation.

Am I missing something? I see you have remote S3 config in RemoteS3Connection and I am not sure where that config would be coming from.

That being said, I think having separate files for remote S3 facade plugins would be good too, if that is required or makes things easier. I like the similarity with Trino as well.

I think i may not have fully gotten what you meant in the first comment.

My goal with this change is to make it so the RemoteS3Connection (which you get from the RemoteS3ConnectionProvider which might be calling an external http service) has control over the endpoint that the request ends up being proxied to. The way we control the endpoint in aws-proxy is though the RemoteS3Facade abstraction - so we need a way to create a RemoteS3Facade instance from a RemoteS3Connection object.

RemoteS3Connection has a set of configs (like the ones in config.properties) which are used through Airlift to inject a RemoteS3Facade instance. That Facade is used once to resolve the URI for this request and then is discarded (we could cache these, might be a decent idea).

On each call, RemoteS3FacadeFactory is creating a new instance of RemoteS3Facade using the config defined in the implementation.

I my mind, this would be "using the config from the RemoteS3ConnectionProvider".

I will have a go at a draft implementation

Laonel · 2025-04-10T06:46:06Z

trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/SecurityFilter.java

@@ -68,10 +70,9 @@ public void filter(ContainerRequestContext requestContext)
            RequestLoggingSession requestLoggingSession = requestLoggerController.newRequestSession(request, signingServiceType);
            containerRequest.setProperty(RequestLoggingSession.class.getName(), requestLoggingSession);

-            SigningMetadata signingMetadata;
+            SigningController.SigningIdentity signingIdentity;


nit: static import?

Laonel · 2025-04-10T06:46:40Z

trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/TrinoS3ProxyClient.java

        this.s3SecurityController = requireNonNull(s3SecurityController, "securityController is null");
        this.s3PresignController = requireNonNull(s3PresignController, "presignController is null");
        this.limitStreamController = requireNonNull(limitStreamController, "quotaStreamController is null");
        this.s3RequestRewriter = requireNonNull(s3RequestRewriter, "s3RequestRewriter is null");
+        this.remoteS3ConnectionController = requireNonNull(credentialsProvider, "credentialsController is null");


fix rnn message and / or param name

Laonel · 2025-04-10T06:49:01Z

trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/signing/InternalSigningController.java

    {
-        this.credentialsController = requireNonNull(credentialsController, "credentialsController is null");
+        this.credentialsProvider = requireNonNull(credentialsProvider, "signingCredentialProvider is null");


Suggested change

this.credentialsProvider = requireNonNull(credentialsProvider, "signingCredentialProvider is null");

this.credentialsProvider = requireNonNull(credentialsProvider, "credentialsProvider is null");

Laonel · 2025-04-10T06:51:26Z

trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/containers/S3Container.java

    {
        this.initialBuckets = requireNonNull(initialBuckets, "initialBuckets is null");
-        this.credential = requireNonNull(credentials, "credentials is null").requiredRemoteCredential();
+        this.credential = requireNonNull(remoteCredentials, "credentials is null");


Suggested change

this.credential = requireNonNull(remoteCredentials, "credentials is null");

this.credential = requireNonNull(remoteCredentials, "remoteCredentials is null");

cla-bot bot added the cla-signed label Apr 1, 2025

mosiac1 mentioned this pull request Apr 1, 2025

Add RemoteS3ConnectionProvider plugin implementations #191

Open

mosiac1 force-pushed the feat/refactor-remote branch from 00c4af6 to abd2f9d Compare April 2, 2025 14:40

Randgalt requested changes Apr 8, 2025

View reviewed changes

Randgalt requested a review from Laonel April 8, 2025 06:26

Randgalt reviewed Apr 8, 2025

View reviewed changes

Laonel reviewed Apr 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CredentialsController, add RemoteConnection plugin #190

Refactor CredentialsController, add RemoteConnection plugin #190

mosiac1 commented Apr 1, 2025 •

edited

Loading

Randgalt left a comment

Randgalt commented Apr 8, 2025

Randgalt Apr 8, 2025

mosiac1 Apr 8, 2025

Laonel left a comment

Laonel Apr 10, 2025

Laonel Apr 10, 2025

Laonel Apr 10, 2025

Laonel Apr 10, 2025

mosiac1 Apr 10, 2025

Laonel Apr 10, 2025

mosiac1 Apr 10, 2025

Laonel Apr 10, 2025

Laonel Apr 10, 2025

Laonel Apr 10, 2025

Laonel Apr 10, 2025

		requireNonNull(emulatedCredential, "emulatedCredential is null");
		requireNonNull(emulatedCredential, "credential is null");

	this.credentialsProvider = requireNonNull(credentialsProvider, "signingCredentialProvider is null");
	this.credentialsProvider = requireNonNull(credentialsProvider, "credentialsProvider is null");

	this.credential = requireNonNull(remoteCredentials, "credentials is null");
	this.credential = requireNonNull(remoteCredentials, "remoteCredentials is null");

Refactor CredentialsController, add RemoteConnection plugin #190

Are you sure you want to change the base?

Refactor CredentialsController, add RemoteConnection plugin #190

Conversation

mosiac1 commented Apr 1, 2025 • edited Loading

Randgalt left a comment

Choose a reason for hiding this comment

Randgalt commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laonel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosiac1 commented Apr 1, 2025 •

edited

Loading