fix(s3api): validate SSE-KMS chunk IV during prep, before any fetch

Addresses CodeRabbit review on PR #9228: in createMultipartSSEKMSDecryptedReaderDirect the per-chunk SSE-KMS metadata was deserialized in the prep loop but the IV length was only validated later, inside CreateSSEKMSDecryptedReader, which runs from the wrap closure -- AFTER the chunk's volume-server fetch has already started. That weakens the new "reject malformed chunks before any fetch" contract for SSE-KMS specifically: a chunk with a missing/short/long IV would fire its HTTP GET, then fail mid-stream during decrypt. The fix moves the existing ValidateIV check into the prep loop, matching the SSE-S3 and SSE-C paths. Drive-by: extract the SSE-KMS prep loop into a free buildMultipartSSEKMSReader helper that mirrors buildMultipartSSES3Reader, so the new contract is unit-testable without an S3ApiServer. The exported method (createMultipartSSEKMSDecryptedReaderDirect) stays a thin caller, so behavior for production callers is unchanged. New tests in weed/s3api/s3api_multipart_ssekms_test.go pin the contract: - TestBuildMultipartSSEKMSReader_RejectsBadIVBeforeAnyFetch covers missing IV, empty IV, short IV, long IV. Each case asserts both that an error is returned AND that the fetch callback is never invoked. - TestBuildMultipartSSEKMSReader_RejectsMissingMetadataBeforeAnyFetch pins the analogous behavior when SseMetadata is nil on a chunk in position N: chunks 0..N-1 must not be fetched (the earlier eager implementation depended on a closeAppendedReaders cleanup path; the new contract is stronger -- nothing is opened in the first place). - TestBuildMultipartSSEKMSReader_RejectsUnparseableMetadataBeforeAnyFetch covers the JSON-unmarshal failure branch. - TestBuildMultipartSSEKMSReader_SortsByOffset smoke-tests the documented sort-by-offset contract by recording the order in which fetch is invoked. All four pass under `go test ./weed/s3api/`. Existing weed/s3api unit suite + the SSE integration suite (with the local KMS provider enabled via s3-config-template.json) continue to pass.
2026-08-19 05:36:58 +00:00 · 2026-04-26 15:09:52 -07:00
parent f9b9bcf95e
commit f083fa9fed
2 changed files with 271 additions and 13 deletions
@@ -0,0 +1,241 @@
+package s3api
+
+import (
+	"bytes"
+	"crypto/rand"
+	"fmt"
+	"io"
+	"strings"
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
+)
+
+// TestBuildMultipartSSEKMSReader_RejectsBadIVBeforeAnyFetch pins the contract
+// that a per-chunk SSE-KMS metadata blob with a missing or wrong-length IV is
+// rejected during preparation, before any volume-server fetch fires.
+//
+// DeserializeSSEKMSMetadata only proves the JSON parses; it leaves the
+// kmsKey.IV field at whatever the metadata actually carried. CreateSSEKMSDecryptedReader
+// does call ValidateIV, but only when the wrap closure runs -- after the
+// chunk's HTTP body has already been opened. The lazy reader's whole point
+// is to never start an HTTP fetch for a chunk we know we cannot decrypt, so
+// IV validation must happen in the prep loop. This test is the regression
+// guard for that, addressing CodeRabbit review feedback on PR #9228.
+func TestBuildMultipartSSEKMSReader_RejectsBadIVBeforeAnyFetch(t *testing.T) {
+	makeMetadata := func(iv []byte) []byte {
+		t.Helper()
+		key := &SSEKMSKey{
+			KeyID:            "test-kms-key",
+			EncryptedDataKey: bytes.Repeat([]byte{0x42}, 32),
+			IV:               iv,
+		}
+		md, err := SerializeSSEKMSMetadata(key)
+		if err != nil {
+			t.Fatalf("SerializeSSEKMSMetadata: %v", err)
+		}
+		return md
+	}
+
+	cases := []struct {
+		name      string
+		iv        []byte
+		expectErr string
+	}{
+		{"missing IV", nil, "invalid"},
+		{"empty IV", []byte{}, "invalid"},
+		{"short IV", []byte("too-short"), "invalid"}, // 9 bytes, not 16
+		{"long IV", bytes.Repeat([]byte{1}, 32), "invalid"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fetchCalled := false
+			fetch := func(c *filer_pb.FileChunk) (io.ReadCloser, error) {
+				fetchCalled = true
+				return io.NopCloser(bytes.NewReader([]byte("ignored"))), nil
+			}
+
+			chunks := []*filer_pb.FileChunk{
+				{
+					FileId:      "1,bad-iv",
+					Offset:      0,
+					Size:        8,
+					SseType:     filer_pb.SSEType_SSE_KMS,
+					SseMetadata: makeMetadata(tc.iv),
+				},
+			}
+
+			_, err := buildMultipartSSEKMSReader(chunks, fetch)
+			if err == nil {
+				t.Fatal("expected error for invalid SSE-KMS IV, got nil")
+			}
+			if !strings.Contains(err.Error(), tc.expectErr) {
+				t.Errorf("expected %q in error, got: %v", tc.expectErr, err)
+			}
+			// The whole point of upfront validation: no HTTP fetch must fire
+			// for a chunk that fails the metadata gate.
+			if fetchCalled {
+				t.Error("fetchChunk was called for a chunk with invalid IV; metadata validation must run before any fetch")
+			}
+		})
+	}
+}
+
+// TestBuildMultipartSSEKMSReader_RejectsMissingMetadataBeforeAnyFetch verifies
+// that a chunk tagged SSE-KMS but with no SseMetadata bytes is rejected during
+// preparation, also without firing a fetch. Mirrors the SSE-S3 contract pinned
+// by TestBuildMultipartSSES3Reader_RejectsBadChunkBeforeAnyFetch.
+func TestBuildMultipartSSEKMSReader_RejectsMissingMetadataBeforeAnyFetch(t *testing.T) {
+	fetched := map[string]int{}
+	fetch := func(c *filer_pb.FileChunk) (io.ReadCloser, error) {
+		fetched[c.GetFileIdString()]++
+		return io.NopCloser(bytes.NewReader([]byte("ignored"))), nil
+	}
+
+	// First chunk has valid SSE-KMS metadata; second chunk is tagged SSE-KMS
+	// but has no metadata blob. The eager pre-#9228 implementation would have
+	// opened chunk 0's HTTP body before discovering chunk 1's problem; the
+	// lazy implementation must reject up front and leave both alone.
+	validKey := &SSEKMSKey{
+		KeyID:            "test-kms-key",
+		EncryptedDataKey: bytes.Repeat([]byte{0x42}, 32),
+		IV:               make([]byte, s3_constants.AESBlockSize),
+	}
+	if _, err := rand.Read(validKey.IV); err != nil {
+		t.Fatalf("rand.Read: %v", err)
+	}
+	validMeta, err := SerializeSSEKMSMetadata(validKey)
+	if err != nil {
+		t.Fatalf("SerializeSSEKMSMetadata: %v", err)
+	}
+
+	chunks := []*filer_pb.FileChunk{
+		{
+			FileId:      "1,good",
+			Offset:      0,
+			Size:        16,
+			SseType:     filer_pb.SSEType_SSE_KMS,
+			SseMetadata: validMeta,
+		},
+		{
+			FileId:      "2,no-metadata",
+			Offset:      16,
+			Size:        16,
+			SseType:     filer_pb.SSEType_SSE_KMS,
+			SseMetadata: nil, // triggers "missing per-chunk metadata"
+		},
+	}
+
+	_, err = buildMultipartSSEKMSReader(chunks, fetch)
+	if err == nil {
+		t.Fatal("expected error from missing chunk metadata, got nil")
+	}
+	if !strings.Contains(err.Error(), "missing per-chunk metadata") {
+		t.Errorf("expected 'missing per-chunk metadata' in error, got: %v", err)
+	}
+	if len(fetched) != 0 {
+		t.Errorf("expected no chunks fetched on validation failure, got %v", fetched)
+	}
+}
+
+// TestBuildMultipartSSEKMSReader_RejectsUnparseableMetadataBeforeAnyFetch
+// covers the prep-loop branch where SseMetadata is non-empty but JSON-malformed
+// so DeserializeSSEKMSMetadata itself returns an error. Same contract: no
+// fetch fires.
+func TestBuildMultipartSSEKMSReader_RejectsUnparseableMetadataBeforeAnyFetch(t *testing.T) {
+	fetchCalled := false
+	fetch := func(c *filer_pb.FileChunk) (io.ReadCloser, error) {
+		fetchCalled = true
+		return io.NopCloser(bytes.NewReader([]byte("ignored"))), nil
+	}
+
+	chunks := []*filer_pb.FileChunk{
+		{
+			FileId:      "1,garbage",
+			Offset:      0,
+			Size:        8,
+			SseType:     filer_pb.SSEType_SSE_KMS,
+			SseMetadata: []byte("{not-json"),
+		},
+	}
+
+	_, err := buildMultipartSSEKMSReader(chunks, fetch)
+	if err == nil {
+		t.Fatal("expected error from unparseable SSE-KMS metadata, got nil")
+	}
+	if !strings.Contains(err.Error(), "deserialize SSE-KMS metadata") {
+		t.Errorf("expected 'deserialize SSE-KMS metadata' in error, got: %v", err)
+	}
+	if fetchCalled {
+		t.Error("fetchChunk was called for a chunk with garbage metadata; deserialize must fail before any fetch")
+	}
+}
+
+// TestBuildMultipartSSEKMSReader_SortsByOffset is a smoke test that the prep
+// loop reorders chunks by Offset before constructing the lazy reader, matching
+// the documented contract and the SSE-S3 helper. It does not exercise actual
+// decryption (that requires a live KMS provider); it just verifies the
+// chunk-fetch order observed by the fetch callback once the lazy reader is
+// drained matches ascending offset, regardless of input order.
+func TestBuildMultipartSSEKMSReader_SortsByOffset(t *testing.T) {
+	// Build three chunks with valid SSE-KMS metadata, deliberately out of
+	// offset order on the way in. We never actually decrypt -- the chunks
+	// hold dummy ciphertext and we make CreateSSEKMSDecryptedReader fail
+	// inside the wrap closure by reading 0 bytes; we only care about the
+	// order in which fetch is invoked.
+	makeChunk := func(fid string, offset int64) *filer_pb.FileChunk {
+		key := &SSEKMSKey{
+			KeyID:            "test-kms-key",
+			EncryptedDataKey: bytes.Repeat([]byte{0x42}, 32),
+			IV:               bytes.Repeat([]byte{0x10}, s3_constants.AESBlockSize),
+		}
+		meta, err := SerializeSSEKMSMetadata(key)
+		if err != nil {
+			t.Fatalf("SerializeSSEKMSMetadata: %v", err)
+		}
+		return &filer_pb.FileChunk{
+			FileId:      fid,
+			Offset:      offset,
+			Size:        1,
+			SseType:     filer_pb.SSEType_SSE_KMS,
+			SseMetadata: meta,
+		}
+	}
+	chunks := []*filer_pb.FileChunk{
+		makeChunk("c2", 200),
+		makeChunk("c0", 0),
+		makeChunk("c1", 100),
+	}
+
+	var fetchOrder []string
+	fetch := func(c *filer_pb.FileChunk) (io.ReadCloser, error) {
+		fetchOrder = append(fetchOrder, c.GetFileIdString())
+		// Return an error so we don't actually try to decrypt the dummy
+		// payload; we only care that fetch was reached for each chunk.
+		return nil, fmt.Errorf("synthetic stop after fetch order recorded")
+	}
+
+	reader, err := buildMultipartSSEKMSReader(chunks, fetch)
+	if err != nil {
+		t.Fatalf("buildMultipartSSEKMSReader: %v", err)
+	}
+	// Drive the reader: each Read should advance through chunks in offset
+	// order. We expect the first Read to record c0, then on the next iteration
+	// after the synthetic fetch error the reader marks itself finished.
+	buf := make([]byte, 1)
+	for i := 0; i < 4; i++ {
+		_, err := reader.Read(buf)
+		if err != nil {
+			break
+		}
+	}
+
+	if len(fetchOrder) == 0 {
+		t.Fatal("expected at least one fetch call, got none")
+	}
+	if fetchOrder[0] != "c0" {
+		t.Errorf("expected first fetch to be c0 (offset 0), got %v", fetchOrder)
+	}
+}
@@ -2658,18 +2658,27 @@ func (s3a *S3ApiServer) createMultipartSSEKMSDecryptedReaderDirect(ctx context.C
 		defer encryptedStream.Close()
 	}

-	// Sort a copy of the slice so entry.Chunks is not reordered (other code
-	// paths, e.g. ETag computation, can rely on the original chunk order).
-	// IV length is validated inside CreateSSEKMSDecryptedReader via ValidateIV.
-	originalChunks := entry.GetChunks()
-	chunks := make([]*filer_pb.FileChunk, len(originalChunks))
-	copy(chunks, originalChunks)
-	sort.Slice(chunks, func(i, j int) bool {
-		return chunks[i].GetOffset() < chunks[j].GetOffset()
+	return buildMultipartSSEKMSReader(entry.GetChunks(), func(chunk *filer_pb.FileChunk) (io.ReadCloser, error) {
+		return s3a.createEncryptedChunkReader(ctx, chunk)
+	})
+}
+
+// buildMultipartSSEKMSReader composes a decrypted reader from a set of
+// multipart SSE-KMS chunks. Mirrors buildMultipartSSES3Reader: chunks are
+// validated upfront (per-chunk metadata parses, IV has the right length) and
+// fetched + decrypted lazily through lazyMultipartChunkReader, so at most one
+// volume-server HTTP body is live at a time. Exposed as a free function so
+// tests can inject a mock chunk fetcher and pin the "no fetch on bad
+// metadata" contract without spinning up an S3ApiServer.
+func buildMultipartSSEKMSReader(chunks []*filer_pb.FileChunk, fetchChunk func(*filer_pb.FileChunk) (io.ReadCloser, error)) (io.Reader, error) {
+	sortedChunks := make([]*filer_pb.FileChunk, len(chunks))
+	copy(sortedChunks, chunks)
+	sort.Slice(sortedChunks, func(i, j int) bool {
+		return sortedChunks[i].GetOffset() < sortedChunks[j].GetOffset()
 	})

-	preparedChunks := make([]preparedMultipartChunk, 0, len(chunks))
-	for _, chunk := range chunks {
+	preparedChunks := make([]preparedMultipartChunk, 0, len(sortedChunks))
+	for _, chunk := range sortedChunks {
 		if chunk.GetSseType() != filer_pb.SSEType_SSE_KMS {
 			preparedChunks = append(preparedChunks, preparedMultipartChunk{chunk: chunk})
 			continue
@@ -2681,6 +2690,16 @@ func (s3a *S3ApiServer) createMultipartSSEKMSDecryptedReaderDirect(ctx context.C
 		if err != nil {
 			return nil, fmt.Errorf("failed to deserialize SSE-KMS metadata for chunk %s: %v", chunk.GetFileIdString(), err)
 		}
+		// Validate IV length up front, mirroring the SSE-S3 / SSE-C
+		// preparation paths. CreateSSEKMSDecryptedReader does call
+		// ValidateIV internally, but only when the wrap closure runs --
+		// after the chunk's volume-server fetch has already started. We
+		// want the "reject malformed chunks before any fetch" contract to
+		// hold for SSE-KMS too, so a missing or short IV must fail here
+		// in the prep loop rather than turn into a mid-stream error.
+		if err := ValidateIV(kmsKey.IV, fmt.Sprintf("SSE-KMS chunk %s IV", chunk.GetFileIdString())); err != nil {
+			return nil, err
+		}
 		// Capture kmsKey and chunk into the wrap closure so each prepared
 		// entry decrypts with its own per-chunk SSE-KMS key.
 		fileId := chunk.GetFileIdString()
@@ -2699,9 +2718,7 @@ func (s3a *S3ApiServer) createMultipartSSEKMSDecryptedReaderDirect(ctx context.C

 	return &lazyMultipartChunkReader{
 		chunks: preparedChunks,
-		fetch: func(c *filer_pb.FileChunk) (io.ReadCloser, error) {
-			return s3a.createEncryptedChunkReader(ctx, c)
-		},
+		fetch:  fetchChunk,
 	}, nil
 }