-
Notifications
You must be signed in to change notification settings - Fork 24
feat(policy): Return Simple Kas Keys from non-Key RPCs #2387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-code-assist here with a summary of this pull request. The primary goal of this PR is to standardize the representation of Key Access Server (KAS) keys returned by policy-related gRPC services. Instead of returning the full KasKey object, which includes sensitive or internal details like private key contexts or provider configurations, non-key specific RPCs will now return a simplified SimpleKasKey object. This object contains only the essential public information needed by clients: the KAS URI, KAS ID, and the public key details (algorithm, KID, and PEM). This change improves the API's clarity and reduces unnecessary data exposure.
Highlights
- Simplified KAS Key Representation: Introduced
SimpleKasKeyandSimpleKasPublicKeymessage definitions inpolicy.objects.prototo provide a minimal representation of KAS keys for use in policy-related responses. - API Response Updates: Modified various policy service RPCs (e.g., GetAttribute, ListAttributesByFqns, GetAttributeValue, GetNamespace, ListKeyAccessServers, GetKeyAccessServer, GetBaseKey) to return lists of
SimpleKasKeyobjects instead ofKasKeyobjects. - Database Query Adjustments: Updated SQL queries (
service/policy/db/query.sql) to fetch the necessary data and format it into theSimpleKasKeystructure, including joining with thekey_access_serverstable to retrieve KAS URI and ID. - Integration Test Adaptations: Updated integration tests across attribute FQNs, attribute values, attributes, KAS registry keys, and KAS registry to handle the new
SimpleKasKeyreturn type. This involved fetching the fullKasKeyseparately in some cases to perform comprehensive validation against the simplified data returned by the policy RPCs. - Internal Helper Updates: Refactored internal database helper functions (
service/pkg/db/marshalHelpers.go,service/policy/db/grant_mappings.go) to correctly unmarshal and map the newSimpleKasKeystructure.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Added documentation links for
policy.SimpleKasKeyandpolicy.SimpleKasPublicKey. - Removed documentation links for
policy.kasregistry.SimpleKasKeyandpolicy.kasregistry.SimpleKasPublicKey. - Updated references from
policy.KasKeytopolicy.SimpleKasKeyin the documentation forAttribute,KeyAccessServer,Namespace, andValue.
- Added documentation links for
- service/integration/attribute_fqns_test.go
- Imported
encoding/base64. - Modified tests (
TestGetAttributeByFqn_WithAttributeDefKeysAssociated,TestGetAttributeByFqn_WithAttributeValueKeysAssociated,TestGetAttributeByFqn_WithKeysAssociatedWithNamespace,TestGetAttributeByFqn_WithKeysAssociatedAttributes_MultipleAttributes,TestGetAttributeByValueFqns_KAS_Keys_Returned) to fetch the fullpolicy.KasKeyseparately usingGetKeybefore validating theSimpleKasKeyreturned by the policy RPCs. - Updated assertions to check fields on the returned
SimpleKasKeyobjects. - Added
validateSimpleKasKeyhelper function to validateSimpleKasKeystructure.
- Imported
- service/integration/attribute_values_test.go
- Modified
Test_AssignPublicKeyToAttributeValue_Succeedsto fetch the fullpolicy.KasKeyseparately usingGetKeybefore validating theSimpleKasKeyreturned. - Updated assertions to check fields on the returned
SimpleKasKeyobjects. - Removed redundant KAS server lookup.
- Modified
- service/integration/attributes_test.go
- Modified
Test_AssociatePublicKeyToAttribute_Succeedsto fetch the fullpolicy.KasKeyseparately usingGetKeybefore validating theSimpleKasKeyreturned. - Updated assertions to check fields on the returned
SimpleKasKeyobjects. - Removed redundant KAS server lookup.
- Modified
- service/integration/kas_registry_key_test.go
- Updated
keyCtxconstant value. - Modified
Test_RotateKey_Multiple_Attributes_Values_NamespaceandTest_RotateKey_Two_Attribute_Two_Namespace_0_AttributeValue_Successassertions to checkSimpleKasKeyfields on the rotated keys. - Updated
Test_SetBaseKey_Insert_Successassertions to checkSimpleKasKeyfields on the new base key. - Updated
validatePublicKeyCtxhelper to accept*policy.SimpleKasKey. - Updated
validatePrivatePublicCtxhelper to construct aSimpleKasKeyfor validation.
- Updated
- service/integration/kas_registry_test.go
- Modified
getKasToKeysFixtureMapto fetch and return[]*policy.KasKey. - Updated
validateKasRegistryKeysto use the newvalidateSimpleKasKeyhelper for validation.
- Modified
- service/integration/namespaces_test.go
- Modified
Test_AssociatePublicKeyToNamespace_Succeedsto fetch the fullpolicy.KasKeyseparately usingGetKeybefore validating theSimpleKasKeyreturned. - Updated assertions to check fields on the returned
SimpleKasKeyobjects.
- Modified
- service/pkg/db/marshalHelpers.go
- Removed unused imports (
encoding/base64,strconv). - Renamed
formatAlgtoFormatAlgand made it exported. - Removed old
UnmarshalSimpleKasKeyfunction. - Added
SimpleKasKeysProtoJSONfunction to unmarshal a JSON array ofpolicy.SimpleKasKey. - Added
UnmarshalSimpleKasKeyfunction to unmarshal a singlepolicy.SimpleKasKey.
- Removed unused imports (
- service/policy/db/attribute_values.go
- Changed the type of the
keysvariable to[]*policy.SimpleKasKeyand useddb.SimpleKasKeysProtoJSONfor unmarshalling.
- Changed the type of the
- service/policy/db/attributes.go
- Added a comment about formatting keys in
attributesValuesProtojson. - Changed the type of the
keysvariable to[]*policy.SimpleKasKeyand useddb.SimpleKasKeysProtoJSONfor unmarshalling inGetAttributeandListAttributesByFqns.
- Added a comment about formatting keys in
- service/policy/db/grant_mappings.go
- Removed unused import (
encoding/base64). - Added
errKasInfoIncompleteerror. - Updated
mapAlgorithmToKasPublicKeyAlgto remove theALGORITHM_UNSPECIFIEDcase. - Changed the input type of
mapKasKeysToGrantsto[]*policy.SimpleKasKey. - Updated
mapKasKeysToGrantslogic to work withSimpleKasKeyfields and added checks for required fields.
- Removed unused import (
- service/policy/db/grant_mappings_test.go
- Removed unused import (
encoding/base64). - Updated test cases to use
policy.SimpleKasKeyandpolicy.SimpleKasPublicKeydirectly. - Added test cases for incomplete KAS information in
SimpleKasKey.
- Removed unused import (
- service/policy/db/key_access_server_registry.go
- Changed the type of the
keysvariable to[]*policy.SimpleKasKeyand useddb.SimpleKasKeysProtoJSONfor unmarshalling inListKeyAccessServersandGetKeyAccessServer. - Changed the return type of
GetBaseKeyto*policy.SimpleKasKey. - Updated
SetBaseKeyOnWellKnownConfigto usedb.FormatAlgand explicitly set the algorithm string.
- Changed the type of the
- service/policy/db/models.go
- Added a comment to the
SelectorValuesfield.
- Added a comment to the
- service/policy/db/query.sql
- Modified queries (
list_key_access_servers,get_key_access_server,get_attribute_by_fqn,list_attributes_by_fqns,get_attribute_value,list_attribute_values_by_fqns,get_namespace,list_namespaces,get_base_key) to construct JSON objects matching theSimpleKasKeystructure. - Added
INNER JOIN key_access_serversto relevant queries to fetch KAS URI and ID. - Updated JSON construction to include
kas_uri,kas_id, and apublic_keyobject withalgorithm(as integer),kid, and decodedpem.
- Modified queries (
- service/policy/kasregistry/key_access_server_registry.proto
- Removed
SimpleKasPublicKeyandSimpleKasKeymessage definitions.
- Removed
- service/policy/objects.proto
- Added
SimpleKasPublicKeyandSimpleKasKeymessage definitions. - Updated the
kas_keysfield type inNamespace,Attribute,Value, andKeyAccessServermessages torepeated SimpleKasKey.
- Added
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 complex and deep,
Now simplified secrets to keep.
Just URI and ID,
And public key, you see,
A lighter load, while you sleep.
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.
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 effectively refactors the KAS key representation by introducing SimpleKasKey and SimpleKasPublicKey in the policy package and updating various RPCs and database queries to use this new structure. The changes are quite extensive and appear to be well-executed, especially the SQL modifications that now handle data transformations like PEM decoding.
Key changes include:
- Moving
SimpleKasKeyandSimpleKasPublicKeydefinitions frompolicy.kasregistrytopolicy/objects.proto. - Updating
SimpleKasKeyto includekas_idandSimpleKasPublicKeyto use thepolicy.Algorithmenum and expect raw PEM strings. - Modifying SQL queries to construct
SimpleKasKeydirectly, including base64 decoding PEM strings and casting algorithm enums. - Updating Go code (database clients, marshalling helpers, integration tests) to align with these new structures and data formats.
Overall, the changes improve consistency and type safety. The integration tests have been adapted well to these changes. I have a few suggestions, primarily around documentation.
Summary of Findings
- gRPC Documentation: Field descriptions for
SimpleKasPublicKeyin the generated gRPC HTML documentation are missing and should be added for clarity. - Code Comment Clarification: A new
TODOstyle comment inservice/policy/db/attributes.goregarding key formatting needs clarification, as recent SQL changes might have made it obsolete forSimpleKasKey. - Refactoring Consistency: The refactoring of KAS key representation to
SimpleKasKeyand the associated SQL/Go changes appear consistent and well-implemented across the codebase.
Merge Readiness
The pull request introduces significant and positive refactoring for KAS key representation. The changes are largely well-implemented. However, there are a few areas that need attention, primarily the missing field descriptions in the gRPC documentation and a clarifying question about a code comment. Addressing these points would improve the overall quality and maintainability. I am unable to approve pull requests, but I recommend addressing these medium-severity items before merging. Other reviewers should make the final decision.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
🤖 I have created a release *beep* *boop* --- ## [0.4.0](protocol/go/v0.3.6...protocol/go/v0.4.0) (2025-06-05) ### Features * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **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)) --- 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>
🤖 I have created a release *beep* *boop* --- ## [0.6.0](service/v0.5.5...service/v0.6.0) (2025-06-06) ### Features * **authz:** DSPX-894 auth svc registered resource GetEntitlement support ([#2358](#2358)) ([a199aa7](a199aa7)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **core:** DSPX-608 - Deprecate public_client_id ([#2185](#2185)) ([0f58efa](0f58efa)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Unique name for the key provider. ([#2391](#2391)) ([bb58b78](bb58b78)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to 0.4.0 in /service ([#2399](#2399)) ([1c6fa75](1c6fa75)) * **deps:** bump the external group across 1 directory with 21 updates ([#2401](#2401)) ([3d0d4d1](3d0d4d1)) * **policy:** move action sub queries to CTE in sm list and match sql ([#2369](#2369)) ([0fd6feb](0fd6feb)) * **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 support for creation/updation of SubjectMappings with deprecated proto actions ([#2373](#2373)) ([3660200](3660200)) --- 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>
🤖 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]>
Proposed Changes
1.) Return Simple Kas Key from all RPCs that are not Key routes.
Checklist
Testing Instructions