aws-signing: add es and glacier for payloads special treatment by azihsoyn · Pull Request #12020 · envoyproxy/envoy

azihsoyn · 2020-07-10T05:00:37Z

Commit Message: aws-signing: add elasticsearch and glacier for payloads special treatment
Additional Description:

I found aws_request_signing for elasticsearch service failed in POST.
ESS too needs x-amz-content-sha256 like s3, so I added service_name es ( and gracier too ).

https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-request-signing.html
https://docs.aws.amazon.com/amazonglacier/latest/dev/amazon-glacier-signing-requests.html

Risk Level: Low
Testing: yes
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: azihsoyn azihsoyn@gmail.com

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio

Thanks! A request for pasting the link to the doc inline. Plus, could you add test for those cases, here: test/extensions/common/aws/signer_impl_test.cc? Probably very similar with SignHeadersS3.

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio · 2020-07-10T10:09:23Z

 }

+// Verify signing headers for es
+TEST_F(SignerImplTest, SignHeadersES) {


Do you think having a helper here will be helpful? For example: SignerImplTest::expectSignHeaders(absl::string_view service_name, absl::string_view signature) with the service name and the expected signature as params.

That looks good!

added expectSignHeaders. 😄

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio

Thanks! A couple of super nits!

dio · 2020-07-10T14:30:25Z

+  // https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-authenticating-requests.html
+  // ES:
+  // https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-request-signing.html
+  // Gracier:


Nit. Glacier?

😱
fixed it.
thx.

dio · 2020-07-10T14:30:38Z

-  if (service_name_ == "s3") {
+  // S3, Gracier, ES payloads require special treatment.
+  // S3:
+  // https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-authenticating-requests.html


Please ends with ".".

dio · 2020-07-10T14:31:01Z

-            headers.get(Http::CustomHeaders::get().Authorization)->value().getStringView());
-  EXPECT_EQ(SignatureConstants::get().HashedEmptyString,
-            headers.get(SignatureHeaders::get().ContentSha256)->value().getStringView());
+// Verify signing headers for services


Add a period at the end of the sentence. Thanks!

dio · 2020-07-10T14:31:38Z

 GETting
 GLB
 GOAWAY
+Gracier


Not sure if we need to add "Glacier".

Ooops...
Sorry, I fix soon.

dio · 2020-07-10T14:46:43Z

/assign @marcomagdy
/assign @rgs1

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio

One more thing.

dio · 2020-07-13T03:42:33Z

+  // https://docs.aws.amazon.com/elasticsearch-service/latest/developerguide/es-request-signing.html.
+  // Glacier:
+  // https://docs.aws.amazon.com/amazonglacier/latest/dev/amazon-glacier-signing-requests.html.
+  if (service_name_ == "s3" || service_name_ == "glacier" || service_name_ == "es") {


Sorry, I think we can have this decision at initialization. Let's have a const bool member variable (not sure about the name, e.g. require_special_treatment_). We can initialize that bool with require_special_treatment_(service_name_ == "s3" || service_name_ == "glacier" || service_name_ == "es") and use that here.

azihsoyn · 2020-07-13T04:50:31Z

Is this as you intended?
@dio

dio · 2020-07-13T05:38:27Z

Add newline here, then I think we're good.

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio · 2020-07-13T06:19:55Z

GitHub is experiencing problems, I think mostly on serving archives. So let's wait until it is resolved then I'll restart the build. https://www.githubstatus.com/incidents/j597fw8kv04c

dio · 2020-07-13T14:53:37Z

/azp run envoy-presubmit

azure-pipelines · 2020-07-13T14:53:47Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-07-13T16:01:16Z

/retest

repokitteh-read-only · 2020-07-13T16:01:20Z

🔨 rebuilding ci/circleci: api (failed build)
🔨 rebuilding ci/circleci: go_control_plane_mirror (failed build)

🐱

Caused by: a #12020 (comment) was created by @dio.

see: more, trace.

dio

Thanks!

marcomagdy · 2020-07-13T18:30:10Z

+  const bool require_content_hash_{service_name_ == "s3" || service_name_ == "glacier" ||
+                                   service_name_ == "es"};


nit: if the initialization has logic, then it's better to put it in the ctor.

marcomagdy · 2020-07-13T18:33:24Z


+  void expectSignHeaders(absl::string_view service_name, absl::string_view signature,
+                         absl::string_view payload) {
+    auto* credentials_provider = new NiceMock<MockCredentialsProvider>();


nit: why use new and then create a shared_ptr later?
This should should be:
CredentialsProviderSharedPtr credentials_provider = std::make_shared<NiceMock<MockCredentialsProvider>>();

maybe no member named 'gmock_getCredentials' in 'Envoy::Extensions::Common::Aws::CredentialsProvider'

marcomagdy · 2020-07-13T18:35:46Z

 GETting
 GLB
 GOAWAY
+Glacier


'Glacier' shouldn't need to be in this file. You had to add it probably because it was misspelled earlier?

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio

Thanks!

azihsoyn · 2020-07-15T23:44:50Z

Thank you for the review!

…proxy#12020) Signed-off-by: azihsoyn <azihsoyn@gmail.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…proxy#12020) Signed-off-by: azihsoyn <azihsoyn@gmail.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

azihsoyn requested a review from mattklein123 as a code owner July 10, 2020 05:00

aws-signing: add es and gracier for payloads special treatment

befb45c

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio suggested changes Jul 10, 2020

View reviewed changes

Comment thread source/extensions/common/aws/signer_impl.cc Outdated

add test and docs link

2ecb848

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

azihsoyn requested a review from dio July 10, 2020 05:54

azihsoyn added 4 commits July 10, 2020 14:55

fix test

6926b4a

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

fix comment format

dce46eb

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

add Gracier to spelling dictionary

92a519d

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

fix test

45f4cf4

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio reviewed Jul 10, 2020

View reviewed changes

refactor test

89348f5

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio changed the title ~~aws-signing: add es and gracier for payloads special treatment~~ aws-signing: add es and glacier for payloads special treatment Jul 10, 2020

dio reviewed Jul 10, 2020

View reviewed changes

repokitteh-read-only Bot assigned marcomagdy and rgs1 Jul 10, 2020

fix typo

280d36e

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

azihsoyn requested a review from dio July 13, 2020 02:39

dio reviewed Jul 13, 2020

View reviewed changes

azihsoyn commented Jul 13, 2020

View reviewed changes

Comment thread source/extensions/common/aws/signer_impl.h Outdated

azihsoyn Jul 13, 2020

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this as you intended?
@dio

dio reviewed Jul 13, 2020

View reviewed changes

azihsoyn added 2 commits July 13, 2020 14:44

check require_content_hash at initialize

d25421f

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

add new line

e428afa

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

dio previously approved these changes Jul 13, 2020

View reviewed changes

marcomagdy reviewed Jul 13, 2020

View reviewed changes

mattklein123 added the waiting label Jul 14, 2020

fix nits

375e25c

Signed-off-by: azihsoyn <azihsoyn@gmail.com>

azihsoyn dismissed dio’s stale review via 375e25c July 14, 2020 00:54

repokitteh-read-only Bot removed the waiting label Jul 14, 2020

azihsoyn requested a review from marcomagdy July 15, 2020 05:53

marcomagdy approved these changes Jul 15, 2020

View reviewed changes

dio approved these changes Jul 15, 2020

View reviewed changes

mattklein123 merged commit 0e6cff3 into envoyproxy:master Jul 15, 2020

azihsoyn deleted the aws-request-signing-for-ess branch July 15, 2020 23:42

KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020

aws-signing: add es and glacier for payloads special treatment (envoy…

8fbfaff

…proxy#12020) Signed-off-by: azihsoyn <azihsoyn@gmail.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020

aws-signing: add es and glacier for payloads special treatment (envoy…

29fdac7

…proxy#12020) Signed-off-by: azihsoyn <azihsoyn@gmail.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

lavignes mentioned this pull request Nov 6, 2020

AWS Request Signing Documentation enhancement #13904

Closed

jstewmon mentioned this pull request Apr 22, 2021

aws_request_signing_filter hash payload by default #15846

Merged

		const bool require_content_hash_{service_name_ == "s3" \|\| service_name_ == "glacier" \|\|
		service_name_ == "es"};

Conversation

azihsoyn commented Jul 10, 2020 • edited by dio Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dio left a comment

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!

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

dio commented Jul 10, 2020

Uh oh!

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

dio commented Jul 13, 2020

Uh oh!

dio commented Jul 13, 2020

Uh oh!

azure-pipelines Bot commented Jul 13, 2020

Uh oh!

dio commented Jul 13, 2020

Uh oh!

repokitteh-read-only Bot commented Jul 13, 2020

Uh oh!

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

dio left a comment

Choose a reason for hiding this comment

Uh oh!

azihsoyn commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

azihsoyn commented Jul 10, 2020 •

edited by dio

Loading