Skip to content

feat(multipart): buffered (encrypted) multipart uploads + list/get fidelity fixes - #52

Open
pszafarczyk wants to merge 9 commits into
Intrinsec:mainfrom
pszafarczyk:encryptedmultipart
Open

feat(multipart): buffered (encrypted) multipart uploads + list/get fidelity fixes#52
pszafarczyk wants to merge 9 commits into
Intrinsec:mainfrom
pszafarczyk:encryptedmultipart

Conversation

@pszafarczyk

Copy link
Copy Markdown

Summary

Adds an opt-in buffered multipart upload mode that keeps data at rest encrypted (the existing --allow-multipart
forwards plaintext upstream), plus a set of fixes that make intercepted reads and listings report the plaintext view
clients expect.

What changed

Buffered multipart uploads (opt-in via S3PROXY_MULTIPART_BUFFER_DIR)

  • s3proxy now accepts the full multipart protocol, buffers each part to a node-local disk directory, and on
    CompleteMultipartUpload concatenates the parts, encrypts the assembled object through the existing AES-256-GCM
    envelope path, and stores it upstream as a single ciphertext PutObject.
  • New internal/multipart disk-buffer session manager with a background sweeper that evicts uploads idle past
    S3PROXY_MULTIPART_TTL (default 24h) and reclaims orphaned directories after a restart.
  • New config knobs: S3PROXY_MULTIPART_BUFFER_DIR (enables the mode), S3PROXY_MULTIPART_MAX_SIZE (assembled-object cap,
    default/hard cap 5 GiB), S3PROXY_MULTIPART_TTL. Mutually exclusive with --allow-multipart.
  • New metrics (s3proxy_multipart_uploads_active, _buffer_bytes, _parts_total, _completed_total, _aborted_total,
    _assemble_duration_seconds), a Grafana dashboard row, and two PrometheusRule alerts (S3ProxyMultipartBufferHigh,
    S3ProxyMultipartUploadsStuck).

ListObjects/ListObjectsV2 report decrypted plaintext size

  • Bucket-level list responses now subtract the fixed 28-byte envelope overhead (12-byte nonce + 16-byte GCM tag) from
    each , clamped at 0. Bucket sub-resource GETs (acl, versioning, multipart listings, ?versions, …) are still
    forwarded unchanged.

Fixes

  • Content-Length on intercepted GetObject — fixes s3cmd 2.4.0 downloading empty files (the proxy previously streamed
    without a length, triggering chunked encoding).
  • Plaintext ETag on intercepted GetObject — overrides the response ETag with md5(plaintext) so ETag-validating/caching
    clients match the delivered bytes. Pass-through objects keep the upstream ETag.

Constraints / known limitations

  • Multipart buffering requires single-instance / session affinity (buffers are node-local); the assembled object must
    fit in RAM (~2× peak); ListParts is unsupported; Complete acks only after the upstream store succeeds.
  • List sizes are 28 bytes short (or 0 after clamp) for objects not written through the proxy (legacy plaintext,
    server-side copies, multipart), since list responses carry no per-object encryption metadata.
  • PUT-response and HEAD ETags still reflect the ciphertext — a known consistency gap.

Testing

  • Unit tests for the multipart manager, router multipart handlers, and list-size logic.
  • e2e coverage for buffer-mode multipart (//go:build e2e, Docker required).

Piotr Szafarczyk added 9 commits June 24, 2026 15:53
Pull the envelope-crypto + upstream-PutObject sequence (DEK encrypt, KEK-version
metadata tagging, client.PutObject, buffer release) out of object.put into a
reusable encryptAndUpload(ctx, log) method. put keeps the detached-context setup,
error-code mapping, response headers, and 200 write. No behaviour change.

Prepares a single shared crypto path for buffered-multipart CompleteMultipartUpload.
Manager that buffers each part to local disk and assembles
them into a single plaintext buffer for the existing encrypt→PutObject
path. Not yet wired into the router or main.

- Create/WritePart/Assemble/Abort over a baseDir + in-memory session table
- per-part MD5 hex ETags; running object-size cap enforced during the
streamed copy (LimitReader, never trusts a client-supplied length)
- Cleanup goroutine: TTL eviction + orphan-dir sweep (reclaims dirs left
by a restart, since the session table is not persisted)
- nil-safe Metrics interface; package does not import monitoring to keep
the dependency direction clean (concrete impl lands in Phase 5)

Single-instance / session-affinity component by design.

Tests cover create, out-of-order parts, re-upload overwrite, cap
enforcement, TTL eviction, orphan sweep, concurrency, and metrics.
Expose three S3PROXY_MULTIPART_* settings for the upcoming disk-buffer
multipart mode; inert until the handlers/main wiring read them.

- MultipartBufferDir  <- S3PROXY_MULTIPART_BUFFER_DIR ("" = disabled)
- MultipartMaxSize    <- S3PROXY_MULTIPART_MAX_SIZE (default/clamp 5 GiB)
- MultipartTTL        <- S3PROXY_MULTIPART_TTL (default 24h)

Includes matching GetX shims and config_test.go for defaults/clamping.
Replace the multipart 501 stubs with buffer-mode handlers when a
*multipart.Manager is wired. Create captures object metadata and opens a
disk-backed session; UploadPart de-frames aws-chunked bodies and streams
each part to disk; Complete assembles the parts, reuses encryptAndUpload
to store one AES-256-GCM-SIV PutObject upstream, then drops the buffer;
Abort releases the session.

getMultipartHandler now follows forward -> buffer -> block precedence.
Serve exempts UploadPart from the 256 MiB PutObject body cap while
keeping the 5 GiB ContentLength ceiling. New aws-chunked de-framer and S3
multipart XML structs back the handlers.

Complete responds 200 only after upstream success and keeps the session
on failure so the client can retry, matching the single-shot ack order.
Wire the disk-buffer multipart Manager into main: when
S3PROXY_MULTIPART_BUFFER_DIR is set, build the Manager, start its
Cleanup goroutine, and pass it to router.New (was nil). Error if
--allow-multipart is also set (mutually exclusive); log the
single-instance / session-affinity constraint at startup.

Add multipart Prometheus collectors (uploads_active, parts_total,
buffer_bytes, completed_total, aborted_total, assemble_duration) and
make *monitoring.Metrics satisfy the multipart.Metrics interface so
the Manager updates them. Increment completed/aborted from the
Complete/Abort handlers via nil-safe router helpers.
…e 6)

Operational completeness for buffered multipart uploads.

- dashboards: add "Multipart (buffer mode)" row (active uploads, buffer
disk bytes, parts/completed/aborted throughput, assemble p95) to both
the monitoring/ source and the deployed chart copy
- alerts: S3ProxyMultipartBufferHigh (disk exhaustion) and
S3ProxyMultipartUploadsStuck (orphaned/affinity-misrouted sessions),
tunable via values.yaml
- README: new env vars, metrics, alerts, and a Multipart uploads section
(forward vs buffer mode, single-instance/affinity, size/memory cap,
durability ordering, ETag semantics, ListParts unsupported)
- CHANGELOG: Unreleased entry
- e2e: Create -> 3x UploadPart -> Complete through the proxy against
MinIO; asserts single ciphertext object + DEK tag, byte-equal GET,
buffer-dir reclamation
- fix: existing proxy_e2e_test.go used the pre-Phase-5 router.New
signature (5 args); add the missing nil manager arg so the e2e build
compiles
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.
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.
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 <Size>, 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 <Size> 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.
@louisgls

Copy link
Copy Markdown
Collaborator

Reviewed this in full while assembling the v1.9.0 release. It was deliberately left out of that release — not because of anything wrong with the design, but because a 2600-line change to the crypto path deserves its own review pass rather than riding along with four small fixes. #49, #50, #51, #53 and #54 went out in #55; main is now at v1.9.1.

The design is sound, and the constraints are stated honestly rather than glossed over. Opt-in via S3PROXY_MULTIPART_BUFFER_DIR, mutually exclusive with --allow-multipart, Complete acking only after the upstream store is confirmed, orphan sweep at startup, and a full metrics + alerting surface. The buffer-then-single-PutObject approach is the right answer given the envelope is two-pass and cannot stream.

Needs doing before merge

1. Rebase — this no longer compiles against main. #53 removed the s3OperationTimeout constant from object.go; handler_multipart.go:176 adds a new use of it. The two land in different files, so git merges them cleanly and the build then fails with undefined: s3OperationTimeout. Switching to config.GetS3OperationTimeout() is the whole fix. The branch also carries rebased copies of #49/#50/#51, which are now in main.

2. Chart changed without a version bump. prometheusrule.yaml gains two alerts, dashboards/s3proxy.json a row, and values.yaml five thresholds — all still under version: 1.9.3, which is already published to ghcr. main is at chart 1.10.1 now, so this needs a bump plus a ### Chart changelog entry.

3. Five new errcheck findings. m.Create unchecked in manager_test.go at lines 99, 127, 145, 163, 174. main has zero errcheck issues, so this is a regression the lint job will catch.

Operational notes

Session affinity cannot be expressed through the chart. The package doc is explicit that every part must reach the process that created the upload, and main.go logs a warning at startup. But the chart ships replicaCount and an autoscaling block with nothing preventing buffer mode from being enabled alongside them. Our own deployment runs 3 replicas — enabling this as-is would break every multipart upload with ErrUploadNotFound. Worth either a chart-level guard (fail the render when the buffer dir is set and replicas > 1) or a prominent README warning next to the values.

The default max size is a footgun. Assemble allocates the whole object (make([]byte, total)), MultipartMaxSize defaults to MaxObjectSize = 5 GiB, and the chart's default memory limit is 256Mi. The README does say peak RAM is ~2× the object, which is the right disclosure — but an operator who sets only S3PROXY_MULTIPART_BUFFER_DIR inherits a 5 GiB ceiling and gets OOMKilled. A default tied to something more conservative, or a startup check against the cgroup memory limit, would fail loudly instead of at 3am.

uploadId is not bound to bucket/key. handleUploadPart(key, bucket string) uses neither parameter — revive flags it — and handleCompleteMultipartUpload writes to the key from the request path, never to s.key/s.bucket recorded at Create. In S3 an uploadId is scoped to a specific bucket+key; here a Complete against a different path silently stores the object elsewhere with the original metadata. The uploadID is a server-generated v4 UUID so this is not readily exploitable, but validating it is cheap and closes the semantic gap.

Smaller points

  • WritePart holds s.mu across the entire io.Copy, so concurrent part uploads for the same upload serialize. That is a defensible tradeoff, but the comment on session claims the opposite ("so concurrent UploadPart writes to different parts of the same upload do not race").
  • Assemble concatenates parts in the order supplied by the client. S3 requires ascending part numbers and rejects anything else; here an out-of-order list silently produces a differently-ordered object.
  • Errors are returned as plain text via http.Error, where S3 SDKs expect an XML <Error><Code> document. Consistent with the rest of the codebase, so not a regression — just noting it as a shared gap.
  • The Complete response returns the upstream ciphertext ETag while fix(router): return plaintext ETag on intercepted GetObject #50 made GetObject return the plaintext ETag. Already called out in fix(router): return plaintext ETag on intercepted GetObject #50's changelog as a known gap, but this widens the surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants