From f8a799db2a1e3654fcc17a0d03160b79e19ac531 Mon Sep 17 00:00:00 2001
From: Nipunn Koorapati <nipunn1313@gmail.com>
Date: Fri, 3 Feb 2023 10:15:25 -0800
Subject: [PATCH] Fix handling of repeated headers in AWS request
 canonicalization (#2261)

---
 CHANGELOG.next.toml                           |  8 +++
 .../src/http_request/canonical_request.rs     | 55 +++++++++++++++----
 2 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
index 46be412dab..1f24199f58 100644
--- a/CHANGELOG.next.toml
+++ b/CHANGELOG.next.toml
@@ -124,3 +124,11 @@ The rationale behind this change is that the previous design was tailored toward
 references = ["smithy-rs#2276"]
 meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server"}
 author = "hlbarber"
+
+[[aws-sdk-rust]]
+message = """
+Fix request canonicalization for HTTP requests with repeated headers (for example S3's `GetObjectAttributes`). Previously requests with repeated headers would fail with a 403 signature mismatch due to this bug.
+"""
+references = ["smithy-rs#2261", "aws-sdk-rust#720"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "nipunn1313"
diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs b/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs
index b1aa4f2b00..172f2cbce7 100644
--- a/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs
+++ b/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs
@@ -13,7 +13,7 @@ use crate::http_request::url_escape::percent_encode_path;
 use crate::http_request::PercentEncodingMode;
 use crate::http_request::{PayloadChecksumKind, SignableBody, SignatureLocation, SigningParams};
 use crate::sign::sha256_hex_string;
-use http::header::{HeaderName, HOST};
+use http::header::{AsHeaderName, HeaderName, HOST};
 use http::{HeaderMap, HeaderValue, Method, Uri};
 use std::borrow::Cow;
 use std::cmp::Ordering;
@@ -225,7 +225,7 @@ impl<'a> CanonicalRequest<'a> {
         }
 
         let mut signed_headers = Vec::with_capacity(canonical_headers.len());
-        for (name, _) in &canonical_headers {
+        for name in canonical_headers.keys() {
             if let Some(excluded_headers) = params.settings.excluded_headers.as_ref() {
                 if excluded_headers.contains(name) {
                     continue;
@@ -328,6 +328,19 @@ impl<'a> CanonicalRequest<'a> {
         canonical_headers.insert(x_amz_date, date_header.clone());
         date_header
     }
+
+    fn header_values_for(&self, key: impl AsHeaderName) -> String {
+        let values: Vec<&str> = self
+            .headers
+            .get_all(key)
+            .into_iter()
+            .map(|value| {
+                std::str::from_utf8(value.as_bytes())
+                    .expect("SDK request header values are valid UTF-8")
+            })
+            .collect();
+        values.join(",")
+    }
 }
 
 impl<'a> fmt::Display for CanonicalRequest<'a> {
@@ -337,15 +350,8 @@ impl<'a> fmt::Display for CanonicalRequest<'a> {
         writeln!(f, "{}", self.params.as_deref().unwrap_or(""))?;
         // write out _all_ the headers
         for header in &self.values.signed_headers().headers {
-            // a missing header is a bug, so we should panic.
-            let value = &self.headers[&header.0];
             write!(f, "{}:", header.0.as_str())?;
-            writeln!(
-                f,
-                "{}",
-                std::str::from_utf8(value.as_bytes())
-                    .expect("SDK request header values are valid UTF-8")
-            )?;
+            writeln!(f, "{}", self.header_values_for(&header.0))?;
         }
         writeln!(f)?;
         // write out the signed headers
@@ -538,6 +544,35 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_repeated_header() {
+        let mut req = test_request("get-vanilla-query-order-key-case");
+        req.headers_mut().append(
+            "x-amz-object-attributes",
+            HeaderValue::from_static("Checksum"),
+        );
+        req.headers_mut().append(
+            "x-amz-object-attributes",
+            HeaderValue::from_static("ObjectSize"),
+        );
+        let req = SignableRequest::from(&req);
+        let settings = SigningSettings {
+            payload_checksum_kind: PayloadChecksumKind::XAmzSha256,
+            ..Default::default()
+        };
+        let signing_params = signing_params(settings);
+        let creq = CanonicalRequest::from(&req, &signing_params).unwrap();
+
+        assert_eq!(
+            creq.values.signed_headers().to_string(),
+            "host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes"
+        );
+        assert_eq!(
+            creq.header_values_for("x-amz-object-attributes"),
+            "Checksum,ObjectSize",
+        );
+    }
+
     #[test]
     fn test_set_xamz_sha_256() {
         let req = test_request("get-vanilla-query-order-key-case");