Merge pull request 'force uri encoding before check signature' (#1382) from gwenlg/garage:signature_doesnt_match_1155 into main-v2

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/1382
Reviewed-by: Alex <lx@deuxfleurs.fr>

Cargo.lock

@@ -1548,6 +1548,7 @@ dependencies = [
  "md-5",
  "nom",
  "opentelemetry",
+ "percent-encoding",
  "pin-project",
  "quick-xml",
  "serde",

src/api/common/Cargo.toml

@@ -27,6 +27,7 @@ thiserror.workspace = true
 hex.workspace = true
 hmac.workspace = true
 md-5.workspace = true
+percent-encoding.workspace = true
 tracing.workspace = true
 nom.workspace = true
 pin-project.workspace = true

src/api/common/signature/error.rs

@@ -11,8 +11,13 @@ pub enum Error {
 	Common(CommonError),
 
 	/// Authorization Header Malformed
-	#[error("Authorization header malformed, unexpected scope: {0}")]
-	AuthorizationHeaderMalformed(String),
+	#[error(
+		"Authorization header malformed, unexpected scope: '{unexpected}', expected: '{expected}'"
+	)]
+	AuthorizationHeaderMalformed {
+		unexpected: String,
+		expected: String,
+	},
 
 	// Category: bad request
 	/// The request contained an invalid UTF-8 sequence in its path or in other parameters

src/api/common/signature/payload.rs

@@ -340,7 +340,11 @@ pub fn canonical_request(
 	let canonical_uri: std::borrow::Cow<str> = if service != "s3" {
 		uri_encode(canonical_uri, false).into()
 	} else {
-		canonical_uri.into()
+		//TODO: decode is already do for construct Api::EndPoint, should be better to be able to keep it instead of compute it again.
+		let key = percent_encoding::percent_decode_str(canonical_uri)
+			.decode_utf8()
+			.unwrap();
+		uri_encode(&key, false).into()
 	};
 
 	// Canonical query string from passed HeaderMap
@@ -400,7 +404,10 @@ pub fn verify_v4(
 ) -> Result<Key, Error> {
 	let scope_expected = compute_scope(&auth.date, &garage.config.s3_api.s3_region, service);
 	if auth.scope != scope_expected {
-		return Err(Error::AuthorizationHeaderMalformed(auth.scope.to_string()));
+		return Err(Error::AuthorizationHeaderMalformed {
+			unexpected: auth.scope.to_string(),
+			expected: scope_expected,
+		});
 	}
 
 	let key = garage

src/api/k2v/error.rs

@@ -20,8 +20,13 @@ pub enum Error {
 
 	// Category: cannot process
 	/// Authorization Header Malformed
-	#[error("Authorization header malformed, unexpected scope: {0}")]
-	AuthorizationHeaderMalformed(String),
+	#[error(
+		"Authorization header malformed, unexpected scope: '{unexpected}', expected: '{expected}'"
+	)]
+	AuthorizationHeaderMalformed {
+		unexpected: String,
+		expected: String,
+	},
 
 	/// The provided digest (checksum) value was invalid
 	#[error("Invalid digest: {0}")]
@@ -54,9 +59,13 @@ impl From<SignatureError> for Error {
 	fn from(err: SignatureError) -> Self {
 		match err {
 			SignatureError::Common(c) => Self::Common(c),
-			SignatureError::AuthorizationHeaderMalformed(c) => {
-				Self::AuthorizationHeaderMalformed(c)
-			}
+			SignatureError::AuthorizationHeaderMalformed {
+				unexpected,
+				expected,
+			} => Self::AuthorizationHeaderMalformed {
+				unexpected,
+				expected,
+			},
 			SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i),
 			SignatureError::InvalidDigest(d) => Self::InvalidDigest(d),
 		}
@@ -72,7 +81,7 @@ impl Error {
 			Error::Common(c) => c.aws_code(),
 			Error::NoSuchKey => "NoSuchKey",
 			Error::NotAcceptable(_) => "NotAcceptable",
-			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
+			Error::AuthorizationHeaderMalformed { .. } => "AuthorizationHeaderMalformed",
 			Error::InvalidBase64(_) => "InvalidBase64",
 			Error::InvalidUtf8Str(_) => "InvalidUtf8String",
 			Error::InvalidCausalityToken => "CausalityToken",
@@ -88,7 +97,7 @@ impl ApiError for Error {
 			Error::Common(c) => c.http_status_code(),
 			Error::NoSuchKey => StatusCode::NOT_FOUND,
 			Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE,
-			Error::AuthorizationHeaderMalformed(_)
+			Error::AuthorizationHeaderMalformed { .. }
 			| Error::InvalidBase64(_)
 			| Error::InvalidUtf8Str(_)
 			| Error::InvalidDigest(_)

src/api/s3/error.rs

@@ -31,8 +31,13 @@ pub enum Error {
 
 	// Category: cannot process
 	/// Authorization Header Malformed
-	#[error("Authorization header malformed, unexpected scope: {0}")]
-	AuthorizationHeaderMalformed(String),
+	#[error(
+		"Authorization header malformed, unexpected scope: '{unexpected}', expected: '{expected}'"
+	)]
+	AuthorizationHeaderMalformed {
+		unexpected: String,
+		expected: String,
+	},
 
 	/// The object requested don't exists
 	#[error("Key not found")]
@@ -121,9 +126,13 @@ impl From<SignatureError> for Error {
 	fn from(err: SignatureError) -> Self {
 		match err {
 			SignatureError::Common(c) => Self::Common(c),
-			SignatureError::AuthorizationHeaderMalformed(c) => {
-				Self::AuthorizationHeaderMalformed(c)
-			}
+			SignatureError::AuthorizationHeaderMalformed {
+				unexpected,
+				expected,
+			} => Self::AuthorizationHeaderMalformed {
+				unexpected,
+				expected,
+			},
 			SignatureError::InvalidUtf8Str(i) => Self::InvalidUtf8Str(i),
 			SignatureError::InvalidDigest(d) => Self::InvalidDigest(d),
 		}
@@ -146,7 +155,7 @@ impl Error {
 			Error::InvalidPart => "InvalidPart",
 			Error::InvalidPartOrder => "InvalidPartOrder",
 			Error::EntityTooSmall => "EntityTooSmall",
-			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
+			Error::AuthorizationHeaderMalformed { .. } => "AuthorizationHeaderMalformed",
 			Error::InvalidXml(_) => "MalformedXML",
 			Error::InvalidXmlDe(_) => "MalformedXML",
 			Error::InvalidXmlSe(_) => "InternalError",
@@ -172,7 +181,7 @@ impl ApiError for Error {
 			Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED,
 			Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE,
 			Error::InvalidXmlSe(_) => StatusCode::INTERNAL_SERVER_ERROR,
-			Error::AuthorizationHeaderMalformed(_)
+			Error::AuthorizationHeaderMalformed { .. }
 			| Error::InvalidPart
 			| Error::InvalidPartOrder
 			| Error::EntityTooSmall

src/garage/tests/s3/mod.rs

@@ -2,6 +2,7 @@ mod list;
 mod multipart;
 mod objects;
 mod presigned;
+mod signature_encoding;
 mod simple;
 mod ssec;
 mod streaming_signature;

src/garage/tests/s3/signature_encoding.rs

@@ -0,0 +1,72 @@
+use crate::common;
+
+use aws_sdk_s3::presigning::PresigningConfig;
+use bytes::Bytes;
+use http::{Request, StatusCode};
+use http_body_util::Full;
+use std::time::Duration;
+
+#[tokio::test]
+async fn test_signature_encoding() {
+	let ctx = common::context();
+	let bucket = ctx.create_bucket("signature-encoding");
+
+	let obj_key = "key@good~.txt";
+	let obj_content = "hello world of special characters";
+
+	let _put_obj_info = ctx
+		.client
+		.put_object()
+		.bucket(&bucket)
+		.key(obj_key)
+		.body(obj_content.as_bytes().to_vec().into())
+		.send()
+		.await
+		.expect("failed to put object");
+
+	let _get_obj = ctx
+		.client
+		.get_object()
+		.bucket(&bucket)
+		.key(obj_key)
+		.send()
+		.await
+		.expect("failed to get object");
+
+	let presign_config = PresigningConfig::builder()
+		.expires_in(Duration::from_secs(10))
+		.build()
+		.expect("failed to build presigning config");
+	let presigned_request = ctx
+		.client
+		.get_object()
+		.bucket(&bucket)
+		.key(obj_key)
+		.presigned(presign_config)
+		.await
+		.expect("failed to construct presigned request");
+
+	let altered_url = presigned_request
+		.uri()
+		.replace("%40", "@")
+		.replace("~", "%7E");
+
+	let client = ctx.custom_request.client();
+	let req_builder = Request::builder()
+		.method(presigned_request.method())
+		.uri(altered_url);
+	let req = presigned_request
+		.headers()
+		.fold(req_builder, |req_builder, (key, value)| {
+			req_builder.header(key, value)
+		})
+		.body(Full::new(Bytes::new()))
+		.expect("failed to construct request from presigned_request");
+
+	let res = client
+		.request(req)
+		.await
+		.expect("failed to execute presigned request");
+
+	assert_eq!(res.status(), StatusCode::OK);
+}