Unverified Commit e63b827a authored by Zelda Hessler's avatar Zelda Hessler Committed by GitHub
Browse files

Fix query string signing bug (#965) 

* update: use shared encoder definition when encoding url paths, query strings
add: query string writer test
add: changelog entry for bugfix

* format: test file

* fix: integration test
add: ", ^, `, \, (space), {, }, and | to list of chars to percent encode

* add: tracing and tracing-subscriber dep to generated integration tests

* add: canonical req query param test
remove: obsolete LABEL_SET ascii set

* fix: outdated aws-hyper import
parent c8e9f19d

CHANGELOG.next.toml

+9 −3
Original line number Diff line number Diff line
@@ -11,6 +11,12 @@ 
# meta = { "breaking" = false, "tada" = false, "bug" = false }

# author = "rcoh"



[[aws-sdk-rust]]

message = "A bug that occurred when signing certain query strings has been fixed"

references = ["aws-sdk-rust#330"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "Velfi"



[[aws-sdk-rust]]

message = "Fix incorrect argument order in the builder for `LazyCachingCredentialsProvider`"

references = ["smithy-rs#949"]
@@ -25,7 +31,7 @@ that ensures `rustls` is not in the dependency tree 
"""

references = ["aws-sdk-rust#304"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "zhessler"

author = "Velfi"



[[aws-sdk-rust]]

message = """
@@ -61,7 +67,7 @@ the formerly default features from those crates must now be explicitly set in yo 
'''

references = ["smithy-rs#930"]

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "zhessler"

author = "Velfi"



[[smithy-rs]]

message = '''
@@ -77,7 +83,7 @@ Runtime crates no longer have default features. You must now specify the feature 
'''

references = ["smithy-rs#930"]

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "zhessler"

author = "Velfi"



[[aws-sdk-rust]]

message = "Use provided `sleep_impl` for retries instead of using Tokio directly."

aws/rust-runtime/aws-sigv4/Cargo.toml

+1 −0
Original line number Diff line number Diff line
@@ -15,6 +15,7 @@ default = ["sign-http"] 


[dependencies]

aws-smithy-eventstream = { path = "../../../rust-runtime/aws-smithy-eventstream", optional = true }

aws-smithy-http = { path = "../../../rust-runtime/aws-smithy-http" }

bytes = { version = "1", optional = true }

form_urlencoded = { version = "1.0", optional = true }

hex = "0.4"

aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs

+24 −0
Original line number Diff line number Diff line
@@ -497,12 +497,14 @@ mod tests { 
    use crate::http_request::canonical_request::{

        normalize_header_value, trim_all, CanonicalRequest, SigningScope, StringToSign,

    };

    use crate::http_request::query_writer::QueryWriter;

    use crate::http_request::test::{test_canonical_request, test_request, test_sts};

    use crate::http_request::{

        PayloadChecksumKind, SignableBody, SignableRequest, SigningSettings,

    };

    use crate::http_request::{SignatureLocation, SigningParams};

    use crate::sign::sha256_hex_string;

    use http::Uri;

    use pretty_assertions::assert_eq;

    use proptest::{proptest, strategy::Strategy};

    use std::time::Duration;
@@ -650,6 +652,28 @@ mod tests { 
        );

    }



    #[test]

    fn test_signing_urls_with_percent_encoded_query_strings() {

        let all_printable_ascii_chars: String = (32u8..127).map(char::from).collect();

        let uri = Uri::from_static("https://s3.us-east-1.amazonaws.com/my-bucket");



        let mut query_writer = QueryWriter::new(&uri);

        query_writer.insert("list-type", "2");

        query_writer.insert("prefix", &all_printable_ascii_chars);



        let req = http::Request::builder()

            .uri(query_writer.build_uri())

            .body("")

            .unwrap();

        let req = SignableRequest::from(&req);

        let signing_params = signing_params(SigningSettings::default());

        let creq = CanonicalRequest::from(&req, &signing_params).unwrap();



        let expected = "list-type=2&prefix=%20%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F0123456789%3A%3B%3C%3D%3E%3F%40ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5C%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B%7C%7D~";

        let actual = creq.params.unwrap();

        assert_eq!(expected, actual);

    }



    // It should exclude user-agent, content-type, content-length, and x-amz-user-agent headers from presigning

    #[test]

    fn presigning_header_exclusion() {

aws/rust-runtime/aws-sigv4/src/http_request/query_writer.rs

+30 −0
Original line number Diff line number Diff line
@@ -124,6 +124,36 @@ mod test { 
        assert_eq!("key=val%25ue&ano%25ther=value", query_writer.build_query());

    }



    #[test]

    // This test ensures that the percent encoding applied to queries always produces a valid URI if

    // the starting URI is valid

    fn doesnt_panic_when_adding_query_to_valid_uri() {

        let uri = Uri::from_static("http://www.example.com");



        let mut problematic_chars = Vec::new();



        for byte in u8::MIN..=u8::MAX {

            match std::str::from_utf8(&[byte]) {

                // If we can't make a str from the byte then we certainly can't make a URL from it

                Err(_) => {

                    continue;

                }

                Ok(value) => {

                    let mut query_writer = QueryWriter::new(&uri);

                    query_writer.insert("key", value);



                    if let Err(_) = std::panic::catch_unwind(|| query_writer.build_uri()) {

                        problematic_chars.push(char::from(byte));

                    };

                }

            }

        }



        if !problematic_chars.is_empty() {

            panic!("we got some bad bytes here: {:#?}", problematic_chars)

        }

    }



    #[test]

    fn clear_params() {

        let uri = Uri::from_static("http://www.example.com/path?original=here&foo=1");

aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs

+3 −32
Original line number Diff line number Diff line
@@ -3,41 +3,12 @@ 
 * SPDX-License-Identifier: Apache-2.0.

 */



use percent_encoding::{AsciiSet, CONTROLS};



/// base set of characters that must be URL encoded

const BASE_SET: &AsciiSet = &CONTROLS

    .add(b' ')

    // RFC-3986 §3.3 allows sub-delims (defined in section2.2) to be in the path component.

    // This includes both colon ':' and comma ',' characters.

    // Smithy protocol tests & AWS services percent encode these expected values. Signing

    // will fail if these values are not percent encoded

    .add(b':')

    .add(b',')

    .add(b'?')

    .add(b'#')

    .add(b'[')

    .add(b']')

    .add(b'@')

    .add(b'!')

    .add(b'$')

    .add(b'&')

    .add(b'\'')

    .add(b'(')

    .add(b')')

    .add(b'*')

    .add(b'+')

    .add(b';')

    .add(b'=')

    .add(b'%');



const QUERY_SET: &AsciiSet = &BASE_SET.add(b'/');

const PATH_SET: &AsciiSet = BASE_SET;

use aws_smithy_http::{label, query};



pub(super) fn percent_encode_query(value: &str) -> String {

    percent_encoding::percent_encode(value.as_bytes(), QUERY_SET).to_string()

    query::fmt_string(value)

}



pub(super) fn percent_encode_path(value: &str) -> String {

    percent_encoding::percent_encode(value.as_bytes(), PATH_SET).to_string()

    label::fmt_string(value, true)

}