refactor(cli): give streamio one input resolver - #4100
Draft
dmihalcik-virtru wants to merge 2 commits into
Draft
dmihalcik-virtru wants to merge 2 commits into
dmihalcik-virtru wants to merge 2 commits into
Conversation
DSPX-4499 fixed encrypt's OOM by streaming, but CreateTDF still required an io.ReadSeeker, so piped stdin had to be spooled to a temporary file first. That traded the whole-payload allocation for a disk write and a writable TMPDIR -- better, but not the point. Now that CreateTDF takes an io.Reader, the pipe goes straight to the SDK. What is spooled is decided by what the fd actually is, not by which argument it arrived through. A file argument and a shell redirect from a regular file both seek, so both reach the SDK unwrapped; only a genuine stream is buffered. The SDK measures whatever it can seek, and under ~2 GiB that keeps the archive in the compact ZIP32 layout; an unmeasurable payload has to be ZIP64, because the choice is baked into the payload's local file header before the first segment goes out. So `encrypt file.txt` and `encrypt < file.txt` are unchanged byte for byte, and `... | encrypt` produces a slightly larger TDF than it did when it was spooled. That is the trade, and it is the right way round: nobody should need a writable temp directory to encrypt a stream. Measurability is worth a second thing the archive layout does not show: the SDK fails a measured payload whose reader runs dry early, rather than returning a TDF that is silently short. A pipe has no declared length to check against. handlers.Handler's Encrypt is now the one place that states both, including that sdk.WithInputSize could restore the check for a caller who knows the length and that otdfctl does not expose it. The same distinction reaches decrypt and inspect, which spool because a TDF's manifest lives at the end of the archive. A redirect from a regular file is seekable already, so they read it in place rather than copying the whole TDF into TMPDIR to get a seekable view of it. MIME sniffing is what made this more than a deletion. It reads the first megabyte and previously seeked back to zero, which a pipe cannot do. Wrapping the input in a bufio.Reader is not an option either -- that hides the Seeker and would silently flip every file encrypt to ZIP64. detectMimeType now returns a reader alongside the type: the input itself, restored to the offset it was handed over at, when it can be; the sniffed prefix pushed back with io.MultiReader when it cannot. A megabyte in memory at most, and only when --mime-type was not given. Restoring the saved offset rather than seeking to zero matters once a redirect stays seekable: stdin arrives part-consumed whenever a wrapper reads a header before exec'ing us, and the payload is what remains, not the whole file. Rewindability is decided by attempting the seek, not by asserting io.Seeker. An *os.File on a FIFO, a process substitution, or /dev/stdin on the end of a pipe satisfies the interface and then returns ESPIPE, and treating that as fatal made `encrypt <(echo hi)` fail while `encrypt --mime-type text/plain <(echo hi)` succeeded -- a manifest-shaping flag deciding whether the command ran at all. A failed lseek leaves the offset untouched, so replaying the prefix is always correct. resolveInputSize in sdk/tdf.go already drew the same line. Input resolution moves into resolveEncryptInput so the ZIP32 invariant can be asserted without a running platform. That invariant lives in encryptRun, not in detectMimeType: anything wrapping the file between there and CreateTDF would flip every file encrypt to ZIP64 with the unit suite still green. encryptRun probes it the same way the SDK does, so the "this will be ZIP64" breadcrumb is logged once and is right whether or not detection ran -- --mime-type skips detectMimeType entirely, and that was the path on which an input that satisfies io.Seeker and then refuses reached the SDK unremarked. Testing: TestDetectMimeTypePreservesThePayload runs each case over all three reader shapes -- rewindable, Seek hidden, Seek refused with ESPIPE -- asserting both that the payload arrives whole and that only the first comes back measurable. Cases added for a payload of exactly the sniff window, the shortest input that fills the buffer and the boundary at which replaying the prefix could duplicate or drop a byte; for a reader handed over mid-payload, which must be restored to where it was rather than to zero; and for a read that fails partway, which must abort rather than encrypt a truncated payload. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. streamio gains regular-file stdin coverage it never had: the file comes back unwrapped with its offset untouched, an empty or fully-consumed one still reports absent, and OpenSeekable hands it over without creating a spool. e2e gains "encrypt measures a file and streams a pipe", which reads the local file header's extra field length to tell the two layouts apart -- both forms round-trip, so a quiet return to spooling would show up nowhere else -- now covering the redirect alongside the file and the pipe. A process-substitution case covers the ESPIPE path end to end, with and without --mime-type, which unit tests can only reach through a hand-written fake. The multi-segment case uses a real pipe rather than a redirect, since a redirect is now measured; 3 MiB clears the SDK's 2 MiB segment size, so the archive spans several segments whose hashes have to be combined on the way out and verified on the way back in. Decrypt from stdin is split into a piped case, which still spools and must remove the spool, and a redirect case, which no longer spools at all. extra_field_len fails rather than returning a number when it cannot read a full header: empty od output makes bash evaluate the arithmetic to 0, which is exactly the value the ZIP32 assertion passes on, so a missing archive would have looked like a pass. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
encrypt open-coded the file-argument-or-stdin choice that decrypt and inspect already got from streamio, and PipeReader was named for the case it handles most easily rather than the one that is hard. streamio.Open resolves either source without copying, preferring the file argument; OpenExclusive is the same for a command that refuses to guess, reporting ErrTwoInputs instead of silently dropping one of two payloads. OpenSeekable is now Open plus a spool, which drops its duplicate stdin branch, and encrypt's input block becomes one call and a switch. OpenFile is unexported behind them, and PipeReader is Piped. Behavior is unchanged: encrypt still rejects two inputs, decrypt and inspect still prefer the file argument and never touch a stdin they were not given -- Open leaves it unread, so a command inside a while-read loop cannot eat the loop's own input.
dmihalcik-virtru
force-pushed
the
dspx-2604-19a-streamio-open
branch
from
September 25, 2026 16:00
e9c2ded to
4c81900
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Reportgovulncheck-failure-3 |
dmihalcik-virtru
marked this pull request as draft
September 25, 2026 16:04
Contributor
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
This branch has not been deployed
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
1 participant
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.
Follow-up to #3948 (stacked on
dspx-2604-19-drop-encrypt-spool; review that one first). Pure refactor ofotdfctl/pkg/streamio— no behavior change.Proposed Changes
#3948 left
encryptopen-coding the "file argument or stdin" choice thatdecryptandinspectalready got fromstreamio, and leftPipeReadernamed for the case it handles most easily rather than the one that is hard.streamio.Open(path)— resolves either source without copying, so a measurable payload stays measurable. Prefers the file argument and does not look at stdin at all when one is given: a command insidewhile read f; do otdfctl … "$f"; done < listmust not peek the loop's own input out from under it.streamio.OpenExclusive(path)—Openfor a command that refuses to guess. Being handed both a file argument and a payload on stdin is the newErrTwoInputsrather than a silent preference. This is whatencrypthas always done; it is now said once, in the package that owns it.OpenSeekableisOpen+Spool— its duplicate stdin branch goes away (30 lines → 12).OpenFile→ unexportedopenFile, reached throughOpen/OpenExclusive. It keeps the zero-stat rule from fix(cli): drop the encrypt-side stdin spool #3948 and its tests.PipeReader→Piped— it takes redirects as much as pipes, and the old name collided withio.PipeReader.encryptRun's input block — aPipeReadercall, aninputCounttally, acliExitclosure and an open/branch (35 lines) become one call and aswitch, matching the shapedecryptRunalready uses.Behavior
Unchanged, deliberately:
encryptstill rejects two inputs, with the same two messages.decryptandinspectstill prefer the file argument and still never touch a stdin they were not given. Making the ambiguity check universal was the obvious way to fold this into one entry point, and it is whyOpenExclusiveexists separately instead — detecting a non-empty pipe means peeking it, and peeking eats bytes that belong to whatever shell construct supplied them.Checklist
Testing Instructions
cd otdfctl && go test ./... -race. Seven new tests coverOpen/OpenExclusive: file-beats-stdin (asserting stdin comes back unread), stdin-only,ErrNoInputfor neither,ErrTwoInputsfor both, and that a piped payload reaches the SDK unmeasurable — i.e. unspooled, which is the guarantee #3948 adds.Also run on Linux, where the procfs-dependent cases actually execute rather than skip: