S3 client: private-CA TLS, a 5 MiB part minimum, and a fresh signature per attempt - #149
Merged
Merged
Conversation
S3Client::Exchange built x-amz-date and the SigV4 Authorization once,
before the retry loop, and replayed both on every retry. SigV4 binds the
signature to that date and S3 refuses a request dated more than 15
minutes off, so a slow first attempt (up to read_timeout_s each, plus
backoff) aged every retry behind it. Each attempt now stamps a fresh
x-amz-date and signs the request itself; the URL and the other signed
headers are still built once.
Evidence: test_every_attempt_is_signed_afresh makes the fake S3's first
attempt outlive a second before answering 500. Before the fix both
attempts carried the same date ('20260924T154415Z' > '20260924T154415Z'
failed); after it the retry carries a later date and a different
signature, and both pass the server's botocore re-signing check.
test_native_s3_client.py, test_native_s3_sign.py (the conformance_sign
byte-parity suites) and test_native_uploader.py: 49 passed.
S3 refuses every part of a multipart upload but the last when it is under 5 MiB, and it says so only at CompleteMultipartUpload, after every part was sent. The Python store refuses such a part size when it is configured (s3.py _MIN_MULTIPART_BYTES); the native client did not. S3Client::ValidateConfig now refuses multipart_chunk_bytes under kMinMultipartPartBytes, and an invalid config makes the client refuse every request before it goes out, as the https-with-insecure-flag refusal already did (that check moves into the same function, message kept). ValidateConfig is static so callers with an error channel of their own can check a config at construction. The multipart round trip used 1 MiB parts, which real S3 refuses. It now sends a 12 MiB payload in 5 MiB parts (5 + 5 + 2 MiB tail) rather than adding a test-only override: the override would be a bypass in the production client for a limit every real store enforces, and 12 MiB over localhost costs well under a second. The fake S3 now answers EntityTooSmall for a short non-final part, as S3 does, so a test that slips below the minimum fails the way production would. Evidence: test_multipart_part_under_5_mib_is_refused_before_any_request failed first with "CompleteMultipartUpload failed: HTTP 400: EntityTooSmall" -- all three 1 MiB parts already sent. It now gets the client's refusal with no request made, and exactly 5 MiB is accepted. test_native_s3_client.py + test_native_s3_sign.py + test_native_uploader.py: 50 passed.
An object store behind a private CA (an internal Garage, say) could not be reached over https: the client trusted only libcurl's default store, and allow_insecure_http deliberately never downgrades TLS. S3Config gains ca_file (a PEM bundle, CURLOPT_CAINFO) and ca_path (an OpenSSL-hashed directory, CURLOPT_CAPATH). A CA adds trust and never relaxes the check: https now states CURLOPT_SSL_VERIFYPEER=1 and VERIFYHOST=2 explicitly instead of leaning on the defaults. ValidateConfig refuses a CA on a plain-http endpoint (it would read as "this is TLS" while credentials go in the clear) and names a missing CA file or directory up front, instead of libcurl's "problem with the SSL CA cert" at the first request. The store driver takes ca_file / ca_path. The plan put the HTTPS coverage in A5's MinIO job. MinIO has since left CI, so the coverage is a CPU test instead: the same signature-verifying fake S3 behind TLS, with a server certificate from a CA the test mints with the openssl CLI (own config files -- the system openssl.cnf's v3_ca duplicates basicConstraints, and OpenSSL then rejects the CA). Evidence, each red against the previous binary first: - round trip over https with ca_file and with ca_path: "SSL peer certificate or SSH remote key was not OK" before, byte-equal after; - a 17 x 4 MiB native-sink pack (over the 64 MiB threshold) through the uploader's upload_pending over TLS: refused without the CA (pack stays staged, no request lands), then uploaded as 16 MiB parts with it and read back byte-equal to the staged file; - CA options on http:// and a missing CA path are refused before any request. test_native_s3_client.py + test_native_s3_sign.py + test_native_uploader.py: 56 passed.
NativeCaptureStorageConfig gains s3_ca_file and s3_ca_path (empty: the system trust store), passed to _dmi_native_store, whose s3_config() reads them into the client's config for both the StorageService and the CaptureReader. The config refuses a CA on an http:// endpoint and a non-str value, as the native client does. The bindings now run S3Client::ValidateConfig when the service or reader is built, so a missing CA file, a CA on http:// or a sub-5 MiB part raises ValueError at construction rather than failing every later request. Evidence, red first: - wiring (CPU, native modules faked): the CA fields reach both native constructors, a CA on http:// and a non-str CA are refused -- failed with "unexpected keyword argument 's3_ca_file'"; - the real _dmi_native_store names a missing CA file or directory when a CaptureReader or StorageService is built, with no store or catalog contacted; - live (clickhouse and manual): a 17 x 4 MiB pack (over the 64 MiB multipart threshold) goes spool -> StorageService -> TLS fake S3 with a private CA -> ClickHouse catalog, and NativeCaptureReader hydrates all 17 captures byte-equal over the same TLS; a reader without the CA is refused on the certificate. Failed before with the same TypeError. tests/test_native_capture_storage_wiring.py: 44 passed; the live test: 1 passed.
Every TLS test connected to 127.0.0.1 with a certificate naming 127.0.0.1, so turning host-name verification off (VERIFYHOST 0) left the suite green. A second leaf from the same private CA, for other.example, is now served on 127.0.0.1: with the CA trusted, the mismatch alone must refuse the request, with one attempt and none landing. With VERIFYHOST set to 0 the test fails; restored, it passes.
conformance_store read ca_file/ca_path, but conformance_catalog built its own S3Config for select, hydrate, summarize_core and index without them, so a native-binaries-only run could not reach an https store with a private CA. All four now read both options. S3 client, uploader and catalog driver CPU suites: 124 passed; native reader parity live suite: 47 passed.
zaoxing
force-pushed
the
feat/s3-ca-and-part-size
branch
from
September 24, 2026 22:59
18d6347 to
f44542d
Compare
zaoxing
added a commit
that referenced
this pull request
Sep 26, 2026
Conflicts were side-by-side additions: helpers in native_capture.py and new tests appended at the ends of the storage wiring and live test files; both sides kept. One semantic fix: this branch required clickhouse_request_timeout_s to be at least twice a fixed 5 s _PUBLISH_TIMEOUT_S, written when the config did not expose the publish timeout. #150 made publish_timeout_s a config field, so the rule is now twice the configured publish_timeout_s, checked after the lease fields are validated; the integration doc says so, and a wiring test pins it (a 7 s publish cap needs a 14 s request timeout).
zaoxing
added a commit
that referenced
this pull request
Sep 26, 2026
One conflict, in src/dmi/storage/native_capture.py: this branch added NativeSinkConfig where main added the connection and lease validation helpers; both kept, helpers first. On the merged tree: pytest -m cpu 2520 passed; the capture storage, catalog lease, capture chain and reader parity live suites 126 passed. The ring, sink and engine code is identical to 52b9627, which passed the GPU suites (test_ring_engine 174/174, test_record_failure_policy_gpu 8/8).
claude Bot
pushed a commit
that referenced
this pull request
Sep 26, 2026
Brings in #149-#152 and #139 (two-phase native search page). One conflict, native/csrc/catalog/reader.cpp search(): #139 split the page into an inner key-only query and an outer argMax query over the same filters, while this branch moved snapshot membership out of `clauses` into the FROM clause (snapshot(), the join that supplies member_version). Resolved by running BOTH phases over snapshot() with the same caller `clauses` (possibly empty), so the inner LIMIT only counts member keys (#139's walk-ends-early guard) and the outer argMax still ranks on (member_version, store_id, pack_id, index_version). The design doc's two-phase paragraph now says both queries read the same snapshot join. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019oKb8SWwCwPRAtxuWLQSXt
zaoxing
added a commit
that referenced
this pull request
Sep 28, 2026
#150 was squash-merged as 7419fd0, and main then took #151, #152 and #139. The specs cited #150's head c0361d7; they now cite main at 71be2af. - storage_service.cpp moved up two lines (#151 builds the ClickHouse client from one ClickHouseConnection), native_capture.py moved with #149/#151/#152, and deciding_read() is now clickhouse_client.cpp:374. - #151 also made execute() retry a read after a transient failure, up to max_attempts (3 by default), and never a write that may have reached the server. The README's O1 caveat, its Limitations entry and LIMITS 3 in LeaseLifecycle.tla now say a lease request's reads can take up to three request timeouts, and RenewIfDue says the quarantining exception is the first to outlast those retries. No modelled outcome changes. - Refs that missed the code they describe, in files main did not change: the O1a quote is storage_service.h:233-234, not storage_service.cpp; the renewal in publish_snapshot is catalog_writer.cpp:490 and :579; publish_snapshot ends at :669; the config check with the quorum rule is :148-169; the chunk loop is :531; the watermark read-back is :608-628 (:611-627 for its refusal); the version allocator's statement lines; reject_live's comparison is lease_coordinator.cpp:222; and the start wait's knob checks are native_capture.py:351-354. - The README says what 204a8d2 is now that #150's branch is squashed. Comment and prose changes only; every verdict is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Plan milestone B7a. The native S3 client gains what a production object store behind TLS needs, plus two correctness fixes.
Changes
x-amz-dateand signature on every retry, so a retry after a slow first attempt carried a stale date. The URL and fixed headers are still built once; each attempt now stamps its own date and signature from the same header map it sends.EntityTooSmallis now checked as config validation, the way the Python reference does it. That way a misconfigured client fails at once, not only when a payload crosses the multipart threshold. The in-repo fake S3 now answersEntityTooSmallfor a short non-final part too, as real S3 does.S3Config.ca_file/ca_path(CURLOPT_CAINFO/CAPATH); https setsVERIFYPEER=1andVERIFYHOST=2explicitly.http://, and a missing CA file or directory, which is named in the error.NativeCaptureStorageConfig(s3_ca_file,s3_ca_path) and the bindings, for both the storage service and the reader. The bindings raiseValueErrorat construction.Deviation from the plan: HTTPS isn't tested against MinIO, which was removed from CI. It runs against the in-repo signature-verifying fake S3 behind TLS, with a CA minted in the test (openssl CLI, its own config files, verified with
openssl verify).Evidence
Each behaviour was red first:
EntityTooSmallcame back after every part had been sent. After, the client refuses with no request made.ca_fileandca_path: a certificate error before, byte-equal after. Without the CA, and with a certificate for another host name, the request is refused with no retry and no request landing.NativeCaptureReader, byte-equal. A reader without the CA is refused.conformance_sign) is unchanged.An independent review judged it ship; its two minor findings are fixed here.