aws: support for Unsigned Payload or provided content sha256 in AWS signing (#6581) #6611

prasanthj · 2024-03-05T18:56:54Z

To support uses cases where OPA is used for signing s3 requests whose payload is not known upfront or payload is big enough (big file upload) to be sent over wire, this PR adds support for unsigned payloads.

AWS signer has configurable option to use unsigned payload where the x-amz-content-sha256 is set to "UNSIGNED-PAYLOAD" and is included as part of signing process. This PR provides an option for unsigned payload if aws_config.disable_payload_signing is set to true. If payload signing is disabled, SignV4 method will not compute the content sha from the request body but instead use "UNSIGNED-PAYLOAD" string literal for x-amz-content-sha256 header during signature computation.

References:
https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html

Solves #6581

netlify · 2024-03-05T18:58:27Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`37c2da8`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/65eb5ce5c0cc7d0008c8e6de
😎 Deploy Preview	https://deploy-preview-6611--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ashutosh-narkar

Thanks for working on this @prasanthj. From the approach described by @philipaconrad here, I was thinking the user would simply set the value of the x-amz-content-sha256 header in the headers field of the providers.aws.sign_req builtin and we would overwrite the computed value if that header exists. If we do that then we don't need a new AWS config parameter (disable_payload_signing). Am I missing something?

topdown/providers.go

ashutosh-narkar · 2024-03-06T00:48:22Z

docs/content/policy-reference.md

@@ -997,6 +998,7 @@ The `aws_config` object parameter may contain the following fields:
 | `aws_service` | yes | `string` | AWS service the request will be valid for. (e.g. `"s3"`) |
 | `aws_region` | yes | `string` | AWS region for the request. (e.g. `"us-east-1"`) |
 | `aws_session_token` | no | `string` | AWS security token. Used for the `x-amz-security-token` request header. |
+| `disable_payload_signing` | no | `boolean` | When `true` an `UNSIGNED-PAYLOAD` value will be used for calculating the `x-amz-content-sha256` header during signing, and will be returned in the response. Default: `false`. |


For consistency should we prefix this option with aws like the others?

since it is already under aws_config, not sure if aws_ prefix is required. This config is also more specific to s3 and not generic like other configs.

This config is also more specific to s3 and not generic like other configs.

If that's the case, we should at least update the description to clarify that.

prasanthj · 2024-03-06T03:26:27Z

Sorry I should have put that up in description. I was also suggesting that approach initially to use whatever is the provided header but it turns out to be a change in behavior. The change in behavior is, prior to this PR even if content sha is provided via headers it will not be used during signing as content sha is always computed from body. If provided headers contain content sha and if we start using it, it would cause change in behavior. There could be some users out there who would be using it and it would suddenly cause change in signature after this change. To avoid changing the behavior, enabling this feature explicitly via config option. This also aligns with how AWS SDK is doing.

ashutosh-narkar · 2024-03-06T20:26:10Z

docs/content/policy-reference.md

@@ -986,17 +987,18 @@ The following fields will have effects on the output `Authorization` header sign
 | `method` | yes | `string` | HTTP method to specify in request. Used in the signature. |
 | `body` | no | `any` | HTTP message body. The JSON serialized version of this value will be used for the payload portion of the signature if present. |
 | `raw_body` | no | `string` | HTTP message body. This will be used for the payload portion of the signature if present. |
-| `headers` | no | `object` | HTTP headers to include in the request. These will be added to the list of headers to sign. |
+| `headers` | no | `object` | HTTP headers to include in the request. These will be added to the list of headers to sign. If `aws_config.disable_payload_signing` is set to `true` then `UNSIGNED-PAYLOAD` string will be used as value for `x-amz-content-sha256` header during the signing process (any provided value for `x-amz-content-sha256` header will be discarded when payload signing is disabled).|


(any provided value for x-amz-content-sha256 header will be discarded when payload signing is disabled).

This does not seem to be the case looking at the code. If the request's headers contains x-amz-content-sha256, it will override the computed or static (ie. UNSIGNED-PAYLOAD) value and the provided value will be used for signing.

I'm looking at this line.

Good catch. This comment is stale and was carried over from the initial implementation which did not use explicit config. I will remove this comment.

Cool. Please also update the docs to reflect this behavior. Also an example in the builtins page would be helpful. Thanks.

ashutosh-narkar · 2024-03-07T22:20:43Z

docs/content/policy-reference.md

+The [AWS S3 request signing API](https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html)
+supports unsigned payload signing option. This example below shows s3 request signing with payload signing disabled. 
+
+```live:providers/aws/sign_req_basic:module


You'll need to rename this to get the website to build. sign_req_basic:module -> sign_req_unsign:module for instance.

ashutosh-narkar

@prasanthj the changes lgtm. If you can address the comment about the docs and squash your commits, we can get this in. Thanks!

…igning (open-policy-agent#6581) To support uses cases where OPA is used for signing s3 requests whose payload is not known upfront or payload is big enough (big file upload) to be sent over wire, this PR adds support for unsigned payloads. AWS signer has configurable option to use unsigned payload where the x-amz-content-sha256 is set to "UNSIGNED-PAYLOAD" and is included as part of signing process. This PR provides an option for unsigned payload if aws_config.disable_payload_signing is set to true. If payload signing is disabled, SignV4 method will not compute the content sha from the request body but instead use "UNSIGNED-PAYLOAD" string literal for x-amz-content-sha256 header during signature computation. References: https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-auth-using-authorization-header.html Signed-off-by: Prasanth Jayachandran <p_jayachandran@apple.com>

ashutosh-narkar

LGTM. Thanks @prasanthj!

prasanthj force-pushed the pr-6581 branch 3 times, most recently from de595d2 to 177d177 Compare March 5, 2024 19:27

ashutosh-narkar reviewed Mar 6, 2024

View reviewed changes

ashutosh-narkar requested a review from philipaconrad March 6, 2024 00:54

prasanthj force-pushed the pr-6581 branch 4 times, most recently from 192d41d to e84992f Compare March 6, 2024 10:33

ashutosh-narkar force-pushed the pr-6581 branch from e84992f to 968e5bf Compare March 6, 2024 18:13

ashutosh-narkar reviewed Mar 6, 2024

View reviewed changes

ashutosh-narkar force-pushed the pr-6581 branch from af7cb1f to d6c0753 Compare March 7, 2024 22:14

ashutosh-narkar reviewed Mar 7, 2024

View reviewed changes

prasanthj force-pushed the pr-6581 branch from d6c0753 to f26d744 Compare March 8, 2024 07:16

ashutosh-narkar force-pushed the pr-6581 branch from f26d744 to 37c2da8 Compare March 8, 2024 18:45

ashutosh-narkar approved these changes Mar 8, 2024

View reviewed changes

ashutosh-narkar merged commit 7f5e3a9 into open-policy-agent:main Mar 8, 2024
26 checks passed

BrewTestBot mentioned this pull request Mar 28, 2024

opa 0.63.0 Homebrew/homebrew-core#167431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aws: support for Unsigned Payload or provided content sha256 in AWS signing (#6581) #6611

aws: support for Unsigned Payload or provided content sha256 in AWS signing (#6581) #6611

prasanthj commented Mar 5, 2024

netlify bot commented Mar 5, 2024 •

edited

ashutosh-narkar left a comment

ashutosh-narkar Mar 6, 2024

prasanthj Mar 6, 2024

ashutosh-narkar Mar 6, 2024

prasanthj commented Mar 6, 2024 •

edited

ashutosh-narkar Mar 6, 2024

ashutosh-narkar Mar 6, 2024

prasanthj Mar 6, 2024

ashutosh-narkar Mar 6, 2024

ashutosh-narkar Mar 7, 2024

prasanthj Mar 8, 2024

ashutosh-narkar left a comment

ashutosh-narkar left a comment

aws: support for Unsigned Payload or provided content sha256 in AWS signing (#6581) #6611

aws: support for Unsigned Payload or provided content sha256 in AWS signing (#6581) #6611

Conversation

prasanthj commented Mar 5, 2024

netlify bot commented Mar 5, 2024 • edited

✅ Deploy Preview for openpolicyagent ready!

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasanthj commented Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 5, 2024 •

edited

prasanthj commented Mar 6, 2024 •

edited