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.

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

implement auth middleware for s3 #3102

Merged
merged 1 commit into from
Dec 5, 2022
Merged

implement auth middleware for s3 #3102

merged 1 commit into from
Dec 5, 2022

Conversation

chanwit
Copy link
Member

@chanwit chanwit commented Dec 2, 2022

Signed-off-by: Chanwit Kaewkasi [email protected]

@chanwit chanwit self-assigned this Dec 2, 2022
@chanwit chanwit added the type/enhancement New feature or request label Dec 2, 2022
pkg/http/auth_middleware.go Outdated Show resolved Hide resolved
cmd/gitops-bucket-server/main.go Outdated Show resolved Hide resolved
pkg/http/auth_middleware.go Outdated Show resolved Hide resolved
pkg/http/auth_middleware.go Outdated Show resolved Hide resolved
pkg/http/auth_middleware.go Outdated Show resolved Hide resolved
@pjbgf
Copy link

pjbgf commented Dec 2, 2022

After the review I noticed that upstream exposes some of this functionality:
https://github.com/aws/aws-sdk-go-v2/blob/main/aws/signer/v4/v4.go

We may benefit from using their implementation for the signing and verification part. They also have a middleware implementation, but I am not sure how useful that would be for us.

@chanwit
Copy link
Member Author

chanwit commented Dec 2, 2022

After the review I noticed that upstream exposes some of this functionality: https://github.com/aws/aws-sdk-go-v2/blob/main/aws/signer/v4/v4.go

We may benefit from using their implementation for the signing and verification part. They also have a middleware implementation, but I am not sure how useful that would be for us.

Here's some background.
The codes I'm using was taken from the v4 signing of the MinIO lib (Apache license, also used by Flux).
Instead of signing, I reverse its process to implement the verification method.

@pjbgf
Copy link

pjbgf commented Dec 2, 2022

Here's some background.
The codes I'm using was taken from the v4 signing of the MinIO lib (Apache license, also used by Flux).
Instead of signing, I reverse its process to implement the verification method.

@chanwit thank you for the context. Unfortunately neither MiniIO nor AWS seems to expose the entire logic, so it does make sense the approach.

Copy link

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

LGTM

thanks @chanwit! 🙇

@chanwit chanwit force-pushed the s3-auth branch 2 times, most recently from 4c59194 to 9a529d9 Compare December 3, 2022 12:00
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Tested it locally with:

AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=some_secret_key ./bin/gitops-bucket-server --cert-file ~/dev/local-ca/ca.pem --key-file ~/dev/local-ca/ca.key

and the minio CLI:

$ mc alias set test https://localhost:9443 foo some_secret_key --insecure
Added `test` successfully.
$ $ /tmp/mc ls test --insecure
[2022-12-05 10:48:01 CET]     0B probe-bucket-sign-gza44hmacqgz/
[2022-12-05 10:48:29 CET]     0B minio/
[2022-12-05 10:55:29 CET]     0B probe-bucket-sign-wt0spypubjre/

Works fine, good job!

Just a remark on picking a different package name for the middleware code.

pkg/http/auth_middleware.go Outdated Show resolved Hide resolved
pkg/s3/server.go Outdated Show resolved Hide resolved
pkg/s3/server_test.go Outdated Show resolved Hide resolved
@chanwit chanwit force-pushed the s3-auth branch 3 times, most recently from 105617c to 23f8957 Compare December 5, 2022 13:31
Signed-off-by: Chanwit Kaewkasi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants