feat(policy): map and merge grants and keys #2324
Conversation
There was a problem hiding this comment.
Hello @strantalis, 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, providing a summary of this pull request by @strantalis.
This PR introduces a new feature to the policy service's database layer: mapping and merging KAS public key information (KasKey messages) into the Grants field (KeyAccessServer messages) when retrieving policy attributes, values, and namespaces. The goal is to consolidate KAS public key data into the standard Grants structure, making it more readily available and consistent across policy objects fetched from the database.
The core logic resides in a new helper function that takes a list of KasKey and existing KeyAccessServer grants, merges them based on KAS URI, adds public keys (deduplicating by KID), and decodes the base64-encoded PEM keys.
Highlights
- New Mapping Logic: Introduces a new function
mapKasKeysToGrantsto mapKasKeymessages toKeyAccessServer(Grants) messages. - Grant Merging: The
mapKasKeysToGrantsfunction merges public key information fromKasKeyobjects into existingKeyAccessServergrants based on the KAS URI. It also handles adding new KAS entries if the URI is not already present in the grants list. - Public Key Deduplication: Ensures that duplicate public keys (identified by Key ID - KID) are not added to the same KAS entry during the merging process.
- Base64 Decoding: Decodes the base64-encoded PEM content found within the
PublicKeyCtxofKasKeyobjects before adding it to theKeyAccessServerpublic key structure. - Application in DB Retrieval: Applies the new mapping and merging logic when fetching policy attributes by FQN (
GetAttributesByValueFqnsandListAttributesByFqns) to populate theGrantsfield on the returned attribute, value, and namespace objects. - Comprehensive Unit Tests: Adds extensive unit tests for the
mapKasKeysToGrantsfunction, covering various scenarios including empty inputs, new keys, existing grants, merging, deduplication, and handling of nil/empty fields.
Changelog
Click here to see the changelog
- service/policy/db/attribute_fqn.go
- Added a loop after line 113 to iterate through the fetched attribute/value pairs and call
mapKasKeysToGrantsfor the attribute, value, and namespace associated with each pair, updating theirGrantsfields.
- Added a loop after line 113 to iterate through the fetched attribute/value pairs and call
- service/policy/db/attributes.go
- Modified the loop constructing
policy.Attributeobjects to callmapKasKeysToGrantsfor the attribute's keys and existing grants (line 321), the value's keys and grants (line 332), and the namespace's keys and grants (line 339). - Updated the assignment of the
Grantsfield for thepolicy.Attributeobject to use the result ofmapKasKeysToGrantsfor the attribute (line 351). - Appended the results of
mapKasKeysToGrantsfor values (line 336) and namespaces (line 343) to their respectiveGrantsfields.
- Modified the loop constructing
- service/policy/db/grant_mappings.go
- New file containing helper functions for mapping KAS keys to grants.
- Introduced
mapAlgorithmToKasPublicKeyAlg(line 10) to map policy algorithms to KAS public key algorithm enums. - Introduced
mapKasKeysToGrants(line 29) which implements the core logic for mergingKasKeydata intoKeyAccessServergrants, handling deduplication and base64 decoding of PEM keys.
- service/policy/db/grant_mappings_test.go
- New file containing unit tests for the
grant_mappings.gofile. - Includes
TestMapKasKeysToGrants(line 13) with numerous test cases covering various input combinations and edge cases. - Includes helper functions
findGrantByURI(line 301),findKasPublicKeyByKID(line 310),sortGrants(line 319), andsortKasPublicKeys(line 326) to facilitate test assertions.
- New file containing unit tests for 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.
Keys and grants align,
Merged data, clear design,
Policy takes shape.
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 introduces functionality to map and merge KAS keys into the grants structures for attributes, values, and namespaces. The core logic for mapping keys to grants in grant_mappings.go is well-tested and handles various scenarios robustly. However, there's a critical issue in service/policy/db/attributes.go concerning how these mapped grants are integrated, and a minor clarity issue in a comment
8000
within grant_mappings.go.
Summary of Findings
- Incorrect Grant List Modification: In
service/policy/db/attributes.go, the mapped grants for attribute values and namespaces are appended to the existing grants list (e.g.,val.Grants = append(val.Grants, valGrants...)). SincemapKasKeysToGrantsalready handles merging with existing grants, this append operation will likely cause duplicate grant entries. This should be changed to an assignment (e.g.,val.Grants = valGrants). This is a critical issue affecting data correctness. - Misleading Comment: In
service/policy/db/grant_mappings.go, a comment regarding PEM encoding (// Grant pem isn't expected to be b64 encoded.) is inconsistent with the code that follows, which decodes a base64 string. This can be confusing for future maintainers. - PR Housekeeping (Not commented due to severity settings): The PR description is minimal, and the checklist items (unit tests, integration tests, documentation) are unchecked. It would be beneficial to update these to reflect the work done, especially since new unit tests have been added.
Merge Readiness
This pull request makes good progress in integrating KAS keys with the grants system. The new mapKasKeysToGrants function and its tests are well-implemented. However, there's a critical issue in service/policy/db/attributes.go where grants are appended instead of assigned, which could lead to data duplication and incorrect policy information. This critical issue, along with the medium-severity comment clarification, should be addressed before this PR is considered for merging. I am not authorized to approve pull requests, so please ensure these changes are reviewed and approved by other maintainers after the suggested modifications are made.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Chris Reed <87077975+c-r33d@users.noreply.github.com>
Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com>
Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com>
Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com>
e475dd1 to
457bc9a
Compare
Invalidated by push of 457bc9a
Proposed Changes
Checklist
Testing Instructions