Add AWS http request signer by lavignes · Pull Request #5580 · envoyproxy/envoy

lavignes · 2019-01-12T01:39:50Z

Description: As part of leveraging Envoy in AWS App Mesh, we implemented a gRPC credentials extension that uses AWS IAM auth. To be able to use authenticate gRPC requests with AWS IAM credentials, we needed to add a code to Envoy to sign HTTP requests. More high-level details in #5215.

I opened up a prior PR: #5546 which was admittedly too large and unwieldy. This is part 1 of a series of smaller patches needed to add our gRPC authentication extension to Envoy.

Risk Level: Low
Testing: Unit tests for this API. This has also been verified to work when communicating with App Mesh.
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch

Looks great in general, some comments on structure, implementation and Envoy style. This is definitely heading in the right direction, thanks for splitting up the PRs!

htuch · 2019-01-14T04:26:56Z

source/extensions/filters/http/common/aws/BUILD

+envoy_package()
+
+envoy_cc_library(
+    name = "signer_lib",


Nit: if this is going to only ever be a pure interface, signer_interface would be more consistent with other conventions.

Good to know. I thought that convention was only used the public headers. e.g. include/envoy.

htuch · 2019-01-14T04:28:45Z

source/extensions/filters/http/common/aws/credentials_provider.h

+
+  const absl::optional<std::string>& secretAccessKey() const { return secret_access_key_; }
+
+  void setSessionToken(const std::string& session_token) {


Do we need both setters and constructors for these fields? Could this be an immutable object?

We don't need them. I can remove them.

htuch · 2019-01-14T04:29:39Z

source/extensions/filters/http/common/aws/credentials_provider.h

+
+  ~Credentials() = default;
+
+  Credentials(const std::string& access_key_id, const std::string& secret_access_key)


Nit: we tend to use absl::string_view rather than const std::string& in new code, as it's strictly more general. You can still turn this into a string copy below.

clang-tidy will suggest pass by value here and std::move below for constructors like this.
https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html

By doing this you may save one copy if caller does move too.

htuch · 2019-01-14T04:30:04Z

source/extensions/filters/http/common/aws/credentials_provider.h

+public:
+  Credentials() = default;
+
+  ~Credentials() = default;


Nit: probably don't need this if we're not doing polymorphic inheritance.

Removed the destructor, but I do use the no-args constructor.

htuch · 2019-01-14T04:37:45Z

source/extensions/filters/http/common/aws/region_provider.h

+namespace Common {
+namespace Aws {
+
+class RegionProvider {


I had a look at how this works in #5546. This allows both static and environment to specify region. I guess my main question is "do we really need this?". Specifically, Envoy's Node ID includes locality information (and a variety of ways to specify, from bootstrap config to CLI). Can we pull region from that in the interest of economy of configuration mechanism and making this more consistent with the rest of Envoy config?

My intention was to eventually add a region provider impl that pulls the region from an AWS-endpoint, much like we pull the credentials.

I guess the larger point is "shouldn't we be synthesizing the Node locality in the bootstrap or CLI for Envoys based on the AWS endpoint data source?" If you do that, there is no need to manage data sources for this information in the extension.

Credentials can also be placed in Node metadata, but the limitation there is that if they change dynamically during the lifetime of the Envoy, they can become stale. For region, this should be static for the lifetime of an Envoy.

Sorry for not addressing that larger point. I think it makes sense. I'll pipe the node metadata through in the later PR. I'll eliminate this class, especially since it will probably just provide static data for the foreseeable future.

htuch · 2019-01-14T04:42:57Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  // Path
+  const auto* path_header = headers.Path();
+  if (path_header == nullptr || path_header->value().empty()) {
+    throw EnvoyException("Message is missing :path header");


I don't think either the method or path should be missing by construction, so you can probably omit these.

Actually there doesn't seem to be anything preventing someone from not setting the path or method. Due to the way I plan to use this in the gRPC credentials plugin, I'd like to ensure these are set instead of seg-faulting.

Yeah, I was thinking on the request receive path, but here you are creating a new request.

htuch · 2019-01-14T04:43:36Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  }
+  out << "\n" << signing_headers << "\n";
+  // Content Hash
+  out << content_hash;


Might be slightly more readable to use fmt::format here instead of streams, but up to you if you prefer streams.

+1, IIRC fmt::format is much faster than streams, if this is potentially being used in data path, format is better.

This code is mostly just "\n" joining a bunch of strings. Though it is clearly not evident :). I'm updating it to use absl::StrJoin instead.

htuch · 2019-01-14T04:46:28Z

source/extensions/filters/http/common/aws/signer_impl.cc

+}
+
+std::map<std::string, std::string>
+SignerImpl::canonicalizeHeaders(const Http::HeaderMap& headers) const {


The logic in here is quite arcane; do you think we could split this out of a private method into a standalone utility and write some extensive tests for this? It seems a possible source of compliance bugs.

Sure. It is one of the trickier parts of the code. And even the public documentation of the canonicalization process glosses over some details.

htuch · 2019-01-14T04:47:41Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  RELEASE_ASSERT(len <= EVP_MAX_MD_SIZE, "Hmac length too large");
+  HMAC_CTX_cleanup(&ctx);
+  mac.resize(len);
+  return mac;


I think hmac and hash are general enough to put in source/common/ssl/utility.cc or somewhere like that, this seems like something other folks might want to reuse.

Thats a good idea

FYI, @bdecoste is working on moving SSL library dependent stuff to extensions, to allow compiling out BoringSSL #5543 , I think this might be fine but not sure how SSL library specific this is (BoringSSL vs OpenSSL).

htuch · 2019-01-14T04:48:25Z

test/extensions/filters/http/common/aws/signer_impl_test.cc

+               message_->headers().Authorization()->value().c_str());
+}
+
+TEST_F(SignerImplTest, ContentHeader) {


Can you add short // explanations of what each of these tests does above each TEST_F?

jmarantz · 2019-01-15T01:55:16Z

source/extensions/filters/http/common/aws/signer_impl.h

+
+#include "extensions/filters/http/common/aws/credentials_provider.h"
+#include "extensions/filters/http/common/aws/region_provider.h"
+#include "extensions/filters/http/common/aws/signer.h"


Would it make sense to have an abstract signing API in include/envoy somewhere, and have AwsSigner implement that as an extension?

I think this makes sense if we want a generic request signing extension. If @lavignes wants to take that on in this PR series, that's fine, otherwise we could leave a TODO and do the refactor when we have a second signing extension that wants to do something similar.

I opted for the TODO. It'd be trivial to expose it in a public header in the future.

* Use absl::string_view more aggresively * Removing Aws::RegionProvider * Move SHA-256 Digest/HMAC utils to common/ssl/utility * Removed setters from Aws::Credentials * Remove usage of stringstream * Use ConstSingleton for string constants * Add test comments Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch

This is looking great, just some minor stuff and would like @PiotrSikora to take a quick pass on the BoringSSL side.

htuch · 2019-01-16T06:56:14Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  // Path
+  const auto* path_header = headers.Path();
+  if (path_header == nullptr || path_header->value().empty()) {
+    throw EnvoyException("Message is missing :path header");


Yeah, I was thinking on the request receive path, but here you are creating a new request.

htuch · 2019-01-16T07:00:10Z

source/common/ssl/utility.cc

  return std::chrono::system_clock::from_time_t(days * 24 * 60 * 60 + seconds);
 }

+std::vector<uint8_t> Utility::getSha256Digest(const Buffer::Instance& buffer) {


@PiotrSikora can you do a review pass on these two functions and their tests from a BoringSSL perspective? Thanks.

htuch · 2019-01-16T07:03:01Z

source/common/ssl/utility.cc

+  unsigned int len;
+  rc = HMAC_Final(&ctx, mac.data(), &len);
+  RELEASE_ASSERT(rc == 1, "Failed to finalize HMAC");
+  RELEASE_ASSERT(len <= EVP_MAX_MD_SIZE, "HMAC length too large");


Is this best an ASSERT? I.e. it can never happen? Same with the other length check above?

I followed the conventions of the other parts of envoy that used these types of SSL apis. I think these, for all intents and purposes, don't happen.

htuch · 2019-01-16T07:08:24Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  parts.emplace_back(signing_headers);
+  parts.emplace_back(content_hash);
+  return absl::StrJoin(parts, "\n");
+}


This one might also belong in the utility like canonical headers creation, it has some edge cases that would benefit from unit tests unless already well covered.

I do have tests in the signer_tests specifically for this function but I can pull it out into a utility. I'm not sure what the convention is on util creation in envoy. Is it for any one function that is sufficiently complex on its own? Or is it just about reuse? I don't really see the request or header canonicalization being used outside of signing AWS requests.

It's kind of up to you largely, as long as you can provide adequate test coverage. The philosophy is at a bare minimum we have code coverage based on lines, but for complex/tricky code, it's often useful to ensure we can do behavioral/branch/functional coverage via unit tests. Some folks use testing peers, others code structure to do this. I personally don't like moving private to public methods in a class just for test, but I do like adding a logically separate utility class that has the methods as public where it makes sense.

htuch · 2019-01-16T07:09:42Z

source/extensions/filters/http/common/aws/signer_impl.h

+  DateFormatter long_date_formatter_;
+  DateFormatter short_date_formatter_;
+
+  std::string createContentHash(Http::Message& message) const;


Nit: generally we prefer methods before data members in each access scoped section, e.g. private here.

htuch · 2019-01-16T07:10:32Z

source/extensions/filters/http/common/aws/utility.h

+class Utility {
+public:
+  /**
+   * Creates a canonicalized header map according to


Maybe explain why we are doing this; it's just for signing, right? We never put these on the wire..

The whole canonicalization process is at the end of the day about consistently hashing HTTP requests. We do the exact same canonicalization upstream as part of our AuthN. This map is really just part of the input string to the hash we put in the Authorization header.

I'll explicitly say this is to be used as an input to the request canonicalization function. (Which I'll also move into this util)

htuch · 2019-01-16T07:11:17Z

source/extensions/filters/http/common/aws/utility.cc

+      [](const Http::HeaderEntry& entry, void* context) -> Http::HeaderMap::Iterate {
+        auto* map = static_cast<std::map<std::string, std::string>*>(context);
+        // Pseudo-headers should not be canonicalized
+        if (entry.key().c_str()[0] == ':') {


Maybe an empty header in there somehow as well?

htuch · 2019-01-16T07:12:38Z

source/extensions/filters/http/common/aws/utility.cc

+        }
+        return Http::HeaderMap::Iterate::Continue;
+      },
+      &out);


I have a feeling this is more generally useful (without the quirks below), but it's fine to leave here, just thinking out loud.

htuch · 2019-01-16T07:14:21Z

source/extensions/filters/http/common/aws/utility.cc

+    } else {
+      out.emplace(Http::Headers::get().HostLegacy.get(), std::string(value));
+    }
+  }


I'm going to completely trust you on the business logic here, code looks good.

PiotrSikora · 2019-01-16T09:52:59Z

source/common/common/logger.h

 // clang-format off
 #define ALL_LOGGER_IDS(FUNCTION) \
  FUNCTION(admin)                \
+  FUNCTION(aws)                  \


Is there any reason not to use http?

Given where the code is now, it could just be classified as http. I'm fine doing that. :)

PiotrSikora · 2019-01-16T10:13:36Z

source/extensions/filters/http/common/aws/signer_impl.cc

+namespace Common {
+namespace Aws {
+
+void SignerImpl::sign(Http::Message& message) {


This whole function assumes that the complete request body is available at this point. I'm not sure how are you going to use this, but Envoy doesn't buffer requests before forwarding them, so this won't work for proxied requests.

That is a good point. In the sigv4 algorithm, the request body signing is technically optional (for this very reason), but adds an extra layer of security for certain types of requests. I'll add a parameter, much like we do in the official SDK to control signing the body. The caveat being, that the body must be fully available at that point.

PiotrSikora · 2019-01-16T10:21:42Z

source/common/ssl/utility.cc

+  unsigned int digest_length;
+  rc = EVP_DigestFinal(&ctx, digest.data(), &digest_length);
+  RELEASE_ASSERT(rc == 1, "Failed to finalize digest");
+  RELEASE_ASSERT(digest_length == SHA256_DIGEST_LENGTH, "Digest length mismatch");


I know we check this in a few other places, but it turns out this isn't necessary.

PiotrSikora · 2019-01-16T10:31:03Z

source/common/ssl/utility.cc

+}
+
+std::vector<uint8_t> Utility::getSha256Hmac(const std::vector<uint8_t>& key,
+                                            absl::string_view string) {


Nit: string isn't very descriptive. Could you use message or data here?

PiotrSikora · 2019-01-16T10:46:09Z

source/common/ssl/utility.cc

+}
+
+std::vector<uint8_t> Utility::getSha256Hmac(const std::vector<uint8_t>& key,
+                                            absl::string_view string) {


Also, you can replace this whole function with:

std::vector<uint8_t> hmac(SHA256_DIGEST_LENGTH); unsigned int len; rc = HMAC(EVP_sha256(), key.data(), key.size(), message.data(), message.size(), hmac.data(), &len); RELEASE_ASSERT(rc != nullptr, "Failed to create HMAC"); return hmac;

PiotrSikora · 2019-01-16T10:49:06Z

source/common/ssl/utility.cc

@@ -1,8 +1,12 @@
 #include "common/ssl/utility.h"


Note: This file doesn't exist anymore. @bdecoste moved TLS socket to extensions, so that it can be compiled out (and another TLS library can be used in it's place), it's new home is in source/extensions/transport_sockets/tls.

Having said that, I'd prefer if you created another target for this (e.g. source/extensions/common/crypto?), since those functions have nothing to do with SSL/TLS.

I was actually thinking about making a "crypto" util but just ran with the suggestion to put it in common/ssl/utillty. Easy change :)

I think this should be in core actually, since even core code might want to compute HMAC and SHAs. How about source/common/crypto?

Right now I don't leverage any code that is different between Open/BoringSSL, but it would require some compile-time plumbing, which is not as clean as swapping in/out an extension, if neither lib is used or some other crypto impl was used instead.

How about I move this to source/common/crypto for now. And make an issue to propose adding a crypto impl extension api?

Sure, that sounds good. Would like to merge this PR soon, so whatever is reasonable here and we can then look at the next one, thanks.

* Remove Logging::Id::aws * Move crypto utils to Extensions::Common::Crypto * Make request body signing optional * Move request canonicalization method to Aws::Utility * Explicity check for empty headers when canonicalizing Signed-off-by: Scott LaVigne <lavignes@amazon.com>

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch · 2019-01-18T18:51:00Z

source/extensions/common/crypto/utility.cc

+                                            absl::string_view message) {
+  std::vector<uint8_t> hmac(SHA256_DIGEST_LENGTH);
+  unsigned int len;
+  auto ret =


htuch · 2019-01-18T18:53:22Z

source/extensions/filters/http/common/aws/signer_impl.cc

+  parts.emplace_back(signing_headers);
+  parts.emplace_back(content_hash);
+  return absl::StrJoin(parts, "\n");
+}


It's kind of up to you largely, as long as you can provide adequate test coverage. The philosophy is at a bare minimum we have code coverage based on lines, but for complex/tricky code, it's often useful to ensure we can do behavioral/branch/functional coverage via unit tests. Some folks use testing peers, others code structure to do this. I personally don't like moving private to public methods in a class just for test, but I do like adding a logically separate utility class that has the methods as public where it makes sense.

htuch

LGTM modulo remaining comments, thanks!

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch · 2019-01-22T03:41:57Z

LGTM, @PiotrSikora WDYT?

PiotrSikora

LGTM, sans the typo.

PiotrSikora · 2019-01-22T09:48:38Z

test/common/crypto/utility_test.cc

+namespace Common {
+namespace Crypto {
+
+TEST(UtilityTest, TestSha256Disgest) {


Typo: s/Disgest/Digest/g (here and in the 2 tests below).

* github/master: (78 commits) transport capture: rename to tap (#5666) Misc additions for stubbing out QUICHE platform impls. (#5684) router: fix http/2 shadow request with body. (#5674) jwt_authn: cleanup, separate verifier from the matcher (#5689) redis: add latency stats for commands (#5593) config: invoke initialization callbacks on remote close (#5671) ext_authz: v2Alpha migration and documentation improvements (#5672) Update Datadog tracer version to v0.4.1 (#5691) envoy: basic spellcheck (#5688) fix a google string vs std::string issue for easier import (#5685) tools: tagging deprecation issues as no stalebot (#5680) health filter: cache degraded health checks (#5659) upstream: fix degraded health check and thread posting (#5662) tracing: inject tracing context in router (#5661) mysql_utils.cc: access EnvoyException by reference. (#5669) tap: introduce HTTP tap filter (#5515) fix a comment error (#5664) docs: add relase note for degraded health (#5653) Add yes flag for gcc-7 apt-repository (#5657) test: fix ssl_integration_test flake. (#5650) ... Signed-off-by: Scott LaVigne <lavignes@amazon.com>

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch

Thanks!

As part of leveraging Envoy in AWS App Mesh, we implemented a gRPC credentials extension that uses AWS IAM auth. To be able to use authenticate gRPC requests with AWS IAM credentials, we needed to add a code to Envoy to sign HTTP requests. More high-level details in envoyproxy#5215. I opened up a prior PR: envoyproxy#5546 which was admittedly too large and unwieldy. This is part 1 of a series of smaller patches needed to add our gRPC authentication extension to Envoy. Risk Level: Low Testing: Unit tests for this API. This has also been verified to work when communicating with App Mesh. Signed-off-by: Scott LaVigne <lavignes@amazon.com> Signed-off-by: Dan Zhang <danzh@google.com>

As part of leveraging Envoy in AWS App Mesh, we implemented a gRPC credentials extension that uses AWS IAM auth. To be able to use authenticate gRPC requests with AWS IAM credentials, we needed to add a code to Envoy to sign HTTP requests. More high-level details in envoyproxy#5215. I opened up a prior PR: envoyproxy#5546 which was admittedly too large and unwieldy. This is part 1 of a series of smaller patches needed to add our gRPC authentication extension to Envoy. Risk Level: Low Testing: Unit tests for this API. This has also been verified to work when communicating with App Mesh. Signed-off-by: Scott LaVigne <lavignes@amazon.com> Signed-off-by: Scott LaVigne <1406859+lavignes@users.noreply.github.com>

As part of leveraging Envoy in AWS App Mesh, we implemented a gRPC credentials extension that uses AWS IAM auth. To be able to use authenticate gRPC requests with AWS IAM credentials, we needed to add a code to Envoy to sign HTTP requests. More high-level details in envoyproxy#5215. I opened up a prior PR: envoyproxy#5546 which was admittedly too large and unwieldy. This is part 1 of a series of smaller patches needed to add our gRPC authentication extension to Envoy. Risk Level: Low Testing: Unit tests for this API. This has also been verified to work when communicating with App Mesh. Signed-off-by: Scott LaVigne <lavignes@amazon.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Add AWS http request signer

929fc45

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

mattklein123 assigned htuch Jan 13, 2019

htuch suggested changes Jan 14, 2019

View reviewed changes

lizan added the waiting label Jan 14, 2019

jmarantz reviewed Jan 15, 2019

View reviewed changes

mattklein123 mentioned this pull request Jan 15, 2019

REST xDS API with HTTP GET requests #4526

Open

repokitteh-read-only bot removed the waiting label Jan 15, 2019

htuch suggested changes Jan 16, 2019

View reviewed changes

PiotrSikora suggested changes Jan 16, 2019

View reviewed changes

lavignes added 2 commits January 16, 2019 17:01

Formatting

78d49c8

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

htuch reviewed Jan 18, 2019

View reviewed changes

move Extensions::Common::Crypto to Envoy::Common::Crypto

c8b887c

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

PiotrSikora previously approved these changes Jan 23, 2019

View reviewed changes

lavignes added 2 commits January 23, 2019 11:00

Fix typo in crypto test names

0fd58cd

Signed-off-by: Scott LaVigne <lavignes@amazon.com>

lavignes dismissed PiotrSikora’s stale review via 0fd58cd January 23, 2019 19:18

htuch approved these changes Jan 23, 2019

View reviewed changes

htuch merged commit 040acac into envoyproxy:master Jan 23, 2019

lavignes deleted the aws-sigv4-signer branch February 5, 2019 18:45

ivitjuk mentioned this pull request Jun 5, 2019

Added AWS region provider #7172

Merged


		const absl::optional<std::string>& secretAccessKey() const { return secret_access_key_; }

		void setSessionToken(const std::string& session_token) {


		~Credentials() = default;

		Credentials(const std::string& access_key_id, const std::string& secret_access_key)

Conversation

lavignes commented Jan 12, 2019

Uh oh!

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

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!

lavignes Jan 15, 2019 • 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!

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!

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

lavignes Jan 15, 2019 •

edited

Loading