chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt - #3935
Conversation
github-actions
Bot
added
the
comp:sdk
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds SDK tests for RSA and EC key-access creation, metadata encryption behavior, invalid keys, and deterministic TDF salt generation. ChangesSDK key access and encryption validation
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This change adds regression coverage for SDK key access, metadata encryption, and TDF salt behavior without modifying runtime implementation. No current merge-blocking risk is identified. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
96172ef to
7537812
Compare
dmihalcik-virtru
changed the title
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
… tdfSalt
createKeyAccess builds the manifest key access object and picks the
wrapping scheme from the KAS key algorithm. Today the only thing
exercising it is TDFSuite, which asserts that a round trip against its
own fake KAS succeeds -- so it cannot pin which KAO fields are populated,
cannot catch an error path that silently returns a zero value, and says
nothing about encryptMetadata or tdfSalt, which have no direct coverage.
Adds, test-only:
- the RSA KAO field shape, including the deliberately empty
ephemeralPublicKey and the schemaVersion the KAS dispatches on
- empty and malformed KAS public keys are rejected
- an EC-wrapped DEK unwrapped the way service/kas/access/rewrap.go
does it, so the envelope is checked to be AES-GCM over the HKDF
output rather than merely tagged "ec-wrapped"
- encryptMetadata's envelope shape, key sensitivity, empty input, and
unusable key
- tdfSalt determinism and length
The three hybrid KEM schemes already round-trip in tdf_hybrid_test.go
and are not repeated.
Landing this ahead of the DSPX-2604 work that deletes the duplicate
key-wrapping implementation in sdk/experimental/tdf makes that deletion
checkable as a de-duplication rather than a coverage loss.
Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
7537812 to
35f0532
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
sujankota
left a comment
sujankota
left a comment
There was a problem hiding this comment.
Reviewed the diff statically against main (I did not check out the branch or run the tests). This looks good to me — the tests are meaningfully stronger than what they replace, and everything I spot-checked holds up. Comments below are non-blocking follow-ups.
What I verified independently:
- The
tdfSalt()hex pinaa17cf44…8197is exactlysha256("TDF"). - The EC unwrap really does mirror the KAS — see the inline note on
TestCreateKeyAccessECUnwrap. - Both bad-key cases genuinely error (
pem.Decodefails insideocrypto.FromPublicPEM,lib/ocrypto/asym_encryption.go:73), andencryptMetadata([]byte{}, …)errors viaaes.NewCipher'sKeySizeError. - No identifier collisions with the existing
sdkpackage tests, despite sharing constant names withexperimental/tdf/key_access_test.go.
One nit on the PR description rather than the code: it says the test covers "tdfSalt determinism and length", but the hex pin is strictly stronger than either. The description undersells it.
| assert.Equal(t, keyAccessSchemaVersion, kao.SchemaVersion) | ||
| assert.Equal(t, "encrypted-metadata", kao.EncryptedMetadata) | ||
| assert.Equal(t, PolicyBinding{Alg: "HS256", Hash: "test-binding"}, kao.PolicyBinding) | ||
| assert.NotEmpty(t, kao.WrappedKey) |
There was a problem hiding this comment.
The RSA path is asserted but never round-tripped: this checks a non-empty string was produced, whereas TestCreateKeyAccessECUnwrap below proves the KAS can actually recover the DEK. The asymmetry exists because testRSAPublicKey is a bare public key with no matching private key.
The fixture to close it already exists in this package — sdk/tdf_test.go:91 defines mockRSAPublicKey1 (a CERTIFICATE PEM, which FromPublicPEM handles via getPublicPart) alongside mockRSAPrivateKey1 at sdk/tdf_test.go:108. Switching to that pair would delete the new 9-line constant and let this test decrypt kao.WrappedKey back to symKey, giving RSA the same strength as EC.
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| _, err := createKeyAccess( | ||
| KASInfo{URL: testKAS1URL, PublicKey: tc.pubKey, Algorithm: "rsa:2048"}, |
There was a problem hiding this comment.
Two gaps in this table, the second more interesting than the first:
-
Only the RSA branch is exercised. The EC and KEM branches parse
PublicKeythrough their own paths and have no malformed-input case. -
There's a silent-failure path here that's squarely in this PR's stated scope.
createKeyAccess(sdk/tdf.go:679) dispatches onkasInfo.Algorithm, butgenerateWrapKeyWithRSA→ocrypto.FromPublicPEMdispatches on the PEM contents. Pass an EC PEM withAlgorithm: "rsa:2048"— or an emptyAlgorithm, plausible if a KAS omits it — and the default branch returns anECEncryptor(lib/ocrypto/asym_encryption.go:112-117). The result is a KAO carrying an ECIES envelope withKeyTypestill"wrapped"andEphemeralPublicKeyempty. Encrypt succeeds; rewrap fails much later with nothing pointing back here.
That's exactly the "error path that silently returns a zero value" the description targets. Worth a case pinning the current behaviour, or a bug filed if it should be rejected outright.
| // TestCreateKeyAccessECUnwrap wraps a DEK to an EC KAS key and then unwraps it | ||
| // exactly the way service/kas/access/rewrap.go does for "ec-wrapped". | ||
| // Asserting only on KeyType would not catch the envelope being, say, a raw XOR | ||
| // of the HKDF output instead of AES-GCM. |
There was a problem hiding this comment.
Confirming the claim in this comment, since it's the load-bearing one: service/kas/access/rewrap.go:688-755 runs the identical GetECKeySize → ECSizeToMode → pem.Decode → ParsePKIXPublicKey → CompressedECPublicKey sequence, and service/internal/security/standard_crypto.go:478 calls ocrypto.NewSaltedECDecryptor(sk, TDFSalt(), nil) with the same salt. This is faithful, and it's the strongest test in the file.
|
|
||
| priv, err := ocrypto.ECPrivateKeyFromPem([]byte(kasPrivPEM)) | ||
| require.NoError(t, err) | ||
| dec, err := ocrypto.NewSaltedECDecryptor(priv, tdfSalt(), nil) |
There was a problem hiding this comment.
Minor: this uses the tdfSalt() under test on both sides of the round trip, so a change to the salt would move the test with the code rather than fail it. TestTdfSalt pins the value independently, so the coverage is there overall — just noting the two tests are load-bearing together.
|
|
||
| ciphertext, err := ocrypto.Base64Decode([]byte(encMeta.Cipher)) | ||
| require.NoError(t, err) | ||
| require.GreaterOrEqual(t, len(ciphertext), ocrypto.GcmStandardNonceSize) |
There was a problem hiding this comment.
The real floor is nonce + GCM tag (12 + 16 = 28), not the nonce alone — a truncated or tagless envelope would pass this.
|
|
||
| func TestTdfSalt(t *testing.T) { | ||
| assert.Equal(t, | ||
| "aa17cf44585fe15fd634c27b9512d842b42af1bac6178d92161edb4e2abf8197", |
There was a problem hiding this comment.
Worth a comment noting this is sha256("TDF"), so whoever next sees this fail can regenerate the value rather than copy it out of the diff.
> **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>
3 participants
Proposed Changes
createKeyAccess builds the manifest key access object and picks the
wrapping scheme from the KAS key algorithm. Today the only thing
exercising it is TDFSuite, which asserts that a round trip against its
own fake KAS succeeds -- so it cannot pin which KAO fields are populated,
cannot catch an error path that silently returns a zero value, and says
nothing about encryptMetadata or tdfSalt, which have no direct coverage.
Adds, test-only:
ephemeralPublicKey and the schemaVersion the KAS dispatches on
does it, so the envelope is checked to be AES-GCM over the HKDF
output rather than merely tagged "ec-wrapped"
unusable key
The three hybrid KEM schemes already round-trip in tdf_hybrid_test.go
and are not repeated.
Landing this ahead of the DSPX-2604 work that deletes the duplicate
key-wrapping implementation in sdk/experimental/tdf makes that deletion
checkable as a de-duplication rather than a coverage loss.
Checklist
Testing Instructions
The full DSPX-2604 stack — 20 PRs
mainmainmainmainmainmainmaindspx-2604-base-11= #3932 + #3934 + #3935dspx-2604-base-17= #3944 + #3945dspx-2604-base-19= #3947 + #3939Reviewable in parallel right now, since they sit directly on
mainand depend onnothing 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 emptymerge 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
mainat 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 verifytiming out onhttps://golangci-lint.run/.../golangci.v2.8.jsonschema.json(fails the wholego (<module>)job and fail-fast cancels its siblings), the bats installer getting a 403,Docker Hub timing out on
keycloak/keycloak:26.4, andbufreporting "the serverhosted at that remote is unavailable" while the Java SDK generates sources. The
govulncheckstep also emits##[error]annotations against the go1.25.11 stdlib, butit is
continue-on-error: trueand never fails a job — 01 bumps the toolchain andclears those annotations.
Summary by CodeRabbit