Repository navigation
Conversation
Collaborator
Author
|
Closing: every hunk documents a finding the audit already refuted, and the tests restate the implementation; no behaviour change worth carrying. |
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.
Split out of #1262.
Audit findings worked to a conclusion and refuted; the evidence that decides them is in the diff as tests and doc comments rather than nothing, so what was checked stays checked. No behaviour change: every hunk is a comment, a
gateway.tomlcomment, or a test.data_timeout_enabled = falseremoves the idle window, not a per-operation timerThe shipped comment read like a relaxation ("might impact performance").
ProxyConfig::idle_timeout()returnsNonewhen it is off: the buffered bridge falls back tocopy_bidirectionaland the spliced and kTLS relays drop their watchdog, sotimeouts.total(5h) is the only bound left on a connection that transfers nothing. The comment ingateway.tomland onTimeouts::data_timeout_enablednow says so;disabling_data_timeouts_removes_the_idle_window_entirelypins it.TLS 1.3 being off by default is deliberate, and the ticketer is not dead machinery
a5230e601e("install rustls session ticketer for TLS 1.3 resumption") measured both arms on a 4-core gateway and states the numbers: TLS 1.3 reconnect CPS 6.2k → 9.6k with the ticketer, TLS 1.2 ~20k — 1.2 is more than twice the connection rate because it resumes from rustls' session-ID cache, which 1.3 cannot use. That commit also says "Default TLS version is unchanged", so the 1.2-only default was knowingly carried. And rustls' 1.2 is already ECDHE-only, AEAD-only, no renegotiation, no compression — what 1.3 adds here is handshake privacy and latency, not data confidentiality. The ticketer exists for operators who settls_versions = ["1.2","1.3"].No behaviour change: flipping the default would alter every deployment's negotiated version and its untested interaction with
enable_secret_extraction/ kTLS. The measurement and the trade-off are now recorded attls_versionsingateway.tomland at the ticketer.Config::uuid()cannot regenerate identity on an unwritable data dirIt is unreachable.
ProxyInner::newcallsKvStore::new(..., &config.sync.data_dir, ...)beforeconfig.uuid(), andKvStore::newroutes any io error that is notUnexpectedEof/InvalidDatathroughis_storage_failureinto a hardErr("cannot open the WaveKV data dir …; refusing to start").PermissionDeniedis one of those, so the gateway exits before a uuid is ever generated.Downstream would not accumulate ghost peers even if it did: the node id is config, so
node/info/<id>is overwritten rather than appended, and wavekv'scheck_uuidapplies to inbound requests only — the peer's next outbound sync carries the fresh record and the pair reconverges, whichsync.rs:495-509documents as the deliberate recovery path.No code change;
an_unwritable_data_dir_fails_the_boot_before_a_uuid_is_generatedplus a doc comment pin the ordering invariant, so this stays safe by construction rather than by luck.carry_unknown_fieldsdoes not grow foreverGrowth is a fixed cost, not a compounding one, and the module already ruled on it: "A field this binary stops declaring is carried forever… Retiring one for real needs an explicit list of names to drop on write; until something needs to be retired, that list would have no entries to hold."
Mechanically,
carried = stored_fields \ declared(T) \ encoded_fields, copied from the stored record on each write — so successive rewrites are a fixed point. A name can only enter the set by being declared by some gateway build: local writes use this binary's field list, and remote records arrive only over mTLS whereauthorize_peerrequires the peer certificate's app-id to equal this gateway's own. The bound is the union of declared field names across gateway builds in the cluster, and no KV value type has ever retired a field, so that set is empty today.No mechanism added —
CODING_TASTEsays bring the numbers before adding a limit.rewriting_a_record_forever_does_not_grow_it(50 alternating old/new rewrites, field count constant) turns "grows forever" into a pinned "does not".The certbot backup directory is not an unbounded growth problem
Three things, none of which the finding assumed. The gateway never writes one:
store_cert/new_cert_dirare reachable only fromCertBot::renew_inner, andgrepovergateway/finds noauto_renew/create_cert_if_needed/store_cert—DistributedCertBotstores certificates in WaveKV, and the gateway CVM ships no certbot binary. The rate is ~4.5 directories a year:renew_days_beforedefaults to 10 against 90-day Let's Encrypt certs andrenew_inneronly callsstore_certon an actual issuance — ~6 KB each, ~27 KB/year, from the standalone CLI only. And "list_cert_public_keysreads all of them on every call" describes no call:rgacross the whole tree finds zero callers outside the definitions. The archive is also the documented rollback path (live_links_can_roll_back_to_a_complete_generation).No change. Separate observation, not acted on:
WorkDir::list_certs/list_cert_public_keysandCertBot::list_certs/list_cert_public_keysare dead public API, a "delete, don't accumulate" candidate for its own PR.Verification
cargo test -j 16 -p dstack-gateway -p certbot: 379 passed, 0 failedcargo clippy -p dstack-gateway -p certbot --all-features -- -D warnings --allow unused_variables: cleancargo fmt --all -- --check: clean