Skip to content

aws-signing: add es and glacier for payloads special treatment #12020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 11 commits into from
Jul 15, 2020
Merged

aws-signing: add es and glacier for payloads special treatment #12020

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
befb45c
aws-signing: add es and gracier for payloads special treatment
azihsoyn Jul 10, 2020
2ecb848
add test and docs link
azihsoyn Jul 10, 2020
6926b4a
fix test
azihsoyn Jul 10, 2020
dce46eb
fix comment format
azihsoyn Jul 10, 2020
92a519d
add Gracier to spelling dictionary
azihsoyn Jul 10, 2020
45f4cf4
fix test
azihsoyn Jul 10, 2020
89348f5
refactor test
azihsoyn Jul 10, 2020
280d36e
fix typo
azihsoyn Jul 10, 2020
d25421f
check require_content_hash at initialize
azihsoyn Jul 13, 2020
e428afa
add new line
azihsoyn Jul 13, 2020
375e25c
fix nits
azihsoyn Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions source/extensions/common/aws/signer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@ void SignerImpl::sign(Http::RequestMessage& message, bool sign_body) {
}

void SignerImpl::sign(Http::RequestHeaderMap& headers) {
// S3 payloads require special treatment.
if (service_name_ == "s3") {
// S3, Glacier, ES payloads require special treatment.
// S3:
// 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.
// Glacier:
// https://docs.aws.amazon.com/amazonglacier/latest/dev/amazon-glacier-signing-requests.html.
if (service_name_ == "s3" || service_name_ == "glacier" || service_name_ == "es") {

@dio dio Jul 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@rgs1 rgs1 Jul 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@azihsoyn 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.

ok

headers.setReference(SignatureHeaders::get().ContentSha256,
SignatureConstants::get().UnsignedPayload);
sign(headers, SignatureConstants::get().UnsignedPayload);
Expand Down
71 changes: 31 additions & 40 deletions test/extensions/common/aws/signer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,27 @@ class SignerImplTest : public testing::Test {
message_->body() = std::make_unique<Buffer::OwnedImpl>(body);
}

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

@marcomagdy marcomagdy Jul 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@azihsoyn azihsoyn Jul 14, 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.

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

EXPECT_CALL(*credentials_provider, getCredentials()).WillOnce(Return(credentials_));
Http::TestRequestHeaderMapImpl headers{};
headers.setMethod("GET");
headers.setPath("/");
headers.addCopy(Http::LowerCaseString("host"), "www.example.com");

SignerImpl signer(service_name, "region", CredentialsProviderSharedPtr{credentials_provider},
time_system_);
signer.sign(headers);

EXPECT_EQ(fmt::format("AWS4-HMAC-SHA256 Credential=akid/20180102/region/{}/aws4_request, "
"SignedHeaders=host;x-amz-content-sha256;x-amz-date, "
"Signature={}",
service_name, signature),
headers.get(Http::CustomHeaders::get().Authorization)->value().getStringView());
EXPECT_EQ(payload, headers.get(SignatureHeaders::get().ContentSha256)->value().getStringView());
}

NiceMock<MockCredentialsProvider>* credentials_provider_;
Event::SimulatedTimeSystem time_system_;
Http::RequestMessagePtr message_;
Expand Down Expand Up @@ -169,46 +190,16 @@ TEST_F(SignerImplTest, SignHostHeader) {
message_->headers().get(Http::CustomHeaders::get().Authorization)->value().getStringView());
}

// Verify signing headers for S3
TEST_F(SignerImplTest, SignHeadersS3) {
auto* credentials_provider = new NiceMock<MockCredentialsProvider>();
EXPECT_CALL(*credentials_provider, getCredentials()).WillOnce(Return(credentials_));
Http::TestRequestHeaderMapImpl headers{};
headers.setMethod("GET");
headers.setPath("/");
headers.addCopy(Http::LowerCaseString("host"), "www.example.com");

SignerImpl signer("s3", "region", CredentialsProviderSharedPtr{credentials_provider},
time_system_);
signer.sign(headers);

EXPECT_EQ("AWS4-HMAC-SHA256 Credential=akid/20180102/region/s3/aws4_request, "
"SignedHeaders=host;x-amz-content-sha256;x-amz-date, "
"Signature=d97cae067345792b78d2bad746f25c729b9eb4701127e13a7c80398f8216a167",
headers.get(Http::CustomHeaders::get().Authorization)->value().getStringView());
EXPECT_EQ(SignatureConstants::get().UnsignedPayload,
headers.get(SignatureHeaders::get().ContentSha256)->value().getStringView());
}

// Verify signing headers for non S3
TEST_F(SignerImplTest, SignHeadersNonS3) {
auto* credentials_provider = new NiceMock<MockCredentialsProvider>();
EXPECT_CALL(*credentials_provider, getCredentials()).WillOnce(Return(credentials_));
Http::TestRequestHeaderMapImpl headers{};
headers.setMethod("GET");
headers.setPath("/");
headers.addCopy(Http::LowerCaseString("host"), "www.example.com");

SignerImpl signer("service", "region", CredentialsProviderSharedPtr{credentials_provider},
time_system_);
signer.sign(headers);

EXPECT_EQ("AWS4-HMAC-SHA256 Credential=akid/20180102/region/service/aws4_request, "
"SignedHeaders=host;x-amz-content-sha256;x-amz-date, "
"Signature=d9fd9be575a254c924d843964b063d770181d938ae818f5b603ef0575a5ce2cd",
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.
TEST_F(SignerImplTest, SignHeadersByService) {
expectSignHeaders("s3", "d97cae067345792b78d2bad746f25c729b9eb4701127e13a7c80398f8216a167",
SignatureConstants::get().UnsignedPayload);
expectSignHeaders("service", "d9fd9be575a254c924d843964b063d770181d938ae818f5b603ef0575a5ce2cd",
SignatureConstants::get().HashedEmptyString);
expectSignHeaders("es", "0fd9c974bb2ad16c8d8a314dca4f6db151d32cbd04748d9c018afee2a685a02e",
SignatureConstants::get().UnsignedPayload);
expectSignHeaders("glacier", "8d1f241d77c64cda57b042cd312180f16e98dbd7a96e5545681430f8dbde45a0",
SignatureConstants::get().UnsignedPayload);
}

} // namespace
Expand Down
1 change: 1 addition & 0 deletions tools/spelling/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ GCP
GETting
GLB
GOAWAY
Glacier

@marcomagdy marcomagdy Jul 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@azihsoyn 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.

It was.

GRPC
GSS
GTEST
Expand Down