Fix handling of repeated headers in aws request canonicalization

[nipunn1313]

This PR fixes it to appear once.

This should fix issues like this one
awslabs/aws-sdk-rust#720

Motivation and Context

awslabs/aws-sdk-rust#720

Previously, repeated headers would appear twice on separate lines in the CanonicalHeaders section and twice in the SignedHeaders section, resulting in invalid signatures failing requests.

See spec in
https://docs.aws.amazon.com/general/latest/gr/create-signed-request.html#create-canonical-request

Description

Per the spec, the header should appear once with a comma separated list of values.

Testing

I added a test to show the output that is appearing.
I would love to actually test end-to-end with aws to make sure the final requests in fact works, but I'm actually not sure how. Would appreciate assistance (or if someone from amazon's side wants to take over the PR, that would be fine with me as well).

Checklist

• I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[nipunn1313]

I was actually able to test that this solves my original bug by running my company's app with this modification to Cargo.toml

-aws-config = "0.54.0"
-aws-sdk-lambda = "0.24.0"
-aws-sdk-s3 = "0.24.0"
-aws-types = "0.54.0"

+aws-config = { path = "/Users/nipunn/src/smithy-rs/aws/sdk/build/aws-sdk/sdk/aws-config" }
+aws-sdk-lambda = { path = "/Users/nipunn/src/smithy-rs/aws/sdk/build/aws-sdk/sdk/lambda" }
+aws-sdk-s3 = { path = "/Users/nipunn/src/smithy-rs/aws/sdk/build/aws-sdk/sdk/s3" }
+aws-types = { path = "/Users/nipunn/src/smithy-rs/aws/sdk/build/aws-sdk/sdk/aws-types" }

It does successfully talk to aws with the new correctly canonicalized headers.

[nipunn1313]

From my app:

Before the fix:

 2023-01-28T08:34:20.991Z TRACE aws_sigv4::http_request::sign > canonical_request=GET
/{redacted object key}
attributes=
host:test-convex-files.s3.us-east-1.amazonaws.com
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20230128T083420Z
x-amz-object-attributes:Checksum
x-amz-object-attributes:Checksum
x-amz-user-agent:aws-sdk-rust/0.54.1 api/s3/0.24.0 os/macos lang/rust/1.66.0-nightly

host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes;x-amz-object-attributes;x-amz-user-agent
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

including the following failure:

Error { code: "SignatureDoesNotMatch", message: "The request signature we calculated does not match the signature you provided. Check your key and signing method.", request_id: "6271CBPD71BZ7TS8", s3_extended_request_id: "zeMM9J2/9ey9RdtXUIzerhFnA0Z3X9RcgBRv2xqMKLIJqLYp2G9Cp2tlG+Iihur4tMZcWRZFelY=" }

After

 2023-01-28T09:45:07.418Z TRACE aws_sigv4::http_request::sign   > canonical_request=GET
/{redacted object key}
attributes=
host:test-convex-modules.s3.us-east-1.amazonaws.com
x-amz-content-sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
x-amz-date:20230128T094507Z
x-amz-object-attributes:Checksum,ObjectSize
x-amz-user-agent:aws-sdk-rust/0.54.1 api/s3/0.0.0-local os/macos lang/rust/1.66.0-nightly

host;x-amz-content-sha256;x-amz-date;x-amz-object-attributes;x-amz-user-agent
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

and success!

[nipunn1313]

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

[nipunn1313]

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.

[jdisanti]

Thank you for the contribution! These changes look good to me.

We're having some issues in CI with Error response from daemon: toomanyrequests: Data limit exceeded at the moment, so you'll see some failures from that. I'll check back and retry those in a bit so we can get a better view of CI.

[ysaito1001]

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"]);

[nipunn1313]

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.

[nipunn1313]

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.

[ysaito1001]

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.

[ysaito1001]

LGTM! Thanks for incorporating the feedback.

[nipunn1313]

Sounds good! Let me know if there's anything further you need from me. Feel free to do any rebasing/merge conflicts/minor fixes etc on my behalf if that's what's blocking.

[jdisanti]

I think we're ready to merge. Just waiting for resolution on our CI issues to avoid having to restart the tests a bunch of times 😅