Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ namespace Logger {
// clang-format off
#define ALL_LOGGER_IDS(FUNCTION) \
FUNCTION(admin) \
FUNCTION(aws) \

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.

Is there any reason not to use http?

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.

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

FUNCTION(assert) \
FUNCTION(backtrace) \
FUNCTION(client) \
Expand Down
2 changes: 2 additions & 0 deletions source/common/ssl/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ envoy_cc_library(
"ssl",
],
deps = [
"//include/envoy/buffer:buffer_interface",
"//source/common/common:assert_lib",
"//source/common/common:stack_array",
"//source/common/common:utility_lib",
],
)
42 changes: 42 additions & 0 deletions source/common/ssl/utility.cc
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
#include "common/ssl/utility.h"

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.

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.

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.

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

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.

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

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.

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?

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.

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.


#include "common/common/assert.h"
#include "common/common/stack_array.h"

#include "absl/strings/str_join.h"
#include "openssl/evp.h"
#include "openssl/hmac.h"
#include "openssl/sha.h"
#include "openssl/x509v3.h"

namespace Envoy {
Expand Down Expand Up @@ -101,5 +105,43 @@ SystemTime Utility::getExpirationTime(const X509& cert) {
return std::chrono::system_clock::from_time_t(days * 24 * 60 * 60 + seconds);
}

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

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.

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

std::vector<uint8_t> digest(SHA256_DIGEST_LENGTH);
EVP_MD_CTX ctx;
auto rc = EVP_DigestInit(&ctx, EVP_sha256());
RELEASE_ASSERT(rc == 1, "Failed to init digest context");
const auto num_slices = buffer.getRawSlices(nullptr, 0);
STACK_ARRAY(slices, Buffer::RawSlice, num_slices);
buffer.getRawSlices(slices.begin(), num_slices);
for (const auto& slice : slices) {
rc = EVP_DigestUpdate(&ctx, slice.mem_, slice.len_);
RELEASE_ASSERT(rc == 1, "Failed to update digest");
}
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");

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.

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

return digest;
}

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

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: string isn't very descriptive. Could you use message or data here?

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.

no problem

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.

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;

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.

sure thing

std::vector<uint8_t> mac(EVP_MAX_MD_SIZE);
HMAC_CTX ctx;
RELEASE_ASSERT(key.size() < std::numeric_limits<int>::max(), "HMAC key is too long");
HMAC_CTX_init(&ctx);
auto rc = HMAC_Init_ex(&ctx, key.data(), static_cast<int>(key.size()), EVP_sha256(), nullptr);
RELEASE_ASSERT(rc == 1, "Failed to init HMAC context");
rc = HMAC_Update(&ctx, reinterpret_cast<const uint8_t*>(string.data()), string.size());
RELEASE_ASSERT(rc == 1, "Failed to update HMAC");
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");

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.

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

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.

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.

HMAC_CTX_cleanup(&ctx);
mac.resize(len);
return mac;
}

} // namespace Ssl
} // namespace Envoy
17 changes: 17 additions & 0 deletions source/common/ssl/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <string>
#include <vector>

#include "envoy/buffer/buffer.h"

#include "common/common/utility.h"

#include "openssl/ssl.h"
Expand Down Expand Up @@ -56,6 +58,21 @@ SystemTime getValidFrom(const X509& cert);
*/
SystemTime getExpirationTime(const X509& cert);

/**
* Computes the SHA-256 digest of a buffer.
* @param buffer the buffer.
* @return a vector of bytes for the computed digest.
*/
std::vector<uint8_t> getSha256Digest(const Buffer::Instance& buffer);

/**
* Computes the SHA-256 HMAC for a given key and data.
* @param key the HMAC function key.
* @param string string data for the HMAC function.
* @return a vector of bytes for the computed HMAC.
*/
std::vector<uint8_t> getSha256Hmac(const std::vector<uint8_t>& key, absl::string_view string);

} // namespace Utility
} // namespace Ssl
} // namespace Envoy
51 changes: 51 additions & 0 deletions source/extensions/filters/http/common/aws/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "signer_interface",
hdrs = ["signer.h"],
deps = [
"//include/envoy/http:message_interface",
],
)

