relay: audit every request line on a kept connection - #197
Merged
Merged
Conversation
The audit tee recorded the first client line and then passed bytes through, on the premise that a connection carried one RPC. silkd now serves RPCs back to back on one connection (#196), so the tee scans every line: request frames are recorded, the input frames of the RPC in flight (stdin, stdin_close, data, data_end) are skipped by their canonical head, and a request frame past the 4 KB cap is recorded once as oversized. wire.IsContinuation names that head check for the relay and the SDKs alike. The Go-side docs drop the one-connection-per-RPC wording, and the agent endpoint's doc says that an open relay holds the sandbox's idle clock, which is what a client keeping a connection warm must know. Hot path: nothing changes without audit; with audit, one newline scan per relayed chunk and at most 4 KB copied per line, where only the first line was scanned before.
IsContinuation matched four canonical heads by prefix, which misread a producer that orders its keys differently and restated the op strings the encoders own; it now takes the op through frameTag, the canonical fast path with the token-walk fallback, and switches on the request types' own Op(). The audit tee regains a WriteTo with a bulk-sized buffer, since without it io.Copy's 32 KB buffer split every upload frame into eight reads, and it copies at most the cap plus one byte of an oversized frame instead of the whole chunk. The relay test builds its frames with wire.EncodeRequest so the predicate is tested against the encoders, not against literals.
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.
Second step of #195, the sandboxd side of a persistent data-plane connection; independent of #196 in code, motivated by it.
What changes
The audit tee in
sandboxd/server/relay.gorecorded the first client line and then degraded to a pass-through, on the premise that one connection carried one RPC. With silkd serving RPCs back to back on one connection (#196) it now scans every client line:stdin,stdin_close,data,data_end) are skipped by their canonical head, so an upload's 256 KB chunks never reach the journal;AuditLineCapis recorded once asoversized, the same event the cap produced before;wire.IsContinuationnames the head check on the shared wire package, next to the encoders that emit that head, so the relay and the SDKs agree on which frames open an RPC.Docs:
protocol/wire, the root README anddocs/sandboxd-api.mddrop the one-connection-per-RPC wording; the agent endpoint's doc now says that an open relay holds the sandbox's idle clock, which a client keeping a connection warm has to know foridle_hibernate_secondsto apply. Two stale comments (engine/silkd.go,pool/telemetry.go) follow.Nothing else in the relay changes: the hold, the wake-on-dial, the drain grace and the half-close handling are as before.
Hot path
Without audit: unchanged, the tee is not installed. With audit: one
IndexByteper relayed chunk and at most 4 KB copied per line, where only the first line was scanned before; the post-first-lineio.Copypass-through is gone, so an audited upload now flows through the tee'sReadfor its whole length.Evidence
TestAuditTeeRecordsRequestLinesdrives the tee whole and one byte at a time: requests back to back, an upload with a 12 KBdatachunk, stdin frames, an oversized request, a partial tail.TestIsContinuationpins the predicate to the package's own encoders.