Skip to content

Remove keyval / file based storage, replaced with ngx.shared. Additional x-amz-* headers were not signed #29

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bgardner-noggin
Copy link

Proposed changes

x-amz-* headers were not signed

Additional headers, eg x-amz-expected-bucket-owner for an S3 HEAD bucket request, were not being signed. All x-amz-* headers must be included in the signature

Removed keyval / file store for credentials, replaced with ngx.shared dictionary

The commercial keyval or falling back to a temporary file to store the credentials has been removed in favour of the ngx.shared model. This requires a dictionary aws to be configured as documented in the README.md changes

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md and CHANGELOG.md)

@dekobon dekobon requested review from shawnhankim and 4141done May 17, 2024 20:45
@shawnhankim
Copy link
Contributor

Hi @bgardner-noggin ,

Thank you for contributing your value on this repo. I have just read an email which is sent by @dekobon and am not sure why I can't find your pull request in my email.

We planned to reuse this library into the nginx-s3-gateway after making NPM package using this repo. In the meantime, it would be good to change both repos. I was wondering if you could also update https://github.com/nginxinc/nginx-s3-gateway and test.

Please let me know once you contribute https://github.com/nginxinc/nginx-s3-gateway the repo.
cc: @dekobon, @4141done

Thanks!

@bgardner-noggin
Copy link
Author

I'm not using the nginx-s3-gateway sorry.

My use case is that I have nginx running as a proxy to limit customers whitelists for signed S3 urls.

I am only using this library to send a signed HEAD request on a bucket, to verify that it is owned by my account, before allowing the signed url to proceed.

@shawnhankim
Copy link
Contributor

shawnhankim commented May 21, 2024

Thanks for sharing your use case. I am sure that your contribution is valuable.

As this repo is to provide a compatible solution for anyone who uses nginx-s3-gatewy, nginx-lambda-gateway, or the other nginx based Amazon Web Services such as your case, any PR needs to be verified with at least nginx-s3-gatewy or nginx-lambda-gateway until we do have better automation. So we can ensure that we do not block the existing customers who use not only NGINX OSS but also NGINX Plus.

Let's see who can verify first among @shawnhankim , @4141done, @dekobon, @bgardner-noggin if you can't verify as you focus on your use case. 😃

@4141done: Feel free to review if you find anything that needs to be enhanced in the nginx-s3-gateway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants