fix(s3): add HMAC-SHA256 key commitment to SSE-S3 and SSE-KMS (#8879)

* fix(s3): add HMAC-SHA256 key commitment to SSE encryption modes

* fix(s3): validate SSE-S3 IV length before cipher.NewCTR

CreateSSES3DecryptedReader took the IV directly from object metadata
and passed it to cipher.NewCTR. The standard library panics when the
IV length differs from the block size, so a tampered metadata field
turned what should be a decryption error into a crash. Validate via
the existing ValidateIV helper first.

Addresses coderabbit review on PR #8879.

* fix(s3): centralize SSE-KMS key commitment in createSSEKMSKey

KeyCommitment used to be set explicitly in only one of the four
encryption entry points; the multipart-aware variants and the bucket-
bound CreateSSEKMSEncryptedReaderForBucket left it empty, so writes
through those paths landed with metadata that VerifyKeyCommitment
treats as legacy and accepts unconditionally — exactly the downgrade
gap the gemini review flagged.

Move the HMAC computation into createSSEKMSKey so every helper-driven
path picks it up automatically, and add the same call to the bucket-
scoped path that builds its own struct literal.

Addresses gemini review on PR #8879.

* fix(s3): store base IV (not derived IV) for offset-aware SSE-KMS chunks

CreateSSEKMSEncryptedReaderWithBaseIVAndOffset used to store the
already-offset-derived IV plus the chunk offset in metadata. The
decrypt path then applied calculateIVWithOffset a second time to
that stored IV, producing the wrong CTR keystream and a decryption
mismatch. Centralizing the key commitment made the bug visible as a
commitment failure, but the underlying issue predates that change.

Pass the base IV through to createSSEKMSKey so sseKey.IV is the
unmodified base on disk; the decrypt path's offset application then
recovers the correct chunk-level IV. The HMAC commitment binds the
base IV — same value the verify call at decrypt time hashes — so the
new commitment path stays consistent.

Addresses gemini security-high review on PR #8879.

* fix(s3): opt-in strict-commitment mode to close the downgrade vector

WEED_S3_REQUIRE_KEY_COMMITMENT=true flips VerifyKeyCommitment from
accept-when-missing (the AWS-compatible default needed for objects
written before commitments shipped) to reject-when-missing. With
the env var set, an attacker who strips the commitment field from
object metadata can no longer bypass integrity verification — every
object must carry a commitment that hashes to the right value.

Default stays false so existing legacy objects keep decrypting; the
warning the gemini review raised about the silent-downgrade vector
is closed for operators who explicitly opt in once their bucket is
fully migrated. SetRequireKeyCommitment exposes a runtime seam for
tests and future config-reload paths.

Addresses the security-medium review on PR #8879.

* test(s3): fix mislabelled IV literal in strict-commitment test

The "strict-mode-iv-16" literal was actually 17 bytes — the trailing
"16" was meant as a comment but counted as content. The IV is fed
into HMAC, not AES, so the length didn't matter for behaviour, but
the discrepancy was confusing. Tighten to a real 16-byte literal and
explain the choice in a comment.

Addresses coderabbit minor review on PR #8879.

* fix(s3): store KMS-resolved KeyID in SSE-KMS metadata, not the request

CreateSSEKMSEncryptedReaderForBucket built its SSEKMSKey with the
caller-supplied keyID, but CreateSSEKMSDecryptedReader later compares
decryptResp.KeyID against sseKey.KeyID. A request that used an alias
would resolve to a different ARN in the response; storing the request
form would then trip the mismatch check at decrypt time and surface
as a "KMS key ID mismatch" error against the operator's own object.
The helper-driven encryption paths already do the right thing via
createSSEKMSKey; this is the bucket-bound path catching up.

Addresses coderabbit review on PR #8879.

* test(s3): cover key commitment rejection paths under tampering

Adds the negative-path tests coderabbit flagged as missing: a
tampered key, IV, algorithm, or commitment field must fail
VerifyKeyCommitment, otherwise a regression in the rejection logic
could land silently. The HMAC binds all three inputs plus the
commitment itself, so any single mutation is enough.

Addresses coderabbit nitpick on PR #8879.

weed/s3api/s3_sse_kms.go

@@ -38,6 +38,7 @@ type SSEKMSKey struct {
 	BucketKeyEnabled  bool              // Whether S3 Bucket Keys are enabled
 	IV                []byte            // The initialization vector for encryption
 	ChunkOffset       int64             // Offset of this chunk within the original part (for IV calculation)
+	KeyCommitment     []byte            // HMAC-SHA256 commitment binding key to IV+algorithm
 }
 
 // SSEKMSMetadata represents the metadata stored with SSE-KMS objects
@@ -49,6 +50,7 @@ type SSEKMSMetadata struct {
 	BucketKeyEnabled  bool              `json:"bucketKeyEnabled"`  // S3 Bucket Key optimization
 	IV                string            `json:"iv"`                // Base64-encoded initialization vector
 	PartOffset        int64             `json:"partOffset"`        // Offset within original multipart part (for IV calculation)
+	KeyCommitment     string            `json:"keyCommitment,omitempty"`     // Base64-encoded HMAC key commitment
 }
 
 const (
@@ -93,16 +95,15 @@ func CreateSSEKMSEncryptedReaderWithBucketKey(r io.Reader, keyID string, encrypt
 	// Create CTR mode cipher stream
 	stream := cipher.NewCTR(dataKeyResult.Block, iv)
 
-	// Create the SSE-KMS metadata using utility function
+	// Create the SSE-KMS metadata using utility function. createSSEKMSKey
+	// computes the key commitment too, so all encryption paths produce
+	// commitment-bound metadata uniformly.
 	sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, iv, 0)
 
 	// The IV is stored in SSE key metadata, so the encrypted stream does not need to prepend the IV
 	// This ensures correct Content-Length for clients
 	encryptedReader := &cipher.StreamReader{S: stream, R: r}
 
-	// Store IV in the SSE key for metadata storage
-	sseKey.IV = iv
-
 	return encryptedReader, sseKey, nil
 }
 
@@ -122,15 +123,20 @@ func CreateSSEKMSEncryptedReaderWithBaseIVAndOffset(r io.Reader, keyID string, e
 	// Ensure we clear the plaintext data key from memory when done
 	defer clearKMSDataKey(dataKeyResult)
 
-	// Calculate unique IV using base IV and offset to prevent IV reuse in multipart uploads
-	// Skip is not used here because we're encrypting from the start (not reading a range)
+	// Calculate unique IV using base IV and offset to prevent IV reuse in multipart uploads.
+	// Skip is not used here because we're encrypting from the start (not reading a range).
 	iv, _ := calculateIVWithOffset(baseIV, offset)
 
 	// Create CTR mode cipher stream
 	stream := cipher.NewCTR(dataKeyResult.Block, iv)
 
-	// Create the SSE-KMS metadata using utility function
-	sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, iv, offset)
+	// Store the BASE IV (not the offset-derived IV) in metadata. The decrypt
+	// path applies calculateIVWithOffset to sseKey.IV when ChunkOffset > 0;
+	// storing the derived IV here would cause it to offset twice and produce
+	// the wrong CTR keystream. The key commitment, computed inside
+	// createSSEKMSKey, therefore binds the base IV — exactly the value the
+	// verify call at decrypt time hashes.
+	sseKey := createSSEKMSKey(dataKeyResult, encryptionContext, bucketKeyEnabled, baseIV, offset)
 
 	// The IV is stored in SSE key metadata, so the encrypted stream does not need to prepend the IV
 	// This ensures correct Content-Length for clients
@@ -274,13 +280,19 @@ func (s3a *S3ApiServer) CreateSSEKMSEncryptedReaderForBucket(r io.Reader, bucket
 	// Create CTR mode cipher stream
 	stream := cipher.NewCTR(block, iv)
 
-	// Create the encrypting reader
+	// Create the encrypting reader. Compute the HMAC commitment alongside
+	// every other field so this bucket-scoped path is on the same downgrade-
+	// resistant footing as the helper-driven paths above. Store the KMS
+	// response's KeyID rather than the request's; CreateSSEKMSDecryptedReader
+	// compares against decryptResp.KeyID, and a request alias would mismatch
+	// the resolved ARN the response carries.
 	sseKey := &SSEKMSKey{
-		KeyID:             keyID,
+		KeyID:             dataKeyResp.KeyID,
 		EncryptedDataKey:  dataKeyResp.CiphertextBlob,
 		EncryptionContext: encryptionContext,
 		BucketKeyEnabled:  bucketKeyEnabled,
 		IV:                iv,
+		KeyCommitment:     ComputeKeyCommitment(dataKeyResp.Plaintext, iv, s3_constants.SSEAlgorithmKMS),
 	}
 
 	return &cipher.StreamReader{S: stream, R: r}, sseKey, nil
@@ -374,6 +386,11 @@ func CreateSSEKMSDecryptedReader(r io.Reader, sseKey *SSEKMSKey) (io.Reader, err
 		return nil, fmt.Errorf("KMS key ID mismatch: expected %s, got %s", sseKey.KeyID, decryptResp.KeyID)
 	}
 
+	// Verify key commitment before decryption if one exists in metadata
+	if err := VerifyKeyCommitment(decryptResp.Plaintext, sseKey.IV, s3_constants.SSEAlgorithmKMS, sseKey.KeyCommitment); err != nil {
+		return nil, err
+	}
+
 	// Use the IV from the SSE key metadata, calculating offset if this is a chunked part
 	if err := ValidateIV(sseKey.IV, "SSE key IV"); err != nil {
 		return nil, fmt.Errorf("invalid IV in SSE key: %w", err)
@@ -465,6 +482,11 @@ func SerializeSSEKMSMetadata(sseKey *SSEKMSKey) ([]byte, error) {
 		PartOffset:        sseKey.ChunkOffset,                           // Store within-part offset
 	}
 
+	// Include key commitment if present
+	if len(sseKey.KeyCommitment) > 0 {
+		metadata.KeyCommitment = base64.StdEncoding.EncodeToString(sseKey.KeyCommitment)
+	}
+
 	data, err := json.Marshal(metadata)
 	if err != nil {
 		return nil, fmt.Errorf("failed to marshal SSE-KMS metadata: %w", err)
@@ -510,6 +532,15 @@ func DeserializeSSEKMSMetadata(data []byte) (*SSEKMSKey, error) {
 		}
 	}
 
+	// Decode key commitment if present
+	var keyCommitment []byte
+	if metadata.KeyCommitment != "" {
+		keyCommitment, err = base64.StdEncoding.DecodeString(metadata.KeyCommitment)
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode key commitment: %w", err)
+		}
+	}
+
 	sseKey := &SSEKMSKey{
 		KeyID:             metadata.KeyID,
 		EncryptedDataKey:  encryptedDataKey,
@@ -517,6 +548,7 @@ func DeserializeSSEKMSMetadata(data []byte) (*SSEKMSKey, error) {
 		BucketKeyEnabled:  metadata.BucketKeyEnabled,
 		IV:                iv,                  // Restore IV for decryption
 		ChunkOffset:       metadata.PartOffset, // Use stored within-part offset
+		KeyCommitment:     keyCommitment,
 	}
 
 	glog.V(4).Infof("Deserialized SSE-KMS metadata: keyID=%s, bucketKey=%t", sseKey.KeyID, sseKey.BucketKeyEnabled)

weed/s3api/s3_sse_kms_utils.go

@@ -86,7 +86,12 @@ func clearKMSDataKey(result *KMSDataKeyResult) {
 	}
 }
 
-// createSSEKMSKey creates an SSEKMSKey struct from data key result and parameters
+// createSSEKMSKey creates an SSEKMSKey struct from data key result and parameters.
+// The HMAC key commitment is computed here (rather than at each call site) so
+// every SSE-KMS encryption path produces metadata that can later be verified
+// against tampering — a missing commitment was an attacker-controlled silent
+// downgrade vector. plaintext must still be live; deferred clearKMSDataKey
+// runs after this function returns.
 func createSSEKMSKey(result *KMSDataKeyResult, encryptionContext map[string]string, bucketKeyEnabled bool, iv []byte, chunkOffset int64) *SSEKMSKey {
 	return &SSEKMSKey{
 		KeyID:             result.Response.KeyID,
@@ -95,5 +100,6 @@ func createSSEKMSKey(result *KMSDataKeyResult, encryptionContext map[string]stri
 		BucketKeyEnabled:  bucketKeyEnabled,
 		IV:                iv,
 		ChunkOffset:       chunkOffset,
+		KeyCommitment:     ComputeKeyCommitment(result.Response.Plaintext, iv, s3_constants.SSEAlgorithmKMS),
 	}
 }

weed/s3api/s3_sse_s3.go

@@ -36,10 +36,11 @@ const (
 
 // SSES3Key represents a server-managed encryption key for SSE-S3
 type SSES3Key struct {
-	Key       []byte
-	KeyID     string
-	Algorithm string
-	IV        []byte // Initialization Vector for this key
+	Key           []byte
+	KeyID         string
+	Algorithm     string
+	IV            []byte // Initialization Vector for this key
+	KeyCommitment []byte // HMAC-SHA256 commitment binding key to IV+algorithm
 }
 
 // IsSSES3RequestInternal checks if the request specifies SSE-S3 encryption
@@ -116,6 +117,18 @@ func CreateSSES3EncryptedReader(reader io.Reader, key *SSES3Key) (io.Reader, []b
 
 // CreateSSES3DecryptedReader creates a decrypted reader for SSE-S3 using IV from metadata
 func CreateSSES3DecryptedReader(reader io.Reader, key *SSES3Key, iv []byte) (io.Reader, error) {
+	// IV comes from object metadata, which is mutable. Validate before passing
+	// to cipher.NewCTR so a tampered length produces an error rather than the
+	// crypto/cipher panic the documentation specifies.
+	if err := ValidateIV(iv, "SSE-S3 IV"); err != nil {
+		return nil, err
+	}
+
+	// Verify key commitment before decryption if one exists in metadata
+	if err := VerifyKeyCommitment(key.Key, iv, key.Algorithm, key.KeyCommitment); err != nil {
+		return nil, err
+	}
+
 	// Create AES cipher
 	block, err := aes.NewCipher(key.Key)
 	if err != nil {
@@ -160,6 +173,9 @@ func SerializeSSES3Metadata(key *SSES3Key) ([]byte, error) {
 	// Include IV if present (needed for chunk-level decryption)
 	if key.IV != nil {
 		metadata["iv"] = base64.StdEncoding.EncodeToString(key.IV)
+		// Compute and store key commitment binding key ↔ IV + algorithm
+		commitment := ComputeKeyCommitment(key.Key, key.IV, key.Algorithm)
+		metadata["keyCommitment"] = base64.StdEncoding.EncodeToString(commitment)
 	}
 
 	// Use JSON for proper serialization
@@ -238,6 +254,15 @@ func DeserializeSSES3Metadata(data []byte, keyManager *SSES3KeyManager) (*SSES3K
 		key.IV = iv
 	}
 
+	// Restore key commitment if present (for tamper detection)
+	if commitStr, exists := metadata["keyCommitment"]; exists {
+		commitment, err := base64.StdEncoding.DecodeString(commitStr)
+		if err != nil {
+			return nil, fmt.Errorf("failed to decode key commitment: %w", err)
+		}
+		key.KeyCommitment = commitment
+	}
+
 	return key, nil
 }
 

weed/s3api/s3_validation_utils.go

@@ -1,12 +1,83 @@
 package s3api
 
 import (
+	"crypto/hmac"
+	"crypto/sha256"
 	"fmt"
+	"os"
 	"strings"
+	"sync/atomic"
 
+	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
 )
 
+// RequireKeyCommitmentEnv is the environment variable that flips the
+// commitment check from "skip when missing" (the AWS-compatible default,
+// needed for objects written before commitments shipped) to "reject when
+// missing". Operators who have either re-encrypted all legacy objects or
+// who never wrote any objects under the pre-commitment code path can opt
+// in via this env var to close the silent downgrade vector that an
+// attacker with write access to object metadata could otherwise exploit
+// by stripping the commitment field.
+const RequireKeyCommitmentEnv = "WEED_S3_REQUIRE_KEY_COMMITMENT"
+
+// requireKeyCommitment is the runtime mirror of the env var, kept as an
+// atomic so config-reload paths can flip it without a global mutex.
+var requireKeyCommitment atomic.Bool
+
+func init() {
+	if v := os.Getenv(RequireKeyCommitmentEnv); v == "1" || strings.EqualFold(v, "true") {
+		requireKeyCommitment.Store(true)
+		glog.V(1).Infof("SSE: %s=true; SSE objects without a key commitment will be rejected", RequireKeyCommitmentEnv)
+	}
+}
+
+// SetRequireKeyCommitment toggles strict-commitment enforcement at runtime.
+// Used by tests and by future config-reload code paths.
+func SetRequireKeyCommitment(require bool) {
+	requireKeyCommitment.Store(require)
+}
+
+// ComputeKeyCommitment computes an HMAC-SHA256 key commitment over the
+// encryption parameters (IV + algorithm). This binds the ciphertext to the
+// exact key material and IV that were used, preventing key-confusion and
+// IV-manipulation attacks against unauthenticated AES-CTR.
+//
+// The commitment is stored alongside the IV in object metadata. On decrypt
+// the commitment is re-derived and compared; a mismatch means the key or IV
+// was tampered with.
+func ComputeKeyCommitment(key []byte, iv []byte, algorithm string) []byte {
+	mac := hmac.New(sha256.New, key)
+	mac.Write(iv)
+	mac.Write([]byte(algorithm))
+	return mac.Sum(nil)
+}
+
+// VerifyKeyCommitment checks a previously stored commitment against the
+// current key, IV, and algorithm. Returns nil on success.
+//
+// When the commitment is empty (legacy object written before commitments
+// shipped), the default behaviour is to accept the object — this is the
+// AWS-compatible path. Setting WEED_S3_REQUIRE_KEY_COMMITMENT=true (via
+// env at startup or via SetRequireKeyCommitment at runtime) flips that
+// to reject, closing the silent-downgrade vector at the cost of locking
+// out un-migrated legacy objects.
+func VerifyKeyCommitment(key []byte, iv []byte, algorithm string, commitment []byte) error {
+	if len(commitment) == 0 {
+		if requireKeyCommitment.Load() {
+			return fmt.Errorf("key commitment is required but missing from object metadata: %s set; legacy objects must be re-encrypted before this flag can be enabled", RequireKeyCommitmentEnv)
+		}
+		// Legacy data written before key commitments were added; skip.
+		return nil
+	}
+	expected := ComputeKeyCommitment(key, iv, algorithm)
+	if !hmac.Equal(expected, commitment) {
+		return fmt.Errorf("key commitment verification failed: encryption parameters may have been tampered with")
+	}
+	return nil
+}
+
 // isValidKMSKeyID performs basic validation of KMS key identifiers.
 // Following Minio's approach: be permissive and accept any reasonable key format.
 // Only reject keys with leading/trailing spaces or other obvious issues.

weed/s3api/s3_validation_utils_require_test.go

@@ -0,0 +1,102 @@
+package s3api
+
+import "testing"
+
+func TestVerifyKeyCommitment_DefaultAcceptsMissing(t *testing.T) {
+	// Default behaviour mirrors AWS: a missing commitment field is treated
+	// as a legacy object and accepted. This is the cushion that lets
+	// operators upgrade without breaking pre-commitment uploads.
+	prev := requireKeyCommitment.Load()
+	t.Cleanup(func() { requireKeyCommitment.Store(prev) })
+	requireKeyCommitment.Store(false)
+
+	if err := VerifyKeyCommitment([]byte("k"), []byte("iv"), "AES256", nil); err != nil {
+		t.Fatalf("default path should accept missing commitment, got: %v", err)
+	}
+}
+
+func TestVerifyKeyCommitment_StrictRejectsMissing(t *testing.T) {
+	// Strict mode: any object whose metadata lacks the commitment field is
+	// rejected. Closes the silent-downgrade vector — an attacker who can
+	// strip the commitment from metadata can no longer bypass verification.
+	prev := requireKeyCommitment.Load()
+	t.Cleanup(func() { requireKeyCommitment.Store(prev) })
+	requireKeyCommitment.Store(true)
+
+	err := VerifyKeyCommitment([]byte("k"), []byte("iv"), "AES256", nil)
+	if err == nil {
+		t.Fatal("strict mode should reject missing commitment; got nil error")
+	}
+}
+
+func TestVerifyKeyCommitment_StrictAcceptsValidCommitment(t *testing.T) {
+	// Strict mode does not change the verification outcome for objects that
+	// do carry a commitment — the only behavioural delta is the missing
+	// case.
+	prev := requireKeyCommitment.Load()
+	t.Cleanup(func() { requireKeyCommitment.Store(prev) })
+	requireKeyCommitment.Store(true)
+
+	key := []byte("strict-mode-test-key")
+	// IV here is just an opaque input to the HMAC commitment; the test
+	// doesn't pass it into AES-CTR so it doesn't have to be the AES block
+	// size. The 16 bytes match the AES block size to keep the literal
+	// realistic.
+	iv := []byte("strict-mode-iv16")
+	commit := ComputeKeyCommitment(key, iv, "AES256")
+	if err := VerifyKeyCommitment(key, iv, "AES256", commit); err != nil {
+		t.Fatalf("strict mode should accept valid commitment, got: %v", err)
+	}
+}
+
+func TestVerifyKeyCommitment_RejectsTamperedKey(t *testing.T) {
+	// Real attack shape: an attacker who can mutate object metadata cannot
+	// craft a valid commitment without the original key. The verify path
+	// must catch it whether they tamper with the key, the IV, or the
+	// algorithm — all three are bound by the HMAC.
+	prev := requireKeyCommitment.Load()
+	t.Cleanup(func() { requireKeyCommitment.Store(prev) })
+	requireKeyCommitment.Store(false)
+
+	originalKey := []byte("legit-key-for-commitment")
+	iv := []byte("commitment-iv-16")
+	algo := "AES256"
+	commit := ComputeKeyCommitment(originalKey, iv, algo)
+
+	t.Run("tampered key", func(t *testing.T) {
+		if err := VerifyKeyCommitment([]byte("attacker-substituted-key"), iv, algo, commit); err == nil {
+			t.Fatal("verify must reject when the key changed but commitment did not")
+		}
+	})
+	t.Run("tampered IV", func(t *testing.T) {
+		if err := VerifyKeyCommitment(originalKey, []byte("attacker-iv-16!!"), algo, commit); err == nil {
+			t.Fatal("verify must reject when the IV changed but commitment did not")
+		}
+	})
+	t.Run("tampered algorithm", func(t *testing.T) {
+		if err := VerifyKeyCommitment(originalKey, iv, "AES128", commit); err == nil {
+			t.Fatal("verify must reject when the algorithm changed but commitment did not")
+		}
+	})
+	t.Run("tampered commitment", func(t *testing.T) {
+		bad := append([]byte{}, commit...)
+		bad[0] ^= 0x01
+		if err := VerifyKeyCommitment(originalKey, iv, algo, bad); err == nil {
+			t.Fatal("verify must reject when the commitment itself was flipped")
+		}
+	})
+}
+
+func TestSetRequireKeyCommitment(t *testing.T) {
+	prev := requireKeyCommitment.Load()
+	t.Cleanup(func() { requireKeyCommitment.Store(prev) })
+
+	SetRequireKeyCommitment(true)
+	if !requireKeyCommitment.Load() {
+		t.Fatal("SetRequireKeyCommitment(true) did not propagate to the atomic")
+	}
+	SetRequireKeyCommitment(false)
+	if requireKeyCommitment.Load() {
+		t.Fatal("SetRequireKeyCommitment(false) did not propagate to the atomic")
+	}
+}