chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt - #3935
Open
dmihalcik-virtru wants to merge 3 commits into
Open
chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt#3935dmihalcik-virtru wants to merge 3 commits into
dmihalcik-virtru wants to merge 3 commits into
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 |
This was referenced Sep 1, 2026
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
force-pushed
the
dspx-2604-06-key-access-tests
branch
from
September 1, 2026 03:36
96172ef to
7537812
Compare
dmihalcik-virtru
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
… 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>
dmihalcik-virtru
force-pushed
the
dspx-2604-06-key-access-tests
branch
from
September 3, 2026 14:13
7537812 to
35f0532
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
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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