From e9f759d4cb9be28584ab511a0a2dc78b579475c8 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Wed, 28 Feb 2024 00:27:54 +0100 Subject: [PATCH] [fix-presigned] presigned requests: allow x-amz-* query parameters to stand in for equivalent headers --- src/api/k2v/api_server.rs | 5 ++- src/api/s3/api_server.rs | 5 ++- src/api/signature/payload.rs | 45 +++++++++++++++++---- src/garage/tests/common/custom_requester.rs | 2 +- 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index e97da2af..5ed7e286 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -69,7 +69,7 @@ impl ApiHandler for K2VApiServer { async fn handle( &self, - req: Request, + mut req: Request, endpoint: K2VApiEndpoint, ) -> Result, Error> { let K2VApiEndpoint { @@ -86,7 +86,8 @@ impl ApiHandler for K2VApiServer { return Ok(options_res.map(|_empty_body: EmptyBody| empty_body())); } - let (api_key, mut content_sha256) = check_payload_signature(&garage, "k2v", &req).await?; + let (api_key, mut content_sha256) = + check_payload_signature(&garage, "k2v", &mut req).await?; let api_key = api_key .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 08405923..fdfaf0a4 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -107,7 +107,7 @@ impl ApiHandler for S3ApiServer { async fn handle( &self, - req: Request, + mut req: Request, endpoint: S3ApiEndpoint, ) -> Result, Error> { let S3ApiEndpoint { @@ -125,7 +125,8 @@ impl ApiHandler for S3ApiServer { return Ok(options_res.map(|_empty_body: EmptyBody| empty_body())); } - let (api_key, mut content_sha256) = check_payload_signature(&garage, "s3", &req).await?; + let (api_key, mut content_sha256) = + check_payload_signature(&garage, "s3", &mut req).await?; let api_key = api_key .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 8a7a4341..435b4206 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -3,7 +3,7 @@ use std::convert::TryFrom; use chrono::{DateTime, Duration, NaiveDateTime, TimeZone, Utc}; use hmac::Mac; -use hyper::header::{HeaderMap, HeaderName, AUTHORIZATION, CONTENT_TYPE, HOST}; +use hyper::header::{HeaderMap, HeaderName, HeaderValue, AUTHORIZATION, CONTENT_TYPE, HOST}; use hyper::{body::Incoming as IncomingBody, Method, Request}; use sha2::{Digest, Sha256}; @@ -36,7 +36,7 @@ pub type QueryMap = HashMap; pub async fn check_payload_signature( garage: &Garage, service: &'static str, - request: &Request, + request: &mut Request, ) -> Result<(Option, Option), Error> { let query = parse_query_map(request.uri())?; @@ -96,7 +96,7 @@ async fn check_standard_signature( request.uri().path(), &query, request.headers(), - signed_headers, + &signed_headers, &authorization.content_sha256, )?; let string_to_sign = string_to_sign( @@ -127,7 +127,7 @@ async fn check_standard_signature( async fn check_presigned_signature( garage: &Garage, service: &'static str, - request: &Request, + request: &mut Request, mut query: QueryMap, ) -> Result<(Option, Option), Error> { let algorithm = query.get(X_AMZ_ALGORITHM.as_str()).unwrap(); @@ -163,7 +163,7 @@ async fn check_presigned_signature( request.uri().path(), &query, request.headers(), - signed_headers, + &signed_headers, &authorization.content_sha256, )?; let string_to_sign = string_to_sign( @@ -177,6 +177,35 @@ async fn check_presigned_signature( let key = verify_v4(garage, service, &authorization, string_to_sign.as_bytes()).await?; + // AWS specifies that if a signed query parameter and a signed header of the same name + // have different values, 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 signed_headers.contains(&name) && existing.as_bytes() != value.as_bytes() { + return Err(Error::bad_request(format!( + "Conflicting values for `{}` in query parameters and request headers", + name + ))); + } + } + if name.as_str().starts_with("x-amz-") { + // Query parameters that start by x-amz- are actually intended to stand in for + // headers that can't be added at the time the request is made. + // What we do is just add them to the Request object as regular headers, + // that will be handled downstream as if they were included like in a normal request. + // (Here we allow such query parameters to override headers with the same name + // if they are not signed, however there is not much reason that this would happen) + headers_mut.insert( + name, + HeaderValue::from_bytes(value.as_bytes()) + .ok_or_bad_request("invalid query parameter value")?, + ); + } + } + Ok((Some(key), None)) } @@ -197,12 +226,13 @@ pub fn parse_query_map(uri: &http::uri::Uri) -> Result { } fn split_signed_headers(authorization: &Authorization) -> Result, Error> { - let signed_headers = authorization + let mut signed_headers = authorization .signed_headers .split(';') .map(HeaderName::try_from) .collect::, _>>() .ok_or_bad_request("invalid header name")?; + signed_headers.sort_by(|h1, h2| h1.as_str().cmp(h2.as_str())); Ok(signed_headers) } @@ -224,7 +254,7 @@ pub fn canonical_request( canonical_uri: &str, query: &QueryMap, headers: &HeaderMap, - mut signed_headers: Vec, + signed_headers: &[HeaderName], content_sha256: &str, ) -> Result { // There seems to be evidence that in AWSv4 signatures, the path component is url-encoded @@ -273,7 +303,6 @@ pub fn canonical_request( }; // Canonical header string calculated from signed headers - signed_headers.sort_by(|h1, h2| h1.as_str().cmp(h2.as_str())); let canonical_header_string = signed_headers .iter() .map(|name| { diff --git a/src/garage/tests/common/custom_requester.rs b/src/garage/tests/common/custom_requester.rs index f311418c..2cac5cd5 100644 --- a/src/garage/tests/common/custom_requester.rs +++ b/src/garage/tests/common/custom_requester.rs @@ -251,7 +251,7 @@ impl<'a> RequestBuilder<'a> { uri.path(), &query, &all_headers, - signed_headers, + &signed_headers, &body_sha, ) .unwrap();