diff --git a/Makefile b/Makefile index 4f85617..edebd16 100644 --- a/Makefile +++ b/Makefile @@ -26,10 +26,13 @@ verify-passthrough: # Integration shards for parallel CI (make test-integration-shard SHARD=memory_usage) INTEGRATION_memory_usage_TESTS = tests/integration/test_memory_usage.py +INTEGRATION_memory_usage_PYTEST_OPTS = -n0 +INTEGRATION_memory_usage_COMPOSE_OPTS = -f tests/docker-compose.oom.yml INTEGRATION_memory_leak_TESTS = tests/integration/test_memory_leak.py INTEGRATION_memory_copy_TESTS = tests/integration/test_copy_memory_governor.py tests/integration/test_copy_per_part_metrics.py tests/integration/test_upload_part_copy_passthrough_e2e.py INTEGRATION_memory_copy_PYTEST_OPTS = -n0 INTEGRATION_core_TESTS = \ + tests/integration/test_generation_roundtrip.py \ tests/integration/test_integration.py \ tests/integration/test_handlers.py \ tests/integration/test_concurrent_operations.py \ @@ -72,15 +75,15 @@ test-integration-shard: ifndef SHARD $(error SHARD is required, e.g. make test-integration-shard SHARD=memory) endif - @docker compose -f tests/docker-compose.yml down 2>/dev/null || true - @docker compose -f tests/docker-compose.yml up -d + @docker compose -f tests/docker-compose.yml $(INTEGRATION_$(SHARD)_COMPOSE_OPTS) down 2>/dev/null || true + @docker compose -f tests/docker-compose.yml $(INTEGRATION_$(SHARD)_COMPOSE_OPTS) up -d @sleep 3 @AWS_ACCESS_KEY_ID=minioadmin AWS_SECRET_ACCESS_KEY=minioadmin \ uv run pytest -m "e2e" -v \ $(if $(INTEGRATION_$(SHARD)_PYTEST_OPTS),$(INTEGRATION_$(SHARD)_PYTEST_OPTS),-n auto --dist loadgroup) \ $(INTEGRATION_$(SHARD)_TESTS); \ EXIT_CODE=$$?; \ - docker compose -f tests/docker-compose.yml down; \ + docker compose -f tests/docker-compose.yml $(INTEGRATION_$(SHARD)_COMPOSE_OPTS) down; \ exit $$EXIT_CODE # Run all tests with containers (unit + integration) diff --git a/README.md b/README.md index 4b9361b..6849f64 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,10 @@ ## Overview +For the generation-format upgrade, migration requirements and measured performance, see +[Generation-bound writes and streaming changes](docs/GENERATION_FORMAT.md). + + S3's server-side encryption is great, but your cloud provider holds the keys. S3Proxy sits between your app and S3, encrypting everything **before** it leaves your infrastructure. ``` diff --git a/docs/CODE_REVIEW.md b/docs/CODE_REVIEW.md new file mode 100644 index 0000000..8dc7f22 --- /dev/null +++ b/docs/CODE_REVIEW.md @@ -0,0 +1,223 @@ +# S3Proxy Python: correctness, performance, and maintainability review + +Reviewed on September 5, 2026. Repository: `s3proxy-python`. Reviewed commit: `27b77cf92b5956c9ae671d581403294205dfed77`. + +The working tree was clean when reviewed. No application code was changed. This English report translates the earlier review and expands the proposed fixes and validation criteria. + +The largest immediate performance opportunities are removing forced garbage collection from request cleanup, reusing S3 clients and connection pools, and reducing backend round trips. Several correctness issues should be addressed first: multipart operations can report false success, overwrite data using conflicting part numbers, reuse cryptographic nonces, and leave objects unreadable after an overwrite. + +## Scope and evidence + +The review focuses on the Python backend: GET, HEAD, PUT, multipart operations, metadata persistence, request signatures, and memory management. It is not a complete security audit of the dashboard, Helm chart, or deployment environment. + +Reproductions used the repository's mock S3 client, local in-memory upload state, and synthetic credentials. No external storage service was contacted. Findings distinguish reproduced behavior from consequences inferred from code. Performance measurements are local microbenchmarks, not production throughput measurements. + +Source references below are relative to the repository root and refer to the reviewed commit. P1 means high priority because of security, data integrity, or false-success behavior; P2 means a correctness or resource-management issue that should follow. + +## Priority findings and proposed solutions + +### 1. P1 — AES-GCM nonce reuse when a multipart part is replaced + +**Source:** `s3proxy/crypto.py:367`, `s3proxy/handlers/multipart/upload_part.py:535`, `s3proxy/state/manager.py:266`. + +The nonce is derived only from the upload ID, internal part number, and, for framed encryption, frame index. Re-uploading a client part reuses its internal part numbers. If the content changes, the same DEK and nonce encrypt different plaintexts. + +**Reproduced:** upload `AAAA`, then `BBBB`, as part 1 of the same upload. Both encryptions use the same nonce, and XOR of the ciphertext payloads equals XOR of the plaintexts. + +Replacing an existing part is valid [S3 UploadPart behavior](https://docs.aws.amazon.com/AmazonS3/latest/API/API_UploadPart.html). Reusing a nonce under the same key violates [AES-GCM's security requirements](https://cryptography.io/en/latest/hazmat/primitives/aead/#cryptography.hazmat.primitives.ciphers.aead.AESGCM). + +**Proposed solution:** + +- Give every new encryption a fresh random nonce with an appropriate collision budget, or derive it from a unique encryption-attempt identity in addition to the existing fields. +- Distinguish a new encryption attempt from a network retry. A network retry can safely resend the exact same previously encrypted bytes. +- The nonce is already embedded in the ciphertext. Check legacy readers and any deterministic nonce validation before changing generation behavior. + +**Acceptance criteria:** replacing a part with different bytes never reuses a nonce/key pair; transport retries resend identical ciphertext; existing stored objects still decrypt. + +### 2. P1 — CompleteMultipartUpload can report success for the wrong upload + +**Source:** `s3proxy/handlers/multipart/lifecycle.py:169` and `:274`. + +`_try_idempotent_complete_response()` checks that the existing object's size matches its existing metadata sidecar. It does not establish that either belongs to the requested `upload_id`. This check happens before reading the requested upload state or validating the client's part list. + +**Reproduced:** after creating a multipart object, submit Complete with `uploadId=never-created` and invalid XML. The handler returns HTTP 200. Consequently, a new upload targeting an existing key can appear complete while the previous object remains in place. + +**Proposed solution:** persist a completion record tied to the exact upload ID and committed object generation, including the resulting client ETag. Accept an idempotent retry only when that record proves the requested upload completed. Do not infer completion from object size. + +**Acceptance criteria:** an unknown upload ID does not succeed because an older object exists; a retry of the actual completed upload returns its recorded result; a new upload to the same key follows the normal completion path. + +### 3. P1 — Replacing a multipart object with a small PUT leaves stale metadata + +**Source:** `s3proxy/handlers/objects/put.py:174`, `s3proxy/handlers/objects/get.py:67`. + +A buffered PUT replaces the object but leaves its old multipart sidecar. GET and HEAD prioritize that sidecar over the new object's encryption metadata. + +**Reproduced:** write `old-content` through streaming PUT, then overwrite it with `new` through a small signed PUT. HEAD still reports 11 bytes instead of 3, and GET fails using the old multipart metadata against the new ciphertext. + +**Proposed solution:** identify the storage format and generation from the current object's metadata. Only load a sidecar belonging to that generation. Clean up obsolete sidecars separately. Simply deleting a shared sidecar after every PUT is insufficient because concurrent writers can delete each other's metadata. + +**Acceptance criteria:** multipart-to-buffered and buffered-to-multipart overwrites return the new bytes and size; concurrent overwrites never combine one generation's ciphertext with another's metadata. Coordinate this work with finding 7. + +### 4. P1 — Small signed PUT requests accept a modified request body + +**Source:** `s3proxy/request_handler.py:278`, `s3proxy/client/verifier.py:376`, `s3proxy/handlers/objects/put.py:133`. + +Signature verification uses the supplied `x-amz-content-sha256` value, but the buffered PUT path never compares it with the actual body's hash. The larger streaming path performs a separate check. + +**Reproduced:** generate a valid SigV4 signature for `original`, preserve the signature and hash header, and replace the body with `changed!`. Header verification succeeds and the PUT handler returns HTTP 200 for the modified data. + +**Proposed solution:** centralize payload validation and invoke it for every signed write path. For buffered PUT, compute the actual SHA-256 and reject a mismatch before writing to S3. Preserve the intentional semantics of `UNSIGNED-PAYLOAD`; validate streaming signature formats through their dedicated verification path. + +**Acceptance criteria:** a modified signed body is rejected without replacing an existing object. Test just below, at, and above the buffering threshold. This finding does not imply arbitrary signature forgery; it shows that a valid signature does not bind the buffered body as intended. + +### 5. P1 — Multipart parts are published before their hash or signature is accepted + +**Source:** `s3proxy/handlers/multipart/upload_part.py:204`, `:471`, and `:603`. + +Both the backend upload and `add_part()` occur before late signature validation. A failed check returns an error without restoring the modified part or state. + +**Reproduced:** UploadPart rejects a wrong SHA-256, but the rejected body's MD5 is already stored in the upload's part state. The backend write can also replace a previously valid part. + +**Proposed solution:** make validation the boundary before publication. For buffered parts, validate before uploading. For streamed parts, use isolated staging or bounded disk spooling so unverified bytes cannot overwrite an accepted part. Publish the client-part mapping only after verification succeeds, and clean up failed attempts. + +Moving `add_part()` alone is insufficient: the backend part may already have been overwritten. Staging must also respect S3's part numbering, ordering, and size constraints. + +**Acceptance criteria:** a rejected replacement leaves both the previous accepted part state and its backend bytes unchanged. Inject hash failure, signature failure, cancellation, and disconnect during replacement. + +### 6. P1 — Switching part-number allocation strategies creates collisions + +**Source:** `s3proxy/state/manager.py:251`, `s3proxy/crypto.py:173`. + +The upload starts with dense numbering: client part 2 maps to internal part 2. When a client part requires multiple internal parts, the upload switches to sparse numbering without relocating or protecting previous allocations. + +**Reproduced:** allocate one internal part for client part 2: internal number 2. Then allocate two internals for client part 1: internal numbers 1–2. Both allocations include internal number 2. Concurrency is not required; out-of-order parts suffice. + +**Proposed solution:** maintain an explicit, atomically updated mapping from client part and attempt to backend allocations. Do not change the meaning of existing allocations when workload shape changes. Design final assembly to preserve client-part order and S3's maximum part count; a monotonic allocator by itself does not solve final ordering. + +**Acceptance criteria:** property-based or randomized tests cover out-of-order uploads, changing part sizes, replacements, and simultaneous allocations. No live allocations overlap, and completed plaintext is ordered correctly. + +### 7. P1 — Ciphertext and required metadata are published separately + +**Source:** `s3proxy/handlers/objects/put.py:310`, `s3proxy/state/metadata.py:265`. + +Streaming PUT completes the object before saving its sidecar. This path does not first persist the new DEK in durable upload state. If the sidecar write fails or the process dies between these operations, ciphertext is already visible and the information needed to decrypt it can be lost. This failure window is identified from code; a process-crash scenario was not executed. + +Separately, `load_multipart_metadata()` interprets all exceptions as missing metadata, including service failures, authorization failures, and corrupt compressed data. + +**Reproduced:** two simulated HTTP 503 backend errors result in `None`. Streaming PUT does not mark the main object with encryption metadata, so a subsequent GET can select unencrypted passthrough when the sidecar cannot be loaded. Other multipart formats can select an incorrect decryption path instead. + +**Proposed solution:** + +- Generate a stable object-generation identity before upload. Store the format, generation, and necessary wrapped-key information durably before publishing ciphertext. +- Bind immutable sidecars to that generation and make readers resolve only the referenced generation. +- Design a recoverable commit sequence, including crash recovery and obsolete-generation cleanup. Reordering two independent writes alone does not make them atomic. +- Return `None` only for confirmed metadata absence. Propagate service, permission, and decoding errors instead of treating them as plaintext-object detection. + +**Acceptance criteria:** inject failure at each commit step, then restart. Every visible encrypted generation must remain decryptable or produce an explicit recoverable error; never silently serve ciphertext as plaintext. A transient metadata error must not trigger format fallback. + +### 8. P1 — aws-chunked decoding accepts truncated and unverified input + +**Source:** `s3proxy/streaming/chunked.py:88`, `s3proxy/handlers/objects/put.py:90`. + +The decoder does not require a terminal zero-size chunk. It does not validate `chunk-signature`; the upload path also disables ordinary payload hash checking for `STREAMING-*`. The decoder skips the two trailing bytes after chunk data without verifying that they are CRLF. + +**Reproduced:** a complete `abc` chunk with an invalid chunk signature, followed by an incomplete chunk, produces `abc` without a decoder error. + +**Proposed solution:** implement an explicit parser state machine with header, payload, CRLF, terminal chunk, and supported trailer states. Require a valid end state at EOF. Validate the signature chain or checksum/trailer requirements for each supported encoding. Explicitly reject signed streaming variants that are not correctly verified. + +**Acceptance criteria:** test truncation at every framing boundary, invalid CRLF, invalid signatures, missing terminal chunks, and supported trailer variants. Failed validation must not publish an object or accepted part. + +### 9. P2 — ETags differ across PUT, HEAD, GET, and LIST + +**Source:** `s3proxy/handlers/objects/misc.py:72`, `s3proxy/handlers/objects/get.py:51`, `s3proxy/state/attr_cache.py:21`. + +For multipart objects, HEAD and LIST use MD5 of the plaintext size. GET uses the backend ETag. Streaming PUT returns MD5 of the plaintext content. HEAD evaluates conditional headers before replacing its effective ETag with the synthetic response ETag. + +**Reproduced:** HEAD and GET return different ETags for the same multipart object. Different contents of equal length necessarily share the synthetic HEAD/LIST ETag. + +**Proposed solution:** persist one client-facing ETag in generation-bound metadata and use it consistently for responses and conditional checks. It must distinguish content or object generations rather than only lengths. Define compatible behavior for existing objects whose metadata lacks the field. + +**Acceptance criteria:** PUT/Complete, HEAD, GET, and LIST agree. Conditional requests using the returned ETag behave consistently, including after a same-size content replacement. + +### 10. P2 — Nested memory reservations can block their own request + +**Source:** `s3proxy/handlers/objects/get.py:154`, `s3proxy/concurrency.py:105`. + +GET reserves 8 MiB at admission and can subsequently request additional memory. If that extra request is clamped to the entire budget, admission requires no memory to be reserved — while the same GET still holds its baseline reservation. + +**Reproduced at limiter level:** a 64 MiB budget, an 8 MiB baseline reservation, and an additional reservation calculated for a 40 MiB object result in SlowDown with the timeout set to zero. Production can wait through the full backpressure timeout. Large single-envelope objects are a legacy/compatibility case; new buffered PUT objects stay below the streaming threshold. Multiple smaller GETs can also hold baseline reservations while waiting for each other to release memory. + +**Proposed solution:** reserve the complete working set atomically once the object format and size are known, or implement a reservation transition that cannot leave multiple waiters holding mutually blocking partial allocations. Do not clamp a whole-buffer requirement while pretending actual memory use fits the budget. Use bounded streaming or spooling where feasible, preserving authenticated-decryption semantics. + +**Acceptance criteria:** one large compatible GET and several simultaneous smaller GETs either progress within a bounded budget or fail promptly and predictably. Cancellation releases reservations exactly once. + +## Performance improvements + +| Priority | Change | Evidence and proposed implementation | +|---|---|---| +| 1 | Remove forced full GC from every reservation release | `s3proxy/concurrency.py:199` runs `gc.collect(0)`, `(1)`, and `(2)` synchronously on the event loop. A 25-iteration local measurement had a median of approximately **30.6 ms**, versus **0.03 ms** with those calls mocked out. Retain normal automatic GC and evaluate whether any exceptional reclamation policy is needed under sustained load. Linux also calls `malloc_trim`, which was not measured on macOS. | +| 2 | Reuse S3 clients and connection pools | `s3proxy/client/s3.py:65` creates a fresh SDK client per context and closes it afterward. A shared Session reuses model loading, but does not preserve these clients' connection pools across their lifetimes. Own a bounded client registry in application lifespan, isolated by credentials and endpoint/configuration. Close clients at shutdown or safe eviction, after active streams finish. | +| 3 | Eliminate redundant metadata requests | A small GET without a sidecar performs HEAD, two sidecar probes, and data GET: **four backend requests**. Multipart performs HEAD, sidecar GET, and a second HEAD before data retrieval. Pass the first HEAD result through the read path; skip sidecar probes only when an unambiguous current-format marker permits it. Preserve safe legacy detection. | +| 4 | Fetch contiguous frames in fewer Range GETs | `s3proxy/handlers/objects/get.py:479` performs one GET per frame, normally 8 MiB. A 1 GiB object with full frames needs approximately 128 data GETs. Fetch a larger contiguous ciphertext range while reading, authenticating, and emitting one frame at a time. Preserve bounded buffering, backpressure, and frame-aligned recovery after network errors. | +| 5 | Reduce duplicate listing metadata work | `s3proxy/handlers/buckets.py:211` already uses bounded parallelism and an attribute cache. A cold listing of 1,000 multipart objects still requires roughly 1,000 HEADs and 1,000 sidecar GETs in addition to LIST. Coalesce concurrent lookups for the same generation and evaluate a generation-keyed metadata cache or index. Increasing concurrency alone increases backend pressure and memory consumption. | + +The GC measurement is a microbenchmark of reservation release, not a claim of a similar improvement in end-to-end throughput. Measure changes individually against a test backend using p50/p95/p99 latency, time to first byte, MiB/s, backend requests per operation, event-loop lag, peak RSS, and error rate. + +The 8 MiB GET reservation also understates the working set during prefetch: the current plaintext frame can remain alive while the next frame is downloaded and decrypted. Model the whole lifecycle rather than reserving only one frame's nominal size. Do not raise general concurrency before validating that model. + +## Reducing complexity + +### A shared upload pipeline + +Use common stages for PutObject and UploadPart: + +`read/decode → hash → encrypt frames → stage → verify → publish` + +The operations can share streaming, hashing, encryption, and cleanup while retaining operation-specific publication rules. This directly addresses inconsistent hash checking, buffering assumptions, and failure cleanup. For small buffered payloads, verification can occur before encryption and staging. + +### A common object descriptor + +Resolve an object into one typed descriptor containing format version, generation identity, plaintext size, client-facing ETag, encryption metadata, and frame/part index. GET, HEAD, LIST, and COPY should consume the same interpretation. This removes duplicated metadata resolution and ETag logic. + +### Explicit multipart state transitions + +Separate receiving, verified, published, and completed states. Model client part numbers separately from backend part allocations, and make attempt identity explicit. State transitions should specify what is durable and what a retry may safely repeat. + +### Centralized resource ownership + +Make request/stream lifetime own the memory reservation, backend response body, client reference, and final metrics. Prefer small explicit services over additional cross-dependent mixins. Cleanup should follow resource lifetime, including disconnects and exceptions after response headers have been sent. + +### Narrow exception handling + +Replace broad `except Exception: return None/pass` where the intended condition is “not found.” Keep authorization errors, unavailable backends, corrupt metadata, and actual absence distinct. + +Retain the current frame format during initial optimization. `FRAME_PLAINTEXT_SIZE` is part of existing read compatibility; changing it without a versioned format can make stored objects unreadable. + +## Additional observations from static inspection + +These observations were not included in the executed fault reproductions: + +| Observation | Proposed follow-up | +|---|---| +| `s3proxy/handlers/objects/put.py:57` checks If-None-Match through HEAD followed by PUT. Two writers can both pass, and non-NotFound HEAD exceptions are swallowed. | Use supported atomic backend preconditions at publication. Define consistent precondition behavior across buffered and multipart writes; test concurrent writers. | +| `s3proxy/handlers/objects/put.py:174` writes internal metadata but does not forward user `x-amz-meta-*` fields, whereas multipart initialization does. | Centralize user-metadata extraction and reserved-key handling, then test parity across upload paths. | +| `s3proxy/handlers/objects/get.py:38` exits the client context before an unencrypted StreamingResponse is consumed. The mock client does not close connections. | Verify a large, slow unencrypted download against real aiohttp/aiobotocore. Keep the backend client alive until stream cleanup. | +| `s3proxy/request_handler.py:161` acquires memory outside the main `try/finally`; rejection can leave the in-flight metric incremented. Streaming requests are recorded as complete before their bodies finish. | Include admission failures in cleanup and finalize streaming duration/status metrics at stream completion, with explicit accounting for post-header failures. | + +## Suggested implementation sequence + +1. Add regression coverage for findings 1–8. Correct nonce generation, upload identity, payload validation, and allocation collisions before expanding concurrency. +2. Design generation-bound metadata and recoverable publication together. This addresses stale sidecars, lost decryption information, incorrect format fallback, and completion identity without independent patches that conflict. +3. Remove forced per-request GC and redundant HEAD calls. Benchmark latency and RSS separately after each change. +4. Introduce lifetime-correct client pooling and contiguous frame reads. Test real backend connections, retries, disconnects, and memory limits. +5. Consolidate the shared pipeline, object descriptor, and memory ownership in small changes protected by compatibility tests. + +## Validation performed + +- Existing unit suite: **642 passed, 2 warnings in 293.07 seconds**. +- Command: `PYTHONDONTWRITEBYTECODE=1 .venv/bin/python -m pytest tests/unit -q -p no:cacheprovider --disable-warnings`. +- Lint: `.venv/bin/python -m ruff check s3proxy --no-cache` passed. +- The contents of the two warnings were not reviewed in that run. +- No integration tests against real S3/Redis and no production load tests were run. +- The separate reproductions expose cases missing from the passing unit suite. They are diagnostic reproductions, not yet committed regression tests. + diff --git a/docs/GENERATION_FORMAT.md b/docs/GENERATION_FORMAT.md new file mode 100644 index 0000000..87aba1f --- /dev/null +++ b/docs/GENERATION_FORMAT.md @@ -0,0 +1,63 @@ +# Generation-bound writes and streaming changes + +This change implements the correctness findings and performance work from [the code review](CODE_REVIEW.md). It changes the format of newly written objects. Read this document before upgrading a running cluster. + +## Publication and retries + +New buffered PUTs carry `s3proxy-format=single-v3`. Streaming PUTs, multipart uploads and streaming copies carry `s3proxy-format=multipart-v3` and an immutable generation pointer. Required manifests are stored at `.s3proxy-internal/generations/.meta` **before** publishing ciphertext. The generation is derived from the initial wrapped random upload key; it remains the same if the first copy selects a source key before any writer starts. Readers resolve the pointer from the current HEAD response. A missing, corrupt or unavailable required manifest is an error, never a signal to return ciphertext as plaintext. + +Each UploadPart attempt writes a private staging object at `.s3proxy-internal/attempts//`. Hash/signature validation and completion of that staging object precede publication in upload state. A rejected replacement cannot modify an accepted attempt. When a state write has an uncertain outcome, its completed staging object is retained because the state write may already have succeeded. + +Complete reads the accepted client-part mapping, validates the client's ordered ETag list, and assembles the selected staging objects in client order with server-side copies. State remains available until completion succeeds. The manifest records the originating bucket, key, upload ID and client ETag, so size alone cannot prove that a retry succeeded. Redis completion locks renew their leases; loss of a lease interrupts the operation. In-memory lock entries disappear after the last waiter finishes. + +The first writer atomically freezes the upload's DEK. A whole-object UploadPartCopy may select its source DEK at that point and snapshot the ciphertext using a source ETag precondition. Subsequent copies using a different key, partial ranges and ordinary uploads encrypt with the already frozen key. No active writer can change that key. New encryption uses fresh random nonces; transport retries reuse the already sealed bytes. Full CopyObject retains native copying and publishes the corresponding manifest first. + +## Compatibility and deployment + +- Existing single-seal and framed objects remain readable. The existing 8 MiB encryption frame boundary is unchanged. Legacy multipart ETags retain their historical fallback where no stored client ETag exists. +- Upgrade all readers before allowing v3 writes. Old releases do not understand the new generation pointers. Do not run old and new writers against the same keys. Use a maintenance window to drain existing uploads and switch the fleet together. +- Drain or restart legacy in-flight multipart uploads. UploadPart and UploadPartCopy reject the old active layout. Do not rely on completing legacy active state after the upgrade. +- Use persistent Redis for uploads that must survive process restarts or move between pods. Configure its TTL above the maximum permitted upload duration. Loss of v3 upload state fails closed: accepted attempts are not reconstructed by guessing from backend parts. Committed objects do not depend on Redis. +- Keep the manifest namespace available to the same backend credentials that operate on objects. The existing internal-prefix filter hides it from proxy listings. Backend permissions now also need multipart creation/copy, listing and deletion for the attempt prefix. +- `If-None-Match: *` is passed to the backend publication operation, including CompleteMultipartUpload for streaming PUT. Verify support on the target S3-compatible service. Failed preconditions return 412. +- Supported signed streaming mode is `STREAMING-AWS4-HMAC-SHA256-PAYLOAD`. Its entire signature chain, terminal chunk and decoded length are checked. Unsupported trailer/signature modes are explicitly rejected. Clients using trailer checksums must select a supported encoding until that protocol is implemented. +- Control request bodies are bounded at 8 MiB. Client parts are bounded at 5 GiB; assembled uploads remain subject to backend object-size and 10,000-part limits. Manifest JSON is bounded at 10 MiB and rejected before publication when it exceeds the reader limit. + +## Storage cleanup + +Successful completion and abort attempt to delete all completed staging objects for that upload generation. Replaced attempts remain immutable until terminal cleanup to avoid racing a concurrent completion snapshot. Configure a backend lifecycle rule for `.s3proxy-internal/attempts/` to expire completed orphan attempts and abort incomplete staging MPUs after a period **longer than the maximum supported upload duration and retry window**. For example, seven days is appropriate only if the deployment prohibits uploads lasting that long. This PR does not install or modify bucket lifecycle policies. + +Crashes, cancelled writes and failed cleanup can leave attempts or unpublished manifests. Generation manifests are intentionally retained: native copies and backend object versions may reference them. Do not apply an age-only deletion policy to `.s3proxy-internal/generations/`. Reclaiming those manifests requires checking all retained object versions, copy references and active uploads. Automatic generation garbage collection is outside this change; retaining metadata is the safe default. + +## Read path, resources and performance + +GET, HEAD, LIST and CopyObject share an object descriptor for plaintext size, ETag and manifest interpretation. GET reuses the initial HEAD. Consecutive frames share backend range requests of up to 64 MiB while buffering and authenticating one frame at a time. Retry resumes at the first unpublished frame. Output is emitted in bounded chunks. Legacy large single seals authenticate to a temporary spool before any plaintext is exposed; configure sufficient local temporary disk for concurrent legacy reads. + +The frame reader allocates exact-size buffers instead of repeatedly growing bytearrays. This was necessary to prevent allocator fragmentation observed during repeated real HTTP reads. GC and allocator trimming no longer run synchronously on each memory release. GET reserves 32 MiB for the working set, and copy operations own their reservation instead of nesting it inside another reservation. + +An application-owned pool retains up to 32 credential-isolated S3 clients, evicts idle entries, and waits for active leases on shutdown. Response ownership keeps bodies, clients and reservations alive until streaming finishes, including failure before the body starts. Metrics finish with the stream; dashboard metric failures cannot prevent reservation release. Concurrent listing lookups for the same object/ETag coalesce and recheck the existing bounded attribute cache. Cold listings still need metadata reads for previously unseen objects; this is not a bucket-wide metadata index. + +Staging adds temporary storage and server-side copy work to ordinary multipart writes. Full compatible copies avoid re-encryption; partial copies and copies after an incompatible key selection use bounded re-encryption. The change prioritizes verified publication over overwriting unverified backend parts. It does not promise that every write workload gets faster. + +## Measured results + +A local macOS/Python 3.14.7 test compared `d45732d` with this implementation against isolated Docker MinIO. Each process wrote a 32 MiB object, performed one warm-up GET, then ten sequential GETs. A fresh process was used for each version. Requests used the same client configuration and verified every returned byte. + +| Metric | Before | After | +| --- | ---: | ---: | +| Median GET latency | 317.12 ms | 289.26 ms | +| Maximum observed latency | 332.85 ms | 331.64 ms | +| Median transfer rate | 100.91 MiB/s | 110.63 MiB/s | +| Highest sampled process RSS | 179.23 MiB | 175.06 MiB | + +This is about 8.8% lower median latency in this small local sample, not a production forecast. [Raw results](read-benchmark.json) are included. A separate 50-request run stayed below 176 MiB sampled RSS. Sampling occurred after requests and can miss transient peaks; the governor budget is not a hard process RSS limit. No p99 claim or AWS/Ceph throughput claim is made. + +Regression tests separately demonstrate eight consecutive frames using one backend GET, frame-aligned recovery without duplicate plaintext, AWS-published streaming-signature vectors, corrupt legacy seals, cancelled publication, failed completion retry, immutable key selection, coalesced listings and resource cleanup on failed response headers. + +## Validation and remaining rollout checks + +Local validation: the CI unit selection passed 779 tests, including 110 mock integration tests (two upstream deprecation warnings). The separate full unit-directory run passed 670 tests, including its slow case. Real-backend validation passed 27 compatibility tests, 11 copy/concurrency tests and nine native-copy tests covering 1,280 MiB objects and concurrent copies. Ruff lint and formatting checks passed. CI repeats the Linux integration shards; the memory-usage shard runs serially, removes all data from each unique test bucket and uses disk-backed MinIO. Its previous 4 GiB tmpfs filled during the multi-gigabyte staging workload; proxy memory limits and memory assertions are unchanged. + +Run `uv run pytest tests/unit -q`, `uv run ruff check .` and `uv run ruff format --check .`. `tests/integration/test_generation_roundtrip.py` exercises real HTTP and MinIO, including hash tampering below/at/above the buffering threshold, generation overwrite, conditional writes, ListParts, native copying and UploadPartCopy. Set `S3PROXY_TEST_REDIS_URL` to a dedicated test Redis to repeat the scenario with durable state. `S3PROXY_TEST_BACKEND` selects the backend for the shared integration fixtures. + +Before production rollout, run the target backend's compatibility suite and workload benchmarks with its real latency, upload sizes, concurrency, lifecycle policy and pod limits. Existing benchmark/compatibility tests for legacy copy internals remain explicitly separate from the public v3 route. No production deployment or bucket policy changes are part of this PR. diff --git a/docs/read-benchmark.json b/docs/read-benchmark.json new file mode 100644 index 0000000..2fedd01 --- /dev/null +++ b/docs/read-benchmark.json @@ -0,0 +1,18 @@ +{ + "baseline": { + "samples": 10, + "bytes": 33554432, + "median_ms": 317.12, + "max_ms": 332.85, + "median_MiB_s": 100.91, + "sampled_peak_RSS_MiB": 179.23 + }, + "changed": { + "samples": 10, + "bytes": 33554432, + "median_ms": 289.26, + "max_ms": 331.64, + "median_MiB_s": 110.63, + "sampled_peak_RSS_MiB": 175.06 + } +} \ No newline at end of file diff --git a/s3proxy/app.py b/s3proxy/app.py index 41701a2..9238032 100644 --- a/s3proxy/app.py +++ b/s3proxy/app.py @@ -200,14 +200,16 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: tracemalloc_task = _maybe_start_tracemalloc() - yield - - if tracemalloc_task is not None: - tracemalloc_task.cancel() - await stats_store.aclose() # flush buffered samples before Redis closes - await close_redis() - await close_http_client() - logger.info("Shutting down") + try: + yield + finally: + if tracemalloc_task is not None: + tracemalloc_task.cancel() + await stats_store.aclose() # flush buffered samples before Redis closes + await close_redis() + await close_http_client() + await handler.client_pool.close() + logger.info("Shutting down") return lifespan diff --git a/s3proxy/client/pool.py b/s3proxy/client/pool.py new file mode 100644 index 0000000..b5b16a5 --- /dev/null +++ b/s3proxy/client/pool.py @@ -0,0 +1,51 @@ +"""Bounded credential-isolated S3 client pool owned by one application lifespan.""" + +import asyncio +from contextlib import asynccontextmanager + +from ..errors import S3Error +from .s3 import S3Client + + +class S3ClientPool: + def __init__(self, settings, max_clients=32): + self.settings = settings + self.max_clients = max_clients + self.entries = {} + self.condition = asyncio.Condition() + self.closed = False + + @asynccontextmanager + async def acquire(self, credentials): + key = (credentials.access_key, credentials.secret_key, credentials.region) + async with self.condition: + if self.closed: + raise S3Error.slow_down("S3 client pool is shutting down") + if key not in self.entries: + if len(self.entries) >= self.max_clients: + idle = next((k for k, (_, refs) in self.entries.items() if refs == 0), None) + if idle is None: + raise S3Error.slow_down("S3 client pool is busy") + client, _ = self.entries.pop(idle) + await client.__aexit__(None, None, None) + client = S3Client(self.settings, credentials) + await client.__aenter__() + self.entries[key] = [client, 0] + entry = self.entries[key] + entry[1] += 1 + try: + yield entry[0] + finally: + async with self.condition: + entry[1] -= 1 + self.condition.notify_all() + + async def close(self): + async with self.condition: + self.closed = True + await self.condition.wait_for( + lambda: all(refs == 0 for _, refs in self.entries.values()) + ) + entries, self.entries = self.entries, {} + for client, _ in entries.values(): + await client.__aexit__(None, None, None) diff --git a/s3proxy/client/s3.py b/s3proxy/client/s3.py index 3b82b7c..165fea1 100644 --- a/s3proxy/client/s3.py +++ b/s3proxy/client/s3.py @@ -41,7 +41,7 @@ class S3Client: Memory management: - Uses a shared aioboto3 Session to avoid repeated JSON model loading - - Creates fresh clients per request for proper connection cleanup + - Clients are leased from the credential-isolated application pool - Each session load costs ~30-150MB (botocore service definitions) See: https://github.com/boto/boto3/issues/1670 @@ -118,6 +118,7 @@ async def put_object( tagging: str | None = None, cache_control: str | None = None, expires: str | None = None, + if_none_match: str | None = None, ) -> dict[str, Any]: """Put object to S3.""" kwargs: dict[str, Any] = {"Bucket": bucket, "Key": key, "Body": body} @@ -128,6 +129,7 @@ async def put_object( Tagging=tagging, CacheControl=cache_control, Expires=expires, + IfNoneMatch=if_none_match, ) return await self._cached_client.put_object(**kwargs) @@ -213,6 +215,7 @@ async def complete_multipart_upload( key: str, upload_id: str, parts: list[dict[str, Any]], + if_none_match: str | None = None, ) -> dict[str, Any]: """Complete multipart upload.""" start = time.monotonic() @@ -221,6 +224,7 @@ async def complete_multipart_upload( Key=key, UploadId=upload_id, MultipartUpload={"Parts": parts}, + **({"IfNoneMatch": if_none_match} if if_none_match is not None else {}), ) duration = time.monotonic() - start logger.info( @@ -284,6 +288,9 @@ async def copy_object( content_type: str | None = None, tagging_directive: str | None = None, tagging: str | None = None, + copy_source_if_match: str | None = None, + cache_control: str | None = None, + expires=None, ) -> dict[str, Any]: """Copy object within S3.""" kwargs: dict[str, Any] = { @@ -300,6 +307,12 @@ async def copy_object( kwargs["TaggingDirective"] = tagging_directive if tagging and tagging_directive == "REPLACE": kwargs["Tagging"] = tagging + _add_optional_kwargs( + kwargs, + CopySourceIfMatch=copy_source_if_match, + CacheControl=cache_control, + Expires=expires, + ) return await self._cached_client.copy_object(**kwargs) async def delete_objects( @@ -399,6 +412,7 @@ async def upload_part_copy( part_number: int, copy_source: str, copy_source_range: str | None = None, + copy_source_if_match: str | None = None, ) -> dict[str, Any]: """Copy a part from another object.""" kwargs: dict[str, Any] = { @@ -408,5 +422,7 @@ async def upload_part_copy( "PartNumber": part_number, "CopySource": copy_source, } - _add_optional_kwargs(kwargs, CopySourceRange=copy_source_range) + _add_optional_kwargs( + kwargs, CopySourceRange=copy_source_range, CopySourceIfMatch=copy_source_if_match + ) return await self._cached_client.upload_part_copy(**kwargs) diff --git a/s3proxy/concurrency.py b/s3proxy/concurrency.py index b1a638e..c203d47 100644 --- a/s3proxy/concurrency.py +++ b/s3proxy/concurrency.py @@ -4,11 +4,7 @@ import asyncio import contextlib -import ctypes -import gc import os -import sys -from collections.abc import Callable import structlog @@ -29,26 +25,6 @@ MAX_BUFFER_SIZE = 8 * 1024 * 1024 # 8MB streaming buffer size -def _create_malloc_release() -> Callable[[], int] | None: - """Create platform-specific function to release memory back to OS. - - Only works on Linux via malloc_trim(0). Returns None on other platforms. - """ - if sys.platform != "linux": - return None - - try: - libc = ctypes.CDLL("libc.so.6") - libc.malloc_trim.argtypes = [ctypes.c_size_t] - libc.malloc_trim.restype = ctypes.c_int - return lambda: libc.malloc_trim(0) - except OSError, AttributeError: - return None - - -_malloc_release = _create_malloc_release() - - BACKPRESSURE_TIMEOUT = int(os.environ.get("S3PROXY_BACKPRESSURE_TIMEOUT", "120")) @@ -195,18 +171,6 @@ async def release(self, bytes_reserved: int) -> None: MEMORY_RESERVED_BYTES.set(self._active_bytes) self._condition.notify_all() - # Run garbage collection and release memory to OS - gc.collect(0) - gc.collect(1) - gc.collect(2) - - if _malloc_release: - with contextlib.suppress(OSError): - _malloc_release() - - # Yield to allow OS memory reclaim - await asyncio.sleep(0) - # Default instance used by module-level functions _default = ConcurrencyLimiter(limit_mb=int(os.environ.get("S3PROXY_MEMORY_LIMIT_MB", "64"))) @@ -225,9 +189,9 @@ def estimate_memory_footprint(method: str, content_length: int) -> int: if method in ("HEAD", "DELETE"): return 0 if method == "GET": - return MAX_BUFFER_SIZE + return 4 * MAX_BUFFER_SIZE if method == "POST": - return MIN_RESERVATION + return max(MIN_RESERVATION, 2 * min(max(content_length, 0), MAX_BUFFER_SIZE)) return max(MIN_RESERVATION, governor_memory_footprint(content_length)) diff --git a/s3proxy/crypto.py b/s3proxy/crypto.py index b58ca10..666f4c7 100644 --- a/s3proxy/crypto.py +++ b/s3proxy/crypto.py @@ -399,8 +399,8 @@ def framed_ciphertext_size(plaintext_size: int) -> int: def encrypt_frame( plaintext: bytes, dek: bytes, upload_id: str, part_number: int, frame_index: int ) -> bytes: - """Encrypt a single frame (nonce || ciphertext || tag) with its derived nonce.""" - return encrypt(plaintext, dek, derive_frame_nonce(upload_id, part_number, frame_index)) + """Encrypt a frame with a fresh nonce; network retries must reuse these sealed bytes.""" + return encrypt(plaintext, dek) def ciphertext_frame_byte_sizes(plaintext_size: int, ciphertext_size: int) -> list[int]: @@ -529,7 +529,7 @@ def decrypt(ciphertext: bytes, dek: bytes) -> bytes: ) nonce = ciphertext[:NONCE_SIZE] - ct_with_tag = ciphertext[NONCE_SIZE:] + ct_with_tag = memoryview(ciphertext)[NONCE_SIZE:] try: aesgcm = AESGCM(dek) diff --git a/s3proxy/errors.py b/s3proxy/errors.py index 11c3e04..7f36616 100644 --- a/s3proxy/errors.py +++ b/s3proxy/errors.py @@ -250,6 +250,14 @@ def raise_for_client_error( msg = e.response.get("Error", {}).get("Message", str(e)) _log_upstream_failure(source="client_error", exc=e, bucket=bucket, key=key) + if code == "PreconditionFailed": + raise S3Error.precondition_failed(msg) from e + if code in ("AccessDenied", "403"): + raise S3Error.access_denied(msg) from e + if code == "InvalidRange": + raise S3Error.invalid_range(msg) from e + if code == "ConditionalRequestConflict": + raise S3Error(409, code, msg) from e if code == "NoSuchUpload": raise S3Error.no_such_upload(msg) from e if code in ("NoSuchKey", "404"): diff --git a/s3proxy/handlers/base.py b/s3proxy/handlers/base.py index 7fcf129..a406b5f 100644 --- a/s3proxy/handlers/base.py +++ b/s3proxy/handlers/base.py @@ -220,9 +220,12 @@ def __init__( self.multipart_manager = multipart_manager self.complete_upload_lock = complete_upload_lock or create_complete_upload_lock() self.keyring = settings.keyring + from ..client.pool import S3ClientPool + + self.client_pool = S3ClientPool(settings) def _client(self, creds: S3Credentials) -> S3Client: - return S3Client(self.settings, creds) + return self.client_pool.acquire(creds) def _parse_path(self, path: str) -> tuple[str, str]: if m := PATH_RE.match(path): @@ -251,6 +254,16 @@ def _internal_meta_keys(self) -> set[str]: self.settings.kidtag_name.lower(), "client-etag", "plaintext-size", + "s3proxy-format", + "s3proxy-generation", + } + + def _user_metadata(self, request: Request) -> dict[str, str]: + internal = self._internal_meta_keys() + return { + k[11:]: v + for k, v in request.headers.items() + if k.startswith("x-amz-meta-") and k[11:] not in internal } def _parse_range(self, header: str, size: int) -> tuple[int, int]: @@ -374,6 +387,29 @@ def _check_conditional_headers( return None + async def _resolve_object(self, client, bucket, key, head=None): + from ..state.metadata import load_multipart_metadata, multipart_etag + from ..state.object import ObjectDescriptor + + if head is None: + head = await client.head_object(bucket, key) + multipart = await load_multipart_metadata(client, bucket, key, head) + metadata = head.get("Metadata", {}) + return ObjectDescriptor( + head=head, + multipart=multipart, + plaintext_size=( + multipart.total_plaintext_size + if multipart + else self._get_plaintext_size(metadata, head.get("ContentLength", 0)) + ), + etag=( + multipart_etag(multipart) + if multipart + else self._get_effective_etag(metadata, head.get("ETag", "")) + ), + ) + async def _download_encrypted_single( self, client: S3Client, bucket: str, key: str, wrapped_dek_b64: str, kid: str = "" ) -> bytes: @@ -381,6 +417,27 @@ async def _download_encrypted_single( wrapped_dek = base64.b64decode(wrapped_dek_b64) return crypto.decrypt_object(ciphertext, wrapped_dek, self.keyring.key_by_id(kid)) + async def _iter_single_plaintext( + self, client, bucket, key, wrapped_dek_b64, kid="", start=0, end=None, if_match=None + ): + from contextlib import aclosing + + from ..streaming.authenticated import decrypt_to_file, file_range + + dek = crypto.unwrap_key(base64.b64decode(wrapped_dek_b64), self.keyring.key_by_id(kid)) + response = await client.get_object( + bucket, key, **({"if_match": if_match} if if_match else {}) + ) + spool, length = await decrypt_to_file(response["Body"], dek) + try: + async with aclosing( + file_range(spool, start, length - 1 if end is None else end) + ) as stream: + async for chunk in stream: + yield chunk + finally: + spool.close() + async def _iter_multipart_plaintext( self, client: S3Client, @@ -390,6 +447,8 @@ async def _iter_multipart_plaintext( dek: bytes, range_start: int | None = None, range_end: int | None = None, + *, + if_match=None, ) -> AsyncIterator[bytes]: """Yield decrypted plaintext for a multipart-encrypted object, one frame at a time. @@ -405,44 +464,17 @@ async def _iter_multipart_plaintext( ~50MB client part. Frames outside the requested plaintext range are skipped (no fetch); frames that partially overlap are trimmed before yielding. """ - sorted_parts = sorted(meta.parts, key=lambda p: p.part_number) - pt_offset = 0 - ct_offset = 0 - - for part in sorted_parts: - if part.internal_parts: - segments = [ - (ip.plaintext_size, ip.ciphertext_size) - for ip in sorted(part.internal_parts, key=lambda p: p.internal_part_number) - ] - else: - segments = [(part.plaintext_size, part.ciphertext_size)] - - for seg_pt_size, seg_ct_size in segments: - for fsize in crypto.ciphertext_frame_byte_sizes(seg_pt_size, seg_ct_size): - frame_pt_size = fsize - crypto.ENCRYPTION_OVERHEAD - frame_pt_end = pt_offset + frame_pt_size - 1 + from contextlib import aclosing - in_range = range_start is None or ( - frame_pt_end >= range_start and pt_offset <= range_end - ) + from ..streaming.frames import plaintext_frames - if in_range: - ct_end = ct_offset + fsize - 1 - ciphertext = await read_source_bytes( - client, bucket, key, f"bytes={ct_offset}-{ct_end}" - ) - plaintext = crypto.decrypt(ciphertext, dek) - - if range_start is not None: - trim_start = max(0, range_start - pt_offset) - trim_end = min(frame_pt_size, range_end - pt_offset + 1) - plaintext = plaintext[trim_start:trim_end] - - yield plaintext - - pt_offset += frame_pt_size - ct_offset += fsize + async with aclosing( + plaintext_frames( + client, bucket, key, meta, dek, range_start, range_end, if_match=if_match + ) + ) as stream: + async for chunk in stream: + yield chunk async def _download_encrypted_multipart( self, diff --git a/s3proxy/handlers/buckets.py b/s3proxy/handlers/buckets.py index c2c8436..8957de7 100644 --- a/s3proxy/handlers/buckets.py +++ b/s3proxy/handlers/buckets.py @@ -19,9 +19,7 @@ INTERNAL_PREFIX, META_SUFFIX_LEGACY, delete_multipart_metadata, - load_multipart_metadata, plaintext_attr_cache, - synthetic_multipart_etag, ) from ..xml_utils import find_element, find_elements from .base import BaseHandler @@ -223,26 +221,29 @@ async def resolve(obj: dict) -> dict: if cached is not None: size, etag = cached return self._list_entry(obj, size, etag) - async with sem: + async with plaintext_attr_cache.coalesce(bucket, obj["Key"], backend_etag), sem: + cached = plaintext_attr_cache.get(bucket, obj["Key"], backend_etag) + if cached is not None: + return self._list_entry(obj, *cached) try: head = await client.head_object(bucket, obj["Key"]) - meta = head.get("Metadata", {}) - if "plaintext-size" in meta: - size = self._get_plaintext_size(meta, obj.get("Size", 0)) - etag = self._get_effective_etag(meta, obj.get("ETag", "")) - elif mp_meta := await load_multipart_metadata(client, bucket, obj["Key"]): - # Multipart objects can't carry plaintext-size in user - # metadata (it is fixed at CreateMultipartUpload); the - # size lives in the .meta sidecar. Reporting the backend - # Size here would leak the ciphertext size and make sync - # clients re-upload every multipart object on each pass. - size = mp_meta.total_plaintext_size - etag = synthetic_multipart_etag(size) - else: - size = self._get_plaintext_size(meta, obj.get("Size", 0)) - etag = self._get_effective_etag(meta, obj.get("ETag", "")) - plaintext_attr_cache.put(bucket, obj["Key"], backend_etag, size, etag) - except Exception: + head.setdefault("ContentLength", obj.get("Size", 0)) + head.setdefault("ETag", obj.get("ETag", "")) + descriptor = await self._resolve_object(client, bucket, obj["Key"], head) + size, etag = descriptor.plaintext_size, descriptor.etag + plaintext_attr_cache.put( + bucket, + obj["Key"], + str(head.get("ETag", backend_etag)).strip('"'), + size, + etag, + ) + except ClientError as error: + from ..state.metadata import is_not_found + + if not is_not_found(error): + raise + # LIST and HEAD are separate snapshots; the object may have been deleted. size, etag = obj.get("Size", 0), backend_etag return self._list_entry(obj, size, etag) diff --git a/s3proxy/handlers/multipart/copy.py b/s3proxy/handlers/multipart/copy.py index f796de6..c6a727f 100644 --- a/s3proxy/handlers/multipart/copy.py +++ b/s3proxy/handlers/multipart/copy.py @@ -89,6 +89,18 @@ class _PlaintextRangeSplit: class CopyPartMixin(BaseHandler): async def handle_upload_part_copy(self, request: Request, creds: S3Credentials) -> Response: + bucket, key = self._parse_path(request.url.path) + upload_id, _ = self._extract_multipart_params(request) + state = await self.multipart_manager.get_upload(bucket, key, upload_id) + if state is None: + raise S3Error.no_such_upload(upload_id) + if state.layout_version < 3: + raise S3Error.invalid_request( + "Legacy in-flight uploads must be restarted after upgrade" + ) + return await self._copy_part_impl(request, creds) + + async def _copy_part_impl(self, request: Request, creds: S3Credentials) -> Response: bucket, key = self._parse_path(request.url.path) async with self._client(creds) as client: upload_id, part_num = self._extract_multipart_params(request) @@ -107,7 +119,7 @@ async def handle_upload_part_copy(self, request: Request, creds: S3Credentials) try: head_resp = await client.head_object(src_bucket, src_key) - except Exception as e: + except ClientError as e: logger.error( "UPLOAD_PART_COPY_HEAD_FAILED", bucket=bucket, @@ -118,11 +130,13 @@ async def handle_upload_part_copy(self, request: Request, creds: S3Credentials) error_type=type(e).__name__, error=str(e), ) - raise S3Error.no_such_key(src_key) from e + self._raise_s3_error(e, src_bucket, src_key) src_metadata = head_resp.get("Metadata", {}) src_wrapped_dek = src_metadata.get(self.settings.dektag_name) - src_multipart_meta = await load_multipart_metadata(client, src_bucket, src_key) + src_multipart_meta = await load_multipart_metadata( + client, src_bucket, src_key, head_resp + ) total_plaintext = self._copy_plaintext_size( head_resp, None, src_wrapped_dek, src_multipart_meta @@ -134,6 +148,78 @@ async def handle_upload_part_copy(self, request: Request, creds: S3Credentials) head_resp, copy_source_range, src_wrapped_dek, src_multipart_meta ) + if state.layout_version >= 3: + from .staged import stage_ciphertext_copy, stage_part + + full_source = ( + copy_source_range is None + or copy_source_range == f"bytes=0-{total_plaintext - 1}" + ) + source_etag = ( + src_multipart_meta.client_etag + if src_multipart_meta + else src_metadata.get("client-etag", "") + ) + native = False + if full_source and source_etag and (src_multipart_meta or src_wrapped_dek): + source_dek, source_kid = self._resolve_source_dek( + src_multipart_meta, src_wrapped_dek, src_metadata, creds + ) + state = await self.multipart_manager.begin_write( + bucket, key, upload_id, source_dek, source_kid + ) + native = state.dek == source_dek + else: + state = await self.multipart_manager.begin_write(bucket, key, upload_id) + + async def stage_copy(): + if native: + async with self._client(creds) as work_client: + part = await stage_ciphertext_copy( + self, + work_client, + state, + part_num, + copy_source, + head_resp, + self._source_ciphertext_segments( + src_multipart_meta, head_resp, src_wrapped_dek, src_metadata + ), + source_etag, + ) + logger.info("UPLOAD_PART_COPY_PASSTHROUGH", bucket=bucket, key=key) + return xml_responses.upload_part_copy_result( + part.md5, format_iso8601(datetime.now(UTC)) + ).encode() + + async with ( + self._client(creds) as work_client, + concurrency.reserve_copy_memory(4 * crypto.FRAME_PLAINTEXT_SIZE), + ): + source = self._iter_copy_source( + work_client, + src_bucket, + src_key, + copy_source_range, + src_wrapped_dek, + src_multipart_meta, + head_resp, + src_metadata, + ) + part = await stage_part( + self, request, work_client, state, part_num, source, verify=False + ) + return xml_responses.upload_part_copy_result( + part.md5, format_iso8601(datetime.now(UTC)) + ).encode() + + return StreamingResponse( + self._keepalive_copy_stream( + stage_copy(), bucket=bucket, key=key, part_num=part_num + ), + media_type="application/xml", + ) + passthrough_block = self._passthrough_block_reason( copy_source_range, raw_copy_source_range, @@ -1473,13 +1559,26 @@ async def _iter_copy_source( yield chunk elif src_wrapped_dek: src_kid = src_metadata.get(self.settings.kidtag_name, "") - plaintext = await self._download_encrypted_single( - client, src_bucket, src_key, src_wrapped_dek, src_kid - ) + from contextlib import aclosing + + start, end = 0, None if copy_source_range: - start, end = self._parse_copy_source_range(copy_source_range, len(plaintext)) - plaintext = plaintext[start : end + 1] - yield plaintext + total = crypto.plaintext_size(head_resp["ContentLength"]) + start, end = self._parse_copy_source_range(copy_source_range, total) + async with aclosing( + self._iter_single_plaintext( + client, + src_bucket, + src_key, + src_wrapped_dek, + src_kid, + start, + end, + if_match=head_resp.get("ETag"), + ) + ) as stream: + async for chunk in stream: + yield chunk else: async for chunk in self._stream_raw_source_with_resume( client, src_bucket, src_key, copy_source_range diff --git a/s3proxy/handlers/multipart/lifecycle.py b/s3proxy/handlers/multipart/lifecycle.py index 7e0901e..002e094 100644 --- a/s3proxy/handlers/multipart/lifecycle.py +++ b/s3proxy/handlers/multipart/lifecycle.py @@ -29,6 +29,7 @@ save_multipart_metadata, synthetic_multipart_etag, ) +from ...state.metadata import GENERATION_KEY, multipart_etag, multipart_headers from ...xml_utils import find_elements, get_element_text from ..base import BaseHandler, is_retryable_source_error @@ -90,12 +91,11 @@ async def handle_create_multipart_upload( # Build metadata (include user's x-amz-meta-*) upload_metadata = { + **self._user_metadata(request), + **multipart_headers(wrapped_dek), self.settings.dektag_name: base64.b64encode(wrapped_dek).decode(), self.settings.kidtag_name: kid, } - for hdr, val in request.headers.items(): - if hdr.lower().startswith("x-amz-meta-"): - upload_metadata[hdr[11:]] = val resp = await client.create_multipart_upload( bucket, @@ -109,12 +109,22 @@ async def handle_create_multipart_upload( upload_id = resp["UploadId"] # Store state in Redis/memory first, then persist to S3 as backup - await self.multipart_manager.create_upload(bucket, key, upload_id, dek, kid) + await self.multipart_manager.create_upload( + bucket, + key, + upload_id, + dek, + kid, + generation=upload_metadata[GENERATION_KEY], + layout_version=3, + ) # Persist DEK to S3 as backup - retry once on failure for attempt in range(2): try: - await persist_upload_state(client, bucket, key, upload_id, wrapped_dek, kid) + await persist_upload_state( + client, bucket, key, upload_id, wrapped_dek, kid, layout_version=3 + ) break except Exception as e: if attempt == 0: @@ -170,12 +180,21 @@ async def _handle_complete_multipart_upload_locked( if idempotent is not None: return idempotent - state = await self.multipart_manager.complete_upload(bucket, key, upload_id) + state = await self.multipart_manager.get_upload(bucket, key, upload_id) if not state: state = await self._recover_upload_state( client, bucket, key, upload_id, context="for complete" ) + if state.layout_version >= 3: + from .staged import cleanup_attempts, complete_staged + + response = await complete_staged(self, request, client, state) + await self.multipart_manager.abort_upload(bucket, key, upload_id) + await delete_upload_state(client, bucket, key, upload_id) + await cleanup_attempts(self, client, state) + return response + if state.deferred_copy_tail: logger.info( "COMPLETE_MULTIPART_DEFERRED_TAIL_PENDING", @@ -243,6 +262,7 @@ async def _handle_complete_multipart_upload_locked( key, MultipartMetadata( version=2, + upload_id=upload_id, part_count=len(completed_parts), total_plaintext_size=total_plaintext, parts=completed_parts, @@ -250,6 +270,7 @@ async def _handle_complete_multipart_upload_locked( kid=kid, ), ) + await self.multipart_manager.abort_upload(bucket, key, upload_id) await delete_upload_state(client, bucket, key, upload_id) logger.info( @@ -276,7 +297,9 @@ async def _try_idempotent_complete_response( ) -> Response | None: """Return success if a peer pod already finished this upload.""" meta = await load_multipart_metadata(client, bucket, key) - if meta is None: + if meta is None or meta.upload_id != upload_id: + return None + if meta.generation and (meta.upload_bucket, meta.upload_key) != (bucket, key): return None try: @@ -297,23 +320,27 @@ async def _try_idempotent_complete_response( ) location = f"{self.settings.s3_endpoint}/{bucket}/{key}" - etag = hashlib.md5( - str(meta.total_plaintext_size).encode(), usedforsecurity=False - ).hexdigest() + etag = multipart_etag(meta) return Response( content=xml_responses.complete_multipart(location, bucket, key, etag), media_type="application/xml", ) def _parse_client_parts(self, body: bytes) -> list[dict]: - client_parts = [] - root = ET.fromstring(body.decode()) - for part in find_elements(root, "Part"): - pn_text = get_element_text(part, "PartNumber") - etag_text = get_element_text(part, "ETag") - if pn_text and etag_text: - client_parts.append({"PartNumber": int(pn_text), "ETag": etag_text}) - return client_parts + try: + root = ET.fromstring(body) + client_parts = [] + for part in find_elements(root, "Part"): + number = int(get_element_text(part, "PartNumber") or "") + etag = get_element_text(part, "ETag") + if not 1 <= number <= 10000 or not etag: + raise ValueError("Invalid part") + client_parts.append({"PartNumber": number, "ETag": etag}) + if not client_parts: + raise ValueError("Empty part list") + return client_parts + except (ET.ParseError, ValueError, TypeError) as error: + raise S3Error.malformed_xml() from error def _build_s3_parts( self, @@ -408,7 +435,7 @@ async def _complete_multipart_upload_with_retry( # already be invalidated. Confirm before treating this as failed. if error_code == "NoSuchUpload" and attempt > 1: verified = await self._verify_already_completed( - client, bucket, key, expected_ciphertext_size + client, bucket, key, expected_ciphertext_size, upload_id ) if verified is not None: logger.warning( @@ -440,7 +467,12 @@ async def _complete_multipart_upload_with_retry( raise last_exc async def _verify_already_completed( - self, client: S3Client, bucket: str, key: str, expected_ciphertext_size: int + self, + client: S3Client, + bucket: str, + key: str, + expected_ciphertext_size: int, + upload_id: str = "", ) -> dict[str, Any] | None: """Check whether a retried CompleteMultipartUpload's NoSuchUpload means the prior attempt actually succeeded (backend assembled the object, then the @@ -449,7 +481,13 @@ async def _verify_already_completed( head = await client.head_object(bucket, key) except ClientError: return None - if head.get("ContentLength") == expected_ciphertext_size: + meta = await load_multipart_metadata(client, bucket, key, head) + if ( + meta is not None + and meta.upload_id == upload_id + and (not meta.generation or (meta.upload_bucket, meta.upload_key) == (bucket, key)) + and head.get("ContentLength") == expected_ciphertext_size + ): return {"ETag": head.get("ETag", "")} return None @@ -499,10 +537,15 @@ async def handle_abort_multipart_upload( upload_id=upload_id[:20] + "...", ) + state = await self.multipart_manager.get_upload(bucket, key, upload_id) await asyncio.gather( self.multipart_manager.abort_upload(bucket, key, upload_id), self._safe_abort(client, bucket, key, upload_id), delete_upload_state(client, bucket, key, upload_id), ) + if state is not None and state.layout_version >= 3: + from .staged import cleanup_attempts + + await cleanup_attempts(self, client, state) return Response(status_code=204) diff --git a/s3proxy/handlers/multipart/list.py b/s3proxy/handlers/multipart/list.py index 8ce4654..202dd16 100644 --- a/s3proxy/handlers/multipart/list.py +++ b/s3proxy/handlers/multipart/list.py @@ -25,6 +25,37 @@ async def handle_list_parts(self, request: Request, creds: S3Credentials) -> Res part_number_marker = int(part_number_marker) if part_number_marker else None max_parts = int(query.get("max-parts", ["1000"])[0]) + if not 0 <= max_parts <= 1000 or (part_number_marker or 0) < 0: + raise S3Error.invalid_argument("Invalid ListParts pagination") + state = await self.multipart_manager.get_upload(bucket, key, upload_id) + if state is not None and state.layout_version >= 3: + accepted = sorted( + (p for p in state.parts.values() if p.part_number > (part_number_marker or 0)), + key=lambda p: p.part_number, + ) + page = accepted[:max_parts] + return Response( + content=xml_responses.list_parts( + bucket=bucket, + key=key, + upload_id=upload_id, + parts=[ + { + "PartNumber": p.part_number, + "ETag": p.md5, + "Size": p.plaintext_size, + "LastModified": state.created_at.isoformat(), + } + for p in page + ], + part_number_marker=part_number_marker, + next_part_number_marker=page[-1].part_number if page else None, + max_parts=max_parts, + is_truncated=len(accepted) > len(page), + storage_class="STANDARD", + ), + media_type="application/xml", + ) try: resp = await client.list_parts( bucket, key, upload_id, part_number_marker, max_parts diff --git a/s3proxy/handlers/multipart/staged.py b/s3proxy/handlers/multipart/staged.py new file mode 100644 index 0000000..64fb8a7 --- /dev/null +++ b/s3proxy/handlers/multipart/staged.py @@ -0,0 +1,292 @@ +"""Generation-bound multipart writes with immutable, verified part attempts. + +Each client part is encrypted into a private temporary object. Only a validated, +completed attempt is published in upload state. Final assembly uses server-side +copies in client order, so replacements cannot overwrite accepted backend bytes +and arbitrary arrival order never changes the plaintext order. +""" + +import contextlib +import hashlib +import math +import uuid +from collections.abc import AsyncIterator +from urllib.parse import quote + +from fastapi import Request, Response + +from ... import crypto, xml_responses +from ...errors import S3Error +from ...signature import verify_payload_hash +from ...state import InternalPartMetadata, MultipartMetadata, PartMetadata +from ...state.metadata import ( + INTERNAL_PREFIX, + save_multipart_metadata, +) + + +async def stage_part( + handler, + request: Request, + client, + state, + part_number: int, + source: AsyncIterator[bytes], + *, + verify: bool = True, +) -> PartMetadata: + if not 1 <= part_number <= 10000: + raise S3Error.invalid_part("PartNumber must be between 1 and 10000") + # Local import avoids mixing the legacy pipeline's implementation into ours. + from .upload_part import _PlaintextReader + + state = await handler.multipart_manager.begin_write(state.bucket, state.key, state.upload_id) + stage_key = f"{INTERNAL_PREFIX}attempts/{state.generation}/{uuid.uuid4().hex}" + created = await client.create_multipart_upload(state.bucket, stage_key) + stage_id = created["UploadId"] + md5 = hashlib.md5(usedforsecurity=False) + sha = hashlib.sha256() + reader = _PlaintextReader(source) + parts = [] + uploaded = [] + total = 0 + publishing = False + try: + while data := await reader.read(crypto.FRAME_PLAINTEXT_SIZE): + md5.update(data) + sha.update(data) + total += len(data) + if total > 5 * 1024**3: + raise S3Error.invalid_argument("Client parts cannot exceed 5 GiB") + ciphertext = crypto.encrypt(data, state.dek) + number = len(parts) + 1 + response = await handler._upload_part_with_retry( + client, + state.bucket, + stage_key, + stage_id, + number, + ciphertext, + client_part_num=part_number, + ) + parts.append( + InternalPartMetadata( + number, len(data), len(ciphertext), response["ETag"].strip('"') + ) + ) + uploaded.append({"PartNumber": number, "ETag": response["ETag"]}) + del data, ciphertext + if verify: + verify_payload_hash(request, sha.hexdigest()) + expected = request.headers.get("content-length") + chunked = "aws-chunked" in request.headers.get( + "content-encoding", "" + ) or request.headers.get("x-amz-content-sha256", "").startswith("STREAMING-") + if expected is not None and not chunked and int(expected) != total: + raise S3Error.bad_request("Content-Length does not match uploaded body") + if not parts: + ciphertext = crypto.encrypt(b"", state.dek) + response = await client.upload_part(state.bucket, stage_key, stage_id, 1, ciphertext) + parts.append(InternalPartMetadata(1, 0, len(ciphertext), response["ETag"].strip('"'))) + uploaded.append({"PartNumber": 1, "ETag": response["ETag"]}) + await client.complete_multipart_upload(state.bucket, stage_key, stage_id, uploaded) + part = PartMetadata( + part_number, + total, + sum(p.ciphertext_size for p in parts), + md5.hexdigest(), + md5.hexdigest(), + internal_parts=parts, + staging_key=stage_key, + ) + # The old attempt remains immutable. Replaced attempts are cleaned by a + # bucket lifecycle rule; deleting here could race a Complete snapshot. + publishing = True + await handler.multipart_manager.add_part(state.bucket, state.key, state.upload_id, part) + return part + except BaseException: + with contextlib.suppress(Exception): + await handler._safe_abort(client, state.bucket, stage_key, stage_id) + # A cancelled Redis write may already have published the reference. + if not publishing: + with contextlib.suppress(Exception): + await client.delete_object(state.bucket, stage_key) + raise + finally: + close = getattr(source, "aclose", None) + if close is not None: + await close() + + +def select_parts(handler, body: bytes, state) -> list[PartMetadata]: + requested = handler._parse_client_parts(body) + numbers = [p["PartNumber"] for p in requested] + if not numbers or numbers != sorted(set(numbers)): + raise S3Error.invalid_part("Parts must be unique and ordered") + parts = [] + for item in requested: + part = state.parts.get(item["PartNumber"]) + if part is None or part.md5 != item["ETag"].strip('"') or not part.staging_key: + raise S3Error.invalid_part("Part or ETag does not match an accepted upload") + parts.append(part) + if any(p.plaintext_size < crypto.MIN_PART_SIZE for p in parts[:-1]): + raise S3Error.entity_too_small("All client parts except the last must be at least 5 MiB") + return parts + + +async def complete_staged(handler, request, client, state) -> Response: + parts = select_parts(handler, await request.body(), state) + # One backend copy for almost all legal client parts. Split ciphertext just + # above S3's 5 GiB CopyPart limit into balanced ranges, never a tiny tail. + max_copy = 5 * 1024**3 + counts = [math.ceil(p.ciphertext_size / max_copy) for p in parts] + if sum(counts) > 10000: + raise S3Error.invalid_request("Encrypted upload exceeds S3's 10000 backend part limit") + copies = [] + for part, count in zip(parts, counts, strict=True): + size = math.ceil(part.ciphertext_size / count) + for start in range(0, part.ciphertext_size, size): + number = len(copies) + 1 + end = min(start + size, part.ciphertext_size) - 1 + response = await copy_with_retry( + client, + state.bucket, + state.key, + state.upload_id, + number, + f"{state.bucket}/{quote(part.staging_key, safe='/')}", + f"bytes={start}-{end}", + ) + copies.append({"PartNumber": number, "ETag": response["CopyPartResult"]["ETag"]}) + # ETag is consistent across Complete, HEAD, GET and LIST; no size-only hash. + etag = ( + hashlib.md5( + b"".join(bytes.fromhex(p.md5) for p in parts), usedforsecurity=False + ).hexdigest() + + f"-{len(parts)}" + ) + wrapped = crypto.wrap_key(state.dek, handler.keyring.key_by_id(state.kid)) + meta = MultipartMetadata( + version=3, + generation=state.generation, + upload_id=state.upload_id, + upload_bucket=state.bucket, + upload_key=state.key, + client_etag=etag, + parts=parts, + part_count=len(parts), + total_plaintext_size=sum(p.plaintext_size for p in parts), + wrapped_dek=wrapped, + kid=state.kid, + ) + # The object's immutable generation pointer becomes visible only at Complete. + # Persist its full decryption map first; failed Complete is safely retryable. + await save_multipart_metadata(client, state.bucket, state.key, meta) + await handler._complete_multipart_upload_with_retry( + client, state.bucket, state.key, state.upload_id, copies, parts + ) + location = f"{handler.settings.s3_endpoint}/{state.bucket}/{state.key}" + return Response( + content=xml_responses.complete_multipart(location, state.bucket, state.key, etag), + media_type="application/xml", + ) + + +async def copy_with_retry( + client, bucket, key, upload_id, number, source, byte_range, *, if_match=None +): + import asyncio + + from ..base import SOURCE_READ_ATTEMPTS, SOURCE_READ_BACKOFF_SEC, is_retryable_source_error + + for attempt in range(SOURCE_READ_ATTEMPTS): + try: + return await client.upload_part_copy( + bucket, + key, + upload_id, + number, + source, + byte_range, + **({"copy_source_if_match": if_match} if if_match else {}), + ) + except Exception as error: + if attempt + 1 == SOURCE_READ_ATTEMPTS or not is_retryable_source_error(error): + raise + await asyncio.sleep(SOURCE_READ_BACKOFF_SEC * 2**attempt) + + +async def cleanup_attempts(handler, client, state): + """Best-effort terminal cleanup; lifecycle expiry covers crashes and late writers.""" + try: + token = None + while True: + page = await client.list_objects_v2( + state.bucket, + prefix=f"{INTERNAL_PREFIX}attempts/{state.generation}/", + continuation_token=token, + ) + keys = [o["Key"] for o in page.get("Contents", [])] + if keys: + await client.delete_objects(state.bucket, [{"Key": key} for key in keys]) + if not page.get("IsTruncated"): + break + token = page["NextContinuationToken"] + except Exception as error: + import structlog + + structlog.get_logger(__name__).warning("STAGING_CLEANUP_FAILED", error=str(error)) + + +async def stage_ciphertext_copy(handler, client, state, part_number, source, head, segments, etag): + """Snapshot a whole encrypted source without changing its ciphertext/nonces.""" + if not 1 <= part_number <= 10000 or sum(p.plaintext_size for p in segments) > 5 * 1024**3: + raise S3Error.invalid_argument("Invalid part number or source exceeds 5 GiB") + stage_key = f"{INTERNAL_PREFIX}attempts/{state.generation}/{uuid.uuid4().hex}" + upload = await client.create_multipart_upload(state.bucket, stage_key) + stage_id = upload["UploadId"] + publishing = False + try: + total = head["ContentLength"] + count = max(1, math.ceil(total / (5 * 1024**3))) + step = math.ceil(total / count) + copied = [] + for start in range(0, total, step): + number = len(copied) + 1 + result = await copy_with_retry( + client, + state.bucket, + stage_key, + stage_id, + number, + source, + f"bytes={start}-{min(start + step, total) - 1}", + if_match=head["ETag"], + ) + copied.append({"PartNumber": number, "ETag": result["CopyPartResult"]["ETag"]}) + await client.complete_multipart_upload(state.bucket, stage_key, stage_id, copied) + # S3 ETags are opaque; keep a 128-bit token for multipart ETag composition. + if len(etag) != 32 or any(c not in "0123456789abcdef" for c in etag): + etag = hashlib.md5(etag.encode(), usedforsecurity=False).hexdigest() + part = PartMetadata( + part_number, + sum(p.plaintext_size for p in segments), + total, + etag, + etag, + internal_parts=[ + InternalPartMetadata(i, p.plaintext_size, p.ciphertext_size, "") + for i, p in enumerate(segments, 1) + ], + staging_key=stage_key, + ) + publishing = True + await handler.multipart_manager.add_part(state.bucket, state.key, state.upload_id, part) + return part + except BaseException: + with contextlib.suppress(Exception): + await handler._safe_abort(client, state.bucket, stage_key, stage_id) + if not publishing: + with contextlib.suppress(Exception): + await client.delete_object(state.bucket, stage_key) + raise diff --git a/s3proxy/handlers/multipart/upload_part.py b/s3proxy/handlers/multipart/upload_part.py index ae06e2e..3b94f41 100644 --- a/s3proxy/handlers/multipart/upload_part.py +++ b/s3proxy/handlers/multipart/upload_part.py @@ -17,7 +17,6 @@ from ... import crypto from ...client import S3Client, S3Credentials from ...errors import S3Error, raise_for_client_error, raise_for_exception -from ...signature import deferred_signature_required, verify_deferred_payload_hash from ...state import ( InternalPartMetadata, MultipartUploadState, @@ -98,144 +97,21 @@ async def handle_upload_part(self, request: Request, creds: S3Credentials) -> Re # Get upload state state = await self._get_or_recover_state(client, bucket, key, upload_id, part_num) - # Parse request info - content_encoding = request.headers.get("content-encoding", "") - content_sha = request.headers.get("x-amz-content-sha256", "") - try: - content_length = int(request.headers.get("content-length", "0")) - except ValueError: - content_length = 0 - - upload_start_time = time.monotonic() - logger.info( - "UPLOAD_PART_START", - bucket=bucket, - key=key, - upload_id=upload_id[:20] + "...", - part_number=part_num, - content_length_mb=f"{content_length / 1024 / 1024:.2f}MB", - ) - - # Determine encoding type and upload path. - cls = classify_upload(content_sha, content_encoding, content_length) - is_unsigned = cls.is_unsigned - is_streaming_sig = cls.is_streaming_sig - needs_chunked_decode = cls.needs_chunked_decode - is_large_signed = cls.is_large_signed - use_framed = cls.use_framed - - # Smallest internal part that bounds memory while staying within the - # per-client part-number allocation range (so we never collide and - # never buffer more than necessary). - internal_part_size = crypto.memory_bounded_part_size(content_length) - estimated_parts = max(1, -(-content_length // internal_part_size)) - logger.info( - "UPLOAD_PART_CONFIG", - bucket=bucket, - key=key, - part_number=part_num, - internal_part_size_mb=f"{internal_part_size / 1024 / 1024:.2f}MB", - estimated_internal_parts=estimated_parts, - is_unsigned=is_unsigned, - is_large_signed=is_large_signed, - is_streaming_sig=is_streaming_sig, - needs_chunked_decode=needs_chunked_decode, - upload_path="framed" if use_framed else "buffered", - ) - - # Per-client allocation: dense 1:1 for all-5MB uploads (ClickHouse 600+ - # parts), sparse ranges once a client part needs multiple internals (Scylla). - internal_part_start = await self.multipart_manager.allocate_internal_parts( - bucket, - key, - upload_id, - estimated_parts, - client_part_number=part_num, - ) - internal_part_end = internal_part_start + estimated_parts - 1 - logger.info( - "UPLOAD_PART_INTERNAL_RANGE", - bucket=bucket, - key=key, - part_number=part_num, - internal_part_start=internal_part_start, - internal_part_end=internal_part_end, - estimated_internal_parts=estimated_parts, - ) + if state.layout_version >= 3: + from ..objects.put import _iter_request_body + from .staged import stage_part - try: - # Known-length direct streams (unsigned or large signed, e.g. barman - # backups) can be uploaded frame-by-frame with O(frame) memory. - # aws-chunked / streaming-sig bodies don't know the size up front and - # keep the buffered path. - if use_framed: - result = await self._stream_and_upload_framed( - request, - client, - bucket, - key, - upload_id, - part_num, - state, - content_length, - internal_part_size, - internal_part_start, - estimated_parts, - ) - else: - result = await self._stream_and_upload( - request, - client, - bucket, - key, - upload_id, - part_num, - state, - content_sha, - content_length, - is_unsigned, - is_streaming_sig, - is_large_signed, - needs_chunked_decode, - internal_part_size, - internal_part_start, - ) - - # Late signature verification for large signed uploads - if deferred_signature_required(request): - verify_deferred_payload_hash( - request, request.app.state.verifier, result["computed_sha256"] - ) - elif is_large_signed and content_sha and result["computed_sha256"] != content_sha: - logger.warning( - "UPLOAD_PART_SHA256_MISMATCH", - bucket=bucket, - key=key, - part_num=part_num, - expected=content_sha, - computed=result["computed_sha256"], - ) - raise S3Error.signature_does_not_match("Signature verification failed") - - upload_duration = time.monotonic() - upload_start_time - logger.info( - "UPLOAD_PART_COMPLETE", - bucket=bucket, - key=key, - part_number=part_num, - plaintext_mb=f"{result['total_plaintext_size'] / 1024 / 1024:.2f}MB", - internal_parts=result["internal_parts_count"], - duration_sec=f"{upload_duration:.2f}", + decode = "aws-chunked" in request.headers.get( + "content-encoding", "" + ) or request.headers.get("x-amz-content-sha256", "").startswith("STREAMING-") + part = await stage_part( + self, request, client, state, part_num, _iter_request_body(request, decode) ) + return Response(headers={"ETag": f'"{part.md5}"'}) - return Response(headers={"ETag": f'"{result["client_etag"]}"'}) - - except S3Error: - raise - except ClientError as e: - return self._handle_client_error(e, bucket, key, part_num, upload_id) - except Exception as e: - return self._handle_generic_error(e, bucket, key, part_num, upload_id) + raise S3Error.invalid_request( + "Legacy in-flight uploads must be restarted after upgrade" + ) async def _upload_part_with_retry( self, @@ -689,7 +565,7 @@ async def _upload_internal_part_with_semaphore( try: # Encrypt - nonce = crypto.derive_part_nonce(upload_id, internal_part_num) + nonce = crypto.generate_nonce() ciphertext = crypto.encrypt(data, state.dek, nonce) plaintext_size = len(data) ciphertext_size = len(ciphertext) diff --git a/s3proxy/handlers/objects/get.py b/s3proxy/handlers/objects/get.py index 901f095..9bb8453 100644 --- a/s3proxy/handlers/objects/get.py +++ b/s3proxy/handlers/objects/get.py @@ -4,25 +4,22 @@ import base64 import contextlib from collections.abc import AsyncIterator, Awaitable, Callable -from itertools import accumulate from typing import Any import structlog from botocore.exceptions import ClientError from fastapi import Request, Response -from fastapi.responses import StreamingResponse from structlog.stdlib import BoundLogger -from ... import concurrency, crypto +from ... import crypto from ...client import S3Client, S3Credentials -from ...concurrency import MAX_BUFFER_SIZE from ...errors import S3Error from ...state import ( MultipartMetadata, calculate_part_range, - load_multipart_metadata, ) from ...streaming import STREAM_CHUNK_SIZE +from ...streaming.response import OwnedStreamingResponse from ...utils import format_http_date from ..base import BaseHandler @@ -49,7 +46,9 @@ async def handle_get_object(self, request: Request, creds: S3Credentials) -> Res # Get the effective ETag (client-etag for encrypted, S3 etag otherwise) metadata = head_resp.get("Metadata", {}) - effective_etag = self._get_effective_etag(metadata, head_resp.get("ETag", "")) + descriptor = await self._resolve_object(client, bucket, key, head_resp) + mp_meta = descriptor.multipart + effective_etag = descriptor.etag # Check conditional headers (inherited from BaseHandler) cond_response = self._check_conditional_headers( @@ -64,9 +63,9 @@ async def handle_get_object(self, request: Request, creds: S3Credentials) -> Res if cond_response: return cond_response - if meta := await load_multipart_metadata(client, bucket, key): + if (meta := mp_meta) is not None: response = await self._get_multipart( - client, bucket, key, meta, range_header, last_modified, creds + client, bucket, key, meta, range_header, last_modified, creds, head_resp ) else: response = await self._get_single( @@ -120,20 +119,35 @@ async def _stream_unencrypted( last_modified: str | None, ) -> Response: logger.info("GET_UNENCRYPTED", bucket=bucket, key=key) - resp = await client.get_object(bucket, key, range_header=range_header) + lease = self._client(client.credentials) + stream_client = await lease.__aenter__() + try: + resp = await stream_client.get_object(bucket, key, range_header=range_header) + except BaseException: + await lease.__aexit__(None, None, None) + raise s3_body = resp["Body"] - headers = self._build_response_headers(resp, last_modified) - async def stream_s3_body() -> AsyncIterator[bytes]: + async def stream_s3_body(): async with s3_body: while chunk := await s3_body.read(STREAM_CHUNK_SIZE): yield chunk + async def cleanup(): + try: + await s3_body.__aexit__(None, None, None) + finally: + await lease.__aexit__(None, None, None) + if "ContentRange" in resp: headers["Content-Range"] = resp["ContentRange"] - return StreamingResponse(stream_s3_body(), status_code=206, headers=headers) - return StreamingResponse(stream_s3_body(), headers=headers) + return OwnedStreamingResponse( + stream_s3_body(), + headers=headers, + status_code=206 if "ContentRange" in resp else 200, + cleanup=cleanup, + ) async def _decrypt_single_object( self, @@ -147,52 +161,41 @@ async def _decrypt_single_object( kid: str = "", ) -> Response: logger.info("GET_ENCRYPTED_SINGLE", bucket=bucket, key=key) - resp = await client.get_object(bucket, key) - content_length = resp.get("ContentLength", 0) - - # Encrypted decrypts buffer ciphertext + plaintext simultaneously. - # Acquire additional memory beyond the initial MAX_BUFFER_SIZE reservation. - additional = max(0, content_length * 2 - MAX_BUFFER_SIZE) - extra_reserved = 0 - try: - if additional > 0: - extra_reserved = await concurrency.try_acquire_memory(additional) + resp = await client.get_object(bucket, key, if_match=head_resp.get("ETag")) - wrapped_dek = base64.b64decode(wrapped_dek_b64) - async with resp["Body"] as body: - ciphertext = await body.read() - plaintext = crypto.decrypt_object(ciphertext, wrapped_dek, self.keyring.key_by_id(kid)) - del ciphertext - - content_type = head_resp.get("ContentType", "application/octet-stream") - cache_control = head_resp.get("CacheControl") - expires = head_resp.get("Expires") + wrapped_dek = base64.b64decode(wrapped_dek_b64) + dek = crypto.unwrap_key(wrapped_dek, self.keyring.key_by_id(kid)) + from ...streaming.authenticated import decrypt_to_file, file_range + # Authenticate to bounded disk storage before emitting any plaintext. This + # also handles old single-envelope objects larger than the memory budget. + spool, length = await decrypt_to_file(resp["Body"], dek) + try: + start, end = ( + self._parse_range(range_header, length) if range_header else (0, length - 1) + ) + headers = self._build_headers( + head_resp.get("ContentType", "application/octet-stream"), + end - start + 1, + last_modified, + head_resp.get("CacheControl"), + head_resp.get("Expires"), + ) if range_header: - start, end = self._parse_range(range_header, len(plaintext)) - headers = self._build_headers( - content_type=content_type, - content_length=end - start + 1, - last_modified=last_modified, - cache_control=cache_control, - expires=expires, - ) - headers["Content-Range"] = f"bytes {start}-{end}/{len(plaintext)}" - return Response( - content=plaintext[start : end + 1], status_code=206, headers=headers - ) + headers["Content-Range"] = f"bytes {start}-{end}/{length}" - headers = self._build_headers( - content_type=content_type, - content_length=len(plaintext), - last_modified=last_modified, - cache_control=cache_control, - expires=expires, + async def cleanup(): + spool.close() + + return OwnedStreamingResponse( + file_range(spool, start, end), + headers=headers, + cleanup=cleanup, + status_code=206 if range_header else 200, ) - return Response(content=plaintext, headers=headers) - finally: - if extra_reserved > 0: - await concurrency.release_memory(extra_reserved) + except BaseException: + spool.close() + raise async def _get_multipart( self, @@ -203,26 +206,44 @@ async def _get_multipart( range_header: str | None, last_modified: str | None, creds: S3Credentials, + head_resp: dict | None = None, ) -> Response: dek = crypto.unwrap_key(meta.wrapped_dek, self.keyring.key_by_id(meta.kid)) total = meta.total_plaintext_size start, end = self._parse_range(range_header, total) if range_header else (0, total - 1) parts = calculate_part_range(meta.parts, start, end) - # Build lookup: part_number -> (part_metadata, ciphertext_offset) - sorted_parts = sorted(meta.parts, key=lambda p: p.part_number) - offsets = [0, *accumulate(p.ciphertext_size for p in sorted_parts)] - part_info = {p.part_number: (p, offsets[i]) for i, p in enumerate(sorted_parts)} - # Get actual object size and content type - actual_size, content_type, cache_control, expires_val = await self._get_object_info( - client, bucket, key, meta - ) + head_resp = head_resp or await client.head_object(bucket, key) + content_type = head_resp.get("ContentType", "application/octet-stream") + cache_control = head_resp.get("CacheControl") + expires_val = head_resp.get("Expires") # Create stream generator - stream = self._create_multipart_stream( - creds, bucket, key, parts, part_info, dek, actual_size, start, end - ) + async def stream(): + from ...streaming.frames import plaintext_frames + + async with ( + self._client(creds) as stream_client, + contextlib.aclosing( + plaintext_frames( + stream_client, + bucket, + key, + meta, + dek, + start if range_header else None, + end if range_header else None, + if_match=head_resp.get("ETag"), + ciphertext_size=head_resp.get("ContentLength"), + ) + ) as plaintext, + ): + try: + async for chunk in plaintext: + yield chunk + except ClientError as error: + self._raise_s3_error(error, bucket, key) # Build response length = sum(e - s + 1 for _, s, e in parts) @@ -235,8 +256,12 @@ async def _get_multipart( ) if range_header: headers["Content-Range"] = f"bytes {start}-{end}/{total}" - return StreamingResponse(stream, status_code=206, headers=headers) - return StreamingResponse(stream, headers=headers) + return OwnedStreamingResponse(stream(), status_code=206, headers=headers) + if total == 0: + async for _ in stream(): + pass + return Response(headers=headers) + return OwnedStreamingResponse(stream(), headers=headers) async def _get_object_info( self, client: S3Client, bucket: str, key: str, meta: MultipartMetadata diff --git a/s3proxy/handlers/objects/misc.py b/s3proxy/handlers/objects/misc.py index 75b9d18..fcce474 100644 --- a/s3proxy/handlers/objects/misc.py +++ b/s3proxy/handlers/objects/misc.py @@ -4,8 +4,8 @@ import base64 import hashlib import xml.etree.ElementTree as ET +from dataclasses import replace from datetime import UTC, datetime -from urllib.parse import quote import structlog from botocore.exceptions import ClientError @@ -20,10 +20,15 @@ MultipartMetadata, PartMetadata, delete_multipart_metadata, - load_multipart_metadata, save_multipart_metadata, ) -from ...state.metadata import _internal_meta_key +from ...state.metadata import ( + FORMAT_KEY, + GENERATION_KEY, + generation_for, + multipart_etag, + multipart_headers, +) from ...utils import format_http_date, format_iso8601 from ...xml_utils import find_element, find_elements from ..base import BaseHandler @@ -50,8 +55,8 @@ async def handle_head_object(self, request: Request, creds: S3Credentials) -> Re last_modified_dt = resp.get("LastModified") # Get the effective ETag (client-etag for encrypted, S3 etag otherwise) - metadata = resp.get("Metadata", {}) - effective_etag = self._get_effective_etag(metadata, resp.get("ETag", "")) + descriptor = await self._resolve_object(client, bucket, key, resp) + effective_etag = descriptor.etag # Check conditional headers (inherited from BaseHandler) cond_response = self._check_conditional_headers( @@ -68,27 +73,10 @@ async def handle_head_object(self, request: Request, creds: S3Credentials) -> Re extra_headers = self._build_head_extra_headers(resp, last_modified) - if meta := await load_multipart_metadata(client, bucket, key): - headers = { - "Content-Length": str(meta.total_plaintext_size), - "Content-Type": resp.get("ContentType", "application/octet-stream"), - "ETag": f'"{ - hashlib.md5( - str(meta.total_plaintext_size).encode(), - usedforsecurity=False, - ).hexdigest() - }"', - **extra_headers, - } - return Response(headers=headers) - - size = self._get_plaintext_size(metadata, resp.get("ContentLength", 0)) - etag = self._get_effective_etag(metadata, resp.get("ETag", "")) - headers = { - "Content-Length": str(size), + "Content-Length": str(descriptor.plaintext_size), "Content-Type": resp.get("ContentType", "application/octet-stream"), - "ETag": f'"{etag}"', + "ETag": f'"{effective_etag}"', **extra_headers, } return Response(headers=headers) @@ -162,7 +150,10 @@ async def handle_copy_object(self, request: Request, creds: S3Credentials) -> Re if metadata_directive == "REPLACE": new_metadata = {} for hdr, val in request.headers.items(): - if hdr.lower().startswith("x-amz-meta-"): + if ( + hdr.lower().startswith("x-amz-meta-") + and hdr[11:] not in self._internal_meta_keys() + ): new_metadata[hdr[11:]] = val # Strip x-amz-meta- prefix logger.info( @@ -184,11 +175,12 @@ async def handle_copy_object(self, request: Request, creds: S3Credentials) -> Re src_key=src_key, error=str(e), ) - raise S3Error.no_such_key(src_key) from e + self._raise_s3_error(e, src_bucket, src_key) src_metadata = head_resp.get("Metadata", {}) src_wrapped_dek = src_metadata.get(self.settings.dektag_name) - src_multipart_meta = await load_multipart_metadata(client, src_bucket, src_key) + source = await self._resolve_object(client, src_bucket, src_key, head_resp) + src_multipart_meta = source.multipart if not src_wrapped_dek and not src_multipart_meta: # Not encrypted - pass through @@ -203,6 +195,7 @@ async def handle_copy_object(self, request: Request, creds: S3Credentials) -> Re metadata_directive, new_metadata, request, + head_resp, ) # Encrypted source. A plain COPY needs no re-encrypt: the ciphertext @@ -267,6 +260,7 @@ async def _copy_passthrough( metadata_directive: str, new_metadata: dict[str, str] | None, request: Request, + head_resp: dict, ) -> Response: logger.info( "COPY_PASSTHROUGH", @@ -285,9 +279,19 @@ async def _copy_passthrough( bucket, key, copy_source, - metadata=new_metadata, - metadata_directive=metadata_directive, - content_type=content_type, + metadata={ + **( + new_metadata or {} + if metadata_directive == "REPLACE" + else head_resp.get("Metadata", {}) + ), + FORMAT_KEY: "plain-v3", + }, + metadata_directive="REPLACE", + content_type=content_type or head_resp.get("ContentType"), + copy_source_if_match=head_resp.get("ETag"), + cache_control=head_resp.get("CacheControl") if metadata_directive == "COPY" else None, + expires=head_resp.get("Expires") if metadata_directive == "COPY" else None, tagging_directive=tagging_directive if tagging_directive != "COPY" else None, tagging=tagging, ) @@ -333,30 +337,39 @@ async def _copy_passthrough_encrypted( is_multipart=bool(src_multipart_meta), ) + metadata = dict(head_resp.get("Metadata", {})) + if src_multipart_meta: + if not src_multipart_meta.generation: + src_multipart_meta = replace( + src_multipart_meta, generation=generation_for(src_multipart_meta.wrapped_dek) + ) + metadata.update( + {FORMAT_KEY: "multipart-v3", GENERATION_KEY: src_multipart_meta.generation} + ) + await save_multipart_metadata(client, bucket, key, src_multipart_meta) + else: + metadata[FORMAT_KEY] = "single-v3" resp = await client.copy_object( bucket, key, copy_source, - metadata_directive="COPY", - content_type=content_type, + metadata=metadata, + metadata_directive="REPLACE", + content_type=content_type or head_resp.get("ContentType"), + cache_control=head_resp.get("CacheControl"), + expires=head_resp.get("Expires"), + copy_source_if_match=head_resp.get("ETag"), ) - # Multipart objects keep their part/frame map in a separate sidecar - # object; the destination needs its own copy or the read path can't - # reconstruct (and decrypt) it. - if src_multipart_meta: - await client.copy_object( - bucket, - _internal_meta_key(key), - f"{src_bucket}/{quote(_internal_meta_key(src_key), safe='/')}", - metadata_directive="COPY", - ) - # Encrypted objects report the plaintext md5 (client-etag), not the # ciphertext ETag, to match GET/HEAD and the re-encrypt path. src_metadata = head_resp.get("Metadata", {}) result = resp.get("CopyObjectResult", {}) - etag = src_metadata.get("client-etag") or str(result.get("ETag", "")).strip('"') + etag = ( + multipart_etag(src_multipart_meta) + if src_multipart_meta + else src_metadata.get("client-etag") or str(result.get("ETag", "")).strip('"') + ) last_modified = result.get("LastModified") if hasattr(last_modified, "isoformat"): last_modified = last_modified.isoformat().replace("+00:00", "Z") @@ -469,6 +482,7 @@ async def _copy_encrypted_inner( etag = hashlib.md5(plaintext, usedforsecurity=False).hexdigest() dest_metadata = { + FORMAT_KEY: "single-v3", self.settings.dektag_name: base64.b64encode(encrypted.wrapped_dek).decode(), self.settings.kidtag_name: dest_kid, "client-etag": etag, @@ -550,6 +564,7 @@ async def _copy_encrypted_streaming( wrapped_dek = crypto.wrap_key(dek, dest_kek) upload_metadata: dict[str, str] = { + **multipart_headers(wrapped_dek), self.settings.dektag_name: base64.b64encode(wrapped_dek).decode(), self.settings.kidtag_name: dest_kid, } @@ -593,27 +608,28 @@ async def _copy_encrypted_streaming( head_resp, ) + etag = hashlib.sha256(wrapped_dek).hexdigest() + await save_multipart_metadata( + client, + bucket, + key, + MultipartMetadata( + version=3, + generation=generation_for(wrapped_dek), + upload_id=upload_id, + client_etag=etag, + part_count=len(meta_parts), + total_plaintext_size=total_plaintext, + parts=meta_parts, + wrapped_dek=wrapped_dek, + kid=dest_kid, + ), + ) await client.complete_multipart_upload(bucket, key, upload_id, s3_parts) - except Exception: + except BaseException: await self._safe_abort(client, bucket, key, upload_id) raise - await save_multipart_metadata( - client, - bucket, - key, - MultipartMetadata( - version=2, - part_count=len(meta_parts), - total_plaintext_size=total_plaintext, - parts=meta_parts, - wrapped_dek=wrapped_dek, - kid=dest_kid, - ), - ) - - etag = hashlib.md5(str(total_plaintext).encode(), usedforsecurity=False).hexdigest() - logger.info( "COPY_ENCRYPTED_STREAMING_COMPLETE", src_bucket=src_bucket, @@ -716,7 +732,7 @@ async def _encrypt_and_upload_chunk( meta_parts: list[PartMetadata], total_plaintext: int, ) -> tuple[int, list[dict], list[PartMetadata], int]: - nonce = crypto.derive_part_nonce(upload_id, part_number) + nonce = crypto.generate_nonce() ciphertext = crypto.encrypt(chunk, dek, nonce) resp = await client.upload_part(bucket, key, upload_id, part_number, ciphertext) etag = resp["ETag"].strip('"') @@ -765,10 +781,20 @@ async def _iter_object_plaintext( yield chunk else: src_kid = head_resp.get("Metadata", {}).get(self.settings.kidtag_name, "") - plaintext = await self._download_encrypted_single( - client, src_bucket, src_key, src_wrapped_dek, src_kid - ) - yield plaintext + from contextlib import aclosing + + async with aclosing( + self._iter_single_plaintext( + client, + src_bucket, + src_key, + src_wrapped_dek, + src_kid, + if_match=head_resp.get("ETag"), + ) + ) as stream: + async for chunk in stream: + yield chunk async def handle_get_object_tagging(self, request: Request, creds: S3Credentials) -> Response: bucket, key = self._parse_path(request.url.path) diff --git a/s3proxy/handlers/objects/put.py b/s3proxy/handlers/objects/put.py index ed22513..0a4d98b 100644 --- a/s3proxy/handlers/objects/put.py +++ b/s3proxy/handlers/objects/put.py @@ -1,11 +1,13 @@ """PUT object operations with encryption support.""" +import asyncio import base64 import hashlib from collections.abc import AsyncIterator from typing import Any import structlog +from botocore.exceptions import ClientError from fastapi import Request, Response from starlette.requests import ClientDisconnect from structlog.stdlib import BoundLogger @@ -14,14 +16,14 @@ from ...client import S3Client, S3Credentials from ...disconnect import ClientDisconnectError from ...errors import S3Error -from ...signature import verify_deferred_payload_hash +from ...signature import verify_deferred_payload_hash, verify_payload_hash from ...state import ( MultipartMetadata, PartMetadata, save_multipart_metadata, ) +from ...state.metadata import FORMAT_KEY, generation_for, multipart_headers from ...streaming import decode_aws_chunked, decode_aws_chunked_stream -from ...utils import etag_matches from ..base import BaseHandler logger: BoundLogger = structlog.get_logger(__name__) @@ -52,29 +54,9 @@ class PutObjectMixin(BaseHandler): async def handle_put_object(self, request: Request, creds: S3Credentials) -> Response: bucket, key = self._parse_path(request.url.path) async with self._client(creds) as client: - # Check If-None-Match header (prevents overwriting existing objects) if_none_match = request.headers.get("if-none-match") - if if_none_match: - try: - head_resp = await client.head_object(bucket, key) - # Object exists - check if etag matches - if if_none_match.strip() == "*": - # * means fail if object exists at all - raise S3Error.precondition_failed( - "At least one of the pre-conditions you specified did not hold" - ) - # Check specific etag match - metadata = head_resp.get("Metadata", {}) - existing_etag = self._get_effective_etag(metadata, head_resp.get("ETag", "")) - if etag_matches(existing_etag, if_none_match): - raise S3Error.precondition_failed( - "At least one of the pre-conditions you specified did not hold" - ) - except S3Error: - raise - except Exception: - # Object doesn't exist - proceed with upload - pass + if if_none_match and if_none_match != "*": + raise S3Error.invalid_argument("If-None-Match on PUT must be '*'") content_type = request.headers.get("content-type", "application/octet-stream") content_sha = request.headers.get("x-amz-content-sha256", "") content_encoding = request.headers.get("content-encoding", "") @@ -91,7 +73,12 @@ async def handle_put_object(self, request: Request, creds: S3Credentials) -> Res needs_chunked_decode = "aws-chunked" in content_encoding or is_streaming_sig # Stream large uploads to avoid buffering - if is_unsigned or is_streaming_sig or content_length > crypto.MAX_BUFFER_SIZE: + if ( + is_unsigned + or is_streaming_sig + or content_length > crypto.MAX_BUFFER_SIZE + or "content-length" not in request.headers + ): logger.debug( "PUT_STREAMING", bucket=bucket, @@ -159,6 +146,7 @@ async def _put_buffered( if needs_chunked_decode: body = decode_aws_chunked(body) + verify_payload_hash(request, hashlib.sha256(body).hexdigest()) kid, kek = self.keyring.key_for(client.credentials.access_key) encrypted = crypto.encrypt_object(body, kek) logger.debug( @@ -176,6 +164,8 @@ async def _put_buffered( key, encrypted.ciphertext, metadata={ + **self._user_metadata(request), + FORMAT_KEY: "single-v3", self.settings.dektag_name: base64.b64encode(encrypted.wrapped_dek).decode(), self.settings.kidtag_name: kid, "client-etag": etag, @@ -185,6 +175,7 @@ async def _put_buffered( cache_control=cache_control, expires=expires, tagging=tagging, + **({"if_none_match": "*"} if request.headers.get("if-none-match") else {}), ) return Response(headers={"ETag": f'"{etag}"'}) @@ -209,6 +200,12 @@ async def _put_streaming( bucket, key, content_type=content_type, + metadata={ + **self._user_metadata(request), + **multipart_headers(wrapped_dek), + self.settings.dektag_name: base64.b64encode(wrapped_dek).decode(), + self.settings.kidtag_name: kid, + }, cache_control=cache_control, expires=expires, tagging=tagging, @@ -227,7 +224,7 @@ async def _put_streaming( async def upload_part(data: bytes) -> None: nonlocal part_num part_num += 1 - nonce = crypto.derive_part_nonce(upload_id, part_num) + nonce = crypto.generate_nonce() data_len = len(data) data_md5 = hashlib.md5(data, usedforsecurity=False).hexdigest() ciphertext = crypto.encrypt(data, dek, nonce) @@ -283,13 +280,9 @@ async def upload_part(data: bytes) -> None: # Verify SHA256 if provided, or deferred SigV4 after streaming hash if deferred_sig: - try: - verify_deferred_payload_hash( - request, request.app.state.verifier, sha256_hash.hexdigest() - ) - except S3Error: - await client.abort_multipart_upload(bucket, key, upload_id) - raise + verify_deferred_payload_hash( + request, request.app.state.verifier, sha256_hash.hexdigest() + ) elif expected_sha256 is not None: computed_sha256 = sha256_hash.hexdigest() if computed_sha256 != expected_sha256: @@ -301,19 +294,22 @@ async def upload_part(data: bytes) -> None: expected=expected_sha256, computed=computed_sha256, ) - await client.abort_multipart_upload(bucket, key, upload_id) raise S3Error.signature_does_not_match( f"SHA256 mismatch: {computed_sha256} != {expected_sha256}" ) # Complete upload - await client.complete_multipart_upload(bucket, key, upload_id, parts_complete) + if not parts_complete: + await upload_part(b"") await save_multipart_metadata( client, bucket, key, MultipartMetadata( - version=2, + version=3, + generation=generation_for(wrapped_dek), + upload_id=upload_id, + client_etag=md5_hash.hexdigest(), part_count=len(parts_meta), total_plaintext_size=total_plaintext_size, parts=parts_meta, @@ -322,6 +318,13 @@ async def upload_part(data: bytes) -> None: ), ) + await client.complete_multipart_upload( + bucket, + key, + upload_id, + parts_complete, + **({"if_none_match": "*"} if request.headers.get("if-none-match") else {}), + ) etag = md5_hash.hexdigest() logger.info( "PUT_STREAMING_COMPLETE", @@ -336,7 +339,8 @@ async def upload_part(data: bytes) -> None: except ClientDisconnectError, ClientDisconnect: await self._safe_abort(client, bucket, key, upload_id) raise ClientDisconnectError.raised() from None - except S3Error: + except S3Error, ClientError, asyncio.CancelledError: + await self._safe_abort(client, bucket, key, upload_id) raise except Exception as e: logger.error( diff --git a/s3proxy/request_handler.py b/s3proxy/request_handler.py index 4b34490..00ee2cc 100644 --- a/s3proxy/request_handler.py +++ b/s3proxy/request_handler.py @@ -2,6 +2,7 @@ from __future__ import annotations +import hashlib import os import time from urllib.parse import parse_qs @@ -26,11 +27,20 @@ ) from .request_context import bind_request, clear_request, get_request_context from .routing import RequestDispatcher +from .signature import verify_payload_hash +from .streaming.response import OwnedStreamingResponse pod_name = os.environ.get("HOSTNAME", "unknown") logger: BoundLogger = structlog.get_logger(__name__).bind(pod=pod_name) +async def _record_request_safely(*args): + try: + await record_request(*args) + except Exception as error: + logger.warning("REQUEST_METRICS_FAILED", error=str(error)) + + def _is_dashboard_path(request: Request, path: str) -> bool: """True if the path targets the dashboard (so it's excluded from stats). @@ -138,38 +148,40 @@ async def handle_proxy_request( # Check memory limit BEFORE reading body data - reject if at capacity reserved_memory = 0 - needs_limit = method in ("PUT", "POST", "GET") + needs_limit = method in ("PUT", "POST", "GET") and not request.headers.get("x-amz-copy-source") memory_limit = concurrency.get_memory_limit() - if memory_limit > 0 and needs_limit: - try: - content_length = int(request.headers.get("content-length", "0")) - except ValueError: - content_length = 0 - bind_request(method=method, path=path, query=query, content_length=content_length) - memory_needed = concurrency.estimate_memory_footprint(method, content_length) - - logger.info( - "REQUEST_ARRIVED - attempting to acquire memory", - memory_needed_mb=round(memory_needed / 1024 / 1024, 2), - active_mb=round(concurrency.get_active_memory() / 1024 / 1024, 2), - limit_mb=round(memory_limit / 1024 / 1024, 2), - method=method, - path=path, - content_length=content_length, - ) - reserved_memory = await concurrency.try_acquire_memory(memory_needed) - logger.info( - "MEMORY_RESERVED", - reserved_mb=round(reserved_memory / 1024 / 1024, 2), - active_mb=round(concurrency.get_active_memory() / 1024 / 1024, 2), - limit_mb=round(memory_limit / 1024 / 1024, 2), - method=method, - path=path, - ) - response = None try: + if memory_limit > 0 and needs_limit: + try: + content_length = int(request.headers.get("content-length", "0")) + except ValueError: + content_length = 0 + bind_request(method=method, path=path, query=query, content_length=content_length) + memory_needed = concurrency.estimate_memory_footprint(method, content_length) + if method in ("PUT", "POST") and "content-length" not in request.headers: + memory_needed = 4 * crypto.MAX_BUFFER_SIZE + + logger.info( + "REQUEST_ARRIVED - attempting to acquire memory", + memory_needed_mb=round(memory_needed / 1024 / 1024, 2), + active_mb=round(concurrency.get_active_memory() / 1024 / 1024, 2), + limit_mb=round(memory_limit / 1024 / 1024, 2), + method=method, + path=path, + content_length=content_length, + ) + reserved_memory = await concurrency.try_acquire_memory(memory_needed) + logger.info( + "MEMORY_RESERVED", + reserved_mb=round(reserved_memory / 1024 / 1024, 2), + active_mb=round(concurrency.get_active_memory() / 1024 / 1024, 2), + limit_mb=round(memory_limit / 1024 / 1024, 2), + method=method, + path=path, + ) + response = await _handle_proxy_request_impl(request, handler, verifier) if response is not None: status_code = response.status_code @@ -180,9 +192,63 @@ async def handle_proxy_request( # GETs accumulates frames and OOMs the pod while the limiter reads ~budget. # Hold the reservation for the whole stream lifetime so the limiter bounds # how many streaming GETs run at once (admission control). - if reserved_memory > 0 and isinstance(response, StreamingResponse): - response.body_iterator = _release_after_stream(response.body_iterator, reserved_memory) + if isinstance(response, StreamingResponse): + original = response + reserved = reserved_memory reserved_memory = 0 + stream_status = [status_code] + cleaned = False + + async def stream(): + try: + async for chunk in original.body_iterator: + yield chunk + except BaseException: + stream_status[0] = 500 + raise + finally: + try: + if hasattr(original.body_iterator, "aclose"): + await original.body_iterator.aclose() + finally: + await cleanup() + + async def cleanup(): + nonlocal cleaned + if cleaned: + return + cleaned = True + try: + if isinstance(original, OwnedStreamingResponse) and original.cleanup: + await original.cleanup() + finally: + await concurrency.release_memory(reserved) + if not _is_dashboard_path(request, path): + await _record_request_safely( + method, + path, + operation, + stream_status[0], + time.perf_counter() - start_time, + int(original.headers.get("content-length", "0")), + request.client.host if request.client else "", + ) + REQUESTS_IN_FLIGHT.labels(method=method).dec() + REQUEST_COUNT.labels( + method=method, operation=operation, status=stream_status[0] + ).inc() + REQUEST_DURATION.labels(method=method, operation=operation).observe( + time.perf_counter() - start_time + ) + + response = OwnedStreamingResponse( + stream(), + status_code=original.status_code, + headers=dict(original.headers), + background=original.background, + cleanup=cleanup, + on_error=lambda: stream_status.__setitem__(0, 500), + ) return response except HTTPException as e: status_code = e.status_code @@ -219,9 +285,10 @@ async def handle_proxy_request( clear_request() # Record metrics duration = time.perf_counter() - start_time - REQUESTS_IN_FLIGHT.labels(method=method).dec() - REQUEST_COUNT.labels(method=method, operation=operation, status=status_code).inc() - REQUEST_DURATION.labels(method=method, operation=operation).observe(duration) + if not isinstance(response, StreamingResponse): + REQUESTS_IN_FLIGHT.labels(method=method).dec() + REQUEST_COUNT.labels(method=method, operation=operation, status=status_code).inc() + REQUEST_DURATION.labels(method=method, operation=operation).observe(duration) try: if method == "GET" and response is not None: @@ -235,8 +302,10 @@ async def handle_proxy_request( # "/dashboard" (no trailing slash) doesn't match the mounted dashboard router and # falls through to this S3 catch-all, where it would otherwise be logged # as a phantom "dashboard" bucket. - if not _is_dashboard_path(request, path): - await record_request(method, path, operation, status_code, duration, size, client_ip) + if not isinstance(response, StreamingResponse) and not _is_dashboard_path(request, path): + await _record_request_safely( + method, path, operation, status_code, duration, size, client_ip + ) if reserved_memory > 0: await concurrency.release_memory(reserved_memory) @@ -260,14 +329,28 @@ async def _handle_proxy_request_impl( query = parse_qs(str(request.url.query), keep_blank_values=True) content_length = _parse_content_length(headers) - defer_sig = request.method in ("PUT", "POST") and _defer_signature_for_body( - headers, content_length, query + data_write = ( + request.method == "PUT" + and not headers.get("x-amz-copy-source") + and not any(k in query for k in ("tagging", "acl", "lifecycle", "policy", "cors")) + and "/" in request.url.path.strip("/") + ) + defer_sig = data_write and _defer_signature_for_body( + headers, + content_length if "content-length" in headers else crypto.MAX_BUFFER_SIZE + 1, + query, ) needs_body = request.method in ("PUT", "POST") and _needs_body_for_signature(headers, query) body = b"" if needs_body and not defer_sig: - body = await request.body() + chunks = bytearray() + async for chunk in request.stream(): + chunks.extend(chunk) + if len(chunks) > crypto.MAX_BUFFER_SIZE: + raise S3Error.invalid_request("Control request body exceeds 8 MiB") + body = bytes(chunks) + request._body = body if body: request.state.s3proxy_preloaded_body = body logger.debug( @@ -312,6 +395,17 @@ async def _handle_proxy_request_impl( raise S3Error.signature_does_not_match(error) raise S3Error.access_denied(error or "No credentials") + if request.method in ("PUT", "POST", "DELETE") and not data_write: + if headers.get("x-amz-content-sha256", "").startswith("STREAMING-"): + raise S3Error.invalid_request("Streaming encoding is only supported for data uploads") + chunks = bytearray() + async for chunk in request.stream(): + chunks.extend(chunk) + if len(chunks) > crypto.MAX_BUFFER_SIZE: + raise S3Error.invalid_request("Control request body exceeds 8 MiB") + request._body = bytes(chunks) + verify_payload_hash(request, hashlib.sha256(chunks).hexdigest()) + dispatcher = RequestDispatcher(handler) try: return await dispatcher.dispatch(request, verified_creds) diff --git a/s3proxy/signature.py b/s3proxy/signature.py index ca22d0f..0843b70 100644 --- a/s3proxy/signature.py +++ b/s3proxy/signature.py @@ -38,3 +38,20 @@ def verify_deferred_payload_hash( if error and "signature" in error.lower(): raise S3Error.signature_does_not_match(error) raise S3Error.access_denied(error or "Access Denied") + + +def verify_payload_hash(request: Request, payload_hash: str) -> None: + """Verify the body before publishing any new object or part state.""" + import hmac + + if deferred_signature_required(request): + verify_deferred_payload_hash(request, request.app.state.verifier, payload_hash) + return + expected = request.headers.get("x-amz-content-sha256", "") + if ( + expected + and expected != "UNSIGNED-PAYLOAD" + and not expected.startswith("STREAMING-") + and not hmac.compare_digest(expected, payload_hash) + ): + raise S3Error.signature_does_not_match("Payload SHA256 mismatch") diff --git a/s3proxy/state/attr_cache.py b/s3proxy/state/attr_cache.py index 99c1eb9..4dfdb8d 100644 --- a/s3proxy/state/attr_cache.py +++ b/s3proxy/state/attr_cache.py @@ -10,8 +10,10 @@ backend ETag, which invalidates the cached attributes without coordination. """ +import asyncio import hashlib from collections import OrderedDict +from contextlib import asynccontextmanager # ~100k entries of (bucket, key, etag) -> (size, etag) stays in the tens of # MB even with long backup keys — well inside the pod memory limit. @@ -30,8 +32,22 @@ def synthetic_multipart_etag(plaintext_size: int) -> str: class PlaintextAttrCache: def __init__(self, maxsize: int = _DEFAULT_MAXSIZE) -> None: self._maxsize = maxsize + self._locks = {} self._entries: OrderedDict[tuple[str, str, str], tuple[int, str]] = OrderedDict() + @asynccontextmanager + async def coalesce(self, bucket, key, backend_etag): + cache_key = (bucket, key, backend_etag) + entry = self._locks.setdefault(cache_key, [asyncio.Lock(), 0]) + entry[1] += 1 + try: + async with entry[0]: + yield + finally: + entry[1] -= 1 + if entry[1] == 0: + del self._locks[cache_key] + def get(self, bucket: str, key: str, backend_etag: str) -> tuple[int, str] | None: if not backend_etag: return None diff --git a/s3proxy/state/complete_lock.py b/s3proxy/state/complete_lock.py index dec0538..954b030 100644 --- a/s3proxy/state/complete_lock.py +++ b/s3proxy/state/complete_lock.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio +import contextlib import os import uuid from collections.abc import AsyncIterator @@ -51,7 +52,7 @@ def __init__( self._ttl = ttl_seconds self._acquire_timeout = acquire_timeout_seconds self._poll_interval = poll_interval_seconds - self._memory_locks: dict[str, asyncio.Lock] = {} + self._memory_locks: dict[str, list] = {} self._memory_guard = asyncio.Lock() def _storage_key(self, bucket: str, key: str, upload_id: str) -> str: @@ -73,30 +74,16 @@ async def hold(self, bucket: str, key: str, upload_id: str) -> AsyncIterator[Non async def _memory_hold(self, bucket: str, key: str, upload_id: str) -> AsyncIterator[None]: lk = self._storage_key(bucket, key, upload_id) async with self._memory_guard: - lock = self._memory_locks.get(lk) - if lock is None: - lock = asyncio.Lock() - self._memory_locks[lk] = lock - - await lock.acquire() - logger.debug( - "COMPLETE_LOCK_ACQUIRED", - bucket=bucket, - key=key, - upload_id=upload_id[:20] + "..." if len(upload_id) > 20 else upload_id, - backend="memory", - ) + entry = self._memory_locks.setdefault(lk, [asyncio.Lock(), 0]) + entry[1] += 1 try: - yield + async with entry[0]: + yield finally: - lock.release() - logger.debug( - "COMPLETE_LOCK_RELEASED", - bucket=bucket, - key=key, - upload_id=upload_id[:20] + "..." if len(upload_id) > 20 else upload_id, - backend="memory", - ) + async with self._memory_guard: + entry[1] -= 1 + if entry[1] == 0: + del self._memory_locks[lk] @asynccontextmanager async def _redis_hold(self, bucket: str, key: str, upload_id: str) -> AsyncIterator[None]: @@ -130,17 +117,45 @@ async def _redis_hold(self, bucket: str, key: str, upload_id: str) -> AsyncItera await asyncio.sleep(self._poll_interval) + owner = asyncio.current_task() + lost = False + + async def renew(): + nonlocal lost + try: + while True: + await asyncio.sleep(max(0.1, self._ttl / 3)) + if not await self._renew_redis_lock(redis_key, token): + raise RuntimeError("Completion lease was lost") + except Exception: + lost = True + owner.cancel() + + renewal = asyncio.create_task(renew()) try: yield + except asyncio.CancelledError: + if lost: + raise S3Error.slow_down("Completion lease lost; retry the upload") from None + raise finally: + renewal.cancel() + with contextlib.suppress(asyncio.CancelledError): + await renewal await self._release_redis_lock(redis_key, token) - logger.debug( - "COMPLETE_LOCK_RELEASED", - bucket=bucket, - key=key, - upload_id=upload_id[:20] + "..." if len(upload_id) > 20 else upload_id, - backend="redis", - ) + + async def _renew_redis_lock(self, redis_key, token): + async with self._redis.pipeline(transaction=True) as pipe: + await pipe.watch(redis_key) + current = await pipe.get(redis_key) + current = current.decode() if isinstance(current, bytes) else current + if current != token: + await pipe.unwatch() + return False + pipe.multi() + pipe.expire(redis_key, self._ttl) + await pipe.execute() + return True async def _release_redis_lock(self, redis_key: str, token: str) -> None: import redis.asyncio as redis diff --git a/s3proxy/state/manager.py b/s3proxy/state/manager.py index 995ebf5..73fe95e 100644 --- a/s3proxy/state/manager.py +++ b/s3proxy/state/manager.py @@ -59,6 +59,9 @@ async def create_upload( upload_id: str, dek: bytes, kid: str = "", + *, + generation: str = "", + layout_version: int = 2, ) -> MultipartUploadState: """Create new upload state.""" state = MultipartUploadState( @@ -67,6 +70,8 @@ async def create_upload( key=key, upload_id=upload_id, kid=kid, + generation=generation, + layout_version=layout_version, ) sk = self._storage_key(bucket, key, upload_id) @@ -130,6 +135,31 @@ async def get_upload( ) return state + async def begin_write(self, bucket, key, upload_id, candidate_dek=None, candidate_kid=None): + """Freeze the upload DEK atomically before any concurrent attempt uses it. + + The first whole-object copy may select its source DEK. Once any writer + starts (even a failed one), no later request can change the key. + """ + + def updater(data): + state = deserialize_upload_state(data) + if state is None: + raise StateMissingError("Corrupt multipart state") + if not state.write_started: + if candidate_dek is not None and not state.parts: + state.dek = candidate_dek + state.kid = candidate_kid + state.write_started = True + return serialize_upload_state(state) + + data = await self._store.update( + self._storage_key(bucket, key, upload_id), updater, self._ttl + ) + if data is None: + raise StateMissingError("Upload state missing") + return deserialize_upload_state(data) + async def add_part( self, bucket: str, diff --git a/s3proxy/state/metadata.py b/s3proxy/state/metadata.py index d3452d3..1fde6b1 100644 --- a/s3proxy/state/metadata.py +++ b/s3proxy/state/metadata.py @@ -2,10 +2,13 @@ import base64 import gzip +import hashlib import structlog +from botocore.exceptions import ClientError from structlog.stdlib import BoundLogger +from ..errors import S3Error from .models import InternalPartMetadata, MultipartMetadata, PartMetadata from .serialization import json_dumps, json_loads @@ -35,6 +38,11 @@ def encode_multipart_metadata(meta: MultipartMetadata) -> str: """ data = { "v": meta.version, + "generation": meta.generation, + "upload_id": meta.upload_id, + "upload_bucket": meta.upload_bucket, + "upload_key": meta.upload_key, + "client_etag": meta.client_etag, "pc": meta.part_count, "ts": meta.total_plaintext_size, "dek": base64.b64encode(meta.wrapped_dek).decode(), @@ -63,6 +71,8 @@ def encode_multipart_metadata(meta: MultipartMetadata) -> str: } json_bytes = json_dumps(data) + if len(json_bytes) > MAX_METADATA_SIZE: + raise S3Error.invalid_request("Multipart metadata exceeds the supported size limit") compressed = gzip.compress(json_bytes) return base64.b64encode(compressed).decode() @@ -88,6 +98,11 @@ def decode_multipart_metadata(encoded: str) -> MultipartMetadata: return MultipartMetadata( version=data.get("v", 1), + generation=data.get("generation", ""), + upload_id=data.get("upload_id", ""), + upload_bucket=data.get("upload_bucket", ""), + upload_key=data.get("upload_key", ""), + client_etag=data.get("client_etag", ""), part_count=data.get("pc", 0), total_plaintext_size=data.get("ts", 0), wrapped_dek=base64.b64decode(data.get("dek", "")), @@ -121,10 +136,16 @@ async def persist_upload_state( upload_id: str, wrapped_dek: bytes, kid: str = "", + *, + layout_version: int = 2, ) -> None: """Persist DEK to S3 during upload (fallback for Redis failures).""" state_key = _internal_upload_key(key, upload_id) - data = {"dek": base64.b64encode(wrapped_dek).decode(), "kid": kid} + data = { + "dek": base64.b64encode(wrapped_dek).decode(), + "kid": kid, + "layout_version": layout_version, + } logger.info( "PERSIST_UPLOAD_STATE", @@ -174,6 +195,8 @@ async def load_upload_state( response = await s3_client.get_object(bucket, state_key) body = await response["Body"].read() data = json_loads(body) + if data.get("layout_version", 2) >= 3: + return None # Restart incomplete uploads; never guess accepted part state. wrapped_dek = base64.b64decode(data["dek"]) logger.info( @@ -230,7 +253,7 @@ async def save_multipart_metadata( meta: MultipartMetadata, ) -> None: """Save multipart metadata to S3.""" - meta_key = _internal_meta_key(key) + meta_key = generation_meta_key(meta.generation) if meta.generation else _internal_meta_key(key) encoded = encode_multipart_metadata(meta) logger.info( @@ -262,68 +285,93 @@ async def save_multipart_metadata( raise -async def load_multipart_metadata( - s3_client, - bucket: str, - key: str, -) -> MultipartMetadata | None: - """Load multipart metadata from S3. +FORMAT_KEY = "s3proxy-format" +GENERATION_KEY = "s3proxy-generation" - Checks the new internal prefix first, then falls back to legacy location. - """ - # Try new location first - meta_key = _internal_meta_key(key) - logger.debug("LOAD_METADATA", bucket=bucket, key=key, meta_key=meta_key) - try: - response = await s3_client.get_object(bucket, meta_key) - body = await response["Body"].read() - encoded = body.decode() - meta = decode_multipart_metadata(encoded) +def generation_for(wrapped_dek: bytes) -> str: + """The wrapped random per-upload DEK identifies an immutable generation.""" + return hashlib.sha256(wrapped_dek).hexdigest() - logger.info( - "METADATA_LOADED", - bucket=bucket, - key=key, - meta_key=meta_key, - part_count=meta.part_count, - total_size=meta.total_plaintext_size, - ) - return meta - except Exception as e: - logger.debug( - "METADATA_NOT_AT_NEW_LOCATION", - bucket=bucket, - key=key, - error=str(e), - ) +def generation_meta_key(generation: str) -> str: + if len(generation) != 64 or any(c not in "0123456789abcdef" for c in generation): + raise S3Error.internal_error("Invalid object generation") + return f"{INTERNAL_PREFIX}generations/{generation}.meta" - # Fall back to legacy location - legacy_key = f"{key}{META_SUFFIX_LEGACY}" - try: - response = await s3_client.get_object(bucket, legacy_key) - body = await response["Body"].read() - encoded = body.decode() - meta = decode_multipart_metadata(encoded) - logger.info( - "METADATA_LOADED_LEGACY", - bucket=bucket, - key=key, - legacy_key=legacy_key, - part_count=meta.part_count, - ) - return meta +def multipart_headers(wrapped_dek: bytes) -> dict[str, str]: + return {FORMAT_KEY: "multipart-v3", GENERATION_KEY: generation_for(wrapped_dek)} - except Exception as e: - logger.debug( - "NO_MULTIPART_METADATA", - bucket=bucket, - key=key, - error=str(e), - ) + +def multipart_etag(meta: MultipartMetadata) -> str: + # Legacy ETags were inconsistent across operations. Use the existing HEAD + # representation consistently for old objects; new manifests record identity. + return ( + meta.client_etag + or hashlib.md5(str(meta.total_plaintext_size).encode(), usedforsecurity=False).hexdigest() + ) + + +def is_not_found(error: ClientError) -> bool: + return str(error.response.get("Error", {}).get("Code")) in {"404", "NoSuchKey", "NotFound"} + + +async def load_multipart_metadata( + s3_client, bucket: str, key: str, head: dict | None = None +) -> MultipartMetadata | None: + """Resolve only the current generation; never turn backend failures into plaintext.""" + if head is None: + try: + head = await s3_client.head_object(bucket, key) + except ClientError as error: + if is_not_found(error): + return None + raise + metadata = head.get("Metadata", {}) + fmt = metadata.get(FORMAT_KEY) + if fmt in ("single-v3", "plain-v3") or (not fmt and "plaintext-size" in metadata): return None + if fmt and fmt != "multipart-v3": + raise S3Error.internal_error("Unsupported encrypted object format") + generation = metadata.get(GENERATION_KEY, "") if fmt else "" + keys = ( + [generation_meta_key(generation)] + if fmt + else [_internal_meta_key(key), f"{key}{META_SUFFIX_LEGACY}"] + ) + for meta_key in keys: + try: + response = await s3_client.get_object(bucket, meta_key) + stream = response["Body"] + body = bytearray() + async with stream: + while len(body) <= MAX_METADATA_SIZE: + chunk = await stream.read(min(65536, MAX_METADATA_SIZE + 1 - len(body))) + if not chunk: + break + body.extend(chunk) + if len(body) > MAX_METADATA_SIZE: + raise S3Error.internal_error("Metadata exceeds size limit") + meta = decode_multipart_metadata(body.decode()) + if generation and meta.generation != generation: + raise S3Error.internal_error("Metadata generation mismatch") + if generation: + sizes = [p.plaintext_size for p in meta.parts] + if ( + meta.part_count != len(meta.parts) + or sum(sizes) != meta.total_plaintext_size + or any(size < 0 for size in sizes) + or sum(p.ciphertext_size for p in meta.parts) != head.get("ContentLength") + ): + raise S3Error.internal_error("Inconsistent encryption metadata") + return meta + except ClientError as error: + if not is_not_found(error): + raise + if fmt: + raise S3Error.internal_error("Required encryption metadata is missing") + return None async def delete_multipart_metadata( diff --git a/s3proxy/state/models.py b/s3proxy/state/models.py index 99292f6..8b09082 100644 --- a/s3proxy/state/models.py +++ b/s3proxy/state/models.py @@ -33,6 +33,7 @@ class PartMetadata: md5: str = "" # Internal sub-parts for streaming uploads internal_parts: list[InternalPartMetadata] = field(default_factory=list) + staging_key: str = "" @dataclass(slots=True) @@ -52,6 +53,9 @@ class MultipartUploadState: total_plaintext_size: int = 0 next_internal_part_number: int = 1 # Next S3 part number to use kid: str = "" # Key id that wraps this upload's DEK ("" = default key) + generation: str = "" + layout_version: int = 2 + write_started: bool = False # True while every client part uses a single internal part (e.g. 5MB ClickHouse # shadow tars). Maps client part N → internal N so 600-part uploads stay under # S3's 10k part limit. Cleared on the first multi-internal client part (Scylla). @@ -76,6 +80,12 @@ class MultipartMetadata: wrapped_dek: bytes = b"" kid: str = "" # Key id that wrapped the DEK ("" = legacy/default key) + generation: str = "" + upload_id: str = "" + upload_bucket: str = "" + upload_key: str = "" + client_etag: str = "" + class StateMissingError(Exception): """Raised when upload state is missing from Redis during add_part.""" diff --git a/s3proxy/state/object.py b/s3proxy/state/object.py new file mode 100644 index 0000000..bf5e58d --- /dev/null +++ b/s3proxy/state/object.py @@ -0,0 +1,17 @@ +"""One interpretation of a backend object for reads, conditions, lists and copies.""" + +from dataclasses import dataclass + +from .models import MultipartMetadata + + +@dataclass(frozen=True, slots=True) +class ObjectDescriptor: + head: dict + multipart: MultipartMetadata | None + plaintext_size: int + etag: str + + @property + def generation(self) -> str: + return self.multipart.generation if self.multipart else "" diff --git a/s3proxy/state/serialization.py b/s3proxy/state/serialization.py index e383d40..445eb38 100644 --- a/s3proxy/state/serialization.py +++ b/s3proxy/state/serialization.py @@ -51,6 +51,9 @@ def serialize_upload_state(state: MultipartUploadState) -> bytes: "total_plaintext_size": state.total_plaintext_size, "next_internal_part_number": state.next_internal_part_number, "kid": state.kid, + "layout_version": state.layout_version, + "write_started": state.write_started, + "generation": state.generation, "deferred_copy_tail": base64.b64encode(state.deferred_copy_tail).decode() if state.deferred_copy_tail else "", @@ -62,6 +65,7 @@ def serialize_upload_state(state: MultipartUploadState) -> bytes: "ciphertext_size": p.ciphertext_size, "etag": p.etag, "md5": p.md5, + "staging_key": p.staging_key, "internal_parts": [ { "internal_part_number": ip.internal_part_number, @@ -129,6 +133,7 @@ def deserialize_upload_state(data: bytes) -> MultipartUploadState | None: ciphertext_size=p["ciphertext_size"], etag=p["etag"], md5=p.get("md5", ""), + staging_key=p.get("staging_key", ""), internal_parts=[ InternalPartMetadata( internal_part_number=ip["internal_part_number"], @@ -164,6 +169,9 @@ def deserialize_upload_state(data: bytes) -> MultipartUploadState | None: total_plaintext_size=obj.get("total_plaintext_size", 0), next_internal_part_number=obj.get("next_internal_part_number", 1), kid=obj.get("kid", ""), + layout_version=obj.get("layout_version", 2), + write_started=obj.get("write_started", bool(parts)), + generation=obj.get("generation", ""), deferred_copy_tail=base64.b64decode(obj["deferred_copy_tail"]) if obj.get("deferred_copy_tail") else b"", diff --git a/s3proxy/streaming/authenticated.py b/s3proxy/streaming/authenticated.py new file mode 100644 index 0000000..ad866b4 --- /dev/null +++ b/s3proxy/streaming/authenticated.py @@ -0,0 +1,51 @@ +"""Bounded legacy GCM reads: authenticate fully before exposing plaintext.""" + +import asyncio +import tempfile + +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + +from .. import crypto +from ..errors import S3Error + + +async def decrypt_to_file(body, dek): + spool = tempfile.SpooledTemporaryFile(max_size=crypto.MAX_BUFFER_SIZE) # noqa: SIM115 -- response owns it + # Roll large plaintext to disk without holding a second full plaintext copy. + pending = bytearray() + decryptor = None + length = 0 + try: + async with body: + while chunk := await body.read(1024 * 1024): + pending.extend(chunk) + if decryptor is None and len(pending) >= crypto.NONCE_SIZE: + nonce = bytes(pending[: crypto.NONCE_SIZE]) + del pending[: crypto.NONCE_SIZE] + decryptor = Cipher(algorithms.AES(dek), modes.GCM(nonce)).decryptor() + if decryptor and len(pending) > crypto.TAG_SIZE: + data = decryptor.update(bytes(pending[: -crypto.TAG_SIZE])) + del pending[: -crypto.TAG_SIZE] + length += len(data) + await asyncio.to_thread(spool.write, data) + if decryptor is None or len(pending) != crypto.TAG_SIZE: + raise S3Error.internal_error("Truncated encrypted object") + decryptor.finalize_with_tag(bytes(pending)) + return spool, length + except BaseException: + spool.close() + raise + + +async def file_range(spool, start, end): + try: + await asyncio.to_thread(spool.seek, start) + remaining = end - start + 1 + while remaining > 0: + chunk = await asyncio.to_thread(spool.read, min(1024 * 1024, remaining)) + if not chunk: + raise S3Error.internal_error("Truncated plaintext spool") + remaining -= len(chunk) + yield chunk + finally: + spool.close() diff --git a/s3proxy/streaming/chunked.py b/s3proxy/streaming/chunked.py index 5b82eca..13f0b73 100644 --- a/s3proxy/streaming/chunked.py +++ b/s3proxy/streaming/chunked.py @@ -1,132 +1,161 @@ -"""AWS chunked encoding utilities for streaming SigV4. - -This module handles the aws-chunked transfer encoding used by -AWS SDK v4 streaming uploads. - -Format: ;chunk-signature=\r\n\r\n...0;chunk-signature=\r\n -""" +"""Strict aws-chunked framing with incremental reads and SigV4 chunk validation.""" +import hashlib +import hmac from collections.abc import AsyncIterator, Iterator from fastapi import Request -# Streaming chunk size for reads/writes -STREAM_CHUNK_SIZE = 64 * 1024 # 64KB chunks for streaming - -# Safety limits for chunked decoding -_MAX_CHUNK_HEADER_SIZE = 4096 # Max header line (hex size + signature) -_MAX_CHUNK_SIZE = 64 * 1024 * 1024 # 64 MB max per chunk -_MAX_BUFFER_SIZE = 66 * 1024 * 1024 # Slightly above max chunk to hold chunk + framing +from ..errors import S3Error +STREAM_CHUNK_SIZE = 64 * 1024 +_MAX_CHUNK_HEADER_SIZE = 4096 +_MAX_CHUNK_SIZE = 64 * 1024 * 1024 -def _parse_chunk_size(header: bytes) -> int: - """Parse and validate chunk size from header bytes.""" - size_str = header.split(b";")[0].strip() - if not size_str: - raise ValueError("Empty chunk size") - chunk_size = int(size_str, 16) - if chunk_size < 0: - raise ValueError(f"Negative chunk size: {chunk_size}") - if chunk_size > _MAX_CHUNK_SIZE: - raise ValueError(f"Chunk size {chunk_size} exceeds maximum {_MAX_CHUNK_SIZE}") - return chunk_size +class ChunkDecoder: + """A bounded framing state machine; EOF is valid only after the terminal chunk.""" -def decode_aws_chunked(body: bytes) -> bytes: - """Decode aws-chunked transfer encoding from buffered body. - - Args: - body: Complete body with aws-chunked encoding - - Returns: - Decoded bytes without chunk headers - - Raises: - ValueError: If chunked encoding is malformed or truncated. - """ - result = bytearray() - pos = 0 - while pos < len(body): - header_end = body.find(b"\r\n", pos) - if header_end == -1: - raise ValueError("Truncated chunk: missing header terminator") - header = body[pos:header_end] - chunk_size = _parse_chunk_size(header) - if chunk_size == 0: - break - data_start = header_end + 2 - data_end = data_start + chunk_size - if data_end > len(body): - raise ValueError( - f"Truncated chunk: expected {chunk_size} bytes, " - f"only {len(body) - data_start} available" - ) - result.extend(body[data_start:data_end]) - pos = data_end + 2 - return bytes(result) - - -async def decode_aws_chunked_stream( - request: Request, -) -> AsyncIterator[bytes]: - """Decode aws-chunked encoding from streaming request. - - Yields decoded data chunks without buffering entire body. - Memory-efficient for large uploads. - - Args: - request: FastAPI request with aws-chunked body - - Yields: - Decoded data chunks - - Raises: - ValueError: If buffer exceeds safety limits or encoding is malformed. - """ - buffer = bytearray() - - async for raw_chunk in request.stream(): - buffer.extend(raw_chunk) - - if len(buffer) > _MAX_BUFFER_SIZE: - raise ValueError( - f"Chunked decode buffer ({len(buffer)} bytes) exceeds " - f"maximum ({_MAX_BUFFER_SIZE} bytes)" - ) + def __init__(self, validate=None): + self.buffer = bytearray() + self.remaining = None + self.terminal = False + self.done = False + self.validate = validate + self.digest = None + self.signature = "" + def feed(self, data: bytes) -> Iterator[bytes]: + self.buffer.extend(data) while True: - header_end = buffer.find(b"\r\n") - if header_end == -1: - if len(buffer) > _MAX_CHUNK_HEADER_SIZE: - raise ValueError(f"Chunk header exceeds {_MAX_CHUNK_HEADER_SIZE} bytes") - break - - header = buffer[:header_end] - chunk_size = _parse_chunk_size(header) - - if chunk_size == 0: + if self.done: + if self.buffer: + raise ValueError("Unexpected bytes after terminal chunk") return + if self.remaining is None: + end = self.buffer.find(b"\r\n") + if end < 0: + if len(self.buffer) > _MAX_CHUNK_HEADER_SIZE: + raise ValueError("Chunk header too large") + return + if end > _MAX_CHUNK_HEADER_SIZE: + raise ValueError("Chunk header too large") + header = bytes(self.buffer[:end]) + del self.buffer[: end + 2] + size, *extensions = header.split(b";") + if not size or any(c not in b"0123456789abcdefABCDEF" for c in size): + raise ValueError("Invalid chunk size") + self.remaining = int(size, 16) + if self.remaining > _MAX_CHUNK_SIZE: + raise ValueError("Chunk size exceeds limit") + self.terminal = self.remaining == 0 + self.digest = hashlib.sha256() + self.signature = "" + for extension in extensions: + if extension.startswith(b"chunk-signature=") and not self.signature: + self.signature = extension.split(b"=", 1)[1].decode("ascii") + else: + raise ValueError("Unsupported chunk extension") + if self.remaining: + if not self.buffer: + return + count = min(self.remaining, len(self.buffer), STREAM_CHUNK_SIZE) + chunk = bytes(self.buffer[:count]) + del self.buffer[:count] + self.remaining -= count + self.digest.update(chunk) + yield chunk + continue + if len(self.buffer) < 2: + return + if self.buffer[:2] != b"\r\n": + raise ValueError("Missing chunk data terminator") + del self.buffer[:2] + if self.validate: + self.validate(self.signature, self.digest.hexdigest()) + elif self.signature: + raise ValueError("Signed chunks require signature verification") + if self.terminal: + self.done = True + self.remaining = None + + def finish(self): + if not self.done or self.buffer: + raise ValueError("Truncated aws-chunked body") - data_start = header_end + 2 - data_end = data_start + chunk_size - trailing_end = data_end + 2 - - if len(buffer) < trailing_end: - break - yield bytes(buffer[data_start:data_end]) - del buffer[:trailing_end] +def decode_aws_chunked(body: bytes) -> bytes: + decoder = ChunkDecoder() + result = b"".join(decoder.feed(body)) + decoder.finish() + return result + + +def _chunk_validator(request: Request): + mode = request.headers.get("x-amz-content-sha256", "") + if mode.startswith("STREAMING-") and mode != "STREAMING-AWS4-HMAC-SHA256-PAYLOAD": + raise S3Error.invalid_request("Unsupported streaming signature/trailer format") + if mode != "STREAMING-AWS4-HMAC-SHA256-PAYLOAD": + return None + from ..client import ParsedRequest + from ..client.verifier import _derive_signing_key + + verifier = request.app.state.verifier + parsed = ParsedRequest( + method=request.method, + bucket="", + key="", + query_params={}, + headers=dict(request.headers), + body=b"", + ) + auth = verifier._parse_header_auth(parsed, request.headers.get("authorization", "")) + if auth.error or auth.credentials is None: + raise S3Error.signature_does_not_match("Invalid streaming authorization") + signing_key = _derive_signing_key( + auth.credentials.secret_key, auth.date_stamp, auth.region, auth.service + ) + scope = f"{auth.date_stamp}/{auth.region}/{auth.service}/aws4_request" + previous = auth.signature + + def validate(signature, digest): + nonlocal previous + message = "\n".join( + [ + "AWS4-HMAC-SHA256-PAYLOAD", + auth.amz_date, + scope, + previous, + hashlib.sha256(b"").hexdigest(), + digest, + ] + ) + expected = hmac.new(signing_key, message.encode(), hashlib.sha256).hexdigest() + if not hmac.compare_digest(expected, signature): + raise S3Error.signature_does_not_match("Invalid streaming chunk signature") + previous = signature + + return validate + + +async def decode_aws_chunked_stream(request: Request) -> AsyncIterator[bytes]: + decoder = ChunkDecoder(_chunk_validator(request)) + decoded = 0 + try: + async for raw in request.stream(): + # Bound parser buffering even if the ASGI server delivers a large block. + for offset in range(0, len(raw), STREAM_CHUNK_SIZE): + for chunk in decoder.feed(raw[offset : offset + STREAM_CHUNK_SIZE]): + decoded += len(chunk) + yield chunk + decoder.finish() + expected = request.headers.get("x-amz-decoded-content-length") + if expected is not None and int(expected) != decoded: + raise ValueError("Decoded content length mismatch") + except (ValueError, UnicodeError) as error: + raise S3Error.bad_request(str(error)) from error def chunked(data: bytes, size: int) -> Iterator[tuple[int, bytes]]: - """Split data into numbered chunks for multipart upload. - - Args: - data: Data to split - size: Chunk size in bytes - - Yields: - (part_number, chunk) tuples starting from part 1 - """ for i in range(0, len(data), size): yield i // size + 1, data[i : i + size] diff --git a/s3proxy/streaming/frames.py b/s3proxy/streaming/frames.py new file mode 100644 index 0000000..eb998d7 --- /dev/null +++ b/s3proxy/streaming/frames.py @@ -0,0 +1,127 @@ +"""Read contiguous ciphertext ranges while authenticating one frame at a time.""" + +import asyncio +import contextlib + +from .. import crypto +from ..errors import S3Error + + +async def read_frames(client, bucket, key, frames, *, if_match=None): + # Frames are (ciphertext_offset, ciphertext_size, plaintext_slice_start, end). + # The window limits retry scope, not buffering: only one frame is accumulated. + index = 0 + while index < len(frames): + end_index = index + 1 + end = frames[index][0] + frames[index][1] + while end_index < len(frames): + offset, size, _, _ = frames[end_index] + if offset != end or offset + size - frames[index][0] > 64 * 1024**2: + break + end += size + end_index += 1 + from ..handlers.base import ( + SOURCE_READ_ATTEMPTS, + SOURCE_READ_BACKOFF_SEC, + is_retryable_source_error, + ) + + attempt = 0 + while index < end_index: + response = None + try: + response = await client.get_object( + bucket, + key, + f"bytes={frames[index][0]}-{end - 1}", + **({"if_match": if_match} if if_match else {}), + ) + body = response["Body"] + async with body: + while index < end_index: + _, size, start, stop = frames[index] + ciphertext = bytearray(size) + received = 0 + while received < size: + chunk = await body.read(min(1024**2, size - received)) + if not chunk: + raise EOFError("Truncated ciphertext range") + ciphertext[received : received + len(chunk)] = chunk + received += len(chunk) + yield ciphertext, start, stop + index += 1 + attempt = 0 + except Exception as error: + attempt += 1 + if attempt >= SOURCE_READ_ATTEMPTS or not ( + isinstance(error, EOFError) or is_retryable_source_error(error) + ): + raise + await asyncio.sleep(SOURCE_READ_BACKOFF_SEC * 2 ** (attempt - 1)) + + +async def plaintext_frames( + client, bucket, key, meta, dek, start=None, end=None, *, if_match=None, ciphertext_size=None +): + frames = [] + pt_offset = 0 + ct_offset = 0 + for part in sorted(meta.parts, key=lambda p: p.part_number): + segments = part.internal_parts or [part] + for segment_number, segment in enumerate(segments, 1): + for size in crypto.ciphertext_frame_byte_sizes( + segment.plaintext_size, segment.ciphertext_size + ): + plaintext_size = size - crypto.ENCRYPTION_OVERHEAD + if start is None or (pt_offset + plaintext_size > start and pt_offset <= end): + if ciphertext_size is not None and ct_offset + size > ciphertext_size: + raise S3Error.invalid_range( + f"Metadata corruption: part {part.part_number}, internal part " + f"{segment_number} exceeds object size {ciphertext_size}" + ) + left = max(0, start - pt_offset) if start is not None else 0 + right = ( + min(plaintext_size, end - pt_offset + 1) + if end is not None + else plaintext_size + ) + frames.append((ct_offset, size, left, right)) + pt_offset += plaintext_size + ct_offset += size + # Old objects may contain a single GCM seal larger than the modern frame. + # Authenticate those seals to a bounded spool before releasing any plaintext. + index = 0 + while index < len(frames): + offset, size, left, right = frames[index] + if size > (crypto.FRAME_PLAINTEXT_SIZE + crypto.ENCRYPTION_OVERHEAD): + from .authenticated import decrypt_to_file, file_range + + response = await client.get_object( + bucket, + key, + f"bytes={offset}-{offset + size - 1}", + **({"if_match": if_match} if if_match else {}), + ) + spool, _ = await decrypt_to_file(response["Body"], dek) + try: + async with contextlib.aclosing(file_range(spool, left, right - 1)) as stream: + async for chunk in stream: + yield chunk + finally: + spool.close() + index += 1 + continue + stop = index + 1 + while stop < len(frames) and frames[stop][1] <= ( + crypto.FRAME_PLAINTEXT_SIZE + crypto.ENCRYPTION_OVERHEAD + ): + stop += 1 + async with contextlib.aclosing( + read_frames(client, bucket, key, frames[index:stop], if_match=if_match) + ) as reader: + async for ciphertext, left, right in reader: + plaintext = crypto.decrypt(ciphertext, dek) + for offset in range(left, right, 1024**2): + yield plaintext[offset : min(offset + 1024**2, right)] + del plaintext, ciphertext + index = stop diff --git a/s3proxy/streaming/response.py b/s3proxy/streaming/response.py new file mode 100644 index 0000000..46877a0 --- /dev/null +++ b/s3proxy/streaming/response.py @@ -0,0 +1,27 @@ +"""Streaming responses whose resources also close on ASGI send failures.""" + +import anyio +from fastapi.responses import StreamingResponse + + +class OwnedStreamingResponse(StreamingResponse): + def __init__(self, *args, cleanup=None, on_error=None, **kwargs): + super().__init__(*args, **kwargs) + self.cleanup = cleanup + self.on_error = on_error + + async def __call__(self, scope, receive, send): + try: + await super().__call__(scope, receive, send) + except BaseException: + if self.on_error is not None: + self.on_error() + raise + finally: + with anyio.CancelScope(shield=True): + try: + if hasattr(self.body_iterator, "aclose"): + await self.body_iterator.aclose() + finally: + if self.cleanup is not None: + await self.cleanup() diff --git a/tests/conftest.py b/tests/conftest.py index 62396d2..c757411 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,7 @@ import fakeredis.aioredis import pytest +from botocore.exceptions import ClientError # Set environment variables before importing s3proxy modules os.environ.setdefault("S3PROXY_HOST", "http://localhost:9000") @@ -207,7 +208,9 @@ async def put_object( } return {"ETag": f'"{hashlib.md5(body).hexdigest()}"'} - async def get_object(self, bucket: str, key: str, range_header: str | None = None) -> dict: + async def get_object( + self, bucket: str, key: str, range_header: str | None = None, if_match: str | None = None + ) -> dict: """Retrieve an object.""" self.call_history.append( ("get_object", {"bucket": bucket, "key": key, "range": range_header}) @@ -341,6 +344,7 @@ async def copy_object( metadata: dict[str, str] | None = None, metadata_directive: str = "COPY", content_type: str | None = None, + **kwargs, ) -> dict: """Copy an object.""" self.call_history.append( @@ -413,6 +417,8 @@ async def create_multipart_upload(self, bucket: str, key: str, **kwargs) -> dict "Bucket": bucket, "Key": key, "Parts": {}, + "Metadata": kwargs.get("metadata", {}), + "ContentType": kwargs.get("content_type", "application/octet-stream"), "Initiated": datetime.now(UTC), } return {"UploadId": upload_id} @@ -466,8 +472,8 @@ async def complete_multipart_upload( etag = hashlib.md5(body).hexdigest() self.objects[self._key(bucket, key)] = { "Body": body, - "Metadata": {}, - "ContentType": "application/octet-stream", + "Metadata": upload.get("Metadata", {}), + "ContentType": upload.get("ContentType", "application/octet-stream"), "ContentLength": len(body), "ETag": etag, "LastModified": datetime.now(UTC), @@ -652,6 +658,7 @@ async def upload_part_copy( part_number: int, copy_source: str, copy_source_range: str | None = None, + copy_source_if_match: str | None = None, ) -> dict: """Copy a part from another object.""" self.call_history.append( @@ -704,9 +711,9 @@ async def upload_part_copy( def _not_found_error(self, key: str): """Create a NoSuchKey error.""" - error = Exception(f"NoSuchKey: {key}") - error.response = {"Error": {"Code": "NoSuchKey", "Message": f"Key not found: {key}"}} - return error + return ClientError( + {"Error": {"Code": "NoSuchKey", "Message": f"Key not found: {key}"}}, "GetObject" + ) def _bucket_not_found_error(self, bucket: str): """Create a NoSuchBucket error.""" diff --git a/tests/docker-compose.oom.yml b/tests/docker-compose.oom.yml index 6397005..bfd405a 100644 --- a/tests/docker-compose.oom.yml +++ b/tests/docker-compose.oom.yml @@ -1,6 +1,9 @@ # OOM tests upload 5GB+ into MinIO (e.g. 20 concurrent 256MB PUTs). # The default compose caps MinIO at 4g tmpfs for integration shards; drop # that cap here so MinIO uses disk-backed storage like before #117. +# The memory-usage shard also uses this override: immutable staging adds +# temporary copies to its multi-gigabyte workload. Proxy RSS limits and +# assertions stay unchanged; this removes only the backend storage cap. services: minio: tmpfs: !reset null diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index a51fadc..b9853fb 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -136,7 +136,7 @@ def run_s3proxy( '[{"access_key":"minioadmin","secret_key":"minioadmin",' '"kek":"test-encryption-key-32-bytes!!"}]' ), - "S3PROXY_HOST": "http://localhost:9000", + "S3PROXY_HOST": os.environ.get("S3PROXY_TEST_BACKEND", "http://localhost:9000"), "S3PROXY_REGION": "us-east-1", "S3PROXY_PORT": str(port), "S3PROXY_NO_TLS": "true", diff --git a/tests/integration/passthrough_verify.py b/tests/integration/passthrough_verify.py index 3bfbd61..e090586 100644 --- a/tests/integration/passthrough_verify.py +++ b/tests/integration/passthrough_verify.py @@ -137,7 +137,9 @@ def upload_multipart(ctx: RunContext, key: str, size: int) -> None: UploadId=upload_id, MultipartUpload={"Parts": parts}, ) - meta = f".s3proxy-internal/{key}.meta" + head = ctx.raw.head_object(Bucket=ctx.bucket, Key=key) + generation = head["Metadata"]["s3proxy-generation"] + meta = f".s3proxy-internal/generations/{generation}.meta" assert ctx.raw.head_object(Bucket=ctx.bucket, Key=meta)["ContentLength"] > 0 @@ -191,7 +193,13 @@ def _poll() -> None: def load_sidecar(ctx: RunContext, key: str): - meta_key = f".s3proxy-internal/{key}.meta" + head = ctx.raw.head_object(Bucket=ctx.bucket, Key=key) + generation = head.get("Metadata", {}).get("s3proxy-generation") + meta_key = ( + f".s3proxy-internal/generations/{generation}.meta" + if generation + else f".s3proxy-internal/{key}.meta" + ) raw = ctx.raw.get_object(Bucket=ctx.bucket, Key=meta_key)["Body"].read() return decode_multipart_metadata(raw.decode()) @@ -311,7 +319,7 @@ def check_reencrypt_control(ctx: RunContext, source: str, dest: str, size: int) lambda: upload_part_copy(ctx, dest, source, byte_range=f"bytes=0-{partial_end}"), ) ctx.ok("encrypts partial range", enc >= (partial_end + 1) * 0.5, f"{enc / MB:.0f}MB") - ctx.ok("high peak memory", peak >= CHUNK_PEAK * 0.5, f"{peak / MB:.2f}MB") + ctx.ok("bounded re-encryption memory", peak <= 32 * MB, f"{peak / MB:.2f}MB") def check_scylla_manifest_full_range_passthrough( diff --git a/tests/integration/test_copy_passthrough.py b/tests/integration/test_copy_passthrough.py index 04df925..b9188f4 100644 --- a/tests/integration/test_copy_passthrough.py +++ b/tests/integration/test_copy_passthrough.py @@ -17,7 +17,7 @@ from s3proxy.handlers import S3ProxyHandler from s3proxy.state import MultipartStateManager -from s3proxy.state.metadata import _internal_meta_key +from s3proxy.state.metadata import GENERATION_KEY, generation_meta_key BUCKET = "backups" @@ -109,7 +109,9 @@ async def test_multipart_object_copy_copies_sidecar_and_roundtrips(settings, moc body = b"m" * (256 * 1024) # streamed as multiple parts -> real sidecar await handler.handle_put_object(_stream_put_request(f"/{BUCKET}/sst/big.db", body), credentials) # sanity: a multipart sidecar exists for the source - assert mock_s3._key(BUCKET, _internal_meta_key("sst/big.db")) in mock_s3.objects + source_head = await mock_s3.head_object(BUCKET, "sst/big.db") + manifest = generation_meta_key(source_head["Metadata"][GENERATION_KEY]) + assert mock_s3._key(BUCKET, manifest) in mock_s3.objects mark = len(mock_s3.call_history) await handler.handle_copy_object( @@ -119,7 +121,8 @@ async def test_multipart_object_copy_copies_sidecar_and_roundtrips(settings, moc copied = _keys_touched(during, "copy_object") assert "sst/big.db.snap" in copied # assembled ciphertext, server-side - assert _internal_meta_key("sst/big.db.snap") in copied # sidecar, server-side + dest_head = await mock_s3.head_object(BUCKET, "sst/big.db.snap") + assert dest_head["Metadata"][GENERATION_KEY] == source_head["Metadata"][GENERATION_KEY] assert "sst/big.db" not in _keys_touched(during, "get_object") # no bulk download assert "sst/big.db.snap" not in _keys_touched(during, "put_object") # no re-upload diff --git a/tests/integration/test_elasticsearch_range_scenario.py b/tests/integration/test_elasticsearch_range_scenario.py index 720f91b..e941c8a 100644 --- a/tests/integration/test_elasticsearch_range_scenario.py +++ b/tests/integration/test_elasticsearch_range_scenario.py @@ -16,6 +16,7 @@ MultipartMetadata, PartMetadata, ) +from tests.conftest import MockS3Response @pytest.fixture @@ -105,7 +106,7 @@ async def test_elasticsearch_backup_range_error(self, handler, settings, kek): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() @@ -219,7 +220,7 @@ def get_object_side_effect(bucket, key, range_header=None): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() @@ -283,32 +284,12 @@ async def test_successful_3_part_fetch(self, handler, settings, kek): return_value={"ContentLength": total_ciphertext_size, "LastModified": None} ) - # Mock get_object to return the correct ciphertext for each range - def get_object_side_effect(bucket, key, range_header=None): - if range_header: - # Parse range to determine which part to return - range_str = range_header.replace("bytes=", "") - start, end = map(int, range_str.split("-")) + # A real body honors bounded reads and each GET's requested range. + ciphertext = b"".join(part["ciphertext"] for part in internal_parts_data) - # Find which internal part this range corresponds to - current_offset = 0 - for part_data in internal_parts_data: - part_size = part_data["meta"].ciphertext_size - if start >= current_offset and start < current_offset + part_size: - # This is the right part - mock_body = AsyncMock() - mock_body.read = AsyncMock(return_value=part_data["ciphertext"]) - mock_body.__aenter__ = AsyncMock(return_value=mock_body) - mock_body.__aexit__ = AsyncMock(return_value=None) - return {"Body": mock_body} - current_offset += part_size - - # Default mock - mock_body = AsyncMock() - mock_body.read = AsyncMock(return_value=b"") - mock_body.__aenter__ = AsyncMock(return_value=mock_body) - mock_body.__aexit__ = AsyncMock(return_value=None) - return {"Body": mock_body} + async def get_object_side_effect(bucket, key, range_header=None, **kwargs): + start, end = map(int, range_header.removeprefix("bytes=").split("-")) + return {"Body": MockS3Response(ciphertext[start : end + 1])} mock_client.get_object = AsyncMock(side_effect=get_object_side_effect) @@ -337,7 +318,7 @@ def get_object_side_effect(bucket, key, range_header=None): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() diff --git a/tests/integration/test_entity_too_small_errors.py b/tests/integration/test_entity_too_small_errors.py index 30a522f..d0240c6 100644 --- a/tests/integration/test_entity_too_small_errors.py +++ b/tests/integration/test_entity_too_small_errors.py @@ -17,6 +17,10 @@ class TestEntityTooSmallHandling: async def test_complete_with_missing_part_rejected(self, handler, settings): """Test that CompleteMultipartUpload fails when client requests non-existent parts.""" mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) @@ -82,6 +86,10 @@ async def test_complete_with_missing_part_rejected(self, handler, settings): async def test_entity_too_small_with_small_parts(self, handler, settings): """Test EntityTooSmall error when multiple parts are < 5MB.""" mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) diff --git a/tests/integration/test_generation_roundtrip.py b/tests/integration/test_generation_roundtrip.py new file mode 100644 index 0000000..70b5e3f --- /dev/null +++ b/tests/integration/test_generation_roundtrip.py @@ -0,0 +1,153 @@ +"""Real S3/HTTP coverage for v3 generation publication and staged assembly.""" + +import os +import uuid + +import boto3 +import pytest +from botocore.config import Config +from botocore.exceptions import ClientError + +from .conftest import _find_free_port, minio_backend, run_s3proxy + +pytestmark = pytest.mark.e2e + + +@pytest.mark.parametrize( + "redis_url", + [""] + + ([os.environ["S3PROXY_TEST_REDIS_URL"]] if os.environ.get("S3PROXY_TEST_REDIS_URL") else []), +) +def test_generation_roundtrip(redis_url): + with ( + minio_backend() as backend, + run_s3proxy( + _find_free_port(), + S3PROXY_HOST=backend, + S3PROXY_MEMORY_LIMIT_MB="64", + S3PROXY_REDIS_URL=redis_url, + log_output=True, + ) as (endpoint, _), + ): + client = boto3.client( + "s3", + endpoint_url=endpoint, + aws_access_key_id="minioadmin", + aws_secret_access_key="minioadmin", + region_name="us-east-1", + config=Config( + s3={"addressing_style": "path"}, + request_checksum_calculation="when_required", + response_checksum_validation="when_required", + ), + ) + bucket = "generation-" + uuid.uuid4().hex[:16] + client.create_bucket(Bucket=bucket) + upload = client.create_multipart_upload( + Bucket=bucket, Key="object", Metadata={"owner": "test"} + )["UploadId"] + tail = client.upload_part( + Bucket=bucket, Key="object", UploadId=upload, PartNumber=2, Body=b"tail" + ) + data = b"A" * (9 * 1024**2) + first = client.upload_part( + Bucket=bucket, Key="object", UploadId=upload, PartNumber=1, Body=data + ) + pending = client.list_parts(Bucket=bucket, Key="object", UploadId=upload, MaxParts=1) + assert pending["IsTruncated"] and pending["Parts"][0]["Size"] == len(data) + assert pending["Parts"][0]["ETag"] == first["ETag"] + second_page = client.list_parts( + Bucket=bucket, Key="object", UploadId=upload, PartNumberMarker=1 + ) + assert second_page["Parts"][0]["Size"] == 4 + parts = [{"PartNumber": 1, "ETag": first["ETag"]}, {"PartNumber": 2, "ETag": tail["ETag"]}] + complete = client.complete_multipart_upload( + Bucket=bucket, Key="object", UploadId=upload, MultipartUpload={"Parts": parts} + ) + head = client.head_object(Bucket=bucket, Key="object") + get = client.get_object(Bucket=bucket, Key="object") + assert get["Body"].read() == data + b"tail" + assert head["Metadata"] == {"owner": "test"} + assert head["ETag"] == get["ETag"] == complete["ETag"] + listed = client.list_objects_v2(Bucket=bucket)["Contents"] + assert len(listed) == 1 and listed[0]["ETag"] == complete["ETag"] + ranged = client.get_object(Bucket=bucket, Key="object", Range=f"bytes={len(data) - 2}-") + assert ranged["Body"].read() == b"AAtail" + client.complete_multipart_upload( + Bucket=bucket, Key="object", UploadId=upload, MultipartUpload={"Parts": parts} + ) + with pytest.raises(ClientError): + client.complete_multipart_upload( + Bucket=bucket, + Key="object", + UploadId="wrong-upload", + MultipartUpload={"Parts": parts}, + ) + client.copy_object( + Bucket=bucket, Key="native", CopySource={"Bucket": bucket, "Key": "object"} + ) + assert client.get_object(Bucket=bucket, Key="native")["Body"].read() == data + b"tail" + assert client.head_object(Bucket=bucket, Key="native")["Metadata"] == {"owner": "test"} + client.put_object(Bucket=bucket, Key="object", Body=b"new", Metadata={"owner": "new"}) + assert client.get_object(Bucket=bucket, Key="object")["Body"].read() == b"new" + with pytest.raises(ClientError) as error: + client.put_object(Bucket=bucket, Key="object", Body=b"rejected", IfNoneMatch="*") + assert error.value.response["ResponseMetadata"]["HTTPStatusCode"] == 412 + assert client.get_object(Bucket=bucket, Key="object")["Body"].read() == b"new" + # Exercise the shared staging pipeline for server-side copy parts. + upload = client.create_multipart_upload(Bucket=bucket, Key="copy")["UploadId"] + part = client.upload_part_copy( + Bucket=bucket, + Key="copy", + UploadId=upload, + PartNumber=1, + CopySource={"Bucket": bucket, "Key": "object"}, + ) + client.complete_multipart_upload( + Bucket=bucket, + Key="copy", + UploadId=upload, + MultipartUpload={"Parts": [{"PartNumber": 1, "ETag": part["CopyPartResult"]["ETag"]}]}, + ) + assert client.get_object(Bucket=bucket, Key="copy")["Body"].read() == b"new" + + # A raw source copied over a previously encrypted destination must ignore old sidecars. + raw = boto3.client( + "s3", + endpoint_url=backend, + aws_access_key_id="minioadmin", + aws_secret_access_key="minioadmin", + region_name="us-east-1", + ) + raw.put_object(Bucket=bucket, Key="raw", Body=b"plain") + client.copy_object(Bucket=bucket, Key="native", CopySource={"Bucket": bucket, "Key": "raw"}) + assert client.get_object(Bucket=bucket, Key="native")["Body"].read() == b"plain" + # Fault injection against real HTTP at the buffering boundary. + import hashlib + + import requests + from botocore.auth import S3SigV4Auth + from botocore.awsrequest import AWSRequest + from botocore.credentials import Credentials + + for length in (8 * 1024**2 - 1, 8 * 1024**2, 8 * 1024**2 + 1): + good = b"a" * length + signed = AWSRequest( + method="PUT", + url=f"{endpoint}/{bucket}/object", + data=good, + headers={"x-amz-content-sha256": hashlib.sha256(good).hexdigest()}, + ) + S3SigV4Auth(Credentials("minioadmin", "minioadmin"), "s3", "us-east-1").add_auth(signed) + result = requests.put( + signed.url, headers=dict(signed.headers), data=b"b" * length, timeout=15 + ) + assert result.status_code == 403 + assert client.get_object(Bucket=bucket, Key="object")["Body"].read() == b"new" + # This test owns a unique bucket and removes only that bucket's data. + objects = raw.list_objects_v2(Bucket=bucket).get("Contents", []) + if objects: + raw.delete_objects( + Bucket=bucket, Delete={"Objects": [{"Key": o["Key"]} for o in objects]} + ) + raw.delete_bucket(Bucket=bucket) diff --git a/tests/integration/test_memory_usage.py b/tests/integration/test_memory_usage.py index d9e24fc..e154742 100644 --- a/tests/integration/test_memory_usage.py +++ b/tests/integration/test_memory_usage.py @@ -55,7 +55,7 @@ def s3proxy_with_memory_limit(self): '[{"access_key":"minioadmin","secret_key":"minioadmin",' '"kek":"test-encryption-key-32-bytes!!"}]' ), - "S3PROXY_HOST": "http://localhost:9000", + "S3PROXY_HOST": os.environ.get("S3PROXY_TEST_BACKEND", "http://localhost:9000"), "S3PROXY_REGION": "us-east-1", "S3PROXY_PORT": str(port), "S3PROXY_NO_TLS": "true", @@ -109,7 +109,7 @@ def s3proxy_with_short_backpressure(self): '[{"access_key":"minioadmin","secret_key":"minioadmin",' '"kek":"test-encryption-key-32-bytes!!"}]' ), - "S3PROXY_HOST": "http://localhost:9000", + "S3PROXY_HOST": os.environ.get("S3PROXY_TEST_BACKEND", "http://localhost:9000"), "S3PROXY_REGION": "us-east-1", "S3PROXY_PORT": str(port), "S3PROXY_NO_TLS": "true", @@ -170,14 +170,28 @@ def stress_bucket(self, stress_client): with contextlib.suppress(stress_client.exceptions.BucketAlreadyOwnedByYou): stress_client.create_bucket(Bucket=bucket) yield bucket - try: - response = stress_client.list_objects_v2(Bucket=bucket) - if "Contents" in response: - objects = [{"Key": obj["Key"]} for obj in response["Contents"]] - stress_client.delete_objects(Bucket=bucket, Delete={"Objects": objects}) - stress_client.delete_bucket(Bucket=bucket) - except Exception: - pass + # Delete only this test's unique bucket, including hidden manifests and + # unfinished staging uploads. Proxy LIST deliberately hides these keys. + with contextlib.closing( + boto3.client( + "s3", + endpoint_url=os.environ.get("S3PROXY_TEST_BACKEND", "http://localhost:9000"), + aws_access_key_id="minioadmin", + aws_secret_access_key="minioadmin", + region_name="us-east-1", + ) + ) as raw: + for page in raw.get_paginator("list_multipart_uploads").paginate(Bucket=bucket): + for upload in page.get("Uploads", []): + raw.abort_multipart_upload( + Bucket=bucket, Key=upload["Key"], UploadId=upload["UploadId"] + ) + for page in raw.get_paginator("list_objects_v2").paginate(Bucket=bucket): + objects = [{"Key": obj["Key"]} for obj in page.get("Contents", [])] + if objects: + result = raw.delete_objects(Bucket=bucket, Delete={"Objects": objects}) + assert not result.get("Errors"), result + raw.delete_bucket(Bucket=bucket) def test_backpressure_queues_concurrent_uploads(self, s3proxy_with_memory_limit, stress_bucket): """Verify backpressure queues excess requests instead of rejecting them. diff --git a/tests/integration/test_multipart_range_validation.py b/tests/integration/test_multipart_range_validation.py index c91e45b..a522386 100644 --- a/tests/integration/test_multipart_range_validation.py +++ b/tests/integration/test_multipart_range_validation.py @@ -9,6 +9,7 @@ from s3proxy.errors import S3Error from s3proxy.handlers.objects import ObjectHandlerMixin from s3proxy.state import InternalPartMetadata, MultipartMetadata, PartMetadata +from tests.conftest import MockS3Response @pytest.fixture @@ -97,7 +98,7 @@ async def test_invalid_range_detected_before_fetch( # Mock load_multipart_metadata to return our test metadata with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): # Mock credentials @@ -155,7 +156,7 @@ async def test_handles_s3_invalid_range_error(self, handler, settings, kek): invalid_range_error = ClientError(error_response, "GetObject") mock_client.head_object = AsyncMock( - return_value={"ContentLength": 100, "LastModified": None} + return_value={"ContentLength": 1028, "LastModified": None} ) mock_client.get_object = AsyncMock(side_effect=invalid_range_error) @@ -194,7 +195,7 @@ async def test_handles_s3_invalid_range_error(self, handler, settings, kek): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() @@ -209,10 +210,8 @@ async def test_handles_s3_invalid_range_error(self, handler, settings, kek): pass # Verify error message is helpful - assert ( - "metadata corruption" in str(exc_info.value).lower() - or "cannot read" in str(exc_info.value).lower() - ) + assert exc_info.value.code == "InvalidRange" + mock_client.get_object.assert_awaited_once() @pytest.mark.asyncio async def test_valid_range_succeeds(self, handler, settings, kek): @@ -234,10 +233,7 @@ async def test_valid_range_succeeds(self, handler, settings, kek): ) # Mock get_object to return ciphertext - mock_body = AsyncMock() - mock_body.read = AsyncMock(return_value=ciphertext) - mock_body.__aenter__ = AsyncMock(return_value=mock_body) - mock_body.__aexit__ = AsyncMock(return_value=None) + mock_body = MockS3Response(ciphertext) mock_client.get_object = AsyncMock( return_value={"Body": mock_body, "ContentType": "application/octet-stream"} ) @@ -277,7 +273,7 @@ async def test_valid_range_succeeds(self, handler, settings, kek): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() @@ -290,6 +286,7 @@ async def test_valid_range_succeeds(self, handler, settings, kek): # Verify response is valid assert response is not None assert response.status_code == 200 + assert b"".join([chunk async for chunk in response.body_iterator]) == plaintext @pytest.mark.asyncio async def test_multiple_internal_parts_validation(self, handler, settings, kek): @@ -349,7 +346,7 @@ async def test_multiple_internal_parts_validation(self, handler, settings, kek): mock_request.headers = {} with patch( - "s3proxy.handlers.objects.get.load_multipart_metadata", + "s3proxy.state.metadata.load_multipart_metadata", return_value=meta, ): creds = Mock() diff --git a/tests/integration/test_part_ordering.py b/tests/integration/test_part_ordering.py index 5081f88..eb739e8 100644 --- a/tests/integration/test_part_ordering.py +++ b/tests/integration/test_part_ordering.py @@ -3,6 +3,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from botocore.exceptions import ClientError from s3proxy.client import S3Credentials from s3proxy.handlers import S3ProxyHandler @@ -109,10 +110,14 @@ async def test_out_of_order_client_parts_sorted_internally(self, manager, settin # Mock S3 client mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) - mock_client.complete_multipart_upload = AsyncMock() + mock_client.complete_multipart_upload = AsyncMock(return_value={"ETag": "backend-etag"}) mock_client.head_object = AsyncMock(return_value={"ContentLength": 3156}) creds = S3Credentials( @@ -194,10 +199,14 @@ async def test_sequential_parts_remain_sorted(self, manager, settings): # Mock S3 client mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) - mock_client.complete_multipart_upload = AsyncMock() + mock_client.complete_multipart_upload = AsyncMock(return_value={"ETag": "backend-etag"}) mock_client.head_object = AsyncMock(return_value={"ContentLength": 3156}) creds = S3Credentials( diff --git a/tests/integration/test_partial_complete_fix.py b/tests/integration/test_partial_complete_fix.py index a67d206..08ff614 100644 --- a/tests/integration/test_partial_complete_fix.py +++ b/tests/integration/test_partial_complete_fix.py @@ -3,6 +3,7 @@ from unittest.mock import AsyncMock, Mock, patch import pytest +from botocore.exceptions import ClientError from s3proxy import crypto from s3proxy.state import InternalPartMetadata, PartMetadata @@ -19,6 +20,10 @@ async def test_complete_with_subset_of_parts(self, handler, settings): completes with 3 parts, the metadata should only reference the 3 completed parts. """ mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) @@ -50,7 +55,7 @@ async def test_complete_with_subset_of_parts(self, handler, settings): await handler.multipart_manager.add_part("bucket", "key", "upload-123", part) # Mock S3 client responses - mock_client.complete_multipart_upload = AsyncMock() + mock_client.complete_multipart_upload = AsyncMock(return_value={"ETag": "backend-etag"}) mock_client.head_object = AsyncMock( return_value={"ContentLength": 3 * 1028} # Only 3 parts completed ) @@ -126,6 +131,10 @@ async def capture_save(client, bucket, key, meta): async def test_complete_logs_size_mismatch(self, handler, settings): """Test that size mismatches are logged but don't fail the upload.""" mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) @@ -154,7 +163,7 @@ async def test_complete_logs_size_mismatch(self, handler, settings): ) await handler.multipart_manager.add_part("bucket", "key", "upload-123", part) - mock_client.complete_multipart_upload = AsyncMock() + mock_client.complete_multipart_upload = AsyncMock(return_value={"ETag": "backend-etag"}) # Return size that doesn't match our metadata (simulate S3 corruption or issue) mock_client.head_object = AsyncMock( return_value={"ContentLength": 9999} # Wrong size @@ -194,6 +203,10 @@ async def test_complete_with_no_parts_fails(self, handler, settings): from s3proxy.errors import S3Error mock_client = AsyncMock() + mock_client.head_object = AsyncMock(return_value={"ContentLength": 0}) + mock_client.get_object = AsyncMock( + side_effect=ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") + ) # Make mock_client an async context manager mock_client.__aenter__ = AsyncMock(return_value=mock_client) mock_client.__aexit__ = AsyncMock(return_value=None) diff --git a/tests/integration/test_upload_part_copy_passthrough_e2e.py b/tests/integration/test_upload_part_copy_passthrough_e2e.py index e598579..36fd6bc 100644 --- a/tests/integration/test_upload_part_copy_passthrough_e2e.py +++ b/tests/integration/test_upload_part_copy_passthrough_e2e.py @@ -80,7 +80,7 @@ def test_range_copy_reencrypts_with_high_memory(self, passthrough_env): check_reencrypt_control(ctx, "sst/source-96mb.bin", "sst/dest-d.bin", QUICK_SIZE) assert proc.poll() is None _assert_check(ctx, "encrypts partial range") - _assert_check(ctx, "high peak memory") + _assert_check(ctx, "bounded re-encryption memory") def test_scylla_manifest_full_range_passthrough(self, passthrough_env): ctx, proc, _ = passthrough_env @@ -143,4 +143,4 @@ def test_passthrough_encrypt_delta_vs_reencrypt(self, passthrough_env): assert enc_pt <= 1 * MB assert enc_re >= (partial_end + 1) * 0.5 assert peak_pt <= CHUNK_PEAK * 0.5 - assert peak_re >= CHUNK_PEAK * 0.5 + assert peak_re <= 32 * MB diff --git a/tests/unit/test_complete_multipart_retry.py b/tests/unit/test_complete_multipart_retry.py index 2d30d88..ac2ec70 100644 --- a/tests/unit/test_complete_multipart_retry.py +++ b/tests/unit/test_complete_multipart_retry.py @@ -146,10 +146,19 @@ async def test_does_not_retry_non_retryable_error(handler): @pytest.mark.asyncio -async def test_recovers_via_head_object_when_prior_attempt_already_finished(handler): +async def test_recovers_via_head_object_when_prior_attempt_already_finished(handler, monkeypatch): """The exact prod failure: backend finished the assembly, the client only saw the error, and a naive retry would otherwise report a false failure.""" client = _FlakyCompleteClient(phantom_success_on_first=True) + from unittest.mock import AsyncMock + + from s3proxy.state import MultipartMetadata + + monkeypatch.setattr( + lifecycle, + "load_multipart_metadata", + AsyncMock(return_value=MultipartMetadata(upload_id="upload-1")), + ) resp = await handler._complete_multipart_upload_with_retry( client, "bucket", "key", "upload-1", S3_PARTS, COMPLETED_PARTS diff --git a/tests/unit/test_complete_upload_lock.py b/tests/unit/test_complete_upload_lock.py index 9e6c758..1efb353 100644 --- a/tests/unit/test_complete_upload_lock.py +++ b/tests/unit/test_complete_upload_lock.py @@ -223,6 +223,7 @@ async def test_complete_lock_idempotent_when_peer_already_finished( key, MultipartMetadata( version=2, + upload_id=upload_id, part_count=2, total_plaintext_size=len(chunk1) + len(chunk2), parts=parts, @@ -283,3 +284,20 @@ async def test_complete_lock_redis_acquire_timeout_raises(mock_redis): pass assert getattr(exc.value, "code", None) == "SlowDown" + + +@pytest.mark.asyncio +async def test_lease_is_renewed_and_registry_released(): + from fakeredis.aioredis import FakeRedis + + redis = FakeRedis() + lock = CompleteUploadLock(redis_client=redis, ttl_seconds=1) + async with lock.hold("bucket", "key", "upload"): + await asyncio.sleep(1.2) + assert await redis.exists(lock._redis_key("bucket", "key", "upload")) + assert not await redis.exists(lock._redis_key("bucket", "key", "upload")) + local = CompleteUploadLock() + async with local.hold("bucket", "key", "upload"): + assert len(local._memory_locks) == 1 + assert not local._memory_locks + await redis.aclose() diff --git a/tests/unit/test_concurrency_limit.py b/tests/unit/test_concurrency_limit.py index f7fa4ca..5c6dcb0 100644 --- a/tests/unit/test_concurrency_limit.py +++ b/tests/unit/test_concurrency_limit.py @@ -375,7 +375,7 @@ def test_estimate_memory_footprint_get(self): import s3proxy.concurrency as concurrency_module footprint = concurrency_module.estimate_memory_footprint("GET", 0) - assert footprint == concurrency_module.MAX_BUFFER_SIZE + assert footprint == 4 * concurrency_module.MAX_BUFFER_SIZE def test_estimate_memory_footprint_head(self): """HEAD should return 0 (bypass).""" diff --git a/tests/unit/test_dashboard_encryption.py b/tests/unit/test_dashboard_encryption.py index b256148..5fc2992 100644 --- a/tests/unit/test_dashboard_encryption.py +++ b/tests/unit/test_dashboard_encryption.py @@ -15,6 +15,7 @@ async def test_sidecar_present_means_encrypted(mock_s3) -> None: bucket, key = "scylla-backups", "backup/sst/me-big-Data.db" + await mock_s3.put_object(bucket, key, b"ciphertext") await save_multipart_metadata( mock_s3, bucket, diff --git a/tests/unit/test_disconnect_upload.py b/tests/unit/test_disconnect_upload.py index 538116a..569d3ee 100644 --- a/tests/unit/test_disconnect_upload.py +++ b/tests/unit/test_disconnect_upload.py @@ -1,5 +1,6 @@ """Client disconnect during streaming upload must abort and stop reading.""" +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest @@ -42,6 +43,7 @@ async def stream(self): def _handler() -> PutObjectMixin: h = PutObjectMixin.__new__(PutObjectMixin) + h.settings = SimpleNamespace(dektag_name="isec", kidtag_name="isec-kid") h.keyring = MagicMock() h.keyring.key_for.return_value = ("kid1", b"0" * 32) return h diff --git a/tests/unit/test_generation_writes.py b/tests/unit/test_generation_writes.py new file mode 100644 index 0000000..2142ab0 --- /dev/null +++ b/tests/unit/test_generation_writes.py @@ -0,0 +1,316 @@ +"""Regression coverage for generation publication and immutable part attempts.""" + +import hashlib +import xml.etree.ElementTree as ET +from unittest.mock import AsyncMock + +import pytest +from botocore.exceptions import ClientError +from fastapi import Request + +from s3proxy import crypto +from s3proxy.errors import S3Error +from s3proxy.handlers import S3ProxyHandler +from s3proxy.state import MultipartStateManager +from s3proxy.state.metadata import load_multipart_metadata +from s3proxy.streaming.chunked import decode_aws_chunked_stream + + +def request(method="PUT", body=b"", headers=None, query=""): + headers = dict(headers or {}) + headers.setdefault("content-length", str(len(body))) + + async def receive(): + return {"type": "http.request", "body": body, "more_body": False} + + return Request( + { + "type": "http", + "method": method, + "path": "/bucket/key", + "raw_path": b"/bucket/key", + "query_string": query.encode(), + "headers": [(k.encode(), v.encode()) for k, v in headers.items()], + }, + receive, + ) + + +@pytest.fixture +def proxy(settings, mock_s3): + handler = S3ProxyHandler(settings, {}, MultipartStateManager()) + handler._client = lambda _: mock_s3 + return handler + + +async def consume(response): + if hasattr(response, "body_iterator"): + try: + return b"".join([chunk async for chunk in response.body_iterator]) + finally: + if getattr(response, "cleanup", None): + await response.cleanup() + return response.body + + +async def create(proxy, credentials): + response = await proxy.handle_create_multipart_upload(request("POST"), credentials) + return next(e.text for e in ET.fromstring(response.body).iter() if e.tag.endswith("UploadId")) + + +def completion(upload, parts): + body = ( + "" + + "".join( + f"{n}{etag}" for n, etag in parts + ) + + "" + ) + return request("POST", body.encode(), query=f"uploadId={upload}") + + +async def put_part(proxy, credentials, upload, number, data, sha=None): + return await proxy.handle_upload_part( + request( + body=data, + headers={"x-amz-content-sha256": sha or "UNSIGNED-PAYLOAD"}, + query=f"uploadId={upload}&partNumber={number}", + ), + credentials, + ) + + +async def test_wrong_hash_cannot_replace_buffered_object(proxy, credentials, mock_s3): + await proxy.handle_put_object(request(body=b"before"), credentials) + before = mock_s3.objects["bucket/key"]["Body"] + with pytest.raises(S3Error): + await proxy.handle_put_object( + request( + body=b"after", + headers={"x-amz-content-sha256": hashlib.sha256(b"different").hexdigest()}, + ), + credentials, + ) + assert mock_s3.objects["bucket/key"]["Body"] == before + + +async def test_multipart_to_buffered_overwrite(proxy, credentials): + await proxy.handle_put_object( + request(body=b"old-content", headers={"x-amz-content-sha256": "UNSIGNED-PAYLOAD"}), + credentials, + ) + await proxy.handle_put_object(request(body=b"new"), credentials) + head = await proxy.handle_head_object(request("HEAD"), credentials) + assert head.headers["content-length"] == "3" + assert await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"new" + + +async def test_unknown_complete_does_not_succeed_for_existing_object(proxy, credentials): + await proxy.handle_put_object( + request(body=b"old", headers={"x-amz-content-sha256": "UNSIGNED-PAYLOAD"}), credentials + ) + with pytest.raises(S3Error): + await proxy.handle_complete_multipart_upload( + completion("never-created", [(1, "bad")]), credentials + ) + + +async def test_rejected_part_preserves_accepted_attempt(proxy, credentials): + upload = await create(proxy, credentials) + good = await put_part(proxy, credentials, upload, 1, b"before") + old = (await proxy.multipart_manager.get_upload("bucket", "key", upload)).parts[1] + with pytest.raises(S3Error): + await put_part( + proxy, credentials, upload, 1, b"after", hashlib.sha256(b"wrong").hexdigest() + ) + current = (await proxy.multipart_manager.get_upload("bucket", "key", upload)).parts[1] + assert current.staging_key == old.staging_key + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, good.headers["etag"])]), credentials + ) + assert await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"before" + + +async def test_out_of_order_parts_with_short_logical_tail(proxy, credentials): + upload = await create(proxy, credentials) + tail = await put_part(proxy, credentials, upload, 2, b"tail") + data = b"A" * (9 * 1024**2) + first = await put_part(proxy, credentials, upload, 1, data) + complete_request = completion(upload, [(1, first.headers["etag"]), (2, tail.headers["etag"])]) + response = await proxy.handle_complete_multipart_upload(complete_request, credentials) + assert response.status_code == 200 + get = await proxy.handle_get_object(request("GET"), credentials) + head = await proxy.handle_head_object(request("HEAD"), credentials) + assert get.headers["etag"] == head.headers["etag"] + assert await consume(get) == data + b"tail" + assert ( + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, first.headers["etag"]), (2, tail.headers["etag"])]), credentials + ) + ).status_code == 200 + + +async def test_metadata_failure_never_becomes_plaintext(proxy, credentials, mock_s3, monkeypatch): + await proxy.handle_put_object( + request(body=b"old", headers={"x-amz-content-sha256": "UNSIGNED-PAYLOAD"}), credentials + ) + monkeypatch.setattr( + mock_s3, + "get_object", + AsyncMock(side_effect=ClientError({"Error": {"Code": "ServiceUnavailable"}}, "GetObject")), + ) + with pytest.raises(ClientError): + await load_multipart_metadata(mock_s3, "bucket", "key") + + +async def test_sidecar_failure_does_not_publish_object(proxy, credentials, mock_s3, monkeypatch): + original = mock_s3.put_object + + async def fail_metadata(bucket, key, *args, **kwargs): + if "generations/" in key: + raise RuntimeError("injected metadata failure") + return await original(bucket, key, *args, **kwargs) + + monkeypatch.setattr(mock_s3, "put_object", fail_metadata) + with pytest.raises(S3Error): + await proxy.handle_put_object( + request(body=b"new", headers={"x-amz-content-sha256": "UNSIGNED-PAYLOAD"}), credentials + ) + assert "bucket/key" not in mock_s3.objects + + +@pytest.mark.parametrize("body", [b"3\r\nabc\r\n5\r\nxy", b"3\r\nabcXX0\r\n\r\n", b"0\r\n"]) +async def test_truncated_or_malformed_chunks_rejected(body): + with pytest.raises(S3Error): + _ = [chunk async for chunk in decode_aws_chunked_stream(request(body=body))] + + +def test_new_frame_attempts_do_not_reuse_nonce(): + dek = crypto.generate_dek() + first = crypto.encrypt_frame(b"AAAA", dek, "upload", 1, 0) + second = crypto.encrypt_frame(b"BBBB", dek, "upload", 1, 0) + assert first[:12] != second[:12] + assert crypto.decrypt(first, dek) == b"AAAA" + + +async def test_cancelled_publication_keeps_possibly_accepted_stage( + proxy, credentials, mock_s3, monkeypatch +): + import asyncio + + upload = await create(proxy, credentials) + original = proxy.multipart_manager.add_part + + async def committed_then_cancelled(*args): + await original(*args) + raise asyncio.CancelledError + + monkeypatch.setattr(proxy.multipart_manager, "add_part", committed_then_cancelled) + with pytest.raises(asyncio.CancelledError): + await put_part(proxy, credentials, upload, 1, b"accepted") + state = await proxy.multipart_manager.get_upload("bucket", "key", upload) + assert f"bucket/{state.parts[1].staging_key}" in mock_s3.objects + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, state.parts[1].md5)]), credentials + ) + assert await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"accepted" + + +async def test_completed_copy_does_not_prove_destination_upload_identity(proxy, credentials): + upload = await create(proxy, credentials) + part = await put_part(proxy, credentials, upload, 1, b"source") + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, part.headers["etag"])]), credentials + ) + copy = request(headers={"x-amz-copy-source": "/bucket/key"}) + copy.scope["path"] = "/bucket/destination" + await proxy.handle_copy_object(copy, credentials) + complete = completion(upload, [(1, part.headers["etag"])]) + complete.scope["path"] = "/bucket/destination" + with pytest.raises(S3Error): + await proxy.handle_complete_multipart_upload(complete, credentials) + + +async def test_abort_removes_accepted_and_replaced_attempts(proxy, credentials, mock_s3): + upload = await create(proxy, credentials) + await put_part(proxy, credentials, upload, 1, b"first") + await put_part(proxy, credentials, upload, 1, b"replacement") + assert sum("/attempts/" in k for k in mock_s3.objects) == 2 + await proxy.handle_abort_multipart_upload( + request("DELETE", query=f"uploadId={upload}"), credentials + ) + assert not any("/attempts/" in k for k in mock_s3.objects) + + +async def test_legacy_active_copy_is_rejected(proxy, credentials): + await proxy.multipart_manager.create_upload("bucket", "key", "legacy", crypto.generate_dek()) + with pytest.raises(S3Error, match="Legacy in-flight"): + await proxy.handle_upload_part_copy( + request(query="uploadId=legacy&partNumber=1"), credentials + ) + + +async def test_failed_complete_keeps_upload_retryable(proxy, credentials, monkeypatch): + upload = await create(proxy, credentials) + part = await put_part(proxy, credentials, upload, 1, b"retry-me") + import s3proxy.handlers.multipart.staged as staged + + save = staged.save_multipart_metadata + with monkeypatch.context() as patch: + patch.setattr( + staged, "save_multipart_metadata", AsyncMock(side_effect=RuntimeError("failed")) + ) + with pytest.raises(RuntimeError): + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, part.headers["etag"])]), credentials + ) + assert staged.save_multipart_metadata is save + assert await proxy.multipart_manager.get_upload("bucket", "key", upload) is not None + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, part.headers["etag"])]), credentials + ) + assert await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"retry-me" + + +@pytest.mark.parametrize("copy_first", [True, False]) +async def test_key_selection_cannot_change_after_first_writer( + proxy, credentials, mock_s3, copy_first +): + source = request(body=b"source") + source.scope["path"] = "/bucket/source" + await proxy.handle_put_object(source, credentials) + upload = await create(proxy, credentials) + state = await proxy.multipart_manager.get_upload("bucket", "key", upload) + original_dek = state.dek + if not copy_first: + await put_part(proxy, credentials, upload, 1, b"initial") + copy = request( + headers={"x-amz-copy-source": "/bucket/source"}, query=f"uploadId={upload}&partNumber=1" + ) + response = await proxy.handle_upload_part_copy(copy, credentials) + await consume(response) + state = await proxy.multipart_manager.get_upload("bucket", "key", upload) + chosen = state.dek + if not copy_first: + assert chosen == original_dek + else: + assert chosen != original_dek + assert ( + mock_s3.objects[f"bucket/{state.parts[1].staging_key}"]["Body"] + == mock_s3.objects["bucket/source"]["Body"] + ) + replacement = await put_part(proxy, credentials, upload, 1, b"replacement") + assert (await proxy.multipart_manager.get_upload("bucket", "key", upload)).dek == chosen + await proxy.handle_complete_multipart_upload( + completion(upload, [(1, replacement.headers["etag"])]), credentials + ) + assert ( + await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"replacement" + ) + + +async def test_empty_streaming_put_roundtrip(proxy, credentials): + await proxy.handle_put_object( + request(body=b"", headers={"x-amz-content-sha256": "UNSIGNED-PAYLOAD"}), credentials + ) + assert await consume(await proxy.handle_get_object(request("GET"), credentials)) == b"" diff --git a/tests/unit/test_list_multipart_plaintext_size.py b/tests/unit/test_list_multipart_plaintext_size.py index 981fa58..c70e210 100644 --- a/tests/unit/test_list_multipart_plaintext_size.py +++ b/tests/unit/test_list_multipart_plaintext_size.py @@ -33,6 +33,7 @@ ) from s3proxy.state.attr_cache import PlaintextAttrCache from s3proxy.state.metadata import _internal_meta_key, persist_upload_state +from tests.conftest import MockS3Response as _Body INTERNAL_PREFIX = ".s3proxy-internal/" @@ -47,15 +48,8 @@ def fresh_cache(monkeypatch): plaintext_attr_cache.clear() -class _Body: - def __init__(self, data: bytes) -> None: - self._data = data - - async def read(self) -> bytes: - return self._data - - class FakeHandler: + _resolve_object = BucketHandlerMixin._resolve_object _process_list_objects = BucketHandlerMixin._process_list_objects _list_entry = staticmethod(BucketHandlerMixin._list_entry) @@ -90,7 +84,9 @@ async def get_object(self, bucket, key): if key in self.sidecars: encoded = encode_multipart_metadata(self.sidecars[key]) return {"Body": _Body(encoded.encode())} - raise KeyError(key) + from botocore.exceptions import ClientError + + raise ClientError({"Error": {"Code": "NoSuchKey"}}, "GetObject") def _obj(key, size, etag="backend-etag"): @@ -177,17 +173,33 @@ def test_cache_scoped_to_backend_etag(): assert client.head_calls == head_calls + 1 -def test_failed_head_falls_back_and_does_not_cache(fresh_cache): +def test_failed_head_propagates_and_does_not_cache(fresh_cache): handler = FakeHandler() client = FakeClient(fail_head_key="broken.db") + with pytest.raises(RuntimeError, match="backend HEAD failed"): + asyncio.run(handler._process_list_objects(client, "bucket", [_obj("broken.db", size=555)])) + assert len(fresh_cache) == 0 - result = asyncio.run( - handler._process_list_objects(client, "bucket", [_obj("broken.db", size=555)]) - ) - assert result[0]["size"] == 555 - assert result[0]["etag"] == "backend-etag" - assert len(fresh_cache) == 0 +@pytest.mark.asyncio +async def test_concurrent_listing_coalesces_metadata_lookups(): + handler = FakeHandler() + client = FakeClient(metadata={"small.txt": {"plaintext-size": "42", "client-etag": "abc"}}) + original = client.head_object + + async def delayed(*args): + await asyncio.sleep(0.01) + return await original(*args) + + client.head_object = delayed + results = await asyncio.gather( + *[ + handler._process_list_objects(client, "bucket", [_obj("small.txt", size=100)]) + for _ in range(8) + ] + ) + assert all(r[0]["size"] == 42 for r in results) + assert client.head_calls == 1 def test_cache_evicts_least_recently_used(): diff --git a/tests/unit/test_list_objects_parallel.py b/tests/unit/test_list_objects_parallel.py index 2160633..185a1d3 100644 --- a/tests/unit/test_list_objects_parallel.py +++ b/tests/unit/test_list_objects_parallel.py @@ -8,12 +8,15 @@ import asyncio import datetime as dt +from botocore.exceptions import ClientError + from s3proxy.handlers.buckets import LIST_HEAD_CONCURRENCY, BucketHandlerMixin INTERNAL_PREFIX = "s3proxy-internal/" class FakeHandler: + _resolve_object = BucketHandlerMixin._resolve_object _process_list_objects = BucketHandlerMixin._process_list_objects _list_entry = staticmethod(BucketHandlerMixin._list_entry) @@ -42,7 +45,7 @@ async def head_object(self, bucket, key): try: await asyncio.sleep(0.02) # simulate backend round-trip if key == self.fail_key: - raise RuntimeError("backend HEAD failed") + raise ClientError({"Error": {"Code": "NoSuchKey"}}, "HeadObject") return {"Metadata": {"plaintext-size": "111", "client-etag": f"etag-{key}"}} finally: self.handler.inflight -= 1 diff --git a/tests/unit/test_list_objects_v1_via_v2.py b/tests/unit/test_list_objects_v1_via_v2.py index 778dec2..0ab1c2d 100644 --- a/tests/unit/test_list_objects_v1_via_v2.py +++ b/tests/unit/test_list_objects_v1_via_v2.py @@ -54,7 +54,7 @@ async def list_objects_v2( return self.resp async def head_object(self, bucket, key): - return {"Metadata": {}} + return {"Metadata": {"s3proxy-format": "plain-v3"}} class _Handler(BucketHandlerMixin): diff --git a/tests/unit/test_memory_concurrency.py b/tests/unit/test_memory_concurrency.py index 2997c72..b6c3bd5 100644 --- a/tests/unit/test_memory_concurrency.py +++ b/tests/unit/test_memory_concurrency.py @@ -77,7 +77,7 @@ def test_get_uses_fixed_buffer(self): import s3proxy.concurrency as concurrency_module footprint = concurrency_module.estimate_memory_footprint("GET", 0) - assert footprint == concurrency_module.MAX_BUFFER_SIZE + assert footprint == 4 * concurrency_module.MAX_BUFFER_SIZE def test_head_delete_bypass(self): """HEAD and DELETE reserve 0 (no buffering, bypass limit).""" diff --git a/tests/unit/test_request_handler_body.py b/tests/unit/test_request_handler_body.py index 0c045fe..22ffcf7 100644 --- a/tests/unit/test_request_handler_body.py +++ b/tests/unit/test_request_handler_body.py @@ -132,6 +132,11 @@ async def test_small_put_without_header_loads_body_once(): payload = b"x" * (4 * MB) request = _make_request(content_length=len(payload)) request.body = AsyncMock(return_value=payload) + + async def chunks(): + yield payload + + request.stream = MagicMock(side_effect=chunks) verifier = MagicMock() verifier.verify = MagicMock(return_value=(True, MagicMock(), "")) @@ -139,7 +144,8 @@ async def test_small_put_without_header_loads_body_once(): dispatcher_cls.return_value.dispatch = AsyncMock(return_value=None) await _handle_proxy_request_impl(request, MagicMock(), verifier) - request.body.assert_awaited_once() + request.stream.assert_called_once() + request.body.assert_not_awaited() assert request.state.s3proxy_preloaded_body == payload verifier.verify.assert_called_once() diff --git a/tests/unit/test_scylla_deferred_put_memory.py b/tests/unit/test_scylla_deferred_put_memory.py index 77841bd..b1794b9 100644 --- a/tests/unit/test_scylla_deferred_put_memory.py +++ b/tests/unit/test_scylla_deferred_put_memory.py @@ -10,6 +10,7 @@ import hashlib import tracemalloc +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest @@ -61,6 +62,7 @@ async def stream(self): def _handler() -> PutObjectMixin: h = PutObjectMixin.__new__(PutObjectMixin) + h.settings = SimpleNamespace(dektag_name="isec", kidtag_name="isec-kid") h.keyring = MagicMock() h.keyring.key_for.return_value = ("kid1", b"0" * 32) return h diff --git a/tests/unit/test_source_read_retry.py b/tests/unit/test_source_read_retry.py index 341578b..bfd4049 100644 --- a/tests/unit/test_source_read_retry.py +++ b/tests/unit/test_source_read_retry.py @@ -322,7 +322,7 @@ async def test_passthrough_copy_survives_transient_backend_failures( upload_id = resp_create["UploadId"] await manager.create_upload(BUCKET, "sst/big.db.snap", upload_id, crypto.generate_dek(), kid) - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/big.db.snap", f"/{BUCKET}/sst/big.db", diff --git a/tests/unit/test_streaming_resources.py b/tests/unit/test_streaming_resources.py new file mode 100644 index 0000000..6b0d5cb --- /dev/null +++ b/tests/unit/test_streaming_resources.py @@ -0,0 +1,167 @@ +"""Failure-path and backend round-trip regressions for the shared services.""" + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest +from cryptography.exceptions import InvalidTag +from starlette.requests import ClientDisconnect + +from s3proxy import crypto +from s3proxy.client import SigV4Verifier +from s3proxy.client.pool import S3ClientPool +from s3proxy.errors import S3Error +from s3proxy.state import MultipartMetadata, PartMetadata +from s3proxy.streaming.authenticated import decrypt_to_file +from s3proxy.streaming.chunked import decode_aws_chunked_stream +from s3proxy.streaming.frames import plaintext_frames +from s3proxy.streaming.response import OwnedStreamingResponse +from tests.conftest import MockS3Response +from tests.unit.test_generation_writes import request + + +async def test_pool_reuses_and_isolates_credentials(settings, credentials, monkeypatch): + instances = [] + + class Client: + def __init__(self, *args): + self.closed = False + instances.append(self) + + async def __aenter__(self): + return self + + async def __aexit__(self, *args): + self.closed = True + + monkeypatch.setattr("s3proxy.client.pool.S3Client", Client) + pool = S3ClientPool(settings, max_clients=1) + async with pool.acquire(credentials) as first: + async with pool.acquire(credentials) as second: + assert first is second + other = SimpleNamespace(access_key="other", secret_key="secret", region="us-east-1") + with pytest.raises(S3Error): + async with pool.acquire(other): + pass + closing = asyncio.create_task(pool.close()) + await asyncio.sleep(0) + assert not closing.done() + assert not first.closed + await asyncio.wait_for(closing, 1) + assert first.closed and len(instances) == 1 + + +async def test_response_closes_resources_when_headers_fail(): + cleanup = AsyncMock() + failed = [] + + async def body(): + pytest.fail("The body must not start when headers fail") + yield b"" + + async def send(message): + raise OSError("disconnected") + + response = OwnedStreamingResponse(body(), cleanup=cleanup, on_error=lambda: failed.append(True)) + with pytest.raises(ClientDisconnect): + await response({"type": "http", "asgi": {"spec_version": "2.4"}}, AsyncMock(), send) + cleanup.assert_awaited_once() + assert failed == [True] + + +async def test_contiguous_frames_use_one_backend_request(mock_s3): + dek = crypto.generate_dek() + plaintext = [bytes([n]) * 32768 for n in range(8)] + ciphertext = [crypto.encrypt(p, dek) for p in plaintext] + await mock_s3.put_object("bucket", "key", b"".join(ciphertext)) + meta = MultipartMetadata( + parts=[ + PartMetadata(i, len(p), len(c), "") + for i, (p, c) in enumerate(zip(plaintext, ciphertext, strict=True), 1) + ] + ) + result = b"".join([p async for p in plaintext_frames(mock_s3, "bucket", "key", meta, dek)]) + assert result == b"".join(plaintext) + assert len([c for c in mock_s3.call_history if c[0] == "get_object"]) == 1 + + +async def test_truncated_range_resumes_at_unpublished_frame(mock_s3, monkeypatch): + dek = crypto.generate_dek() + data = [b"a" * 1024, b"b" * 1024] + seals = [crypto.encrypt(p, dek) for p in data] + await mock_s3.put_object("bucket", "key", b"".join(seals)) + original = mock_s3.get_object + calls = [] + + async def truncated(bucket, key, byte_range, **kwargs): + calls.append(byte_range) + response = await original(bucket, key, byte_range, **kwargs) + if len(calls) == 1: + response["Body"] = MockS3Response(seals[0] + seals[1][:5]) + return response + + monkeypatch.setattr(mock_s3, "get_object", truncated) + monkeypatch.setattr("s3proxy.handlers.base.SOURCE_READ_BACKOFF_SEC", 0) + meta = MultipartMetadata( + parts=[ + PartMetadata(i, len(p), len(c), "") + for i, (p, c) in enumerate(zip(data, seals, strict=True), 1) + ] + ) + assert b"".join( + [p async for p in plaintext_frames(mock_s3, "bucket", "key", meta, dek)] + ) == b"".join(data) + assert calls[1].startswith(f"bytes={len(seals[0])}-") + + +async def test_large_legacy_seal_is_authenticated_before_plaintext(mock_s3): + dek = crypto.generate_dek() + data = b"legacy" * (2 * 1024**2) + ciphertext = crypto.encrypt(data, dek) + await mock_s3.put_object("bucket", "key", ciphertext) + meta = MultipartMetadata(parts=[PartMetadata(1, len(data), len(ciphertext), "")]) + assert ( + b"".join([p async for p in plaintext_frames(mock_s3, "bucket", "key", meta, dek)]) == data + ) + with pytest.raises(InvalidTag): + await decrypt_to_file(MockS3Response(ciphertext[:-1] + bytes([ciphertext[-1] ^ 1])), dek) + + +# Public AWS SigV4 test vectors, including the signature on the terminal chunk: +# https://docs.aws.amazon.com/AmazonS3/latest/developerguide/sigv4-streaming.html +@pytest.mark.parametrize("tamper", [False, True]) +async def test_aws_published_chunk_signature_vector(credentials, tamper): + signatures = [ + "ad80c730a21e5b8d04586a2213dd63b9a0e99e0e2307b0ade35a65485a288648", + "0055627c9e194cb4542bae2aa5492e3c1575bbb81b612b7d234b86a503ef5497", + "b6c6ea8a5354eaf15b3cb7646744f4275b71ea724fed81ceb9323e279d449df9", + ] + body = b"".join( + f"{n:x};chunk-signature={sig}\r\n".encode() + b"a" * n + b"\r\n" + for n, sig in zip([65536, 1024, 0], signatures, strict=True) + ) + if tamper: + body = body.replace(b"aaaa", b"baaa", 1) + req = request( + body=body, + headers={ + "x-amz-content-sha256": "STREAMING-AWS4-HMAC-SHA256-PAYLOAD", + "x-amz-date": "20130524T000000Z", + "x-amz-decoded-content-length": "66560", + "authorization": "AWS4-HMAC-SHA256 " + "Credential=AKIAIOSFODNN7EXAMPLE/20130524/us-east-1/s3/aws4_request," + "SignedHeaders=host;x-amz-date," + "Signature=4f232c4386841ef735655705268965c44a0e4690baa4adea153f7db9fa80a0a9", + }, + ) + req.scope["app"] = SimpleNamespace( + state=SimpleNamespace( + verifier=SigV4Verifier({credentials.access_key: credentials.secret_key}) + ) + ) + if tamper: + with pytest.raises(S3Error): + _ = [part async for part in decode_aws_chunked_stream(req)] + else: + assert b"".join([part async for part in decode_aws_chunked_stream(req)]) == b"a" * 66560 diff --git a/tests/unit/test_upload_part_copy_keepalive.py b/tests/unit/test_upload_part_copy_keepalive.py index 509ea07..948360f 100644 --- a/tests/unit/test_upload_part_copy_keepalive.py +++ b/tests/unit/test_upload_part_copy_keepalive.py @@ -134,7 +134,7 @@ async def slow_copy(*args, **kwargs): mock_s3.upload_part_copy = slow_copy - resp = await handler.handle_upload_part_copy(_copy_part_request(upload_id), credentials) + resp = await handler._copy_part_impl(_copy_part_request(upload_id), credentials) assert resp.status_code == 200 stream = resp.body_iterator @@ -173,7 +173,7 @@ async def failing_copy(*args, **kwargs): mock_s3.upload_part_copy = failing_copy - resp = await handler.handle_upload_part_copy(_copy_part_request(upload_id), credentials) + resp = await handler._copy_part_impl(_copy_part_request(upload_id), credentials) body = b"".join([c async for c in resp.body_iterator]) assert resp.status_code == 200 @@ -210,7 +210,7 @@ async def hanging_copy(*args, **kwargs): mock_s3.upload_part_copy = hanging_copy - resp = await handler.handle_upload_part_copy(_copy_part_request(upload_id), credentials) + resp = await handler._copy_part_impl(_copy_part_request(upload_id), credentials) stream = resp.body_iterator assert await anext(stream) == b" " await asyncio.wait_for(started.wait(), 1) @@ -249,7 +249,7 @@ async def tracking_copy(*args, **kwargs): mock_s3.upload_part_copy = tracking_copy - resp = await handler.handle_upload_part_copy(_copy_part_request(upload_id), credentials) + resp = await handler._copy_part_impl(_copy_part_request(upload_id), credentials) body = b"".join([c async for c in resp.body_iterator]) assert ET.fromstring(body).tag.endswith("CopyPartResult") @@ -291,7 +291,7 @@ async def tracked_get(*args, **kwargs): mock_s3.upload_part_copy = slow_copy mock_s3.get_object = tracked_get - resp = await handler.handle_upload_part_copy(_copy_part_request(upload_id), credentials) + resp = await handler._copy_part_impl(_copy_part_request(upload_id), credentials) b"".join([c async for c in resp.body_iterator]) assert first_md5_read is not None and last_copy_done is not None @@ -315,7 +315,7 @@ async def test_hybrid_tail_roundtrip_with_parallel_segments( range_end = 4 * frame_size + frame_size // 2 - 1 # ends mid 5th frame assert range_end + 1 > crypto.STREAMING_THRESHOLD - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request(upload_id, copy_source_range=f"bytes=0-{range_end}"), credentials, ) diff --git a/tests/unit/test_upload_part_copy_passthrough.py b/tests/unit/test_upload_part_copy_passthrough.py index 7253b75..d67268c 100644 --- a/tests/unit/test_upload_part_copy_passthrough.py +++ b/tests/unit/test_upload_part_copy_passthrough.py @@ -123,7 +123,7 @@ async def test_multipart_encrypted_upload_part_copy_is_server_side_passthrough( await manager.create_upload(BUCKET, "sst/big.db.snap", upload_id, dst_dek, kid) mark = len(mock_s3.call_history) - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/big.db.snap", f"/{BUCKET}/sst/big.db", @@ -205,7 +205,7 @@ async def test_upload_part_copy_passthrough_roundtrips_via_get( ) upload_id = _extract_upload_id(create_resp.body) - copy_resp = await handler.handle_upload_part_copy( + copy_resp = await handler._copy_part_impl( _copy_part_request(f"/{BUCKET}/sst/dest.db", f"/{BUCKET}/sst/source.db", upload_id), credentials, ) @@ -271,7 +271,7 @@ async def test_range_copy_still_reencrypts(mock_s3, settings, manager, credentia req.headers["x-amz-copy-source-range"] = f"bytes=0-{crypto.MAX_BUFFER_SIZE - 1}" mark = len(mock_s3.call_history) - await _read(await handler.handle_upload_part_copy(req, credentials)) + await _read(await handler._copy_part_impl(req, credentials)) during = mock_s3.call_history[mark:] assert not any(c[0] == "upload_part_copy" for c in during) @@ -527,7 +527,7 @@ async def test_scylla_prod_shape_range_smaller_than_metadata_uses_passthrough( scylla_range = f"bytes=0-{range_end}" mark = len(mock_s3.call_history) - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/big-Data.db.sm_manifest", f"/{BUCKET}/sst/big-Data.db", @@ -667,7 +667,7 @@ async def test_two_part_hybrid_defer_tail_completes_fast( ) mark = len(mock_s3.call_history) - resp1 = await handler.handle_upload_part_copy( + resp1 = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/small-Data.db.sm_manifest", f"/{BUCKET}/sst/small-Data.db", @@ -692,7 +692,7 @@ async def test_two_part_hybrid_defer_tail_completes_fast( part2_start = part1_range_end + 1 mark2 = len(mock_s3.call_history) - resp2 = await handler.handle_upload_part_copy( + resp2 = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/small-Data.db.sm_manifest", f"/{BUCKET}/sst/small-Data.db", @@ -794,7 +794,7 @@ async def test_two_part_defer_tail_total_plaintext_not_double_counted( (1, f"bytes=0-{part1_range_end}"), (2, f"bytes={part1_range_end + 1}-{total - 1}"), ): - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/{dest_key}", f"/{BUCKET}/sst/big-Data.db", @@ -852,7 +852,7 @@ async def test_defer_tail_flushed_on_complete_keeps_part_accounting( upload_id = resp_create["UploadId"] await handler.multipart_manager.create_upload(BUCKET, dest_key, upload_id, src_dek, kid) - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/{dest_key}", f"/{BUCKET}/sst/solo-Data.db", @@ -1029,7 +1029,7 @@ async def test_scylla_manifest_full_range_uses_passthrough_not_streaming( full_range = f"bytes=0-{len(src_plaintext) - 1}" mark = len(mock_s3.call_history) - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/big-Data.db.sm_manifest", f"/{BUCKET}/sst/big-Data.db", @@ -1089,7 +1089,7 @@ async def blocked_streaming(*args, **kwargs): handler._streaming_copy_part = blocked_streaming # type: ignore[method-assign] async def run_passthrough(): - resp = await handler.handle_upload_part_copy( + resp = await handler._copy_part_impl( _copy_part_request( f"/{BUCKET}/sst/big-Data.db.sm_manifest", f"/{BUCKET}/sst/big-Data.db", @@ -1155,7 +1155,9 @@ async def test_single_segment_passthrough_complete_presents_backend_etag( import xml.etree.ElementTree as ET client_etag = ET.fromstring(await _read(copy_resp)).find("{*}ETag").text.strip('"') - backend_etag = mock_s3.multipart_uploads[upload_id]["Parts"][1]["ETag"] + state = await handler.multipart_manager.get_upload(BUCKET, "single/dst.bin", upload_id) + backend_etag = mock_s3.objects[f"{BUCKET}/{state.parts[1].staging_key}"]["ETag"].strip('"') + assert not mock_s3.multipart_uploads[upload_id]["Parts"] assert client_etag == hashlib.md5(plaintext, usedforsecurity=False).hexdigest() assert client_etag != backend_etag