fix(policy): reject concurrent and non-active key rotations by c-r33d · Pull Request #4200 · opentdf/platform

c-r33d · 2026-10-08T18:14:14Z

Proposed Changes

Require an ACTIVE source before key rotation, returning a logged FailedPrecondition error (key must be ACTIVE). Atomically enforce that status when updating the source so concurrent requests produce one successor; a stale request returns the existing NotFound error. Preserve key metadata and mapping/base-key transfers.

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

Race-enabled tests verify non-base-key rotation, rejection of a rotated source, and a single concurrent successor with no loser record. All 13 CLI rotation BATS cases pass, including the early error and backend logging. Focused unit tests and diff lint pass. Full checks encounter existing lint/vulnerability findings and a round-trip suite requiring a separately provisioned platform.

Summary by CodeRabbit

Bug Fixes
- Key rotation now proceeds only for active keys; attempts to rotate an inactive key return a clear error.
- Concurrent rotation requests no longer create multiple successor keys; only one request succeeds.
- Rotation preserves the selection of an unrelated base key and leaves a key’s status unchanged if it is no longer active.

Signed-off-by: Chris Reed <creed@virtru.com>

coderabbitai · 2026-10-08T18:14:24Z

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: b0f968e4-6b4e-4302-999b-5603ca7b50a4

📥 Commits

Reviewing files that changed from the base of the PR and between 28221af and b6cd558.

📒 Files selected for processing: 1

service/policy/db/key_access_server_registry.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

Key rotation now rejects keys that are not ACTIVE and updates a key only when its persisted status is ACTIVE. Tests cover repeated, stale-state, and concurrent rotation attempts, along with base-key selection.

Changes: Key rotation

Layer / File(s)	Summary
ACTIVE status guard `service/pkg/db/errors.go`, `service/policy/kasregistry/key_access_server_registry.go`	`RotateKey` rejects a retrieved key whose status is not `KEY_STATUS_ACTIVE`. The error maps to Connect `CodeFailedPrecondition` with the message “key must be ACTIVE.”
Conditional status update `service/policy/db/queries/key_access_server_registry.sql`, `service/policy/db/key_access_server_registry.sql.go`, `service/policy/db/key_access_server_registry.go`	The `rotateActiveKey` query changes status only when the ID matches and the current status is ACTIVE. `RotateKey` returns `db.ErrNotFound` when no row changes and reloads the rotated key after a successful update.
Rotation behavior tests `service/integration/kas_registry_key_test.go`, `otdfctl/e2e/kas-keys.bats`	Tests check base-key preservation, stale persisted status, and concurrent rotation outcomes. The end-to-end test checks the failed-precondition response for a repeated rotation attempt.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: jakedoublev

Merge Risk: ⚪ Minimal · up to b6cd5

The rotation changes reject inactive keys and prevent duplicate successors; stale concurrent requests retain the documented NotFound response. No actionable merge risk introduced by this PR remains.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		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 1 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main changes: rejecting non-active key rotations and preventing concurrent rotation races.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · 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

_{I’m a rabbit watching keys rotate,
ACTIVE keys pass through the gate.
One successor wins the race,
Stale keys stop and hold their place.
I thump once for base keys kept,
Then curl up while checks are met.}

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

github-actions · 2026-10-08T18:18:14Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	218.398465ms
Throughput	457.88 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	23.973842991s
Average Latency	239.32009ms
Throughput	208.56 requests/second

github-actions · 2026-10-08T18:28:29Z

X-Test Results

✅ js@main-pull-4200
govulncheck-failure-2
✅ java@main-v0.9.0
✅ go@pull-4200-v0.9.0
✅ go@pull-4200-main
bats-test-results
✅ java@main-main
✅ go@main-main
✅ java@main-pull-4200
cukes-report
✅ go@pull-4200-pull-4200
✅ go@main-v0.9.0
✅ js@main-v0.9.0
✅ go@main-pull-4200
govulncheck-failure-0
✅ js@main-main
opentdf~~platform~~HL8D2U.dockerbuild
govulncheck-failure-7

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions · 2026-10-08T18:40:50Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	344.201401ms
Throughput	290.53 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	33.943314409s
Average Latency	338.557516ms
Throughput	147.30 requests/second

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions · 2026-10-08T18:48:56Z

X-Test Results

✅ java@main-pull-4200
✅ java@main-v0.9.0
✅ js@main-pull-4200
✅ go@pull-4200-v0.9.0
✅ go@pull-4200-pull-4200
✅ java@main-main
✅ go@main-v0.9.0
bats-test-results
✅ js@main-v0.9.0
govulncheck-failure-2
✅ go@main-pull-4200
✅ go@main-main
✅ go@pull-4200-main
opentdf~~platform~~O54H92.dockerbuild
govulncheck-failure-0
✅ js@main-main
govulncheck-failure-7

github-actions · 2026-10-08T18:53:17Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	400.382412ms
Throughput	249.76 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	45.28702015s
Average Latency	451.468928ms
Throughput	110.41 requests/second

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions · 2026-10-08T19:02:05Z

X-Test Failure Report

bats-test-results
✅ go@pull-4200-pull-4200
✅ go@pull-4200-v0.9.0
✅ go@main-v0.9.0
✅ go@main-main
govulncheck-failure-7
✅ java@main-v0.9.0
✅ js@main-pull-4200
govulncheck-failure-0
✅ go@main-pull-4200
govulncheck-failure-2
✅ js@main-v0.9.0
✅ java@main-pull-4200
✅ java@main-main
✅ go@pull-4200-main
✅ js@main-main
opentdf~~platform~~UO5BXV.dockerbuild

