Skip to content

Conversation

c-r33d
Copy link
Contributor

@c-r33d c-r33d commented Jun 4, 2025

Proposed Changes

1.) Update simple kas key to have enum as algorithm

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

@c-r33d c-r33d requested review from a team as code owners June 4, 2025 17:27
@github-actions github-actions bot added comp:db DB component external-contributor External Org Member comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) docs Documentation size/s labels Jun 4, 2025
@c-r33d c-r33d changed the title Feat/dspx 1202 update simple kas key feat(policy): Update simple kas key Jun 4, 2025
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.

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! Gemini here, providing a summary of this pull request. This PR, titled "Feat/dspx 1202 update simple kas key", focuses on refining the structure used for representing KAS public keys, specifically the SimpleKasPublicKey message. The primary goal is to change the algorithm field from a generic string type to a more specific policy.Algorithm enum type. This change propagates through the codebase, requiring updates to the protobuf definition, database interactions (queries and marshalling/unmarshalling logic), and associated documentation and tests.

Highlights

  • Protobuf Definition Update: The SimpleKasPublicKey message in service/policy/kasregistry/key_access_server_registry.proto has its algorithm field type changed from string to the policy.Algorithm enum.
  • Database Query Changes: The SQL query (service/policy/db/query.sql) used to retrieve base keys is updated to cast the stored algorithm to an INTEGER (matching the enum's underlying type) and to decode the base64-encoded PEM content directly within the query using DECODE and CONVERT_FROM.
  • Go Marshalling/Unmarshal Helpers Update: The service/pkg/db/marshalHelpers.go file is modified. Manual parsing of the algorithm string to an integer and base64 decoding of the PEM in UnmarshalSimpleKasKey are removed, as the database query now handles the decoding and the proto unmarshalling handles the enum. The formatAlg function is renamed to FormatAlg and exported.
  • Well-Known Config Update Logic: The SetBaseKeyOnWellKnownConfig function in service/policy/db/key_access_server_registry.go is updated to use the newly exported db.FormatAlg function to format the algorithm enum into a string representation before updating the well-known configuration map.
  • Integration Test Update: An integration test (service/integration/kas_registry_key_test.go) is updated. The keyCtx constant is changed, and new assertions are added to verify that the algorithm and the decoded PEM content are correctly retrieved and set on the new base key.
  • Documentation Update: The generated gRPC documentation (docs/grpc/index.html) is updated to reflect that the algorithm field of SimpleKasPublicKey is now of type policy.Algorithm instead of string.

Changelog

Click here to see the changelog
  • docs/grpc/index.html
    • Updated documentation for SimpleKasPublicKey.algorithm to reference policy.Algorithm.
  • service/integration/kas_registry_key_test.go
    • Updated keyCtx constant.
    • Added assertions in Test_SetBaseKey_Insert_Success to check the algorithm and decoded PEM of the new base key.
  • service/pkg/db/marshalHelpers.go
    • Removed unused imports (encoding/base64, strconv).
    • Renamed formatAlg to FormatAlg and exported it.
    • Removed manual algorithm parsing and PEM decoding logic from UnmarshalSimpleKasKey.
  • service/policy/db/key_access_server_registry.go
    • Added logic to format the algorithm enum using db.FormatAlg and update the well-known configuration map.
  • service/policy/db/models.go
    • Added a comment to the SelectorValues field in SubjectConditionSet.
  • service/policy/db/query.sql
    • Changed cast for kask.key_algorithm to INTEGER.
    • Added decoding of base64-encoded PEM using DECODE and CONVERT_FROM.
  • service/policy/kasregistry/key_access_server_registry.proto
    • Changed the type of the algorithm field in SimpleKasPublicKey from string to Algorithm.
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.


From string to enum,
A type change, small but grand,
Code adapts and flows.

Footnotes

  1. 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.

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 effectively updates the SimpleKasPublicKey to use an enum for the algorithm field, which is a good improvement for type safety and clarity. The changes are consistently applied across the proto definitions, Go code, SQL queries, tests, and documentation.

The simplification in UnmarshalSimpleKasKey by moving the PEM decoding to the SQL query is a notable improvement. The test updates in kas_registry_key_test.go correctly verify the new behavior for both algorithm and PEM handling.

A minor, unrelated comment change was noted in service/policy/db/models.go. While not an issue, it's generally good practice to keep PRs focused on a single concern.

Please ensure the PR checklist item "I have added or updated unit tests" is checked, as tests were indeed updated.

Summary of Findings

  • Core Change: Algorithm as Enum: The primary goal of changing SimpleKasPublicKey.algorithm from a string to the policy.Algorithm enum has been successfully implemented. This enhances type safety and code clarity.
  • PEM Handling: The responsibility for base64 decoding the PEM string has been shifted from Go's UnmarshalSimpleKasKey function to the SQL query getBaseKey. This simplifies the Go unmarshalling logic and seems appropriate, assuming the PEM in public_key_ctx is always base64 encoded.
  • SQL Query Update for PEM: The getBaseKey SQL query now decodes the PEM string. A question was raised to confirm the assumption that the source pem field in public_key_ctx is always valid base64 to ensure query robustness.
  • Test Coverage: Unit tests in kas_registry_key_test.go have been updated to reflect the changes in algorithm type and PEM handling, which is good.
  • Documentation: The gRPC documentation in index.html has been correctly updated to reflect the new enum type for the algorithm.
  • Code Simplification: The UnmarshalSimpleKasKey function in service/pkg/db/marshalHelpers.go is now simpler due to the changes in how algorithm and PEM are handled upstream (in SQL and protojson unmarshalling).
  • Minor Unrelated Change: A comment was updated in service/policy/db/models.go which is unrelated to the main purpose of this PR. This is a minor point and doesn't affect functionality.

Merge Readiness

The pull request is in good shape and implements the intended feature effectively. The core changes are well-executed. There is one medium-severity point of clarification regarding the assumptions in the SQL query for PEM decoding. Once that is confirmed, and the PR checklist for unit tests is updated, this PR should be ready for merge. As an AI, I am not authorized to approve pull requests; please ensure other reviewers approve this code before merging.

'algorithm', kask.key_algorithm::INTEGER,
'kid', kask.key_id,
'pem', kask.public_key_ctx ->> 'pem'
'pem', CONVERT_FROM(DECODE(kask.public_key_ctx ->> 'pem', 'base64'), 'UTF8')
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The change to decode the PEM from base64 directly in the SQL query using CONVERT_FROM(DECODE(kask.public_key_ctx ->> 'pem', 'base64'), 'UTF8') is a good simplification for the Go unmarshalling logic.

Could you confirm if kask.public_key_ctx ->> 'pem' is always guaranteed to contain a valid base64 encoded string as per data insertion/update logic? If it could ever be non-base64 or malformed, the DECODE function might cause the query to error out.

Assuming the data integrity ensures it's always valid base64 (as suggested by the PublicKeyCtx proto definition which states the pem field is base64 encoded), this approach is robust.

Copy link
Contributor Author

@c-r33d c-r33d Jun 4, 2025

Choose a reason for hiding this comment

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

No we have a check in the key creation RPC that verifies the public key is base64.

Copy link
Contributor

github-actions bot commented Jun 4, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 489.78643ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 304.74348ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 374.09948ms
Throughput 267.31 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 57.983663764s
Average Latency 577.291649ms
Throughput 86.23 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4935
Failed Requests 65
Concurrent Requests 50
Total Time 45.511160447s
Average Latency 450.942773ms
Throughput 108.43 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
65 occurrences

Standard Benchmark Metrics Skipped or Failed

Copy link
Contributor

github-actions bot commented Jun 4, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 464.00925ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 280.555093ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 341.139101ms
Throughput 293.14 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 51.636337561s
Average Latency 512.566512ms
Throughput 96.83 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4937
Failed Requests 63
Concurrent Requests 50
Total Time 44.452774401s
Average Latency 439.620254ms
Throughput 111.06 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
63 occurrences

Standard Benchmark Metrics Skipped or Failed

Copy link
Contributor

github-actions bot commented Jun 5, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 471.489828ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 293.779969ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 356.706549ms
Throughput 280.34 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 57.24442082s
Average Latency 568.661008ms
Throughput 87.34 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4948
Failed Requests 52
Concurrent Requests 50
Total Time 47.029587074s
Average Latency 464.965181ms
Throughput 105.21 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
52 occurrences

Standard Benchmark Metrics Skipped or Failed

Copy link
Contributor

github-actions bot commented Jun 5, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 458.907993ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 293.525941ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 351.37478ms
Throughput 284.60 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 52.096644323s
Average Latency 517.665507ms
Throughput 95.98 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4947
Failed Requests 53
Concurrent Requests 50
Total Time 42.373451642s
Average Latency 417.755567ms
Throughput 116.75 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
53 occurrences

Standard Benchmark Metrics Skipped or Failed

@strantalis strantalis merged commit 09d8239 into main Jun 5, 2025
25 of 28 checks passed
@strantalis strantalis deleted the feat/DSPX-1202-update-simple-kas-key branch June 5, 2025 12:58
github-merge-queue bot pushed a commit that referenced this pull request Jun 5, 2025
🤖 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>
github-merge-queue bot pushed a commit that referenced this pull request Jun 6, 2025
🤖 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>
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
Labels
comp:db DB component comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) docs Documentation external-contributor External Org Member size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants