Support for canonicalizing URI properly for AWS SigV4 signer

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 <https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html>`_.
 * 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 <https://docs.aws.amazon.com/xray/latest/devguide/xray-api-segmentdocuments.html>`_. Before this fix, server error was reported under 'annotations' section of the segment data.
 

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",
     ],

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

source/extensions/common/aws/utility.cc

@@ -1,16 +1,28 @@
 #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 {
 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<std::string, std::string>
 Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) {
   std::map<std::string, std::string> out;
@@ -58,32 +70,143 @@ Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) {
   return out;
 }
 
-std::string
-Utility::createCanonicalRequest(absl::string_view method, absl::string_view path,
-                                const std::map<std::string, std::string>& 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<std::string, std::string>& canonical_headers, absl::string_view content_hash) {
   std::vector<absl::string_view> 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<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(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<std::string> 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<std::pair<std::string, std::string>> query_list;
+  for (const auto& query_fragment : query_fragments) {
+    // Only split at the first "=" and encode the rest
+    const std::vector<std::string> 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<std::string, std::string>& canonical_headers) {
   return absl::StrJoin(canonical_headers, ";", [](auto* out, const auto& pair) {

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<std::string, std::string>& 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<std::string, std::string>& 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.