diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 357698e616675..e5847f63b4af0 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -39,6 +39,7 @@ Bug Fixes *Changes expected to improve the state of the world and are unlikely to have negative effects* * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. +* aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion `_. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. diff --git a/source/extensions/common/aws/BUILD b/source/extensions/common/aws/BUILD index 2f79649e36a05..8cdd3fc95a72d 100644 --- a/source/extensions/common/aws/BUILD +++ b/source/extensions/common/aws/BUILD @@ -62,6 +62,7 @@ envoy_cc_library( hdrs = ["utility.h"], external_deps = ["curl"], deps = [ + "//source/common/common:empty_string", "//source/common/common:utility_lib", "//source/common/http:headers_lib", ], diff --git a/source/extensions/common/aws/signer_impl.cc b/source/extensions/common/aws/signer_impl.cc index 368285c91fb05..a071e29149671 100644 --- a/source/extensions/common/aws/signer_impl.cc +++ b/source/extensions/common/aws/signer_impl.cc @@ -59,7 +59,7 @@ void SignerImpl::sign(Http::RequestHeaderMap& headers, const std::string& conten // Phase 1: Create a canonical request const auto canonical_headers = Utility::canonicalizeHeaders(headers); const auto canonical_request = Utility::createCanonicalRequest( - method_header->value().getStringView(), path_header->value().getStringView(), + service_name_, method_header->value().getStringView(), path_header->value().getStringView(), canonical_headers, content_hash); ENVOY_LOG(debug, "Canonical request:\n{}", canonical_request); // Phase 2: Create a string to sign diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc index 497e37f409628..a0ba42fe4eb89 100644 --- a/source/extensions/common/aws/utility.cc +++ b/source/extensions/common/aws/utility.cc @@ -1,9 +1,12 @@ #include "source/extensions/common/aws/utility.h" +#include "source/common/common/empty_string.h" #include "source/common/common/fmt.h" #include "source/common/common/utility.h" +#include "absl/strings/match.h" #include "absl/strings/str_join.h" +#include "absl/strings/str_split.h" #include "curl/curl.h" namespace Envoy { @@ -11,6 +14,15 @@ namespace Extensions { namespace Common { namespace Aws { +constexpr absl::string_view PATH_SPLITTER = "/"; +constexpr absl::string_view QUERY_PARAM_SEPERATOR = "="; +constexpr absl::string_view QUERY_SEPERATOR = "&"; +constexpr absl::string_view QUERY_SPLITTER = "?"; +constexpr absl::string_view RESERVED_CHARS = "-._~"; +constexpr absl::string_view S3_SERVICE_NAME = "s3"; +const std::string URI_ENCODE = "%{:02X}"; +const std::string URI_DOUBLE_ENCODE = "%25{:02X}"; + std::map Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) { std::map out; @@ -58,18 +70,22 @@ Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) { return out; } -std::string -Utility::createCanonicalRequest(absl::string_view method, absl::string_view path, - const std::map& canonical_headers, - absl::string_view content_hash) { +std::string Utility::createCanonicalRequest( + absl::string_view service_name, absl::string_view method, absl::string_view path, + const std::map& canonical_headers, absl::string_view content_hash) { std::vector parts; parts.emplace_back(method); // don't include the query part of the path - const auto path_part = StringUtil::cropRight(path, "?"); - parts.emplace_back(path_part.empty() ? "/" : path_part); - const auto query_part = StringUtil::cropLeft(path, "?"); + const auto path_part = StringUtil::cropRight(path, QUERY_SPLITTER); + const auto canonicalized_path = path_part.empty() + ? std::string{PATH_SPLITTER} + : canonicalizePathString(path_part, service_name); + parts.emplace_back(canonicalized_path); + const auto query_part = StringUtil::cropLeft(path, QUERY_SPLITTER); // if query_part == path_part, then there is no query - parts.emplace_back(query_part == path_part ? "" : query_part); + const auto canonicalized_query = + query_part == path_part ? EMPTY_STRING : Utility::canonicalizeQueryString(query_part); + parts.emplace_back(absl::string_view(canonicalized_query)); std::vector formatted_headers; formatted_headers.reserve(canonical_headers.size()); for (const auto& header : canonical_headers) { @@ -77,13 +93,120 @@ Utility::createCanonicalRequest(absl::string_view method, absl::string_view path parts.emplace_back(formatted_headers.back()); } // need an extra blank space after the canonical headers - parts.emplace_back(""); + parts.emplace_back(EMPTY_STRING); const auto signed_headers = Utility::joinCanonicalHeaderNames(canonical_headers); parts.emplace_back(signed_headers); parts.emplace_back(content_hash); return absl::StrJoin(parts, "\n"); } +/** + * Normalizes the path string based on AWS requirements. + * See step 2 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + */ +std::string Utility::canonicalizePathString(absl::string_view path_string, + absl::string_view service_name) { + // If service is S3, do not normalize but only encode the path + if (absl::EqualsIgnoreCase(service_name, S3_SERVICE_NAME)) { + return encodePathSegment(path_string, service_name); + } + // If service is not S3, normalize and encode the path + const auto path_segments = StringUtil::splitToken(path_string, std::string{PATH_SPLITTER}); + std::vector path_list; + path_list.reserve(path_segments.size()); + for (const auto& path_segment : path_segments) { + if (path_segment.empty()) { + continue; + } + path_list.emplace_back(encodePathSegment(path_segment, service_name)); + } + auto canonical_path_string = + fmt::format("{}{}", PATH_SPLITTER, absl::StrJoin(path_list, PATH_SPLITTER)); + // Handle corner case when path ends with '/' + if (absl::EndsWith(path_string, PATH_SPLITTER) && canonical_path_string.size() > 1) { + canonical_path_string.push_back(PATH_SPLITTER[0]); + } + return canonical_path_string; +} + +bool isReservedChar(const char c) { + return std::isalnum(c) || RESERVED_CHARS.find(c) != std::string::npos; +} + +void encodeS3Path(std::string& encoded, const char& c) { + // Do not encode '/' for S3 + if (c == PATH_SPLITTER[0]) { + encoded.push_back(c); + } else { + absl::StrAppend(&encoded, fmt::format(URI_ENCODE, c)); + } +} + +std::string Utility::encodePathSegment(absl::string_view decoded, absl::string_view service_name) { + std::string encoded; + for (char c : decoded) { + if (isReservedChar(c)) { + // Escape unreserved chars from RFC 3986 + encoded.push_back(c); + } else if (absl::EqualsIgnoreCase(service_name, S3_SERVICE_NAME)) { + encodeS3Path(encoded, c); + } else { + // TODO: @aws, There is some inconsistency between AWS services if this should be double + // encoded or not. We need to parameterize this and expose this in the config. Ref: + // https://github.com/aws/aws-sdk-cpp/blob/main/aws-cpp-sdk-core/source/auth/AWSAuthSigner.cpp#L79-L93 + absl::StrAppend(&encoded, fmt::format(URI_ENCODE, c)); + } + } + return encoded; +} + +/** + * Normalizes the query string based on AWS requirements. + * See step 3 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + */ +std::string Utility::canonicalizeQueryString(absl::string_view query_string) { + // Sort query string based on param name and append "=" if value is missing + const auto query_fragments = StringUtil::splitToken(query_string, QUERY_SEPERATOR); + std::vector> query_list; + for (const auto& query_fragment : query_fragments) { + // Only split at the first "=" and encode the rest + const std::vector query = + absl::StrSplit(query_fragment, absl::MaxSplits(QUERY_PARAM_SEPERATOR, 1)); + if (!query.empty()) { + const absl::string_view param = query[0]; + const absl::string_view value = query.size() > 1 ? query[1] : EMPTY_STRING; + query_list.emplace_back(std::make_pair(param, value)); + } + } + // Sort query params by name and value + std::sort(query_list.begin(), query_list.end()); + // Encode query params name and value separately + for (auto& query : query_list) { + query = std::make_pair(Utility::encodeQueryParam(query.first), + Utility::encodeQueryParam(query.second)); + } + return absl::StrJoin(query_list, QUERY_SEPERATOR, absl::PairFormatter(QUERY_PARAM_SEPERATOR)); +} + +std::string Utility::encodeQueryParam(absl::string_view decoded) { + std::string encoded; + for (char c : decoded) { + if (isReservedChar(c) || c == '%') { + // Escape unreserved chars from RFC 3986 + encoded.push_back(c); + } else if (c == '+') { + // Encode '+' as space + absl::StrAppend(&encoded, "%20"); + } else if (c == QUERY_PARAM_SEPERATOR[0]) { + // Double encode '=' + absl::StrAppend(&encoded, fmt::format(URI_DOUBLE_ENCODE, c)); + } else { + absl::StrAppend(&encoded, fmt::format(URI_ENCODE, c)); + } + } + return encoded; +} + std::string Utility::joinCanonicalHeaderNames(const std::map& canonical_headers) { return absl::StrJoin(canonical_headers, ";", [](auto* out, const auto& pair) { diff --git a/source/extensions/common/aws/utility.h b/source/extensions/common/aws/utility.h index 79f9b937b8959..36b34da02a9f2 100644 --- a/source/extensions/common/aws/utility.h +++ b/source/extensions/common/aws/utility.h @@ -21,16 +21,51 @@ class Utility { /** * Creates an AWS Signature V4 canonical request string. * See https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + * @param service_name the AWS service name. * @param method the HTTP request method. * @param path the request path. * @param canonical_headers the pre-canonicalized request headers. * @param content_hash the hashed request body. * @return the canonicalized request string. */ - static std::string - createCanonicalRequest(absl::string_view method, absl::string_view path, - const std::map& canonical_headers, - absl::string_view content_hash); + static std::string createCanonicalRequest( + absl::string_view service_name, absl::string_view method, absl::string_view path, + const std::map& canonical_headers, absl::string_view content_hash); + + /** + * Normalizes the path string based on AWS requirements. + * See step 2 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + * @param query_string the query string from the HTTP request. + * @param service_name the AWS service name. + * @return the canonicalized query string. + */ + static std::string canonicalizePathString(absl::string_view path_string, + absl::string_view service_name); + + /** + * URI encodes the given string based on AWS requirements. + * See step 2 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + * @param decoded the decoded string. + * @param service_name the AWS service name. + * @return the URI encoded string. + */ + static std::string encodePathSegment(absl::string_view decoded, absl::string_view service_name); + + /** + * Normalizes the query string based on AWS requirements. + * See step 3 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + * @param query_string the query string from the HTTP request. + * @return the canonicalized query string. + */ + static std::string canonicalizeQueryString(absl::string_view query_string); + + /** + * URI encodes the given string based on AWS requirements. + * See step 3 in https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html + * @param decoded the decoded string. + * @return the URI encoded string. + */ + static std::string encodeQueryParam(absl::string_view decoded); /** * Get the semicolon-delimited string of canonical header names. diff --git a/test/extensions/common/aws/utility_test.cc b/test/extensions/common/aws/utility_test.cc index 1f115d9baff27..d19f8e220356a 100644 --- a/test/extensions/common/aws/utility_test.cc +++ b/test/extensions/common/aws/utility_test.cc @@ -100,7 +100,8 @@ TEST(UtilityTest, CanonicalizeHeadersDropMutatingHeaders) { // Verify the format of a minimalist canonical request TEST(UtilityTest, MinimalCanonicalRequest) { std::map headers; - const auto request = Utility::createCanonicalRequest("GET", "", headers, "content-hash"); + const auto request = + Utility::createCanonicalRequest("appmesh", "GET", "", headers, "content-hash"); EXPECT_EQ(R"(GET / @@ -112,10 +113,11 @@ content-hash)", TEST(UtilityTest, CanonicalRequestWithQueryString) { const std::map headers; - const auto request = Utility::createCanonicalRequest("GET", "?query", headers, "content-hash"); + const auto request = + Utility::createCanonicalRequest("appmesh", "GET", "?query", headers, "content-hash"); EXPECT_EQ(R"(GET / -query +query= content-hash)", @@ -128,7 +130,8 @@ TEST(UtilityTest, CanonicalRequestWithHeaders) { {"header2", "value2"}, {"header3", "value3"}, }; - const auto request = Utility::createCanonicalRequest("GET", "", headers, "content-hash"); + const auto request = + Utility::createCanonicalRequest("appmesh", "GET", "", headers, "content-hash"); EXPECT_EQ(R"(GET / @@ -141,6 +144,166 @@ content-hash)", request); } +TEST(UtilityTest, CanonicalizePathStringReturnSlash) { + const absl::string_view path = ""; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringSlash) { + const absl::string_view path = "/"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringSlashes) { + const absl::string_view path = "///"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringPrefixSlash) { + const absl::string_view path = "test"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringSuffixSlash) { + const absl::string_view path = "test/"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringNormalizeSlash) { + const absl::string_view path = "test////test///"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test/test/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringWithEncoding) { + const absl::string_view path = "test$file.txt"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test%24file.txt", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringWithEncodingSpaces) { + const absl::string_view path = "/test and test/"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test%20and%20test/", canonical_path); +} + +TEST(UtilityTest, CanonicalizePathStringWithAlreadyEncodedSpaces) { + const absl::string_view path = "/test%20and%20test/"; + const auto canonical_path = Utility::canonicalizePathString(path, "appmesh"); + EXPECT_EQ("/test%2520and%2520test/", canonical_path); +} + +TEST(UtilityTest, CanonicalizeS3PathStringDoNotNormalizeSlash) { + const absl::string_view path = "/test//test///"; + const auto canonical_path = Utility::canonicalizePathString(path, "s3"); + EXPECT_EQ("/test//test///", canonical_path); +} + +TEST(UtilityTest, CanonicalizeS3PathStringSlashes) { + const absl::string_view path = "///"; + const auto canonical_path = Utility::canonicalizePathString(path, "s3"); + EXPECT_EQ("///", canonical_path); +} + +TEST(UtilityTest, CanonicalizeS3PathStringWithEncoding) { + const absl::string_view path = "/test$file.txt"; + const auto canonical_path = Utility::canonicalizePathString(path, "s3"); + EXPECT_EQ("/test%24file.txt", canonical_path); +} + +TEST(UtilityTest, CanonicalizeS3PathStringWithEncodingSpaces) { + const absl::string_view path = "/test and test/"; + const auto canonical_path = Utility::canonicalizePathString(path, "s3"); + EXPECT_EQ("/test%20and%20test/", canonical_path); +} + +TEST(UtilityTest, EncodePathSegment) { + const absl::string_view path = "test^!@=-_~."; + const auto encoded_path = Utility::encodePathSegment(path, "appmesh"); + EXPECT_EQ("test%5E%21%40%3D-_~.", encoded_path); +} + +TEST(UtilityTest, EncodeS3PathSegment) { + const absl::string_view path = "/test/^!@=/-_~."; + const auto encoded_path = Utility::encodePathSegment(path, "s3"); + EXPECT_EQ("/test/%5E%21%40%3D/-_~.", encoded_path); +} + +TEST(UtilityTest, CanonicalizeQueryString) { + const absl::string_view query = "a=1&b=2"; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=1&b=2", canonical_query); +} + +TEST(UtilityTest, CanonicalizeQueryStringTrailingEquals) { + const absl::string_view query = "a&b"; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=&b=", canonical_query); +} + +TEST(UtilityTest, CanonicalizeQueryStringSorted) { + const absl::string_view query = "a=3&b=1&a=2&a=1"; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=1&a=2&a=3&b=1", canonical_query); +} + +TEST(UtilityTest, CanonicalizeQueryStringEncoded) { + const absl::string_view query = "a=^!@&b=/-_~."; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=%5E%21%40&b=%2F-_~.", canonical_query); +} + +TEST(UtilityTest, CanonicalizeQueryStringWithPlus) { + const absl::string_view query = "a=1+2"; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=1%202", canonical_query); +} + +TEST(UtilityTest, CanonicalizeQueryStringDoubleEncodeEquals) { + const absl::string_view query = "a=!.!=!"; + const auto canonical_query = Utility::canonicalizeQueryString(query); + EXPECT_EQ("a=%21.%21%253D%21", canonical_query); +} + +TEST(UtilityTest, EncodeQuerySegment) { + const absl::string_view query = "^!@/-_~."; + const auto encoded_query = Utility::encodeQueryParam(query); + EXPECT_EQ("%5E%21%40%2F-_~.", encoded_query); +} + +TEST(UtilityTest, EncodeQuerySegmentReserved) { + const absl::string_view query = "?=&"; + const auto encoded_query = Utility::encodeQueryParam(query); + EXPECT_EQ("%3F%253D%26", encoded_query); +} + +TEST(UtilityTest, CanonicalizationFuzzTest) { + std::string fuzz; + fuzz.reserve(3); + // Printable ASCII 32 - 126 + for (unsigned char i = 32; i <= 126; i++) { + fuzz.push_back(i); + for (unsigned char j = 32; j <= 126; j++) { + fuzz.push_back(j); + for (unsigned char k = 32; k <= 126; k++) { + fuzz.push_back(k); + Utility::encodePathSegment(fuzz, "s3"); + Utility::canonicalizePathString(fuzz, "appmesh"); + Utility::encodeQueryParam(fuzz); + Utility::canonicalizeQueryString(fuzz); + fuzz.pop_back(); + } + fuzz.pop_back(); + } + fuzz.pop_back(); + } +} + // Verify headers are joined with ";" TEST(UtilityTest, JoinCanonicalHeaderNames) { std::map headers = {