[fix-presigned] add comments and reorganize

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<IncomingBody>,
+		req: Request<IncomingBody>,
 		endpoint: K2VApiEndpoint,
 	) -> Result<Response<ResBody>, 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<IncomingBody>,
+		req: Request<IncomingBody>,
 		endpoint: S3ApiEndpoint,
 	) -> Result<Response<ResBody>, 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<Sha256>;
 
+pub async fn verify_request(
+	garage: &Garage,
+	mut req: Request<IncomingBody>,
+	service: &'static str,
+) -> Result<(Request<streaming::ReqBody>, Key, Option<Hash>), 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<String, String>;
 
 pub async fn check_payload_signature(
 	garage: &Garage,
-	service: &'static str,
 	request: &mut Request<IncomingBody>,
+	service: &'static str,
 ) -> Result<(Option<Key>, Option<Hash>), 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<IncomingBody>,
 	query: QueryMap,
 ) -> Result<(Option<Key>, Option<Hash>), 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<QueryMap, Error> {
 	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<Vec<HeaderName>, Error> {
 	let mut signed_headers = authorization
 		.signed_headers
@@ -236,6 +229,27 @@ fn split_signed_headers(authorization: &Authorization) -> Result<Vec<HeaderName>
 	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<Utc>, 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<Self, Error> {
+	fn parse_header(headers: &HeaderMap) -> Result<Self, Error> {
 		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(),
-	))
-}