Correctly handle multi-value headers in Aws4Signer

.changes/next-release/bugfix-AWSSDKforJavav2-764e546.json

@@ -0,0 +1,6 @@
+{
+    "category": "AWS SDK for Java v2", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Correctly handle multi-value headers in Aws4Signer"
+}

core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java

@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.auth.credentials.AwsCredentials;
 import software.amazon.awssdk.auth.credentials.AwsSessionCredentials;
@@ -323,46 +324,51 @@ private String getCanonicalizedHeaderString(Map<String, List<String>> canonicali
         StringBuilder buffer = new StringBuilder();
 
         canonicalizedHeaders.forEach((headerName, headerValues) -> {
-            for (String headerValue : headerValues) {
-                appendCompactedString(buffer, headerName);
-                buffer.append(":");
-                if (headerValue != null) {
-                    appendCompactedString(buffer, headerValue);
-                }
-                buffer.append("\n");
-            }
+            buffer.append(headerName);
+            buffer.append(":");
+            buffer.append(String.join(",", trimAll(headerValues)));
+            buffer.append("\n");
         });
 
         return buffer.toString();
     }
 
     /**
-     * This method appends a string to a string builder and collapses contiguous
-     * white space is a single space.
-     *
-     * This is equivalent to:
-     *      destination.append(source.replaceAll("\\s+", " "))
+     * "The Trimall function removes excess white space before and after values,
+     * and converts sequential spaces to a single space."
+     * <p>
+     * https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
+     * <p>
+     * The collapse-whitespace logic is equivalent to:
+     * <pre>
+     *     value.replaceAll("\\s+", " ")
+     * </pre>
      * but does not create a Pattern object that needs to compile the match
      * string; it also prevents us from having to make a Matcher object as well.
-     *
      */
-    private void appendCompactedString(final StringBuilder destination, final String source) {
+    private String trimAll(String value) {
         boolean previousIsWhiteSpace = false;
-        int length = source.length();
+        StringBuilder sb = new StringBuilder(value.length());
 
-        for (int i = 0; i < length; i++) {
-            char ch = source.charAt(i);
+        for (int i = 0; i < value.length(); i++) {
+            char ch = value.charAt(i);
             if (isWhiteSpace(ch)) {
                 if (previousIsWhiteSpace) {
                     continue;
                 }
-                destination.append(' ');
+                sb.append(' ');
                 previousIsWhiteSpace = true;
             } else {
-                destination.append(ch);
+                sb.append(ch);
                 previousIsWhiteSpace = false;
             }
         }
+
+        return sb.toString().trim();
+    }
+
+    private List<String> trimAll(List<String> values) {
+        return values.stream().map(this::trimAll).collect(Collectors.toList());
     }
 
     /**

core/auth/src/test/java/software/amazon/awssdk/auth/signer/Aws4SignerTest.java

@@ -156,6 +156,48 @@ public void xAmznTraceId_NotSigned() throws Exception {
                           "Signature=581d0042389009a28d461124138f1fe8eeb8daed87611d2a2b47fd3d68d81d73");
     }
 
+    /**
+     * Multi-value headers should be comma separated.
+     */
+    @Test
+    public void canonicalizedHeaderString_multiValueHeaders_areCommaSeparated() throws Exception {
+        AwsBasicCredentials credentials = AwsBasicCredentials.create("akid", "skid");
+        SdkHttpFullRequest.Builder request = generateBasicRequest();
+        request.appendHeader("foo","bar");
+        request.appendHeader("foo","baz");
+
+        SdkHttpFullRequest actual = SignerTestUtils.signRequest(signer, request.build(), credentials, "demo", signingOverrideClock, "us-east-1");
+
+        // We cannot easily test the canonical header string value, but the below signature asserts that it contains:
+        // foo:bar,baz
+        assertThat(actual.firstMatchingHeader("Authorization"))
+            .hasValue("AWS4-HMAC-SHA256 Credential=akid/19810216/us-east-1/demo/aws4_request, " 
+                      + "SignedHeaders=foo;host;x-amz-archive-description;x-amz-date, " 
+                      + "Signature=1253bc1751048ea299e688cbe07a2224292e5cc606a079cb40459ad987793c19");
+    }
+
+    /**
+     * Canonical headers should remove excess white space before and after values, and convert sequential spaces to a single 
+     * space.
+     */
+    @Test
+    public void canonicalizedHeaderString_valuesWithExtraWhitespace_areTrimmed() throws Exception {
+        AwsBasicCredentials credentials = AwsBasicCredentials.create("akid", "skid");
+        SdkHttpFullRequest.Builder request = generateBasicRequest();
+        request.putHeader("My-header1","    a   b   c  ");
+        request.putHeader("My-Header2","    \"a   b   c\"  ");
+
+        SdkHttpFullRequest actual = SignerTestUtils.signRequest(signer, request.build(), credentials, "demo", signingOverrideClock, "us-east-1");
+
+        // We cannot easily test the canonical header string value, but the below signature asserts that it contains:
+        // my-header1:a b c
+        // my-header2:"a b c"
+        assertThat(actual.firstMatchingHeader("Authorization"))
+            .hasValue("AWS4-HMAC-SHA256 Credential=akid/19810216/us-east-1/demo/aws4_request, " 
+                      + "SignedHeaders=host;my-header1;my-header2;x-amz-archive-description;x-amz-date, " 
+                      + "Signature=6d3520e3397e7aba593d8ebd8361fc4405e90aed71bc4c7a09dcacb6f72460b9");
+    }
+
     private SdkHttpFullRequest.Builder generateBasicRequest() {
         return SdkHttpFullRequest.builder()
                                  .contentStreamProvider(() -> new ByteArrayInputStream("{\"TableName\": \"foo\"}".getBytes()))