mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-06 05:17:42 +00:00
fix(sse): rewrite data when a same-key copy changes encryption (#5618)
A same-name CopyObject marks the operation `metadata_only`, which lets the store layer rewrite `xl.meta` in place and leave the data blocks untouched. The handler independently strips the source encryption metadata and calls `sse_encryption`, which mints a *fresh* DEK. On an unversioned bucket both happen at once, so the object ends up with a new DEK sitting beside ciphertext sealed under the old one, and can never be decrypted again. The mirror case is silent: an encrypted source copied without any destination SSE keeps its ciphertext while losing the key metadata, so GET returns raw ciphertext as if it were plaintext, with HTTP 200 and no error anywhere. Keep `metadata_only` off whenever either side of the copy is encrypted, so the store layer performs a full read/write rewrite through `put_object`. This is the same resolution the versioned historical-restore path already uses for this risk (issue #4238), and it matches MinIO's `isSourceEncrypted || isTargetEncrypted -> metadataOnly = false` guard in CopyObjectHandler. The target half of the predicate deliberately tests `effective_sse` rather than the request headers MinIO inspects: `effective_sse` also resolves the bucket default-encryption rule, and `sse_encryption` mints a DEK from that resolved value. A header-only check would miss a same-key copy performed under a bucket default rule. The source half reuses `ObjectInfo::is_encrypted` so a future encryption flavour is covered here as soon as it is recognised there. Versioned buckets were already safe: that path falls through to `put_object` regardless of `metadata_only`. RestoreObject also sets `metadata_only` but only appends restore keys and never re-derives a DEK, so it is unaffected.
This commit is contained in:
Zhengchao An
@@ -0,0 +1,351 @@
|
||||
// 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 test: a same-key CopyObject that only rewrites metadata must never re-key a
|
||||
//! managed-SSE (SSE-S3 / SSE-KMS) object.
|
||||
//!
|
||||
//! On an **unversioned** bucket the handler marks a same-name copy `metadata_only`, and the
|
||||
//! store layer then updates `xl.meta` in place without touching the data blocks. The handler
|
||||
//! nevertheless strips the source encryption metadata and generates a *fresh* DEK for the
|
||||
//! destination. Combining the two writes "new DEK + old ciphertext": the object is permanently
|
||||
//! undecryptable. The fix forces a full data rewrite whenever the copy re-derives managed
|
||||
//! encryption material, so the stored bytes always match the key metadata beside them.
|
||||
//!
|
||||
//! Companion to `copy_object_version_restore_sse_test` (issue #4238), which pins the same
|
||||
//! invariant for the versioned historical-restore path.
|
||||
|
||||
use super::common::{LocalKMSTestEnvironment, create_key_with_specific_id};
|
||||
use crate::common::init_logging;
|
||||
use aws_sdk_s3::primitives::ByteStream;
|
||||
use aws_sdk_s3::types::{
|
||||
MetadataDirective, ServerSideEncryption, ServerSideEncryptionByDefault, ServerSideEncryptionConfiguration,
|
||||
ServerSideEncryptionRule,
|
||||
};
|
||||
use serial_test::serial;
|
||||
use tracing::info;
|
||||
|
||||
#[tokio::test]
|
||||
#[serial]
|
||||
async fn test_metadata_replace_self_copy_of_sse_object_stays_decryptable() {
|
||||
init_logging();
|
||||
info!("same-key CopyObject with REPLACE metadata must not re-key an SSE-S3 object");
|
||||
|
||||
let mut kms_env = LocalKMSTestEnvironment::new().await.expect("failed to create local KMS env");
|
||||
let default_key_id = "rustfs-e2e-test-default-key";
|
||||
let keys_dir = kms_env.kms_keys_dir.clone();
|
||||
create_key_with_specific_id(&keys_dir, default_key_id)
|
||||
.await
|
||||
.expect("failed to create local KMS key");
|
||||
kms_env
|
||||
.base_env
|
||||
.start_rustfs_server_with_env(
|
||||
vec![
|
||||
"--kms-enable",
|
||||
"--kms-backend",
|
||||
"local",
|
||||
"--kms-key-dir",
|
||||
&keys_dir,
|
||||
"--kms-default-key-id",
|
||||
default_key_id,
|
||||
],
|
||||
&[("RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS", "true")],
|
||||
)
|
||||
.await
|
||||
.expect("failed to start RustFS with local KMS");
|
||||
tokio::time::sleep(tokio::time::Duration::from_secs(3)).await;
|
||||
|
||||
let client = kms_env.base_env.create_s3_client();
|
||||
// Deliberately an UNVERSIONED bucket: that is the branch where the store layer can service
|
||||
// the self-copy as a pure metadata update.
|
||||
let bucket = "copy-object-self-copy-sse-test";
|
||||
let key = "secrets/report.txt";
|
||||
|
||||
client
|
||||
.create_bucket()
|
||||
.bucket(bucket)
|
||||
.send()
|
||||
.await
|
||||
.expect("failed to create bucket");
|
||||
|
||||
// Content long enough that a truncated/garbled decrypt cannot coincidentally match.
|
||||
let content = b"encrypted payload that must survive a metadata-only self copy -- 0123456789";
|
||||
let put = client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.content_type("text/plain; charset=utf-8")
|
||||
.metadata("stage", "before")
|
||||
.server_side_encryption(ServerSideEncryption::Aes256)
|
||||
.body(ByteStream::from_static(content))
|
||||
.send()
|
||||
.await
|
||||
.expect("PUT failed");
|
||||
assert_eq!(put.server_side_encryption(), Some(&ServerSideEncryption::Aes256));
|
||||
|
||||
// Copy the object onto itself, replacing user metadata. This is the `mc cp --attr` /
|
||||
// "edit metadata in place" shape that AWS supports on an existing object.
|
||||
let copy_out = client
|
||||
.copy_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.copy_source(format!("{bucket}/{key}"))
|
||||
.metadata_directive(MetadataDirective::Replace)
|
||||
.content_type("text/plain; charset=utf-8")
|
||||
.metadata("stage", "after")
|
||||
.server_side_encryption(ServerSideEncryption::Aes256)
|
||||
.send()
|
||||
.await
|
||||
.expect("same-key CopyObject with REPLACE metadata must succeed");
|
||||
assert_eq!(copy_out.server_side_encryption(), Some(&ServerSideEncryption::Aes256));
|
||||
|
||||
// The object must still decrypt to the original plaintext. Before the fix the stored
|
||||
// ciphertext was left untouched while the metadata carried a brand-new DEK, so this GET
|
||||
// either failed outright or returned garbage.
|
||||
let get = client
|
||||
.get_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.send()
|
||||
.await
|
||||
.expect("GET after self-copy failed: the object was re-keyed without rewriting the ciphertext");
|
||||
assert_eq!(get.server_side_encryption(), Some(&ServerSideEncryption::Aes256));
|
||||
assert_eq!(
|
||||
get.metadata().and_then(|m| m.get("stage")),
|
||||
Some(&"after".to_string()),
|
||||
"REPLACE metadata must take effect"
|
||||
);
|
||||
let body = get.body.collect().await.expect("collect body").into_bytes();
|
||||
assert_eq!(
|
||||
body.as_ref(),
|
||||
content,
|
||||
"object must still decrypt to the original plaintext after a metadata-only self copy"
|
||||
);
|
||||
|
||||
kms_env.base_env.stop_server();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[serial]
|
||||
async fn test_metadata_replace_self_copy_dropping_sse_rewrites_plaintext() {
|
||||
init_logging();
|
||||
info!("same-key CopyObject that drops SSE must rewrite the data, not orphan the ciphertext");
|
||||
|
||||
let mut kms_env = LocalKMSTestEnvironment::new().await.expect("failed to create local KMS env");
|
||||
let default_key_id = "rustfs-e2e-test-default-key";
|
||||
let keys_dir = kms_env.kms_keys_dir.clone();
|
||||
create_key_with_specific_id(&keys_dir, default_key_id)
|
||||
.await
|
||||
.expect("failed to create local KMS key");
|
||||
kms_env
|
||||
.base_env
|
||||
.start_rustfs_server_with_env(
|
||||
vec![
|
||||
"--kms-enable",
|
||||
"--kms-backend",
|
||||
"local",
|
||||
"--kms-key-dir",
|
||||
&keys_dir,
|
||||
"--kms-default-key-id",
|
||||
default_key_id,
|
||||
],
|
||||
&[("RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS", "true")],
|
||||
)
|
||||
.await
|
||||
.expect("failed to start RustFS with local KMS");
|
||||
tokio::time::sleep(tokio::time::Duration::from_secs(3)).await;
|
||||
|
||||
let client = kms_env.base_env.create_s3_client();
|
||||
// Unversioned, and deliberately WITHOUT a bucket default-encryption rule, so the copy below
|
||||
// resolves to "no destination encryption".
|
||||
let bucket = "copy-object-self-copy-drop-sse-test";
|
||||
let key = "secrets/report.txt";
|
||||
|
||||
client
|
||||
.create_bucket()
|
||||
.bucket(bucket)
|
||||
.send()
|
||||
.await
|
||||
.expect("failed to create bucket");
|
||||
|
||||
let content = b"encrypted payload whose ciphertext must not survive as bogus plaintext -- 0123456789";
|
||||
client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.metadata("stage", "before")
|
||||
.server_side_encryption(ServerSideEncryption::Aes256)
|
||||
.body(ByteStream::from_static(content))
|
||||
.send()
|
||||
.await
|
||||
.expect("PUT failed");
|
||||
|
||||
// Self-copy with REPLACE and no SSE header. Per AWS semantics the destination ends up
|
||||
// unencrypted. The dangerous outcome is the silent one: the handler strips the source key
|
||||
// metadata while a metadata-only copy leaves the ciphertext in place, so a later GET would
|
||||
// hand back raw ciphertext as if it were plaintext — corruption with no error anywhere.
|
||||
client
|
||||
.copy_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.copy_source(format!("{bucket}/{key}"))
|
||||
.metadata_directive(MetadataDirective::Replace)
|
||||
.metadata("stage", "after")
|
||||
.send()
|
||||
.await
|
||||
.expect("same-key CopyObject dropping SSE must succeed");
|
||||
|
||||
let get = client
|
||||
.get_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.send()
|
||||
.await
|
||||
.expect("GET after self-copy failed");
|
||||
assert_eq!(
|
||||
get.server_side_encryption(),
|
||||
None,
|
||||
"destination must be unencrypted once the copy drops SSE"
|
||||
);
|
||||
assert_eq!(
|
||||
get.metadata().and_then(|m| m.get("stage")),
|
||||
Some(&"after".to_string()),
|
||||
"REPLACE metadata must take effect"
|
||||
);
|
||||
let body = get.body.collect().await.expect("collect body").into_bytes();
|
||||
assert_eq!(
|
||||
body.as_ref(),
|
||||
content,
|
||||
"object must read back as the original plaintext, not the orphaned ciphertext"
|
||||
);
|
||||
|
||||
kms_env.base_env.stop_server();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
#[serial]
|
||||
async fn test_metadata_replace_self_copy_under_bucket_default_sse_stays_decryptable() {
|
||||
init_logging();
|
||||
info!("bucket default encryption must also keep a same-key copy off the metadata-only path");
|
||||
|
||||
let mut kms_env = LocalKMSTestEnvironment::new().await.expect("failed to create local KMS env");
|
||||
let default_key_id = "rustfs-e2e-test-default-key";
|
||||
let keys_dir = kms_env.kms_keys_dir.clone();
|
||||
create_key_with_specific_id(&keys_dir, default_key_id)
|
||||
.await
|
||||
.expect("failed to create local KMS key");
|
||||
kms_env
|
||||
.base_env
|
||||
.start_rustfs_server_with_env(
|
||||
vec![
|
||||
"--kms-enable",
|
||||
"--kms-backend",
|
||||
"local",
|
||||
"--kms-key-dir",
|
||||
&keys_dir,
|
||||
"--kms-default-key-id",
|
||||
default_key_id,
|
||||
],
|
||||
&[("RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS", "true")],
|
||||
)
|
||||
.await
|
||||
.expect("failed to start RustFS with local KMS");
|
||||
tokio::time::sleep(tokio::time::Duration::from_secs(3)).await;
|
||||
|
||||
let client = kms_env.base_env.create_s3_client();
|
||||
let bucket = "copy-object-self-copy-bucket-default-sse-test";
|
||||
let key = "secrets/report.txt";
|
||||
|
||||
client
|
||||
.create_bucket()
|
||||
.bucket(bucket)
|
||||
.send()
|
||||
.await
|
||||
.expect("failed to create bucket");
|
||||
|
||||
// Store the object as PLAINTEXT first: no SSE header and no bucket default rule yet. This is
|
||||
// what makes the case sharp — at copy time the source metadata carries no encryption markers,
|
||||
// so the source-side half of the guard cannot fire.
|
||||
let content = b"plaintext payload that must not be orphaned under a new DEK -- 0123456789";
|
||||
let put = client
|
||||
.put_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.metadata("stage", "before")
|
||||
.body(ByteStream::from_static(content))
|
||||
.send()
|
||||
.await
|
||||
.expect("PUT failed");
|
||||
assert_eq!(put.server_side_encryption(), None, "the object must start out unencrypted");
|
||||
|
||||
// Only NOW enable bucket default encryption. The destination's encryption therefore comes
|
||||
// from the bucket rule and from nowhere else: the source is unencrypted and the copy request
|
||||
// carries no SSE header. A guard that only inspects request headers (MinIO decides
|
||||
// `isTargetEncrypted` from `crypto.S3.IsRequested(r.Header)`) would let this through, yet
|
||||
// `sse_encryption` still mints a fresh DEK from the resolved bucket default — which is why
|
||||
// the guard keys off the *effective* encryption rather than the requested one.
|
||||
let encryption_config = ServerSideEncryptionConfiguration::builder()
|
||||
.rules(
|
||||
ServerSideEncryptionRule::builder()
|
||||
.apply_server_side_encryption_by_default(
|
||||
ServerSideEncryptionByDefault::builder()
|
||||
.sse_algorithm(ServerSideEncryption::Aes256)
|
||||
.build()
|
||||
.unwrap(),
|
||||
)
|
||||
.build(),
|
||||
)
|
||||
.build()
|
||||
.unwrap();
|
||||
client
|
||||
.put_bucket_encryption()
|
||||
.bucket(bucket)
|
||||
.server_side_encryption_configuration(encryption_config)
|
||||
.send()
|
||||
.await
|
||||
.expect("failed to set bucket default encryption");
|
||||
|
||||
// No SSE header on the copy — the bucket default alone drives the destination encryption.
|
||||
client
|
||||
.copy_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.copy_source(format!("{bucket}/{key}"))
|
||||
.metadata_directive(MetadataDirective::Replace)
|
||||
.metadata("stage", "after")
|
||||
.send()
|
||||
.await
|
||||
.expect("same-key CopyObject under bucket default encryption must succeed");
|
||||
|
||||
let get = client
|
||||
.get_object()
|
||||
.bucket(bucket)
|
||||
.key(key)
|
||||
.send()
|
||||
.await
|
||||
.expect("GET after self-copy failed: the object was re-keyed without rewriting the ciphertext");
|
||||
assert_eq!(get.server_side_encryption(), Some(&ServerSideEncryption::Aes256));
|
||||
assert_eq!(
|
||||
get.metadata().and_then(|m| m.get("stage")),
|
||||
Some(&"after".to_string()),
|
||||
"REPLACE metadata must take effect"
|
||||
);
|
||||
let body = get.body.collect().await.expect("collect body").into_bytes();
|
||||
assert_eq!(
|
||||
body.as_ref(),
|
||||
content,
|
||||
"object must still decrypt to the original plaintext after a metadata-only self copy"
|
||||
);
|
||||
|
||||
kms_env.base_env.stop_server();
|
||||
}
|
||||
@@ -48,6 +48,9 @@ mod bucket_default_encryption_test;
|
||||
#[cfg(test)]
|
||||
mod encryption_metadata_test;
|
||||
|
||||
#[cfg(test)]
|
||||
mod copy_object_self_copy_sse_test;
|
||||
|
||||
#[cfg(test)]
|
||||
mod copy_object_version_restore_sse_test;
|
||||
|
||||
|
||||
@@ -969,6 +969,15 @@ impl ECStore {
|
||||
|
||||
if !dst_opts.versioned && src_opts.version_id.is_none() {
|
||||
if src_info.metadata_only {
|
||||
// Zero-copy update: only xl.meta is rewritten, the data blocks stay as they
|
||||
// are. The caller must therefore guarantee that the destination metadata
|
||||
// still describes the stored bytes. In particular a copy that re-derives
|
||||
// encryption material may NOT set metadata_only — that would leave a fresh
|
||||
// DEK beside ciphertext sealed under the old one, permanently destroying the
|
||||
// object. The S3 handler enforces this before calling in (see the
|
||||
// metadata_only decision in rustfs/src/app/object_usecase.rs); the sibling
|
||||
// versioned branch below resolves the same risk by rewriting through
|
||||
// put_object (issue #4238).
|
||||
return self.pools[pool_idx]
|
||||
.copy_object(src_bucket, &src_object, dst_bucket, &dst_object, src_info, src_opts, &dst_opts)
|
||||
.await;
|
||||
|
||||
Reference in New Issue
Block a user