Fix aws-sigv4 canonical request formatting fallibility (#1656)

* Fix panic occurred in formatting CanonicalRequest

This commit addresses panic in the case of formatting a CanonicalRequest
containing invalid UTF-8 in the header value.

The commit aims for the least disturbance to the codebase in order to
pass a given failing test in the PR. We want to quickly determine if
this low-cost approach addresses the problem before we commit ourselves
to start refactoring CanonicalRequest to have a special format function.

Fixes #711

* Update test_signing_utf8_headers to proptest

This commit converts test_signing_utf8_headers to a proptest. The
original test only specified hardcoded non-UTF8 bytes in a request
header. To ensure that `crate::http_request::sign` does not panic no
matter what valid byte sequence for HeaderValue is, the proptest covers
a wider range of inputs.

For consistency, the test has been moved from `canonical_request.rs` to
`sign.rs`.

* Add InvalidHeaderError to make the error explicit

This commit introduces an error type InvalidHeaderError to indicate that
we ran into a problem in handling a HeaderValue within CanonicalRequest.

The error type contains a source error such as Utf8Error so a diagnostic
message can be printed if needed.

* Remove InvalidHeaderError for error refactoring

This commit effectively reverts 739b32c. Knowing that we will be cleaning
up error types, having InvalidHeaderError is too narrow a solution and
does not add value to the codebase.

* Update CHANGELOG.next.toml

Co-authored-by: Russell Cohen <rcoh@amazon.com>
Co-authored-by: Saito <awsaito@c889f3b5ddc4.ant.amazon.com>

Author: 3 people

CHANGELOG.next.toml

@@ -186,3 +186,9 @@ message = "Ability to override the IMDS client in `DefaultCredentialsChain`"
 references = ["aws-sdk-rust#625"]
 meta = { "breaking" = false, "tada" = false, "bug" = false }
 author = "kevinpark1217"
+
+[[aws-sdk-rust]]
+message = "Fix aws-sigv4 canonical request formatting fallibility."
+references = ["smithy-rs#1656"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "ysaito1001"

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

@@ -196,7 +196,7 @@ impl<'a> CanonicalRequest<'a> {
             // 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),
+                normalize_header_value(value)?,
             );
         }
 
@@ -373,11 +373,11 @@ fn trim_spaces_from_byte_string(bytes: &[u8]) -> &[u8] {
     &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 {
+/// Works just like [trim_all] but acts on HeaderValues instead of bytes.
+/// Will ensure that the underlying bytes are valid UTF-8.
+fn normalize_header_value(header_value: &HeaderValue) -> Result<HeaderValue, Error> {
     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()
+    HeaderValue::from_str(std::str::from_utf8(&trimmed_value)?).map_err(Error::from)
 }
 
 #[derive(Debug, PartialEq, Default)]
@@ -738,9 +738,9 @@ mod tests {
         }
 
         #[test]
-        fn test_normalize_header_value_doesnt_panic(v in (".*")) {
+        fn test_normalize_header_value_works_on_valid_header_value(v in (".*")) {
             if let Ok(header_value) = HeaderValue::from_maybe_shared(v) {
-                let _ = normalize_header_value(&header_value);
+                assert!(normalize_header_value(&header_value).is_ok());
             }
         }
 
@@ -749,4 +749,10 @@ mod tests {
             assert_eq!(trim_all(s.as_bytes()).as_ref(), s.as_bytes());
         }
     }
+
+    #[test]
+    fn test_normalize_header_value_returns_expected_error_on_invalid_utf8() {
+        let header_value = HeaderValue::from_bytes(&[0xC0, 0xC1]).unwrap();
+        assert!(normalize_header_value(&header_value).is_err());
+    }
 }

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

@@ -311,6 +311,7 @@ mod tests {
     use crate::http_request::{SignatureLocation, SigningParams, SigningSettings};
     use http::{HeaderMap, HeaderValue};
     use pretty_assertions::assert_eq;
+    use proptest::proptest;
     use std::borrow::Cow;
     use std::time::Duration;
 
@@ -515,6 +516,62 @@ mod tests {
         assert_req_eq!(expected, signed);
     }
 
+    #[test]
+    fn test_sign_headers_returning_expected_error_on_invalid_utf8() {
+        let settings = SigningSettings::default();
+        let params = SigningParams {
+            access_key: "123",
+            secret_key: "asdf",
+            security_token: None,
+            region: "us-east-1",
+            service_name: "foo",
+            time: std::time::SystemTime::now(),
+            settings,
+        };
+
+        let req = http::Request::builder()
+            .uri("https://foo.com/")
+            .header("x-sign-me", HeaderValue::from_bytes(&[0xC0, 0xC1]).unwrap())
+            .body(&[])
+            .unwrap();
+
+        let creq = crate::http_request::sign(SignableRequest::from(&req), &params);
+        assert!(creq.is_err());
+    }
+
+    proptest! {
+        #[test]
+        // Only byte values between 32 and 255 (inclusive) are permitted, excluding byte 127, for
+        // [HeaderValue](https://docs.rs/http/latest/http/header/struct.HeaderValue.html#method.from_bytes).
+        fn test_sign_headers_no_panic(
+            left in proptest::collection::vec(32_u8..=126, 0..100),
+            right in proptest::collection::vec(128_u8..=255, 0..100),
+        ) {
+            let settings = SigningSettings::default();
+            let params = SigningParams {
+                access_key: "123",
+                secret_key: "asdf",
+                security_token: None,
+                region: "us-east-1",
+                service_name: "foo",
+                time: std::time::SystemTime::now(),
+                settings,
+            };
+
+            let bytes = left.iter().chain(right.iter()).cloned().collect::<Vec<_>>();
+            let req = http::Request::builder()
+                .uri("https://foo.com/")
+                .header("x-sign-me", HeaderValue::from_bytes(&bytes).unwrap())
+                .body(&[])
+                .unwrap();
+
+            // The test considered a pass if the creation of `creq` does not panic.
+            let _creq = crate::http_request::sign(
+                SignableRequest::from(&req),
+                &params);
+        }
+    }
+
     #[test]
     fn apply_signing_instructions_headers() {
         let mut headers = HeaderMap::new();