mirror of
https://github.com/rustfs/rustfs.git
synced 2026-11-11 20:54:09 +00:00
fix(s3): normalize leading-slash keys in bulk deletes (#9258)
* fix(s3): normalize leading-slash keys in bulk deletes * test(s3): align bulk delete reserved-prefix guard expectations
This commit is contained in:
cxymds
@@ -21,12 +21,15 @@
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::common::{RustFSTestEnvironment, init_logging};
|
||||
use crate::common::{RustFSTestEnvironment, admin_create_user, init_logging};
|
||||
use aws_sdk_s3::Client;
|
||||
use aws_sdk_s3::primitives::ByteStream;
|
||||
use aws_sdk_s3::types::{BucketVersioningStatus, Delete, ObjectIdentifier, VersioningConfiguration};
|
||||
use std::error::Error;
|
||||
use tracing::info;
|
||||
|
||||
type TestResult = Result<(), Box<dyn Error + Send + Sync>>;
|
||||
|
||||
async fn create_bucket(client: &Client, bucket: &str) -> Result<(), Box<dyn Error + Send + Sync>> {
|
||||
client.create_bucket().bucket(bucket).send().await?;
|
||||
Ok(())
|
||||
@@ -90,7 +93,7 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Duplicate and repeated slashes after a leading slash collapse MinIO-style.
|
||||
/// Duplicate and repeated slashes after a leading slash collapse MinIO-style across single and bulk operations.
|
||||
#[tokio::test]
|
||||
async fn test_put_object_with_duplicate_slashes_normalized() -> Result<(), Box<dyn Error + Send + Sync>> {
|
||||
init_logging();
|
||||
@@ -144,8 +147,258 @@ mod tests {
|
||||
"GET after DELETE with raw key must return HTTP 404, got {error:?}"
|
||||
);
|
||||
|
||||
assert_bulk_delete_normalizes_keys(&client).await?;
|
||||
assert_bulk_delete_authorizes_normalized_keys(&env, &client).await?;
|
||||
assert_bulk_delete_preserves_version_identity(&client).await?;
|
||||
|
||||
env.stop_server();
|
||||
info!("Test completed successfully");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_bulk_delete_normalizes_keys(client: &Client) -> TestResult {
|
||||
let bucket = "test-bulk-leading-slash";
|
||||
create_bucket(client, bucket).await?;
|
||||
let cases = [
|
||||
("/foo/bar", "foo/bar"),
|
||||
("//keyname", "keyname"),
|
||||
("/dir///sub//file", "dir/sub/file"),
|
||||
("/folder///", "folder/"),
|
||||
("/literal%2F+名", "literal%2F+名"),
|
||||
("plain", "plain"),
|
||||
];
|
||||
let invalid = ["///", "/dir/../keep", "dir//keep"];
|
||||
client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key("keep")
|
||||
.body(ByteStream::from_static(b"keep"))
|
||||
.send()
|
||||
.await?;
|
||||
client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key("dir/keep")
|
||||
.body(ByteStream::from_static(b"keep"))
|
||||
.send()
|
||||
.await?;
|
||||
|
||||
for quiet in [false, true] {
|
||||
for (raw, _) in cases {
|
||||
client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(raw)
|
||||
.body(ByteStream::from_static(b""))
|
||||
.send()
|
||||
.await?;
|
||||
}
|
||||
let listing = client.list_objects_v2().bucket(bucket).send().await?;
|
||||
let stored: Vec<_> = listing.contents().iter().filter_map(|object| object.key()).collect();
|
||||
let mut expected: Vec<_> = cases
|
||||
.iter()
|
||||
.map(|(_, normalized)| *normalized)
|
||||
.chain(["dir/keep", "keep"])
|
||||
.collect();
|
||||
expected.sort_unstable();
|
||||
assert_eq!(stored, expected, "PUT must store the exact normalized targets before bulk deletion");
|
||||
let objects = cases
|
||||
.iter()
|
||||
.map(|(raw, _)| *raw)
|
||||
.chain(invalid)
|
||||
.map(|key| ObjectIdentifier::builder().key(key).build())
|
||||
.collect::<Result<Vec<_>, _>>()?;
|
||||
let delete = Delete::builder().set_objects(Some(objects)).quiet(quiet).build()?;
|
||||
// A second identical batch also pins idempotent deletion of absent normalized keys.
|
||||
for _ in 0..2 {
|
||||
let output = client.delete_objects().bucket(bucket).delete(delete.clone()).send().await?;
|
||||
let errors: Vec<_> = output.errors().iter().map(|error| (error.key(), error.code())).collect();
|
||||
assert_eq!(
|
||||
errors,
|
||||
invalid.map(|key| (Some(key), Some("InvalidArgument"))),
|
||||
"unexpected per-key errors: {output:?}"
|
||||
);
|
||||
let deleted: Vec<_> = output.deleted().iter().filter_map(|object| object.key()).collect();
|
||||
let expected = if quiet {
|
||||
vec![]
|
||||
} else {
|
||||
cases.iter().map(|(raw, _)| *raw).collect()
|
||||
};
|
||||
assert_eq!(deleted, expected, "responses must retain request keys, including directory slashes");
|
||||
}
|
||||
let listing = client.list_objects_v2().bucket(bucket).send().await?;
|
||||
let remaining: Vec<_> = listing.contents().iter().filter_map(|object| object.key()).collect();
|
||||
assert_eq!(
|
||||
remaining,
|
||||
["dir/keep", "keep"],
|
||||
"bulk delete must remove normalized objects without resolving dot segments"
|
||||
);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_bulk_delete_authorizes_normalized_keys(env: &RustFSTestEnvironment, admin: &Client) -> TestResult {
|
||||
let bucket = "test-bulk-slash-policy";
|
||||
let user = "bulk-slash-user";
|
||||
admin_create_user(env, user, "bulkSlashTestSecret123!").await?;
|
||||
create_bucket(admin, bucket).await?;
|
||||
for key in ["allowed/item", "protected/item"] {
|
||||
admin
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.body(ByteStream::from_static(b"keep"))
|
||||
.send()
|
||||
.await?;
|
||||
}
|
||||
let policy = serde_json::json!({
|
||||
"Version": "2012-10-17",
|
||||
"Statement": [
|
||||
{"Effect": "Allow", "Principal": {"AWS": [user]}, "Action": ["s3:DeleteObject"],
|
||||
"Resource": [format!("arn:aws:s3:::{bucket}/*")]},
|
||||
{"Effect": "Deny", "Principal": {"AWS": [user]}, "Action": ["s3:DeleteObject"],
|
||||
"Resource": [format!("arn:aws:s3:::{bucket}/protected/*")]}
|
||||
]
|
||||
});
|
||||
admin
|
||||
.put_bucket_policy()
|
||||
.bucket(bucket)
|
||||
.policy(policy.to_string())
|
||||
.send()
|
||||
.await?;
|
||||
let client = env.create_s3_client_with_credentials(user, "bulkSlashTestSecret123!");
|
||||
let output = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(
|
||||
Delete::builder()
|
||||
.objects(ObjectIdentifier::builder().key("/allowed///item").build()?)
|
||||
.objects(ObjectIdentifier::builder().key("/protected///item").build()?)
|
||||
.build()?,
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(output.deleted().len(), 1, "allowed normalized key must be deleted: {output:?}");
|
||||
assert_eq!(output.deleted()[0].key(), Some("/allowed///item"));
|
||||
assert_eq!(output.errors().len(), 1, "normalized resource denial must apply: {output:?}");
|
||||
assert_eq!(output.errors()[0].key(), Some("/protected///item"));
|
||||
assert_eq!(output.errors()[0].code(), Some("AccessDenied"));
|
||||
let listing = admin.list_objects_v2().bucket(bucket).send().await?;
|
||||
let remaining: Vec<_> = listing.contents().iter().filter_map(|object| object.key()).collect();
|
||||
assert_eq!(remaining, ["protected/item"], "only the authorized object may be removed");
|
||||
let protected = admin.get_object().bucket(bucket).key("protected/item").send().await?;
|
||||
assert_eq!(protected.body.collect().await?.into_bytes().as_ref(), b"keep");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_bulk_delete_preserves_version_identity(client: &Client) -> TestResult {
|
||||
let bucket = "test-bulk-slash-versions";
|
||||
let raw = "/history///file";
|
||||
let normalized = "history/file";
|
||||
create_bucket(client, bucket).await?;
|
||||
client
|
||||
.put_bucket_versioning()
|
||||
.bucket(bucket)
|
||||
.versioning_configuration(
|
||||
VersioningConfiguration::builder()
|
||||
.status(BucketVersioningStatus::Enabled)
|
||||
.build(),
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
let first = client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(raw)
|
||||
.body(ByteStream::from_static(b"old"))
|
||||
.send()
|
||||
.await?;
|
||||
let first_id = first.version_id().ok_or("first PUT omitted version ID")?;
|
||||
let latest = client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(raw)
|
||||
.body(ByteStream::from_static(b"latest"))
|
||||
.send()
|
||||
.await?;
|
||||
let latest_id = latest.version_id().ok_or("latest PUT omitted version ID")?;
|
||||
let invalid = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(
|
||||
Delete::builder()
|
||||
.objects(ObjectIdentifier::builder().key("///").build()?)
|
||||
.build()?,
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert!(invalid.deleted().is_empty(), "slash-only keys must not delete a versioned object");
|
||||
assert_eq!(
|
||||
invalid.errors().len(),
|
||||
1,
|
||||
"slash-only keys must remain invalid in versioned buckets: {invalid:?}"
|
||||
);
|
||||
assert_eq!(invalid.errors()[0].key(), Some("///"));
|
||||
assert_eq!(invalid.errors()[0].code(), Some("InvalidArgument"));
|
||||
let deleted = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(
|
||||
Delete::builder()
|
||||
.objects(ObjectIdentifier::builder().key(raw).version_id(first_id).build()?)
|
||||
.build()?,
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert!(deleted.errors().is_empty(), "historical delete failed: {deleted:?}");
|
||||
assert_eq!(deleted.deleted().len(), 1);
|
||||
assert_eq!(deleted.deleted()[0].key(), Some(raw));
|
||||
assert_eq!(deleted.deleted()[0].version_id(), Some(first_id));
|
||||
let listing = client.list_object_versions().bucket(bucket).send().await?;
|
||||
assert_eq!(listing.versions().len(), 1, "only the latest data version should remain: {listing:?}");
|
||||
assert_eq!(listing.versions()[0].version_id(), Some(latest_id));
|
||||
|
||||
let deleted = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(
|
||||
Delete::builder()
|
||||
.objects(ObjectIdentifier::builder().key(raw).build()?)
|
||||
.build()?,
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert!(deleted.errors().is_empty(), "delete marker creation failed: {deleted:?}");
|
||||
assert_eq!(deleted.deleted().len(), 1);
|
||||
assert_eq!(deleted.deleted()[0].key(), Some(raw));
|
||||
assert_eq!(deleted.deleted()[0].delete_marker(), Some(true));
|
||||
let marker = deleted.deleted()[0]
|
||||
.delete_marker_version_id()
|
||||
.ok_or("missing delete marker ID")?;
|
||||
let listing = client.list_object_versions().bucket(bucket).send().await?;
|
||||
assert_eq!(listing.delete_markers().len(), 1);
|
||||
assert_eq!(listing.delete_markers()[0].key(), Some(normalized));
|
||||
assert_eq!(listing.delete_markers()[0].version_id(), Some(marker));
|
||||
|
||||
let deleted = client
|
||||
.delete_objects()
|
||||
.bucket(bucket)
|
||||
.delete(
|
||||
Delete::builder()
|
||||
.objects(ObjectIdentifier::builder().key(raw).version_id(marker).build()?)
|
||||
.build()?,
|
||||
)
|
||||
.send()
|
||||
.await?;
|
||||
assert!(deleted.errors().is_empty(), "explicit marker removal failed: {deleted:?}");
|
||||
assert_eq!(deleted.deleted().len(), 1);
|
||||
assert_eq!(deleted.deleted()[0].key(), Some(raw));
|
||||
assert_eq!(deleted.deleted()[0].version_id(), Some(marker));
|
||||
let listing = client.list_object_versions().bucket(bucket).send().await?;
|
||||
assert!(listing.delete_markers().is_empty());
|
||||
let restored = client.get_object().bucket(bucket).key(normalized).send().await?;
|
||||
assert_eq!(restored.version_id(), Some(latest_id));
|
||||
assert_eq!(restored.body.collect().await?.into_bytes().as_ref(), b"latest");
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@
|
||||
use super::*;
|
||||
use crate::app::storage_api::object_usecase::DeleteObjectCondition;
|
||||
use s3s::dto::ObjectIdentifier;
|
||||
use std::borrow::Cow;
|
||||
use std::collections::HashSet;
|
||||
|
||||
async fn authorize_recursive_delete<T>(
|
||||
@@ -81,6 +82,19 @@ async fn authorize_recursive_delete<T>(
|
||||
result
|
||||
}
|
||||
|
||||
// XML body keys bypass s3s's URL-path normalization. Match its private slash
|
||||
// normalizer without URL-decoding or resolving dot segments in client keys.
|
||||
fn normalize_delete_object_key(key: &str) -> Cow<'_, str> {
|
||||
if !key.starts_with('/') {
|
||||
return Cow::Borrowed(key);
|
||||
}
|
||||
let mut normalized = key.split('/').filter(|part| !part.is_empty()).collect::<Vec<_>>().join("/");
|
||||
if key.ends_with('/') {
|
||||
normalized.push('/');
|
||||
}
|
||||
Cow::Owned(normalized)
|
||||
}
|
||||
|
||||
fn successful_delete_audit_objects(
|
||||
delete: &s3s::dto::Delete,
|
||||
successful_results: impl IntoIterator<Item = bool>,
|
||||
@@ -90,7 +104,9 @@ fn successful_delete_audit_objects(
|
||||
.iter()
|
||||
.zip(successful_results)
|
||||
.filter(|(_, successful)| *successful)
|
||||
.map(|(requested, _)| AuditObjectVersion::new(requested.key.clone(), requested.version_id.clone()))
|
||||
.map(|(requested, _)| {
|
||||
AuditObjectVersion::new(normalize_delete_object_key(&requested.key).into_owned(), requested.version_id.clone())
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
@@ -590,7 +606,10 @@ impl DefaultObjectUsecase {
|
||||
// Per-key denials are audited once per denied action, in aggregate, after the loop.
|
||||
let mut bulk_denials = crate::storage::access::BulkDenialAudit::start(&mut req)?;
|
||||
for (idx, obj_id) in delete.objects.iter().enumerate() {
|
||||
if let Err(err) = validate_object_key(&obj_id.key, "DeleteObjects") {
|
||||
// Authorization and every storage side effect must address the same
|
||||
// normalized object; retain the request key only for response entries.
|
||||
let key = normalize_delete_object_key(&obj_id.key);
|
||||
if let Err(err) = validate_object_key(&key, "DeleteObjects") {
|
||||
delete_results[idx].error = Some(s3s::dto::Error {
|
||||
code: Some(err.code().as_str().to_string()),
|
||||
key: Some(obj_id.key.clone()),
|
||||
@@ -623,7 +642,7 @@ impl DefaultObjectUsecase {
|
||||
{
|
||||
let req_info = req_info_mut(&mut req)?;
|
||||
req_info.bucket = Some(bucket.clone());
|
||||
req_info.object = Some(obj_id.key.clone());
|
||||
req_info.object = Some(key.to_string());
|
||||
req_info.version_id = version_id.clone();
|
||||
}
|
||||
|
||||
@@ -670,7 +689,7 @@ impl DefaultObjectUsecase {
|
||||
}
|
||||
}
|
||||
|
||||
if let Err(err) = validate_table_catalog_object_mutation(&bucket, &obj_id.key).await {
|
||||
if let Err(err) = validate_table_catalog_object_mutation(&bucket, &key).await {
|
||||
delete_results[idx].error = Some(s3s::dto::Error {
|
||||
code: Some("InvalidRequest".to_string()),
|
||||
key: Some(obj_id.key.clone()),
|
||||
@@ -680,9 +699,9 @@ impl DefaultObjectUsecase {
|
||||
continue;
|
||||
}
|
||||
|
||||
let synthetic_version_id = delete_condition.is_none() && version_id.is_none() && is_dir_object(&obj_id.key);
|
||||
let synthetic_version_id = delete_condition.is_none() && version_id.is_none() && is_dir_object(&key);
|
||||
let object = ObjectToDelete {
|
||||
object_name: obj_id.key.clone(),
|
||||
object_name: key.into_owned(),
|
||||
version_id: version_uuid,
|
||||
delete_condition,
|
||||
synthetic_version_id,
|
||||
@@ -910,15 +929,16 @@ impl DefaultObjectUsecase {
|
||||
let deleted = (!delete.quiet.unwrap_or_default()).then(|| {
|
||||
delete_results
|
||||
.iter()
|
||||
.filter_map(|result| result.delete_object.as_ref().map(|object| (result, object)))
|
||||
.map(|(result, object)| DeletedObject {
|
||||
.enumerate()
|
||||
.filter_map(|(idx, result)| result.delete_object.as_ref().map(|object| (idx, result, object)))
|
||||
.map(|(idx, result, object)| DeletedObject {
|
||||
delete_marker: { if object.delete_marker { Some(true) } else { None } },
|
||||
delete_marker_version_id: delete_marker_response_version_id(
|
||||
object.delete_marker_version_id,
|
||||
object.delete_marker && result.version_suspended && result.requested_version_id.is_none(),
|
||||
result.synthetic_version_id,
|
||||
),
|
||||
key: Some(object.object_name.clone()),
|
||||
key: Some(delete.objects[idx].key.clone()),
|
||||
version_id: delete_entry_response_version_id(
|
||||
object.version_id,
|
||||
object.delete_marker,
|
||||
@@ -936,7 +956,12 @@ impl DefaultObjectUsecase {
|
||||
|
||||
let errors = delete_results
|
||||
.iter()
|
||||
.filter_map(|v| v.error.clone())
|
||||
.enumerate()
|
||||
.filter_map(|(idx, result)| {
|
||||
let mut error = result.error.clone()?;
|
||||
error.key = Some(delete.objects[idx].key.clone());
|
||||
Some(error)
|
||||
})
|
||||
.collect::<Vec<s3s::dto::Error>>();
|
||||
let output = DeleteObjectsOutput {
|
||||
deleted,
|
||||
@@ -2818,11 +2843,57 @@ mod tests {
|
||||
assert_eq!(err.message(), Some("Not init"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn delete_objects_key_normalization_matches_s3s_path_keys() {
|
||||
use s3s::path::{
|
||||
parse_path_style_with_validation_and_normalization, parse_virtual_hosted_style_with_validation_and_normalization,
|
||||
};
|
||||
use s3s::validation::AwsNameValidation;
|
||||
|
||||
for key in [
|
||||
"plain",
|
||||
"/a/b",
|
||||
"//a/b",
|
||||
"/dir///sub//file",
|
||||
"/dir///",
|
||||
"/",
|
||||
"///",
|
||||
"dir//file",
|
||||
"/literal%2F+名",
|
||||
"/dir/../file",
|
||||
"/dir/./file",
|
||||
"/dir\\file",
|
||||
"/a\0b",
|
||||
] {
|
||||
let normalized = normalize_delete_object_key(key);
|
||||
let path = parse_path_style_with_validation_and_normalization(
|
||||
&format!("/test-bucket/{key}"),
|
||||
&AwsNameValidation::new(),
|
||||
true,
|
||||
)
|
||||
.expect("path-style key should parse before storage validation");
|
||||
let virtual_host = parse_virtual_hosted_style_with_validation_and_normalization(
|
||||
Some("test-bucket"),
|
||||
&format!("/{key}"),
|
||||
&AwsNameValidation::new(),
|
||||
true,
|
||||
)
|
||||
.expect("virtual-hosted key should parse before storage validation");
|
||||
assert_eq!(Some(normalized.as_ref()), path.get_object_key(), "path-style mismatch for {key:?}");
|
||||
assert_eq!(
|
||||
Some(normalized.as_ref()),
|
||||
virtual_host.get_object_key(),
|
||||
"virtual-hosted mismatch for {key:?}"
|
||||
);
|
||||
}
|
||||
assert_eq!(normalize_delete_object_key(""), "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn delete_objects_audit_details_include_only_successful_request_entries() {
|
||||
let requested = vec![
|
||||
ObjectIdentifier {
|
||||
key: "first-key".to_string(),
|
||||
key: "/first-key".to_string(),
|
||||
version_id: None,
|
||||
..Default::default()
|
||||
},
|
||||
@@ -2832,7 +2903,7 @@ mod tests {
|
||||
..Default::default()
|
||||
},
|
||||
ObjectIdentifier {
|
||||
key: "versioned-key".to_string(),
|
||||
key: "//versioned-key".to_string(),
|
||||
version_id: Some("requested-version".to_string()),
|
||||
..Default::default()
|
||||
},
|
||||
|
||||
@@ -14460,8 +14460,8 @@ fn object_mutation_entrypoints_call_reserved_prefix_guard() {
|
||||
assert_ordered(
|
||||
method(delete, "fn execute_delete_objects_owned("),
|
||||
&[
|
||||
"validate_object_key(&obj_id.key, \"DeleteObjects\")",
|
||||
"if let Err(err) = validate_table_catalog_object_mutation(&bucket, &obj_id.key).await {",
|
||||
"validate_object_key(&key, \"DeleteObjects\")",
|
||||
"if let Err(err) = validate_table_catalog_object_mutation(&bucket, &key).await {",
|
||||
"continue;\n }",
|
||||
"authorized_deletes.push(AuthorizedDelete { idx, object });",
|
||||
],
|
||||
|
||||
Reference in New Issue
Block a user