Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling of repeated headers in aws request canonicalization #2261

Merged
merged 7 commits into from
Feb 3, 2023
Merged

Fix handling of repeated headers in aws request canonicalization #2261

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6c672c9
Fix handling of repeated headers in aws request canonicalization
nipunn1313 Jan 28, 2023
707c1d2
Update changelog.next.toml
nipunn1313 Jan 28, 2023
c5988b8
Undo accidental extra change to test
nipunn1313 Jan 28, 2023
c740a0e
Target test to just the line that is repeated
nipunn1313 Jan 31, 2023
874d390
Merge branch 'main' into nipunn/repeated_header
jdisanti Feb 2, 2023
267da5e
Merge branch 'main' into nipunn/repeated_header
jdisanti Feb 3, 2023
445be37
Merge branch 'main' into nipunn/repeated_header
jdisanti Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.next.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
56 changes: 48 additions & 8 deletions aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change ensures that each key appears only once in the SignedHeaders section.

if let Some(excluded_headers) = params.settings.excluded_headers.as_ref() {
if excluded_headers.contains(name) {
continue;
Expand Down Expand Up @@ -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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change ensures that all values of the header appear in a comma separated list in the CanonicalHeaders section on the same row - per the spec.

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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the unit test! Here is my alternative thought, and feel free to incorporate/discard it. Instead of comparing the entire canonical request in the form of a raw string, could we target our verification on x-amz-object-attributes?

To do so, we need to factor out the code from 341 to 349 into a small method, i.e.

impl<'a> CanonicalRequest<'a> {
    // I just chose some name, but feel free to pick any name that makes sense.
    fn header_values_for(&self, key: impl AsHeaderName) -> 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()
    }
}

Lines 341-349 will then look like:

let values = self.header_values_for(&header.0);

In the unit test, we can use header_values_for on creq, i.e.,

let actual = creq.header_values_for("x-amz-object-attributes");
assert_eq!(actual, vec!["Checksum", "ObjectSize"]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable! Will do it.

I personally also like having a test that has the entire thing as well as a bit of an integration test - it'll be really easy to notice if something regresses about how the whole thing fits together. But of course, happy to defer to whatever maintainers think.

If we wanted that, it'd probably be better as a separate test that is very plain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll move the "," join into the small method so it also gets coverage in the test - similar to how we test the semicolon joining a few lines above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally also like having a test that has the entire thing as well as a bit of an integration test - it'll be really easy to notice if something regresses about how the whole thing fits together.

Thanks for thinking through. If anything goes wrong out of the implementation of this PR, it'll be caught in the existing integration tests (and probably easy to notice as failures should be across the board).

If we wanted that, it'd probably be better as a separate test that is very plain.

I agree and kind of thought that was the benefit of comparing the canonical request as a raw string in your original code.

/
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");
Expand Down