Skip to content

fix(xtest): match URI-scoped private key cache hits - #615

Merged
c-r33d merged 1 commit into
mainfrom
codex/cache-key-uri-assertion
Sep 21, 2026
Merged

c-r33d merged 1 commit into
mainfrom
codex/cache-key-uri-assertion

Conversation

@c-r33d

@c-r33d c-r33d commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

The cache isolation test still expects registry IDs in cache-hit logs, so it fails after opentdf/platform#4056 changes cache identity to quoted KAS URI + key ID. Match the complete cache_key value so /kas cannot also satisfy the shorter URI’s assertion.

Validation: Ruff lint/format and Pyright passed; checked matching against both URIs and rejection of wrong IDs, registry IDs, and the old prefix. Cross-SDK validation is running against platform commit 110f84d3263c0479ca598c793074a1543cb9bc20, with KAO/cache coverage enabled.

Summary by CodeRabbit

  • Tests
    • Updated audit-log validation for cached private keys to verify the complete cache key, including the KAS URI and key ID.
    • Added coverage to ensure entries from different registries are matched distinctly.

Signed-off-by: Chris Reed <creed@virtru.com>
@c-r33d
c-r33d requested review from a team as code owners September 21, 2026 17:46
@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1a5d9e68-07b8-4f8b-b314-f192ad2e5181

📥 Commits

Reviewing files that changed from the base of the PR and between 118f96b and 314f5fc.

📒 Files selected for processing (1)
  • xtest/test_abac.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The ABAC test now matches cached private-key audit entries by the full cache key JSON, using the KAS URI and key ID.

Changes

ABAC cache assertion

Layer / File(s) Summary
Cache-key audit matching
xtest/test_abac.py
The test imports json and matches the audit-log cache_key field against JSON built from key.kas_uri and key.key.key_id. A comment documents the full-key matching requirement.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Other

Suggested reviewers: dmihalcik-virtru

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: matching URI-scoped private key cache hits in the test.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

A rabbit checks the cache at night
Full URI and key make matches right
JSON hops into the log
No twin keys hide in the fog
The test now thumps with delight

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@c-r33d c-r33d changed the title test(xtest): match URI-scoped private key cache hits fix(xtest): match URI-scoped private key cache hits Sep 21, 2026
@c-r33d
c-r33d merged commit b7a1258 into main Sep 21, 2026
29 of 32 checks passed
@c-r33d
c-r33d deleted the codex/cache-key-uri-assertion branch September 21, 2026 18:20
dmihalcik-virtru added a commit that referenced this pull request Sep 21, 2026
Rebased onto main after #606, #612 and #615 landed; findings those PRs already
addressed are dropped, including the km3 action pin, which #612 synchronized
with its siblings.

- 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?". (A later commit in this stack
  turns that skip into a failure.)
- 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.
dmihalcik-virtru added a commit that referenced this pull request Sep 21, 2026
Rebased onto main after #606, #612 and #615 landed; findings those PRs already
addressed are dropped, including the km3 action pin, which #612 synchronized
with its siblings.

- 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?". (A later commit in this stack
  turns that skip into a failure.)
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants