-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
Fix handling of repeated headers in aws request canonicalization #2261
Changes from 3 commits
6c672c9
707c1d2
c5988b8
c740a0e
874d390
267da5e
445be37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 To do so, we need to factor out the code from 341 to 349 into a small method, i.e.
Lines 341-349 will then look like:
In the unit test, we can use
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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).
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"); | ||
|
There was a problem hiding this comment.
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.