Repository navigation
fix(cli): stream encrypt and decrypt instead of buffering whole payload - #3921
Closed
dmihalcik-virtru wants to merge 3 commits into
Closed
dmihalcik-virtru wants to merge 3 commits into
dmihalcik-virtru wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks 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 |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Member
Author
dmihalcik-virtru
force-pushed
the
DSPX-4499-streaming-codec
branch
from
August 27, 2026 13:34
a36892b to
58ab347
Compare
dmihalcik-virtru
force-pushed
the
feat/DSPX-2604-createtdf-chunked
branch
from
August 27, 2026 13:34
1db0af7 to
e2aa96b
Compare
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
added a commit
that referenced
this pull request
Aug 27, 2026
…3923) ## Purpose Intermediate PR in the stack `main ← #3782 ← **this** ← #3865 ← #3921`. #3782 introduced `ChunkedWriter` with its own private copies of crypto helpers that already existed in `sdk/tdf.go` (same package) and in `sdk/experimental/tdf/`. This PR collapses that three-way duplication before #3865 rewrites `CreateTDF` on top of `ChunkedWriter` — so that rewrite has one implementation to build on instead of two to reconcile. Duplication was located with `dupl` (AST/token-based, so it sees through the renames `wrapKeyWithEC` → `chunkedWrapKeyWithEC`). `jscpd` reported only 1.05% *exact* clones; that gap is itself the signal that this was a paraphrased copy rather than a literal one, and it's why CI never flagged it (`dupl` is not in `.golangci.yaml`'s enabled linters). ## Commits 1. **`refactor(sdk): reuse tdf.go crypto helpers in ChunkedWriter`** Deletes `chunkedEncryptMetadata`, `chunkedWrapKeyWith{EC,KEM,RSA,PublicKey}`, and `chunkedCreatePolicyBinding`, routing key access construction through the existing `createKeyAccess` / `encryptMetadata` / `createPolicyBinding`. Also hoists the policy binding and metadata encryption out of the per-KAS loop, which had been re-encrypting identical metadata once per URL in an OR-group. 2. **`refactor(sdk): point experimental/tdf manifest and assertion types at sdk`** Replaces the manifest and assertion types in `sdk/experimental/tdf` with aliases onto their `sdk` counterparts. Source-compatible — `experimental/tdf` is exported public API with likely downstream consumers, so nothing is deleted from its surface. `IntegrityAlgorithm` keeps its own defined type because `sdk`'s is `= int` and cannot carry the `String()` method. 3. **`fix(sdk): support legacy hex signatures in ChunkedWriter`** — see below. ## Bug fixes **EC key access was unusable.** Copied verbatim from `experimental/tdf/key_access.go`, `ChunkedWriter` emitted key type `"eccWrapped"` (the KAS only accepts `"ec-wrapped"`, `rewrap.go:713`) and XOR-wrapped the DEK with the HKDF output where the unwrap path expects AES-GCM. EC-KAS TDFs from `ChunkedWriter` could not be rewrapped. It went unnoticed because `experimental/tdf/reader.go` is a stub and `chunked_test.go`'s fake KAS is RSA-only. Fixed in both packages; `TestChunkedECKeyAccess` covers it and fails on the pre-fix code. **`WithChunkedExcludeVersion` produced unreadable TDFs.** The option omitted `schemaVersion` — which is exactly how a reader detects a pre-4.3.0 TDF and switches to expecting hex-then-base64 signatures — while both signature sites hardcoded `isLegacyTDF: false`. Every such TDF failed root signature verification. The two settings have to travel together, and `useHex` is consumed during `WriteSegment`, long before a `Finalize` option is seen. So this adds `WithChunkedTargetMode(mode string)` at construction, mirroring mainline `WithTargetMode` and reusing the package's existing `isLessThanSemver` / `hexSemverThreshold`. `WithChunkedExcludeVersion` on its own now fails with `ErrChunkedVersionHexMismatch` rather than emitting a TDF no reader can verify. Legacy hex readers are still deployed, so the doubly-encoded form remains fully supported — `TestChunkedLegacyTargetMode` round-trips a `4.2.2`-mode TDF and asserts the root and every segment signature decode to 64 bytes. ## Testing - `make lint` — clean apart from the pre-existing `SA1019` at `sdk/kas_client_test.go:188` - `make test` — passes across `sdk`, `otdfctl`, `service` - New: `TestChunkedECKeyAccess`, `TestChunkedKAOShape`, `TestChunkedLegacyTargetMode`, `TestChunkedCurrentTargetMode`, `TestChunkedExcludeVersionRequiresLegacyMode`, `TestChunkedTargetModeInvalid` - `ChunkedWriter` is only reachable end-to-end through #3865 + #3921, so cross-SDK xtest is run against the tip of the stack. ## Follow-ups (not in scope here) - Enable `dupl` for `sdk/` in `.golangci.yaml` — it would have blocked the original PR. - `SplitResult.KASPublicKeys` could be `map[string]KASInfo`, deleting the exported `KASPublicKey` entirely. That's a public-contract change to the new `KeySplitter` interface and belongs in its own PR. - Invert the relationship fully: mark `sdk/experimental/tdf` deprecated in its godoc pointing at the `sdk` equivalents. --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Comment on lines
+73
to
+80
| // cli.ExitWithError calls os.Exit, which skips deferred functions, so both | ||
| // the spooled input and the partial output have to be discarded first. | ||
| fail := func(msg string, err error) { | ||
| closeIn() | ||
| if outFile != nil { | ||
| outFile.Cleanup() | ||
| } | ||
| cli.ExitWithError(msg, err) |
Member
There was a problem hiding this comment.
@dmihalcik-virtru can you move this into the helper? There are other cases where we pipe results through and it seems like this should be reusable.
Comment on lines
+29
to
+59
| // detectMimeType sniffs the payload's type from its head, and returns a reader | ||
| // that still yields the whole payload. | ||
| // | ||
| // Detection needs only the first megabyte, and Peek does not consume, so those | ||
| // bytes stay in the buffer and reach the encoder. The payload is never held in | ||
| // memory in full. Wrapping is uniform across files and pipes and needs no seek. | ||
| func detectMimeType(in io.Reader, fileExt string) (string, io.Reader, error) { | ||
| buffered, ok := in.(*bufio.Reader) | ||
| if !ok { | ||
| buffered = bufio.NewReaderSize(in, Size1MB) | ||
| in = buffered | ||
| } | ||
|
|
||
| mimetype.SetLimit(Size1MB) // limit to 1MB | ||
| head, err := buffered.Peek(Size1MB) | ||
| // A payload shorter than the peek window is the common case, not an error. | ||
| if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, bufio.ErrBufferFull) { | ||
| return "", in, err | ||
| } | ||
|
|
||
| // defaults to application/octet-stream if nothing is recognized | ||
| detected := mimetype.Detect(head).String() | ||
| if detected == "application/octet-stream" && fileExt != "" { | ||
| // Lookup returns nil for an extension it does not know, which is not an | ||
| // error — octet-stream remains the right answer. | ||
| if byExt := mimetype.Lookup(fileExt); byExt != nil { | ||
| detected = byExt.String() | ||
| } | ||
| } | ||
| return detected, in, nil | ||
| } |
Member
There was a problem hiding this comment.
Shouldn't this be moved to a helper library rather than embedded in the command?
Comment on lines
+166
to
+173
| // cli.ExitWithError calls os.Exit, which skips deferred functions, so the | ||
| // partial output has to be discarded before handing off to it. | ||
| fail := func(msg string, err error) { | ||
| if tdfFile != nil { | ||
| tdfFile.Cleanup() | ||
| } | ||
| cli.ExitWithError(msg, err) | ||
| } |
Member
There was a problem hiding this comment.
Same callout as in the decrypt.
Comment on lines
+20
to
+161
| // stdinReader reports whether stdin is a pipe or redirect carrying at least one | ||
| // byte, and returns a reader over it. | ||
| // | ||
| // Presence is established with a one-byte Peek rather than a read, so the | ||
| // payload still reaches the caller intact and nothing is buffered beyond the | ||
| // reader's window. A terminal, or an empty redirect such as | ||
| // `otdfctl encrypt < /dev/null`, reports false — matching the behavior of the | ||
| // buffered implementation, which decided the same question by checking whether | ||
| // a full read of stdin came back empty. | ||
| // | ||
| // The buffer is sized so that a later Peek for MIME detection is served from | ||
| // it without a second allocation. | ||
| func stdinReader() (*bufio.Reader, bool) { | ||
| r, ok, err := pipeReader(os.Stdin) | ||
| if err != nil { | ||
| cli.ExitWithError("Failed to read stat from stdin", err) | ||
| cli.ExitWithError("failed to scan bytes from stdin", err) | ||
| } | ||
| if (stat.Mode() & os.ModeCharDevice) == 0 { | ||
| buf, err := io.ReadAll(os.Stdin) | ||
| return r, ok | ||
| } | ||
|
|
||
| // pipeReader is the testable half of stdinReader. | ||
| func pipeReader(in *os.File) (*bufio.Reader, bool, error) { | ||
| stat, err := in.Stat() | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| if (stat.Mode() & os.ModeCharDevice) != 0 { | ||
| return nil, false, nil | ||
| } | ||
|
|
||
| r := bufio.NewReaderSize(in, Size1MB) | ||
| if _, err := r.Peek(1); err != nil { | ||
| if errors.Is(err, io.EOF) { | ||
| return nil, false, nil | ||
| } | ||
| return nil, false, err | ||
| } | ||
| return r, true, nil | ||
| } | ||
|
|
||
| // spoolToTempFile copies r into a temporary file and rewinds it, giving a | ||
| // seekable view of a stream that has none. | ||
| // | ||
| // A TDF's manifest sits at the end of the archive, so decrypt and inspect have | ||
| // to seek and cannot consume a pipe directly. Spooling trades disk for the | ||
| // memory the old whole-payload read used, and needs a TMPDIR with room for the | ||
| // full TDF — it fails loudly if there isn't one. | ||
| // | ||
| // The returned cleanup must run on every path. cli.ExitWithError calls os.Exit | ||
| // and skips deferred functions, so deferring it alone is not enough. | ||
| func spoolToTempFile(r io.Reader) (*os.File, func(), error) { | ||
| f, err := os.CreateTemp("", "otdfctl-spool-*.tdf") | ||
| if err != nil { | ||
| return nil, func() {}, err | ||
| } | ||
| cleanup := func() { | ||
| f.Close() | ||
| os.Remove(f.Name()) | ||
| } | ||
| if _, err := io.Copy(f, r); err != nil { | ||
| cleanup() | ||
| return nil, func() {}, err | ||
| } | ||
| if _, err := f.Seek(0, io.SeekStart); err != nil { | ||
| cleanup() | ||
| return nil, func() {}, err | ||
| } | ||
| return f, cleanup, nil | ||
| } | ||
|
|
||
| // openSeekableInput resolves a command's input to something seekable: the named | ||
| // file when one is given, otherwise piped stdin spooled to disk. It reports the | ||
| // same "no input" condition for an absent file argument and an empty pipe. | ||
| // | ||
| // The returned cleanup must run on every path, per spoolToTempFile. | ||
| func openSeekableInput(path string) (*os.File, func(), error) { | ||
| if path != "" { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| cli.ExitWithError("failed to scan bytes from stdin", err) | ||
| return nil, func() {}, err | ||
| } | ||
| return buf | ||
| return f, func() { f.Close() }, nil | ||
| } | ||
|
|
||
| piped, ok := stdinReader() | ||
| if !ok { | ||
| return nil, func() {}, errNoInput | ||
| } | ||
| return spoolToTempFile(piped) | ||
| } | ||
|
|
||
| var errNoInput = errors.New("no input provided") | ||
|
|
||
| // outputFile writes to a temporary file alongside the destination and renames | ||
| // it into place only once the write has succeeded, so an interrupted or failed | ||
| // run leaves no partial output where a complete file is expected. | ||
| // | ||
| // Note that cli.ExitWithError calls os.Exit, which does not run deferred | ||
| // functions. Cleanup must therefore be called explicitly on every error path, | ||
| // not only via defer. | ||
| type outputFile struct { | ||
| f *os.File | ||
| path string | ||
| committed bool | ||
| } | ||
|
|
||
| // newOutputFile creates the temporary file in the destination's own directory, | ||
| // which keeps the final rename atomic — across filesystems it would degrade to | ||
| // a copy. | ||
| func newOutputFile(path string) (*outputFile, error) { | ||
| f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".tmp-*") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &outputFile{f: f, path: path}, nil | ||
| } | ||
|
|
||
| func (o *outputFile) Write(p []byte) (int, error) { return o.f.Write(p) } | ||
|
|
||
| // Commit closes the temporary file and moves it onto the destination path. | ||
| func (o *outputFile) Commit() error { | ||
| if err := o.f.Close(); err != nil { | ||
| os.Remove(o.f.Name()) | ||
| return err | ||
| } | ||
| if err := os.Rename(o.f.Name(), o.path); err != nil { | ||
| os.Remove(o.f.Name()) | ||
| return err | ||
| } | ||
| o.committed = true | ||
| return nil | ||
| } | ||
|
|
||
| // Cleanup discards the temporary file. It is a no-op after a successful | ||
| // Commit, so it is safe to both defer it and call it directly. | ||
| func (o *outputFile) Cleanup() { | ||
| if o.committed { | ||
| return | ||
| } | ||
| o.f.Close() | ||
| os.Remove(o.f.Name()) | ||
| } |
Member
There was a problem hiding this comment.
Can we move these to the /pkg directory as helpers? Also why aren't we updating our shared readPipedStdin and instead developing yet another helper?
Comment on lines
-8
to
-45
| func ReadFromArgsOrPipe(args []string, pipe *os.File) []byte { | ||
| if len(args) > 0 { | ||
| return ReadFromFile(args[0]) | ||
| } | ||
| if pipe == nil { | ||
| pipe = os.Stdin | ||
| } | ||
| return ReadFromPipe(pipe) | ||
| } | ||
|
|
||
| func ReadFromPipe(in *os.File) []byte { | ||
| stat, err := in.Stat() | ||
| if err != nil { | ||
| ExitWithError("failed to read stat from stdin", err) | ||
| } | ||
| if (stat.Mode() & os.ModeCharDevice) == 0 { | ||
| buf, err := io.ReadAll(in) | ||
| if err != nil { | ||
| ExitWithError("failed to scan bytes from stdin", err) | ||
| } | ||
| return buf | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func ReadFromFile(filePath string) []byte { | ||
| fileToEncrypt, err := os.Open(filePath) | ||
| if err != nil { | ||
| ExitWithError("Failed to git open file at path: "+filePath, err) | ||
| } | ||
| defer fileToEncrypt.Close() | ||
|
|
||
| bytes, err := io.ReadAll(fileToEncrypt) | ||
| if err != nil { | ||
| ExitWithError("Failed to read bytes from file at path: "+filePath, err) | ||
| } | ||
| return bytes | ||
| } |
Member
There was a problem hiding this comment.
Are we considering if external libraries might be using this package?
dmihalcik-virtru
force-pushed
the
feat/DSPX-2604-createtdf-chunked
branch
from
August 27, 2026 21:01
e2aa96b to
a41be21
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4499-streaming-codec
branch
from
August 27, 2026 21:01
58ab347 to
228a1ab
Compare
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dmihalcik-virtru
changed the title
Contributor
dmihalcik-virtru
added a commit
that referenced
this pull request
Aug 31, 2026
…3923) ## Purpose Intermediate PR in the stack `main ← #3782 ← **this** ← #3865 ← #3921`. #3782 introduced `ChunkedWriter` with its own private copies of crypto helpers that already existed in `sdk/tdf.go` (same package) and in `sdk/experimental/tdf/`. This PR collapses that three-way duplication before #3865 rewrites `CreateTDF` on top of `ChunkedWriter` — so that rewrite has one implementation to build on instead of two to reconcile. Duplication was located with `dupl` (AST/token-based, so it sees through the renames `wrapKeyWithEC` → `chunkedWrapKeyWithEC`). `jscpd` reported only 1.05% *exact* clones; that gap is itself the signal that this was a paraphrased copy rather than a literal one, and it's why CI never flagged it (`dupl` is not in `.golangci.yaml`'s enabled linters). ## Commits 1. **`refactor(sdk): reuse tdf.go crypto helpers in ChunkedWriter`** Deletes `chunkedEncryptMetadata`, `chunkedWrapKeyWith{EC,KEM,RSA,PublicKey}`, and `chunkedCreatePolicyBinding`, routing key access construction through the existing `createKeyAccess` / `encryptMetadata` / `createPolicyBinding`. Also hoists the policy binding and metadata encryption out of the per-KAS loop, which had been re-encrypting identical metadata once per URL in an OR-group. 2. **`refactor(sdk): point experimental/tdf manifest and assertion types at sdk`** Replaces the manifest and assertion types in `sdk/experimental/tdf` with aliases onto their `sdk` counterparts. Source-compatible — `experimental/tdf` is exported public API with likely downstream consumers, so nothing is deleted from its surface. `IntegrityAlgorithm` keeps its own defined type because `sdk`'s is `= int` and cannot carry the `String()` method. 3. **`fix(sdk): support legacy hex signatures in ChunkedWriter`** — see below. ## Bug fixes **EC key access was unusable.** Copied verbatim from `experimental/tdf/key_access.go`, `ChunkedWriter` emitted key type `"eccWrapped"` (the KAS only accepts `"ec-wrapped"`, `rewrap.go:713`) and XOR-wrapped the DEK with the HKDF output where the unwrap path expects AES-GCM. EC-KAS TDFs from `ChunkedWriter` could not be rewrapped. It went unnoticed because `experimental/tdf/reader.go` is a stub and `chunked_test.go`'s fake KAS is RSA-only. Fixed in both packages; `TestChunkedECKeyAccess` covers it and fails on the pre-fix code. **`WithChunkedExcludeVersion` produced unreadable TDFs.** The option omitted `schemaVersion` — which is exactly how a reader detects a pre-4.3.0 TDF and switches to expecting hex-then-base64 signatures — while both signature sites hardcoded `isLegacyTDF: false`. Every such TDF failed root signature verification. The two settings have to travel together, and `useHex` is consumed during `WriteSegment`, long before a `Finalize` option is seen. So this adds `WithChunkedTargetMode(mode string)` at construction, mirroring mainline `WithTargetMode` and reusing the package's existing `isLessThanSemver` / `hexSemverThreshold`. `WithChunkedExcludeVersion` on its own now fails with `ErrChunkedVersionHexMismatch` rather than emitting a TDF no reader can verify. Legacy hex readers are still deployed, so the doubly-encoded form remains fully supported — `TestChunkedLegacyTargetMode` round-trips a `4.2.2`-mode TDF and asserts the root and every segment signature decode to 64 bytes. ## Testing - `make lint` — clean apart from the pre-existing `SA1019` at `sdk/kas_client_test.go:188` - `make test` — passes across `sdk`, `otdfctl`, `service` - New: `TestChunkedECKeyAccess`, `TestChunkedKAOShape`, `TestChunkedLegacyTargetMode`, `TestChunkedCurrentTargetMode`, `TestChunkedExcludeVersionRequiresLegacyMode`, `TestChunkedTargetModeInvalid` - `ChunkedWriter` is only reachable end-to-end through #3865 + #3921, so cross-SDK xtest is run against the tip of the stack. ## Follow-ups (not in scope here) - Enable `dupl` for `sdk/` in `.golangci.yaml` — it would have blocked the original PR. - `SplitResult.KASPublicKeys` could be `map[string]KASInfo`, deleting the exported `KASPublicKey` entirely. That's a public-contract change to the new `KeySplitter` interface and belongs in its own PR. - Invert the relationship fully: mark `sdk/experimental/tdf` deprecated in its godoc pointing at the `sdk` equivalents. --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
feat/DSPX-2604-createtdf-chunked
branch
from
August 31, 2026 20:34
a41be21 to
3b9d11d
Compare
This was referenced Sep 1, 2026
streaming codec Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
otdfctl encrypt read the entire plaintext into memory, encrypted it into a second whole-payload buffer, then copied that to the destination. Peak RSS ran about 3.6x the payload, so a large enough file OOMs on a machine with ample disk for it. The SDK already streams; this was purely a CLI-layer choice. Reorder the command to resolve its destination first and hand the SDK a reader and a writer, so memory is bounded by segment size rather than payload length. - handlers.EncryptBytes becomes Encrypt(ctx, out, in, EncryptOptions), taking an io.Reader now that CreateTDF no longer requires a seeker. - MIME detection reads the head of the payload with bufio.Peek, which does not consume, instead of inspecting a full in-memory copy. - Buffering hides an *os.File's Seeker, which would push the SDK onto its unknown-size path and force ZIP64. Pass the stat size via WithInputSize for regular files so output stays comparable with the buffered implementation. Piped input is legitimately ZIP64. - Output goes to a temp sibling renamed into place on success, so a failed run leaves no partial .tdf. cli.ExitWithError calls os.Exit and skips deferred functions, so cleanup is also invoked explicitly on error paths. DecryptBytes and InspectTDF are unchanged here; they follow in the next commit. Fills in spec/DSPX-4499.md from the ticket. Note that e2e/encrypt-decrypt.bats carries a file-level skip unrelated to this change, so the new cases will not run until that is lifted. Refs DSPX-4499 Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…ayload Completes DSPX-4499. decrypt read the whole TDF into memory, decrypted it into a second whole-payload buffer, then fmt.Print'd a third copy. inspect read an entire TDF just to reach its manifest. - handlers.DecryptBytes becomes Decrypt(ctx, out, in, DecryptOptions) and InspectTDF takes an io.ReadSeeker, so neither holds the payload. - io.Copy to the destination selects sdk.Reader's WriteTo, which decrypts one segment at a time. A compile-time io.WriterTo assertion guards that: without WriteTo, io.Copy silently falls back to Read/ReadAt and re-buffers the whole payload with no test failure to show for it. - The manifest lives at the end of the archive, so both commands need a seekable input. A file argument is opened directly; piped stdin is spooled to a temp file, trading disk for the memory the old read used. - -o writes to a temp sibling renamed into place on success. Cleanup runs explicitly on error paths, since ExitWithError calls os.Exit and skips defers — in inspect every exit path does, including the successful one. - Drops the 10 GB MaxFileSize CLI cap. It existed to bound RAM; the SDK's real limit now applies. - Deletes pkg/cli/pipe.go, whose three whole-file readers inspect was the last caller of. ReadFromFile had an unbounded io.ReadAll with no cap at all. Measured against a local platform, peak RSS for both commands is now flat in payload size rather than ~3.6x it: payload encrypt decrypt 1 GiB 73 MiB 76 MiB 4 GiB 78 MiB 74 MiB Both round-trips are byte-identical. A file input still produces a non-ZIP64 archive; piped input is ZIP64, since the size cannot be known before the first segment header is written. Note that e2e/encrypt-decrypt.bats carries a file-level skip unrelated to this change, so the new cases will not run until that is lifted. Refs DSPX-4499 Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
feat/DSPX-2604-createtdf-chunked
branch
from
September 1, 2026 15:06
c280206 to
ca2c9ae
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4499-streaming-codec
branch
from
September 1, 2026 15:06
4033c77 to
6645a56
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 8, 2026
> **Part 08 of 20** in the DSPX-2604 re-cut. Base branch: `main`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes Adds otdfctl/pkg/streamio, holding the input and output plumbing that the streaming encrypt and decrypt work needs, and migrates `inspect` onto it so nothing is left calling the buffered helpers it supersedes. This is groundwork with one user-visible consequence: `inspect` no longer reads the whole TDF into memory. Everything else is a move. Why a new package rather than pkg/cli. The helpers in pkg/cli/pipe.go call ExitWithError -- which calls os.Exit -- from inside the read, so they cannot be used from anywhere that wants to handle the failure itself, and they read the entire input into memory. streamio returns errors and leaves the decision to exit with the command layer. What moved in: - PipeReader establishes whether stdin is a non-empty pipe with a one-byte Peek instead of a read, so the payload still reaches the caller. - Spool copies a pipe to a temporary file and rewinds it. A TDF's manifest sits at the end of the archive, so decrypt and inspect have to seek and cannot consume a pipe directly. - OpenSeekable resolves "file argument or piped stdin" to one seekable handle, reporting ErrNoInput for the shared "nothing to read" case. - OutputFile writes to a temporary sibling of the destination and renames it into place on Commit, so a failed run leaves no partial output. The temp file is a sibling so the rename stays atomic rather than degrading to a cross-filesystem copy. Per review feedback on #3921: - readPipedStdin now delegates its detection to streamio.PipeReader rather than answering "is there piped input?" a second way. Its read is still unbounded; the callers that must stop buffering are changed separately. - pkg/cli/pipe.go is deprecated rather than deleted, since the package is exported and may have callers outside this repository. Worth noting that ReadFromFile has no size cap at all -- not even the 10 GB the tdf commands apply -- which is its own argument for the notice. InspectTDF takes an io.ReadSeeker instead of a byte slice. GetTdfType already rewinds to the start, so the reader is positioned for LoadTDF. Because cli.ExitWithError calls os.Exit and skips deferred functions, inspectRun invokes cleanup explicitly on every exit path, including the successful one: piped input is spooled to disk and the temp file would otherwise survive. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd otdfctl && go test ./pkg/streamio/... ./cmd/... -race ``` `inspect` is the only command migrated in this PR; check it still reads both a file argument and piped stdin, and that no `otdfctl-spool-*` file survives either run. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added reliable support for inspecting TDF content from files, piped input, and standard input. - Added safer output handling that prevents incomplete files from replacing existing results. - Added clearer input errors when no content is provided or an input cannot be opened. - **Bug Fixes** - Improved handling of large and non-seekable input streams. - Preserved piped input correctly while processing and inspecting content. - Non-fatal inspection issues are now reported as warnings where possible, allowing processing to continue. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 22, 2026
> **Part 14 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-13-unresolved-kas`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes `sdk/experimental/tdf` carried its own copies of the manifest and assertion types -- `Manifest`, `Segment`, `KeyAccess`, `Assertion`, `Statement`, `AssertionKey` and the rest -- structurally identical to the ones in `sdk` but distinct to the type system, so anything crossing the boundary needed conversion. Two copies of the JWT signing and verification logic also had to be kept in step by hand. Replaces both files' definitions with type aliases. `sdk` owns the definitions; this package re-exports them. Every exported name and every method survives: `Assertion.Sign` / `Verify` / `GetHash`, `Statement.UnmarshalJSON`, `AssertionKey.IsEmpty` / `Algorithm`, `AssertionVerificationKeys.Get` / `IsEmpty` and the five `String()` methods all come along with the aliased types, so importers compile unchanged. A manifest produced here can now be handed to the stable SDK without conversion, which is what the follow-up delegation needs. Two deliberate non-aliases: `Policy`, `PolicyBody` and `PolicyAttribute` stay local. `sdk.PolicyObject` declares `Body` as an anonymous struct over an unexported element type, so there is no nameable sdk equivalent to alias to. Exporting those in `sdk` first would make the alias possible; that is a separate change. `IntegrityAlgorithm` stays a distinct `int` type. `sdk.IntegrityAlgorithm` is itself `= int`, so no method can be attached to it, and aliasing would silently drop `String()` from this package's public API. The underlying values match, so the two convert freely. `kSplitKeyType`, `kPolicyBindingAlg`, `kGMACPayloadLength` and `calculateSignature` are retained verbatim: this package still builds its own manifests and they have callers in `writer.go` and `key_access.go`. The change that removes those callers removes these too. No behavior change. The package's existing tests pass unmodified. ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./experimental/... -race ``` No behavior change — the point is that the package's existing tests pass unmodified against aliased types. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Compatibility** * Experimental TDF manifest and assertion types now align with the stable SDK, enabling direct interoperability without type conversion. * Existing assertion and manifest behavior is provided through the stable SDK definitions. * **Documentation** * Clarified the relationship between the experimental TDF package and the stable SDK. * Updated documentation for missing assertion verification keys to reflect current behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Member
Author
opentdf-automation Bot
added a commit
that referenced
this pull request
Sep 28, 2026
> **Part 19 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-base-19`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes 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. A file is still opened seekably, on purpose. The SDK measures a seekable payload and keeps the archive in the compact ZIP32 layout; an unmeasurable one 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` is 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. 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 same reader for a seekable input, rewound; the sniffed prefix pushed back with io.MultiReader for anything else. A megabyte in memory at most, and only when --mime-type was not given. Testing: TestDetectMimeTypePreservesThePayload now runs each case twice, once over a reader whose Seek method is hidden, and asserts the payload arrives whole either way. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. 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. ### Checklist - [x] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` cd otdfctl && go test ./... -race ``` e2e, against a running platform: ``` cd otdfctl && bats e2e/streaming.bats ``` The file is tagged `payload_streaming` and runs in its own pass ahead of the parallel batch — see #3939 for why that ordering is load-bearing (the platform base key `key-base.bats` sets cannot be un-set, and breaks every unattributed encrypt scheduled after it). The case to look at is `encrypt measures a file and streams a pipe`, which asserts the ZIP layout rather than the round trip: a file must stay ZIP32 (extra field length 0 at offset 28 of the payload's local file header) and a pipe must be ZIP64 (non-zero). A quiet return to spooling would show up nowhere else, since both forms round-trip fine. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Encryption now supports piped and other non-seekable input, including larger streams. * Inputs that cannot be measured in advance produce ZIP64 output; seekable files continue to use ZIP32 when applicable. * Regular-file redirects can be read directly without being copied to a temporary file. * Decryption now supports TDFs supplied through pipes and redirects. * Encrypted output remains compatible with the existing decryption flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com> (cherry picked from commit 5603c2e)
opentdf-automation Bot
added a commit
that referenced
this pull request
Sep 28, 2026
…ap (#3945) > **Part 16 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-07-readfull`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes CreateTDF and CreateTDFContext took an io.ReadSeeker, so a caller with a pipe, a socket, or any other one-pass source had to spool the whole payload to disk or memory first. That is the block DSPX-2604 exists to remove: the Everfox re-wrap pipeline hands us a stream it cannot rewind. Both now take an io.Reader and consume it from its current position through EOF. Seekability was only ever used to measure the input. The length still matters, but it is now resolved rather than required: - WithInputSize(n) declares it outright, for a reader that cannot report it; - failing that, a reader that happens to implement io.Seeker is probed, and the cursor restored to wherever it was; - failing both, the payload is unmeasurable and is read until it ends. The one thing an unmeasurable payload gives up is the compact ZIP32 layout. The ZIP64 decision is baked into the payload's local file header, which is emitted ahead of the first segment, so it cannot be revisited once the archive has started; a payload that might exceed a 32-bit offset has to be written as ZIP64 from the outset. WithInputSize exists to buy that back — declaring the length of a piped payload keeps it in ZIP32 when it fits. The read loop no longer computes a segment count up front. It reads a buffer at a time until EOF, which is what makes an unknown length workable, and happens to be the same code path for a short final segment. An empty payload still produces one empty segment. The segment count is still passed to the archive writer when it is known, because that is what keeps a large declared count from being clamped to a one-segment capacity hint. Three behavior changes worth calling out. The first is the only one that can silently shorten a payload; the other two fail loudly. - A seekable reader is no longer rewound. The old code seeked to the end to measure and then back to byte 0, so it encrypted the whole file no matter where the caller had left the cursor. resolveInputSize saves the current position and restores that one, so the payload is whatever remains from there. Concretely: a caller that sniffs the first 512 bytes for content-type detection and then hands the same *os.File to CreateTDF used to get the whole file and now gets the file minus 512 bytes — no compile error, no runtime error, just a shorter TDF that looks complete. Reading from the current position is the right semantics for an io.Reader-shaped API, and `a seekable reader is encrypted from its current position` pins it, but a caller relying on the rewind has to seek to 0 itself now. - The 64 GB cap (maxFileSizeSupported/errFileTooLarge) is gone. It could only ever be enforced on a measurable payload, so keeping it would have meant `encrypt bigfile` failing where `encrypt < bigfile` succeeded. Both were unexported; nothing outside the package referenced them. - A declared size is exact, not an upper bound. A reader that reaches EOF early now fails the call with errInputShorterThanDeclared instead of returning a TDF that is silently short of the payload the caller asked to encrypt. Reading still stops at the declared size if the reader has more. What bounds a payload now, with the cap gone. maxPayloadSegments refuses a size needing more than MaxInt32 segments, but segmentCount is the only place it is enforced and that runs only when the length resolves — an unmeasurable stream has no segment ceiling. Underneath that, the real limit is manifest memory: one manifest segment and one archive-writer entry per segment, all live until finalize, roughly 500k entries per terabyte at the 2 MiB default. Neither is reachable today — 4 PiB at that segment size — so this is a note about where the wall moved to, not a regression. DSPX-4905 tracks bounding and measuring both. Testing: Test_CreateTDF_StreamingInput covers the three measurement modes across empty, sub-segment, exact-multiple, and partial-final-segment payloads, asserting the ZIP64 choice, the segment count, and a full round trip through LoadTDF. Test_CreateTDF_InputSizeBounds covers the negative, over-long, short, and mid-stream-start cases. Both guards were mutation-checked: removing the io.LimitReader fails "declared size bounds the read", and dropping the unknown-size ZIP64 rule fails every unmeasurable case. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./... -race ``` `Test_CreateTDF_StreamingInput` and `Test_CreateTDF_InputSizeBounds` are the new coverage. Both guards were mutation-checked: removing the `io.LimitReader` fails "declared size bounds the read", and dropping the unknown-size ZIP64 rule fails every unmeasurable case. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * TDF creation now supports streaming inputs, including non-seekable readers such as pipes and network streams. * Added `WithInputSize` to declare the exact payload size when it cannot be determined automatically. * Payloads are processed in segments, supporting unknown sizes and ZIP64 archives when needed. * **Bug Fixes** * Invalid or inaccurate declared sizes now result in clear errors, including when input ends too early. * Payloads exceeding the supported segment limit are rejected. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com> (cherry picked from commit 20ff505)
elizabethhealy
pushed a commit
that referenced
this pull request
Sep 28, 2026
> **Part 19 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-base-19`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes 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. A file is still opened seekably, on purpose. The SDK measures a seekable payload and keeps the archive in the compact ZIP32 layout; an unmeasurable one 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` is 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. 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 same reader for a seekable input, rewound; the sniffed prefix pushed back with io.MultiReader for anything else. A megabyte in memory at most, and only when --mime-type was not given. Testing: TestDetectMimeTypePreservesThePayload now runs each case twice, once over a reader whose Seek method is hidden, and asserts the payload arrives whole either way. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. 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. ### Checklist - [x] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` cd otdfctl && go test ./... -race ``` e2e, against a running platform: ``` cd otdfctl && bats e2e/streaming.bats ``` The file is tagged `payload_streaming` and runs in its own pass ahead of the parallel batch — see #3939 for why that ordering is load-bearing (the platform base key `key-base.bats` sets cannot be un-set, and breaks every unattributed encrypt scheduled after it). The case to look at is `encrypt measures a file and streams a pipe`, which asserts the ZIP layout rather than the round trip: a file must stay ZIP32 (extra field length 0 at offset 28 of the payload's local file header) and a pipe must be ZIP64 (non-zero). A quiet return to spooling would show up nowhere else, since both forms round-trip fine. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Encryption now supports piped and other non-seekable input, including larger streams. * Inputs that cannot be measured in advance produce ZIP64 output; seekable files continue to use ZIP32 when applicable. * Regular-file redirects can be read directly without being copied to a temporary file. * Decryption now supports TDFs supplied through pipes and redirects. * Encrypted output remains compatible with the existing decryption flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com> (cherry picked from commit 5603c2e)
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
> **Part 10 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-09-stream-encrypt`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes `otdfctl decrypt` read the whole TDF into memory, handed the slice to DecryptBytes, which accumulated the whole plaintext in a bytes.Buffer, and then -- for stdout -- called Buffer.String(), allocating a third full copy. Peak RSS was roughly 3.6x the payload; a 1 GiB file cost ~3.7 GiB of RAM and a large enough file simply OOMed on a machine with plenty of disk for it. The plaintext now streams from the SDK reader to the destination. Handler.Decrypt takes an io.ReadSeeker and an io.Writer, with DecryptOptions replacing the positional parameter list, and inspect reaches the manifest through the same seekable reader rather than buffering the archive to get at its tail. io.Copy is what does the streaming, and it does so only because sdk.Reader implements WriteTo, which decrypts one segment at a time. Its Read delegates to ReadAt, which grows an internal bytes.Buffer holding every segment decrypted so far -- so dropping WriteTo would silently restore the old memory profile with no test failure to show for it. A compile-time assertion pins the interface. Removes MaxFileSize. The 10 GB cap existed to bound RAM; the real limit is the SDK maxFileSizeSupported at 64 GiB, which enforces itself. Output to a file is atomic, as on the encrypt side: the plaintext goes to a temporary sibling and is renamed into place only on success. Since cli.ExitWithError calls os.Exit and skips deferred functions, the spooled input and the partial output are discarded explicitly on every exit path -- including inspect's success path, which exits through ExitWithJSON. e2e coverage lands in a new otdfctl/e2e/streaming.bats rather than in encrypt-decrypt.bats, which carries a file-level skip pending the namespaced-subject-mappings migration and would have swallowed the new cases without running them. Nothing in the new file needs an entitlement, so it needs no policy fixtures: the round-trips use no attributes, and the two failure cases are forced with an unresolvable attribute FQN and a KAS allowlist that excludes the platform. As of this change it is the only e2e coverage of encrypt, decrypt and inspect that actually executes in CI. The file is tagged payload_streaming and action.yaml gives it its own pass ahead of the parallel batch. That ordering is load-bearing, not tidiness. An encrypt with no attributes falls back to the platform base key, and key-base.bats sets one pointing at https://test-kas-for-base-keys.com, which does not resolve. It cannot put things back afterwards: a base key can be replaced but never cleared, so every unattributed encrypt scheduled after that file yields a TDF nothing can decrypt. Under --jobs 4 the file order is nondeterministic, so overlapping the two made this suite flaky rather than merely broken -- which is how it presented, a different subset of round-trips failing per run. Running alone also keeps the 1 GiB peak-RSS case from measuring itself against three neighbours competing for the same memory. That leak is worth closing on its own -- encrypt-decrypt.bats walks into it the day its skip is lifted -- but the fix belongs with the file that opens it rather than here. ### Checklist - [x] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` cd otdfctl && go test ./... -race ``` e2e, against a running platform: ``` cd otdfctl && bats --tap e2e --filter-tags payload_streaming ``` The first CI run of this file failed 308–311 and 317, all of them the cases that need a successful decrypt. Cause was not the code under test: an encrypt with no attributes falls back to the platform base key, and `key-base.bats` sets one pointing at `https://test-kas-for-base-keys.com`, which does not resolve — and cannot unset it, because a base key can only be replaced. Under `--jobs 4` the file order is nondeterministic, so which subset failed varied per run. Fixed here by tagging the file `payload_streaming` and giving it its own pass before the parallel batch. Tag arithmetic checks out: 14 + 10 + 330 = 354, the same total as before. The memory case needs GNU `time` (`gtime` on macOS) and skips without it. It allocates a 1 GiB file; peak RSS was ~3.6 GiB per command before this change and the assertion threshold is 512 MiB. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Decryption now streams files and standard input, reducing memory usage for large payloads. - Decrypted output supports standard output, device paths, and symbolic-link destinations. - Failed operations clean up temporary data and avoid leaving incomplete output files. - **Bug Fixes** - Improved handling and validation of empty or invalid encrypted input. - Existing destination files are preserved when decryption fails. - Expanded coverage for streaming, cleanup, special destinations, and large-payload memory usage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
…ntdf#3941) > **Part 12 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-11-chunked-writer`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes `chunkedWriter.GetManifest` held `mu.RLock` for its whole body, and that body calls `KeySplitter.Split`. A real splitter resolves KAS public keys over the network, so every `WriteSegment` racing a `GetManifest` sat on the write lock until those round-trips finished. `sync.RWMutex` bars new readers once a writer is queued, so a second `GetManifest` behind that `WriteSegment` blocked as well -- one manifest snapshot could stall the whole writer. Splits the manifest build in two. `snapshotLocked` resolves the emission order and copies each segment's metadata; `buildManifest` then works from that snapshot and touches no mutable writer state, so it needs no lock -- the dek, the splitter, the integrity algorithms and the signature encoding are all fixed at construction. `GetManifest` now releases the read lock as soon as the snapshot is taken. The snapshot copies `Segment` values rather than the `*Segment` pointers `w.segments` holds. `WriteSegment` mutates those in place when the archive accepts a write, so ranging over the pointers after releasing the lock would be a data race, not merely a stale read. `Finalize` deliberately keeps the write lock across the split. It is terminal -- no `WriteSegment` may succeed after it returns -- so there is no concurrency to preserve, and dropping the lock would open a window for a segment to reach the archive after the snapshot that fixes the manifest. This makes explicit a semantic that was already true: `GetManifest` returns a point-in-time view. A segment committed after the snapshot is absent from that manifest and present in the next one. The new test pins both halves, along with the property that motivated the change -- a `WriteSegment` issued while a split is in flight completes rather than blocking. It deadlocks to a 10s timeout against the previous locking and passes against this one. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./... -race ``` The new test deadlocks to its 10s timeout against the previous locking and passes against this one. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added experimental support for creating encrypted data in concurrent, out-of-order segments. - Added manifest previews, finalization controls, configurable metadata, MIME types, attributes, assertions, target modes, and key-splitting options. - Added validation and reconstruction checks for split encryption keys. - Added clearer errors for invalid segment operations and failed finalization. - **Bug Fixes** - Improved chunked writing responsiveness by preventing manifest generation from blocking concurrent segment writes. - Ensured manifests consistently reflect the segment state captured when generation begins. - Improved retry behavior after non-mutating finalization failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
…ap (opentdf#3945) > **Part 16 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-07-readfull`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes CreateTDF and CreateTDFContext took an io.ReadSeeker, so a caller with a pipe, a socket, or any other one-pass source had to spool the whole payload to disk or memory first. That is the block DSPX-2604 exists to remove: the Everfox re-wrap pipeline hands us a stream it cannot rewind. Both now take an io.Reader and consume it from its current position through EOF. Seekability was only ever used to measure the input. The length still matters, but it is now resolved rather than required: - WithInputSize(n) declares it outright, for a reader that cannot report it; - failing that, a reader that happens to implement io.Seeker is probed, and the cursor restored to wherever it was; - failing both, the payload is unmeasurable and is read until it ends. The one thing an unmeasurable payload gives up is the compact ZIP32 layout. The ZIP64 decision is baked into the payload's local file header, which is emitted ahead of the first segment, so it cannot be revisited once the archive has started; a payload that might exceed a 32-bit offset has to be written as ZIP64 from the outset. WithInputSize exists to buy that back — declaring the length of a piped payload keeps it in ZIP32 when it fits. The read loop no longer computes a segment count up front. It reads a buffer at a time until EOF, which is what makes an unknown length workable, and happens to be the same code path for a short final segment. An empty payload still produces one empty segment. The segment count is still passed to the archive writer when it is known, because that is what keeps a large declared count from being clamped to a one-segment capacity hint. Three behavior changes worth calling out. The first is the only one that can silently shorten a payload; the other two fail loudly. - A seekable reader is no longer rewound. The old code seeked to the end to measure and then back to byte 0, so it encrypted the whole file no matter where the caller had left the cursor. resolveInputSize saves the current position and restores that one, so the payload is whatever remains from there. Concretely: a caller that sniffs the first 512 bytes for content-type detection and then hands the same *os.File to CreateTDF used to get the whole file and now gets the file minus 512 bytes — no compile error, no runtime error, just a shorter TDF that looks complete. Reading from the current position is the right semantics for an io.Reader-shaped API, and `a seekable reader is encrypted from its current position` pins it, but a caller relying on the rewind has to seek to 0 itself now. - The 64 GB cap (maxFileSizeSupported/errFileTooLarge) is gone. It could only ever be enforced on a measurable payload, so keeping it would have meant `encrypt bigfile` failing where `encrypt < bigfile` succeeded. Both were unexported; nothing outside the package referenced them. - A declared size is exact, not an upper bound. A reader that reaches EOF early now fails the call with errInputShorterThanDeclared instead of returning a TDF that is silently short of the payload the caller asked to encrypt. Reading still stops at the declared size if the reader has more. What bounds a payload now, with the cap gone. maxPayloadSegments refuses a size needing more than MaxInt32 segments, but segmentCount is the only place it is enforced and that runs only when the length resolves — an unmeasurable stream has no segment ceiling. Underneath that, the real limit is manifest memory: one manifest segment and one archive-writer entry per segment, all live until finalize, roughly 500k entries per terabyte at the 2 MiB default. Neither is reachable today — 4 PiB at that segment size — so this is a note about where the wall moved to, not a regression. DSPX-4905 tracks bounding and measuring both. Testing: Test_CreateTDF_StreamingInput covers the three measurement modes across empty, sub-segment, exact-multiple, and partial-final-segment payloads, asserting the ZIP64 choice, the segment count, and a full round trip through LoadTDF. Test_CreateTDF_InputSizeBounds covers the negative, over-long, short, and mid-stream-start cases. Both guards were mutation-checked: removing the io.LimitReader fails "declared size bounds the read", and dropping the unknown-size ZIP64 rule fails every unmeasurable case. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./... -race ``` `Test_CreateTDF_StreamingInput` and `Test_CreateTDF_InputSizeBounds` are the new coverage. Both guards were mutation-checked: removing the `io.LimitReader` fails "declared size bounds the read", and dropping the unknown-size ZIP64 rule fails every unmeasurable case. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * TDF creation now supports streaming inputs, including non-seekable readers such as pipes and network streams. * Added `WithInputSize` to declare the exact payload size when it cannot be determined automatically. * Payloads are processed in segments, supporting unknown sizes and ZIP64 archives when needed. * **Bug Fixes** * Invalid or inaccurate declared sizes now result in clear errors, including when input ends too early. * Payloads exceeding the supported segment limit are rejected. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
> **Part 17 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-base-17`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes CreateTDFContext and the chunked writer had grown two full implementations of the same thing: build a policy, split the DEK, wrap each share to its KAS, encrypt segments, accumulate an aggregate hash, sign assertions, emit a manifest. Two copies of TDF construction is one too many — every spec change has to land twice, and the second copy is the one that gets forgotten. CreateTDF now delegates. It keeps the parts that are genuinely its own — the KAO template, autoconfigure, the input-size resolution and read loop from the previous commit — and hands each segment to the chunked writer, which owns manifest assembly for both paths from here on. The two paths differ in when key access is resolved, so that is what the writer is now parameterized on. The chunked writer defers to a KeySplitter at Finalize, because a caller may still be adding attributes while segments are in flight. CreateTDF cannot: it knows its attributes up front and wants an unreachable KAS to fail the call before a single payload byte reaches the output writer. Both are expressed as a keyAccessResolver, with the DEK now injectable so CreateTDF can wrap it ahead of time and hand the writer a staticKeyAccess. Everything downstream of that split is shared: resolvePolicyAndKeyAccess and buildKeyAccessObjects replace prepareManifest's inline loop and the chunked writer's buildChunkedPolicy/buildChunkedKeyAccessObjects, so both paths now emit byte-identical policy and key access for the same attributes. Two things fall out of the unification, both moving the chunked writer onto the shipped classic behavior: - With zero attributes the policy body's "dataAttributes" and "dissem" are now null rather than []. createPolicyObjectFromFQNs initializes them inside the attribute loop; the deleted buildChunkedPolicy did so unconditionally. The classic path has always emitted null here. - A KAS named by a split but missing a public key is still rejected rather than skipped (the check moved into buildKeyAccessObjects), but the error now names the missing PEM rather than the absent map entry — the two cases were indistinguishable in practice and only the outcome matters. The writer also gained an explicit segment size. It previously reported the first segment's actual length as defaultSegmentSize, which is only correct when every segment is full; a single-segment TDF would advertise a short default. CreateTDF knows the configured size and now says so. TDFObject loses aesGcm and payloadKey, which only ever existed to carry state between prepareManifest and the encrypt loop. Since opentdf#3940 (revised) fixed the chunked writer's root to HS256 and its segments to GMAC, there is nothing to plumb through here: CreateTDF stops passing TDFConfig.rootIntegrityAlg and segmentIntegrityAlg to the writer. Those were already the only values the defaults could hold and no exported option set either, so the manifest is unchanged. Deliberately not in this commit: removing enableEncryption, tdfFormat, readActionName, and the two now-unread integrity fields, which are dead but unrelated; they are a separate cleanup (opentdf#3947). Testing: the existing TDFSuite round trips pin byte-level output across segment sizes, target modes, and multi-KAS splits, and pass unchanged. Also verified against the streaming-input and input-size coverage added in the previous commit, the experimental chunked writer suite, and cross-module builds of examples, otdfctl, service, and tests-bdd. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./... -race ``` The existing `TDFSuite` round trips pin byte-level output across segment sizes, target modes and multi-KAS splits, and pass unchanged — that is the main assurance here. This touches key access construction on the shipped path, so it wants a cross-SDK run before merge. This is the branch to pin it to for the whole writer-delegation half of the stack: xtest drives the Go side through `otdfctl` → `SDK.CreateTDF`, and this is the first commit where that call reaches the chunked writer at all. ``` gh workflow run xtest.yml --repo opentdf/tests --ref main \ -f platform-ref=dspx-2604-17-createtdf-delegates \ -f otdfctl-ref=dspx-2604-17-createtdf-delegates \ -f java-ref=main -f js-ref=main ``` `otdfctl-ref` must name the branch, not `main` — otherwise the CLI is built against main's `sdk/` and the run passes without executing any of this. The job label should read `go@dspx-2604-17-createtdf-delegates`. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. Dispatch it against 17 or 20, never 15 on its own: xtest drives the Go side through `otdfctl` -> `SDK.CreateTDF`, and 17 is the first commit where that call reaches the rewritten writer. Set `otdfctl-ref` to the same branch as `platform-ref` -- it defaults to `main`, which builds the CLI against main's `sdk/` and makes the run vacuous. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * TDF creation now uses a consistent process for resolving key access and encrypting payload segments. * Key shares are validated and checked for successful reconstruction before payload data is written. * Existing encrypted output behavior is preserved, including segment sizing based on the configured size or the first segment when no size is configured. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
…gate Writer (opentdf#3944) > **Part 15 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-14-type-aliases`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes Deletes `experimental/tdf` TDF building logic, in favor of existing, more compliant logic found in root `sdk` package. The experimental writer built its own key access objects, and for EC KAS keys it built them wrong in three ways at once. It set keyType `"eccWrapped"`, but `service/kas/access/rewrap.go` dispatches on the exact string `"ec-wrapped"` and has no case for the other spelling. It derived the wrapping key with HKDF and then XORed the DEK, where the spec and every KAS expect AES-GCM under that derived key. And it omitted `schemaVersion` from the KAO entirely. Any TDF this package produced against an EC KAS was undecryptable, and nothing in the repo caught it because the package tested its own output against its own expectations. The fix is not a patch to that code but its deletion. `key_access.go` (-266) goes away and `Writer` delegates to `sdk.NewChunkedWriter`, so key access objects come from `sdk.createKeyAccess` -- the same code path `SDK.CreateTDF` has always used and that the cross-SDK tests exercise. RSA, EC, ML-KEM and hybrid wrapping now have exactly one implementation. `key_access_test.go` (-652) goes with it; equivalent coverage against the sdk functions landed earlier in this stack, so nothing is lost. `writer.go` drops from 680 lines to ~292: `Writer` becomes its config plus an inner `sdk.ChunkedWriter` and a `finalized` flag. Manifest assembly, segment encryption, integrity hashing and assertion signing all move to the one implementation. `manifest.go` sheds the `calculateSignature` copy and the three constants that only its callers needed. `keysplit_adapter.go` (+60) is why this is a delegation rather than a rename. `sdk.DefaultKeySplitter` is single-KAS and ignores attributes; `keysplit.XORSplitter` evaluates the full ABAC boolean expression and XOR-splits the DEK across every KAS the resulting clauses require. The two result shapes are field-identical, so the adapter is a straight copy. The one structural mismatch is where the default KAS enters -- sdk passes it per `Split` call, keysplit takes it at construction -- so the splitter is built inside `Split`. API changes callers will notice `Finalize` error values are aliases of their sdk counterparts rather than copies, so `errors.Is` matches under either name. `WithSegments` no longer requires a contiguous prefix starting at 0. Indices may be sparse -- a caller mapping fixed index blocks onto S3 multipart uploads writes gaps by construction -- but must still name written segments in ascending order and may only drop from the end, because that is the order the payload is laid out in. `WithExcludeVersionFromManifest` is deprecated. It was always a no-op: the manifest builder never read the flag. Omitting `schemaVersion` is how a reader is told the TDF predates 4.3.0, and such a reader then expects hex-then-base64 signatures, which are decided per segment at write time, long before Finalize sees the option. `WithTargetMode` sets both together and is the replacement. `WithIntegrityAlgorithm` and `WithSegmentIntegrityAlgorithm` no longer take effect, and asking for anything but the default is now an error from `NewWriter` rather than a silent substitution. The other variants were unsupported and resulted in incompatible TDFs/ ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` cd sdk && go test ./... -race cd examples && go build ./... ``` This changes the KAS wire format for EC keys, so it wants a cross-SDK run before merge. Pin it to opentdf#3946, not to this branch — that is the first point where `CreateTDF` goes through the changed code, and `otdfctl` is the only Go consumer xtest drives: ``` gh workflow run xtest.yml --repo opentdf/tests --ref main \ -f platform-ref=dspx-2604-17-createtdf-delegates \ -f otdfctl-ref=dspx-2604-17-createtdf-delegates \ -f java-ref=main -f js-ref=main ``` `otdfctl-ref` must name the branch too: it defaults to `main`, which builds the CLI against main's `sdk/` and makes the run vacuous. Check the job label reads `go@<branch>` rather than `go@main`. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. Dispatch it against 17 or 20, never 15 on its own: xtest drives the Go side through `otdfctl` -> `SDK.CreateTDF`, and 17 is the first commit where that call reaches the rewritten writer. Set `otdfctl-ref` to the same branch as `platform-ref` -- it defaults to `main`, which builds the CLI against main's `sdk/` and makes the run vacuous. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an option to target specific TDF specification versions. * Added support for XOR-based key splitting in the experimental TDF writer. * **Bug Fixes** * Unsupported integrity algorithms are now rejected when creating a writer; GMAC is the only supported segment integrity algorithm. * Improved compatibility of writer errors with the stable SDK. * **Documentation** * Updated guidance for sparse segments, finalized output, integrity algorithms, architecture, and thread safety. * **Deprecations** * `WithExcludeVersionFromManifest` is deprecated and has no effect. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
…um (opentdf#3947) > **Part 18 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-17-createtdf-delegates`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes TDFConfig.enableEncryption was set to true at construction and never read again; nothing could turn it off and nothing branched on it. tdfFormat was likewise fixed at JSONFormat forever. readActionName was a leftover constant with no references. All three are gone. rootIntegrityAlg and segmentIntegrityAlg go with them. opentdf#3940 (revised) fixed the chunked writer's root to HS256 and its segments to GMAC, and opentdf#3946 made CreateTDF delegate to it, so nothing reads either field any more. No exported option ever set one, and the defaults they held were exactly those two algorithms, so the manifest is byte-identical. They are unexported, so they are deleted outright rather than deprecated. `TestIntegrityAlgDefaults` asserted on those fields and could not survive them. The invariant it guarded is now asserted on a manifest `CreateTDF` actually produced — `TDFSuite.testEncrypt` checks `manifest.Algorithm` is HS256 and `manifest.SegmentHashAlgorithm` is GMAC — so every encrypt case in the suite carries it rather than one test reading back a struct field. TDFFormat, JSONFormat, and XMLFormat are exported, so they are deprecated rather than deleted. XML manifests were never implemented and the enum has no remaining consumer inside the SDK. ### Checklist - [x] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` make build && make test ``` Nothing reads any of the removed fields; the check is that the tree still builds across `sdk`, `service`, `otdfctl`, `examples` and `tests-bdd`, and that `TDFSuite` still emits HS256/GMAC on every encrypt case. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Policy creation now returns a validation error when supplied with an empty fully qualified name, rather than proceeding with invalid input. * **Configuration** * TDF configuration no longer exposes settings for enabling encryption, choosing a format, or selecting root and segment integrity algorithms. The format constants are marked deprecated; XML format is documented as never implemented. Existing applications that set these options may need to update their configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
pflynn-virtru
pushed a commit
to pflynn-virtru/platform
that referenced
this pull request
Sep 30, 2026
> **Part 19 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-base-19`. > > This stack replaces opentdf#3782 / opentdf#3865 / opentdf#3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes 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. A file is still opened seekably, on purpose. The SDK measures a seekable payload and keeps the archive in the compact ZIP32 layout; an unmeasurable one 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` is 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. 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 same reader for a seekable input, rewound; the sniffed prefix pushed back with io.MultiReader for anything else. A megabyte in memory at most, and only when --mime-type was not given. Testing: TestDetectMimeTypePreservesThePayload now runs each case twice, once over a reader whose Seek method is hidden, and asserts the payload arrives whole either way. TestDetectMimeTypeKeepsSeekability guards the ZIP32 layout directly, since nothing else would fail if a file came back wrapped. 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. ### Checklist - [x] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions ``` cd otdfctl && go test ./... -race ``` e2e, against a running platform: ``` cd otdfctl && bats e2e/streaming.bats ``` The file is tagged `payload_streaming` and runs in its own pass ahead of the parallel batch — see opentdf#3939 for why that ordering is load-bearing (the platform base key `key-base.bats` sets cannot be un-set, and breaks every unattributed encrypt scheduled after it). The case to look at is `encrypt measures a file and streams a pipe`, which asserts the ZIP layout rather than the round trip: a file must stay ZIP32 (extra field length 0 at offset 28 of the payload's local file header) and a pipe must be ZIP64 (non-zero). A quiet return to spooling would show up nowhere else, since both forms round-trip fine. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | opentdf#3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | opentdf#3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | opentdf#3932 fix(sdk): reject a zipstream write set that omits segment 0 | opentdf#3931 | | 04 | opentdf#3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | opentdf#3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | opentdf#3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | opentdf#3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | opentdf#3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | opentdf#3938 fix(cli): stream encrypt instead of buffering the whole payload | opentdf#3937 | | 10 | opentdf#3939 fix(cli): stream decrypt and inspect instead of buffering | opentdf#3938 | | 11 | opentdf#3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = opentdf#3932 + opentdf#3934 + opentdf#3935 | | 12 | opentdf#3941 fix(sdk): stop GetManifest from splitting the key under the lock | opentdf#3940 | | 13 | opentdf#3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | opentdf#3941 | | 14 | opentdf#3943 chore(sdk): alias experimental/tdf manifest and assertion types | opentdf#3942 | | 15 | opentdf#3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | opentdf#3943 | | 16 | opentdf#3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | opentdf#3936 | | 17 | opentdf#3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = opentdf#3944 + opentdf#3945 | | 18 | opentdf#3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | opentdf#3946 | | 19 | opentdf#3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = opentdf#3947 + opentdf#3939 | | 20 | opentdf#3949 feat(sdk): graduate the chunked writer to stable API | opentdf#3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Encryption now supports piped and other non-seekable input, including larger streams. * Inputs that cannot be measured in advance produce ZIP64 output; seekable files continue to use ZIP32 when applicable. * Regular-file redirects can be read directly without being copied to a temporary file. * Decryption now supports TDFs supplied through pipes and redirects. * Encrypted output remains compatible with the existing decryption flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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
2 participants
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.
Proposed Changes
otdfctl encryptandotdfctl decryptheld the entire plaintext and the entireciphertext in memory at once — peak RSS ran ~3.6x the payload, so a large enough file
OOMs on a machine with plenty of disk for it. The SDK already streams; this was purely a
CLI-layer choice. This makes memory bounded by segment size instead of payload size.
Two commits, each building and testing on its own:
fix(otdfctl): stream encrypt instead of buffering whole payloadhandlers.EncryptBytes→Encrypt(ctx, out io.Writer, in io.Reader, EncryptOptions),taking an
io.Readernow thatCreateTDFno longer requires a seeker (refactor(sdk): rewrite CreateTDF on top of ChunkedWriter #3865).bufio.Peek, which does not consume,rather than inspecting a full in-memory copy.
*os.File'sSeeker, which would push the SDK onto its unknown-sizepath and force ZIP64. The stat size is passed via
WithInputSizefor regular files sooutput stays comparable with the buffered implementation. Piped input is legitimately ZIP64.
partial
.tdf.fix(otdfctl): stream decrypt and inspect instead of buffering whole payloadhandlers.DecryptBytes→Decrypt(ctx, out, in, DecryptOptions);InspectTDFtakes anio.ReadSeeker.decryptpreviously made a third whole-payload copy viafmt.Print;inspectread an entire TDF just to reach its manifest.io.Copyselectssdk.Reader'sWriteTo, which decrypts one segment at a time. Acompile-time
var _ io.WriterTo = (*sdk.Reader)(nil)assertion guards that — withoutWriteTo,io.Copysilently falls back toRead/ReadAtand re-buffers the wholepayload with no test failure to show for it.
file argument is opened directly; piped stdin is spooled to a temp file, trading disk for
the memory the old read used.
MaxFileSizeCLI cap — it existed to bound RAM, and the SDK's real limitnow applies.
pkg/cli/pipe.go, whose three whole-file readersinspectwas the last callerof.
ReadFromFilehad an unboundedio.ReadAllwith no cap at all.cli.ExitWithErrorcallsos.Exitand skips deferred functions, so temp-file cleanup isalso invoked explicitly on every error path — and in
inspect, on the success path too,since every exit there goes through
os.Exit.Checklist
Unit tests cover the new output-file, pipe-reader, and spool helpers in
otdfctl/cmd/tdf/tdf_test.go. Nine cases were added tootdfctl/e2e/encrypt-decrypt.batscovering pipe round-trips, empty-stdin rejection, no-partial-output-on-failure, and a
GNU-
time-guarded peak-RSS bound on a 1 GiB payload.spec/DSPX-4499.mdis filled in from the ticket. No flags changed, so nodocs/manupdates were needed.
Testing Instructions
Measured against a local platform stack, peak RSS is now flat in payload size:
Both round-trips are byte-identical (
cmpclean). A file input still produces a non-ZIP64archive; piped input is ZIP64, since the size cannot be known before the first segment
header is written.
To reproduce:
Cross-SDK e2e passed on this branch:
opentdf/tests run 32893257650
— 7 legacy + 68 standard + 88 ABAC tests ran (confirmed not
SKIPPED) against java@mainand js@main.
Refs DSPX-4499