From b6a91e549ba8fa9dad8f90fa8b98f282dc211551 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Mon, 4 Mar 2024 12:52:33 +0100 Subject: [PATCH 1/4] [test-presigned] Add API test for presigned requests --- src/garage/tests/common/custom_requester.rs | 4 ++ src/garage/tests/s3/mod.rs | 1 + src/garage/tests/s3/presigned.rs | 72 +++++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 src/garage/tests/s3/presigned.rs diff --git a/src/garage/tests/common/custom_requester.rs b/src/garage/tests/common/custom_requester.rs index 2cac5cd5..8e1eaa56 100644 --- a/src/garage/tests/common/custom_requester.rs +++ b/src/garage/tests/common/custom_requester.rs @@ -64,6 +64,10 @@ impl CustomRequester { vhost_style: false, } } + + pub fn client(&self) -> &Client { + &self.client + } } pub struct RequestBuilder<'a> { diff --git a/src/garage/tests/s3/mod.rs b/src/garage/tests/s3/mod.rs index 623eb665..4ebc4914 100644 --- a/src/garage/tests/s3/mod.rs +++ b/src/garage/tests/s3/mod.rs @@ -1,6 +1,7 @@ mod list; mod multipart; mod objects; +mod presigned; mod simple; mod streaming_signature; mod website; diff --git a/src/garage/tests/s3/presigned.rs b/src/garage/tests/s3/presigned.rs new file mode 100644 index 00000000..15270361 --- /dev/null +++ b/src/garage/tests/s3/presigned.rs @@ -0,0 +1,72 @@ +use std::time::{Duration, SystemTime}; + +use crate::common; +use aws_sdk_s3::presigning::PresigningConfig; +use bytes::Bytes; +use http_body_util::{BodyExt, Full}; +use hyper::Request; + +const STD_KEY: &str = "hello world"; +const BODY: &[u8; 62] = b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + +#[tokio::test] +async fn test_presigned_url() { + let ctx = common::context(); + let bucket = ctx.create_bucket("presigned"); + + let etag = "\"46cf18a9b447991b450cad3facf5937e\""; + let body = Bytes::from(BODY.to_vec()); + + let psc = PresigningConfig::builder() + .start_time(SystemTime::now() - Duration::from_secs(60)) + .expires_in(Duration::from_secs(3600)) + .build() + .unwrap(); + + { + // PutObject + let req = ctx + .client + .put_object() + .bucket(&bucket) + .key(STD_KEY) + .presigned(psc.clone()) + .await + .unwrap(); + + let client = ctx.custom_request.client(); + let req = Request::builder() + .method("PUT") + .uri(req.uri()) + .body(Full::new(body.clone())) + .unwrap(); + let res = client.request(req).await.unwrap(); + assert_eq!(res.status(), 200); + assert_eq!(res.headers().get("etag").unwrap(), etag); + } + + { + // GetObject + let req = ctx + .client + .get_object() + .bucket(&bucket) + .key(STD_KEY) + .presigned(psc) + .await + .unwrap(); + + let client = ctx.custom_request.client(); + let req = Request::builder() + .method("GET") + .uri(req.uri()) + .body(Full::new(Bytes::new())) + .unwrap(); + let res = client.request(req).await.unwrap(); + assert_eq!(res.status(), 200); + assert_eq!(res.headers().get("etag").unwrap(), etag); + + let body2 = BodyExt::collect(res.into_body()).await.unwrap().to_bytes(); + assert_eq!(body, body2); + } +} From 7c4f3473afa3e5501a21bf6e9466da52726bac8e Mon Sep 17 00:00:00 2001 From: asonix Date: Sun, 3 Mar 2024 14:35:01 -0600 Subject: [PATCH 2/4] Lowercase query parameter keys when parsing --- src/api/signature/payload.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 0029716a..250e007e 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -197,7 +197,7 @@ pub fn parse_query_map(uri: &http::uri::Uri) -> Result { if let Some(query_str) = uri.query() { let query_pairs = url::form_urlencoded::parse(query_str.as_bytes()); for (key, val) in query_pairs { - if query.insert(key.to_string(), val.into_owned()).is_some() { + if query.insert(key.to_lowercase().to_string(), val.into_owned()).is_some() { return Err(Error::bad_request(format!( "duplicate query parameter: `{}`", key From c94bf45cbab61ad6471d43d8a7aabb8a32eaad38 Mon Sep 17 00:00:00 2001 From: asonix Date: Sun, 3 Mar 2024 16:26:57 -0600 Subject: [PATCH 3/4] Store original-cased query keys alongside query values --- src/api/signature/payload.rs | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 250e007e..515c0f91 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -31,7 +31,12 @@ pub const AWS4_HMAC_SHA256: &str = "AWS4-HMAC-SHA256"; pub const UNSIGNED_PAYLOAD: &str = "UNSIGNED-PAYLOAD"; pub const STREAMING_AWS4_HMAC_SHA256_PAYLOAD: &str = "STREAMING-AWS4-HMAC-SHA256-PAYLOAD"; -pub type QueryMap = HashMap; +pub type QueryMap = HashMap; + +pub struct QueryValue { + key: String, + value: String, +} pub async fn check_payload_signature( garage: &Garage, @@ -123,7 +128,7 @@ async fn check_presigned_signature( mut query: QueryMap, ) -> Result<(Option, Option), Error> { let algorithm = query.get(X_AMZ_ALGORITHM.as_str()).unwrap(); - let authorization = Authorization::parse_presigned(algorithm, &query)?; + let authorization = Authorization::parse_presigned(&algorithm.value, &query)?; // Verify that all necessary request headers are included in signed_headers // For AWSv4 pre-signed URLs, the following must be incldued: @@ -165,7 +170,7 @@ async fn check_presigned_signature( let name = HeaderName::from_bytes(name.as_bytes()).ok_or_bad_request("Invalid header name")?; if let Some(existing) = headers_mut.get(&name) { - if signed_headers.contains(&name) && existing.as_bytes() != value.as_bytes() { + if signed_headers.contains(&name) && existing.as_bytes() != value.value.as_bytes() { return Err(Error::bad_request(format!( "Conflicting values for `{}` in query parameters and request headers", name @@ -181,7 +186,7 @@ async fn check_presigned_signature( // that are not signed, however there is not much reason that this would happen) headers_mut.insert( name, - HeaderValue::from_bytes(value.as_bytes()) + HeaderValue::from_bytes(value.value.as_bytes()) .ok_or_bad_request("invalid query parameter value")?, ); } @@ -197,7 +202,14 @@ pub fn parse_query_map(uri: &http::uri::Uri) -> Result { if let Some(query_str) = uri.query() { let query_pairs = url::form_urlencoded::parse(query_str.as_bytes()); for (key, val) in query_pairs { - if query.insert(key.to_lowercase().to_string(), val.into_owned()).is_some() { + let key = key.into_owned(); + + let value = QueryValue { + key: key.clone(), + value: val.into_owned(), + }; + + if query.insert(key.to_lowercase(), value).is_some() { return Err(Error::bad_request(format!( "duplicate query parameter: `{}`", key @@ -306,7 +318,7 @@ pub fn canonical_request( // Canonical query string from passed HeaderMap let canonical_query_string = { let mut items = Vec::with_capacity(query.len()); - for (key, value) in query.iter() { + for (_, QueryValue { key, value }) in query.iter() { items.push(uri_encode(&key, true) + "=" + &uri_encode(&value, true)); } items.sort(); @@ -476,6 +488,7 @@ impl Authorization { let duration = query .get(X_AMZ_EXPIRES.as_str()) .ok_or_bad_request("X-Amz-Expires not found in query parameters")? + .value .parse() .map_err(|_| Error::bad_request("X-Amz-Expires is not a number".to_string()))?; @@ -488,18 +501,18 @@ impl Authorization { let date = query .get(X_AMZ_DATE.as_str()) .ok_or_bad_request("Missing X-Amz-Date field")?; - let date = parse_date(date)?; + let date = parse_date(&date.value)?; if Utc::now() - date > Duration::seconds(duration) { return Err(Error::bad_request("Date is too old".to_string())); } - let (key_id, scope) = parse_credential(cred)?; + let (key_id, scope) = parse_credential(&cred.value)?; Ok(Authorization { key_id, scope, - signed_headers: signed_headers.to_string(), - signature: signature.to_string(), + signed_headers: signed_headers.value.clone(), + signature: signature.value.clone(), content_sha256: UNSIGNED_PAYLOAD.to_string(), date, }) From c8e416aaa5203347a35407a929ac5dfcb1f2a6bc Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Mon, 4 Mar 2024 13:28:54 +0100 Subject: [PATCH 4/4] [test-presigned] Use a HeaderMap type for QueryMap --- src/api/signature/payload.rs | 46 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 515c0f91..d72736bb 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -31,11 +31,12 @@ pub const AWS4_HMAC_SHA256: &str = "AWS4-HMAC-SHA256"; pub const UNSIGNED_PAYLOAD: &str = "UNSIGNED-PAYLOAD"; pub const STREAMING_AWS4_HMAC_SHA256_PAYLOAD: &str = "STREAMING-AWS4-HMAC-SHA256-PAYLOAD"; -pub type QueryMap = HashMap; - +pub type QueryMap = HeaderMap; pub struct QueryValue { - key: String, - value: String, + /// Original key with potential uppercase characters, + /// for use in signature calculation + key: String, + value: String, } pub async fn check_payload_signature( @@ -45,7 +46,7 @@ pub async fn check_payload_signature( ) -> Result<(Option, Option), Error> { let query = parse_query_map(request.uri())?; - if query.contains_key(X_AMZ_ALGORITHM.as_str()) { + if query.contains_key(&X_AMZ_ALGORITHM) { // We check for presigned-URL-style authentification first, because // the browser or someting else could inject an Authorization header // that is totally unrelated to AWS signatures. @@ -127,7 +128,7 @@ async fn check_presigned_signature( request: &mut Request, mut query: QueryMap, ) -> Result<(Option, Option), Error> { - let algorithm = query.get(X_AMZ_ALGORITHM.as_str()).unwrap(); + let algorithm = query.get(&X_AMZ_ALGORITHM).unwrap(); let authorization = Authorization::parse_presigned(&algorithm.value, &query)?; // Verify that all necessary request headers are included in signed_headers @@ -141,7 +142,7 @@ async fn check_presigned_signature( // but the signature cannot be computed from a string that contains itself. // AWS specifies that all query params except X-Amz-Signature are included // in the canonical request. - query.remove(X_AMZ_SIGNATURE.as_str()); + query.remove(&X_AMZ_SIGNATURE); let canonical_request = canonical_request( service, request.method(), @@ -167,9 +168,7 @@ async fn check_presigned_signature( // then an InvalidRequest error is raised. let headers_mut = request.headers_mut(); for (name, value) in query.iter() { - let name = - HeaderName::from_bytes(name.as_bytes()).ok_or_bad_request("Invalid header name")?; - if let Some(existing) = headers_mut.get(&name) { + if let Some(existing) = headers_mut.get(name) { if signed_headers.contains(&name) && existing.as_bytes() != value.value.as_bytes() { return Err(Error::bad_request(format!( "Conflicting values for `{}` in query parameters and request headers", @@ -198,18 +197,19 @@ async fn check_presigned_signature( } pub fn parse_query_map(uri: &http::uri::Uri) -> Result { - let mut query = QueryMap::new(); + let mut query = QueryMap::with_capacity(0); if let Some(query_str) = uri.query() { let query_pairs = url::form_urlencoded::parse(query_str.as_bytes()); for (key, val) in query_pairs { - let key = key.into_owned(); + let name = + HeaderName::from_bytes(key.as_bytes()).ok_or_bad_request("Invalid header name")?; - let value = QueryValue { - key: key.clone(), - value: val.into_owned(), - }; + let value = QueryValue { + key: key.to_string(), + value: val.into_owned(), + }; - if query.insert(key.to_lowercase(), value).is_some() { + if query.insert(name, value).is_some() { return Err(Error::bad_request(format!( "duplicate query parameter: `{}`", key @@ -476,19 +476,19 @@ impl Authorization { } let cred = query - .get(X_AMZ_CREDENTIAL.as_str()) + .get(&X_AMZ_CREDENTIAL) .ok_or_bad_request("X-Amz-Credential not found in query parameters")?; let signed_headers = query - .get(X_AMZ_SIGNEDHEADERS.as_str()) + .get(&X_AMZ_SIGNEDHEADERS) .ok_or_bad_request("X-Amz-SignedHeaders not found in query parameters")?; let signature = query - .get(X_AMZ_SIGNATURE.as_str()) + .get(&X_AMZ_SIGNATURE) .ok_or_bad_request("X-Amz-Signature not found in query parameters")?; let duration = query - .get(X_AMZ_EXPIRES.as_str()) + .get(&X_AMZ_EXPIRES) .ok_or_bad_request("X-Amz-Expires not found in query parameters")? - .value + .value .parse() .map_err(|_| Error::bad_request("X-Amz-Expires is not a number".to_string()))?; @@ -499,7 +499,7 @@ impl Authorization { } let date = query - .get(X_AMZ_DATE.as_str()) + .get(&X_AMZ_DATE) .ok_or_bad_request("Missing X-Amz-Date field")?; let date = parse_date(&date.value)?;