aws_request_signing: extend api to allow header exclusion by rexnp · Pull Request #1 · rexnp/envoy

rexnp · 2021-11-09T23:14:46Z

Commit Message: extends aws request signing filter with header exclusion list.
Additional Description:
Risk Level: Low
Testing: unit tests
Docs Changes: Pending
Release Notes: Pending
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]
Fixes envoyproxy#18695

Signed-off-by: Rex Chang <chiyc@amazon.com>

rexnp · 2021-11-09T23:21:40Z

working on doc changes now

abaptiste · 2021-11-10T16:17:21Z

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

  // <https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html>`_ policy for details.
  bool use_unsigned_payload = 4;
+
+  // A given header that matches any of the configured matchers will not be signed.


Let's rephrase this. I propose:

A list of request headers that will be excluded from signing.

You might expand a bit on the matcher implementation (haven't gotten that far yet) on whether these are prefix matchers etc.

expanded and added an example.

abaptiste

Overall looks good. Had a few comments.

abaptiste · 2021-11-10T16:17:51Z

ci/run_envoy_docker.sh

       -e SYSTEM_STAGEDISPLAYNAME \
       -e SYSTEM_JOBDISPLAYNAME \
       -e SYSTEM_PULLREQUEST_PULLREQUESTNUMBER \
+       -e GOPROXY=direct \


Let's omit this. It may not be applicable to Envoy, and is a direct artifact of building in our environment

yeah that slipped in.

abaptiste · 2021-11-10T16:18:58Z

source/extensions/common/aws/BUILD

        "//source/common/buffer:buffer_lib",
        "//source/common/common:hex_lib",
        "//source/common/common:logger_lib",
+        "//source/common/common:matchers_lib",


Wouldn't the signer library get the matchers_lib from including :utility_lib? Can you double check this and see if things still build and work without this line.

nice catch! that can be removed indeed.

source/extensions/common/aws/utility.cc

abaptiste · 2021-11-10T16:29:15Z

source/extensions/common/aws/signer_impl.h

  SignerImpl(absl::string_view service_name, absl::string_view region,
-             const CredentialsProviderSharedPtr& credentials_provider, TimeSource& time_source)
+             const CredentialsProviderSharedPtr& credentials_provider, TimeSource& time_source,
+             const std::vector<envoy::type::matcher::v3::StringMatcher>& matcher_config)


I suggest changing this so that you consume the vector of matcher pointers from the config when the signer is instantiated.

If this doesn't make sense because of lambda, and gRPC, then "this is fine" :)

I started with this but it seemed simpler to only handle the conversion between the proto and matcher pointer once in the Signer class, vs repeating them in the request signing, lamda, and grpc iam filters.

abaptiste · 2021-11-10T16:34:02Z

source/extensions/common/aws/utility.cc

+        // proxies
+        const auto key = entry.key().getStringView();
+        if (key == Http::Headers::get().ForwardedFor.get() ||
+            key == Http::Headers::get().ForwardedProto.get() || key == "x-amzn-trace-id" ||


Is there any way we can add x-amzn-trace-id as a default entry in the excluded headers?
Or even pre-populate the excluded headers with the XFF headers and amazon trace header?

I don't have strong feelings either way. I think it'd make this block a bit cleaner.

opted to provide a default exclusion list in the signer constructor.

abaptiste · 2021-11-10T16:37:48Z

source/extensions/grpc_credentials/aws_iam/config.cc

        auto signer = std::make_unique<Common::Aws::SignerImpl>(
-            config.service_name(), getRegion(config), credentials_provider, api.timeSource());
+            config.service_name(), getRegion(config), credentials_provider, api.timeSource(),
+            std::vector<envoy::type::matcher::v3::StringMatcher>{});


I suggest having a type defined for std::vector<envoy::type::matcher::v3::StringMatcher> perhaps:

using AwsSigV4HeaderExclusionVector = std::vector<envoy::type::matcher::v3::StringMatcher>;

That way you have a angle bracket less type (AwsSigV4HeaderExclusionVector) you can use throughout the implementation.

You might also have a hardcoded static entry for an empty vector? This last bit isn't a hard requirement.

neat! done.

abaptiste · 2021-11-10T16:43:55Z

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

+  prefix: [x-envoy]
+  exact: [foo]


As I mentioned earlier in the proto file, let's add a bit of details as to how a user can specify a header and what sort of matcher we support.

Signed-off-by: Rex Chang <chiyc@amazon.com>

abaptiste · 2021-11-12T18:45:26Z

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

+    match_excluded_headers:
+        - prefix: x-envoy
+        - prefix: x-forwarded
+        - exact: x-amzn-trace-id


abaptiste · 2021-11-12T18:47:38Z

source/extensions/common/aws/signer_impl.h

+    std::vector<Matchers::StringMatcherPtr> default_excluded_headers{};
+    for (const auto& header : default_excluded_headers_) {


The default_excluded_headers objects are really closely named. Don't really have a suggestion here other than something that disambiguates these a bit more.

renamed to matcher_ptrs to better reflect the return type

abaptiste

One minor change suggested. Looks good!

Signed-off-by: Rex Chang <chiyc@amazon.com>

abaptiste

Ship it!

Signed-off-by: Rex Chang <chiyc@amazon.com> * extends aws request signing filter with header exclusion list Signed-off-by: Rex Chang <chiyc@amazon.com> aws_request_signing: extend api to allow header exclusion (#1) * extends aws request signing filter with header exclusion list Signed-off-by: Rex Chang <chiyc@amazon.com> Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>

extends aws request signing filter with header exclusion list

05836f2

Signed-off-by: Rex Chang <chiyc@amazon.com>

abaptiste reviewed Nov 10, 2021

View reviewed changes

abaptiste requested changes Nov 10, 2021

View reviewed changes

rexnp added 2 commits November 11, 2021 19:49

rev2

03c83ed

Signed-off-by: Rex Chang <chiyc@amazon.com>

use the typedef

908ead4

Signed-off-by: Rex Chang <chiyc@amazon.com>

rexnp requested a review from abaptiste November 11, 2021 22:21

adds doc

40d1297

Signed-off-by: Rex Chang <chiyc@amazon.com>

abaptiste reviewed Nov 12, 2021

View reviewed changes

abaptiste requested changes Nov 12, 2021

View reviewed changes

minor rev

533df6d

Signed-off-by: Rex Chang <chiyc@amazon.com>

rexnp requested a review from abaptiste November 12, 2021 20:04

abaptiste approved these changes Nov 12, 2021

View reviewed changes

rexnp merged this pull request into main Nov 12, 2021

		std::vector<Matchers::StringMatcherPtr> default_excluded_headers{};
		for (const auto& header : default_excluded_headers_) {

Conversation

rexnp commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rexnp commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

rexnp Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

abaptiste left a comment

Choose a reason for hiding this comment

Uh oh!

abaptiste left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rexnp commented Nov 9, 2021 •

edited

Loading

rexnp Nov 11, 2021 •

edited

Loading