From a36248a1695de02cc19b25ba127810bd32b6d350 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 1 Mar 2024 13:11:41 +0100 Subject: [PATCH] [fix-signed-headers] aws signatures v4: don't actually check Content-Type is signed This page of the AWS docs indicate that Content-Type should be part of the CanonicalHeaders (and therefore SignedHeaders) strings in signature calculation: https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html However, testing with Minio Client revealed that it did not sign the Content-Type header, and therefore we broke CI by expecting it to be signed. With this commit, we don't mandate Content-Type to be signed anymore, for better compatibility with the ecosystem. Testing against the official behavior of S3 on AWS has not been done. --- script/test-smoke.sh | 4 +--- src/api/signature/payload.rs | 19 ++++++++----------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/script/test-smoke.sh b/script/test-smoke.sh index 6965c0f3..9f9ea50c 100755 --- a/script/test-smoke.sh +++ b/script/test-smoke.sh @@ -81,11 +81,9 @@ if [ -z "$SKIP_AWS" ]; then echo "Invalid multipart upload" exit 1 fi + aws s3api delete-object --bucket eprouvette --key upload fi -echo "OK!!" -exit 0 - # S3CMD if [ -z "$SKIP_S3CMD" ]; then echo "🛠️ Testing with s3cmd" diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index a9e7d34d..0029716a 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, HeaderValue, AUTHORIZATION, CONTENT_TYPE, HOST}; +use hyper::header::{HeaderMap, HeaderName, HeaderValue, AUTHORIZATION, HOST}; use hyper::{body::Incoming as IncomingBody, Method, Request}; use sha2::{Digest, Sha256}; @@ -74,12 +74,13 @@ async fn check_standard_signature( 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 included: + // The following must be included for all signatures: // - the Host header (mandatory) - // - the Content-Type header, if it is used in the request // - all x-amz-* headers used in the request + // AWS also indicates that the Content-Type header should be signed if + // it is used, but Minio client doesn't sign it so we don't check it for compatibility. let signed_headers = split_signed_headers(&authorization)?; - verify_signed_headers(request.headers(), &signed_headers, &[CONTENT_TYPE])?; + verify_signed_headers(request.headers(), &signed_headers)?; let canonical_request = canonical_request( service, @@ -129,7 +130,7 @@ async fn check_presigned_signature( // - the Host header (mandatory) // - all x-amz-* headers used in the request let signed_headers = split_signed_headers(&authorization)?; - verify_signed_headers(request.headers(), &signed_headers, &[])?; + 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. @@ -229,16 +230,12 @@ fn split_signed_headers(authorization: &Authorization) -> Result Ok(signed_headers) } -fn verify_signed_headers( - headers: &HeaderMap, - signed_headers: &[HeaderName], - extra_headers: &[HeaderName], -) -> Result<(), Error> { +fn verify_signed_headers(headers: &HeaderMap, signed_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 name.as_str().starts_with("x-amz-") { if !signed_headers.contains(name) { return Err(Error::bad_request(format!( "Header `{}` should be signed",