Repository navigation
test(xtest): demonstrate session-key audit gap against unpatched web-sdk - #572
Closed
dmihalcik-virtru wants to merge 4 commits into
Closed
dmihalcik-virtru wants to merge 4 commits into
dmihalcik-virtru wants to merge 4 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
|
Member
Author
|
Reproduced in CI: triggered Run: https://github.com/opentdf/tests/actions/runs/30833192027
(Note for reproducing this yourself: |
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
October 14, 2026 20:21
979fbf0 to
62f0783
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions-js-audit-gap
branch
from
October 14, 2026 20:21
802cfc7 to
8f13ea9
Compare
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 14, 2026 20:26
62f0783 to
e40db95
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions-js-audit-gap
branch
from
October 14, 2026 20:26
8f13ea9 to
4a7cd6c
Compare
Adds test_session_key_ec_roundtrip, a sibling of test_session_key_rsa_roundtrip and test_session_key_mlkem_roundtrip, isolating the gap documented in assert_rewrap_session_key_type()'s TODO into its own unambiguous test. Expected to fail when run with an unpatched web-sdk (main) as the decrypt SDK, paired with a platform build that emits eventMetaData.sessionKeyType (opentdf/platform#3814): web-sdk main silently ignores the requested session-key algorithm on EC rewrap and always negotiates rsa:2048 instead of the requested ec:secp256r1 -- the same bug already fixed in opentdf/web-sdk#975 (untested there, since that fix hasn't merged). Reproduced locally: passes for go/java (all branch combos) and for web-sdk's PR branch, fails specifically for js@main: FAILED test_pqc.py::test_session_key_ec_roundtrip[small-js@refs--pull--975--head-js@main-in_focus0] FAILED test_pqc.py::test_session_key_ec_roundtrip[small-js@main-js@main-in_focus0] AssertionError: Expected rewrap session_key_type='ec:secp256r1', but platform reported {'rsa:2048'} This commit is intentionally stacked on top of (not merged into) DSPX-4221-pq-sessions, as a linkable reproduction of a known, tracked gap -- not part of the feature itself. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions-js-audit-gap
branch
from
October 14, 2026 20:28
4a7cd6c to
a1e4195
Compare
|
dmihalcik-virtru
force-pushed
the
DSPX-4221-pq-sessions
branch
from
October 26, 2026 16:31
e40db95 to
a43eb0f
Compare
Member
Author
|
Closing: the gap this demonstrated is fixed upstream — opentdf/platform#3814 and opentdf/web-sdk#975 both merged on 2026-10-07. |
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.
Stacked on #571 — do not merge into
maindirectly.Adds
test_session_key_ec_roundtrip, isolating a known gap flagged in that PR's review (see theTODO(DSPX-4221)inassert_rewrap_session_key_type(),xtest/audit_logs.py): the audit-log assertion added for ML-KEM/RSA/EC session keys can't statically tell "platform too old to emiteventMetaData.sessionKeyType" apart from "platform new enough, but the client sent the wrong session-key type" — there's no released platform version yet to gate on (opentdf/platform#3814 is still unmerged).That gap is real, not hypothetical:
web-sdkatmainsilently ignores the requested session-key algorithm on EC rewrap and always negotiatesrsa:2048, regardless of what's requested — the same bug already fixed in opentdf/web-sdk#975, just not yet merged tomain. Against today's unpatched platform CI matrix, this is invisible (the field is absent for every rewrap, so the assertion falls back to a warning). Paired with a patched platform (opentdf/platform#3814's branch), it's a hard failure:Reproduced locally against go/java (all branch combos, all pass) and web-sdk's own PR branch (passes) — fails specifically for
js@main.To reproduce in CI, trigger
xtest.ymlviaworkflow_dispatchon this branch withplatform-ref: pr:3814,js-ref: main,focus-sdk: js.Purpose: give the TODO in #571 something concrete to link to, and a CI job that will go green on its own once opentdf/web-sdk#975 merges (or start reliably red-flagging regressions once opentdf/platform#3814 releases and the TODO's version gate lands).