feat(xtest): cover KAO URI lookup with isolated KM3 - #605
Conversation
Signed-off-by: Chris Reed <creed@virtru.com>
Signed-off-by: Chris Reed <creed@virtru.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add platform-level feature overrides, update CI to start the dedicated km3 KAS from multikas support, extend km3 fixtures and audit mappings, add registry key lookup, and strengthen alternate KAS decryption assertions. ChangesPlatform-forced KAO testing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant km3KAS
participant KASFixtures
participant ABACTest
participant AuditLogs
GitHubActions->>km3KAS: start when multikas is supported
KASFixtures->>km3KAS: create or fetch the km3 registry entry
ABACTest->>km3KAS: create alternate KAS registration
ABACTest->>AuditLogs: verify key access and rewrap events
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. (1 skipped: 1 unsupported.)
✨ 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. A rabbit watched km3 wake, Comment |
Add `force-platform-supports` / `XT_FORCE_PLATFORM_SUPPORTS` so tests can exercise unreleased platform features. SDK overrides remain separate. Base PR for #605; merge this first. Validation: 29 unit tests, lint, formatting, type checks, and workflow checks passed. Signed-off-by: Chris Reed <creed@virtru.com>
c-r33d
changed the title
dmihalcik-virtru
left a comment
dmihalcik-virtru
left a comment
There was a problem hiding this comment.
Reviewed this across code quality, test coverage, comments, and silent failures. ruff check, ruff format --check, and pyright all pass in xtest/; fixture scopes are legal; and filecmp.cmp(..., shallow=False) is stricter than every neighbour in the file. Nice.
Two test-design questions inline below — both about whether a green run actually means anything. They're yours to call, so I'm raising rather than pushing.
I'm also opening a stacked PR against this branch with the mechanical fixes (km3 reachability probe so a missing km3 skips instead of dying inside an SDK CLI; km3 in otdf-local so there's a local repro path; an audit assertion so the km3 audit plumbing this PR adds is actually load-bearing; and comment/doc fixes).
Three things I checked that turned out fine — no action needed:
- Adding
kas-km3to the default audit service list is inert.fixtures/audit.py:163-166guards withif log_path:, so the workflow's|| ''is correctly ignored; the../platform/logs/fallback only runs when no env var is set and checkslog_file.exists(); and the loudFileNotFoundErrorfires only on a wholly empty dict. YourKAS_KM3_LOG_FILEexpression is in fact more careful than the pre-existing km1/km2 lines, which degrade to the bare directory string"../../"when their step is skipped. Worth back-porting your guard to those. - The km3 action pin is safe.
d16f6d0is 138 commits ahead of the siblings'6dd5f64and 0 behind, so it contains therequire_noncestartup-YAML fix. The only thing I'd add is an exit condition on the comment so the divergent pin doesn't quietly outlive its reason. services.kas.kas_uri_from_kaois the right key. Confirmed attest/start-additional-kas/action.yaml:212in opentdf/platform#4057. That action also setsregistered_kas_uri = "http://localhost:" + KAS_PORT, i.e. no/kassuffix — which is exactly what makes this test's premise sound.
| assert key.kas_uri == alternate_kas.uri != kas_entry_km3.uri | ||
| assert key.key.key_id not in { | ||
| k.key.key_id for k in otdfctl.kas_registry_keys_list(kas_entry_km3) | ||
| } |
There was a problem hiding this comment.
This assertion can pass vacuously, which matters more than its size suggests — it is the premise that makes the whole test discriminating.
kas_registry_keys_list (xtest/otdfctl.py:191-195) returns [] in two non-obvious cases: when the CLI exits non-zero with not found in stderr, and when output is empty or null. So a misdirected or broken list call yields an empty set and this passes without having checked anything. It's also near-tautological even when it works — km3-alternate-uri-<sha256[:8]> is freshly minted and unique, so it was never going to collide with km3's key list.
If this one passes for the wrong reason, the green decrypt below proves nothing.
Suggest pairing it with a positive check (which proves the list call works at all) and asserting the stronger invariant:
assert key.key.key_id in {
k.key.key_id for k in otdfctl.kas_registry_keys_list(alternate_kas)
}
assert otdfctl.kas_registry_keys_list(kas_entry_km3) == []The second line says the real thing: km3's own registration holds no keys, so a URI-based lookup must miss.
There was a problem hiding this comment.
Added a GetKey check to ensure that the kas registry does not have the same KID as the one being asked for, ensuring that a rewrap failure would occur if the KAS is incorrectly configured.
… test/kas-uri-from-kao
|
Rebased onto main after #605 and #606 landed; the findings those PRs already addressed are dropped. - Skip the km3 tests when no km3 is listening, instead of failing with connection-refused inside an SDK CLI. The feature gate answers "is the override set?", not "does a km3 exist?". - Add kas-km3 (8787) to otdf-local, with the kas_uri_from_kao setting, the 5-minute key cache and debug logging its CI step uses, so all three km3 tests are runnable locally. - Document that kas_uri_from_kao is forced via XT_FORCE_PLATFORM_SUPPORTS (not XT_FORCE_SUPPORTS) and that km3 starts on the PR gate and nightlies while every test behind the gate skips. - Give the divergent km3 action pin an exit condition.
3 participants
Add a round-trip test for KAO-based key lookup using a dedicated KM3 with
kas_uri_from_kaoenabled. Run it withforce-platform-supports: kas_uri_from_kao.Stacked on #604. Uses the action input from opentdf/platform#4057 and covers platform stack opentdf/platform#3951 → opentdf/platform#4048 → opentdf/platform#4053 → opentdf/platform#4056.
Validation: unit, fixture, workflow, lint, and type checks passed. Live round-trip not yet run.
Summary by CodeRabbit
New Features
Bug Fixes