fix(xtest): rename policy binding assertions to reflect tamper classification#422
Open
marythought wants to merge 4 commits intomainfrom
Open
fix(xtest): rename policy binding assertions to reflect tamper classification#422marythought wants to merge 4 commits intomainfrom
marythought wants to merge 4 commits intomainfrom
Conversation
…fication Policy binding failures are integrity failures (tamper), not KAS request errors. KAS intentionally returns a generic "bad request" for these to avoid leaking secret key material information, and the SDK classifies them under ErrTampered. Rename assert_kas_request_error → assert_policy_tamper_error and update the docstring and expected patterns to match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the error handling assertions within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 correctly renames assert_kas_request_error to assert_policy_tamper_error and updates its implementation to better reflect its purpose of checking for policy tampering errors. My review focuses on a minor code quality improvement. I've suggested removing an unused parameter from the newly renamed function and updating its call sites accordingly. This will make the code cleaner and easier to maintain.
5 tasks
…ture branches Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
X-Test Failure Report |
X-Test Results✅ java-v0.13.0 |
…r-split For SDKs that support the tamper-error-split feature (Go SDK >= 0.14.0), assert that policy binding failures produce a tamper error specifically and do NOT produce a KAS request error. Older SDKs fall back to the permissive pattern match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
X-Test Results✅ java-main |
github-merge-queue bot
pushed a commit
to opentdf/platform
that referenced
this pull request
Apr 2, 2026
…figuration (#3166) ## Problem Today, **every** KAS `InvalidArgument` (400) error is classified as `ErrRewrapBadRequest` → `ErrTampered`, regardless of the actual cause. That means `errors.Is(err, sdk.ErrTampered)` returns `true` for misconfiguration errors like a wrong key ID, unsupported key type, or missing EC-wrapped support — not just actual integrity failures like policy binding mismatches or DEK decryption failures. This makes `ErrTampered` unreliable as a signal. SDK consumers cannot distinguish "this TDF was tampered with" from "your KAS setup is wrong," which undermines the tamper detection API. ## Approach Changing gRPC status codes for policy binding failures would leak information about computations involving secret key material. Instead, this PR splits the error signal at the message level: 1. **KAS** (`service/kas/access/rewrap.go`): non-secret 400s now return descriptive messages (e.g. `"unsupported key type"`, `"key access object is nil"`, `"ec-wrapped not enabled"`). Errors involving secret key material — policy binding, DEK decryption, corrupted policy body, malformed binding encoding — keep the generic `"bad request"` to avoid leaking secret-derived information. 2. **SDK** (`sdk/tdf.go`): classifies errors based on the message. The substring `"desc = bad request"` anchored to the gRPC status description field = potential tamper (`ErrRewrapBadRequest` under `ErrTampered`); anything else with `InvalidArgument` = misconfiguration (`ErrKASRequestError`, not `ErrTampered`). Per-KAO errors are serialized as plain strings through the proto response (not as gRPC status errors), so substring matching is the only classification mechanism available. 3. **Shared contract**: the generic message pattern is defined as `kasGenericBadRequest` in `sdk/tdferrors.go` with a cross-reference comment in `service/kas/access/rewrap.go` so both sides stay in sync. The companion xtest PR (opentdf/tests#422) provides runtime enforcement across Go, Java, and JS SDKs. ### Error classification | Error source | KAS message | SDK sentinel | `errors.Is(ErrTampered)`? | |---|---|---|---| | Policy binding mismatch | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | DEK decryption failure | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | Corrupted policy body | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | Misconfiguration | descriptive (e.g. `"unsupported key type"`) | `ErrKASRequestError` | No | | Access denied | `"forbidden"` | `ErrRewrapForbidden` | No | ## Breaking changes ### 1. `ErrTampered` no longer matches KAS misconfiguration errors `errors.Is(err, sdk.ErrTampered)` no longer matches KAS misconfiguration errors (descriptive 400s). Consumers who relied on this to catch all KAS 400s should add a check for `ErrKASRequestError`: ```go if errors.Is(err, sdk.ErrTampered) { // integrity failure (tamper) } else if errors.Is(err, sdk.ErrKASRequestError) { // client/configuration error (400 or 403) } ``` ### 2. `ErrRewrapForbidden` is now under `ErrKASRequestError` `ErrRewrapForbidden` now wraps `ErrKASRequestError`, meaning `errors.Is(err, ErrKASRequestError)` matches both misconfiguration 400s and forbidden 403s. Previously `ErrRewrapForbidden` was a standalone error. This groups all KAS request-level errors under one sentinel, separate from the tamper hierarchy. ### 3. Pre-existing descriptive `err400` calls reclassified 16 pre-existing `err400` calls in the early request validation path (e.g. `"invalid request body"`, `"missing request body"`, `"clientPublicKey failure"`, `"bad key for rewrap"`) already had descriptive messages. Under the old code, ALL `InvalidArgument` errors were classified as `ErrRewrapBadRequest` (tamper) regardless of message. Now they are correctly classified as `ErrKASRequestError` (misconfiguration). These are all client-side validation errors that do not involve secret key material, so this is the correct classification. ### 4. `tdf3Rewrap` no longer short-circuits on per-KAO errors Previously, `tdf3Rewrap` had an early-return path where `verifyRewrapRequests` errors (other than `errNoValidKeyAccessObjects`) would short-circuit with a top-level `err400("invalid request")`, discarding per-KAO results. Now per-KAO results are always preserved in the results map even when errors occur, so tamper signals from individual KAOs reach the SDK instead of being replaced by a generic top-level error. ## Test plan - [x] `TestGetKasErrorToReturn` — all 11 descriptive messages, generic "bad request" → `ErrTampered`, desc-prefix anchoring, middleware false-positive prevention - [x] KAS access tests pass - [x] golangci-lint clean - [x] Companion xtest PR: opentdf/tests#422 — adds `tamper-error-split` feature flag and strict assertion that policy binding failures produce tamper errors (not KAS request errors) - [x] [xtest CI passed](https://github.com/opentdf/tests/actions/runs/23261931197) (Go, Java, JS) — run from xtest branch with tightened assertions against this platform branch > **Note:** The platform `platform-xtest` CI job runs xtests from `main` (`opentdf/tests/.github/workflows/xtest.yml@main`), which still has the permissive assertions. The strict assertions in opentdf/tests#422 should be merged after this PR lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added cross-platform SDK compatibility testing workflow documentation for contributors * **Bug Fixes** * Enhanced KAS error classification to distinguish between request/configuration failures and integrity issues * Improved rewrap operation error messages for better debugging while protecting sensitive information * **Tests** * Expanded KAS error handling test coverage for additional error scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mary Dickson <mary.dickson@virtru.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-merge-queue bot
pushed a commit
to opentdf/platform
that referenced
this pull request
Apr 2, 2026
…figuration (#3166) ## Problem Today, **every** KAS `InvalidArgument` (400) error is classified as `ErrRewrapBadRequest` → `ErrTampered`, regardless of the actual cause. That means `errors.Is(err, sdk.ErrTampered)` returns `true` for misconfiguration errors like a wrong key ID, unsupported key type, or missing EC-wrapped support — not just actual integrity failures like policy binding mismatches or DEK decryption failures. This makes `ErrTampered` unreliable as a signal. SDK consumers cannot distinguish "this TDF was tampered with" from "your KAS setup is wrong," which undermines the tamper detection API. ## Approach Changing gRPC status codes for policy binding failures would leak information about computations involving secret key material. Instead, this PR splits the error signal at the message level: 1. **KAS** (`service/kas/access/rewrap.go`): non-secret 400s now return descriptive messages (e.g. `"unsupported key type"`, `"key access object is nil"`, `"ec-wrapped not enabled"`). Errors involving secret key material — policy binding, DEK decryption, corrupted policy body, malformed binding encoding — keep the generic `"bad request"` to avoid leaking secret-derived information. 2. **SDK** (`sdk/tdf.go`): classifies errors based on the message. The substring `"desc = bad request"` anchored to the gRPC status description field = potential tamper (`ErrRewrapBadRequest` under `ErrTampered`); anything else with `InvalidArgument` = misconfiguration (`ErrKASRequestError`, not `ErrTampered`). Per-KAO errors are serialized as plain strings through the proto response (not as gRPC status errors), so substring matching is the only classification mechanism available. 3. **Shared contract**: the generic message pattern is defined as `kasGenericBadRequest` in `sdk/tdferrors.go` with a cross-reference comment in `service/kas/access/rewrap.go` so both sides stay in sync. The companion xtest PR (opentdf/tests#422) provides runtime enforcement across Go, Java, and JS SDKs. ### Error classification | Error source | KAS message | SDK sentinel | `errors.Is(ErrTampered)`? | |---|---|---|---| | Policy binding mismatch | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | DEK decryption failure | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | Corrupted policy body | `"bad request"` (generic) | `ErrRewrapBadRequest` | Yes | | Misconfiguration | descriptive (e.g. `"unsupported key type"`) | `ErrKASRequestError` | No | | Access denied | `"forbidden"` | `ErrRewrapForbidden` | No | ## Breaking changes ### 1. `ErrTampered` no longer matches KAS misconfiguration errors `errors.Is(err, sdk.ErrTampered)` no longer matches KAS misconfiguration errors (descriptive 400s). Consumers who relied on this to catch all KAS 400s should add a check for `ErrKASRequestError`: ```go if errors.Is(err, sdk.ErrTampered) { // integrity failure (tamper) } else if errors.Is(err, sdk.ErrKASRequestError) { // client/configuration error (400 or 403) } ``` ### 2. `ErrRewrapForbidden` is now under `ErrKASRequestError` `ErrRewrapForbidden` now wraps `ErrKASRequestError`, meaning `errors.Is(err, ErrKASRequestError)` matches both misconfiguration 400s and forbidden 403s. Previously `ErrRewrapForbidden` was a standalone error. This groups all KAS request-level errors under one sentinel, separate from the tamper hierarchy. ### 3. Pre-existing descriptive `err400` calls reclassified 16 pre-existing `err400` calls in the early request validation path (e.g. `"invalid request body"`, `"missing request body"`, `"clientPublicKey failure"`, `"bad key for rewrap"`) already had descriptive messages. Under the old code, ALL `InvalidArgument` errors were classified as `ErrRewrapBadRequest` (tamper) regardless of message. Now they are correctly classified as `ErrKASRequestError` (misconfiguration). These are all client-side validation errors that do not involve secret key material, so this is the correct classification. ### 4. `tdf3Rewrap` no longer short-circuits on per-KAO errors Previously, `tdf3Rewrap` had an early-return path where `verifyRewrapRequests` errors (other than `errNoValidKeyAccessObjects`) would short-circuit with a top-level `err400("invalid request")`, discarding per-KAO results. Now per-KAO results are always preserved in the results map even when errors occur, so tamper signals from individual KAOs reach the SDK instead of being replaced by a generic top-level error. ## Test plan - [x] `TestGetKasErrorToReturn` — all 11 descriptive messages, generic "bad request" → `ErrTampered`, desc-prefix anchoring, middleware false-positive prevention - [x] KAS access tests pass - [x] golangci-lint clean - [x] Companion xtest PR: opentdf/tests#422 — adds `tamper-error-split` feature flag and strict assertion that policy binding failures produce tamper errors (not KAS request errors) - [x] [xtest CI passed](https://github.com/opentdf/tests/actions/runs/23261931197) (Go, Java, JS) — run from xtest branch with tightened assertions against this platform branch > **Note:** The platform `platform-xtest` CI job runs xtests from `main` (`opentdf/tests/.github/workflows/xtest.yml@main`), which still has the permissive assertions. The strict assertions in opentdf/tests#422 should be merged after this PR lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added cross-platform SDK compatibility testing workflow documentation for contributors * **Bug Fixes** * Enhanced KAS error classification to distinguish between request/configuration failures and integrity issues * Improved rewrap operation error messages for better debugging while protecting sensitive information * **Tests** * Expanded KAS error handling test coverage for additional error scenarios <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mary Dickson <mary.dickson@virtru.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dmihalcik-virtru
approved these changes
Apr 8, 2026
📝 WalkthroughWalkthroughThis change introduces support for a new "tamper-error-split" feature that differentiates KAS 400 errors between generic "bad request" (treated as tamper) and descriptive "misconfiguration" (treated as request error). The framework and tests have been updated to validate this error handling differentiation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
xtest/README.md (1)
119-131: Consider adding a one-line local repro command next to this CI flow.A short “after CI, reproduce locally” command (e.g., source env then run via
uv) would make this section even more actionable.As per coding guidelines
xtest/**: Run tests with uv run pytest in xtest directory after sourcing test.env for proper environment configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xtest/README.md` around lines 119 - 131, Add a one-line local repro command next to the CI instructions showing how to reproduce the xtest run locally: instruct the developer to source the test.env and run the xtest suite using the uv runner (e.g., "source test.env && uv run pytest" or equivalent) and place this short command in the xtest README near the CI flow so readers know how to run xtest locally after CI; reference the xtest directory, test.env, and the uv run pytest invocation so maintainers can find and run the local command easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xtest/test_tdfs.py`:
- Around line 578-585: The negative assertion that checks exc.output for "KAS
request error" is too narrow; update the check in the block gated by
decrypt_sdk.supports("tamper-error-split") to reject any KAS request-error
variants (e.g., "KAS request error", "KASRequestError", "ErrKASRequestError") by
using a case-insensitive regex that matches these variants against exc.output
(still using re.search) so the test fails for any of those tokens rather than
only the exact phrase.
---
Nitpick comments:
In `@xtest/README.md`:
- Around line 119-131: Add a one-line local repro command next to the CI
instructions showing how to reproduce the xtest run locally: instruct the
developer to source the test.env and run the xtest suite using the uv runner
(e.g., "source test.env && uv run pytest" or equivalent) and place this short
command in the xtest README near the CI flow so readers know how to run xtest
locally after CI; reference the xtest directory, test.env, and the uv run pytest
invocation so maintainers can find and run the local command easily.
🪄 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: a85c0f8b-b0bf-460b-a67d-3568201dd3e7
📒 Files selected for processing (4)
xtest/README.mdxtest/sdk/go/cli.shxtest/tdfs.pyxtest/test_tdfs.py
Comment on lines
+578
to
+585
| if decrypt_sdk.supports("tamper-error-split"): | ||
| # SDK distinguishes tamper from misconfiguration — assert tamper specifically | ||
| assert re.search(b"tamper", exc.output, re.IGNORECASE), ( | ||
| f"Expected tamper error, got: [{exc.output}]" | ||
| ) | ||
| assert not re.search(b"KAS request error", exc.output, re.IGNORECASE), ( | ||
| f"Policy binding failure must not be classified as KAS request error: [{exc.output}]" | ||
| ) |
There was a problem hiding this comment.
Broaden the strict-mode negative match for request-error classification tokens.
This currently forbids only KAS request error. If output contains KASRequestError/ErrKASRequestError, the test can pass despite wrong classification.
Proposed patch
- assert not re.search(b"KAS request error", exc.output, re.IGNORECASE), (
+ forbidden = b"|".join(
+ re.escape(p)
+ for p in [b"KAS request error", b"KASRequestError", b"ErrKASRequestError"]
+ )
+ assert not re.search(forbidden, exc.output, re.IGNORECASE), (
f"Policy binding failure must not be classified as KAS request error: [{exc.output}]"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xtest/test_tdfs.py` around lines 578 - 585, The negative assertion that
checks exc.output for "KAS request error" is too narrow; update the check in the
block gated by decrypt_sdk.supports("tamper-error-split") to reject any KAS
request-error variants (e.g., "KAS request error", "KASRequestError",
"ErrKASRequestError") by using a case-insensitive regex that matches these
variants against exc.output (still using re.search) so the test fails for any of
those tokens rather than only the exact phrase.
X-Test Results✅ java-main |
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.
Summary
Updates xtest policy tamper assertions to match the revised error classification in opentdf/platform#3166, and adds instructions for running xtests against platform feature branches.
Changes
assert_kas_request_error→assert_policy_tamper_error— policy binding failures are integrity failures (tamper), not KAS request errorstamper-error-splitfeature flag (Go SDK >= 0.14.0) for SDKs that distinguish tamper from misconfiguration"tamper"IS present and"KAS request error"is NOT — instead of the old permissive "any of these strings" matchContext
KAS intentionally returns a generic
"bad request"for policy binding failures to avoid leaking information about secret key computations. The SDK classifies these underErrTampered(notErrKASRequestError). The old assertion accepted both classifications and would never fail.Companion to: opentdf/platform#3166
Test plan
ruff checkandruff formatpass🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
tamper-error-splitfeature support to differentiate KAS 400 errors between generic "bad request" responses (treated as tampering) and specific "misconfiguration" responses (treated as configuration errors).