feat(policy): Update key status's and UpdateKey rpc. #2315
Merged
Conversation
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
github-actions
bot
added
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @c-r33d, 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!
Summary of Changes
Hello team,
Gemini here with a summary of this pull request from @c-r33d. The primary goals of this PR are to update the KeyStatus enum value from INACTIVE to ROTATED and to remove the ability to update a key's status directly via the UpdateKey RPC. The UpdateKey RPC is now intended solely for updating key metadata. Key status changes, specifically to ROTATED, will now be handled as part of the RotateKey RPC process.
Highlights
- KeyStatus Enum Renamed: The
KEY_STATUS_INACTIVEenum value has been renamed toKEY_STATUS_ROTATEDacross the codebase and documentation. - UpdateKey RPC Modified: The
UpdateKeyRequestno longer includes thekey_statusfield, removing the ability to change a key's status using this RPC. It now focuses only on metadata updates. - RotateKey RPC Updates Status: The
RotateKeyRPC implementation has been updated to explicitly set the status of the old key toKEY_STATUS_ROTATEDafter a successful rotation. - Documentation and Tests Updated: Documentation (gRPC and OpenAPI) and integration tests have been updated to reflect the renamed enum value and the changes to the
UpdateKeyandRotateKeyRPCs.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus documentation. - Removed the
key_statusfield from theUpdateKeyRequestdocumentation.
- Renamed
- docs/openapi/policy/actions/actions.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/attributes/attributes.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/kasregistry/key_access_server_registry.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/namespaces/namespaces.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/registeredresources/registered_resources.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/resourcemapping/resource_mapping.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/subjectmapping/subject_mapping.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- docs/openapi/policy/unsafe/unsafe.swagger.json
- Renamed
KEY_STATUS_INACTIVEtoKEY_STATUS_ROTATEDin the KeyStatus enum definition.
- Renamed
- protocol/go/policy/kasregistry/key_access_server_registry.pb.go
- Removed the
KeyStatusfield and its getter from theUpdateKeyRequeststruct. - Updated protobuf descriptor and dependency index to reflect the removal of the
key_statusfield.
- Removed the
- protocol/go/policy/objects.pb.go
- Renamed the
KeyStatus_KEY_STATUS_INACTIVEconstant toKeyStatus_KEY_STATUS_ROTATED(value 2). - Updated the
KeyStatus_nameandKeyStatus_valuemaps accordingly. - Updated protobuf descriptor to reflect the name change.
- Renamed the
- service/integration/kas_registry_key_test.go
- Removed the
Test_UpdateKeyStatus_Successtest case. - Updated existing
RotateKeytests to assert the old key's status isKEY_STATUS_ROTATED(lines 466, 600, 696). - Renamed
Test_SetBaseKey_CannotSetInactiveKey_FailstoTest_SetBaseKey_CannotSetNonActiveKey_Fails(line 955). - Modified
Test_SetBaseKey_CannotSetNonActiveKey_Failsto useRotateKeyto set the status toROTATEDbefore attempting to set it as base (lines 997-1014). - Added
Test_RotateKey_MetadataUnchanged_Successto verify metadata is preserved during rotation (lines 1026-1087).
- Removed the
- service/policy/db/key_access_server_registry.go
- Removed logic to update
KeyStatusfrom theUpdateKeyfunction, making it metadata-only (lines 540-543). - Modified
RotateKeyto explicitly set the old key's status toKEY_STATUS_ROTATEDusing an internal helper (lines 658-660). - Added
updateKeyInternalhelper function for key updates (lines 803-817). - Moved helper functions
populateChangeMappings,rotatePublicKeyTables, androtateBaseKeywithin the file (lines 819-917).
- Removed logic to update
- service/policy/kasregistry/key_access_server_registry.proto
- Removed the
key_statusfield and its validation rule from theUpdateKeyRequestmessage (lines 471-474, 481-485).
- Removed the
- service/policy/kasregistry/key_access_server_registry_keys_test.go
- Removed test cases for updating key status via
UpdateKeyRequest(lines 720-736). - Removed the
KeyStatusfield from theUpdateKeyRequestused in a test case (line 732).
- Removed test cases for updating key status via
- service/policy/objects.proto
- Renamed the
KEY_STATUS_INACTIVEenum value toKEY_STATUS_ROTATED(value 2) (line 466).
- Renamed the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A key once inactive,
Now status rotated,
Update RPC changed,
Metadata arranged,
New tests validated.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on two changes: renaming the KEY_STATUS_INACTIVE to KEY_STATUS_ROTATED and removing the ability to update a key's status directly via the UpdateKey RPC. The changes are consistently applied across documentation, protobuf definitions, generated Go code, and tests.
The refactoring in service/policy/db/key_access_server_registry.go to introduce updateKeyInternal seems reasonable to separate concerns for metadata updates versus status changes during rotation.
One critical area to confirm is how a key's status is now set to KEY_STATUS_COMPROMISED, as the UpdateKey RPC no longer supports direct status modification. Additionally, there's a minor bug in one of the new integration tests.
The PR description's checklist for unit tests, integration tests, and documentation is currently unchecked. While the diff shows modifications to existing tests and updates to generated documentation, please ensure these are comprehensive and the checklist is updated accordingly.
Summary of Findings
- Mechanism for Setting Key Status to COMPROMISED: The
UpdateKeyRPC no longer allows setting thekey_status. WhileRotateKeyhandles setting the old key toROTATED, it's unclear how a key would now be marked asKEY_STATUS_COMPROMISED. This could be a critical gap if no alternative mechanism exists. - Test Assertion Bug: In
service/integration/kas_registry_key_test.go, the new testTest_RotateKey_MetadataUnchanged_Successhas an assertions.NotNil(rotateKey)whererotateKeyis a string constant. This should likely assert the non-nil status of the key creation response object. - PR Checklist: The PR description's checklist items for tests and documentation are unchecked. Please ensure these are up-to-date to reflect the status of testing and documentation for the changes.
Merge Readiness
This pull request makes significant changes to key status management, aligning with the stated objectives. The code modifications are generally consistent and well-implemented.
However, there is a high-severity concern regarding the mechanism for setting a key's status to COMPROMISED, as the previous method via UpdateKey RPC has been removed. It's crucial to clarify how this state will be managed going forward to ensure security and operational integrity. Additionally, a minor bug in a new integration test needs to be addressed.
Given these points, I recommend addressing the high-severity concern and the test bug before merging. I am unable to approve pull requests, but based on this review, changes are requested.
Benchmark results, click to expandBenchmark Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
strantalis
approved these changes
May 27, 2025
Benchmark results, click to expandBenchmark Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
This was referenced May 27, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 27, 2025
🤖 I have created a release *beep* *boop* --- ## [0.3.6](protocol/go/v0.3.5...protocol/go/v0.3.6) (2025-05-27) ### Features * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy** Rename key context structures. ([#2318](#2318)) ([4cb28a9](4cb28a9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Chris Reed <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 29, 2025
🤖 I have created a release *beep* *boop* --- ## [0.5.4](service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([#2264](#2264)) ([7afefb7](7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([#2146](#2146)) ([0fdc259](0fdc259)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([#2325](#2325)) ([ca94425](ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([#2321](#2321)) ([207d10d](207d10d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Elizabeth Healy <[email protected]>
strantalis
pushed a commit
to strantalis/platform
that referenced
this pull request
May 29, 2025
🤖 I have created a release *beep* *boop* --- ## [0.5.4](opentdf/platform@service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([opentdf#2264](opentdf#2264)) ([7afefb7](opentdf@7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([opentdf#2146](opentdf#2146)) ([0fdc259](opentdf@0fdc259)) * **policy:** Default Platform Keys ([opentdf#2254](opentdf#2254)) ([d7447fe](opentdf@d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([opentdf#2315](opentdf#2315)) ([7908db9](opentdf@7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([opentdf#2325](opentdf#2325)) ([ca94425](opentdf@ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([opentdf#2321](opentdf#2321)) ([207d10d](opentdf@207d10d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Elizabeth Healy <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
🤖 I have created a release *beep* *boop* --- ## [0.7.0](protocol/go/v0.6.2...protocol/go/v0.7.0) (2025-08-08) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) * **core:** Require go 1.23+ ([#1979](#1979)) ### Features * add ability to retrieve policy resources by id or name ([#1901](#1901)) ([deb4455](deb4455)) * **authz:** authz v2, ers v2 protos and gencode for ABAC with actions & registered resource ([#2124](#2124)) ([ea7992a](ea7992a)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **authz:** sensible request limit upper bounds ([#2526](#2526)) ([b3093cc](b3093cc)) * **core:** adds bulk rewrap to sdk and service ([#1835](#1835)) ([11698ae](11698ae)) * **core:** EXPERIMENTAL: EC-wrapped key support ([#1902](#1902)) ([652266f](652266f)) * **core:** Require go 1.23+ ([#1979](#1979)) ([164c922](164c922)) * **core:** v2 ERS with proto updates ([#2210](#2210)) ([a161ef8](a161ef8)) * **policy:** add enhanced standard/custom actions protos ([#2020](#2020)) ([bbac53f](bbac53f)) * **policy:** Add legacy keys. ([#2613](#2613)) ([57370b0](57370b0)) * **policy:** Add list key mappings rpc. ([#2533](#2533)) ([fbc2724](fbc2724)) * **policy:** add obligation protos ([#2579](#2579)) ([50882e1](50882e1)) * **policy:** Add validation to delete keys ([#2576](#2576)) ([cc169d9](cc169d9)) * **policy:** add values to CreateObligationRequest ([#2614](#2614)) ([94535cc](94535cc)) * **policy:** adds new public keys table ([#1836](#1836)) ([cad5048](cad5048)) * **policy:** Allow the deletion of a key. ([#2575](#2575)) ([82b96f0](82b96f0)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Change return type for delete key proto. ([#2566](#2566)) ([c1ae924](c1ae924)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** DSPX-1018 NDR retrieval by FQN support ([#2131](#2131)) ([0001041](0001041)) * **policy:** DSPX-1057 registered resource action attribute values (protos only) ([#2217](#2217)) ([6375596](6375596)) * **policy:** DSPX-893 NDR define crud protos ([#2056](#2056)) ([55a5c27](55a5c27)) * **policy:** DSPX-902 NDR service crud protos only (1/2) ([#2092](#2092)) ([24b6cb5](24b6cb5)) * **policy:** Finish resource mapping groups ([#2224](#2224)) ([5ff754e](5ff754e)) * **policy:** key management crud ([#2110](#2110)) ([4c3d53d](4c3d53d)) * **policy:** Key management proto ([#2115](#2115)) ([561f853](561f853)) * **policy:** Modify get request to search for keys by kasid with keyid. ([#2147](#2147)) ([780d2e4](780d2e4)) * **policy:** Return KAS Key structure ([#2172](#2172)) ([7f97b99](7f97b99)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** rotate keys rpc ([#2180](#2180)) ([0d00743](0d00743)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * add pagination to list public key mappings response ([#1889](#1889)) ([9898fbd](9898fbd)) * **core:** Allow 521 curve to be used ([#2485](#2485)) ([aaf43dc](aaf43dc)) * **core:** Fixes protoJSON parse bug on ec rewrap ([#1943](#1943)) ([9bebfd0](9bebfd0)) * **core:** Update fixtures and flattening in sdk and service ([#1827](#1827)) ([d6d6a7a](d6d6a7a)) * **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE GO-2025-3563 ([#2061](#2061)) ([9c16843](9c16843)) * **policy:** protovalidate deprecated action types and removal of gRPC gateway in subject mappings svc ([#2377](#2377)) ([54a6de0](54a6de0)) * **policy:** remove gRPC gateway in policy except where needed ([#2382](#2382)) ([1937acb](1937acb)) * **policy:** remove new public keys rpc's ([#1962](#1962)) ([5049bab](5049bab)) * **policy:** remove predefined rules in actions protos ([#2069](#2069)) ([060f059](060f059)) * **policy:** return kas uri on keys for definition, namespace and values ([#2186](#2186)) ([6c55fb8](6c55fb8)) * **sdk:** Fix compatibility between bulk and non-bulk rewrap ([#1914](#1914)) ([74abbb6](74abbb6)) * update key_mode to provide more context ([#2226](#2226)) ([44d0805](44d0805)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Krish Suchak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed Changes
1.) Update KeyStatus from inactive to rotated.
2.) Remove the ability to update a key's status.
Checklist
Testing Instructions