aws_request_signing_filter hash payload by default by jstewmon · Pull Request #15846 · envoyproxy/envoy

jstewmon · 2021-04-06T03:44:18Z

👋 Hi, I thought I'd have a crack at this one. It's the first time I've written C++ in nearly 20y, so feedback is more than welcome - I mostly waded though similar sources to figure out how to get something working.

Commit Message:
aws_request_signing_filter hash payload by default

canonical must include the hashed payload for most services. The prior
behavior of using UNSIGNED-PAYLOAD is an exception to the rule, which
select services like s3 support, since hashing the payload may be
impractical if the payload is very large.

A new filter option is introduced, so that the filter may be explicitly
configured to use the UNSIGNED-PAYLOAD string literal as specified
in the S3 signing docs:
https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

fixes #13904

Additional Description:
The original implementation was seemingly very specific to S3 and was subsequently amended to extend the same niche singing behaviors for ES and Glacier. This changes the filter's default behavior to match the general SigV4 guidelines while providing a configuration option to enable the specialized UNSIGNED-PAYLOAD behavior.

https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

Risk Level: Medium?

Deployments using the filter will now buffer requests by default, which could result in 413 responses for requests with bodies exceeding the buffer limit. These users can mitigate buffering by enabling the unsigned_payload option.

Testing:

I tested locally with a filter config. I anticipate updating the automated tests based on feedback from maintainers.

Docs Changes:
I think the .proto docstrings are sufficient, though I suppose a changelog entry may be needed somewhere? Pointers appreciated.

Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-04-06T03:44:22Z

Hi @jstewmon, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15846 was opened by jstewmon.

see: more, trace.

repokitteh-read-only · 2021-04-06T03:44:26Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15846 was opened by jstewmon.

see: more, trace.

rojkov

Thank you for working on this! Could you please add unit tests illustrating the new behavior introduced by the added Filter::decodeData() method?

To fix the formatting CI check you need to run ./tools/code_format/check_format.py fix.

rojkov · 2021-04-06T07:22:51Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

Better set the value in the init list right after host_rewrite_:

- host_rewrite_(host_rewrite) {} + host_rewrite_(host_rewrite), unsigned_payload_{unsigned_payload} {}

rojkov · 2021-04-06T07:53:58Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.h

I can't see where this member is initialized. Perhaps you wanted to use config_->isPayloadUnsigned() here?

This was a really helpful comment for me! I wasn't paying close attention to what I was doing and completely failed to thread the config setting through to the filter implementation. I ended up grabbing the option from the config_ instance already available to the filter. Would appreciate another look to see if it looks ok now. 🙏

rgs1

Thanks for working on this. Could you add an entry to docs/root/version_history/current.rst and also some tests for the cases when hashing the body fails?

It'll be nice to update docs/root/configuration/http/http_filters/aws_request_signing_filter.rst with a note about this being supported.

jstewmon · 2021-04-07T03:07:20Z

test/extensions/filters/http/aws_request_signing/aws_request_signing_filter_test.cc

@rojkov I started working on the tests tonight only to realize that the new sign overload can't be disambiguated by the mocks.

I wanted to get feedback on this strategy before updating everywhere. I didn't see Matcher or the other recommended techniques used elsewhere in the envoy code.

Is this a good approach? If not, do you have a recommendation?

This seems to be fine. testing::Matcher is actually used in some Envoy tests.

Ah, thanks - I now see Matcher and others used throughout the tests. I must have had a stray search filter enabled when I was looking before.

lizan

/lgtm api

jstewmon · 2021-04-08T04:37:43Z

I think this PR is in pretty good shape now. Though, I've just realized that none of the tests for decodeData are checking the stats - I can try to add that tomorrow.

I've tested the filter with GET and POST requests to SNS (without unsigned_payload). I've also tested GET and PUT requests to S3 with and without unsigned_payload. I also verified that PUTs to an S3 having a bucket policy which denies unsigned payloads succeeds without unsigned payloads and fails with a 403 with unsigned_payloads.

rojkov · 2021-04-08T07:57:54Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

Do we stop iterating to avoid tampering the signature? Probably a comment could be helpful here.

Hm, I inferred this as necessary because it's what the buffer filter and the aws lambda filter do. I supposed it was necessary to preclude the headers from being sent upstream before the stream ends.

I don't mind adding a comment, but the best I can honestly write with my current understanding is "stop iteration is what other filters do when the request is to be buffered."

Did you have something more concrete in mind?

My experience with filter flows is limited, but I think you are right: StopIteration is needed to buffer the request.

I don't have strong opinion if a comment is needed. Let's leave it as it is.

rojkov · 2021-04-08T08:07:49Z

source/extensions/common/aws/signer.h

The new param needs to be documented in the doc string.

rojkov · 2021-04-08T08:24:00Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

Nit: better put explicit type here instead of auto. It helps readers to disambiguate which variant of the overloaded sign() function is called with less mental effort.

rojkov · 2021-04-08T08:31:12Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.h

Make it const.

rojkov · 2021-04-08T08:34:33Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

The method doesn't mutate the object's state -> can be declared as const.

Hm, I tried this, but it gives a compiler error:

error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers] const bool FilterConfigImpl::useUnsignedPayload() { return unsigned_payload_; }

Doh, I figured it out - was using const in the wrong place.

rojkov · 2021-04-08T08:51:49Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

ASSERT(request_headers_ != nullptr)

rojkov · 2021-04-08T08:57:35Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.h

Nit: usually we initialize to null values with empty braces:

Http::RequestHeaderMap* request_headers_{};

rojkov

Thanks! Looks good to me.

Please fix the CI checks. To run some checks (like DCO) automatically before pushing you can install the git hooks with

./support/bootstrap

rojkov · 2021-04-08T14:18:57Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

My experience with filter flows is limited, but I think you are right: StopIteration is needed to buffer the request.

I don't have strong opinion if a comment is needed. Let's leave it as it is.

rgs1

Thanks, left a few more comments.

rgs1 · 2021-04-08T15:24:44Z

docs/root/configuration/http/http_filters/aws_request_signing_filter.rst

Please link to the unsigned_payload field

rgs1 · 2021-04-08T15:25:33Z

docs/root/configuration/http/http_filters/aws_request_signing_filter.rst

Link to field and I don't think false needs to the inverted quotes.

rgs1 · 2021-04-08T15:27:25Z

docs/root/version_history/current.rst

Single inverted quotes

rgs1 · 2021-04-08T15:27:37Z

source/extensions/common/aws/signer.h

single inverted quotes

rgs1 · 2021-04-08T15:28:45Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

nit: const bool

rgs1 · 2021-04-08T15:30:17Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

nit:

ENVOY_LOG(debug, "aws request signing from decodeHeaders unsigned_payload: {}", unsigned_payload);

rgs1 · 2021-04-08T15:32:36Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

I think we are moving away from exceptions in the serving path, so maybe add a TODO about making sign() unexceptional

rgs1 · 2021-04-08T15:33:53Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

Hmm should we add a new stat signing_body_added or similar? To distinguish from the existing signing code path...

Having separate counters could add fidelity when unsigned_payload is false. I don't actually know under what circumstances end_stream is true in decodeHeaders, so I suppose having discrete counters could be useful in diagnosing any unexpected behavior (like if there's some way end_stream is true when there was a request body).

rgs1 · 2021-04-08T15:36:45Z

api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto

Should we call this use_unsigned_payload or skip_body_buffering?

I'll change it to use_unsigned_payload. Unsigned payload is a very specific signing algorithm variant that isn't widely supported. I would say that using unsigned payload implies buffering is not required, but there could be some future case that warrants disabling buffering without using the unsigned payload variant.

I added two new counters that are only incremented when signing occurs in decodeData.

jstewmon · 2021-04-09T19:41:01Z

I looked at the failed CI checks, and see two issues.

On windows:

//test/extensions/filters/http/aws_request_signing:aws_request_signing_filter_test FAILED TO BUILD

on linux_x64 tsan:

//test/integration:integration_test                                      FAILED in 2 out of 3 in 136.6s
  Stats over 3 runs: max = 136.6s, min = 132.1s, avg = 134.0s, dev = 1.9s
  /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-dbg/testlogs/test/integration/integration_test/shard_1_of_2/test.log
  /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-dbg/testlogs/test/integration/integration_test/shard_1_of_2/test_attempts/attempt_1.log

The windows build failure seems possibly related to this PR, but I don't see a way to get the detailed build output to figure out what the problem is.

I'm not sure how the integration test failure might be related to this PR. I don't see a way to get the test log output to see what the problem was.

I can't reproduce either of these locally (macOS).

dragonbone81 · 2021-04-13T22:30:06Z

Hi, don't want to pollute this PR but not sure where else to ask.
I'm trying to proxy dynamoDB requests through envoy, and in an http lua filter modify the body of the request. This requires a re-sign of the request, however when using the signing_filter aws still sends back an error.
Does the fix in this PR have anything to do with what I'm seeing or are these unrelated?

jstewmon · 2021-04-14T21:19:34Z

@dragonbone81 this PR should resolve your issue. The current aws request signing filter implementation does not sign request payloads, so it only works with requests which do not have a body or with a few services that support a special UNSIGNED PAYLOAD (e.g. s3).

@mattklein123 I've worked through all of the reviewer feedback thus far. There are basically two CI failures which I don't think I can resolve with the limited visibility I have. Could you please take a look and give any feedback you might have that can help me push this over the line? 🙏

rgs1 · 2021-04-15T00:40:22Z

@jstewmon mind merging main and pushing? I can take a look at the CI failures.

canonical must include the hashed payload for most services. The prior behavior of using UNSIGNED-PAYLOAD is an exception to the rule, which select services like s3 support, since hashing the payload may be impractical if the payload is very large. A new filter option is introduced, so that the filter may be explicitly configured to use the UNSIGNED-PAYLOAD string literal as specified in the S3 signing docs: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html fixes envoyproxy#13904 Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

tonya11en · 2021-04-22T08:34:01Z

Overall this is looking good. Just added some minor comments.

Thanks!

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon · 2021-05-03T20:35:35Z

@tonya11en thanks for the last round of feedback - I think I've addressed everything. Could you please take another look? 🙏

tonya11en

I'm digging the thorough tests! Thanks for doing this and for all of the rigor in the implementation.

The explanations around the various decisions make sense. LGTM.

tonya11en · 2021-05-04T18:51:05Z

source/extensions/filters/http/aws_request_signing/aws_request_signing_filter.cc

    headers.setHost(host_rewrite);
  }

+  if (!use_unsigned_payload && !end_stream) {


Makes sense. Thanks for the really thorough answer-- made it really clear.

lizan · 2021-05-07T07:05:40Z

Can you merge main and resolve conflicts?

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon · 2021-05-07T15:37:41Z

@lizan merged main and fixed up the version history 👍

jstewmon · 2021-05-11T19:26:14Z

@lizan is this ready to be merged? Anything else I need to do?

tonya11en · 2021-05-11T19:28:24Z

I think he was waiting on the tests to pass. Looks like one of them was cancelled, so it never happened.

/retest

repokitteh-read-only · 2021-05-11T19:28:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15846 (comment) was created by @tonya11en.

see: more, trace.

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon · 2021-05-12T17:09:45Z

@tonya11en @lizan the retest passed, but in the meantime, the version history required another merge. If you can please approve the CI checks to run again, hopefully we'll get a clean run and can merge this. 🤞 🙏

jstewmon requested a review from mattklein123 as a code owner April 6, 2021 03:44

repokitteh-read-only bot added the api label Apr 6, 2021

repokitteh-read-only bot assigned lizan Apr 6, 2021

rojkov reviewed Apr 6, 2021

View reviewed changes

rgs1 suggested changes Apr 6, 2021

View reviewed changes

jstewmon commented Apr 7, 2021

View reviewed changes

lizan reviewed Apr 7, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Apr 7, 2021

jstewmon force-pushed the aws-request-signing-payload-hash branch from 35a7e2f to 0f70ce0 Compare April 8, 2021 03:55

rojkov reviewed Apr 8, 2021

View reviewed changes

jstewmon requested a review from rojkov April 8, 2021 13:37

rojkov previously approved these changes Apr 8, 2021

View reviewed changes

jstewmon dismissed rojkov’s stale review via 7f03b7b April 8, 2021 14:36

jstewmon force-pushed the aws-request-signing-payload-hash branch from 26b6a01 to 7f03b7b Compare April 8, 2021 14:36

rgs1 suggested changes Apr 8, 2021

View reviewed changes

repokitteh-read-only bot added the api label Apr 8, 2021

jstewmon requested a review from rgs1 April 8, 2021 20:47

dragonbone81 mentioned this pull request Apr 14, 2021

How to use AWS Request signing filter (AwsRequestSigning) #15963

Closed

jstewmon added 5 commits April 14, 2021 19:48

document aws_request_signing buffering behavior change

267c7f3

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

get aws_request_signing unsigned_payload option from config

0685707

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

apply formatting

72e4b82

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

update aws mocks with new sign signature

0ab2d44

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon added 2 commits April 22, 2021 12:57

document new aws request signing filter stats

dc2303d

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

prefer discretely named methods in Signer interface

054f829

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon requested review from lizan, rojkov and tonya11en April 23, 2021 03:22

remove unused using decl Matcher in aws filter tests

1186e2d

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

lizan previously approved these changes Apr 29, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Apr 29, 2021

tonya11en previously approved these changes May 4, 2021

View reviewed changes

jstewmon added 2 commits May 7, 2021 10:32

Merge branch 'main' into aws-request-signing-payload-hash

c8c83e6

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

fix version history docs after merging main

7a964a5

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon dismissed stale reviews from tonya11en and lizan via 7a964a5 May 7, 2021 15:36

repokitteh-read-only bot added the api label May 7, 2021

lizan previously approved these changes May 7, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 7, 2021

Merge branch 'main' into aws-request-signing-payload-hash

12e1e0c

Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

jstewmon dismissed lizan’s stale review via 12e1e0c May 12, 2021 17:08

repokitteh-read-only bot added the api label May 12, 2021

lizan approved these changes May 12, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 12, 2021

lizan merged commit 80e1ca8 into envoyproxy:main May 12, 2021

Conversation

jstewmon commented Apr 6, 2021

Uh oh!

repokitteh-read-only bot commented Apr 6, 2021

Uh oh!

repokitteh-read-only bot commented Apr 6, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

jstewmon commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented May 11, 2021 •

edited

Loading