Skip to content
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

Jump to bottom

Implement support for request signing #6

Merged
merged 1 commit into from
May 21, 2024
Merged

Implement support for request signing #6

merged 1 commit into from
May 21, 2024

Conversation

Randgalt
Copy link
Member

@Randgalt Randgalt commented May 16, 2024 •
edited
Loading

Closes #5

@cla-bot cla-bot bot added the cla-signed label May 16, 2024
@Randgalt Randgalt requested review from dain and vagaerg May 16, 2024 03:14
@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch 8 times, most recently from 35b43f2 to 80cf3c2 Compare May 16, 2024 16:17
dain
dain reviewed May 16, 2024
View reviewed changes
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Generally, lets reformat this code to the Trino style, and update it to Java 22 style. Also, let's rename all of the minio stuff to something else. I think in most cases it can be s3 or aws. I also think we can drop the interfaces unless we need to have multiple implementations

trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/credentials/Credentials.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/Digest.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/Digest.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/Digest.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/Digest.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/S3Escaper.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/S3Escaper.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/Signer.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/emulation/MinioRequestImpl.java Outdated Show resolved Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/minio/emulation/MinioUrlImpl.java Outdated Show resolved Hide resolved
@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch 2 times, most recently from 3d80dcf to 4245318 Compare May 17, 2024 06:15
@Randgalt Randgalt requested a review from dain May 17, 2024 06:16
@Randgalt
Copy link
Member Author

let's rename all of the minio stuff to something else

@dain do you mean rename the Minio classes I copied or do you mean the two shim classes I created? I'm not sure what you're referring to.

@dain
Copy link
Member

dain commented May 17, 2024

let's rename all of the minio stuff to something else

@dain do you mean rename the Minio classes I copied or do you mean the two shim classes I created? I'm not sure what you're referring to.

I'd rename all of them. This isn't a minio proxy but s3 stuff. Or said another way, I don't think we need or want to follow the minio code, we just want a one time copy and we should clean it up completely and make it match the Trino style.

@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch from 4245318 to 44b8194 Compare May 17, 2024 07:18
@Randgalt
Copy link
Member Author

I'd rename all of them. This isn't a minio proxy but s3 stuff. Or said another way, I don't think we need or want to follow the minio code, we just want a one time copy and we should clean it up completely and make it match the Trino style.

OK - I'll do a pass over that. My only concern is potential bug fixes but it's probably not a big problem.

@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch from 44b8194 to d535abb Compare May 17, 2024 08:17
@Randgalt
Copy link
Member Author

@dain I did a major refactor of the Minio classes. Only 1 is left and it's a simple util. PTAL

@Randgalt Randgalt requested a review from mosiac1 May 17, 2024 08:32
@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch 3 times, most recently from 94fa65c to 0287a5f Compare May 19, 2024 12:33
@Randgalt Randgalt changed the title Import Minio signing classes Implement support for request signing May 19, 2024
@Randgalt
Copy link
Member Author

Hey folks - it turns out the signing code can be done with AWS SDK methods. I removed the Minio code.

@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch 3 times, most recently from ec39d06 to 5787e06 Compare May 20, 2024 08:44
@Randgalt
Implement support for request signing
fa2de77 
- Use AWS SDK signing code
- Added an initial test for a simple `aws s3 ls` based on this doc: https://min.io/docs/minio/linux/integrations/aws-cli-with-minio.html

Closes #5
@Randgalt Randgalt force-pushed the jordanz/initial-minio-signer-import branch from 5787e06 to fa2de77 Compare May 20, 2024 14:36
dain
dain approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

This is much simpler!

trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/credentials/Signer.java
Clock clock = Clock.fixed(zonedDateTime.toInstant(), zonedDateTime.getZone());
signerParamsBuilder.signingClockOverride(clock);

// TODO - only allow a configured time window for the request
Copy link
Member

Choose a reason for hiding this comment

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

Let's do this sooner rather than later

Copy link
Member Author

Choose a reason for hiding this comment

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

#14

@Randgalt Randgalt mentioned this pull request May 21, 2024
Closed
@Randgalt Randgalt merged commit 6749fab into main May 21, 2024
2 checks passed
@Randgalt Randgalt deleted the jordanz/initial-minio-signer-import branch May 21, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dain dain dain approved these changes

@vagaerg vagaerg Awaiting requested review from vagaerg

@mosiac1 mosiac1 Awaiting requested review from mosiac1

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Controller to sign/re-sign requests
2 participants
@Randgalt @dain