github-actions · 2026-10-08T19:04:43Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	346.823592ms
Throughput	288.33 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	35.426474324s
Average Latency	353.404507ms
Throughput	141.14 requests/second

github-actions · 2026-10-08T19:15:24Z

X-Test Results

✅ go@pull-4200-pull-4200
✅ go@main-main
✅ java@main-main
✅ go@main-v0.9.0
✅ js@main-pull-4200
✅ js@main-v0.9.0
✅ go@pull-4200-v0.9.0
cukes-report
✅ java@main-v0.9.0
✅ go@main-pull-4200
✅ go@pull-4200-main
bats-test-results
✅ java@main-pull-4200
govulncheck-failure-2
govulncheck-failure-7
opentdf~~platform~~P0CF88.dockerbuild
govulncheck-failure-0
✅ js@main-main

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions · 2026-10-08T19:44:33Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	277.694119ms
Throughput	360.11 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	30.08556217s
Average Latency	300.270328ms
Throughput	166.19 requests/second

github-actions · 2026-10-08T19:53:50Z

X-Test Results

✅ go@main-main
✅ js@main-pull-4200
✅ go@main-pull-4200
✅ js@main-main
✅ go@main-v0.9.0
✅ java@main-pull-4200
✅ java@main-v0.9.0
✅ js@main-v0.9.0
govulncheck-failure-7
✅ go@pull-4200-v0.9.0
bats-test-results
✅ go@pull-4200-main
✅ java@main-main
opentdf~~platform~~6YRKQH.dockerbuild
✅ go@pull-4200-pull-4200
govulncheck-failure-0
govulncheck-failure-2

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions · 2026-10-14T13:07:45Z

Benchmark results, click to expand

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	346.191203ms
Throughput	288.86 requests/second

TDF3 Benchmark Results

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	42.471485122s
Average Latency	422.832323ms
Throughput	117.73 requests/second

github-actions · 2026-10-14T13:08:52Z

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

otdfctl
sdk
service
lib/fixtures
tests-bdd

See the workflow run for details.

github-actions · 2026-10-14T13:16:16Z

X-Test Results

✅ go@pull-4200-v0.9.0
✅ go@pull-4200-main
✅ java@main-main
✅ go@pull-4200-pull-4200
✅ java@main-pull-4200
✅ js@main-v0.9.0
✅ go@main-v0.9.0
✅ js@main-pull-4200
✅ go@main-pull-4200
✅ java@main-v0.9.0
✅ go@main-main
bats-test-results
govulncheck-failure-2
opentdf~~platform~~IXORH6.dockerbuild
govulncheck-failure-0
✅ js@main-main
govulncheck-failure-4
govulncheck-failure-1
govulncheck-failure-7

fix(policy): serialize key rotation and record successor

5330930

Signed-off-by: Chris Reed <creed@virtru.com>

github-actions Bot added comp:db DB component comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) labels Oct 8, 2026

github-actions Bot added the size/s label Oct 8, 2026

c-r33d commented Oct 8, 2026

View reviewed changes

Comment thread service/policy/db/key_access_server_registry.go

fix(policy): preserve key metadata during rotation

f2c2cb1

Signed-off-by: Chris Reed <creed@virtru.com>

c-r33d changed the title ~~fix(policy): serialize key rotation and record successor~~ fix(policy): reject concurrent and non-active key rotations Oct 8, 2026

fix(policy): reject stale rotation before reading source

95da6b6

Signed-off-by: Chris Reed <creed@virtru.com>

fix(policy): reuse existing rotation error contract

b45ea00

Signed-off-by: Chris Reed <creed@virtru.com>

fix(policy): validate rotation sources and consolidate coverage

28221af

Signed-off-by: Chris Reed <creed@virtru.com>

c-r33d marked this pull request as ready for review October 8, 2026 19:41

c-r33d requested review from a team as code owners October 8, 2026 19:41

fix(policy): skip rotation check for incomplete base keys

b6cd558

Signed-off-by: Chris Reed <creed@virtru.com>

strantalis approved these changes Oct 14, 2026

View reviewed changes

c-r33d added this pull request to the merge queue Oct 14, 2026

Merged via the queue into main with commit 5679545 Oct 14, 2026
81 of 85 checks passed

c-r33d deleted the codex/cks-rotation-active-guard branch October 14, 2026 16:30

This was referenced Oct 14, 2026

chore(main): release service 0.29.0 #4168

Merged

chore(main): release otdfctl 0.39.0 #4101

Merged

Conversation

c-r33d commented Oct 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Checklist

Testing Instructions

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 8, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

Uh oh!

github-actions Bot commented Oct 8, 2026

X-Test Results

Uh oh!

github-actions Bot commented Oct 8, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

github-actions Bot commented Oct 8, 2026

X-Test Results

Uh oh!

github-actions Bot commented Oct 8, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

github-actions Bot commented Oct 8, 2026

X-Test Failure Report

Uh oh!

github-actions Bot commented Oct 8, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

github-actions Bot commented Oct 8, 2026

X-Test Results

Uh oh!

github-actions Bot commented Oct 8, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

github-actions Bot commented Oct 8, 2026

X-Test Results

Uh oh!

github-actions Bot commented Oct 14, 2026

Bulk Benchmark Results

TDF3 Benchmark Results

Uh oh!

github-actions Bot commented Oct 14, 2026

⚠️ Govulncheck found vulnerabilities ⚠️

Uh oh!

github-actions Bot commented Oct 14, 2026

X-Test Results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

c-r33d commented Oct 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading