Let a managed deployment bind somewhere other than loopback - #24
Conversation
![adaptcom[bot]](https://avatars.githubusercontent.com/in/1609964?s=60&v=4){kind=small}
There was a problem hiding this comment.
Confidence Score: 3/5
Summary
Adds opt-in remote access with bounded DNS/connect waits; previous findings are resolved. Two sandbox suite failures predate this revision, and Shane must verify deployment isolation before merging.
Important Files Changed
| File | Overview |
|---|---|
| crates/kinbase/src/company/client.rs | Propagates endpoint permission; authority channels remain loopback-only. |
| crates/kinbase/src/company/mod.rs | Passes service endpoint permission to steward publication. |
| crates/kinbase/src/company/server.rs | Conditionally relaxes Host validation with the bind opt-out. |
| crates/kinbase/src/config.rs | Adds default-off flags and rejects non-boolean values. |
| crates/kinbase/src/hooks.rs | Propagates endpoint permission into the bounded connectivity probe. |
| crates/kinbase/src/http.rs | Bounds DNS/connect waits and tests timeout behavior with deliberately slow work. |
| crates/kinbase/src/launcher.rs | Propagates configured permission to clients and port lookup. |
| crates/kinbase/src/repository.rs | Applies configured endpoint permission to certificate issuance. |
| crates/kinbase/tests/registry_publication.rs | Preserves loopback-only behavior in publication fixtures. |
| crates/kinbase/tests/service_bind_scope.rs | Checks network failure, bind defaults, and boolean type rejection. |
| .get("allow_non_loopback") | ||
| .and_then(toml::Value::as_bool) | ||
| .unwrap_or(false); | ||
| validate_loopback_url(&url, allow_non_loopback)?; |
There was a problem hiding this comment.
With allow_non_loopback=true, Client::new still rejects remote URLs in http::parse_url; propagate the configured flag through transport callers and cover a real non-loopback client request.
| // policy instead, so the guard is opt-out and stays on by default. | ||
| let allow_non_loopback = table | ||
| .get("allow_non_loopback") | ||
| .and_then(toml::Value::as_bool) |
There was a problem hiding this comment.
Both parsers silently treat allow_non_loopback="true" or 1 as false; reject present non-boolean values with CONFIG_INVARIANT instead of accepting malformed configuration.
f613a4e to
92db7f0
Compare
The proof binds loopback so a laptop store cannot be reached by anyone but its operator. A deployment behind a cluster Service takes that isolation from the network policy instead, and today it cannot start at all: the bind address, the Host header and the client's company.url are each required to be loopback. One opt-out, `allow_non_loopback`, default off, so nothing changes shape for an existing config. The bind and Host checks move together because they have to: enforcing Host against loopback on a routable bind refuses every request. The client flag is its own key in its own file, and a URL passed on the command line is now held to the operator's configured setting rather than to the argument. Tests cover the part that matters, which is that the opt-out stays opt-out: a config that does not mention the key still refuses a routable bind, and setting it false is the same as leaving it out.
92db7f0 to
53ec266
Compare
Allowing a Company endpoint to be a name rather than a loopback address moved work in front of the connection budget without extending it. `to_socket_addrs` blocks with no bound of its own, so a slow resolver pushed SessionStart past its own two-second budget while every message still said 250 ms. Resolution now runs on its own thread and the deadline covers both halves, with whatever remains handed to the connection. A resolver that answers late finds nobody listening; nothing is cancelled because the standard library offers no way to. Measured on the deployment host: an unresolvable name answers in 390-418 ms and an address that accepts nothing in 621 ms, against 465 ms for a working loopback endpoint.
The resolver tests only prove the bound on a machine whose resolver is slow enough to notice, so all three passed with the timeout removed. The bounded wait now lives in `within`, on its own, and is tested against work that sleeps five seconds against a fifty millisecond budget: unambiguous anywhere, and it fails in exactly five seconds when the bound is taken away.
|
Independent review. I traced the changed lines rather than reading the description, so let me lead with the thing I went in expecting to find and did not. The default is genuinely unchanged, and you proved it the right way
Three things here are better than the usual version of this change:
The third commit is the best thing in the PR: you re-ran the resolver tests with the timeout removed, found all three still passed, said so in the commit message, and wrote one that fails in exactly five seconds when the bound is gone. That is a mutation test on your own guard, unprompted. Blocking1. The spec is contradicted, not amended. 2. On a routable bind the bearer token is the whole credential, and it crosses the network in cleartext. The signature does not save this: Concrete: with If NetworkPolicy is the intended isolation, that is a defensible position, but it should be written down as an accepted risk rather than inherited silently — and the PR offers no transport option for operators who want defence in depth. 3. Pre-authentication work on an open port: SQLite writes and root-key signing. Compounding it: Non-blocking4. The Host check is deleted, not relaxed. I tried hard to build a rebinding exploit and failed — 5. Two owners for one fact, unreconciled. The client-side flag looks redundant. 6. The 250ms budget now has to cover DNS and TCP. 7. First resolved address only, no fallback. 8. Minor. Things I checked and could not make stick
VerdictCHANGES_REQUESTED — narrowly. The default path is provably unchanged and the mechanism is well built. The blocking asks are: amend the spec (or get the governance sign-off) before merge, and state in the PR — or fix — the cleartext-token and pre-auth-work exposures the opt-in creates. 4-8 are comments, not blockers. |
The bind, Host and client endpoint rules were lifted behind one opt-out key
without amending the three passages that state them, so the code and the spec
disagreed and the reader had no way to know which was current. REVIEW.md asks
for the passage to be amended or the defect raised, not for the deviation to
ship quietly.
Also records what the opt-in costs, which was inherited silently before. On a
routable bind the transport is cleartext: the endpoint parser accepts http://
only, so TLS is unreachable rather than absent, and the bearer token crosses the
network on every request. The request signature does not narrow this, because a
caller supplies its own client key and the first-use binding that would pin a
token to a key is recorded but never enforced. Isolation comes from the network
rather than from the service, which is defensible for a managed deployment and
is now written down as an accepted risk with the two open items named.
Spec: spec/cli.md bind address, spec/architecture.md Host validation and
credential model, spec/verification.md HTTP rejection list
Opening the database and running due maintenance both happened on arrival, ahead of the status short-circuit and ahead of authenticate. Maintenance prunes nonces, retires lapsed manifest observations and signs fact-events with the Company root key, so connecting was enough to make the service write SQLite and sign with its root key. On loopback the peer set was the operator's own processes and this did not arise. On a routable bind it is anyone who reaches the port. Maintenance now runs immediately after authenticate, so the same obligations are paid by a caller that proved a credential, and no handler can observe a stale observation the old order would have retired. Opening the handle stays in front because authenticate needs it to read the token and record a failure, and a SQLite open is a read, not a write. The accept loop also spawned a thread per connection with no ceiling, each held up to the five second read timeout before any credential was looked at, so silent connections could starve real clients long before the throttles inside authenticate were reached. Connections now claim a place under a ceiling of 64 and give it back on drop; over the ceiling the peer gets the typed 503 refusal rather than a thread. Spec: spec/architecture.md Host validation and credential model
The connect budget was sized when company.url was always a loopback literal and resolution cost nothing. A cluster Service name is the shape this opt-out exists for, and resolution now spent part of that budget before connect saw it: CoreDNS on a cold cache or with ndots search-path expansion can take most of it, leaving a healthy service to fail as COMPANY_UNREACHABLE. Resolution gets its own budget and the connect budget goes back to meaning only the connection, which leaves the ratified number alone. Only the first resolved address was tried. A dual-stack Service name can return AAAA first, so a pod with no working IPv6 route failed every request against a healthy service. Every address is now tried in order inside one deadline. A dead resolver thread and a spent budget both arrived as None and were both reported as a spent budget, so a thread that died read as a slow network. They are now distinct, a thread the OS refuses returns a typed error rather than panicking, and a URL naming no host is refused as configuration instead of surfacing later as an unreachable endpoint. Spec: spec/architecture.md connection budget
|
All three blockers addressed: Spec amended across Cleartext token recorded as accepted risk rather than inherited silently, including the part the spec had wrong: it claims tokens are "bound to a client-instance public key," but Pre-auth work: 6, 7 and 8 are fixed. Holding 4 and 5 as follow-ups rather than expanding a diff you'd already traced, though I agree the client-side flag is the redundant one. Say the word if you'd rather they land here. |
|
Re-reviewed at head The blockersSpec contradiction — fixed. Cleartext bearer token — accepted, not remediated. No code changed: Pre-auth work on an open port — mostly fixed, and the test is the good part.
Residual: 64 is a single process-global counter with no per-peer limit, so one peer can still take every slot and starve real clients for the 5 s read timeout. The contended resource moved from threads to slots; the denial didn't go away. What's holding the merge
Worst-case Company refresh is now 500 ms; both spec passages still say 250 ms; neither is in this PR's spec diff. This is the identical defect class I'm holding rather than merging because this PR's own thesis is that code and spec must not silently disagree, and you demonstrated that standard yourself in the first commit. Amend the two passages (or split the budget language so "connection budget" means the connect half) and I'll merge on sight — it's one commit. Also worth a look
In practice it's fine — WAL lets readers proceed, From the first reviewAddressed: the 250 ms budget covering DNS+TCP ( Not addressed: Host is still entirely unchecked once One commit from a merge. |
Giving resolution its own budget put two 250 ms budgets in series, so a refresh
against a named endpoint is bounded at 500 ms rather than 250. Two passages
still stated the old number: architecture.md attaches it to the refresh, and
verification.md sits inside a latency obligation, so anyone measuring either
would have measured against a bound the code no longer holds to.
The previous commit carried a Spec: trailer naming the connection budget and
amended no spec file, which made the record look discharged when it was not.
That is the same defect the first commit in this branch exists to fix.
Spec: spec/architecture.md SessionStart refresh budget,
spec/verification.md Company connect budget
An earlier commit here claimed a SQLite open is a read. It is not. CompanyDb::open sets the file mode and runs migrate, and migrate is two INSERT OR IGNORE statements against the meta table, so every connection took two write transactions against a ten second busy timeout before any credential was looked at. Moving maintenance behind authenticate removed the root-key signing but left this. open keeps the startup behaviour and is what the launcher and the tests use. connect is what a request uses: it opens, sets the busy timeout and the pragmas, and writes nothing. The symlink check stays on both, because a file swapped underneath a running service is what it exists to catch. Both are pinned by tests that fail without the change: open against a directory that does not exist yet, which is the ordering a first install depends on, and connect against a fresh file, which must come back with no schema.
|
Verified both at
Good work on this one — three blockers and two follow-ups in a day, and the Two things left open deliberately, neither blocking, both worth a follow-up ticket: the Host is still entirely unchecked once |
2 participants
The proof binds loopback so a laptop store cannot be reached by anyone but its operator. A deployment behind a cluster Service takes that isolation from the network policy instead, and today it cannot start at all.
Three gates enforce loopback, each labelled "in this proof":
bindaddressconfig.rs:865Hostheader, every requestserver.rs:278company.urlconfig.rs:625A sidecar proxy fixes the first two and cannot fix the third, because that one lives in every client’s own config check.
What this does
One opt-out key,
allow_non_loopback, default off, so nothing changes shape for an existing config.The bind and Host checks move together because they have to: enforcing Host against loopback on a routable bind refuses every request. The client flag is a separate key in a separate file, and a URL passed to
repo issue --companyis now held to the operator’s configured setting rather than to the argument.Tests
tests/service_bind_scope.rscovers the property that matters, which is that the opt-out stays opt-out:allow_non_loopback = falseis the same as leaving it outFull suite: 215 passed, 0 failed. Clippy: 671 warnings before and after, none added. Positive control: flipping the default to
truefails exactly the "still refuses" test; reverting passes it.Verified by hand as well — served on
0.0.0.0:8422with the flag, and a request withHost: kinbase.kinbase.svc.cluster.localreturned 401 rather than 403, i.e. it reached authentication instead of being refused at the Host check.Why it is safe to relax
The root key signs only on authenticated fact-event write paths.
GET /snapshotdoes not sign and exposes only the public key. Every route still requires a scoped token, a bound client key, a fresh nonce, an expiry and a request signature. Reaching the port is a confidentiality and DoS surface, not an integrity one.Part of a set
Addressed from review
A fourth gate existed and I had missed it.
http::parse_urlvalidates the endpoint again on every request, so the client flag passed config validation and was then refused at the socket — configured for a routable store, and reported as unreachable.parse_urlnow takes the same setting, threaded throughClient::newand its three call sites.deliver_to_channelkeeps passingfalse: an authority channel is loopback by its own rule, not the endpoint's.A present non-boolean is now malformed configuration, not a false.
allow_non_loopback = "true"previously read as off, refusing the bind while the file said otherwise.optional_boolrejects it with the section and key named.A real non-loopback client request is covered.
repo issueis the shortest path that actually builds a client, so the test drives that rather thanstatus, which returns on an uncertified repository before reaching the transport.Worth saying how that last one was found: the first version of the test passed with the fix and with the fix reverted. It was vacuous. The positive control caught it, and the fixture needed an
admin_token_filebeforerepo issuewould get as far as constructing a client at all.Now 218 tests pass, 7 in this file. Both new behaviours have positive controls: reverting the transport gate fails the client test, and restoring the silent coercion fails the boolean test.