chore(xtest): DPoP nonce-challenge test coverage and CLI/CI hardening#529
Open
dmihalcik-virtru wants to merge 23 commits into
Open
chore(xtest): DPoP nonce-challenge test coverage and CLI/CI hardening#529dmihalcik-virtru wants to merge 23 commits into
dmihalcik-virtru wants to merge 23 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds DPoP nonce-challenge support across the xtest suite: a new ChangesDPoP Nonce-Challenge Test Infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant _post_rewrap_with_nonce_retry
participant KAS
Test->>_post_rewrap_with_nonce_retry: request rewrap (auth_scheme)
_post_rewrap_with_nonce_retry->>KAS: POST rewrap with nonce-free DPoP proof
KAS-->>_post_rewrap_with_nonce_retry: 401 + DPoP-Nonce header
_post_rewrap_with_nonce_retry->>_post_rewrap_with_nonce_retry: re-sign DPoP proof with nonce
_post_rewrap_with_nonce_retry->>KAS: retry POST rewrap with new proof
KAS-->>Test: final response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
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
dmihalcik-virtru
force-pushed
the
fix-dpop-nonce-challenge
branch
from
e61eb55 to
7746ba2
Compare
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 <[email protected]> Signed-off-by: Dave Mihalcik <[email protected]>
- 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 <[email protected]>
Signed-off-by: Dave Mihalcik <[email protected]>
…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 <[email protected]>
Signed-off-by: Dave Mihalcik <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Dave Mihalcik <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Dave Mihalcik <[email protected]>
Signed-off-by: Dave Mihalcik <[email protected]>
Signed-off-by: Dave Mihalcik <[email protected]>
…gnosis Signed-off-by: Dave Mihalcik <[email protected]>
… on the parent command) Signed-off-by: Dave Mihalcik <[email protected]>
…lidation) Signed-off-by: Dave Mihalcik <[email protected]>
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. Signed-off-by: Dave Mihalcik <[email protected]>
- 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). Signed-off-by: Dave Mihalcik <[email protected]>
Signed-off-by: Dave Mihalcik <[email protected]>
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. Signed-off-by: Dave Mihalcik <[email protected]>
…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). Signed-off-by: Dave Mihalcik <[email protected]>
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. Signed-off-by: Dave Mihalcik <[email protected]>
…et (DSPX-3397) The DPoP tests authenticate as the opentdf-dpop client, but otdf_client_scs only entitled opentdf/opentdf-sdk, so the DPoP rewrap reached authorization and was denied (entitled:false -> 403). Add opentdf-dpop to the clientId allowlist so the DPoP-bound client is entitled like the other test clients. Signed-off-by: Dave Mihalcik <[email protected]>
dmihalcik-virtru
force-pushed
the
fix-dpop-nonce-challenge
branch
from
6c8953e to
7bd3ad1
Compare
X-Test Failure Report |
Convert remaining single-bracket [ ] test conditionals to [[ ]] throughout xtest/sdk/js/cli.sh to clear SonarCloud SC2292 findings and match the style already applied to the DPoP credential-restore checks. shellcheck and shfmt pass clean.
- js/cli.sh: add explicit `return 0` to echo_redacted() - js/cli.sh: merge redundant nested ECDSA-binding if (== "true" implies -n) - java/cli.sh: add explicit `return 0` to jar_help() All behavior-preserving. shellcheck and shfmt pass clean.
X-Test Failure Report |
X-Test Failure Report |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xtest/sdk/java/cli.sh (1)
49-67: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider
mktempfor the intermediate cache file.
tmp="${cache}.$$"is a predictable path in a shared/tmp; a co-resident process could pre-create it (symlink or file) before thejava ... >"$tmp"write. Low practical risk in ephemeral CI runners, butmktempremoves the guesswork entirely.♻️ Optional hardening
- local tmp="${cache}.$$" - java -jar "$jar" help "$@" >"$tmp" 2>/dev/null + local tmp + tmp=$(mktemp "${cache}.XXXXXX") + java -jar "$jar" help "$@" >"$tmp" 2>/dev/null mv -f "$tmp" "$cache"🤖 Prompt for 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. In `@xtest/sdk/java/cli.sh` around lines 49 - 67, The intermediate temp file in jar_help is predictable because it is built from the cache name plus the PID, which leaves a race window in shared /tmp. Update the jar_help flow to use mktemp for the temporary help output file before the java -jar invocation, then rename that secure temp file into the cache path after generation; keep the rest of the caching logic in jar_help unchanged.
🤖 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.
Nitpick comments:
In `@xtest/sdk/java/cli.sh`:
- Around line 49-67: The intermediate temp file in jar_help is predictable
because it is built from the cache name plus the PID, which leaves a race window
in shared /tmp. Update the jar_help flow to use mktemp for the temporary help
output file before the java -jar invocation, then rename that secure temp file
into the cache path after generation; keep the rest of the caching logic in
jar_help unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cee8895d-8c59-4d24-b31e-4b2387690c29
📒 Files selected for processing (5)
.github/workflows/xtest.ymlxtest/fixtures/obligations.pyxtest/sdk/java/cli.shxtest/sdk/js/cli.shxtest/test_dpop.py
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.
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; add
dpop-challengeboolean input (defaultfalse)to
workflow_dispatchandworkflow_callso callers opt in to the noncechallenge rather than having it hardcoded; pass
OTDFCTL_HEADSto all pyteststeps; 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