diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 8b9835ba69f86a2a2acc9c9700691e17687da6e7..764553b100aced899a07d1c4c6103003aeaf7e40 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -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." diff --git a/aws/rust-runtime/aws-sigv4/Cargo.toml b/aws/rust-runtime/aws-sigv4/Cargo.toml index ef421c065abf82715a766c1280582e4e98bc23c3..6ab18fa83438852bccd73c803e189648bcc79c94 100644 --- a/aws/rust-runtime/aws-sigv4/Cargo.toml +++ b/aws/rust-runtime/aws-sigv4/Cargo.toml @@ -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" diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs b/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs index 8a09b843b078a3ae174b0c499303afcec36f865c..c32bb42815d46aba5ce01932e705ee7f0c2772d4 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/canonical_request.rs @@ -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() { diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/query_writer.rs b/aws/rust-runtime/aws-sigv4/src/http_request/query_writer.rs index 87b8e89d42144dc254125f056fe07d0453dd788b..d67891c44cb576e195ddcedfd10864fc276431d9 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/query_writer.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/query_writer.rs @@ -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"); diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs b/aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs index 6f4cce6d92a4d979356192e47cbb8b7778280d2b..a377039bba4d66117a2f8f1c385bc2a8ceeba38a 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/url_escape.rs @@ -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) } diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/IntegrationTestDependencies.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/IntegrationTestDependencies.kt index 9721b8263a29202213761fe30529df7d8a734759..fd21cee43e826fbc003fce75b1a711cb713db876 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/IntegrationTestDependencies.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/IntegrationTestDependencies.kt @@ -63,6 +63,8 @@ class IntegrationTestDependencies( addDependency(SerdeJson) addDependency(Tokio) addDependency(FuturesUtil) + addDependency(Tracing) + addDependency(TracingSubscriber) } if (hasBenches) { addDependency(Criterion) @@ -95,3 +97,5 @@ private val Hound = CargoDependency("hound", CratesIo("3.4"), DependencyScope.De private val SerdeJson = CargoDependency("serde_json", CratesIo("1"), features = emptySet(), scope = DependencyScope.Dev) private val Tokio = CargoDependency("tokio", CratesIo("1"), features = setOf("macros", "test-util"), scope = DependencyScope.Dev) private val FuturesUtil = CargoDependency("futures-util", CratesIo("0.3"), scope = DependencyScope.Dev) +private val Tracing = CargoDependency("tracing", CratesIo("0.1"), scope = DependencyScope.Dev) +private val TracingSubscriber = CargoDependency("tracing-subscriber", CratesIo("0.2"), scope = DependencyScope.Dev) diff --git a/aws/sdk/integration-tests/s3/Cargo.toml b/aws/sdk/integration-tests/s3/Cargo.toml index 818ec3a24c25f843ff04dc4b47c64c15a8d64e26..a21c7b4b6817f422955059d63bf2eee30d9a2a53 100644 --- a/aws/sdk/integration-tests/s3/Cargo.toml +++ b/aws/sdk/integration-tests/s3/Cargo.toml @@ -8,17 +8,19 @@ edition = "2018" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dev-dependencies] +aws-config = { path = "../../build/aws-sdk/sdk/aws-config" } aws-http = { path = "../../build/aws-sdk/sdk/aws-http" } aws-sdk-s3 = { path = "../../build/aws-sdk/sdk/s3" } +aws-sdk-sts = { path = "../../build/aws-sdk/sdk/sts" } aws-smithy-async = { path = "../../build/aws-sdk/sdk/aws-smithy-async", features = ["rt-tokio"] } aws-smithy-client = { path = "../../build/aws-sdk/sdk/aws-smithy-client", features = ["test-util", "rustls"] } aws-smithy-http = { path = "../../build/aws-sdk/sdk/aws-smithy-http" } -aws-smithy-types = { path = "../../build/aws-sdk/sdk/aws-smithy-types" } aws-smithy-protocol-test = { path = "../../build/aws-sdk/sdk/aws-smithy-protocol-test" } +aws-smithy-types = { path = "../../build/aws-sdk/sdk/aws-smithy-types" } bytes = "1" http = "0.2.3" serde_json = "1" tokio = { version = "1", features = ["full", "test-util"] } -tracing-subscriber = "0.2.18" -aws-config = "0.2.0" +tracing-subscriber = "0.3" +tracing = "0.1" tracing-test = "0.2.1" diff --git a/aws/sdk/integration-tests/s3/tests/query-strings-are-correctly-encoded.rs b/aws/sdk/integration-tests/s3/tests/query-strings-are-correctly-encoded.rs new file mode 100644 index 0000000000000000000000000000000000000000..e6d484c09cb8116f789fee806d2db41585f1081e --- /dev/null +++ b/aws/sdk/integration-tests/s3/tests/query-strings-are-correctly-encoded.rs @@ -0,0 +1,168 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0. + */ + +use aws_http::user_agent::AwsUserAgent; +use aws_sdk_s3::middleware::DefaultMiddleware; +use aws_sdk_s3::operation::ListObjectsV2; +use aws_sdk_s3::{Credentials, Region}; +use aws_smithy_client::test_connection::capture_request; +use aws_smithy_client::Client as CoreClient; +use std::time::{Duration, UNIX_EPOCH}; + +pub type Client = CoreClient; + +#[tokio::test] +async fn test_s3_signer_query_string_with_all_valid_chars() -> Result<(), aws_sdk_s3::Error> { + let creds = Credentials::new( + "ANOTREAL", + "notrealrnrELgWzOk3IfjzDKtFBhDby", + Some("notarealsessiontoken".to_string()), + None, + "test", + ); + let conf = aws_sdk_s3::Config::builder() + .credentials_provider(creds) + .region(Region::new("us-east-1")) + .build(); + let (conn, rcvr) = capture_request(None); + + let client = Client::new(conn.clone()); + + // Generate a string containing all printable ASCII chars + let prefix: String = (32u8..127).map(char::from).collect(); + + let mut op = ListObjectsV2::builder() + .bucket("test-bucket") + .prefix(&prefix) + .build() + .unwrap() + .make_operation(&conf) + .await + .expect("failed to construct operation"); + op.properties_mut() + .insert(UNIX_EPOCH + Duration::from_secs(1624036048)); + op.properties_mut().insert(AwsUserAgent::for_tests()); + + // The response from the fake connection won't return the expected XML but we don't care about + // that error in this test + let _ = client.call(op).await; + + let expected_req = rcvr.expect_request(); + let auth_header = expected_req + .headers() + .get("Authorization") + .unwrap() + .to_owned(); + + // This is a snapshot test taken from a known working test result + let snapshot_signature = + "Signature=775f88213304a5233ff295f869571554140e3db171a2d4a64f63902c49f79880"; + assert!( + auth_header + .to_str() + .unwrap() + .contains(snapshot_signature), + "authorization header signature did not match expected signature: got {}, expected it to contain {}", + auth_header.to_str().unwrap(), + snapshot_signature + ); + + Ok(()) +} + +// This test can help identify individual characters that break the signing of query strings. This +// test must be run against an actual bucket so we `ignore` it unless the runner specifically requests it +#[tokio::test] +#[ignore] +async fn test_query_strings_are_correctly_encoded() -> Result<(), aws_sdk_s3::Error> { + use aws_sdk_s3::error::{ListObjectsV2Error, ListObjectsV2ErrorKind}; + use aws_smithy_http::result::SdkError; + + tracing_subscriber::fmt::init(); + let config = aws_config::load_from_env().await; + let client = aws_sdk_s3::Client::new(&config); + + let mut chars_that_break_signing = Vec::new(); + let mut chars_that_break_uri_parsing = Vec::new(); + let mut chars_that_are_invalid_arguments = Vec::new(); + + // We test all possible bytes to check for issues with URL construction or signing + for byte in u8::MIN..u8::MAX { + let char = char::from(byte); + let res = client + .list_objects_v2() + .bucket("telephone-game") + .prefix(char) + .send() + .await; + if let Err(SdkError::ServiceError { + err: ListObjectsV2Error { kind, .. }, + .. + }) = res + { + match kind { + ListObjectsV2ErrorKind::Unhandled(e) + if e.to_string().contains("SignatureDoesNotMatch") => + { + chars_that_break_signing.push(byte); + } + ListObjectsV2ErrorKind::Unhandled(e) if e.to_string().contains("InvalidUri") => { + chars_that_break_uri_parsing.push(byte); + } + ListObjectsV2ErrorKind::Unhandled(e) + if e.to_string().contains("InvalidArgument") => + { + chars_that_are_invalid_arguments.push(byte); + } + ListObjectsV2ErrorKind::Unhandled(e) if e.to_string().contains("InvalidToken") => { + panic!("refresh your credentials and run this test again"); + } + e => todo!("unexpected error: {:?}", e), + } + } + } + + if chars_that_break_signing.is_empty() + && chars_that_break_uri_parsing.is_empty() + && chars_that_are_invalid_arguments.is_empty() + { + Ok(()) + } else { + fn char_transform(c: u8) -> String { + format!("byte {}: {}\n", c, char::from(c)) + } + if !chars_that_break_signing.is_empty() { + tracing::error!( + "The following characters caused a signature mismatch:\n{}(end)", + chars_that_break_signing + .clone() + .into_iter() + .map(char_transform) + .collect::() + ); + } + if !chars_that_break_uri_parsing.is_empty() { + tracing::error!( + "The following characters caused a URI parse failure:\n{}(end)", + chars_that_break_uri_parsing + .clone() + .into_iter() + .map(char_transform) + .collect::() + ); + } + if !chars_that_are_invalid_arguments.is_empty() { + tracing::error!( + "The following characters caused an \"Invalid Argument\" failure:\n{}(end)", + chars_that_are_invalid_arguments + .clone() + .into_iter() + .map(char_transform) + .collect::() + ); + } + panic!("test failed, see logs for the problem chars") + } +} diff --git a/rust-runtime/aws-smithy-http/src/label.rs b/rust-runtime/aws-smithy-http/src/label.rs index 6d1065a6b61b15506a74bf835ffe3a644ee6984a..9d5bab5c3ea45ff0e02f831edffb4409c15dce28 100644 --- a/rust-runtime/aws-smithy-http/src/label.rs +++ b/rust-runtime/aws-smithy-http/src/label.rs @@ -6,15 +6,15 @@ //! Formatting values as Smithy //! [httpLabel](https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httplabel-trait) -use crate::urlencode::LABEL_SET; +use crate::urlencode::BASE_SET; use aws_smithy_types::date_time::{DateTimeFormatError, Format}; use aws_smithy_types::DateTime; use percent_encoding::AsciiSet; -const GREEDY: &AsciiSet = &LABEL_SET.remove(b'/'); +const GREEDY: &AsciiSet = &BASE_SET.remove(b'/'); pub fn fmt_string>(t: T, greedy: bool) -> String { - let uri_set = if greedy { GREEDY } else { LABEL_SET }; + let uri_set = if greedy { GREEDY } else { BASE_SET }; percent_encoding::utf8_percent_encode(t.as_ref(), uri_set).to_string() } diff --git a/rust-runtime/aws-smithy-http/src/urlencode.rs b/rust-runtime/aws-smithy-http/src/urlencode.rs index e41f8da1f7ef347f7892613c2eb23df6c1f80192..f14c5901d6f4e8d2a567260460fa9bdb6ddb122a 100644 --- a/rust-runtime/aws-smithy-http/src/urlencode.rs +++ b/rust-runtime/aws-smithy-http/src/urlencode.rs @@ -19,6 +19,9 @@ pub const BASE_SET: &AsciiSet = &CONTROLS .add(b'#') .add(b'[') .add(b']') + .add(b'{') + .add(b'}') + .add(b'|') .add(b'@') .add(b'!') .add(b'$') @@ -33,9 +36,10 @@ pub const BASE_SET: &AsciiSet = &CONTROLS .add(b'%') .add(b'<') .add(b'>') - .add(b'"'); - -pub const LABEL_SET: &AsciiSet = &BASE_SET.add(b'`').add(b'?').add(b' '); + .add(b'"') + .add(b'^') + .add(b'`') + .add(b'\\'); #[cfg(test)] mod test {