chore(xtest): DPoP nonce-challenge test coverage and CLI/CI hardening#529
chore(xtest): DPoP nonce-challenge test coverage and CLI/CI hardening#529dmihalcik-virtru wants to merge 16 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEnables DPoP nonce-challenge support across the xtest infrastructure: the CI workflow activates ChangesDPoP Nonce-Challenge Test Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces caching for the Java CLI help output to avoid JVM startup overhead, adds support for DPoP-related environment variables and client overrides in the JS CLI shim, improves subprocess error logging in Python tests, and updates DPoP test fixtures. Feedback highlights a potential race condition in the Java CLI caching mechanism under parallel test execution, where concurrent processes could read an incomplete cache file, and suggests using an atomic write-and-rename pattern to resolve it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🤖 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/java/cli.sh`:
- Around line 58-61: The cache file is being written to directly after checking
existence, creating a race condition where concurrent invocations can read a
partially written cache file. Instead of writing directly to the final cache
file location in the line that runs the java command, write to a temporary file
first (using a temp filename derived from the cache variable), then atomically
move that temporary file to the final cache location using the mv command. This
ensures that other processes reading the cache file will either see the old
complete version or the new complete version, never a partial write. Reference
the cache variable and the java jar invocation in your fix.
In `@xtest/sdk/js/cli.sh`:
- Around line 149-150: The --auth argument containing CLIENTSECRET is being
logged/echoed in subsequent command output, which exposes sensitive credentials
in CI logs. Locate where the command arguments are being printed or logged (the
echo statement that prints full args), and add logic to redact or mask the
--auth argument value before logging while keeping the actual command execution
intact with the real credentials. Ensure the secret portion of the auth
credentials is not visible in any log output or CI artifacts.
- Around line 139-140: Replace the single bracket conditionals `[ -n ... ]` with
double bracket syntax `[[ -n ... ]]` for the two conditional checks involving
$_pre_clientid and $_pre_clientsecret variables. Update both lines that check
$_pre_clientid and $_pre_clientsecret to use `[[ ... ]]` instead of `[ ... ]` to
comply with ShellCheck best practices and improve code robustness.
🪄 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
Run ID: 4855622a-9727-479f-a484-91316676a003
📒 Files selected for processing (5)
.github/workflows/xtest.ymlxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/tdfs.pyxtest/test_dpop.py
dmihalcik-virtru
changed the title
Update start-up-with-containers and start-additional-kas action SHAs to DSPX-3397-platform-service tip, and pass dpop-challenge-enabled: true so the DPoP nonce challenge tests are not skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Capture caller env vars before sourcing test.env (which unconditionally
resets them), then restore them — allows pytest monkeypatch to override
CLIENTID=opentdf-dpop for DPoP-specific tests.
- Replace hardcoded --auth opentdf:secret with ${CLIENTID:-opentdf}:${CLIENTSECRET:-secret}.
- Add XT_WITH_DPOP (algorithm, e.g. ES256) and XT_WITH_DPOP_KEY (PEM path)
support, wired to --dpop / --dpop-key CLI flags.
- Update _dpop_client_env fixture to also export XT_WITH_DPOP=ES256 so
DPoP proof generation is actually exercised in test_dpop.py.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…st binary
load_otdfctl() in conftest.py reads OTDFCTL_HEADS to resolve sdk/go/dist/{tag}/otdfctl.sh.
Without it, every test step fell through to sdk/go/dist/main/otdfctl.sh or the non-dist
sdk/go/otdfctl.sh, which falls back to go run github.com/opentdf/otdfctl@latest instead
of the built branch binary.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… on the parent command)
Running 'java -jar cmdline.jar help' on every encrypt/decrypt paid 150-500ms of JVM startup per probe (kas-allowlist and --verbose checks). Add a jar_help() helper that caches help output to a tmpfile keyed by the jar's mtime, and discards stderr to keep JVM warnings out of test logs.
- java cli.sh: write jar_help cache to a temp file then mv, so concurrent xdist workers never read a partially written cache (gemini/coderabbit). - java/js cli.sh: use [[ ]] for the new conditionals (SonarCloud SC2292). - js cli.sh: mask the --auth secret in echoed commands so CI logs don't capture client credentials (coderabbit).
dmihalcik-virtru
force-pushed
the
fix-dpop-nonce-challenge
branch
from
e61eb55 to
7746ba2
Compare
Pass dpop-challenge-enabled:true to the alpha/beta/gamma/delta/km1/km2 start-additional-kas steps and re-pin that action to the go-branch commit (2305c4ab) that adds the input. Fixes test_dpop_rejects_tampered_nonce, which targets the alpha KAS that previously never set require_nonce.
…once-agnostic - Repin start-up-with-containers and all start-additional-kas steps to opentdf/platform DSPX-3397-platform-go-sdk @4a19b297 (adds dpop enforce alongside require_nonce on additional KAS). - test_dpop_bearer_scheme: route both rewrap calls through a new _post_rewrap_with_nonce_retry helper so the test passes whether or not the target KAS enforces require_nonce (satisfies the use_dpop_nonce challenge before asserting lenient 200 + WARN).
Platform action split DPoP enforcement into a separate dpop-enforce-required input (default false); xtest only passes dpop-challenge-enabled, so require_nonce stays on and global enforce stays off, restoring the non-DPoP suite.
|
1 participant
What
Wires up end-to-end DPoP (Demonstrating Proof-of-Possession) coverage in the
cross-client test suite, including the
dpop_nonce_challengecapability, andhardens the surrounding CLI shims and CI diagnostics.
Why
DPoP-bound clients were not actually exercised end-to-end: the shims read
CLIENTIDbut never enabled DPoP proof generation, so the tests passedwithout verifying the bound flow. This branch makes the DPoP client provision
and emit proofs, validates the nonce-challenge path, and fixes the platform
action pins that carry the DPoP
htu/htmvalidation fixes.Changes
Tests
test_dpop.py: the autouse_dpop_client_envfixture now also setsXT_WITH_DPOP=ES256, so the DPoP-boundopentdf-dpopclient actuallygenerates proofs during encrypt/decrypt.
SDK CLI shims
sdk/js/cli.sh: wireCLIENTID,CLIENTSECRET, and DPoP into the shim.sdk/java/cli.sh: delegatedpop_nonce_challengedetection to the binary;enable
--verbosewhen available (checked on the root help, where it isScopeType.INHERIT); cache thehelpcapability probes by jar mtime to cutper-operation JVM startup overhead and keep JVM warnings out of logs.
tdfs.py: surface thedpop_nonce_challengecapability and log subprocessstdout/stderr on encrypt/decrypt failure for xdist visibility.
CI
xtest.yml: bump platform action SHA pins to pick up the DPoPhtu/htmstrict-validation fixes; enable
dpop-nonce-challenge; passOTDFCTL_HEADSto all pytest steps; upload platform and KAS server logs as artifacts on
failure.
Test plan
uv run pytest test_dpop.py --sdks "go java js" -vagainst a platform builtfrom the pinned action SHA (DPoP enabled).
Summary by CodeRabbit
New Features
Improvements