Support for canonicalizing URI properly for AWS SigV4 signer by Y0Username · Pull Request #17137 · envoyproxy/envoy

Y0Username · 2021-06-24T23:25:48Z

Support for canonicalizing URI properly for AWS SigV4 signer

Canonicalizing query string
Canonicalizing path string
Adding tests for canonical path and query

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

Signed-off-by: Yathish Gangolli yathishsg@gmail.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-06-24T23:25:50Z

Hi @Y0Username, 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: #17137 was opened by Y0Username.

see: more, trace.

yanavlasov · 2021-06-25T17:00:26Z

If I understand this correctly you now created possibility that Envoy and request signer will "observe" different URL path representations. I do not know this feature well enough to say if this important from security perspective or not. Specifically the signer might think that request goes to service A while Envoy ends up routing request to service B because of the differences in path.
In any case I think it would be quite important to clarify why Envoy's path normalization can not be used in this case. It could be a gotcha for users of this filter and at the very least should be documented.

yanavlasov · 2021-06-25T17:00:41Z

/wait-any

Y0Username · 2021-06-25T23:33:59Z

@yanavlasov thanks for looking at it. Yes the signer and Envoy can technically see different path representations due to differences in normalizing and encoding. However, this path representation being created here is being used create a hash.

This doc explains how to create and sign requests AWS that are not being made by CLI/SDK. The sigv4 algorithm includes a hash of the request payload as part of the request signature to protect against malicious spoofing of a request by using the credentials from a non-malicious request. So the client and server need to hash the request identically. For various reasons (both practical and historical) this is the algorithm that sigv4 uses. Hence the need for different representation of URL path, if we use Envoy's path representation, requests to AWS will fail with: The request signature we calculated does not match the signature you provided.

I can add documentation to clarify this behavior.

mattklein123

It would be great to not have different URL canonicalizing if we don't need it. Does this follow a standard? Can we use the common code that we already have? cc @htuch @yanavlasov @tonya11en for more details.

/wait-any

lavignes · 2021-06-28T18:00:09Z

source/extensions/common/aws/utility.cc

Use StringUtil::CaseInsensitiveCompare

Used absl::EqualsIgnoreCase

lavignes · 2021-06-28T18:02:16Z

source/extensions/common/aws/utility.cc

I would just make all of these all into static constexpr absl::string_view in some places you are having to allocate strings for the chars which is unnecessary.

lavignes · 2021-06-28T18:05:49Z

source/extensions/common/aws/utility.cc

couldn't you use absl::EndsWith here?

lavignes · 2021-06-28T18:14:15Z

source/extensions/common/aws/utility.cc

StringUtil::CaseInsensitiveCompare

Used absl::EqualsIgnoreCase

lavignes · 2021-06-28T18:17:19Z

source/extensions/common/aws/utility.cc

value.empty() ? EMPTY_STRING : value is redundant. Just pass value

lavignes · 2021-06-28T18:25:21Z

source/extensions/common/aws/utility.cc

You should use absl::StrSplit(query_fragment, absl::MaxSplits('=', 1)) instead.

htuch · 2021-06-29T16:16:50Z

Yeah, I would like to understand what is going on with this canonicalization logic and how it relates to other path normalization in Envoy. This seems another opportunity for GOLDPANDA-style fun.

tonya11en · 2021-06-29T16:51:28Z

There are a lot of deviations in these AWS utilities from the standard way Envoy does things, cURL/AsyncClient being another example. Adding another one is just making folks uncomfortable, especially since there have been CVEs related to path normalization in the past. This could turn out to be another vector for similar bugs.

If you could provide more info on why the common Envoy utilities can't be used, it would help.

Y0Username · 2021-06-29T18:29:09Z

To be clear, the canonicalized path here is not used for routing. It is just used for calculating the request hash. The hash calculated on the client should match the hash calculated on the server side, otherwise the request fails. So the client needs to canonicalize the path exactly like the server does. Unfortunately, Envoy canonicalizes the path little differently compared to AWS recommendation.

The AWS recommendation is also based on RFC 3986, but with little tweaks that are listed here: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html

mattklein123 · 2021-07-13T19:18:12Z

If the canonicalization is not the same I'm not sure there is much we can do about it unfortunately.

@tonya11en can you do the initial review here and then we can do a final pass?

/wait-any

yanavlasov · 2021-07-13T19:59:43Z

Thanks for the AWS spec reference. Looking into it more, I agree that there is nothing we can do with existing Envoy normalizations.
Also thinking more about it I think custom normalization is likely ok. The only problem would be if the signing server makes some decisions based on path, but it does not look to be the case.

You may also suggest that operators enable Envoy's normalize_path option as it will minimize the differences in path observed by the signing server an Envoy.

yanavlasov

/wait

source/extensions/common/aws/utility.cc

tonya11en · 2021-07-27T17:55:45Z

@Y0Username in the future when updating patches, can you please avoid force-pushing? It makes it so we cannot just see what changed since our last review.

tonya11en · 2021-07-27T21:52:25Z

@Y0Username you'll need to add a release note to docs/root/version_history/current.rst

tonya11en · 2021-07-27T21:52:41Z

/wait

- Canonicalizing query string - Canonicalizing path string - Adding tests for canonical path and query Ref: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html Issue: envoyproxy#16918 Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

tonya11en · 2021-07-28T19:38:14Z

@Y0Username not sure if you saw my earlier message, but can you please refrain from force-pushing commits to your PR branch? It makes it so we can't see the diffs between your various revisions.

Y0Username · 2021-07-28T22:35:03Z

@Y0Username not sure if you saw my earlier message, but can you please refrain from force-pushing commits to your PR branch? It makes it so we can't see the diffs between your various revisions.

I have stopped amending existing commits. I force push because I rebase on upstream/main. However, discussed offline to use merge commits.

tonya11en

I think it's ready for others to sweep through now.

mattklein123

LGTM with small release note comment, thanks.

/wait

mattklein123 · 2021-07-29T22:15:01Z

docs/root/version_history/current.rst


 * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value.
 * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false.
+* extensions: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>`_.


nit: instead of "extensions" can you make this "aws request signer" or something?

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

mattklein123

Thanks!

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

…oxy#17137) - Canonicalizing query string - Canonicalizing path string - Adding tests for canonical path and query Ref: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html Issue: envoyproxy#16918 Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Y0Username requested a review from mattklein123 as a code owner June 24, 2021 23:25

alyssawilk assigned lavignes and mattklein123 Jun 25, 2021

yanavlasov self-assigned this Jun 25, 2021

repokitteh-read-only bot added the waiting:any label Jun 25, 2021

repokitteh-read-only bot removed the waiting:any label Jun 25, 2021

mattklein123 reviewed Jun 28, 2021

View reviewed changes

repokitteh-read-only bot added the waiting:any label Jun 28, 2021

lavignes suggested changes Jun 28, 2021

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Jun 28, 2021

mattklein123 added the waiting label Jun 28, 2021

Y0Username force-pushed the fix-16918 branch from 2497c3a to beca9dc Compare July 12, 2021 10:49

repokitteh-read-only bot removed the waiting label Jul 12, 2021

mattklein123 assigned tonya11en Jul 13, 2021

repokitteh-read-only bot added the waiting:any label Jul 13, 2021

repokitteh-read-only bot removed the waiting:any label Jul 13, 2021

yanavlasov requested changes Jul 13, 2021

View reviewed changes

source/extensions/common/aws/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 13, 2021

Y0Username force-pushed the fix-16918 branch from beca9dc to 4103126 Compare July 14, 2021 05:43

repokitteh-read-only bot removed the waiting label Jul 14, 2021

mattklein123 added the waiting label Jul 14, 2021

mattklein123 added the waiting:any label Jul 27, 2021

Y0Username force-pushed the fix-16918 branch from 186bf4f to c046a19 Compare July 27, 2021 18:45

repokitteh-read-only bot removed the waiting:any label Jul 27, 2021

repokitteh-read-only bot added the waiting label Jul 27, 2021

Y0Username force-pushed the fix-16918 branch from c046a19 to ad8a29b Compare July 28, 2021 00:17

repokitteh-read-only bot removed the waiting label Jul 28, 2021

mattklein123 added the waiting label Jul 28, 2021

Y0Username added 4 commits July 28, 2021 19:26

Removing hash set for reserved chars

93de066

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Adding version history

eef5dc5

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Fix version history format

0ede2ac

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Y0Username force-pushed the fix-16918 branch from ad8a29b to 0ede2ac Compare July 28, 2021 19:26

repokitteh-read-only bot removed the waiting label Jul 28, 2021

tonya11en previously approved these changes Jul 29, 2021

View reviewed changes

mattklein123 requested changes Jul 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 29, 2021

Y0Username added 2 commits August 2, 2021 18:27

Updating version history

c716752

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Merge remote-tracking branch 'upstream/main' into fix-16918

f6131bb

Signed-off-by: Yathish Gangolli <yathishsg@gmail.com>

Y0Username dismissed tonya11en’s stale review via f6131bb August 2, 2021 18:41

repokitteh-read-only bot removed the waiting label Aug 2, 2021

mattklein123 approved these changes Aug 2, 2021

View reviewed changes

mattklein123 merged commit 003ff42 into envoyproxy:main Aug 2, 2021

santoshankr mentioned this pull request Aug 10, 2021

AWS SigV4 signer does not canonicalize URI and query string properly #16918

Closed

Conversation

Y0Username commented Jun 24, 2021

Uh oh!

repokitteh-read-only bot commented Jun 24, 2021

Uh oh!

yanavlasov commented Jun 25, 2021

Uh oh!

yanavlasov commented Jun 25, 2021

Uh oh!

Y0Username commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 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!

htuch commented Jun 29, 2021

Uh oh!

tonya11en commented Jun 29, 2021

Uh oh!

Y0Username commented Jun 29, 2021

Uh oh!

mattklein123 commented Jul 13, 2021

Uh oh!

yanavlasov commented Jul 13, 2021

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonya11en commented Jul 27, 2021

Uh oh!

tonya11en commented Jul 27, 2021

Uh oh!

tonya11en commented Jul 27, 2021

Uh oh!

tonya11en commented Jul 28, 2021

Uh oh!

Y0Username commented Jul 28, 2021

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Y0Username commented Jun 25, 2021 •

edited

Loading