diff --git a/crates/e2e_test/src/delete_object_no_content_length_test.rs b/crates/e2e_test/src/delete_object_no_content_length_test.rs new file mode 100644 index 000000000..76bee5e3d --- /dev/null +++ b/crates/e2e_test/src/delete_object_no_content_length_test.rs @@ -0,0 +1,165 @@ +// Copyright 2024 RustFS Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Regression coverage for signed `DELETE Object?versionId` requests sent with +//! no body and no `Content-Length` header. + +#[cfg(test)] +mod tests { + use crate::common::{RustFSTestEnvironment, init_logging}; + use aws_sdk_s3::primitives::ByteStream; + use aws_sdk_s3::types::{BucketVersioningStatus, VersioningConfiguration}; + use http::header::HOST; + use rustfs_signer::constants::UNSIGNED_PAYLOAD; + use rustfs_signer::sign_v4; + use s3s::Body; + use serial_test::serial; + use std::error::Error; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpStream; + use tokio::time::{Duration, timeout}; + use tracing::info; + + const RAW_RESPONSE_TIMEOUT: Duration = Duration::from_secs(10); + + fn parse_status(raw_response: &str) -> Option { + raw_response.lines().next()?.split_whitespace().nth(1)?.parse().ok() + } + + fn response_body(raw_response: &str) -> &str { + raw_response + .split_once("\r\n\r\n") + .map(|(_, body)| body) + .or_else(|| raw_response.split_once("\n\n").map(|(_, body)| body)) + .unwrap_or("") + } + + async fn signed_delete_without_content_length( + url: &str, + access_key: &str, + secret_key: &str, + ) -> Result> { + let uri = url.parse::()?; + let authority = uri.authority().ok_or("request URL missing authority")?.to_string(); + let path_and_query = uri.path_and_query().ok_or("request URL missing path")?.as_str().to_string(); + + let request = http::Request::builder() + .method(http::Method::DELETE) + .uri(uri) + .header(HOST, authority.clone()) + .header("x-amz-content-sha256", UNSIGNED_PAYLOAD) + .body(Body::empty())?; + + let signed = sign_v4(request, 0, access_key, secret_key, "", "us-east-1"); + + let mut raw_request = format!("DELETE {path_and_query} HTTP/1.1\r\nHost: {authority}\r\nConnection: close\r\n"); + for (name, value) in signed.headers() { + if name.as_str().eq_ignore_ascii_case("host") || name.as_str().eq_ignore_ascii_case("content-length") { + continue; + } + raw_request.push_str(name.as_str()); + raw_request.push_str(": "); + raw_request.push_str(value.to_str()?); + raw_request.push_str("\r\n"); + } + raw_request.push_str("\r\n"); + + assert!( + !raw_request.to_ascii_lowercase().contains("\r\ncontent-length:"), + "raw regression request must omit Content-Length; request was:\n{raw_request}" + ); + + let mut stream = TcpStream::connect(&authority).await?; + stream.write_all(raw_request.as_bytes()).await?; + stream.flush().await?; + + let mut response = Vec::new(); + timeout(RAW_RESPONSE_TIMEOUT, stream.read_to_end(&mut response)) + .await + .map_err(|_| std::io::Error::new(std::io::ErrorKind::TimedOut, "timed out reading raw DELETE response"))??; + Ok(String::from_utf8_lossy(&response).into_owned()) + } + + #[tokio::test] + #[serial] + async fn test_delete_object_version_without_content_length_succeeds() -> Result<(), Box> { + init_logging(); + info!("๐Ÿงช TEST: signed DELETE Object?versionId succeeds without Content-Length"); + + let mut env = RustFSTestEnvironment::new().await?; + env.start_rustfs_server(vec![]).await?; + + let client = env.create_s3_client(); + let bucket = "delete-version-no-content-length"; + let key = "versioned-delete-target.txt"; + + client.create_bucket().bucket(bucket).send().await?; + + client + .put_object() + .bucket(bucket) + .key(key) + .body(ByteStream::from_static(b"delete me after versioning is enabled")) + .send() + .await?; + + client + .put_bucket_versioning() + .bucket(bucket) + .versioning_configuration( + VersioningConfiguration::builder() + .status(BucketVersioningStatus::Enabled) + .build(), + ) + .send() + .await?; + + let listed_versions = client.list_object_versions().bucket(bucket).prefix(key).send().await?; + let version_id = listed_versions + .versions() + .iter() + .find(|version| version.key() == Some(key)) + .and_then(|version| version.version_id()) + .ok_or("ListObjectVersions did not return the pre-versioning object version")?; + + let url = format!("{}/{}/{}?versionId={}", env.url, bucket, key, urlencoding::encode(version_id)); + let raw_response = signed_delete_without_content_length(&url, &env.access_key, &env.secret_key).await?; + info!("raw DELETE response:\n{}", raw_response); + + assert_eq!( + parse_status(&raw_response), + Some(204), + "DELETE Object?versionId without Content-Length should succeed, got:\n{raw_response}" + ); + assert!( + !raw_response.contains("MissingContentLength"), + "DELETE Object?versionId without Content-Length regressed to MissingContentLength: {raw_response}" + ); + assert!( + response_body(&raw_response).trim().is_empty(), + "successful DELETE Object?versionId should not return an error body: {raw_response}" + ); + + let get_deleted_version = client + .get_object() + .bucket(bucket) + .key(key) + .version_id(version_id) + .send() + .await; + assert!(get_deleted_version.is_err(), "explicitly deleted version should no longer be readable"); + + Ok(()) + } +} diff --git a/crates/e2e_test/src/lib.rs b/crates/e2e_test/src/lib.rs index d56914c27..f855c4486 100644 --- a/crates/e2e_test/src/lib.rs +++ b/crates/e2e_test/src/lib.rs @@ -74,6 +74,10 @@ mod compression_test; #[cfg(test)] mod delete_objects_versioning_test; +// Regression test for signed DELETE Object?versionId requests without Content-Length. +#[cfg(test)] +mod delete_object_no_content_length_test; + // Regression test for Issue #2252: ListObjectVersions misses newest version after put -> delete -> put #[cfg(test)] mod list_object_versions_regression_test; diff --git a/rustfs/src/server/http.rs b/rustfs/src/server/http.rs index 1a058b193..ef046f40f 100644 --- a/rustfs/src/server/http.rs +++ b/rustfs/src/server/http.rs @@ -22,7 +22,7 @@ use crate::server::{ compress::{CompressionConfig, PathAwareCompressionPredicate, PathCategoryInjectionLayer}, hybrid::hybrid, layer::{ - AdminChunkedContentLengthCompatLayer, BodylessStatusFixLayer, ConditionalCorsLayer, HeadRequestBodyFixLayer, + BodylessStatusFixLayer, ConditionalCorsLayer, EmptyBodyContentLengthCompatLayer, HeadRequestBodyFixLayer, ObjectAttributesEtagFixLayer, PublicHealthEndpointLayer, RedirectLayer, RequestContextLayer, S3ErrorMessageCompatLayer, }, tls_material::{TlsAcceptorHolder, TlsHandshakeFailureKind, TlsMaterialSnapshot, spawn_reload_loop}, @@ -680,7 +680,7 @@ fn process_connection( // 3. TrustedProxyLayer โ€” conditional, parses X-Forwarded-For // 4. SetRequestIdLayer โ€” generates X-Request-ID // 5. RequestContextLayer โ€” creates RequestContext in extensions - // 6. AdminChunkedContentLengthCompatLayer โ€” admin API compat + // 6. EmptyBodyContentLengthCompatLayer โ€” adds Content-Length: 0 for known empty-body API routes // 7. CatchPanicLayer โ€” panic โ†’ 500 // 8. ReadinessGateLayer โ€” blocks until ready // 9. KeystoneAuthLayer โ€” X-Auth-Token validation @@ -711,7 +711,7 @@ fn process_connection( .option_layer(trusted_proxy_layer) .layer(SetRequestIdLayer::x_request_id(MakeRequestUuid)) .layer(RequestContextLayer) - .layer(AdminChunkedContentLengthCompatLayer) + .layer(EmptyBodyContentLengthCompatLayer) .layer(CatchPanicLayer::new()) // CRITICAL: Insert ReadinessGateLayer before business logic // This stops requests from hitting IAMAuth or Storage if they are not ready. diff --git a/rustfs/src/server/layer.rs b/rustfs/src/server/layer.rs index a6db2af38..3c02d0dfc 100644 --- a/rustfs/src/server/layer.rs +++ b/rustfs/src/server/layer.rs @@ -220,23 +220,29 @@ where } } +/// Adds `Content-Length: 0` for routes whose requests are known to carry no +/// body, but where some S3-compatible clients omit the header entirely. +/// +/// The normalization runs before authentication so downstream request +/// validation sees an explicit empty body length without requiring every +/// handler to special-case absent `Content-Length`. #[derive(Clone)] -pub struct AdminChunkedContentLengthCompatLayer; +pub struct EmptyBodyContentLengthCompatLayer; -impl Layer for AdminChunkedContentLengthCompatLayer { - type Service = AdminChunkedContentLengthCompatService; +impl Layer for EmptyBodyContentLengthCompatLayer { + type Service = EmptyBodyContentLengthCompatService; fn layer(&self, inner: S) -> Self::Service { - AdminChunkedContentLengthCompatService { inner } + EmptyBodyContentLengthCompatService { inner } } } #[derive(Clone)] -pub struct AdminChunkedContentLengthCompatService { +pub struct EmptyBodyContentLengthCompatService { inner: S, } -impl Service> for AdminChunkedContentLengthCompatService +impl Service> for EmptyBodyContentLengthCompatService where S: Service, Response = Response> + Clone + Send + 'static, S::Future: Send + 'static, @@ -253,7 +259,7 @@ where } fn call(&mut self, mut req: HttpRequest) -> Self::Future { - if should_force_zero_content_length_for_admin_empty_body(&req) { + if should_force_zero_content_length_for_empty_body_route(&req) { req.headers_mut() .insert(http::header::CONTENT_LENGTH, HeaderValue::from_static("0")); } @@ -263,8 +269,20 @@ where } } -fn should_force_zero_content_length_for_admin_empty_body(req: &HttpRequest) -> bool { - is_empty_body_admin_path(req.method(), req.uri().path()) && !req.headers().contains_key(http::header::CONTENT_LENGTH) +fn should_force_zero_content_length_for_empty_body_route(req: &HttpRequest) -> bool { + if req.headers().contains_key(http::header::CONTENT_LENGTH) { + return false; + } + + if req.headers().contains_key(http::header::TRANSFER_ENCODING) { + return false; + } + + if is_empty_body_admin_path(req.method(), req.uri().path()) { + return true; + } + + is_empty_body_s3_path(req.method(), req.uri()) } fn is_empty_body_admin_path(method: &Method, path: &str) -> bool { @@ -291,6 +309,10 @@ fn is_empty_body_admin_path(method: &Method, path: &str) -> bool { } } +fn is_empty_body_s3_path(method: &Method, uri: &http::Uri) -> bool { + *method == Method::DELETE && ConditionalCorsLayer::is_s3_path(uri.path()) +} + #[derive(Clone)] pub struct S3ErrorMessageCompatLayer; @@ -1218,7 +1240,7 @@ mod tests { .body(()) .expect("request"); - assert!(should_force_zero_content_length_for_admin_empty_body(&request)); + assert!(should_force_zero_content_length_for_empty_body_route(&request)); } #[test] @@ -1238,17 +1260,17 @@ mod tests { let request = Request::builder().method(Method::POST).uri(path).body(()).expect("request"); assert!( - should_force_zero_content_length_for_admin_empty_body(&request), + should_force_zero_content_length_for_empty_body_route(&request), "{path} should force Content-Length: 0" ); } } #[tokio::test] - async fn admin_empty_body_post_layer_inserts_zero_content_length() { + async fn empty_body_layer_inserts_zero_content_length_for_admin_post() { let capture = HeaderCaptureService::default(); let headers = capture.headers(); - let mut service = AdminChunkedContentLengthCompatLayer.layer(capture); + let mut service = EmptyBodyContentLengthCompatLayer.layer(capture); let request = Request::builder() .method(Method::POST) .uri("/rustfs/admin/v3/rebalance/start") @@ -1261,6 +1283,88 @@ mod tests { assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0"); } + #[tokio::test] + async fn empty_body_layer_inserts_zero_content_length_for_admin_put() { + let capture = HeaderCaptureService::default(); + let headers = capture.headers(); + let mut service = EmptyBodyContentLengthCompatLayer.layer(capture); + let request = Request::builder() + .method(Method::PUT) + .uri("/rustfs/admin/v3/set-group-status?group=test&status=enabled") + .body(()) + .expect("request"); + + let _ = service.call(request).await.expect("service call"); + + let headers = headers.lock().expect("captured headers").take().expect("captured headers"); + assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0"); + } + + #[tokio::test] + async fn empty_body_layer_preserves_admin_transfer_encoding_without_content_length() { + let capture = HeaderCaptureService::default(); + let headers = capture.headers(); + let mut service = EmptyBodyContentLengthCompatLayer.layer(capture); + let request = Request::builder() + .method(Method::PUT) + .uri("/rustfs/admin/v3/set-group-status?group=test&status=enabled") + .header(http::header::TRANSFER_ENCODING, "chunked") + .body(()) + .expect("request"); + + let _ = service.call(request).await.expect("service call"); + + let headers = headers.lock().expect("captured headers").take().expect("captured headers"); + assert!(headers.get(http::header::CONTENT_LENGTH).is_none()); + assert_eq!(headers.get(http::header::TRANSFER_ENCODING).unwrap(), "chunked"); + } + + #[test] + fn s3_delete_object_version_without_content_length_is_normalized() { + let request = Request::builder() + .method(Method::DELETE) + .uri("/bucket/object.txt?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd") + .body(()) + .expect("request"); + + assert!(should_force_zero_content_length_for_empty_body_route(&request)); + } + + #[tokio::test] + async fn empty_body_layer_inserts_zero_content_length_for_s3_delete_object_version() { + let capture = HeaderCaptureService::default(); + let headers = capture.headers(); + let mut service = EmptyBodyContentLengthCompatLayer.layer(capture); + let request = Request::builder() + .method(Method::DELETE) + .uri("/bucket/object.txt?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd") + .body(()) + .expect("request"); + + let _ = service.call(request).await.expect("service call"); + + let headers = headers.lock().expect("captured headers").take().expect("captured headers"); + assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "0"); + } + + #[tokio::test] + async fn empty_body_layer_preserves_explicit_content_length_header() { + let capture = HeaderCaptureService::default(); + let headers = capture.headers(); + let mut service = EmptyBodyContentLengthCompatLayer.layer(capture); + let request = Request::builder() + .method(Method::PUT) + .uri("/minio/admin/v3/set-group-status?group=test&status=enabled") + .header(http::header::CONTENT_LENGTH, "7") + .body(()) + .expect("request"); + + let _ = service.call(request).await.expect("service call"); + + let headers = headers.lock().expect("captured headers").take().expect("captured headers"); + assert_eq!(headers.get(http::header::CONTENT_LENGTH).unwrap(), "7"); + } + #[test] fn admin_request_with_explicit_content_length_is_left_unchanged() { let request = Request::builder() @@ -1270,18 +1374,75 @@ mod tests { .body(()) .expect("request"); - assert!(!should_force_zero_content_length_for_admin_empty_body(&request)); + assert!(!should_force_zero_content_length_for_empty_body_route(&request)); } #[test] - fn non_admin_chunked_put_is_not_normalized() { + fn s3_put_object_is_not_normalized() { let request = Request::builder() .method(Method::PUT) .uri("/bucket/object") .body(()) .expect("request"); - assert!(!should_force_zero_content_length_for_admin_empty_body(&request)); + assert!(!should_force_zero_content_length_for_empty_body_route(&request)); + } + + #[test] + fn s3_delete_bucket_without_content_length_is_normalized() { + let request = Request::builder() + .method(Method::DELETE) + .uri("/bucket") + .body(()) + .expect("request"); + + assert!(should_force_zero_content_length_for_empty_body_route(&request)); + } + + #[test] + fn s3_delete_object_without_version_id_is_normalized() { + let request = Request::builder() + .method(Method::DELETE) + .uri("/bucket/object") + .body(()) + .expect("request"); + + assert!(should_force_zero_content_length_for_empty_body_route(&request)); + } + + #[test] + fn s3_delete_with_transfer_encoding_is_not_normalized() { + let request = Request::builder() + .method(Method::DELETE) + .uri("/bucket/object?versionId=3HL4kqtJlcpXrof3Gj0OmxJnVBH40Nrjfkd") + .header(http::header::TRANSFER_ENCODING, "chunked") + .body(()) + .expect("request"); + + assert!(!should_force_zero_content_length_for_empty_body_route(&request)); + } + + #[test] + fn non_s3_delete_paths_are_not_normalized() { + let paths = [ + "/minio/admin/v3/pools/cancel?versionId=unused", + "/rustfs/admin/v3/pools/cancel?versionId=unused", + "/rustfs/rpc/read_file_stream?versionId=unused", + "/rustfs/console/index.html?versionId=unused", + "/health?versionId=unused", + "/health/ready?versionId=unused", + "/profile/cpu?versionId=unused", + "/profile/memory?versionId=unused", + ]; + + for path in paths { + let request = Request::builder().method(Method::DELETE).uri(path).body(()).expect("request"); + + assert!( + !should_force_zero_content_length_for_empty_body_route(&request), + "{path} should not force Content-Length: 0" + ); + } } #[test] @@ -1292,7 +1453,7 @@ mod tests { .body(()) .expect("request"); - assert!(!should_force_zero_content_length_for_admin_empty_body(&request)); + assert!(!should_force_zero_content_length_for_empty_body_route(&request)); } #[test]