Beginnings of S3 request proxying

[Randgalt]

See ReadMe for details: https://github.com/trinodb/s3-proxy/blob/jordanz/initial-proxied-request/README.md

E.g.

~/dev/starburst/trino-s3-proxy (jordanz/initial-proxied-request)$ aws --endpoint-url http://localhost:53893/api/v1/s3Proxy/s3 s3 ls s3://galaxy-cur/     
                           PRE demo/
                           PRE dev/
                           PRE prod/
                           PRE staging/
2024-05-16 11:41:31          4 aws-programmatic-access-test-object

Closes #8
Closes #10

[vagaerg]

I understand that this is to support us obtaining the metadata in the same way whether this is using the emulated access key or the external one. Is the idea that we may want to proxy requests signed with the real access key?

However, we currently never use this argument. Even if we were to use it, we would need to somehow pass it onto isValidAuthorization and we should have a way to retrieve a Credentials pair from a real key in the same way we do for emulated ones right?

[Randgalt]

Is the idea that we may want to proxy requests signed with the real access key?

We need to sign the real request being sent to AWS (or whatever real object store we're using). The signing mechanism is the same. However, for the real request we must use the real AccessKey/Secret.

However, we currently never use this argument

It's used by TrinoS3ProxyClient. See here: https://github.com/trinodb/s3-proxy/pull/11/files#diff-86567ef27fb9d1f3e89e0d28033e383903d06ae883810cb03d45ca40400bb665R104

[vagaerg]

It is used for the sign method but not for the signingMetadataFromRequest right? I completely agree with needing this for the sign method, just wondering its purpose for this one

[Randgalt]

I see. signingMetadataFromRequest always uses the emulated credentials. I find it easier to reason about when SigningController is agnostic about which credentials are used. I'd prefer to leave as is unless you feel strongly about it.

[vagaerg]

SigningController can be agnostic as to what credentials are used, I agree that's good. But this method is taking an argument that is unused, so it is misleading for a caller as it gives the impression you can ask it to use the real credentials, but it won't.

If we want to make it explicit that this method (unlike the others in SigningController) only deals with emulated credentials, should we rename to validateEmulatedCredentialsAndGetMetadata?

[Randgalt]

OK - it's fixed now. PTAL. Thanks for your persistence.

[Randgalt]

Also, now that I look at it fresh, we can change signRequest() to just take a region and a Credential and not have it take the SigningMetadata. wdyt? However, we may want that metadata object in the future. I'm 50/50

[vagaerg]

No worries! I originally had this design in mind, but at present there is no way to allow users to pass in Credentials::real to this method (as our credential store doesn't store a mapping from real credential to emulated ones, only the other way around)

However, that is personal preference - and in any case, we could tweak the CredentialsController later if we wanted to make this possible for any reason.

Looks good!

[vagaerg]

Whoops just saw your last comment. If we did that we'd need to also pass in the session, if any - right?

[Randgalt]

Yes ^^^ - we should leave it due to that.

[vagaerg]

LGTM!