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.

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 aws-sigv4 canonical request formatting fallibility #711

Closed
jdisanti opened this issue Sep 23, 2021 · 1 comment
Closed

Fix aws-sigv4 canonical request formatting fallibility #711

jdisanti opened this issue Sep 23, 2021 · 1 comment
Labels
bug Something isn't working client good first issue Good for newcomers sdk-ga
Milestone

Comments

@jdisanti
Copy link
Collaborator

The aws-sigv4 crate currently implements the Display trait for formatting the canonical request, and since it's using ToString to do the actual formatting, it will panic if there's any error during that formatting.

In #710, we fixed a bug where header values that contained non-ASCII characters would cause a panic, but the problem was just punted a little bit since now it can panic if there's invalid UTF-8 in the header value.

Options we should evaluate and choose from:

  1. Refactor CanonicalRequest to have a fallible format function instead of implementing Display, and bubble the error up.
  2. Refactor CanonicalRequest to have an infallible format function, but produce bytes instead of a string, and put the header value's bytes directly into the formatted canonical request.
@jdisanti jdisanti changed the title Deep dive aws-sigv4 canonical request formatting fallibility Fix aws-sigv4 canonical request formatting fallibility Sep 23, 2021
@jdisanti jdisanti added bug Something isn't working good first issue Good for newcomers labels Sep 23, 2021
@rcoh rcoh added this to the GA milestone Dec 13, 2021
@Velfi
Copy link
Contributor

Velfi commented Aug 22, 2022

Russell wrote a (currently failing) test case for this issue:

#[test]
    fn test_signing_utf8_headers() {
        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),
            &SigningParams::builder()
                .region("us-east-1")
                .access_key("123")
                .service_name("foo")
                .secret_key("asdf")
                .time(SystemTime::now())
                .settings(SigningSettings::default())
                .build()
                .unwrap(),
        );
    }

ysaito1001 added a commit to ysaito1001/smithy-rs that referenced this issue Aug 23, 2022
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 smithy-lang#711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client good first issue Good for newcomers sdk-ga
Projects
None yet
Development

No branches or pull requests

3 participants