Repository navigation
feat(xtest): add ML-KEM rewrap session-key roundtrip test - #571
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1
✨ Finishing Touches: 💡 1
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. A rabbit sets the session key, Comment |
This was referenced Jul 31, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtest/tdfs.py (1)
578-596: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine precedence for
ecwrapvssession_key_algorithm.
SDK.decrypt()can pass both options, but the Go, Java, and JavaScript CLI shims emit them in separate order. This lets the CLI see two rewrap/session-key values and either fail or pick one implicitly. Reject the combination or emit one option with explicit precedence, such assession_key_algorithmoverridingecwrap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/tdfs.py` around lines 578 - 596, Update the option construction in the decrypt helper around session_key_algorithm and ecwrap to define deterministic precedence when both are provided. Reject the combination explicitly or emit only one setting, with session_key_algorithm taking precedence over ecwrap, and ensure the generated environment cannot contain conflicting rewrap/session-key values.
🧹 Nitpick comments (2)
spec/DSPX-4221.md (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
client-generatedin the session-key description.Replace
client generated key pairwithclient-generated key pair.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/DSPX-4221.md` at line 19, Update the session encryption key description in DSPX-4221 to use the hyphenated phrase “client-generated key pair” instead of “client generated key pair,” without changing the surrounding requirement.Source: Linters/SAST tools
xtest/sdk/go/cli.sh (1)
209-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
[[in both new Bash conditionals.SonarCloud reports the same conditional-style issue at both sites.
xtest/sdk/go/cli.sh#L209-L211: replace[ -n "$XT_WITH_SESSION_KEY_ALGORITHM" ]with[[ -n "$XT_WITH_SESSION_KEY_ALGORITHM" ]].xtest/sdk/java/cli.sh#L198-L200: replace[ -n "$XT_WITH_SESSION_KEY_ALGORITHM" ]with[[ -n "$XT_WITH_SESSION_KEY_ALGORITHM" ]].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/sdk/go/cli.sh` around lines 209 - 211, Update the XT_WITH_SESSION_KEY_ALGORITHM condition in xtest/sdk/go/cli.sh at lines 209-211 and xtest/sdk/java/cli.sh at lines 198-200 to use Bash [[ -n ... ]] syntax instead of single brackets, preserving the existing argument-appending behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtest/tdfs.py`:
- Around line 152-156: Make ML-KEM session-key capability detection
algorithm-specific across xtest/tdfs.py:152-156 and 252-254,
xtest/sdk/go/cli.sh:119-123, xtest/sdk/java/cli.sh:129-133,
xtest/sdk/js/cli.sh:104-108, and xtest/test_pqc.py:379-403: define or expose
distinct capabilities for mlkem:768 and mlkem:1024, include mlkem:1024 in
fallback handling while preserving exact algorithm results, have each SDK probe
and report only the requested algorithm, and gate each parameterized PQC test
with its matching capability.
In `@xtest/test_pqc.py`:
- Around line 399-400: Update the pytest.skip call in the focus-filter logic to
provide an actionable reason, including the excluded encrypt_sdk and decrypt_sdk
values or clearly describing the focus rule; preserve the existing skip
condition.
- Line 421: Update the filecmp.cmp assertion in the decrypted-output test to
pass shallow=False, ensuring pt_file and rt_file are compared by byte content
rather than metadata alone.
---
Outside diff comments:
In `@xtest/tdfs.py`:
- Around line 578-596: Update the option construction in the decrypt helper
around session_key_algorithm and ecwrap to define deterministic precedence when
both are provided. Reject the combination explicitly or emit only one setting,
with session_key_algorithm taking precedence over ecwrap, and ensure the
generated environment cannot contain conflicting rewrap/session-key values.
---
Nitpick comments:
In `@spec/DSPX-4221.md`:
- Line 19: Update the session encryption key description in DSPX-4221 to use the
hyphenated phrase “client-generated key pair” instead of “client generated key
pair,” without changing the surrounding requirement.
In `@xtest/sdk/go/cli.sh`:
- Around line 209-211: Update the XT_WITH_SESSION_KEY_ALGORITHM condition in
xtest/sdk/go/cli.sh at lines 209-211 and xtest/sdk/java/cli.sh at lines 198-200
to use Bash [[ -n ... ]] syntax instead of single brackets, preserving the
existing argument-appending behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05ddc315-d152-4758-ad81-2a05754b1f38
📒 Files selected for processing (6)
spec/DSPX-4221.mdxtest/sdk/go/cli.shxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_pqc.py
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
July 31, 2026 22:18
baacb54 to
66d032f
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
August 1, 2026 11:37
66d032f to
56a9e8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtest/sdk/go/cli.sh`:
- Line 209: Update the new XT_WITH_SESSION_KEY_ALGORITHM conditionals to use
Bash [[ -n ... ]] syntax in xtest/sdk/go/cli.sh lines 209-209 and
xtest/sdk/java/cli.sh lines 200-200, preserving the existing non-empty check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85a7130d-9c13-48e1-b7df-a64fb5ca9b08
📒 Files selected for processing (6)
xtest/audit_logs.pyxtest/sdk/go/cli.shxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_pqc.py
🚧 Files skipped from review as they are similar to previous changes (3)
- xtest/sdk/js/cli.sh
- xtest/tdfs.py
- xtest/test_pqc.py
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
2 times, most recently
from
August 1, 2026 13:22
9ffd4d6 to
1f7c8b2
Compare
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
August 1, 2026 14:30
1f7c8b2 to
0cefcf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/DSPX-4221.md`:
- Line 53: Update the session-key algorithm statement in DSPX-4221 to match the
existing test coverage by removing “and vice versa” from the independence claim;
do not add a reverse-combination test.
- Around line 34-36: Update test_session_key_mlkem_roundtrip readiness gating in
tdfs.py to require a session-key-specific ML-KEM capability probe or an explicit
platform build that accepts ML-KEM clientPublicKey, rather than relying solely
on the shared session-key-mlkem flag. Preserve the test’s existing audit-log
sessionKeyType assertion once the capability check passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee99aa8a-c8d3-462e-85ec-725886e64223
📒 Files selected for processing (7)
spec/DSPX-4221.mdxtest/audit_logs.pyxtest/sdk/go/cli.shxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_pqc.py
🚧 Files skipped from review as they are similar to previous changes (6)
- xtest/sdk/js/cli.sh
- xtest/sdk/go/cli.sh
- xtest/test_pqc.py
- xtest/sdk/java/cli.sh
- xtest/audit_logs.py
- xtest/tdfs.py
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
2 times, most recently
from
August 1, 2026 15:03
398f300 to
17eda9c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end xtest coverage for ML-KEM as the client-generated rewrap session key, including verification via KAS rewrap audit logs so tests assert the negotiated session-key type (not just successful decrypt).
Changes:
- Add
session_key_algorithmplumbing totdfs.SDK.decrypt()viaXT_WITH_SESSION_KEY_ALGORITHM, and wire it through Go/Java/JS CLI wrappers (plus SDK capability probes). - Extend audit-log parsing/assertion utilities to match on
eventMetaData.sessionKeyType. - Add new PQC tests covering ML-KEM-768/1024 session-key roundtrips and audit-log verification; update existing EC rewrap audit assertion to pin the negotiated session-key type.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| xtest/test_tdfs.py | Tightens EC rewrap audit assertion to check session_key_type. |
| xtest/test_pqc.py | Adds ML-KEM session-key roundtrip test (768/1024) plus RSA session-key audit-field sanity test. |
| xtest/tdfs.py | Introduces session-key-mlkem feature flag and threads session_key_algorithm into decrypt via env var. |
| xtest/sdk/go/cli.sh | Wires XT_WITH_SESSION_KEY_ALGORITHM to --session-key-algorithm and adds a supports session-key-mlkem probe. |
| xtest/sdk/java/cli.sh | Wires XT_WITH_SESSION_KEY_ALGORITHM to --rewrap-key-type and adds a supports session-key-mlkem probe. |
| xtest/sdk/js/cli.sh | Wires XT_WITH_SESSION_KEY_ALGORITHM to --rewrapKeyType and adds a supports session-key-mlkem probe. |
| xtest/audit_logs.py | Adds ParsedAuditEvent.session_key_type and session_key_type filtering to rewrap assertions. |
| spec/DSPX-4221.md | Documents DSPX-4221 scope/acceptance criteria and links companion PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
2 times, most recently
from
August 3, 2026 12:53
fb0e5a9 to
3bff44e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtest/audit_logs.py (1)
1395-1459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImprove diagnostics on the final mismatch assertion.
When the fallback path detects a real type mismatch, the final
assert expected in reported, ...at Line 1450-1452 raises a plainAssertionErrorwith only the expected and reported sets. Every other assertion inAuditLogAsserter(for exampleassert_rewrapat Line 1358) routes through_raise_assertion_error, which includes matching/recent log lines and clock-skew context. Losing that context here makes debugging a real cross-service session-key mismatch harder in CI.Include the raw matching events in the failure message so a real mismatch is as easy to diagnose as any other rewrap assertion failure.
♻️ Proposed diagnostic improvement
- assert expected in reported, ( - f"Expected rewrap session_key_type={expected!r}, but platform reported {reported!r}" - ) + if expected not in reported: + event_lines = "\n".join( + f" [{e.timestamp}] {e.raw_entry.raw_line}" for e in events + ) + raise AssertionError( + f"Expected rewrap session_key_type={expected!r}, but platform reported " + f"{reported!r}.\nMatching successful rewrap events:\n{event_lines}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtest/audit_logs.py` around lines 1395 - 1459, Update the final mismatch assertion in assert_rewrap_session_key_type to include the raw matching events from the fallback lookup, alongside the expected and reported session-key types. Preserve the existing platform-field absence warning and ensure the failure message exposes enough event detail for diagnosing cross-service mismatches, consistent with the diagnostic context provided by _raise_assertion_error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xtest/audit_logs.py`:
- Around line 1395-1459: Update the final mismatch assertion in
assert_rewrap_session_key_type to include the raw matching events from the
fallback lookup, alongside the expected and reported session-key types. Preserve
the existing platform-field absence warning and ensure the failure message
exposes enough event detail for diagnosing cross-service mismatches, consistent
with the diagnostic context provided by _raise_assertion_error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 797400c8-5442-438c-bd16-881c678e7a8a
📒 Files selected for processing (8)
spec/DSPX-4221.mdxtest/audit_logs.pyxtest/sdk/go/cli.shxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_pqc.pyxtest/test_tdfs.py
🚧 Files skipped from review as they are similar to previous changes (5)
- xtest/sdk/java/cli.sh
- xtest/sdk/js/cli.sh
- xtest/sdk/go/cli.sh
- spec/DSPX-4221.md
- xtest/tdfs.py
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
2 times, most recently
from
August 3, 2026 16:34
3f3bc36 to
c7bfda5
Compare
X-Test Failure Report |
|
dmihalcik-virtru
added a commit
that referenced
this pull request
Aug 17, 2026
## Problem `test_tdf_with_malicious_kao` rewrites a KAO's `url` to an unreachable KAS and expects the decrypt to fail. It hardcoded `http://localhost:8585`, with the comment `# nothing running at 8585` — true when it was written, but `otdf-local` later assigned that port to **km1** ([`otdf-local/src/otdf_local/config/ports.py:23`](../blob/main/otdf-local/src/otdf_local/config/ports.py#L23)). That matters because the test tolerates two different failure modes: - **go** screens the URL against its KAS allowlist and never connects, so its error says `allowlist`. It passes either way. - **js** (v0.20.0) doesn't, and falls through to a real request. This is why `AggregateError` — what Node's `fetch` raises for a refused connection — is one of the accepted alternatives in the assertion. With km1 listening on 8585, that request gets an *answer* instead of a refusal: ``` POST http://localhost:8585/malicious/kas/v2/rewrap => 404 Not Found NetworkError ``` which matches none of `allowlist|not allowed|disallowed KASes|AggregateError`, so both js-decrypt parametrizations fail: ``` FAILED test_tdfs.py::test_tdf_with_malicious_kao[small-go@v0.36.0-js@v0.20.0-in_focus0] FAILED test_tdfs.py::test_tdf_with_malicious_kao[small-js@v0.20.0-js@v0.20.0-in_focus0] ``` The failure is environmental — it reproduces against any `otdf-local` stack, and not in CI, where nothing binds 8585. ## Fix Move the URL to port 9999, lift it into a `MALICIOUS_KAS_URL` constant, and replace the stale comment with the actual constraint plus a pointer to the port map, so a future port assignment doesn't silently reclaim it. ## Testing Against a local `otdf-local` platform: ``` uv run pytest test_tdfs.py::test_tdf_with_malicious_kao --sdks "go js" -v ``` `4 passed in 11.28s` — including the two js-decrypt combinations that previously failed. Found while verifying #571; split out because it's unrelated to that change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated test configuration to use the revised local endpoint for malicious service scenarios. * Improved consistency by centralizing the endpoint value used during testing. * **User Impact** * No user-facing functionality or behavior changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
2 times, most recently
from
October 14, 2026 20:26
62f0783 to
e40db95
Compare
X-Test Failure Report |
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 15, 2026
## Summary java-sdk releases don't attach `cmdline.jar` to their GitHub Releases, and `cmdline` isn't published to Maven Central. So every Java release that xtest uses has been built from source with a full `mvn clean install`. That build's `sdk` module runs `buf generate` against buf.build. With a dozen xtest jobs doing this at the same moment, BSR returns `resource_exhausted: too many requests` and fails the run (e.g. [38632037584](https://github.com/opentdf/tests/actions/runs/38632037584) on #571). This is DSPX-5177. Everything `cmdline` depends on (`sdk-pom`, `sdk`, `sdk-pqc-bc`, `sdk-fips-bc`) **is** on Maven Central. This PR builds just the CLI against those published jars. ### Changes (`otdf-sdk-mgr`) - **`install_java_release`:** when the GitHub Release has no `cmdline.jar`, the installer checks Maven Central for `io.opentdf.platform:sdk:<version>`. If it's there, it shallow-clones the tag into a temp dir and runs `mvn -pl .,cmdline package`, so `sdk*` resolves from Central and **no codegen or BSR calls happen**. - The temp dir is outside `src/` so the java `Makefile` doesn't rebuild it. - The build uses its own local repo, `~/.m2/otdf-sdk-mgr-releases`. java-sdk `main` carries the last release's version (currently `0.19.1`), so an `sdk` that a head build `mvn install`ed into `~/.m2/repository` would otherwise shadow the release. I hit this locally: the v0.19.1 `cmdline` failed to compile against `main`'s sdk. - Any failure (not on Central, clone or build error) still raises `InstallError`, so `setup-cli-tool` falls back to today's full source build. - **`versions resolve java latest`:** no longer returns `head: true` when no release has a `cmdline.jar` asset. It's now an ordinary release row and takes the path above. - Behaviour change: in platform-head cells, `latest` is no longer rebuilt against the platform under test via `PLATFORM_BRANCH`. It's tested as published. - **`install stable java`:** drops the `has_cli` filter, which had made it skip Java entirely. No workflow changes: JDK and Maven are already set up before `setup-cli-tool` in every job that installs Java. ## Expected effect In a run like 38632037584, each job's v0.19.1 build no longer calls BSR. Combined with opentdf/java-sdk#419 (which stops `attach-sources` from running codegen a second time), BSR traffic per run drops from about 96 to about 24 `buf generate` passes. Only real heads still build from source. Building each head once per run is a separate follow-up. ## Testing - Maven Central has `sdk` for every release since 0.7.2 except 0.8.0 and 0.17.0. Those fall back to a source build. - `mvn -pl .,cmdline package` checked against an empty local repo: - v0.19.1 (`latest`): builds, zero `buf` calls. - v0.9.0 (LTS): builds, zero `buf` calls, with Maven 3.9.11 as on the runners. Maven 3.10 rejects that tag's `sdk/pom.xml` for a duplicate plugin declaration in the `fuzz` profile. - End to end: `otdf-sdk-mgr install artifact --sdk java --version v0.19.1 --dist-name v0.19.1` takes the new path, and `cmdline.jar --version` reports `{"version":"0.19.1","tdfSpecVersion":"4.3.0"}`, which `cli.sh supports` gates on. - Added `tests/test_installers.py` and updated `test_resolve.py`. `ruff check`, `ruff format`, `pyright` and `pytest` (144 passed) are all clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Java installations now use the published command-line package when available. If it isn’t available, installation can build the CLI using the published SDK release. * Selecting the latest Java version now resolves to the newest stable release, whether or not it includes a prebuilt CLI package. * **Bug Fixes** * Installation reports an error when neither the published package nor the fallback build is available. CI can then fall back to a full source build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
pq-sessions
Address CodeRabbit review comments on spec/DSPX-4221.md:
- Note the platform-side session-key-mlkem feature-detection imprecision
(reuses the pre-existing Preview.MLKEMTDFEnabled/mechanism probe rather
than a dedicated readiness check; each SDK's own hardcoded capability
flag is what actually gates the test), matching the comment already in
tdfs.py's feature-detection block.
- Fix invalid Go syntax in the SDK contract example (ocrypto.KeyType is a
string type; bitwise-OR between two enum values doesn't compile).
- Remove the "and vice versa" independence claim, which overstated test
coverage (only RSA-wrapped-TDF + ML-KEM-session-key is tested).
Fix a real CI failure this surfaced: the plain assert_rewrap_success(
session_key_type=X) pattern used for the new EC/RSA/ML-KEM session-key-type
assertions has two problems that only show up once you assert on the
*specific* algorithm rather than just "a rewrap succeeded":
1. eventMetaData.sessionKeyType is new (added by the companion platform PR,
unreleased) and not behind any version/preview flag we can check
statically. Asserting a specific value against a baseline/pre-fix
platform build finds zero matches -- indistinguishable from a real
negotiation bug. Broke test_tdf_roundtrip's new ecwrap assertion and
test_session_key_rsa_roundtrip against CI's baseline (unpatched) platform
matrix.
2. Log collection is poll-based. When two rewraps for different session
keys happen back-to-back in the same test (a plain decrypt immediately
followed by one requesting a specific algorithm), the earlier rewrap's
log line can still be un-tailed at the time of the later mark and get
folded into the same collection batch -- a bare count-based assertion
then reports success using the wrong (earlier) event instead of waiting
for the right one. Reproduced locally: test_tdf_roundtrip[...-ztdf-ecwrap]
failed 3/3 runs with "Expected session_key_type=ec:secp256r1, but
platform reported {'rsa:2048'}" even against the patched platform PR
branch, where the feature itself works correctly (confirmed via a
standalone otdfctl decrypt + raw platform.log inspection).
Added AuditLogAsserter.assert_rewrap_session_key_type(), which waits for an
event matching the expected type before falling back to a short existence
check (present-but-different-value still fails loudly; field-never-present
logs a warning and passes). Used by test_tdf_roundtrip's ecwrap block and
both test_session_key_rsa_roundtrip and test_session_key_mlkem_roundtrip.
Verified locally against the platform PR branch: all three pass
consistently now (previously ecwrap failed 3/3 runs).
Also address a Copilot review finding on SDK.decrypt(): ecwrap=True and
session_key_algorithm could both be set, which the CLI wrappers turn into
duplicate/competing --session-key-algorithm (or --rewrap-key-type) flags,
with the winner depending on the underlying CLI's parsing order rather than
caller intent. No current call site combines them, but rather than leave
that as an implicit landmine, decrypt() now raises ValueError if both are
passed -- ecwrap is shorthand for session_key_algorithm="ec:secp256r1"
anyway.
Remove the now-redundant decrypt-side XT_WITH_ECWRAP shortcut. It predates
session_key_algorithm and was pure sugar for
session_key_algorithm="ec:secp256r1"; XT_WITH_ECWRAP stays for encrypt
(selects the TDF's own KAO wrapping mechanism, an unrelated axis).
Dropped SDK.decrypt()'s ecwrap bool param (one call site, migrated to
session_key_algorithm), removed the dead decrypt-branch XT_WITH_ECWRAP
handling from all three cli.sh wrappers, and updated each script's env-var
doc comment. Verified against go, java, and js (companion PR branch)
decrypt SDKs -- all pass.
Add a TODO in assert_rewrap_session_key_type() documenting the plan for
closing the remaining gap: once opentdf/platform#3814 merges and cuts a
release, gate on a version-checked "audit-session-key-type" feature
(mirroring the existing audit_logging pattern) and hard-fail on mismatch
instead of warning. Not actionable yet since there is no release version
to check against pre-merge -- see the stacked draft PR reproducing the gap
this leaves open (unpatched web-sdk silently sending the wrong session-key
algorithm on EC rewrap, masked when paired with an unpatched platform that
does not emit the field at all).
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
…ibutes
ztdf-ecwrap was never a container format. simple_container() collapsed it
straight back to "ztdf", and the only real difference was XT_WITH_ECWRAP=true
on encrypt, which made each SDK CLI pass --wrapping-key-algorithm
ec:secp256r1 (--encap-key-type / --encapKeyType for java/js). So it was a
client-side override of the KAO wrapping algorithm wearing a container type's
clothes, and it fanned every test taking the `container` fixture out over a
second, near-duplicate run.
That conflated two independent axes:
- the KAO wrap key, chosen by policy (the algorithm of the KAS key mapped
to the attribute), surfacing as kao.type "wrapped" vs "ec-wrapped";
- the rewrap session key, chosen by the client per-decrypt, which this
branch already plumbs properly via SDK.decrypt(session_key_algorithm=...)
-> XT_WITH_SESSION_KEY_ALGORITHM.
Changes:
- container_type is now Literal["ztdf"]. simple_container() is deleted; it
had become the identity function. encrypt()/decrypt() use `container`
directly. --containers, the `container` fixture, and the parametrization
all still work, they just have one value now.
- Removed the XT_WITH_ECWRAP encrypt block and doc line from the go, java,
and js cli.sh shims. The `supports ecwrap` probes stay: the feature still
gates decrypting an ec-wrapped KAO, requesting an EC rewrap session key,
and the corrected EC HKDF salt floor.
- ec-wrapped KAO coverage moves to a dedicated test_ec_wrapped_kao_roundtrip
in test_tdfs.py, backed by a new attribute_with_ec_key fixture mapping an
ec:secp256r1 managed key (key_e256, on km2) to an attribute. It asserts
kao.type == "ec-wrapped", a present ephemeralPublicKey, and that kid and
url match the mapped key -- which is what the ztdf-ecwrap parametrization
was actually checking.
- test_session_key_mlkem_roundtrip and test_session_key_rsa_roundtrip merge
into one parametrized test_session_key_roundtrip over rsa:2048,
ec:secp256r1, mlkem:768 and mlkem:1024, gated by a SESSION_KEY_FEATURES
map. It encrypts against attribute_default_rsa so the KAO stays
RSA-wrapped and any failure is attributable to the session-key channel
alone.
- Narrowed otdf-sdk-mgr's independent ContainerKind duplicate to match.
- Retired the DSPX-4221 spec scaffold.
Known narrowing: the new EC KAO test needs key_management + autoconfigure, so
ec-wrapped KAO coverage no longer runs on pre-key-management platform tags,
where XT_WITH_ECWRAP worked purely client-side. Accepted as the cost of
policy-driven key selection; called out in the test docstring.
Verified against a local platform with go v0.36.0 and js v0.20.0 (java has no
stable release to install, so it was not exercised):
- test_ec_wrapped_kao_roundtrip: 4 passed.
- test_session_key_roundtrip, audit assertions on: 8 passed, 8 skipped. Both
mlkem parametrizations skip on stable SDK builds, which do not advertise
session-key-mlkem -- so the ML-KEM half of this test is still unproven
locally and needs tip builds.
- test_tdfs.py test_policytypes.py test_abac.py: 152 passed, 6 skipped, and 2
failures in test_tdf_with_malicious_kao, which is untouched here. Its
hardcoded "nothing running at 8585" KAS URL now collides with otdf-local's
km1; fixed in a separate PR.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
October 26, 2026 16:31
e40db95 to
a43eb0f
Compare
sujankota
approved these changes
Oct 29, 2026
dmihalcik-virtru
added a commit
to opentdf/java-sdk
that referenced
this pull request
Oct 29, 2026
## Summary Part of DSPX-4221 (Session Keys should support ML-KEM). `KASClient.unwrap()` only branched EC vs RSA for the client's ephemeral rewrap "session key" (as opposed to a KAS-managed wrapping key), so requesting an ML-KEM session key would fail. `KeyType` already had `MLKEM768Key`/`MLKEM1024Key` entries and `sdk-pqc-bc` already had the BouncyCastle KEM primitives (used for the KAS-managed mechanism-key path), but nothing generated a fresh ML-KEM keypair for the session-key role. ## Changes - **`spi/KemProvider`**: add `generateSessionKey(KeyType)`, returning a `KemSessionKey`: the ephemeral rewrap session key. It exposes only the public key (SPKI PEM), unwraps the KAS response itself, and erases its private key on `close()`. It's a `default` method that throws `UnsupportedOperationException`, so existing third-party providers keep compiling and loading. The core `sdk` module has no compile-time dependency on BouncyCastle (per ADR 0001), so generation lives in the optional `sdk-pqc-bc` module and is reached via the existing `ServiceLoader`-based `KemProviders` registry — same pattern already used for `wrapDEK`/`unwrapDEK`. - **`sdk-pqc-bc`**: `HybridCrypto.generateSessionKey` dispatches over all five supported PQC key types (X-Wing, both NIST hybrids, ML-KEM-768/1024), mirroring the `wrapDEK`/`unwrapDEK` switch. The new `BouncyCastleKemSessionKey` holds the raw private key bytes and zeroes them on `close()`; the private key is never PEM-encoded or held as a `String`. `BouncyCastleKemProvider` implements the new SPI method. - **`KASClient.unwrap()`**: branch on `sessionKeyType.isMLKEM()`, generating a session key via `KemProviders.get(...).generateSessionKey(...)`, unwrapping the response with it, and closing it when the rewrap finishes. The long-lived RSA session keypair is now published through a single immutable holder, so a concurrent first caller can't see the private key without its public PEM. - **`cmdline supports`**: reports `session-key-mlkem` only when a provider can actually generate ML-KEM-768 and ML-KEM-1024 session keys (probed at runtime), so the `fips` build, which excludes `sdk-pqc-bc`, reports it as unsupported. - New unit tests: `HybridCryptoSessionKeyTest` round-trips `generateSessionKey -> wrapDEK -> KemSessionKey.unwrapDEK` for all five PQC key types and checks that a different key, a tampered KEM ciphertext, a tampered sealed DEK, and a closed key all fail. `KASClientTest` covers concurrent first RSA unwraps and the missing-provider error; `KemProviderTest` covers the SPI default. `cmdline`'s `--rewrap-key-type` already accepts `mlkem:768`/`mlkem:1024` with **no code changes needed**: picocli's enum candidate list is derived from `KeyType#toString()`, which already returns `"mlkem:768"`/`"mlkem:1024"`. Verified by building `cmdline.jar` and checking `help decrypt` output. ## Test plan ``` mvn verify mvn test -P 'fips,!non-fips' ``` Both pass. `cmdline.jar` from the default build reports `session-key-mlkem` via `supports`; the `fips` build does not. Cross-SDK coverage comes from opentdf/tests#571's `test_session_key_roundtrip`, which runs `mlkem:768` and `mlkem:1024` with Java decrypting when X-Test is dispatched with `java-ref: DSPX-4221-pq-sessions`. ## Related work Companion PRs: - opentdf/tests#571 — xtest coverage (`test_session_key_roundtrip`) gated behind a new `session-key-mlkem` feature flag. - opentdf/platform#3814 — accepts ML-KEM client session keys in `rewrap`, and adds the equivalent Go SDK branch. Ref: DSPX-4221 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Added ML-KEM session-key unwrapping when a compatible provider is available. * Added key-pair generation for supported hybrid and ML-KEM algorithms, with public and private keys available in PEM format. * Updated the `supports` command to report ML-KEM session-key support when available. * **Bug Fixes** * Improved handling of EC, ML-KEM, and RSA session keys during unwrap. * Invalid or tampered key material is rejected during wrap and unwrap operations. <!-- 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
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.
Summary
Implements the test-first step of DSPX-4221 (Session Keys should support ML-KEM): a client-generated ML-KEM ephemeral key pair used as the rewrap session key (the key KAS wraps the response DEK back to), as distinct from the existing
mechanism-mlkem/mechanism-xwing/mechanism-secpmlkemcoverage which only exercises KAS-managed TDF wrapping keys.Changes
xtest/tdfs.py: newsession-key-mlkemfeature flag (detected alongsidemechanism-mlkem, since both are gated server-side by the samePreview.MLKEMTDFEnabledflag).SDK.decrypt()gains asession_key_algorithmparam, threaded through a newXT_WITH_SESSION_KEY_ALGORITHMenv var.xtest/sdk/{go,java,js}/cli.sh: wire the new env var to each CLI's real flag (--session-key-algorithm/--rewrap-key-type/--rewrapKeyType), and addsupports session-key-mlkemprobes. The Java probe joins wrapped help output before grepping — picocli line-wraps the long--rewrap-key-typechoice list at ~80 columns, which can splitmlkem:768across two lines and produce a false negative (caught by running against a real build, not just eyeballing--helpoutput).xtest/test_pqc.py: newtest_session_key_mlkem_roundtrip, parametrized overmlkem:768/mlkem:1024. Encrypts with a plain RSA-wrapped attribute (attribute_default_rsa) so a failure can only be attributed to the session-key transport channel, not to KAS-managed PQC mechanism support.Audit-log assertion (not just a successful decrypt)
A successful decrypt alone doesn't prove KAS actually used ML-KEM for the session key — if an SDK silently ignored the requested algorithm and fell back to RSA, the roundtrip would still pass either way, since both sides would agree on whatever was actually used. To make the test load-bearing, it now asserts against KAS's own rewrap audit log, which is independent of anything the client reports about itself:
xtest/audit_logs.py:ParsedAuditEventgains asession_key_typeproperty (eventMetaData.sessionKeyType);matches_rewrap/assert_rewrap/assert_rewrap_success/assert_rewrap_failuregain a matchingsession_key_typeparameter, mirroring the existingalgorithmparameter.audit_logs.assert_rewrap_success(session_key_type=session_key_algorithm, since_mark=mark)after decrypting.This depends on the companion platform PR (below) adding the
sessionKeyTypefield to the rewrap audit event — without it,session_key_typeis simply absent fromeventMetaDataand the assertion fails.Skips cleanly (with a clear reason) on platforms/SDKs that don't yet support ML-KEM session keys.
Verified end-to-end locally
Brought up a live platform + KAS instances (built from opentdf/platform#3814) and ran this test against Go, Java, and JS SDKs built from their respective companion-PR branches:
All 18 include a real
session_key_typeaudit-log match, not just a successful roundtrip. This run is also what caught a real web-sdk bug (opentdf/web-sdk#975) where the requested session-key algorithm was silently dropped and every decrypt negotiated RSA regardless of what was requested — the audit assertion is what surfaced it; a plain roundtrip-success check would have missed it entirely.Related work
Companion PRs:
rewrap, wires the Go SDK's response decrypt path, and adds thesessionKeyTypeaudit field this test relies on.Test plan
All pass. Plus the live 18/18 run described above.
Ref: DSPX-4221
Summary by CodeRabbit
New Features
Bug Fixes
Tests