-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
plugins/rest: SigV4 Signing for any AWS service #3210
plugins/rest: SigV4 Signing for any AWS service #3210
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good! And super impressive how you learnt enough Go to submit this in such a short time frame 🚀 👍
I left a couple of comments but they were all nits more or less. Only thing that annoys me now is how we've named the credential plugin s3_signing
, but I guess we can consider renaming that if other AWS services becomes a popular target for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Nitpicks, no blockers, inside 👇 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🎉
Uhmm I guess the changes are due to #3204 overlapping a bit with this PR. Should be trivial, though. |
@srenatus @anderseknert wondered if you could have a quick look at the rebased version? :) Also wondered what the timeline might be for a change to make it into a release? :) Cheers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Squash the commits and this should be good to merge. We don't have a fixed release schedule but next release, including this improvement, should be out in a month or two.
This adds a new `service` option to the `s3_signing` config, allowing for other AWS services (such as API Gateway endpoints) to be used for bundles, decision logs etc. For example: ``` services: decision-log-service: url: https://myrestapi.execute-api.ap-southeast-2.amazonaws.com/prod/ credentials: s3_signing: service: execute-api environment_credentials: {} decision_logs: service: decision-log-service reporting: min_delay_seconds: 300 max_delay_seconds: 600 ``` If no service is specified, we default to `s3` to maintain backwards compatibility. This updates the sigv4 signer to include the specified service in the signature, and to sign all request headers for better compatibility with other AWS services, except an explicit ignore list, as per https://github.com/aws/aws-sdk-go/blob/master/aws/signer/v4/v4.go#L92 Additionally, this fixes a bug in the signer where the body ReadCloser was consumed and not reset, meaning requests that were signed were always sent with an empty body! Fixes open-policy-agent#3193 Signed-off-by: Jack Stevenson <jacsteve@amazon.com>
f7ce5d6
to
0e9c270
Compare
Thank you! |
Notes
This adds a new
service
option to thes3_signing
config, allowing for other AWS services (suchas API Gateway endpoints) to be used for bundles, decision logs etc.
For example:
If no service is specified, we default to
s3
to maintain backwards compatibility.This updates the sigv4 signer to include the specified service in the signature, and to sign all
request headers for better compatibility with other AWS services, except an explicit ignore list,
as per https://github.com/aws/aws-sdk-go/blob/master/aws/signer/v4/v4.go#L92
Additionally, this fixes a bug in the signer where the body ReadCloser was consumed and not reset,
meaning requests that were signed were always sent with an empty body!
Fixes #3193
Testing
Signed-off-by: Jack Stevenson jacsteve@amazon.com