fix(xtest): address review findings on KAO URI coverage - #608
Open
dmihalcik-virtru wants to merge 4 commits into
Open
dmihalcik-virtru wants to merge 4 commits into
dmihalcik-virtru wants to merge 4 commits into
Conversation
Signed-off-by: Chris Reed <creed@virtru.com>
Signed-off-by: Chris Reed <creed@virtru.com>
Follow-ups from the review of #605, stacked on that branch. - Skip test_decrypt_uses_kao_kas_registration when no km3 is listening, instead of failing with connection-refused inside an SDK CLI. - Add kas-km3 (8787) to otdf-local so the test is runnable locally. - Document kas_uri_from_kao as force-only via XT_FORCE_PLATFORM_SUPPORTS, and that neither the PR gate nor the nightlies exercise it. - Give the divergent km3 action pin an exit condition. - Assert the km3 rewrap audit event the PR added collection for.
|
Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
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 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
2 participants
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 #605 — base is
test/kas-uri-from-kao, notmain. Merge #605 first, or merge this into it.These are the mechanical follow-ups from my review of #605. The two design-level findings (the vacuous key-absence assertion and the missing negative control) are inline comments on #605 itself rather than edits here, since they're the author's call.
What's here
I1 — km3 reachability probe.
kas_uri_from_kaois force-only, so the feature gate answers "is the override set?", not "does a km3 exist?". Nothing in the fixture path ever contacts:8787—kas_registry_create_if_not_presentand_get_or_create_keytalk only to the policy service on:8080. So with the override set but no km3 running, registry create, key create and encrypt all succeed and decrypt dies with a connection-refused buried in an SDK CLI's stderr. Newkas_reachable()inxtest/fixtures/kas.pyturns that into a skip that names the port. This also covers the CI case whereforce-platform-supportsis dispatched whilemultikas/km-checkis false:kao-checkis gated on both, so km3 never starts, but the pytest gate has no such condition.I2 —
kas-km3inotdf-local, started by default. Port 8787, mirroring km1/km2. km3 alone getsservices.kas.kas_uri_from_kao: true; km1 and km2 keep the defaultfalseso they stay usable as the negative-control instances. Cost, accepted deliberately:all_kas_names()drivesup/status/env, so everyotdf-local upnow starts a 7thgo run ./service. In exchangeotdf-local envemitsKAS_KM3_LOG_FILE, the audit fixture picks km3 up automatically, and the new test becomes runnable locally — which matters because CI only runs it on manual dispatch.I3 + I5 — documentation only, no CI behaviour change.
XT_FORCE_PLATFORM_SUPPORTSis empty onpull_requestand on theschedulecrons, sotest_decrypt_uses_kao_kas_registrationskips every cell in both the PR gate and the nightlies. Leaving that as-is and making it legible: thetdfs.pycomment now namesXT_FORCE_PLATFORM_SUPPORTSexplicitly (it said only "force-only", and the one prior use of that phrase means the SDK override — and because both parse against the samefeature_typeLiteral,XT_FORCE_SUPPORTS=kas_uri_from_kaois accepted and then silently does nothing), names theservices.kas.kas_uri_from_kaoservice setting and where it's turned on, and states the un-force step.xtest/README.mdrecords the force-only status and the exact dispatch invocation.I4 — pin comment exit condition. The
d16f6d0pin is safe —gh api .../compare/6dd5f64...d16f6d0is 138 ahead / 0 behind, so it contains the sibling steps'require_noncefix. But the comment documented a temporary divergence with no exit condition, so it stayed true forever while the pin quietly persisted. Comment only; the SHA is unchanged.I6 — assert on the km3 audit log. #605 wires km3 into the audit service list, the env mapping, the workflow env and the failure artifact, but the test never requests
audit_logs, so none of that plumbing is exercised. Since the feature is specifically about which KAS URI is authoritative, the rewrap event is the observable worth asserting. Note no key-management test intest_abac.pyusesAuditLogAssertertoday, so this steps slightly beyond the immediate family — justified by #605 having added km3 audit collection in the first place.Known follow-ups, left out to keep this reviewable
Misplaced comment at
test_abac.py:974; missingattribute_with_alternate_kas_registrationdocstring; the cross-module private import attest_abac.py:19(_get_or_create_key/_create_keyed_attribute— a new precedent, not an existing pattern); chained assert at:935; the(kao,) = ...unpack at:970; deadec-tdf-enabled: trueon km3; and thekao-checkstep's style divergence from its siblings plus its missing decisionecho.Verification
ruff check,ruff formatandpyrightare clean in bothxtest/andotdf-local/;test_tdfs_units.pypasses (29). The localotdf-local upsmoke test for km3 has not been run — worth doing before merge.