From e610e16abb783ea7a7c4ca8eb3ca0976e215cfb1 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Wed, 28 Feb 2024 11:43:27 +0100 Subject: [fix-presigned] add comments and reorganize --- src/api/signature/payload.rs | 81 +++++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 5935724d..9ee38023 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -40,10 +40,13 @@ pub async fn check_payload_signature( ) -> 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)) } @@ -236,6 +218,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 +384,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")? -- cgit v1.2.3