From 90cab5b8f2b5212668975bf445a1e86f638fe1c7 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Wed, 28 Feb 2024 10:51:08 +0100 Subject: [PATCH] [fix-presigned] add comments and reorganize --- src/api/k2v/api_server.rs | 18 +----- src/api/s3/api_server.rs | 18 +----- src/api/signature/mod.rs | 29 +++++++++- src/api/signature/payload.rs | 105 ++++++++++++++++++----------------- 4 files changed, 87 insertions(+), 83 deletions(-) diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index 5ed7e286..fdb5db4c 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -15,8 +15,7 @@ use garage_model::garage::Garage; use crate::generic_server::*; use crate::k2v::error::*; -use crate::signature::payload::check_payload_signature; -use crate::signature::streaming::*; +use crate::signature::verify_request; use crate::helpers::*; use crate::k2v::batch::*; @@ -69,7 +68,7 @@ impl ApiHandler for K2VApiServer { async fn handle( &self, - mut req: Request, + req: Request, endpoint: K2VApiEndpoint, ) -> Result, Error> { let K2VApiEndpoint { @@ -86,18 +85,7 @@ 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", &mut req).await?; - let api_key = api_key - .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; - - let req = parse_streaming_body( - &api_key, - req, - &mut content_sha256, - &garage.config.s3_api.s3_region, - "k2v", - )?; + let (req, api_key, _content_sha256) = verify_request(&garage, req, "k2v").await?; let bucket_id = garage .bucket_helper() diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index fdfaf0a4..51f19554 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -17,8 +17,7 @@ use garage_model::key_table::Key; use crate::generic_server::*; use crate::s3::error::*; -use crate::signature::payload::check_payload_signature; -use crate::signature::streaming::*; +use crate::signature::verify_request; use crate::helpers::*; use crate::s3::bucket::*; @@ -107,7 +106,7 @@ impl ApiHandler for S3ApiServer { async fn handle( &self, - mut req: Request, + req: Request, endpoint: S3ApiEndpoint, ) -> Result, Error> { let S3ApiEndpoint { @@ -125,18 +124,7 @@ 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", &mut req).await?; - let api_key = api_key - .ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; - - let req = parse_streaming_body( - &api_key, - req, - &mut content_sha256, - &garage.config.s3_api.s3_region, - "s3", - )?; + let (req, api_key, content_sha256) = verify_request(&garage, req, "s3").await?; let bucket_name = match bucket_name { None => { diff --git a/src/api/signature/mod.rs b/src/api/signature/mod.rs index 4b8b990f..6514da43 100644 --- a/src/api/signature/mod.rs +++ b/src/api/signature/mod.rs @@ -2,19 +2,44 @@ use chrono::{DateTime, Utc}; use hmac::{Hmac, Mac}; use sha2::Sha256; +use hyper::{body::Incoming as IncomingBody, Request}; + +use garage_model::garage::Garage; +use garage_model::key_table::Key; use garage_util::data::{sha256sum, Hash}; +use error::*; + pub mod error; pub mod payload; pub mod streaming; -use error::*; - pub const SHORT_DATE: &str = "%Y%m%d"; pub const LONG_DATETIME: &str = "%Y%m%dT%H%M%SZ"; type HmacSha256 = Hmac; +pub async fn verify_request( + garage: &Garage, + mut req: Request, + service: &'static str, +) -> Result<(Request, Key, Option), Error> { + let (api_key, mut content_sha256) = + payload::check_payload_signature(&garage, &mut req, service).await?; + let api_key = + api_key.ok_or_else(|| Error::forbidden("Garage does not support anonymous access yet"))?; + + let req = streaming::parse_streaming_body( + &api_key, + req, + &mut content_sha256, + &garage.config.s3_api.s3_region, + service, + )?; + + Ok((req, api_key, content_sha256)) +} + pub fn verify_signed_content(expected_sha256: Hash, body: &[u8]) -> Result<(), Error> { if expected_sha256 != sha256sum(body) { return Err(Error::bad_request( diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 435b4206..949da601 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -35,15 +35,18 @@ pub type QueryMap = HashMap; pub async fn check_payload_signature( garage: &Garage, - service: &'static str, request: &mut Request, + service: &'static str, ) -> Result<(Option, Option), Error> { let query = parse_query_map(request.uri())?; - if request.headers().contains_key(AUTHORIZATION) { - check_standard_signature(garage, service, request, query).await - } else if query.contains_key(X_AMZ_ALGORITHM.as_str()) { + if query.contains_key(X_AMZ_ALGORITHM.as_str()) { + // 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. check_presigned_signature(garage, service, request, query).await + } else if request.headers().contains_key(AUTHORIZATION) { + check_standard_signature(garage, service, request, query).await } else { // Unsigned (anonymous) request let content_sha256 = request @@ -68,27 +71,15 @@ async fn check_standard_signature( request: &Request, query: QueryMap, ) -> Result<(Option, Option), Error> { - let authorization = Authorization::parse(request.headers())?; + let authorization = Authorization::parse_header(request.headers())?; // Verify that all necessary request headers are included in signed_headers - // For standard AWSv4 signatures, the following must be incldued: - // - the Host header (in all cases) + // For standard AWSv4 signatures, the following must be included: + // - the Host header (mandatory) // - the Content-Type header, if it is used in the request // - all x-amz-* headers used in the request let signed_headers = split_signed_headers(&authorization)?; - if !signed_headers.contains(&HOST) { - return Err(Error::bad_request("Header `Host` should be signed")); - } - for (name, _) in request.headers().iter() { - if name == CONTENT_TYPE || name.as_str().starts_with("x-amz-") { - if !signed_headers.contains(name) { - return Err(Error::bad_request(format!( - "Header `{}` should be signed", - name - ))); - } - } - } + verify_signed_headers(request.headers(), &signed_headers, &[CONTENT_TYPE])?; let canonical_request = canonical_request( service, @@ -135,22 +126,10 @@ async fn check_presigned_signature( // Verify that all necessary request headers are included in signed_headers // For AWSv4 pre-signed URLs, the following must be incldued: - // - the Host header (in all cases) + // - the Host header (mandatory) // - all x-amz-* headers used in the request let signed_headers = split_signed_headers(&authorization)?; - if !signed_headers.contains(&HOST) { - return Err(Error::bad_request("Header `Host` should be signed")); - } - for (name, _) in request.headers().iter() { - if name.as_str().starts_with("x-amz-") { - if !signed_headers.contains(name) { - return Err(Error::bad_request(format!( - "Header `{}` should be signed", - name - ))); - } - } - } + verify_signed_headers(request.headers(), &signed_headers, &[])?; // The X-Amz-Signature value is passed as a query parameter, // but the signature cannot be computed from a string that contains itself. @@ -172,13 +151,14 @@ async fn check_presigned_signature( &canonical_request, ); - trace!("canonical request:\n{}", canonical_request); - trace!("string to sign:\n{}", string_to_sign); + trace!("canonical request (presigned url):\n{}", canonical_request); + trace!("string to sign (presigned url):\n{}", string_to_sign); 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. + // In the page on presigned URLs, 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 = @@ -197,7 +177,7 @@ async fn check_presigned_signature( // 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) + // that are not signed, however there is not much reason that this would happen) headers_mut.insert( name, HeaderValue::from_bytes(value.as_bytes()) @@ -206,6 +186,8 @@ async fn check_presigned_signature( } } + // Presigned URLs always use UNSIGNED-PAYLOAD, + // so there is no sha256 hash to return. Ok((Some(key), None)) } @@ -225,6 +207,17 @@ pub fn parse_query_map(uri: &http::uri::Uri) -> Result { Ok(query) } +fn parse_credential(cred: &str) -> Result<(String, String), Error> { + let first_slash = cred + .find('/') + .ok_or_bad_request("Credentials does not contain '/' in authorization field")?; + let (key_id, scope) = cred.split_at(first_slash); + Ok(( + key_id.to_string(), + scope.trim_start_matches('/').to_string(), + )) +} + fn split_signed_headers(authorization: &Authorization) -> Result, Error> { let mut signed_headers = authorization .signed_headers @@ -236,6 +229,27 @@ fn split_signed_headers(authorization: &Authorization) -> Result Ok(signed_headers) } +fn verify_signed_headers( + headers: &HeaderMap, + signed_headers: &[HeaderName], + extra_headers: &[HeaderName], +) -> Result<(), Error> { + if !signed_headers.contains(&HOST) { + return Err(Error::bad_request("Header `Host` should be signed")); + } + for (name, _) in headers.iter() { + if name.as_str().starts_with("x-amz-") || extra_headers.contains(name) { + if !signed_headers.contains(name) { + return Err(Error::bad_request(format!( + "Header `{}` should be signed", + name + ))); + } + } + } + Ok(()) +} + pub fn string_to_sign(datetime: &DateTime, scope_string: &str, canonical_req: &str) -> String { let mut hasher = Sha256::default(); hasher.update(canonical_req.as_bytes()); @@ -381,7 +395,7 @@ pub struct Authorization { } impl Authorization { - fn parse(headers: &HeaderMap) -> Result { + fn parse_header(headers: &HeaderMap) -> Result { let authorization = headers .get(AUTHORIZATION) .ok_or_bad_request("Missing authorization header")? @@ -535,14 +549,3 @@ impl Authorization { Ok(auth) } } - -fn parse_credential(cred: &str) -> Result<(String, String), Error> { - let first_slash = cred - .find('/') - .ok_or_bad_request("Credentials does not contain '/' in authorization field")?; - let (key_id, scope) = cred.split_at(first_slash); - Ok(( - key_id.to_string(), - scope.trim_start_matches('/').to_string(), - )) -}