Skip to content

Conversation

@elizabethhealy
Copy link
Member

@elizabethhealy elizabethhealy commented Nov 18, 2025

Proposed Changes

  • Added comprehensive nil checks for requests, policies, and key access objects
  • Updated tdf3Rewrap and nanoTDFRewrap functions to return errors instead of silently failing
  • Added validation for empty wrapped keys and unsupported key types
  • Improved error handling in the Rewrap main handler

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@github-actions github-actions bot added comp:kas Key Access Server size/s labels Nov 18, 2025
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 175.592381ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 101.849407ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 372.960374ms
Throughput 268.12 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.51537425s
Average Latency 393.390479ms
Throughput 126.53 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.747360198s
Average Latency 276.530845ms
Throughput 180.20 requests/second

@elizabethhealy
Copy link
Member Author

/gemini review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds defensive nil-checking and error handling to prevent panics in the KAS rewrap service when processing malformed requests. The changes ensure that nil requests, policies, and key access objects are handled gracefully with appropriate error responses instead of causing service crashes.

Key Changes:

  • Added comprehensive nil checks for requests, policies, and key access objects
  • Updated tdf3Rewrap and nanoTDFRewrap functions to return errors instead of silently failing
  • Added validation for empty wrapped keys and unsupported key types
  • Improved error handling in the Rewrap main handler

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
service/kas/access/rewrap.go Added nil checks, error handling, and validation for requests, policies, and key access objects; updated function signatures to propagate errors
service/kas/access/rewrap_test.go Added comprehensive test coverage for nil request handling in verifyRewrapRequests and verifyNanoRewrapRequests functions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request significantly improves the robustness of the Key Access Service by adding numerous checks to prevent panics on malformed requests. The changes are well-implemented and include comprehensive unit tests for the new error handling paths. My review includes a few suggestions to further refine the error handling logic for consistency and clarity, particularly regarding the return values on error paths in batch processing.

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 184.729268ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 108.279289ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 360.690666ms
Throughput 277.25 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.086285865s
Average Latency 389.403131ms
Throughput 127.92 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.62505459s
Average Latency 275.309793ms
Throughput 181.00 requests/second

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 138.334838ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 83.231216ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 383.372263ms
Throughput 260.84 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 42.214742866s
Average Latency 420.203103ms
Throughput 118.44 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 29.936430998s
Average Latency 298.347731ms
Throughput 167.02 requests/second

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 145.315858ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 77.085075ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 370.920363ms
Throughput 269.60 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.151709055s
Average Latency 390.024447ms
Throughput 127.71 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.286423856s
Average Latency 271.798351ms
Throughput 183.24 requests/second

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 149.194693ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 80.941669ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 382.795872ms
Throughput 261.24 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 39.619798351s
Average Latency 394.300427ms
Throughput 126.20 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.44121559s
Average Latency 273.43289ms
Throughput 182.21 requests/second

@github-actions
Copy link
Contributor

@elizabethhealy elizabethhealy marked this pull request as ready for review November 18, 2025 15:12
@elizabethhealy elizabethhealy requested review from a team as code owners November 18, 2025 15:12
@elizabethhealy elizabethhealy added this pull request to the merge queue Nov 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 18, 2025
@elizabethhealy elizabethhealy added this pull request to the merge queue Nov 18, 2025
Merged via the queue into main with commit 182b463 Nov 18, 2025
63 of 66 checks passed
@elizabethhealy elizabethhealy deleted the dspx-1776-fix-kas-panic branch November 18, 2025 22:37
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.

4 participants