From 6c672c9da66f2b5ec01e613121ade0712a426aa6 Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Sat, 28 Jan 2023 01:21:32 -0800 Subject: [PATCH 1/4] Fix handling of repeated headers in aws request canonicalization See spec in https://docs.aws.amazon.com/general/latest/gr/create-signed-request.html#create-canonical-request Previously, repeated headers would appear twice on separate lines in the `CanonicalHeaders` section and twice in the `SignedHeaders` section. This PR fixes it to appear once. This should fix issues like this one https://github.com/awslabs/aws-sdk-rust/issues/720 --- .../src/http_request/canonical_request.rs | 64 ++++++++++++++++--- 1 file changed, 54 insertions(+), 10 deletions(-) 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..f4aeef04b0 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 @@ -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; @@ -338,14 +338,17 @@ impl<'a> fmt::Display for CanonicalRequest<'a> { // 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]; + let values: Vec<&str> = self + .headers + .get_all(&header.0) + .into_iter() + .map(|value| { + std::str::from_utf8(value.as_bytes()) + .expect("SDK request header values are valid UTF-8") + }) + .collect(); 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, "{}", values.join(","))?; } writeln!(f)?; // write out the signed headers @@ -538,6 +541,43 @@ 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" + ); + + let expected = "GET +/ +Param1=value1&Param2=value2 +host:example.amazonaws.com +x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 +x-amz-date:20210511T154045Z +x-amz-object-attributes:Checksum,ObjectSize + +host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes +e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; + assert_eq!(creq.to_string(), expected); + } + #[test] fn test_set_xamz_sha_256() { let req = test_request("get-vanilla-query-order-key-case"); @@ -548,6 +588,7 @@ mod tests { }; let mut signing_params = signing_params(settings); let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); + assert_eq!( creq.values.content_sha256(), "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" @@ -555,12 +596,15 @@ mod tests { // assert that the sha256 header was added assert_eq!( creq.values.signed_headers().as_str(), - "host;x-amz-content-sha256;x-amz-date" + "host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes;x-amz-object-attributes" ); signing_params.settings.payload_checksum_kind = PayloadChecksumKind::NoHeader; let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); - assert_eq!(creq.values.signed_headers().as_str(), "host;x-amz-date"); + assert_eq!( + creq.values.signed_headers().as_str(), + "host;x-amz-date;x-amz-object-attributes;x-amz-object-attributes" + ); } #[test] From 707c1d236dbfc621bd2853abb1b825478e17bd00 Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Sat, 28 Jan 2023 01:53:45 -0800 Subject: [PATCH 2/4] Update changelog.next.toml --- CHANGELOG.next.toml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 39f49ae2ed..08fca81185 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -56,3 +56,11 @@ let result = cache references = ["smithy-rs#2246"] meta = { "breaking" = false, "tada" = false, "bug" = false } author = "ysaito1001" + +[[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" From c5988b850f678de5a636d847a88bc327dbe9af21 Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Sat, 28 Jan 2023 01:55:54 -0800 Subject: [PATCH 3/4] Undo accidental extra change to test --- .../aws-sigv4/src/http_request/canonical_request.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 f4aeef04b0..8a30f37e81 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 @@ -588,7 +588,6 @@ e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; }; let mut signing_params = signing_params(settings); let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); - assert_eq!( creq.values.content_sha256(), "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" @@ -596,15 +595,12 @@ e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; // assert that the sha256 header was added assert_eq!( creq.values.signed_headers().as_str(), - "host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes;x-amz-object-attributes" + "host;x-amz-content-sha256;x-amz-date" ); signing_params.settings.payload_checksum_kind = PayloadChecksumKind::NoHeader; let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); - assert_eq!( - creq.values.signed_headers().as_str(), - "host;x-amz-date;x-amz-object-attributes;x-amz-object-attributes" - ); + assert_eq!(creq.values.signed_headers().as_str(), "host;x-amz-date"); } #[test] From c740a0e6887f61fce519a4e98df6e7713864d89c Mon Sep 17 00:00:00 2001 From: Nipunn Koorapati Date: Mon, 30 Jan 2023 19:54:00 -0800 Subject: [PATCH 4/4] Target test to just the line that is repeated --- .../src/http_request/canonical_request.rs | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) 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 8a30f37e81..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; @@ -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,18 +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 values: Vec<&str> = self - .headers - .get_all(&header.0) - .into_iter() - .map(|value| { - std::str::from_utf8(value.as_bytes()) - .expect("SDK request header values are valid UTF-8") - }) - .collect(); write!(f, "{}:", header.0.as_str())?; - writeln!(f, "{}", values.join(","))?; + writeln!(f, "{}", self.header_values_for(&header.0))?; } writeln!(f)?; // write out the signed headers @@ -564,18 +567,10 @@ mod tests { creq.values.signed_headers().to_string(), "host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes" ); - - let expected = "GET -/ -Param1=value1&Param2=value2 -host:example.amazonaws.com -x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 -x-amz-date:20210511T154045Z -x-amz-object-attributes:Checksum,ObjectSize - -host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes -e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; - assert_eq!(creq.to_string(), expected); + assert_eq!( + creq.header_values_for("x-amz-object-attributes"), + "Checksum,ObjectSize", + ); } #[test]