From 768530a0ce9c1677361d2d9394d68fef0f26a90e Mon Sep 17 00:00:00 2001 From: Piotr Szafarczyk Date: Thu, 25 Jun 2026 09:14:41 +0200 Subject: [PATCH 1/6] fix(router): emit Content-Length on intercepted GetObject responses Intercepted GetObject responses streamed the decrypted body without a Content-Length header, so Go's server fell back to chunked transfer encoding. s3cmd 2.4.0 reads content-length unconditionally and downloaded an empty file as a result. The full plaintext is already buffered before any bytes are written, so set Content-Length from len(plaintext) before WriteHeader. The upstream length describes the ciphertext (+28 bytes GCM overhead, or arbitrary for legacy objects) and can't be reused, mirroring the existing omission of x-amz-checksum-* headers. --- CHANGELOG.md | 7 ++++++ s3proxy/internal/router/object.go | 6 ++++++ s3proxy/internal/router/router_test.go | 30 ++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e779d8f..500c119 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,13 @@ Two version streams move independently: ## [Unreleased] +### Fixed +- **s3cmd `get` compatibility**: intercepted `GetObject` responses now + carry a `Content-Length` header reflecting the decrypted body size. + Previously the proxy streamed decrypted objects without a length, so + Go fell back to chunked transfer encoding and s3cmd 2.4.0 downloaded + an empty file. Downloads now complete with the correct size. + ### Chart - **`chart/1.9.3`** — Dashboard usability + Go runtime panels. diff --git a/s3proxy/internal/router/object.go b/s3proxy/internal/router/object.go index d7be9d6..884af18 100644 --- a/s3proxy/internal/router/object.go +++ b/s3proxy/internal/router/object.go @@ -16,6 +16,7 @@ import ( "net/http" "net/url" "runtime/debug" + "strconv" "strings" "syscall" "time" @@ -157,6 +158,11 @@ func (o object) get(w http.ResponseWriter, r *http.Request) { releaseLargeBuffer(&plaintext) return default: + // The full plaintext is already buffered, so emit an explicit Content-Length + // (the upstream value describes the ciphertext, not the decrypted body) so the + // server uses identity encoding instead of chunked. Some clients (e.g. s3cmd) + // require a Content-Length header on the response. + w.Header().Set("Content-Length", strconv.Itoa(len(plaintext))) w.WriteHeader(http.StatusOK) _, writeErr := w.Write(plaintext) releaseLargeBuffer(&plaintext) diff --git a/s3proxy/internal/router/router_test.go b/s3proxy/internal/router/router_test.go index 2eae233..a445ceb 100644 --- a/s3proxy/internal/router/router_test.go +++ b/s3proxy/internal/router/router_test.go @@ -13,6 +13,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" @@ -198,6 +199,35 @@ func TestGetObjectUsesRouterKEK(t *testing.T) { assert.Equal(t, "secret payload", rec.Body.String()) } +func TestGetObjectSetsPlaintextContentLength(t *testing.T) { + keks := newTestKEKs(t, "expected encryption key") + version, curKEK := keks.Current() + client := newEncryptedGetObjectClient(t, curKEK, version, []byte("secret payload")) + router := Router{keks: keks, log: testLogger()} + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rec := httptest.NewRecorder() + + router.getHandler(req, client, true, "key", "bucket").ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + // The header must reflect the plaintext length, not the +28-byte ciphertext. + assert.Equal(t, strconv.Itoa(len("secret payload")), rec.Result().Header.Get("Content-Length")) +} + +func TestGetObjectSetsZeroContentLengthForEmptyObject(t *testing.T) { + keks := newTestKEKs(t, "expected encryption key") + version, curKEK := keks.Current() + client := newEncryptedGetObjectClient(t, curKEK, version, []byte{}) + router := Router{keks: keks, log: testLogger()} + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rec := httptest.NewRecorder() + + router.getHandler(req, client, true, "key", "bucket").ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "0", rec.Result().Header.Get("Content-Length")) +} + func TestGetObjectFailsWithWrongRouterKEK(t *testing.T) { storedKeks := newTestKEKs(t, "old encryption key") routerKeks := newTestKEKs(t, "new encryption key") From 93361f6f317f4af65e3cc90e4cd4217acffa7a0f Mon Sep 17 00:00:00 2001 From: Piotr Szafarczyk Date: Thu, 25 Jun 2026 14:56:09 +0200 Subject: [PATCH 2/6] fix(router): return plaintext ETag on intercepted GetObject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Intercepted GETs decrypt the body but returned the upstream ETag, which S3 computes over the ciphertext at rest. Clients/SDKs that validate the body against the ETag, or cache by it, saw a mismatch. Override the response ETag with md5(plaintext) — the ETag S3 would have produced for the unencrypted object — computed on the fly from the buffer already in memory, so no stored metadata or migration is needed. Pass-through objects (no DEK tag) keep the upstream ETag. PUT-response and HEAD ETags still reflect the ciphertext and remain a known gap. --- CHANGELOG.md | 12 +++++++ s3proxy/internal/router/object.go | 9 ++++++ s3proxy/internal/router/router_test.go | 44 ++++++++++++++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e779d8f..88c4fc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,18 @@ Two version streams move independently: ## [Unreleased] +### Fixed +- **`GetObject` returns the plaintext ETag.** Intercepted GETs decrypt the + body but previously returned the upstream ETag, which S3 computes over the + ciphertext at rest. Clients (or SDKs) that validate the body against the + ETag, or cache by it, saw a mismatch. The proxy now overrides the response + ETag with `md5(plaintext)` — the ETag S3 would have produced for the + unencrypted object — computed on the fly from the buffer already held in + memory, so no stored metadata or migration is needed. Pass-through objects + (no DEK tag) keep the upstream ETag, which already describes the delivered + bytes. PUT-response and HEAD ETags still reflect the ciphertext and remain + a known consistency gap. + ### Chart - **`chart/1.9.3`** — Dashboard usability + Go runtime panels. diff --git a/s3proxy/internal/router/object.go b/s3proxy/internal/router/object.go index d7be9d6..4585acd 100644 --- a/s3proxy/internal/router/object.go +++ b/s3proxy/internal/router/object.go @@ -9,6 +9,7 @@ package router import ( "context" + "crypto/md5" "encoding/hex" "errors" "fmt" @@ -149,6 +150,14 @@ func (o object) get(w http.ResponseWriter, r *http.Request) { http.Error(w, "failed to decrypt object", http.StatusInternalServerError) return } + + // The upstream ETag is S3's MD5 of the ciphertext at rest, which does not + // match the decrypted body we deliver. Override it with the plaintext MD5 + // so clients that validate or cache by ETag see a consistent value. Single + // part only (multipart is blocked/forwarded), so ETag == hex(md5(content)). + //nolint:gosec // MD5 is not used as a security primitive here; it is the S3 ETag definition. + sum := md5.Sum(plaintext) + w.Header().Set("ETag", hex.EncodeToString(sum[:])) } select { diff --git a/s3proxy/internal/router/router_test.go b/s3proxy/internal/router/router_test.go index 2eae233..adf7fa7 100644 --- a/s3proxy/internal/router/router_test.go +++ b/s3proxy/internal/router/router_test.go @@ -8,6 +8,7 @@ package router import ( "bytes" "context" + "crypto/md5" "encoding/hex" "io" "log/slog" @@ -198,6 +199,49 @@ func TestGetObjectUsesRouterKEK(t *testing.T) { assert.Equal(t, "secret payload", rec.Body.String()) } +func TestGetObjectReturnsPlaintextETag(t *testing.T) { + keks := newTestKEKs(t, "expected encryption key") + version, curKEK := keks.Current() + plaintext := []byte("secret payload") + client := newEncryptedGetObjectClient(t, curKEK, version, plaintext) + // Upstream ETag describes the ciphertext at rest, not the body we deliver. + upstreamETag := "\"deadbeefdeadbeefdeadbeefdeadbeef\"" + client.getObjectOut.ETag = &upstreamETag + router := Router{keks: keks, log: testLogger()} + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rec := httptest.NewRecorder() + + router.getHandler(req, client, true, "key", "bucket").ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + want := md5.Sum(plaintext) + assert.Equal(t, hex.EncodeToString(want[:]), rec.Header().Get("ETag")) + assert.NotEqual(t, strings.Trim(upstreamETag, "\""), rec.Header().Get("ETag")) +} + +func TestGetObjectPassThroughKeepsUpstreamETag(t *testing.T) { + // No DEK metadata tag: the object was not proxy-encrypted, so the upstream + // ETag already describes the bytes we deliver and must be left untouched. + body := []byte("plain passthrough") + upstreamETag := "\"deadbeefdeadbeefdeadbeefdeadbeef\"" + client := &recordingS3Client{ + getObjectOut: &s3.GetObjectOutput{ + Body: io.NopCloser(bytes.NewReader(body)), + ContentLength: awsInt64(int64(len(body))), + ETag: &upstreamETag, + }, + } + router := Router{keks: newTestKEKs(t, "expected encryption key"), log: testLogger()} + req := httptest.NewRequest(http.MethodGet, "/bucket/key", nil) + rec := httptest.NewRecorder() + + router.getHandler(req, client, true, "key", "bucket").ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, body, rec.Body.Bytes()) + assert.Equal(t, strings.Trim(upstreamETag, "\""), rec.Header().Get("ETag")) +} + func TestGetObjectFailsWithWrongRouterKEK(t *testing.T) { storedKeks := newTestKEKs(t, "old encryption key") routerKeks := newTestKEKs(t, "new encryption key") From b372aa1a2a2f240a34760e6209c0ee6f23c70180 Mon Sep 17 00:00:00 2001 From: Piotr Szafarczyk Date: Thu, 25 Jun 2026 16:02:29 +0200 Subject: [PATCH 3/6] feat(router): report decrypted plaintext size in ListObjects responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListObjects/ListObjectsV2 previously reported the at-rest ciphertext size (plaintext + 28-byte AES-GCM-SIV envelope). Intercept bucket-level list requests, subtract the fixed encryption overhead from every , and clamp at 0. Bucket sub-resource GETs (acl, versioning, multipart listings, ?versions, …) are still forwarded unchanged. - cryptoutil: export EncryptionOverhead constant + invariant test - router: bucketOnlyPattern + isListObjects, wired into getHandler - handler: extract forwardUpstream; add handleListObjects (buffer, rewrite on 2xx, fix Content-Length) - listsize: byte-preserving regex rewrite + unit tests - e2e: assert v1 and v2 listings report plaintext size Known limitation: objects not written through the proxy (legacy plaintext, server-side copies, multipart) are reported 28 bytes short / 0 after clamp, since list responses carry no per-object encryption metadata. --- CHANGELOG.md | 12 +++ s3proxy/internal/cryptoutil/cryptoutil.go | 5 + .../internal/cryptoutil/cryptoutil_test.go | 22 ++++ s3proxy/internal/e2e/proxy_e2e_test.go | 16 +++ s3proxy/internal/router/handler.go | 101 +++++++++++++----- s3proxy/internal/router/listsize.go | 39 +++++++ s3proxy/internal/router/listsize_test.go | 62 +++++++++++ s3proxy/internal/router/router.go | 39 ++++++- 8 files changed, 271 insertions(+), 25 deletions(-) create mode 100644 s3proxy/internal/router/listsize.go create mode 100644 s3proxy/internal/router/listsize_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e779d8f..dd0a74c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,18 @@ Two version streams move independently: ## [Unreleased] +### Added + +- `ListObjects`/`ListObjectsV2` responses now report the **decrypted plaintext + size** of each object instead of the larger at-rest ciphertext size. The proxy + intercepts bucket-level list requests, subtracts the fixed 28-byte encryption + overhead (12-byte nonce + 16-byte GCM tag) from every ``, and clamps at 0. + Bucket sub-resource GETs (acl, versioning, multipart listings, `?versions`, …) + are still forwarded unchanged. + - *Known limitation:* objects **not** written through the proxy (legacy + plaintext, server-side copies, multipart) are reported 28 bytes short (or 0 + after clamp), since a list response carries no per-object encryption metadata. + ### Chart - **`chart/1.9.3`** — Dashboard usability + Go runtime panels. diff --git a/s3proxy/internal/cryptoutil/cryptoutil.go b/s3proxy/internal/cryptoutil/cryptoutil.go index 1a0be9b..2e5aabe 100644 --- a/s3proxy/internal/cryptoutil/cryptoutil.go +++ b/s3proxy/internal/cryptoutil/cryptoutil.go @@ -18,6 +18,11 @@ import ( "github.com/tink-crypto/tink-go/v2/subtle/random" ) +// EncryptionOverhead is the fixed number of bytes Encrypt adds to the plaintext: +// a 12-byte AES-GCM-SIV nonce prefix plus a 16-byte authentication tag suffix. +// Ciphertext length is always plaintext length + EncryptionOverhead. +const EncryptionOverhead = 28 + // Encrypt generates a random key to encrypt a plaintext using AES-256-GCM. // The generated key is encrypted using the supplied key encryption key (KEK). // The ciphertext and encrypted data encryption key (DEK) are returned. diff --git a/s3proxy/internal/cryptoutil/cryptoutil_test.go b/s3proxy/internal/cryptoutil/cryptoutil_test.go index 6412452..17ec09d 100644 --- a/s3proxy/internal/cryptoutil/cryptoutil_test.go +++ b/s3proxy/internal/cryptoutil/cryptoutil_test.go @@ -46,3 +46,25 @@ func TestEncryptDecrypt(t *testing.T) { }) } } + +// TestEncryptionOverhead asserts that Encrypt adds exactly EncryptionOverhead bytes to the +// plaintext, regardless of plaintext size. This invariant is what lets the router subtract a +// constant to report decrypted sizes in LIST responses; it fails loudly if the envelope changes. +func TestEncryptionOverhead(t *testing.T) { + for _, size := range []int{0, 1, 28, 29, 1024, 1610862} { + t.Run(fmt.Sprintf("size_%d", size), func(t *testing.T) { + kek := [32]byte{} + _, err := rand.Read(kek[:]) + require.NoError(t, err) + + plaintext := make([]byte, size) + _, err = rand.Read(plaintext) + require.NoError(t, err) + + ciphertext, _, err := Encrypt(plaintext, kek) + require.NoError(t, err) + + assert.Equal(t, size+EncryptionOverhead, len(ciphertext)) + }) + } +} diff --git a/s3proxy/internal/e2e/proxy_e2e_test.go b/s3proxy/internal/e2e/proxy_e2e_test.go index f3fa1db..0af7bc4 100644 --- a/s3proxy/internal/e2e/proxy_e2e_test.go +++ b/s3proxy/internal/e2e/proxy_e2e_test.go @@ -167,6 +167,22 @@ func TestProxyRoundtripAgainstMinio(t *testing.T) { require.NoError(t, got.Body.Close()) assert.Equal(t, plaintext, gotBody) + // Listing through the proxy must report the decrypted plaintext size, not the larger + // ciphertext size that actually sits at rest. Cover both ListObjectsV2 and ListObjects v1. + listV2, err := proxyClient.ListObjectsV2(ctx, &awss3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + }) + require.NoError(t, err) + require.Len(t, listV2.Contents, 1) + assert.EqualValues(t, len(plaintext), *listV2.Contents[0].Size, "ListObjectsV2 must report plaintext size") + + listV1, err := proxyClient.ListObjects(ctx, &awss3.ListObjectsInput{ + Bucket: aws.String(bucket), + }) + require.NoError(t, err) + require.Len(t, listV1.Contents, 1) + assert.EqualValues(t, len(plaintext), *listV1.Contents[0].Size, "ListObjects v1 must report plaintext size") + // Housekeeping endpoints stay reachable while traffic is flowing. healthResp, err := http.Get(proxyURL.String() + "/healthz") require.NoError(t, err) diff --git a/s3proxy/internal/router/handler.go b/s3proxy/internal/router/handler.go index 5871879..0f8a4de 100644 --- a/s3proxy/internal/router/handler.go +++ b/s3proxy/internal/router/handler.go @@ -15,6 +15,7 @@ import ( "io" "log/slog" "net/http" + "strconv" "time" v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" @@ -138,59 +139,111 @@ func handlePutObject(client s3Client, key string, bucket string, keks cryptoutil } } +// forwardUpstream repackages, signs and sends req to the upstream S3 API, returning the +// upstream response. The caller owns resp.Body and must close it. metrics may be nil. +func forwardUpstream(client *s3.Client, req *http.Request, metrics *monitoring.Metrics) (*http.Response, error) { + newReq, err := repackage(req) + if err != nil { + return nil, fmt.Errorf("repackaging request: %w", err) + } + + cfg := client.GetConfig() + + creds, err := cfg.Credentials.Retrieve(context.TODO()) + if err != nil { + return nil, fmt.Errorf("retrieving aws creds: %w", err) + } + + signer := v4.NewSigner() + + err = signer.SignHTTP(context.TODO(), creds, newReq, newReq.Header.Get("X-Amz-Content-Sha256"), "s3", cfg.Region, time.Now()) + if err != nil { + return nil, fmt.Errorf("signing request: %w", err) + } + + resp, err := forwardHTTPClient.Do(newReq) + if err != nil { + if metrics != nil { + metrics.UpstreamErrors.Inc() + } + return nil, fmt.Errorf("do request: %w", err) + } + + return resp, nil +} + func handleForwards(client *s3.Client, log *slog.Logger, metrics *monitoring.Metrics) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { log.Debug("forwarding", "path", req.URL.Path, "method", req.Method, "host", req.Host) - newReq, err := repackage(req) + resp, err := forwardUpstream(client, req, metrics) if err != nil { - log.Error("failed to repackage request", "error", err) + log.Error("forwarding request", "error", err) http.Error(w, "internal server error", http.StatusInternalServerError) return } + defer func() { + if cerr := resp.Body.Close(); cerr != nil { + log.Error("failed to close upstream response body", "error", cerr) + } + }() - cfg := client.GetConfig() + // Preserve multi-value headers (Set-Cookie, Vary, etc.). + for key, values := range resp.Header { + w.Header()[key] = values + } + w.WriteHeader(resp.StatusCode) - creds, err := cfg.Credentials.Retrieve(context.TODO()) - if err != nil { - log.Error("unable to retrieve aws creds", "error", err) - http.Error(w, "internal server error", http.StatusInternalServerError) - return + if _, err := io.Copy(w, resp.Body); err != nil { + log.Error("failed to stream response", "error", err) + // Headers already written; cannot send error response. } + } +} - signer := v4.NewSigner() +// handleListObjects forwards a ListObjects/ListObjectsV2 request upstream, then rewrites the +// reported object sizes in the XML response to the decrypted plaintext size (ciphertext size +// minus the fixed encryption overhead). List pages are bounded (≤1000 keys), so buffering the +// body is safe. Only successful (2xx) responses are rewritten; everything else passes through. +func handleListObjects(client *s3.Client, log *slog.Logger, metrics *monitoring.Metrics) http.HandlerFunc { + return func(w http.ResponseWriter, req *http.Request) { + log.Debug("intercepting ListObjects", "path", req.URL.Path, "method", req.Method, "host", req.Host) - err = signer.SignHTTP(context.TODO(), creds, newReq, newReq.Header.Get("X-Amz-Content-Sha256"), "s3", cfg.Region, time.Now()) + resp, err := forwardUpstream(client, req, metrics) if err != nil { - log.Error("failed to sign request", "error", err) + log.Error("forwarding ListObjects request", "error", err) http.Error(w, "internal server error", http.StatusInternalServerError) return } - - resp, err := forwardHTTPClient.Do(newReq) - if err != nil { - log.Error("do request", "error", err) - if metrics != nil { - metrics.UpstreamErrors.Inc() - } - http.Error(w, "do request: "+err.Error(), http.StatusInternalServerError) - return - } defer func() { if cerr := resp.Body.Close(); cerr != nil { log.Error("failed to close upstream response body", "error", cerr) } }() + body, err := io.ReadAll(resp.Body) + if err != nil { + log.Error("reading ListObjects response body", "error", err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + + out := body + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + out = rewriteListSizes(body) + } + // Preserve multi-value headers (Set-Cookie, Vary, etc.). for key, values := range resp.Header { w.Header()[key] = values } + // The rewritten body is shorter than the upstream ciphertext sizes, so the upstream + // Content-Length no longer applies — set it to the actual body length. + w.Header().Set("Content-Length", strconv.Itoa(len(out))) w.WriteHeader(resp.StatusCode) - if _, err := io.Copy(w, resp.Body); err != nil { - log.Error("failed to stream response", "error", err) - // Headers already written; cannot send error response. + if _, err := w.Write(out); err != nil { + log.Error("failed to write ListObjects response", "error", err) } } } diff --git a/s3proxy/internal/router/listsize.go b/s3proxy/internal/router/listsize.go new file mode 100644 index 0000000..91044094 --- /dev/null +++ b/s3proxy/internal/router/listsize.go @@ -0,0 +1,39 @@ +/* +Copyright (c) Intrinsec 2024 + +SPDX-License-Identifier: AGPL-3.0-only +*/ + +package router + +import ( + "fmt" + "regexp" + "strconv" + + "github.com/intrinsec/s3proxy/internal/cryptoutil" +) + +// sizeElementPattern matches a N element. In ListObjects v1/v2 responses this +// element only appears inside , so a targeted byte-preserving substitution avoids +// the namespace/formatting loss of an encoding/xml round-trip that could break S3 clients. +var sizeElementPattern = regexp.MustCompile(`(\d+)`) + +// rewriteListSizes replaces each N in a ListObjects response with +// max(0, N-EncryptionOverhead), reporting the decrypted plaintext size. The clamp at 0 covers +// 0-byte folder markers and objects not written through the proxy (smaller than the overhead). +// Everything outside the matched elements is preserved byte-for-byte. +func rewriteListSizes(body []byte) []byte { + return sizeElementPattern.ReplaceAllFunc(body, func(m []byte) []byte { + n, err := strconv.Atoi(string(sizeElementPattern.FindSubmatch(m)[1])) + if err != nil { + // Unparseable (e.g. overflows int) — leave the element untouched. + return m + } + dec := n - cryptoutil.EncryptionOverhead + if dec < 0 { + dec = 0 + } + return []byte(fmt.Sprintf("%d", dec)) + }) +} diff --git a/s3proxy/internal/router/listsize_test.go b/s3proxy/internal/router/listsize_test.go new file mode 100644 index 0000000..c5922de --- /dev/null +++ b/s3proxy/internal/router/listsize_test.go @@ -0,0 +1,62 @@ +/* +Copyright (c) Intrinsec 2024 + +SPDX-License-Identifier: AGPL-3.0-only +*/ + +package router + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRewriteListSizes(t *testing.T) { + tests := map[string]struct { + in int + want int + }{ + "zero clamps to zero": {in: 0, want: 0}, + "below overhead clamps to zero": {in: 5, want: 0}, + "exactly overhead clamps to zero": {in: 28, want: 0}, + "one byte plaintext": {in: 29, want: 1}, + "large object": {in: 1610890, want: 1610862}, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + body := []byte(fmt.Sprintf("%d", tt.in)) + got := rewriteListSizes(body) + assert.Equal(t, fmt.Sprintf("%d", tt.want), string(got)) + }) + } +} + +// TestRewriteListSizesPreservesStructure asserts only values change; surrounding XML — +// including CommonPrefixes (directories, which carry no ) — is byte-for-byte preserved. +func TestRewriteListSizesPreservesStructure(t *testing.T) { + body := []byte(`` + + `` + + `bucket` + + `a.txt1610890"abc"` + + `b.txt28` + + `dir/` + + ``) + + want := []byte(`` + + `` + + `bucket` + + `a.txt1610862"abc"` + + `b.txt0` + + `dir/` + + ``) + + assert.Equal(t, string(want), string(rewriteListSizes(body))) +} + +func TestRewriteListSizesNoSizeElement(t *testing.T) { + body := []byte(`dir/`) + assert.Equal(t, string(body), string(rewriteListSizes(body))) +} diff --git a/s3proxy/internal/router/router.go b/s3proxy/internal/router/router.go index 9815f86..f4e52ca 100644 --- a/s3proxy/internal/router/router.go +++ b/s3proxy/internal/router/router.go @@ -43,8 +43,41 @@ import ( var ( bucketAndKeyPattern = regexp.MustCompile("/([^/?]+)/(.+)") + // bucketOnlyPattern matches a bucket-level path (no object key), with an optional + // trailing slash. Used to detect ListObjects/ListObjectsV2 requests. + bucketOnlyPattern = regexp.MustCompile("^/([^/?]+)/?$") ) +// listSubResourceMarkers are query parameters that turn a bucket-level GET into a +// sub-resource request (bucket ACL, versioning, multipart listing, …). Those return +// XML that has no , or is out of scope (versions), so they must not be +// rewritten — only a "clean" ListObjects v1/v2 carries object sizes. +var listSubResourceMarkers = []string{ + "acl", "policy", "versioning", "location", "tagging", "lifecycle", "cors", + "website", "replication", "encryption", "object-lock", "accelerate", "analytics", + "intelligent-tiering", "inventory", "metrics", "logging", "notification", + "ownershipControls", "publicAccessBlock", "requestPayment", "uploads", "versions", +} + +// isListObjects reports whether req is a clean ListObjects (v1) or ListObjectsV2 request, +// i.e. a bucket-level GET with no sub-resource marker query parameters. Both v1 (no +// list-type) and v2 (list-type=2) match. +func isListObjects(req *http.Request) bool { + if req.Method != http.MethodGet { + return false + } + if !bucketOnlyPattern.MatchString(req.URL.Path) { + return false + } + query := req.URL.Query() + for _, marker := range listSubResourceMarkers { + if _, ok := query[marker]; ok { + return false + } + } + return true +} + // Router implements the interception logic for the s3proxy. type Router struct { region string @@ -374,8 +407,12 @@ func (r Router) getHandler(req *http.Request, client s3Client, matchingPath bool }) } - // Forward if path doesn't match + // Forward if path doesn't match. Bucket-level ListObjects requests land here (they have + // no object key); intercept them to report decrypted plaintext sizes. if !matchingPath { + if s3Client, ok := client.(*s3.Client); ok && isListObjects(req) { + return handleListObjects(s3Client, r.log, r.metrics) + } return forwardHandler() } From 661dad74e33ece72e7418535f34c6f582b24e639 Mon Sep 17 00:00:00 2001 From: Shevchenko Vitaliy Date: Fri, 17 Jul 2026 14:04:55 +0300 Subject: [PATCH 4/6] feat(config): make S3 operation timeout configurable --- CHANGELOG.md | 6 +++++ README.md | 1 + s3proxy/internal/config/config.go | 19 ++++++++++++++ s3proxy/internal/config/validation.go | 10 ++++++++ s3proxy/internal/config/validation_test.go | 29 ++++++++++++++++++++++ s3proxy/internal/router/object.go | 10 ++------ 6 files changed, 67 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e779d8f..40ee3c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ Two version streams move independently: ## [Unreleased] +### Added + +- Configurable timeout for upstream `GetObject` and `PutObject` operations via + `S3PROXY_S3_OPERATION_TIMEOUT`; defaults to 120 seconds and accepts values up + to 30 minutes. + ### Chart - **`chart/1.9.3`** — Dashboard usability + Go runtime panels. diff --git a/README.md b/README.md index 8efcbe4..d370645 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ All runtime configuration is sourced from environment variables. The Helm chart | `AWS_SECRET_ACCESS_KEY` | yes | — | Idem. | | `S3PROXY_THROTTLING_REQUESTSMAX` | no | `0` (off) | Cap on **concurrent in-flight requests** (not RPS). Excess requests are rejected. | | `S3PROXY_PUTBODY_MAX` | no | `268435456` (256 MiB) | Per-request PutObject body size ceiling, in bytes. Up to `5368709120` (5 GiB, the S3 hard cap). | +| `S3PROXY_S3_OPERATION_TIMEOUT` | no | `120` (2 min) | Upper bound, in seconds, on a single upstream GetObject/PutObject call. Raise this for large objects on slow links. Up to `1800` (30 min). | | `S3PROXY_DEKTAG_NAME` | no | `isec` | S3 object-metadata key used to store the encrypted DEK. | | `S3PROXY_DEKTAG_KEKVER` | no | `-kek-ver` | S3 object-metadata key recording which KEK derivation version wrapped the DEK. | | `S3PROXY_INSECURE` | no | unset | Set to `1` to use plain HTTP (not HTTPS) when talking to upstream. **Dev / e2e only.** Emits a loud warning at startup. | diff --git a/s3proxy/internal/config/config.go b/s3proxy/internal/config/config.go index 7c41a7d..6d7d5a5 100644 --- a/s3proxy/internal/config/config.go +++ b/s3proxy/internal/config/config.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "strings" + "time" "github.com/knadh/koanf/providers/env" "github.com/knadh/koanf/v2" @@ -95,6 +96,21 @@ func (c *Config) MaxPutBodySize() int64 { return v } +// S3OperationTimeout returns the configured upper bound on a single S3 +// GetObject/PutObject call. Falls back to DefaultS3OperationTimeout when +// S3PROXY_S3_OPERATION_TIMEOUT (seconds) is unset, zero, or out of range. +func (c *Config) S3OperationTimeout() time.Duration { + const key = "s3proxy.s3.operation.timeout" + if !c.k.Exists(key) { + return DefaultS3OperationTimeout + } + v := time.Duration(c.k.Int64(key)) * time.Second + if v <= 0 || v > MaxS3OperationTimeout { + return DefaultS3OperationTimeout + } + return v +} + // DecryptionFallback reports whether GetObject should retry decryption with an // all-zero KEK when the configured KEK fails (S3PROXY_DECRYPTION_FALLBACK=1). // Intended for one-shot migrations away from objects written without an @@ -155,5 +171,8 @@ func GetMaxPutBodySize() int64 { return Default().MaxPutBodySize() } // GetInsecure returns whether upstream S3 traffic should use http:// from the default config. func GetInsecure() bool { return Default().Insecure() } +// GetS3OperationTimeout returns the S3 operation timeout from the default config. +func GetS3OperationTimeout() time.Duration { return Default().S3OperationTimeout() } + // GetDecryptionFallback returns the decryption-fallback flag from the default config. func GetDecryptionFallback() bool { return Default().DecryptionFallback() } diff --git a/s3proxy/internal/config/validation.go b/s3proxy/internal/config/validation.go index 48ea508..edb56a9 100644 --- a/s3proxy/internal/config/validation.go +++ b/s3proxy/internal/config/validation.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "regexp" + "time" ) // Validate asserts that the required s3proxy configuration is present and well-formed. @@ -93,6 +94,15 @@ const MaxObjectSize = 5 * 1024 * 1024 * 1024 // streaming encryption path is introduced. const DefaultMaxPutBodySize = 256 * 1024 * 1024 +// DefaultS3OperationTimeout is the default upper bound on a single S3 +// GetObject/PutObject call. Large objects on slow links can need more time; +// operators can raise this via S3PROXY_S3_OPERATION_TIMEOUT (seconds). +const DefaultS3OperationTimeout = 2 * time.Minute + +// MaxS3OperationTimeout bounds how high S3PROXY_S3_OPERATION_TIMEOUT can be set, +// so a misconfiguration cannot leave requests hanging indefinitely. +const MaxS3OperationTimeout = 30 * time.Minute + // ValidateContentLength validates the content length of a request against the S3 hard cap. func ValidateContentLength(contentLength int64) error { if contentLength < 0 { diff --git a/s3proxy/internal/config/validation_test.go b/s3proxy/internal/config/validation_test.go index 422c732..90a381a 100644 --- a/s3proxy/internal/config/validation_test.go +++ b/s3proxy/internal/config/validation_test.go @@ -9,7 +9,9 @@ package config import ( "strings" "testing" + "time" + "github.com/knadh/koanf/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -104,3 +106,30 @@ func TestValidatePutBodySize(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "exceeds PutObject body cap") } + +func TestS3OperationTimeout(t *testing.T) { + tests := map[string]struct { + value string + want time.Duration + }{ + "valid": {value: "300", want: 5 * time.Minute}, + "at maximum": {value: "1800", want: MaxS3OperationTimeout}, + "zero": {value: "0", want: DefaultS3OperationTimeout}, + "negative": {value: "-1", want: DefaultS3OperationTimeout}, + "over maximum": {value: "1801", want: DefaultS3OperationTimeout}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + t.Setenv("S3PROXY_S3_OPERATION_TIMEOUT", tc.value) + cfg, err := Load() + require.NoError(t, err) + assert.Equal(t, tc.want, cfg.S3OperationTimeout()) + }) + } + + t.Run("unset", func(t *testing.T) { + cfg := &Config{k: koanf.New(".")} + assert.Equal(t, DefaultS3OperationTimeout, cfg.S3OperationTimeout()) + }) +} diff --git a/s3proxy/internal/router/object.go b/s3proxy/internal/router/object.go index d7be9d6..f0141fc 100644 --- a/s3proxy/internal/router/object.go +++ b/s3proxy/internal/router/object.go @@ -45,12 +45,6 @@ func releaseLargeBuffer(buf *[]byte) { } } -// s3OperationTimeout bounds the total duration of an individual S3 GetObject/PutObject call. -// We detach from the request context (context.WithoutCancel) so a client disconnect does not -// abort a partially-uploaded PutObject, but we still cap overall work to protect against -// hung upstreams producing zombie requests. -const s3OperationTimeout = 2 * time.Minute - // object bundles data to implement http.Handler methods that use data from incoming requests. type object struct { keks cryptoutil.KEKProvider @@ -84,7 +78,7 @@ func (o object) get(w http.ResponseWriter, r *http.Request) { // Detach from the request cancellation to avoid aborting an S3 operation when // the client disconnects mid-flight, but cap the total duration so a hung // upstream cannot produce a zombie request. - ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), s3OperationTimeout) + ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), config.GetS3OperationTimeout()) defer cancel() output, err := o.client.GetObject(ctx, o.bucket, o.key, o.versionID, o.sseCustomerAlgorithm, o.sseCustomerKey, o.sseCustomerKeyMD5) @@ -211,7 +205,7 @@ func (o object) put(w http.ResponseWriter, r *http.Request) { o.metadata[config.GetDekTagName()] = hex.EncodeToString(encryptedDEK) o.metadata[config.GetKEKVersionTagName()] = kekVersion - ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), s3OperationTimeout) + ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), config.GetS3OperationTimeout()) defer cancel() output, err := o.client.PutObject(ctx, o.bucket, o.key, o.tags, o.contentType, o.objectLockLegalHoldStatus, o.objectLockMode, o.sseCustomerAlgorithm, o.sseCustomerKey, o.sseCustomerKeyMD5, o.objectLockRetainUntilDate, o.metadata, ciphertext) From 97cc321d7644598a7ddfa351e6db0b2a100439d8 Mon Sep 17 00:00:00 2001 From: lgs <11679637+louisgls@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:29:36 +0200 Subject: [PATCH 5/6] feat(chart): add deploymentAnnotations for Deployment metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chart only exposed `podAnnotations`, which renders onto the pod template. Controllers that watch the workload object itself had no way to be configured through values. The concrete failure: the TLS cert secret is mounted with `subPath`, and kubelet never refreshes a subPath volume in place. When cert-manager rotates the certificate, the running pods keep serving the old one until they are restarted. Stakater Reloader does exactly that, but it reads `reloader.stakater.com/auto` from `Deployment.metadata.annotations` — which the chart could not render. The proxy therefore served an expired certificate and every TLS client failed with `x509: certificate has expired`. Add `deploymentAnnotations` (default `{}`, declared in values.schema.json) and render it onto the Deployment metadata. No behavior change when unset. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 10 +++++++++- charts/s3proxy/Chart.yaml | 2 +- charts/s3proxy/templates/deployment.yaml | 4 ++++ charts/s3proxy/values.schema.json | 1 + charts/s3proxy/values.yaml | 9 +++++++++ 5 files changed, 24 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e779d8f..c127190 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,15 @@ Two version streams move independently: ### Chart -- **`chart/1.9.3`** — Dashboard usability + Go runtime panels. +- **`chart/1.10.0`** — New `deploymentAnnotations` value, rendered onto the + `Deployment` object's own `metadata.annotations`. Until now the chart only + exposed `podAnnotations`, which lands on the pod template — so controllers + that watch the workload metadata had no way in. The motivating case is + Stakater Reloader (`reloader.stakater.com/auto: "true"`): the TLS secret is + mounted with `subPath`, which kubelet never refreshes in place, so a renewed + certificate is only picked up when the pods restart. Without the annotation + the proxy keeps serving the expired certificate after cert-manager rotates + it, and every TLS client fails with `x509: certificate has expired`. - `Job` picker now defaults to **All** (`allValue: ".*s3proxy.*"`) and restricts its dropdown to jobs whose name contains `s3proxy` via the template `regex: /s3proxy/`. Multi-release clusters land on a diff --git a/charts/s3proxy/Chart.yaml b/charts/s3proxy/Chart.yaml index 7884401..8a8d384 100644 --- a/charts/s3proxy/Chart.yaml +++ b/charts/s3proxy/Chart.yaml @@ -18,5 +18,5 @@ maintainers: annotations: org.opencontainers.image.source: "https://github.com/intrinsec/s3proxy/" type: application -version: 1.9.3 +version: 1.10.0 appVersion: "1.8.1" diff --git a/charts/s3proxy/templates/deployment.yaml b/charts/s3proxy/templates/deployment.yaml index f7e2471..2520d6c 100644 --- a/charts/s3proxy/templates/deployment.yaml +++ b/charts/s3proxy/templates/deployment.yaml @@ -4,6 +4,10 @@ metadata: name: {{ include "s3proxy.fullname" . }} labels: {{- include "s3proxy.labels" . | nindent 4 }} + {{- with .Values.deploymentAnnotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} spec: {{- if not .Values.autoscaling.enabled }} replicas: {{ .Values.replicaCount }} diff --git a/charts/s3proxy/values.schema.json b/charts/s3proxy/values.schema.json index 38ee718..ef1ef02 100644 --- a/charts/s3proxy/values.schema.json +++ b/charts/s3proxy/values.schema.json @@ -99,6 +99,7 @@ "type": "object", "additionalProperties": true }, + "deploymentAnnotations": { "type": "object" }, "podAnnotations": { "type": "object" }, "podLabels": { "type": "object" }, "podSecurityContext": { "type": "object" }, diff --git a/charts/s3proxy/values.yaml b/charts/s3proxy/values.yaml index 4cc25b1..df55c2b 100644 --- a/charts/s3proxy/values.yaml +++ b/charts/s3proxy/values.yaml @@ -80,6 +80,15 @@ valsSecret: {} # secretKey: ref+vault://s3proxy/secrets/secret-key # encryptKey: ref+vault://s3proxy/secrets/encrypt-key +# Annotations set on the Deployment object itself (not the pod template). +# Required by controllers that watch the workload metadata, e.g. Stakater +# Reloader, which restarts the pods when the TLS secret is renewed: +# deploymentAnnotations: +# reloader.stakater.com/auto: "true" +# The cert secret is mounted with subPath, so kubelet never refreshes it +# in-place — a pod restart is the only way to pick up a renewed certificate. +deploymentAnnotations: {} + podAnnotations: {} podLabels: {} From 547cbd3c53d5df5861ea46b1c890ecbd290a9e61 Mon Sep 17 00:00:00 2001 From: lgs Date: Wed, 29 Jul 2026 14:58:20 +0200 Subject: [PATCH 6/6] chore(release): v1.9.0 + chart 1.10.0 Bundles the client-compatibility fixes and the configurable operation timeout, and bumps the chart appVersion to match. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 9 +++++++-- charts/s3proxy/Chart.yaml | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5455be3..d048040 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ Two version streams move independently: ## [Unreleased] +## [1.9.0] — 2026-07-29 + +Ships the S3 client-compatibility fixes (`Content-Length`, plaintext `ETag`), +plaintext sizes in `ListObjects`, and a configurable upstream operation timeout. +Chart `1.10.0` accompanies this release. + ### Fixed - **s3cmd `get` compatibility**: intercepted `GetObject` responses now carry a `Content-Length` header reflecting the decrypted body size. @@ -30,6 +36,7 @@ Two version streams move independently: (no DEK tag) keep the upstream ETag, which already describes the delivered bytes. PUT-response and HEAD ETags still reflect the ciphertext and remain a known consistency gap. + ### Added - `ListObjects`/`ListObjectsV2` responses now report the **decrypted plaintext @@ -41,8 +48,6 @@ Two version streams move independently: - *Known limitation:* objects **not** written through the proxy (legacy plaintext, server-side copies, multipart) are reported 28 bytes short (or 0 after clamp), since a list response carries no per-object encryption metadata. -### Added - - Configurable timeout for upstream `GetObject` and `PutObject` operations via `S3PROXY_S3_OPERATION_TIMEOUT`; defaults to 120 seconds and accepts values up to 30 minutes. diff --git a/charts/s3proxy/Chart.yaml b/charts/s3proxy/Chart.yaml index 8a8d384..d07ac76 100644 --- a/charts/s3proxy/Chart.yaml +++ b/charts/s3proxy/Chart.yaml @@ -19,4 +19,4 @@ annotations: org.opencontainers.image.source: "https://github.com/intrinsec/s3proxy/" type: application version: 1.10.0 -appVersion: "1.8.1" +appVersion: "1.9.0"