From f083fa9fedf819c550d25c5d70279ad6a3786b8d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 26 Apr 2026 15:09:52 -0700 Subject: [PATCH] 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. --- weed/s3api/s3api_multipart_ssekms_test.go | 241 ++++++++++++++++++++++ weed/s3api/s3api_object_handlers.go | 43 ++-- 2 files changed, 271 insertions(+), 13 deletions(-) create mode 100644 weed/s3api/s3api_multipart_ssekms_test.go diff --git a/weed/s3api/s3api_multipart_ssekms_test.go b/weed/s3api/s3api_multipart_ssekms_test.go new file mode 100644 index 000000000..2b168d03b --- /dev/null +++ b/weed/s3api/s3api_multipart_ssekms_test.go @@ -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) + } +} diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 2f9f067c8..c35807a95 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -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 }