Skip to content

fix(core): distinguish rewrap denials - #4204

Merged
elizabethhealy merged 3 commits into
mainfrom
codex/DSPX-5091-rewrap-denial-result
Oct 9, 2026
Merged

elizabethhealy merged 3 commits into
mainfrom
codex/DSPX-5091-rewrap-denial-result

Conversation

@elizabethhealy

@elizabethhealy elizabethhealy commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Proposed Changes

  • Add a dedicated RewrapDenied audit helper that records ActionResultFailure.
  • Use RewrapDenied when an existing GetDecision authorization result rejects a rewrap.
  • Preserve RewrapFailure and ActionResultError for technical rewrap failures.
  • Add focused coverage asserting that authorization DENY emits a failure-result rewrap audit event and PERMIT emits a success-result event.

Scope

This change only reclassifies audit events that are already emitted for policy authorization denials. It does not add new audit-emission paths.

Existing cases that do not currently emit a rewrap audit event remain unchanged and should be addressed separately, including:

  • GetDecision or GetDecisionMultiResource service/RPC errors.
  • Policy-binding validation failures.
  • Wrapped-key or other pre-authorization key-validation failures.

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (not required for this change)
  • I have added or updated documentation (not required for this change)

Testing Instructions

From service/:

go test ./kas/access ./logger/audit -count=1

The focused KAS access and audit logger packages pass locally.

Summary by CodeRabbit

  • Bug Fixes
    • Rewrap audit events now distinguish policy-denied access from other errors: denied attempts are recorded as failures, successful attempts as successes, and other rewrap errors retain the error result.

Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: db4975f5-cb69-4837-a15b-5bd480266af9

📥 Commits

Reviewing files that changed from the base of the PR and between 6d3a01a and c4c2afc.


📒 Files selected for processing (5)
  • service/kas/access/rewrap.go
  • service/kas/access/rewrap_test.go
  • service/logger/audit/logger.go
  • service/logger/audit/logger_test.go
  • service/logger/audit/rewrap.go

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



📝 Walkthrough

Walkthrough

Denied TDF3 rewraps now use a dedicated audit helper. The audit event records a failure result for denied access, while successful rewraps record success and other failures retain the error result.

Changes

TDF3 Rewrap Audit Results

Layer / File(s) Summary
Rewrap audit result semantics
service/logger/audit/rewrap.go, service/logger/audit/logger.go, service/logger/audit/logger_test.go
The audit parameters and helpers distinguish denied rewraps from other failures. Denied rewraps record a failure result. Tests cover the denied helper and its processor-error handling.
Denied TDF3 rewrap integration
service/kas/access/rewrap.go, service/kas/access/rewrap_test.go
Denied access calls RewrapDenied; audit errors continue to be logged before the KAO is marked forbidden. Tests check audit results for permit and deny decisions.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: dmihalcik-virtru


Merge Risk: ⚪ Minimal · up to c4c2a

No actionable merge-blocking risk is evident from the supplied changes; merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: distinguishing authorization denials from technical rewrap failures in audit events.
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.


  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 audit trail,
And sees denied events marked well.
A permit earns success,
A failure keeps its status,
Then hops away, content.

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

@github-actions github-actions Bot added comp:kas Key Access Server size/s labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 299.706434ms
Throughput 333.66 requests/second

TDF3 Benchmark Results

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 34.348011974s
Average Latency 342.707878ms
Throughput 145.57 requests/second

Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
@elizabethhealy elizabethhealy changed the title fix(core): distinguish rewrap denials fix(core): DONT MERGE distinguish rewrap denials Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 406.376901ms
Throughput 246.08 requests/second

TDF3 Benchmark Results

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 42.436842377s
Average Latency 422.933062ms
Throughput 117.82 requests/second

Signed-off-by: Elizabeth Healy <ehealy@virtru.com>
@elizabethhealy elizabethhealy changed the title fix(core): DONT MERGE distinguish rewrap denials fix(core): distinguish rewrap denials Oct 9, 2026
@elizabethhealy
elizabethhealy marked this pull request as ready for review October 9, 2026 15:17
@elizabethhealy
elizabethhealy requested review from a team as code owners October 9, 2026 15:17
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 330.128477ms
Throughput 302.91 requests/second

TDF3 Benchmark Results

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 37.186780887s
Average Latency 371.027078ms
Throughput 134.46 requests/second

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • sdk
  • service
  • lib/fixtures
  • tests-bdd

See the workflow run for details.

@elizabethhealy
elizabethhealy added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit 4d9e555 Oct 9, 2026
83 of 87 checks passed
@elizabethhealy
elizabethhealy deleted the codex/DSPX-5091-rewrap-denial-result branch October 9, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:kas Key Access Server size/s

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants