Unverified Commit 521fa8d0 authored by ysaito1001's avatar ysaito1001 Committed by GitHub
Browse files

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: default avatarRussell Cohen <rcoh@amazon.com>
Co-authored-by: default avatarSaito <awsaito@c889f3b5ddc4.ant.amazon.com>
parent 374a0a25

CHANGELOG.next.toml

+6 −0
Original line number Diff line number Diff line
@@ -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

+13 −7
Original line number Diff line number Diff line
@@ -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

+57 −0
Original line number Diff line number Diff line
@@ -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();