From 2d61502221d446695f6a0ff9799352ab38ad31cd Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Wed, 16 Aug 2023 16:28:32 -0400 Subject: [PATCH] Remove the public HTTP dependency from aws-sigv4 (#2921) ## Motivation and Context Removes the public http dependency from the aws-sigv4 crate to avoid compatibility issues with http = 1.0 and to support the http refactor ## Description - Changes `SignableRequest::new` to remove the direct exposure of HTTP types - Assorted test refactorings as a result - Update calling code ## Testing IT/UT ## Checklist TODO: changelog - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS SDK, generated SDK code, or SDK 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._ --- CHANGELOG.next.toml | 11 + aws/rust-runtime/aws-runtime/Cargo.toml | 3 +- .../aws-runtime/src/auth/sigv4.rs | 15 +- aws/rust-runtime/aws-sig-auth/Cargo.toml | 3 +- aws/rust-runtime/aws-sig-auth/src/signer.rs | 15 +- aws/rust-runtime/aws-sigv4/Cargo.toml | 1 + .../aws-sigv4/external-types.toml | 7 +- .../src/http_request/canonical_request.rs | 81 ++-- .../aws-sigv4/src/http_request/error.rs | 12 + .../aws-sigv4/src/http_request/mod.rs | 22 +- .../aws-sigv4/src/http_request/settings.rs | 11 +- .../aws-sigv4/src/http_request/sign.rs | 346 ++++++++---------- .../aws-sigv4/src/http_request/test.rs | 126 +++++-- 13 files changed, 358 insertions(+), 295 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index e0ea56e68..e7158b599 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -34,3 +34,14 @@ message = "Fix requests to S3 with `no_credentials` set." references = ["smithy-rs#2907", "aws-sdk-rust#864"] meta = { "breaking" = false, "tada" = false, "bug" = true } author = "jdisanti" + +[[aws-sdk-rust]] +message = """Several breaking changes were made to the aws-sigv4 API to remove the direct HTTP dependency: +- The `take_parameters` and `take_headers` APIs were removed from `SigningInstructions`. Use `into_parts()` instead +- The arguments of `SignableRequest::new` were changed to accept string types instead of types from the HTTP crate +- `SigningInstructions::apply_to_request` was gated beyond an `http0-compat` feature flag for backwards compatibility. This API MAY be removed in a future release. +- Several public accessors were removed from `SigningInstructions`. +""" +references = ["smithy-rs#2921"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "rcoh" diff --git a/aws/rust-runtime/aws-runtime/Cargo.toml b/aws/rust-runtime/aws-runtime/Cargo.toml index fddff0c9f..b6c47352d 100644 --- a/aws/rust-runtime/aws-runtime/Cargo.toml +++ b/aws/rust-runtime/aws-runtime/Cargo.toml @@ -14,7 +14,8 @@ test-util = [] [dependencies] aws-credential-types = { path = "../aws-credential-types" } aws-http = { path = "../aws-http" } -aws-sigv4 = { path = "../aws-sigv4" } +# TODO(httpRefactor): Remove the http0-compat feature +aws-sigv4 = { path = "../aws-sigv4", features = ["http0-compat"] } aws-smithy-async = { path = "../../../rust-runtime/aws-smithy-async" } aws-smithy-eventstream = { path = "../../../rust-runtime/aws-smithy-eventstream", optional = true } aws-smithy-http = { path = "../../../rust-runtime/aws-smithy-http" } diff --git a/aws/rust-runtime/aws-runtime/src/auth/sigv4.rs b/aws/rust-runtime/aws-runtime/src/auth/sigv4.rs index ac1eab1ea..0ef19307e 100644 --- a/aws/rust-runtime/aws-runtime/src/auth/sigv4.rs +++ b/aws/rust-runtime/aws-runtime/src/auth/sigv4.rs @@ -356,11 +356,17 @@ impl Signer for SigV4Signer { }); let signable_request = SignableRequest::new( - request.method(), - request.uri(), - request.headers(), + request.method().as_str(), + request.uri().to_string(), + request.headers().iter().map(|(k, v)| { + ( + k.as_str(), + // use from_utf8 instead of to_str because we _do_ allow non-ascii header values + std::str::from_utf8(v.as_bytes()).expect("only utf-8 headers are signable"), + ) + }), signable_body, - ); + )?; sign(signable_request, &signing_params)? } .into_parts(); @@ -384,7 +390,6 @@ impl Signer for SigV4Signer { .expect("failed to send deferred signer"); } } - signing_instructions.apply_to_request(request); Ok(()) } diff --git a/aws/rust-runtime/aws-sig-auth/Cargo.toml b/aws/rust-runtime/aws-sig-auth/Cargo.toml index deece1ac3..c5192d8aa 100644 --- a/aws/rust-runtime/aws-sig-auth/Cargo.toml +++ b/aws/rust-runtime/aws-sig-auth/Cargo.toml @@ -12,7 +12,8 @@ sign-eventstream = ["aws-smithy-eventstream", "aws-sigv4/sign-eventstream"] [dependencies] aws-credential-types = { path = "../aws-credential-types" } -aws-sigv4 = { path = "../aws-sigv4" } +# TODO(httpRefactor): Remove feature was http refactor is complete +aws-sigv4 = { path = "../aws-sigv4", features = ["http0-compat"] } aws-smithy-eventstream = { path = "../../../rust-runtime/aws-smithy-eventstream", optional = true } aws-smithy-http = { path = "../../../rust-runtime/aws-smithy-http" } aws-smithy-async = { path = "../../../rust-runtime/aws-smithy-async" } diff --git a/aws/rust-runtime/aws-sig-auth/src/signer.rs b/aws/rust-runtime/aws-sig-auth/src/signer.rs index d71c6ecf4..7542dc12d 100644 --- a/aws/rust-runtime/aws-sig-auth/src/signer.rs +++ b/aws/rust-runtime/aws-sig-auth/src/signer.rs @@ -16,6 +16,7 @@ use std::time::{Duration, SystemTime}; use crate::middleware::Signature; pub use aws_sigv4::http_request::SignableBody; + pub type SigningError = aws_sigv4::http_request::SigningError; const EXPIRATION_WARNING: &str = "Presigned request will expire before the given \ @@ -212,11 +213,17 @@ impl SigV4Signer { }); let signable_request = SignableRequest::new( - request.method(), - request.uri(), - request.headers(), + request.method().as_str(), + request.uri().to_string(), + request.headers().iter().map(|(k, v)| { + ( + k.as_str(), + std::str::from_utf8(v.as_bytes()) + .expect("only string headers are signable"), + ) + }), signable_body, - ); + )?; sign(signable_request, &signing_params)? } .into_parts(); diff --git a/aws/rust-runtime/aws-sigv4/Cargo.toml b/aws/rust-runtime/aws-sigv4/Cargo.toml index ea5402555..3a8b59022 100644 --- a/aws/rust-runtime/aws-sigv4/Cargo.toml +++ b/aws/rust-runtime/aws-sigv4/Cargo.toml @@ -11,6 +11,7 @@ repository = "https://github.com/awslabs/smithy-rs" [features] sign-http = ["http", "percent-encoding", "form_urlencoded"] sign-eventstream = ["aws-smithy-eventstream", "bytes"] +http0-compat = ["http"] default = ["sign-http"] [dependencies] diff --git a/aws/rust-runtime/aws-sigv4/external-types.toml b/aws/rust-runtime/aws-sigv4/external-types.toml index 9139940c2..0e56bc569 100644 --- a/aws/rust-runtime/aws-sigv4/external-types.toml +++ b/aws/rust-runtime/aws-sigv4/external-types.toml @@ -1,11 +1,6 @@ allowed_external_types = [ - "http::header::map::HeaderMap", - "http::header::name::HeaderName", - "http::header::value::HeaderValue", - "http::method::Method", + # TODO(refactorHttp): Remove this and remove the signing helpers "http::request::Request", - "http::uri::Uri", - # TODO(https://github.com/awslabs/smithy-rs/issues/1193): Once tooling permits it, only allow the following types in the `event-stream` feature "aws_smithy_eventstream::frame::Message", ] 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 221463ada..a16a7d412 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 @@ -15,7 +15,7 @@ use crate::http_request::{PayloadChecksumKind, SignableBody, SignatureLocation, use crate::sign::sha256_hex_string; use aws_smithy_http::query_writer::QueryWriter; use http::header::{AsHeaderName, HeaderName, HOST}; -use http::{HeaderMap, HeaderValue, Method, Uri}; +use http::{HeaderMap, HeaderValue, Uri}; use std::borrow::Cow; use std::cmp::Ordering; use std::convert::TryFrom; @@ -103,7 +103,7 @@ impl<'a> SignatureValues<'a> { #[derive(Debug, PartialEq)] pub(super) struct CanonicalRequest<'a> { - pub(super) method: &'a Method, + pub(super) method: &'a str, pub(super) path: Cow<'a, str>, pub(super) params: Option, pub(super) headers: HeaderMap, @@ -212,7 +212,7 @@ impl<'a> CanonicalRequest<'a> { // Header names and values need to be normalized according to Step 4 of https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html // Using append instead of insert means this will not clobber headers that have the same lowercased name canonical_headers.append( - HeaderName::from_str(&name.as_str().to_lowercase())?, + HeaderName::from_str(&name.to_lowercase())?, normalize_header_value(value)?, ); } @@ -237,7 +237,7 @@ impl<'a> CanonicalRequest<'a> { let mut signed_headers = Vec::with_capacity(canonical_headers.len()); for name in canonical_headers.keys() { if let Some(excluded_headers) = params.settings.excluded_headers.as_ref() { - if excluded_headers.contains(name) { + if excluded_headers.iter().any(|it| name.as_str() == it) { continue; } } @@ -406,9 +406,7 @@ fn trim_spaces_from_byte_string(bytes: &[u8]) -> &[u8] { /// Works just like [trim_all] but acts on HeaderValues instead of bytes. /// Will ensure that the underlying bytes are valid UTF-8. -fn normalize_header_value( - header_value: &HeaderValue, -) -> Result { +fn normalize_header_value(header_value: &str) -> Result { let trimmed_value = trim_all(header_value.as_bytes()); HeaderValue::from_str( std::str::from_utf8(&trimmed_value) @@ -544,8 +542,8 @@ mod tests { use crate::http_request::{SignatureLocation, SigningParams}; use crate::sign::sha256_hex_string; use aws_smithy_http::query_writer::QueryWriter; - use http::Uri; - use http::{header::HeaderName, HeaderValue}; + use http::{HeaderValue, Uri}; + use pretty_assertions::assert_eq; use proptest::{prelude::*, proptest}; use std::time::Duration; @@ -565,14 +563,14 @@ 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"), - ); + req.headers.push(( + "x-amz-object-attributes".to_string(), + "Checksum".to_string(), + )); + req.headers.push(( + "x-amz-object-attributes".to_string(), + "ObjectSize".to_string(), + )); let req = SignableRequest::from(&req); let settings = SigningSettings { payload_checksum_kind: PayloadChecksumKind::XAmzSha256, @@ -618,13 +616,10 @@ mod tests { #[test] fn test_unsigned_payload() { - let req = test_request("get-vanilla-query-order-key-case"); - let req = SignableRequest::new( - req.method(), - req.uri(), - req.headers(), - SignableBody::UnsignedPayload, - ); + let mut req = test_request("get-vanilla-query-order-key-case"); + req.set_body(SignableBody::UnsignedPayload); + let req: SignableRequest<'_> = SignableRequest::from(&req); + let settings = SigningSettings { payload_checksum_kind: PayloadChecksumKind::XAmzSha256, ..Default::default() @@ -638,13 +633,9 @@ mod tests { #[test] fn test_precomputed_payload() { let payload_hash = "44ce7dd67c959e0d3524ffac1771dfbba87d2b6b4b4e99e42034a8b803f8b072"; - let req = test_request("get-vanilla-query-order-key-case"); - let req = SignableRequest::new( - req.method(), - req.uri(), - req.headers(), - SignableBody::Precomputed(String::from(payload_hash)), - ); + let mut req = test_request("get-vanilla-query-order-key-case"); + req.set_body(SignableBody::Precomputed(String::from(payload_hash))); + let req = SignableRequest::from(&req); let settings = SigningSettings { payload_checksum_kind: PayloadChecksumKind::XAmzSha256, ..Default::default() @@ -712,7 +703,7 @@ mod tests { #[test] fn test_tilde_in_uri() { let req = http::Request::builder() - .uri("https://s3.us-east-1.amazonaws.com/my-bucket?list-type=2&prefix=~objprefix&single&k=&unreserved=-_.~").body("").unwrap(); + .uri("https://s3.us-east-1.amazonaws.com/my-bucket?list-type=2&prefix=~objprefix&single&k=&unreserved=-_.~").body("").unwrap().into(); let req = SignableRequest::from(&req); let signing_params = signing_params(SigningSettings::default()); let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); @@ -734,7 +725,8 @@ mod tests { let req = http::Request::builder() .uri(query_writer.build_uri()) .body("") - .unwrap(); + .unwrap() + .into(); let req = SignableRequest::from(&req); let signing_params = signing_params(SigningSettings::default()); let creq = CanonicalRequest::from(&req, &signing_params).unwrap(); @@ -786,7 +778,8 @@ mod tests { .header("x-amzn-trace-id", "test-trace-id") .header("x-amz-user-agent", "test-user-agent") .body("") - .unwrap(); + .unwrap() + .into(); let request = SignableRequest::from(&request); let settings = SigningSettings { @@ -816,7 +809,8 @@ mod tests { .header("x-amzn-trace-id", "test-trace-id") .header("x-amz-user-agent", "test-user-agent") .body("") - .unwrap(); + .unwrap() + .into(); let request = SignableRequest::from(&request); let settings = SigningSettings { @@ -847,7 +841,7 @@ mod tests { for key in &excluded_headers { request_builder = request_builder.header(key, "value"); } - let request = request_builder.body("").unwrap(); + let request = request_builder.body("").unwrap().into(); let request = SignableRequest::from(&request); @@ -857,9 +851,7 @@ mod tests { excluded_headers: Some( excluded_headers .into_iter() - .map(|header_string| { - HeaderName::from_static(Box::leak(header_string.into_boxed_str())) - }) + .map(std::borrow::Cow::Owned) .collect(), ), ..Default::default() @@ -908,9 +900,8 @@ mod tests { #[test] fn test_normalize_header_value_works_on_valid_header_value(v in (".*")) { - if let Ok(header_value) = HeaderValue::from_maybe_shared(v) { - assert!(normalize_header_value(&header_value).is_ok()); - } + prop_assume!(HeaderValue::from_str(&v).is_ok()); + assert!(normalize_header_value(&v).is_ok()); } #[test] @@ -918,10 +909,4 @@ mod tests { assert_eq!(trim_all(s.as_bytes()).as_ref(), s.as_bytes()); } } - - #[test] - fn test_normalize_header_value_returns_expected_error_on_invalid_utf8() { - let header_value = HeaderValue::from_bytes(&[0xC0, 0xC1]).unwrap(); - assert!(normalize_header_value(&header_value).is_err()); - } } diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/error.rs b/aws/rust-runtime/aws-sigv4/src/http_request/error.rs index d67acbc32..0d5f1b5b7 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/error.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/error.rs @@ -4,6 +4,7 @@ */ use http::header::{InvalidHeaderName, InvalidHeaderValue}; +use http::uri::InvalidUri; use std::error::Error; use std::fmt; use std::str::Utf8Error; @@ -50,6 +51,7 @@ enum CanonicalRequestErrorKind { InvalidHeaderName { source: InvalidHeaderName }, InvalidHeaderValue { source: InvalidHeaderValue }, InvalidUtf8InHeaderValue { source: Utf8Error }, + InvalidUri { source: InvalidUri }, } #[derive(Debug)] @@ -64,6 +66,7 @@ impl fmt::Display for CanonicalRequestError { InvalidHeaderName { .. } => write!(f, "invalid header name"), InvalidHeaderValue { .. } => write!(f, "invalid header value"), InvalidUtf8InHeaderValue { .. } => write!(f, "invalid UTF-8 in header value"), + InvalidUri { .. } => write!(f, "the uri was invalid"), } } } @@ -75,6 +78,7 @@ impl Error for CanonicalRequestError { InvalidHeaderName { source } => Some(source), InvalidHeaderValue { source } => Some(source), InvalidUtf8InHeaderValue { source } => Some(source), + InvalidUri { source } => Some(source), } } } @@ -102,3 +106,11 @@ impl From for CanonicalRequestError { } } } + +impl From for CanonicalRequestError { + fn from(source: InvalidUri) -> Self { + Self { + kind: CanonicalRequestErrorKind::InvalidUri { source }, + } + } +} diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/mod.rs b/aws/rust-runtime/aws-sigv4/src/http_request/mod.rs index db021bbee..d9fe5b4c8 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/mod.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/mod.rs @@ -7,18 +7,16 @@ //! //! # Example: Signing an HTTP request //! +//! **Note**: This requires `http0-compat` to be enabled. +//! //! ```rust -//! # fn test() -> Result<(), aws_sigv4::http_request::SigningError> { +//! # use aws_sigv4::http_request::SignableBody; +//! #[cfg(feature = "http0-compat")] +//! fn test() -> Result<(), aws_sigv4::http_request::SigningError> { //! use aws_sigv4::http_request::{sign, SigningSettings, SigningParams, SignableRequest}; //! use http; //! use std::time::SystemTime; //! -//! // Create the request to sign -//! let mut request = http::Request::builder() -//! .uri("https://some-endpoint.some-region.amazonaws.com") -//! .body("") -//! .unwrap(); -//! //! // Set up information and settings for the signing //! let signing_settings = SigningSettings::default(); //! let signing_params = SigningParams::builder() @@ -31,11 +29,17 @@ //! .build() //! .unwrap(); //! // Convert the HTTP request into a signable request -//! let signable_request = SignableRequest::from(&request); +//! let signable_request = SignableRequest::new( +//! "GET", +//! "https://some-endpoint.some-region.amazonaws.com", +//! std::iter::empty(), +//! SignableBody::Bytes(&[]) +//! ).expect("signable request"); //! +//! let mut my_req = http::Request::new("..."); //! // Sign and then apply the signature to the request //! let (signing_instructions, _signature) = sign(signable_request, &signing_params)?.into_parts(); -//! signing_instructions.apply_to_request(&mut request); +//! signing_instructions.apply_to_request(&mut my_req); //! # Ok(()) //! # } //! ``` diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/settings.rs b/aws/rust-runtime/aws-sigv4/src/http_request/settings.rs index 501cd5c77..8349c4285 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/settings.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/settings.rs @@ -3,7 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -use http::header::{HeaderName, AUTHORIZATION, USER_AGENT}; +use http::header::{AUTHORIZATION, USER_AGENT}; +use std::borrow::Cow; use std::time::Duration; /// HTTP signing parameters @@ -30,7 +31,7 @@ pub struct SigningSettings { pub expires_in: Option, /// Headers that should be excluded from the signing process - pub excluded_headers: Option>, + pub excluded_headers: Option>>, /// Specifies whether the absolute path component of the URI should be normalized during signing. pub uri_path_normalization_mode: UriPathNormalizationMode, @@ -109,11 +110,11 @@ impl Default for SigningSettings { let excluded_headers = Some( [ // This header is calculated as part of the signing process, so if it's present, discard it - AUTHORIZATION, + Cow::Borrowed(AUTHORIZATION.as_str()), // Changes when sent by proxy - USER_AGENT, + Cow::Borrowed(USER_AGENT.as_str()), // Changes based on the request from the client - HeaderName::from_static(HEADER_NAME_X_RAY_TRACE_ID), + Cow::Borrowed(HEADER_NAME_X_RAY_TRACE_ID), ] .to_vec(), ); diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/sign.rs b/aws/rust-runtime/aws-sigv4/src/http_request/sign.rs index aaeda06bb..2c6da9d12 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/sign.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/sign.rs @@ -8,56 +8,61 @@ use super::{PayloadChecksumKind, SignatureLocation}; use crate::http_request::canonical_request::header; use crate::http_request::canonical_request::param; use crate::http_request::canonical_request::{CanonicalRequest, StringToSign, HMAC_256}; +use crate::http_request::error::CanonicalRequestError; use crate::http_request::SigningParams; use crate::sign::{calculate_signature, generate_signing_key, sha256_hex_string}; use crate::SigningOutput; -use aws_smithy_http::query_writer::QueryWriter; -use http::header::HeaderValue; -use http::{HeaderMap, Method, Uri}; + +use http::Uri; use std::borrow::Cow; -use std::convert::TryFrom; + +use std::fmt::{Debug, Formatter}; use std::str; /// Represents all of the information necessary to sign an HTTP request. #[derive(Debug)] #[non_exhaustive] pub struct SignableRequest<'a> { - method: &'a Method, - uri: &'a Uri, - headers: &'a HeaderMap, + method: &'a str, + uri: Uri, + headers: Vec<(&'a str, &'a str)>, body: SignableBody<'a>, } impl<'a> SignableRequest<'a> { - /// Creates a new `SignableRequest`. If you have an [`http::Request`], then - /// consider using [`SignableRequest::from`] instead of `new`. + /// Creates a new `SignableRequest`. pub fn new( - method: &'a Method, - uri: &'a Uri, - headers: &'a HeaderMap, + method: &'a str, + uri: impl Into>, + headers: impl Iterator, body: SignableBody<'a>, - ) -> Self { - Self { + ) -> Result { + let uri = uri + .into() + .parse() + .map_err(|e| SigningError::from(CanonicalRequestError::from(e)))?; + let headers = headers.collect(); + Ok(Self { method, uri, headers, body, - } + }) } /// Returns the signable URI - pub fn uri(&self) -> &Uri { - self.uri + pub(crate) fn uri(&self) -> &Uri { + &self.uri } /// Returns the signable HTTP method - pub fn method(&self) -> &Method { + pub(crate) fn method(&self) -> &str { self.method } /// Returns the request headers - pub fn headers(&self) -> &HeaderMap { - self.headers + pub(crate) fn headers(&self) -> &[(&str, &str)] { + self.headers.as_slice() } /// Returns the signable body @@ -66,21 +71,6 @@ impl<'a> SignableRequest<'a> { } } -impl<'a, B> From<&'a http::Request> for SignableRequest<'a> -where - B: 'a, - B: AsRef<[u8]>, -{ - fn from(request: &'a http::Request) -> SignableRequest<'a> { - SignableRequest::new( - request.method(), - request.uri(), - request.headers(), - SignableBody::Bytes(request.body().as_ref()), - ) - } -} - /// A signable HTTP request body #[derive(Debug, Clone, Eq, PartialEq)] #[non_exhaustive] @@ -106,48 +96,83 @@ pub enum SignableBody<'a> { /// Instructions for applying a signature to an HTTP request. #[derive(Debug)] pub struct SigningInstructions { - headers: Option>, - params: Option)>>, + headers: Vec
, + params: Vec<(&'static str, Cow<'static, str>)>, +} + +/// Header representation for use in [`SigningInstructions`] +pub struct Header { + key: &'static str, + value: String, + sensitive: bool, +} + +impl Debug for Header { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let mut fmt = f.debug_struct("Header"); + fmt.field("key", &self.key); + let value = if self.sensitive { + "** REDACTED **" + } else { + &self.value + }; + fmt.field("value", &value); + fmt.finish() + } +} + +impl Header { + /// The name of this header + pub fn name(&self) -> &'static str { + self.key + } + + /// The value of this header + pub fn value(&self) -> &str { + &self.value + } + + /// Whether this header has a sensitive value + pub fn sensitive(&self) -> bool { + self.sensitive + } } impl SigningInstructions { - fn new( - headers: Option>, - params: Option)>>, - ) -> Self { + fn new(headers: Vec
, params: Vec<(&'static str, Cow<'static, str>)>) -> Self { Self { headers, params } } - /// Returns a reference to the headers that should be added to the request. - pub fn headers(&self) -> Option<&HeaderMap> { - self.headers.as_ref() + /// Returns the headers and query params that should be applied to this request + pub fn into_parts(self) -> (Vec
, Vec<(&'static str, Cow<'static, str>)>) { + (self.headers, self.params) } - /// Returns the headers and sets the internal value to `None`. - pub fn take_headers(&mut self) -> Option> { - self.headers.take() + /// Returns a reference to the headers that should be added to the request. + pub fn headers(&self) -> impl Iterator { + self.headers + .iter() + .map(|header| (header.key, header.value.as_str())) } /// Returns a reference to the query parameters that should be added to the request. - pub fn params(&self) -> Option<&Vec<(&'static str, Cow<'static, str>)>> { - self.params.as_ref() - } - - /// Returns the query parameters and sets the internal value to `None`. - pub fn take_params(&mut self) -> Option)>> { - self.params.take() + pub fn params(&self) -> &[(&str, Cow<'static, str>)] { + self.params.as_slice() } + #[cfg(any(feature = "http0-compat", test))] /// Applies the instructions to the given `request`. - pub fn apply_to_request(mut self, request: &mut http::Request) { - if let Some(new_headers) = self.take_headers() { - for (name, value) in new_headers.into_iter() { - request.headers_mut().insert(name.unwrap(), value); - } + pub fn apply_to_request(self, request: &mut http::Request) { + let (new_headers, new_query) = self.into_parts(); + for header in new_headers.into_iter() { + let mut value = http::HeaderValue::from_str(&header.value).unwrap(); + value.set_sensitive(header.sensitive); + request.headers_mut().insert(header.key, value); } - if let Some(params) = self.take_params() { - let mut query = QueryWriter::new(request.uri()); - for (name, value) in params { + + if !new_query.is_empty() { + let mut query = aws_smithy_http::query_writer::QueryWriter::new(request.uri()); + for (name, value) in new_query { query.insert(name, &value); } *request.uri_mut() = query.build_uri(); @@ -167,14 +192,14 @@ pub fn sign<'a>( let (signing_headers, signature) = calculate_signing_headers(&request, params)?.into_parts(); Ok(SigningOutput::new( - SigningInstructions::new(Some(signing_headers), None), + SigningInstructions::new(signing_headers, vec![]), signature, )) } SignatureLocation::QueryParams => { let (params, signature) = calculate_signing_params(&request, params)?; Ok(SigningOutput::new( - SigningInstructions::new(None, Some(params)), + SigningInstructions::new(vec![], params), signature, )) } @@ -238,7 +263,7 @@ fn calculate_signing_params<'a>( fn calculate_signing_headers<'a>( request: &'a SignableRequest<'a>, params: &'a SigningParams<'a>, -) -> Result>, SigningError> { +) -> Result>, SigningError> { // Step 1: https://docs.aws.amazon.com/en_pv/general/latest/gr/sigv4-create-canonical-request.html. let creq = CanonicalRequest::from(request, params)?; tracing::trace!(canonical_request = %creq); @@ -263,12 +288,14 @@ fn calculate_signing_headers<'a>( // Step 4: https://docs.aws.amazon.com/en_pv/general/latest/gr/sigv4-add-signature-to-request.html let values = creq.values.as_headers().expect("signing with headers"); - let mut headers = HeaderMap::new(); + let mut headers = vec![]; add_header(&mut headers, header::X_AMZ_DATE, &values.date_time, false); - headers.insert( - "authorization", - build_authorization_header(params.access_key, &creq, sts, &signature), - ); + headers.push(Header { + key: "authorization", + value: build_authorization_header(params.access_key, &creq, sts, &signature), + + sensitive: false, + }); if params.settings.payload_checksum_kind == PayloadChecksumKind::XAmzSha256 { add_header( &mut headers, @@ -290,10 +317,12 @@ fn calculate_signing_headers<'a>( Ok(SigningOutput::new(headers, signature)) } -fn add_header(map: &mut HeaderMap, key: &'static str, value: &str, sensitive: bool) { - let mut value = HeaderValue::try_from(value).expect(key); - value.set_sensitive(sensitive); - map.insert(key, value); +fn add_header(map: &mut Vec
, key: &'static str, value: &str, sensitive: bool) { + map.push(Header { + key, + value: value.to_string(), + sensitive, + }); } // add signature to authorization header @@ -303,46 +332,54 @@ fn build_authorization_header( creq: &CanonicalRequest<'_>, sts: StringToSign<'_>, signature: &str, -) -> HeaderValue { - let mut value = HeaderValue::try_from(format!( +) -> String { + format!( "{} Credential={}/{}, SignedHeaders={}, Signature={}", HMAC_256, access_key, sts.scope, creq.values.signed_headers().as_str(), signature - )) - .unwrap(); - value.set_sensitive(true); - value + ) } #[cfg(test)] mod tests { - use super::{sign, SigningInstructions}; + use super::sign; use crate::date_time::test_parsers::parse_date_time; use crate::http_request::sign::SignableRequest; use crate::http_request::test::{ - make_headers_comparable, test_request, test_signed_request, - test_signed_request_query_params, + test_request, test_signed_request, test_signed_request_query_params, }; use crate::http_request::{ - SessionTokenMode, SignatureLocation, SigningParams, SigningSettings, + SessionTokenMode, SignableBody, SignatureLocation, SigningParams, SigningSettings, }; - use http::{HeaderMap, HeaderValue}; + use http::{HeaderValue, Request}; use pretty_assertions::assert_eq; use proptest::proptest; - use std::borrow::Cow; + use std::iter; + use std::time::Duration; macro_rules! assert_req_eq { - ($a:tt, $b:tt) => { - make_headers_comparable(&mut $a); - make_headers_comparable(&mut $b); - assert_eq!(format!("{:?}", $a), format!("{:?}", $b)) + (http: $expected:expr, $actual:expr) => { + let mut expected = ($expected).map(|_b|"body"); + let mut actual = ($actual).map(|_b|"body"); + make_headers_comparable(&mut expected); + make_headers_comparable(&mut actual); + assert_eq!(format!("{:?}", expected), format!("{:?}", actual)); + }; + ($expected:tt, $actual:tt) => { + assert_req_eq!(http: ($expected).as_http_request(), $actual); }; } + pub(crate) fn make_headers_comparable(request: &mut Request) { + for (_name, value) in request.headers_mut() { + value.set_sensitive(false); + } + } + #[test] fn test_sign_vanilla_with_headers() { let settings = SigningSettings::default(); @@ -364,10 +401,10 @@ mod tests { out.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out.output.apply_to_request(&mut signed); - let mut expected = test_signed_request("get-vanilla-query-order-key-case"); + let expected = test_signed_request("get-vanilla-query-order-key-case"); assert_req_eq!(expected, signed); } @@ -393,10 +430,10 @@ mod tests { out.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out.output.apply_to_request(&mut signed); - let mut expected = test_signed_request(test); + let expected = test_signed_request(test); assert_req_eq!(expected, signed); } @@ -425,10 +462,10 @@ mod tests { out.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out.output.apply_to_request(&mut signed); - let mut expected = test_signed_request_query_params("get-vanilla-query-order-key-case"); + let expected = test_signed_request_query_params("get-vanilla-query-order-key-case"); assert_req_eq!(expected, signed); } @@ -449,7 +486,8 @@ mod tests { .uri("https://some-endpoint.some-region.amazonaws.com") .header("some-header", HeaderValue::from_str("テスト").unwrap()) .body("") - .unwrap(); + .unwrap() + .into(); let signable = SignableRequest::from(&original); let out = sign(signable, ¶ms).unwrap(); assert_eq!( @@ -457,10 +495,10 @@ mod tests { out.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out.output.apply_to_request(&mut signed); - let mut expected = http::Request::builder() + let expected = http::Request::builder() .uri("https://some-endpoint.some-region.amazonaws.com") .header("some-header", HeaderValue::from_str("テスト").unwrap()) .header( @@ -479,7 +517,7 @@ mod tests { ) .body("") .unwrap(); - assert_req_eq!(expected, signed); + assert_req_eq!(http: expected, signed); } #[test] @@ -501,7 +539,8 @@ mod tests { let original = http::Request::builder() .uri("https://some-endpoint.some-region.amazonaws.com") .body("") - .unwrap(); + .unwrap() + .into(); let out_without_session_token = sign(SignableRequest::from(&original), ¶ms).unwrap(); params.security_token = Some("notarealsessiontoken"); @@ -516,12 +555,12 @@ mod tests { out_without_session_token.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out_with_session_token_but_excluded .output .apply_to_request(&mut signed); - let mut expected = http::Request::builder() + let expected = http::Request::builder() .uri("https://some-endpoint.some-region.amazonaws.com") .header( "x-amz-date", @@ -541,9 +580,9 @@ mod tests { "x-amz-security-token", HeaderValue::from_str("notarealsessiontoken").unwrap(), ) - .body("") + .body(b"") .unwrap(); - assert_req_eq!(expected, signed); + assert_req_eq!(http: expected, signed); } #[test] @@ -566,7 +605,8 @@ mod tests { HeaderValue::from_str("  test test ").unwrap(), ) .body("") - .unwrap(); + .unwrap() + .into(); let signable = SignableRequest::from(&original); let out = sign(signable, ¶ms).unwrap(); assert_eq!( @@ -574,10 +614,10 @@ mod tests { out.signature ); - let mut signed = original; + let mut signed = original.as_http_request(); out.output.apply_to_request(&mut signed); - let mut expected = http::Request::builder() + let expected = http::Request::builder() .uri("https://some-endpoint.some-region.amazonaws.com") .header( "some-header", @@ -599,30 +639,7 @@ mod tests { ) .body("") .unwrap(); - assert_req_eq!(expected, signed); - } - - #[test] - fn test_sign_headers_returning_expected_error_on_invalid_utf8() { - let settings = SigningSettings::default(); - let params = SigningParams { - access_key: "123", - secret_key: "asdf", - security_token: None, - region: "us-east-1", - service_name: "foo", - time: std::time::SystemTime::UNIX_EPOCH, - settings, - }; - - 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), ¶ms); - assert!(creq.is_err()); + assert_req_eq!(http: expected, signed); } proptest! { @@ -630,8 +647,7 @@ mod tests { // Only byte values between 32 and 255 (inclusive) are permitted, excluding byte 127, for // [HeaderValue](https://docs.rs/http/latest/http/header/struct.HeaderValue.html#method.from_bytes). fn test_sign_headers_no_panic( - left in proptest::collection::vec(32_u8..=126, 0..100), - right in proptest::collection::vec(128_u8..=255, 0..100), + header in ".*" ) { let settings = SigningSettings::default(); let params = SigningParams { @@ -644,57 +660,17 @@ mod tests { settings, }; - let bytes = left.iter().chain(right.iter()).cloned().collect::>(); - let req = http::Request::builder() - .uri("https://foo.com/") - .header("x-sign-me", HeaderValue::from_bytes(&bytes).unwrap()) - .body(&[]) - .unwrap(); - - // The test considered a pass if the creation of `creq` does not panic. - let _creq = crate::http_request::sign( - SignableRequest::from(&req), - ¶ms); - } - } + let req = SignableRequest::new( + "GET", + "https://foo.com", + iter::once(("x-sign-me", header.as_str())), + SignableBody::Bytes(&[]) + ); - #[test] - fn apply_signing_instructions_headers() { - let mut headers = HeaderMap::new(); - headers.insert("some-header", HeaderValue::from_static("foo")); - headers.insert("some-other-header", HeaderValue::from_static("bar")); - let instructions = SigningInstructions::new(Some(headers), None); - - let mut request = http::Request::builder() - .uri("https://some-endpoint.some-region.amazonaws.com") - .body("") - .unwrap(); - - instructions.apply_to_request(&mut request); - - let get_header = |n: &str| request.headers().get(n).unwrap().to_str().unwrap(); - assert_eq!("foo", get_header("some-header")); - assert_eq!("bar", get_header("some-other-header")); - } - - #[test] - fn apply_signing_instructions_query_params() { - let params = vec![ - ("some-param", Cow::Borrowed("f&o?o")), - ("some-other-param?", Cow::Borrowed("bar")), - ]; - let instructions = SigningInstructions::new(None, Some(params)); - - let mut request = http::Request::builder() - .uri("https://some-endpoint.some-region.amazonaws.com/some/path") - .body("") - .unwrap(); - - instructions.apply_to_request(&mut request); - - assert_eq!( - "/some/path?some-param=f%26o%3Fo&some-other-param%3F=bar", - request.uri().path_and_query().unwrap().to_string() - ); + if let Ok(req) = req { + // The test considered a pass if the creation of `creq` does not panic. + let _creq = crate::http_request::sign(req, ¶ms); + } + } } } diff --git a/aws/rust-runtime/aws-sigv4/src/http_request/test.rs b/aws/rust-runtime/aws-sigv4/src/http_request/test.rs index 93bef370f..8b349538a 100644 --- a/aws/rust-runtime/aws-sigv4/src/http_request/test.rs +++ b/aws/rust-runtime/aws-sigv4/src/http_request/test.rs @@ -5,8 +5,8 @@ //! Functions shared between the tests of several modules. -use bytes::Bytes; -use http::{Method, Request, Uri, Version}; +use crate::http_request::{SignableBody, SignableRequest}; +use http::{Method, Request, Uri}; use std::error::Error as StdError; fn path(name: &str, ext: &str) -> String { @@ -34,19 +34,19 @@ pub(crate) fn test_sts(name: &str) -> String { read(&path(name, "sts")) } -pub(crate) fn test_request(name: &str) -> Request { +pub(crate) fn test_request(name: &str) -> TestRequest { test_parsed_request(name, "req") } -pub(crate) fn test_signed_request(name: &str) -> Request { +pub(crate) fn test_signed_request(name: &str) -> TestRequest { test_parsed_request(name, "sreq") } -pub(crate) fn test_signed_request_query_params(name: &str) -> Request { +pub(crate) fn test_signed_request_query_params(name: &str) -> TestRequest { test_parsed_request(name, "qpsreq") } -fn test_parsed_request(name: &str, ext: &str) -> Request { +fn test_parsed_request(name: &str, ext: &str) -> TestRequest { let path = path(name, ext); match parse_request(read(&path).as_bytes()) { Ok(parsed) => parsed, @@ -54,15 +54,86 @@ fn test_parsed_request(name: &str, ext: &str) -> Request { } } -pub(crate) fn make_headers_comparable(request: &mut Request) { - for (_name, value) in request.headers_mut() { - value.set_sensitive(false); +pub(crate) struct TestRequest { + pub(crate) uri: String, + pub(crate) method: String, + pub(crate) headers: Vec<(String, String)>, + pub(crate) body: TestSignedBody, +} + +pub(crate) enum TestSignedBody { + Signable(SignableBody<'static>), + Bytes(Vec), +} + +impl TestSignedBody { + fn as_signable_body(&self) -> SignableBody<'_> { + match self { + TestSignedBody::Signable(data) => data.clone(), + TestSignedBody::Bytes(data) => SignableBody::Bytes(data.as_slice()), + } + } +} + +impl TestRequest { + pub(crate) fn set_body(&mut self, body: SignableBody<'static>) { + self.body = TestSignedBody::Signable(body); + } + + pub(crate) fn as_http_request(&self) -> http::Request<&'static str> { + let mut builder = http::Request::builder() + .uri(&self.uri) + .method(Method::from_bytes(self.method.as_bytes()).unwrap()); + for (k, v) in &self.headers { + builder = builder.header(k, v); + } + builder.body("body").unwrap() } } -fn parse_request( - s: &[u8], -) -> Result, Box> { +impl> From> for TestRequest { + fn from(value: Request) -> Self { + let invalid = value + .headers() + .values() + .find(|h| std::str::from_utf8(h.as_bytes()).is_err()); + if let Some(invalid) = invalid { + panic!("invalid header: {:?}", invalid); + } + Self { + uri: value.uri().to_string(), + method: value.method().to_string(), + headers: value + .headers() + .iter() + .map(|(k, v)| { + ( + k.to_string(), + String::from_utf8(v.as_bytes().to_vec()).unwrap(), + ) + }) + .collect::>(), + body: TestSignedBody::Bytes(value.body().as_ref().to_vec()), + } + } +} + +impl<'a> From<&'a TestRequest> for SignableRequest<'a> { + fn from(request: &'a TestRequest) -> SignableRequest<'a> { + SignableRequest::new( + &request.method, + &request.uri, + request + .headers + .iter() + .map(|(k, v)| (k.as_str(), v.as_str())), + request.body.as_signable_body(), + ) + .expect("URI MUST be valid") + } +} + +fn parse_request(s: &[u8]) -> Result> { let mut headers = [httparse::EMPTY_HEADER; 64]; // httparse 1.5 requires two trailing newlines to head the header section. let mut with_newline = Vec::from(s); @@ -70,37 +141,30 @@ fn parse_request( let mut req = httparse::Request::new(&mut headers); let _ = req.parse(&with_newline).unwrap(); - let version = match req.version.unwrap() { - 1 => Version::HTTP_11, - _ => unimplemented!(), - }; - - let method = match req.method.unwrap() { - "GET" => Method::GET, - "POST" => Method::POST, - _ => unimplemented!(), - }; - - let mut builder = Request::builder(); - builder = builder.version(version); - builder = builder.method(method); - let mut uri_builder = Uri::builder().scheme("https"); if let Some(path) = req.path { uri_builder = uri_builder.path_and_query(path); } + + let mut headers = vec![]; for header in req.headers { let name = header.name.to_lowercase(); if name == "host" { uri_builder = uri_builder.authority(header.value); } else if !name.is_empty() { - builder = builder.header(&name, header.value); + headers.push(( + header.name.to_string(), + std::str::from_utf8(header.value)?.to_string(), + )); } } - builder = builder.uri(uri_builder.build()?); - let req = builder.body(bytes::Bytes::new())?; - Ok(req) + Ok(TestRequest { + uri: uri_builder.build()?.to_string(), + method: req.method.unwrap().to_string(), + headers, + body: TestSignedBody::Bytes(vec![]), + }) } #[test] -- GitLab