envoy_cc_library(
name = "signer_impl_lib",
srcs = ["signer_impl.cc"],
hdrs = ["signer_impl.h"],
deps = [
":credentials_provider_interface",
":signer_interface",
":utility_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:hex_lib",
"//source/common/common:logger_lib",
"//source/common/common:utility_lib",
"//source/common/http:headers_lib",
"//source/common/singleton:const_singleton",
"//source/common/ssl:utility_lib",
],
)

envoy_cc_library(
name = "credentials_provider_interface",
hdrs = ["credentials_provider.h"],
external_deps = ["abseil_optional"],
)

envoy_cc_library(
name = "utility_lib",
srcs = ["utility.cc"],
hdrs = ["utility.h"],
deps = [
"//source/common/common:utility_lib",
"//source/common/http:headers_lib",
],
)
53 changes: 53 additions & 0 deletions source/extensions/filters/http/common/aws/credentials_provider.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#pragma once

#include <string>

#include "envoy/common/pure.h"

#include "absl/strings/string_view.h"
#include "absl/types/optional.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

class Credentials {
public:
Credentials() = default;

Credentials(absl::string_view access_key_id, absl::string_view secret_access_key)
: access_key_id_(access_key_id), secret_access_key_(secret_access_key) {}

Credentials(absl::string_view access_key_id, absl::string_view secret_access_key,
absl::string_view session_token)
: access_key_id_(access_key_id), secret_access_key_(secret_access_key),
session_token_(session_token) {}

const absl::optional<std::string>& accessKeyId() const { return access_key_id_; }

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

const absl::optional<std::string>& sessionToken() const { return session_token_; }

private:
absl::optional<std::string> access_key_id_;
absl::optional<std::string> secret_access_key_;
absl::optional<std::string> session_token_;
};

class CredentialsProvider {
public:
virtual ~CredentialsProvider() = default;

virtual Credentials getCredentials() PURE;
};

typedef std::shared_ptr<CredentialsProvider> CredentialsProviderSharedPtr;

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
30 changes: 30 additions & 0 deletions source/extensions/filters/http/common/aws/signer.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#pragma once

#include "envoy/common/pure.h"
#include "envoy/http/message.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

// TODO(lavignes): Move this interface to include/envoy if this is needed elsewhere
class Signer {
public:
virtual ~Signer() = default;

/**
* Sign an AWS request.
* @param message an AWS API request message.
*/
virtual void sign(Http::Message& message) PURE;
};

typedef std::unique_ptr<Signer> SignerPtr;

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
145 changes: 145 additions & 0 deletions source/extensions/filters/http/common/aws/signer_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
#include "extensions/filters/http/common/aws/signer_impl.h"

#include "envoy/common/exception.h"

#include "common/buffer/buffer_impl.h"
#include "common/common/fmt.h"
#include "common/common/hex.h"
#include "common/http/headers.h"
#include "common/ssl/utility.h"

#include "extensions/filters/http/common/aws/utility.h"

#include "absl/strings/str_join.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

void SignerImpl::sign(Http::Message& message) {

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.

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.

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.

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.

const auto& credentials = credentials_provider_->getCredentials();
if (!credentials.accessKeyId() || !credentials.secretAccessKey()) {
// Empty or "anonymous" credentials are a valid use-case for non-production environments.
// This behavior matches what the AWS SDK would do.
return;

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.

Silent failure?

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.

Not exactly. We treat no credentials as being an anonymous request. This is important for testing situations where a developer may not want to (or is able to) authenticate it. I could log here that the request is not being signed..

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.

Got it, I think maybe just a comment then to explain this would be helpful.

}
auto& headers = message.headers();
if (credentials.sessionToken()) {
headers.addCopy(SignatureHeaders::get().SecurityToken, credentials.sessionToken().value());
}
const auto long_date = long_date_formatter_.now(time_source_);
const auto short_date = short_date_formatter_.now(time_source_);
headers.addCopy(SignatureHeaders::get().Date, long_date);
const auto content_hash = createContentHash(message);
headers.addCopy(SignatureHeaders::get().ContentSha256, content_hash);
// Phase 1: Create a canonical request
const auto canonical_headers = Utility::canonicalizeHeaders(headers);
const auto signing_headers = createSigningHeaders(canonical_headers);
const auto canonical_request =
createCanonicalRequest(message, canonical_headers, signing_headers, content_hash);
ENVOY_LOG(debug, "Canonical request:\n{}", canonical_request);
// Phase 2: Create a string to sign
const auto credential_scope = createCredentialScope(short_date);
const auto string_to_sign = createStringToSign(canonical_request, long_date, credential_scope);
ENVOY_LOG(debug, "String to sign:\n{}", string_to_sign);
// Phase 3: Create a signature
const auto signature =
createSignature(credentials.secretAccessKey().value(), short_date, string_to_sign);
// Phase 4: Sign request
const auto authorization_header = createAuthorizationHeader(
credentials.accessKeyId().value(), credential_scope, signing_headers, signature);
ENVOY_LOG(debug, "Signing request with: {}", authorization_header);
headers.addCopy(Http::Headers::get().Authorization, authorization_header);
}

std::string SignerImpl::createContentHash(Http::Message& message) const {
if (!message.body()) {
return SignatureConstants::get().HashedEmptyString;
}
return Hex::encode(Ssl::Utility::getSha256Digest(*message.body()));
}

std::string SignerImpl::createCanonicalRequest(
Http::Message& message, const std::map<std::string, std::string>& canonical_headers,
absl::string_view signing_headers, absl::string_view content_hash) const {
std::vector<absl::string_view> parts;
const auto* method_header = message.headers().Method();
if (method_header == nullptr || method_header->value().empty()) {
throw EnvoyException("Message is missing :method header");
}
parts.emplace_back(method_header->value().getStringView());
const auto* path_header = message.headers().Path();
if (path_header == nullptr || path_header->value().empty()) {
throw EnvoyException("Message is missing :path header");

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.

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

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.

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.

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.

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.

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

}
// don't include the query part of the path
const auto path = StringUtil::cropRight(path_header->value().getStringView(), "?");
parts.emplace_back(path.empty() ? "/" : path);
const auto query = StringUtil::cropLeft(path_header->value().getStringView(), "?");
// if query == path, then there is no query
parts.emplace_back(query == path ? "" : query);
std::vector<std::string> formatted_headers;
formatted_headers.reserve(canonical_headers.size());
for (const auto& header : canonical_headers) {
formatted_headers.emplace_back(fmt::format("{}:{}", header.first, header.second));
parts.emplace_back(formatted_headers.back());
}
// need an extra blank space after the canonical headers
parts.emplace_back("");
parts.emplace_back(signing_headers);
parts.emplace_back(content_hash);
return absl::StrJoin(parts, "\n");
}

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.

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.

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.

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.

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.

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.


std::string SignerImpl::createSigningHeaders(
const std::map<std::string, std::string>& canonical_headers) const {
std::vector<absl::string_view> keys;
keys.reserve(canonical_headers.size());
for (const auto& header : canonical_headers) {
keys.emplace_back(header.first);
}
return absl::StrJoin(keys, ";");
}

std::string SignerImpl::createCredentialScope(absl::string_view short_date) const {
return fmt::format(SignatureConstants::get().CredentialScopeFormat, short_date, region_,
service_name_);
}

std::string SignerImpl::createStringToSign(absl::string_view canonical_request,
absl::string_view long_date,
absl::string_view credential_scope) const {
return fmt::format(
SignatureConstants::get().StringToSignFormat, long_date, credential_scope,
Hex::encode(Ssl::Utility::getSha256Digest(Buffer::OwnedImpl(canonical_request))));
}

std::string SignerImpl::createSignature(absl::string_view secret_access_key,
absl::string_view short_date,
absl::string_view string_to_sign) const {
const auto secret_key =
absl::StrCat(SignatureConstants::get().SignatureVersion, secret_access_key);
const auto date_key = Ssl::Utility::getSha256Hmac(
std::vector<uint8_t>(secret_key.begin(), secret_key.end()), short_date);
const auto region_key = Ssl::Utility::getSha256Hmac(date_key, region_);
const auto service_key = Ssl::Utility::getSha256Hmac(region_key, service_name_);
const auto signing_key =
Ssl::Utility::getSha256Hmac(service_key, SignatureConstants::get().Aws4Request);
return Hex::encode(Ssl::Utility::getSha256Hmac(signing_key, string_to_sign));
}

std::string SignerImpl::createAuthorizationHeader(absl::string_view access_key_id,
absl::string_view credential_scope,
absl::string_view signing_headers,
absl::string_view signature) const {
return fmt::format(SignatureConstants::get().AuthorizationHeaderFormat, access_key_id,
credential_scope, signing_headers, signature);
}

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
Loading