-
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
SimpleKasKey
andSimpleKasPublicKey
message definitions inpolicy.objects.proto
to 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
SimpleKasKey
objects instead ofKasKey
objects. - Database Query Adjustments: Updated SQL queries (
service/policy/db/query.sql
) to fetch the necessary data and format it into theSimpleKasKey
structure, including joining with thekey_access_servers
table 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
SimpleKasKey
return type. This involved fetching the fullKasKey
separately 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 newSimpleKasKey
structure.
Changelog
Click here to see the changelog
- docs/grpc/index.html
- Added documentation links for
policy.SimpleKasKey
andpolicy.SimpleKasPublicKey
. - Removed documentation links for
policy.kasregistry.SimpleKasKey
andpolicy.kasregistry.SimpleKasPublicKey
. - Updated references from
policy.KasKey
topolicy.SimpleKasKey
in 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.KasKey
separately usingGetKey
before validating theSimpleKasKey
returned by the policy RPCs. - Updated assertions to check fields on the returned
SimpleKasKey
objects. - Added
validateSimpleKasKey
helper function to validateSimpleKasKey
structure.
- Imported
- service/integration/attribute_values_test.go
- Modified
Test_AssignPublicKeyToAttributeValue_Succeeds
to fetch the fullpolicy.KasKey
separately usingGetKey
before validating theSimpleKasKey
returned. - Updated assertions to check fields on the returned
SimpleKasKey
objects. - Removed redundant KAS server lookup.
- Modified
- service/integration/attributes_test.go
- Modified
Test_AssociatePublicKeyToAttribute_Succeeds
to fetch the fullpolicy.KasKey
separately usingGetKey
before validating theSimpleKasKey
returned. - Updated assertions to check fields on the returned
SimpleKasKey
objects. - Removed redundant KAS server lookup.
- Modified
- service/integration/kas_registry_key_test.go
- Updated
keyCtx
constant value. - Modified
Test_RotateKey_Multiple_Attributes_Values_Namespace
andTest_RotateKey_Two_Attribute_Two_Namespace_0_AttributeValue_Success
assertions to checkSimpleKasKey
fields on the rotated keys. - Updated
Test_SetBaseKey_Insert_Success
assertions to checkSimpleKasKey
fields on the new base key. - Updated
validatePublicKeyCtx
helper to accept*policy.SimpleKasKey
. - Updated
validatePrivatePublicCtx
helper to construct aSimpleKasKey
for validation.
- Updated
- service/integration/kas_registry_test.go
- Modified
getKasToKeysFixtureMap
to fetch and return[]*policy.KasKey
. - Updated
validateKasRegistryKeys
to use the newvalidateSimpleKasKey
helper for validation.
- Modified
- service/integration/namespaces_test.go
- Modified
Test_AssociatePublicKeyToNamespace_Succeeds
to fetch the fullpolicy.KasKey
separately usingGetKey
before validating theSimpleKasKey
returned. - Updated assertions to check fields on the returned
SimpleKasKey
objects.
- Modified
- service/pkg/db/marshalHelpers.go
- Removed unused imports (
encoding/base64
,strconv
). - Renamed
formatAlg
toFormatAlg
and made it exported. - Removed old
UnmarshalSimpleKasKey
function. - Added
SimpleKasKeysProtoJSON
function to unmarshal a JSON array ofpolicy.SimpleKasKey
. - Added
UnmarshalSimpleKasKey
function to unmarshal a singlepolicy.SimpleKasKey
.
- Removed unused imports (
- service/policy/db/attribute_values.go
- Changed the type of the
keys
variable to[]*policy.SimpleKasKey
and useddb.SimpleKasKeysProtoJSON
for unmarshalling.
- Changed the type of the
- service/policy/db/attributes.go
- Added a comment about formatting keys in
attributesValuesProtojson
. - Changed the type of the
keys
variable to[]*policy.SimpleKasKey
and useddb.SimpleKasKeysProtoJSON
for unmarshalling inGetAttribute
andListAttributesByFqns
.
- Added a comment about formatting keys in
- service/policy/db/grant_mappings.go
- Removed unused import (
encoding/base64
). - Added
errKasInfoIncomplete
error. - Updated
mapAlgorithmToKasPublicKeyAlg
to remove theALGORITHM_UNSPECIFIED
case. - Changed the input type of
mapKasKeysToGrants
to[]*policy.SimpleKasKey
. - Updated
mapKasKeysToGrants
logic to work withSimpleKasKey
fields 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.SimpleKasKey
andpolicy.SimpleKasPublicKey
directly. - 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
keys
variable to[]*policy.SimpleKasKey
and useddb.SimpleKasKeysProtoJSON
for unmarshalling inListKeyAccessServers
andGetKeyAccessServer
. - Changed the return type of
GetBaseKey
to*policy.SimpleKasKey
. - Updated
SetBaseKeyOnWellKnownConfig
to usedb.FormatAlg
and explicitly set the algorithm string.
- Changed the type of the
- service/policy/db/models.go
- Added a comment to the
SelectorValues
field.
- 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 theSimpleKasKey
structure. - Added
INNER JOIN key_access_servers
to relevant queries to fetch KAS URI and ID. - Updated JSON construction to include
kas_uri
,kas_id
, and apublic_key
object withalgorithm
(as integer),kid
, and decodedpem
.
- Modified queries (
- service/policy/kasregistry/key_access_server_registry.proto
- Removed
SimpleKasPublicKey
andSimpleKasKey
message definitions.
- Removed
- service/policy/objects.proto
- Added
SimpleKasPublicKey
andSimpleKasKey
message definitions. - Updated the
kas_keys
field type inNamespace
,Attribute
,Value
, andKeyAccessServer
messages 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
SimpleKasKey
andSimpleKasPublicKey
definitions frompolicy.kasregistry
topolicy/objects.proto
. - Updating
SimpleKasKey
to includekas_id
andSimpleKasPublicKey
to use thepolicy.Algorithm
enum and expect raw PEM strings. - Modifying SQL queries to construct
SimpleKasKey
directly, 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
SimpleKasPublicKey
in the generated gRPC HTML documentation are missing and should be added for clarity. - Code Comment Clarification: A new
TODO
style comment inservice/policy/db/attributes.go
regarding key formatting needs clarification, as recent SQL changes might have made it obsolete forSimpleKasKey
. - Refactoring Consistency: The refactoring of KAS key representation to
SimpleKasKey
and 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