From b6cc7297af26fce1cf0900fdc954142f5652ce6e Mon Sep 17 00:00:00 2001 From: chrislu Date: Mon, 17 Nov 2025 17:20:43 -0800 Subject: [PATCH] address comments --- weed/s3api/s3_sse_s3_multipart_test.go | 5 +- weed/s3api/s3api_object_handlers.go | 85 +++++++++++++++----------- 2 files changed, 54 insertions(+), 36 deletions(-) diff --git a/weed/s3api/s3_sse_s3_multipart_test.go b/weed/s3api/s3_sse_s3_multipart_test.go index 38f408b66..dda930ebd 100644 --- a/weed/s3api/s3_sse_s3_multipart_test.go +++ b/weed/s3api/s3_sse_s3_multipart_test.go @@ -64,7 +64,10 @@ func TestSSES3MultipartChunkViewDecryption(t *testing.T) { // Verify decryption works with the chunk's IV decryptedData := make([]byte, len(ciphertext)) - decryptBlock, _ := aes.NewCipher(key) + decryptBlock, err := aes.NewCipher(key) + if err != nil { + t.Fatalf("Failed to create decrypt cipher: %v", err) + } decryptStream := cipher.NewCTR(decryptBlock, chunkIV) decryptStream.XORKeyStream(decryptedData, ciphertext) diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 1662fb8c0..911895ff2 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -732,24 +732,7 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R return err } - // Set standard HTTP headers from entry metadata - // IMPORTANT: Set ALL headers BEFORE calling WriteHeader (headers are ignored after WriteHeader) - tHeaderSet := time.Now() - s3a.setResponseHeaders(w, entry, totalSize) - - // Override/add range-specific headers if this is a range request - if isRangeRequest { - w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", offset, offset+size-1, totalSize)) - w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) - } - headerSetTime = time.Since(tHeaderSet) - - // Now write status code (headers are all set) - if isRangeRequest { - w.WriteHeader(http.StatusPartialContent) - } - - // Get chunks + // Get chunks and validate BEFORE setting headers chunks := entry.GetChunks() glog.Infof("streamFromVolumeServers: entry has %d chunks, totalSize=%d, isRange=%v, offset=%d, size=%d", len(chunks), totalSize, isRangeRequest, offset, size) @@ -759,17 +742,13 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R if totalSize > 0 && len(entry.Content) == 0 { glog.Errorf("streamFromVolumeServers: Data integrity error - entry reports size %d but has no content or chunks", totalSize) // IMPORTANT: Write error status before returning, since headers haven't been written yet - // Clear any headers set by setResponseHeaders to avoid misleading Content-Length - w.Header().Del("Content-Length") - w.Header().Del("Content-Range") w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "Internal Server Error: data integrity issue (size %d reported but no content available)", totalSize) return fmt.Errorf("data integrity error: size %d reported but no content available", totalSize) } - // Empty object - need to set headers before writing status - if !isRangeRequest { - // Headers were already set by setResponseHeaders above - w.WriteHeader(http.StatusOK) - } + // Empty object - set headers and write status + s3a.setResponseHeaders(w, entry, totalSize) + w.WriteHeader(http.StatusOK) return nil } @@ -778,7 +757,8 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R glog.Infof(" GET Chunk[%d]: fid=%s, offset=%d, size=%d", i, chunk.GetFileIdString(), chunk.Offset, chunk.Size) } - // Create lookup function via filer client (reuse shared helper) + // CRITICAL: Resolve chunks and prepare stream BEFORE WriteHeader + // This ensures we can write proper error responses if these operations fail ctx := r.Context() lookupFileIdFn := s3a.createLookupFileIdFunction() @@ -788,7 +768,8 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R chunkResolveTime = time.Since(tChunkResolve) if err != nil { glog.Errorf("streamFromVolumeServers: failed to resolve chunks: %v", err) - // Don't try to write headers if we already wrote them for range request + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "Internal Server Error: failed to resolve chunks") return fmt.Errorf("failed to resolve chunks: %v", err) } @@ -810,10 +791,29 @@ func (s3a *S3ApiServer) streamFromVolumeServers(w http.ResponseWriter, r *http.R streamPrepTime = time.Since(tStreamPrep) if err != nil { glog.Errorf("streamFromVolumeServers: failed to prepare stream: %v", err) - // Don't try to write headers if we already wrote them for range request + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "Internal Server Error: failed to prepare stream") return fmt.Errorf("failed to prepare stream: %v", err) } + // All validation and preparation successful - NOW set headers and write status + tHeaderSet := time.Now() + s3a.setResponseHeaders(w, entry, totalSize) + + // Override/add range-specific headers if this is a range request + if isRangeRequest { + w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", offset, offset+size-1, totalSize)) + w.Header().Set("Content-Length", strconv.FormatInt(size, 10)) + } + headerSetTime = time.Since(tHeaderSet) + + // Now write status code (headers are all set, stream is ready) + if isRangeRequest { + w.WriteHeader(http.StatusPartialContent) + } else { + w.WriteHeader(http.StatusOK) + } + // Stream directly to response tStreamExec := time.Now() glog.Infof("streamFromVolumeServers: starting streamFn, offset=%d, size=%d", offset, size) @@ -1458,21 +1458,28 @@ func (s3a *S3ApiServer) decryptSSES3ChunkView(ctx context.Context, fileChunk *fi return nil, fmt.Errorf("failed to fetch full chunk: %w", err) } - // Use the chunk's IV directly (already adjusted for part offset during encryption) - // Note: SSE-S3 stores the offset-adjusted IV in chunk metadata, unlike SSE-C which stores base IV + PartOffset + // Use the chunk's IV directly (already adjusted for block offset during encryption) + // Note: SSE-S3 encryption flow: + // 1. Upload: CreateSSES3EncryptedReaderWithBaseIV(reader, key, baseIV, partOffset) + // calls calculateIVWithOffset(baseIV, partOffset) → (blockAlignedIV, skip) + // The blockAlignedIV is stored in chunk metadata + // 2. Download: We decrypt the FULL chunk from offset 0 using that blockAlignedIV + // Then skip to chunkView.OffsetInChunk in the PLAINTEXT (not ciphertext) + // This differs from SSE-C which stores base IV + PartOffset and calculates IV during decryption + // No ivSkip needed here because we're decrypting from chunk start (offset 0) iv := chunkSSES3Metadata.IV glog.V(4).Infof("Decrypting multipart SSE-S3 chunk %s with chunk-specific IV length=%d", chunkView.FileId, len(iv)) - // Decrypt the full chunk + // Decrypt the full chunk starting from offset 0 decryptedReader, decryptErr := CreateSSES3DecryptedReader(fullChunkReader, chunkSSES3Metadata, iv) if decryptErr != nil { fullChunkReader.Close() return nil, fmt.Errorf("failed to create SSE-S3 decrypted reader: %w", decryptErr) } - // Skip to position within chunk and limit to ViewSize + // Skip to position within the decrypted chunk (plaintext offset, not ciphertext offset) if chunkView.OffsetInChunk > 0 { _, err = io.CopyN(io.Discard, decryptedReader, chunkView.OffsetInChunk) if err != nil { @@ -2873,8 +2880,16 @@ func (s3a *S3ApiServer) createMultipartSSECDecryptedReaderDirect(ctx context.Con glog.V(4).Infof("Decrypting SSE-C chunk %s with IV=%x, PartOffset=%d", chunk.GetFileIdString(), chunkIV[:8], ssecMetadata.PartOffset) - // Note: For multipart SSE-C, each part was encrypted with offset=0 - // So we don't need to adjust the IV with PartOffset - just use the stored IV directly + // Note: SSE-C multipart behavior (differs from SSE-KMS/SSE-S3): + // - Upload: CreateSSECEncryptedReader generates RANDOM IV per part (no base IV + offset) + // - Metadata: PartOffset is stored but not used during encryption + // - Decryption: Use stored random IV directly (no offset adjustment needed) + // + // This differs from: + // - SSE-KMS/SSE-S3: Use base IV + calculateIVWithOffset(partOffset) during encryption + // - CopyObject: Applies calculateIVWithOffset to SSE-C (which may be incorrect) + // + // TODO: Investigate CopyObject SSE-C PartOffset handling for consistency decryptedChunkReader, decErr := CreateSSECDecryptedReader(chunkReader, customerKey, chunkIV) if decErr != nil { chunkReader.Close()