fix: sigv4 now correctly trims spaces

aws/SDK_CHANGELOG.md

@@ -5,6 +5,7 @@ vNext (Month Day, Year)
 
 **Breaking Changes**
 - `<operation>.make_operation(&config)` is now an `async` function for all operations. Code should be updated to call `.await`. This will only impact users using the low-level API. (smithy-rs#797)
+- :bug: S3 request metadata signing now correctly trims headers fixing [problems like this](https://github.com/awslabs/aws-sdk-rust/issues/248) (smithy-rs#761)
 
 **New this week**
 

aws/rust-runtime/aws-sigv4/Cargo.toml

@@ -20,11 +20,14 @@ chrono = { version = "0.4", default-features = false, features = ["clock", "std"
 form_urlencoded = { version = "1.0", optional = true }
 hex = "0.4"
 http = { version = "0.2", optional = true }
+once_cell = "1.8"
 percent-encoding = { version = "2.1", optional = true }
+regex = "1.5"
 ring = "0.16"
 tracing = "0.1"
 
 [dev-dependencies]
 bytes = "1"
-pretty_assertions = "1.0"
 httparse = "1.5"
+pretty_assertions = "1.0"
+proptest = "1"

aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs

@@ -17,6 +17,7 @@ use std::cmp::Ordering;
 use std::convert::TryFrom;
 use std::fmt;
 use std::fmt::Formatter;
+use std::str::FromStr;
 
 pub(crate) mod header {
     pub(crate) const X_AMZ_CONTENT_SHA_256: &str = "x-amz-content-sha256";
@@ -181,13 +182,22 @@ impl<'a> CanonicalRequest<'a> {
         date_time: &str,
     ) -> Result<(Vec<CanonicalHeaderName>, HeaderMap), Error> {
         // Header computation:
-        // The canonical request will include headers not present in the input. We need to clone
-        // the headers from the original request and add:
+        // The canonical request will include headers not present in the input. We need to clone and
+        // normalize the headers from the original request and add:
         // - host
         // - x-amz-date
         // - x-amz-security-token (if provided)
         // - x-amz-content-sha256 (if requested by signing settings)
-        let mut canonical_headers = req.headers().clone();
+        let mut canonical_headers = HeaderMap::with_capacity(req.headers().len());
+        for (name, value) in req.headers().iter() {
+            // Header names and values need to be normalized according to Step 4 of https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
+            // Using append instead of insert means this will not clobber headers that have the same lowercased name
+            canonical_headers.append(
+                HeaderName::from_str(&name.as_str().to_lowercase())?,
+                normalize_header_value(&value),
+            );
+        }
+
         Self::insert_host_header(&mut canonical_headers, req.uri());
 
         if params.settings.signature_location == SignatureLocation::Headers {
@@ -335,6 +345,68 @@ impl<'a> fmt::Display for CanonicalRequest<'a> {
     }
 }
 
+/// A regex for matching on 2 or more spaces that acts on bytes.
+static MULTIPLE_SPACES: once_cell::sync::Lazy<regex::bytes::Regex> =
+    once_cell::sync::Lazy::new(|| regex::bytes::Regex::new(r" {2,}").unwrap());
+
+/// Removes excess spaces before and after a given byte string, and converts multiple sequential
+/// spaces to a single space e.g. "  Some  example   text  " -> "Some example text".
+///
+/// This function ONLY affects spaces and not other kinds of whitespace.
+fn trim_all(text: &[u8]) -> Cow<'_, [u8]> {
+    // The normal trim function will trim non-breaking spaces and other various whitespace chars.
+    // S3 ONLY trims spaces so we use trim_matches to trim spaces only
+    let text = trim_spaces_from_byte_string(text);
+    MULTIPLE_SPACES.replace_all(text, " ".as_bytes())
+}
+
+/// Removes excess spaces before and after a given byte string by returning a subset of those bytes.
+/// Will return an empty slice if a string is composed entirely of whitespace.
+fn trim_spaces_from_byte_string(bytes: &[u8]) -> &[u8] {
+    if bytes.is_empty() {
+        return bytes;
+    }
+
+    let mut starting_index = 0;
+
+    for i in 0..bytes.len() {
+        // If we get to the end of the array without hitting a non-whitespace char, return empty slice
+        if i == bytes.len() - 1 {
+            // This range equates to an empty slice
+            return &bytes[0..0];
+        // otherwise, skip over each instance of whitespace
+        } else if bytes[i] == b' ' {
+            continue;
+        }
+
+        // return the index of the first non-whitespace character
+        starting_index = i;
+        break;
+    }
+
+    // Now we do the same but in reverse
+    let mut ending_index = 0;
+    for i in (0..bytes.len()).rev() {
+        // skip over each instance of whitespace
+        if bytes[i] == b' ' {
+            continue;
+        }
+
+        // return the index of the first non-whitespace character
+        ending_index = i;
+        break;
+    }
+
+    &bytes[starting_index..=ending_index]
+}
+
+/// Works just like [trim_all] but acts on HeaderValues instead of bytes
+fn normalize_header_value(header_value: &HeaderValue) -> HeaderValue {
+    let trimmed_value = trim_all(header_value.as_bytes());
+    // This can't fail because we started with a valid HeaderValue and then only trimmed spaces
+    HeaderValue::from_bytes(&trimmed_value).unwrap()
+}
+
 #[derive(Debug, PartialEq, Default)]
 pub(super) struct SignedHeaders {
     headers: Vec<CanonicalHeaderName>,
@@ -490,14 +562,17 @@ impl<'a> fmt::Display for StringToSign<'a> {
 #[cfg(test)]
 mod tests {
     use crate::date_fmt::parse_date_time;
-    use crate::http_request::canonical_request::{CanonicalRequest, SigningScope, StringToSign};
+    use crate::http_request::canonical_request::{
+        normalize_header_value, trim_all, CanonicalRequest, SigningScope, StringToSign,
+    };
     use crate::http_request::test::{test_canonical_request, test_request, test_sts};
     use crate::http_request::{
         PayloadChecksumKind, SignableBody, SignableRequest, SigningSettings,
     };
     use crate::http_request::{SignatureLocation, SigningParams};
     use crate::sign::sha256_hex_string;
     use pretty_assertions::assert_eq;
+    use proptest::{proptest, strategy::Strategy};
     use std::convert::TryFrom;
     use std::time::Duration;
 
@@ -662,4 +737,39 @@ mod tests {
         let values = canonical.values.into_query_params().unwrap();
         assert_eq!("host", values.signed_headers.as_str());
     }
+
+    #[test]
+    fn test_trim_all_handles_spaces_correctly() {
+        // Can't compare a byte array to a Cow so we convert both to slices before comparing
+        let expected = &b"Some example text"[..];
+        let actual = &trim_all(b"  Some  example   text  ")[..];
+
+        assert_eq!(expected, actual);
+    }
+
+    #[test]
+    fn test_trim_all_ignores_other_forms_of_whitespace() {
+        // Can't compare a byte array to a Cow so we convert both to slices before comparing
+        let expected = &b"\t\xA0Some\xA0 example \xA0text\xA0\n"[..];
+        // \xA0 is a non-breaking space character
+        let actual = &trim_all(b"\t\xA0Some\xA0     example   \xA0text\xA0\n")[..];
+
+        assert_eq!(expected, actual);
+    }
+
+    proptest! {
+        #[test]
+        fn test_trim_all_doesnt_elongate_strings(s in ".*") {
+            assert!(trim_all(s.as_bytes()).len() <= s.len())
+        }
+
+        // TODO: Using filter map is not ideal here but I wasn't sure how to define a range that covers
+        //       the extended ASCII chars above \x7F. It would be better to define a generator for
+        //       chars in the range of [\x21-\x7E\x80-\xFF] and then prop_map those into HeaderValues.
+        //       _(\x7F is the largest value accepted currently)_
+        #[test]
+        fn test_normalize_header_value_doesnt_panic(v in (".*").prop_filter_map("Must be a valid HeaderValue", |v| http::HeaderValue::from_maybe_shared(v).ok())) {
+            let _ = normalize_header_value(&v);
+        }
+    }
 }

aws/rust-runtime/aws-sigv4/src/http_request/sign.rs

@@ -426,6 +426,62 @@ mod tests {
         assert_req_eq!(expected, signed);
     }
 
+    #[test]
+    fn test_sign_headers_space_trimming() {
+        let settings = SigningSettings::default();
+        let params = SigningParams {
+            access_key: "AKIDEXAMPLE",
+            secret_key: "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY",
+            security_token: None,
+            region: "us-east-1",
+            service_name: "service",
+            date_time: parse_date_time("20150830T123600Z").unwrap(),
+            settings,
+        };
+
+        let original = http::Request::builder()
+            .uri("https://some-endpoint.some-region.amazonaws.com")
+            .header(
+                "some-header",
+                HeaderValue::from_str("  test  test   ").unwrap(),
+            )
+            .body("")
+            .unwrap();
+        let signable = SignableRequest::from(&original);
+        let out = sign(signable, &params).unwrap();
+        assert_eq!(
+            "0bd74dbf6f21161f61a1a3a1c313b6a4bc67ec57bf5ea9ae956a63753ca1d7f7",
+            out.signature
+        );
+
+        let mut signed = original;
+        out.output.apply_to_request(&mut signed);
+
+        let mut expected = http::Request::builder()
+            .uri("https://some-endpoint.some-region.amazonaws.com")
+            .header(
+                "some-header",
+                HeaderValue::from_str("  test  test   ").unwrap(),
+            )
+            .header(
+                "x-amz-date",
+                HeaderValue::from_str("20150830T123600Z").unwrap(),
+            )
+            .header(
+                "authorization",
+                HeaderValue::from_str(
+                    "AWS4-HMAC-SHA256 \
+                        Credential=AKIDEXAMPLE/20150830/us-east-1/service/aws4_request, \
+                        SignedHeaders=host;some-header;x-amz-date, \
+                        Signature=0bd74dbf6f21161f61a1a3a1c313b6a4bc67ec57bf5ea9ae956a63753ca1d7f7",
+                )
+                .unwrap(),
+            )
+            .body("")
+            .unwrap();
+        assert_req_eq!(expected, signed);
+    }
+
     #[test]
     fn apply_signing_instructions_headers() {
         let mut headers = HeaderMap::new();

aws/sdk/integration-tests/dynamodb/Cargo.toml

@@ -17,6 +17,7 @@ aws-smithy-types = { path = "../../build/aws-sdk/aws-smithy-types" }
 aws-types = { path = "../../build/aws-sdk/aws-types" }
 bytes = "1"
 criterion = { version = "0.3.4" }
+futures-util = "0.3"
 http = "0.2.4"
 serde_json = "1"
 tokio = { version = "1", features = ["full", "test-util"]}

aws/sdk/integration-tests/lambda/Cargo.toml

@@ -0,0 +1,21 @@
+[package]
+name = "lambda"
+version = "0.1.0"
+authors = ["AWS Rust SDK Team <aws-sdk-rust@amazon.com>", "Zelda Hessler <zhessler@amazon.com>"]
+edition = "2018"
+
+[dependencies]
+async-stream = "0.3"
+aws-http = { path = "../../build/aws-sdk/aws-http"}
+aws-hyper = { path = "../../build/aws-sdk/aws-hyper"}
+aws-sdk-lambda = { path = "../../build/aws-sdk/lambda" }
+base64 = "0.13"
+bytes = "1"
+futures-core = "0.3"
+http = "0.2.3"
+serde_json = "1"
+smithy-client = { path = "../../build/aws-sdk/smithy-client", features = ["test-util"] }
+smithy-eventstream = { path = "../../build/aws-sdk/smithy-eventstream" }
+smithy-http = { path = "../../build/aws-sdk/smithy-http" }
+tokio  = { version = "1", features = ["full"]}
+tracing-subscriber = "0.2.18"

aws/sdk/integration-tests/lambda/tests/blns/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) 2015-2020 Max Woolf
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.