aws_request_signing: extend api to allow excluding headers from signing#18998
aws_request_signing: extend api to allow excluding headers from signing#18998mattklein123 merged 3 commits intoenvoyproxy:mainfrom
Conversation
|
Hi @rexnp, 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. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
At a high level looks reasonable. Can you check DCO/docs/etc. CI? /wait |
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>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small comments, thanks.
/wait
| const std::vector<std::string> default_excluded_headers_ = { | ||
| Http::Headers::get().ForwardedFor.get(), Http::Headers::get().ForwardedProto.get(), | ||
| "x-amzn-trace-id"}; | ||
| std::vector<Matchers::StringMatcherPtr> excluded_header_matchers_ = defaultMatchers(); |
There was a problem hiding this comment.
I see these defaults are pre-existing, but are they documented anywhere? Perhaps make it more clear in the API docs what the defaults are if not set?
There was a problem hiding this comment.
Thanks Matt! Yes, I'll list them out in the doc.
| service_name, region, std::move(credentials_provider), | ||
| context.mainThreadDispatcher().timeSource()); | ||
| context.mainThreadDispatcher().timeSource(), | ||
| Extensions::Common::Aws::AwsSigV4HeaderExclusionVector{}); |
There was a problem hiding this comment.
Should there be a TODO here for allow configuration of this? Wouldn't the same problem exist for this filter?
There was a problem hiding this comment.
Yes, I think we'd eventually want the same changes for the lambda and grpc iam filters. I'll add a TODO.
| const std::string yaml = R"EOF( | ||
| service_name: s3 | ||
| region: us-west-2 | ||
| match_excluded_headers: |
There was a problem hiding this comment.
Can you actually test this populates the config correctly?
Signed-off-by: Rex Chang <58710378+rexnp@users.noreply.github.com>
2 participants
Commit Message:
This change extends the AWS Request Signing filter with an optional list of header exclusions (implemented as a list of StringMatchers) named
match_excluded_headers. This allows configuring certain headers to not be signed and addresses the issue in #18695.I also took the opportunity to move the existing hard-coded rules for skipping certain header signings from the signing method itself to the initialization of a signer.
Additional Description: N/A
Risk Level: Low
Testing: Updated unit test cases
Docs Changes: Added description of the new field and updated the example filter configuration with the new field.
Release Notes: Added
Platform Specific Features: N/A
Fixes #18695