fix(policy): Ensure non active keys cannot be assigned. #2321
Conversation
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, providing a summary of this pull request to help everyone get up to speed quickly.
This PR addresses a policy enforcement issue where it was possible to assign a non-active Key Access Server (KAS) key to policy entities like attribute values, attributes, and namespaces. The core change introduces a check during the assignment process to ensure that only active keys can be linked. This prevents potential issues that could arise from using keys that are no longer considered valid for new assignments.
The changes involve adding a new helper function in the database layer to verify key status and integrating this check into the relevant assignment functions. Additionally, new integration tests have been added to specifically cover the scenario where an attempt is made to assign an inactive key, ensuring the expected failure behavior.
Highlights
- Policy Enforcement: Implemented a check to prevent the assignment of non-active KAS keys to attribute values, attributes, and namespaces.
- Database Logic: Added a new helper function
verifyKeyIsActivein the policy database layer to check the status of a KAS key. - Integration Tests: Added new integration tests (
Test_AssignPublicKeyToAttributeValue_NotActiveKey_Fail,Test_AssociatePublicKeyToAttribute_NotActiveKey_Fail,Test_AssignPublicKeyToNamespace_NotActiveKey_Fail) to validate that assigning inactive keys correctly results in an error. - Test Error Handling: Updated existing integration tests for assigning non-existent keys to expect
db.ErrNotFoundinstead ofdb.ErrForeignKeyViolation.
Changelog
Click here to see the changelog
- service/integration/attribute_values_test.go
- Added import for
fmtandkasregistry. - Added new test
Test_AssignPublicKeyToAttributeValue_NotActiveKey_Fail(lines 986-1058) to test assigning an inactive key to an attribute value. - Updated
Test_AssignPublicKeyToAttributeValue_Returns_Error_When_Key_Not_Found(lines 1060-1070) to assertdb.ErrNotFoundinstead ofdb.ErrForeignKeyViolationwhen a key is not found.
- Added import for
- service/integration/attributes_test.go
- Updated
Test_AssociatePublicKeyToAttribute_Returns_Error_When_Key_Not_Found(lines 1335-1339) to assertdb.ErrNotFoundinstead ofdb.ErrForeignKeyViolationwhen a key is not found. - Added new test
Test_AssociatePublicKeyToAttribute_NotActiveKey_Fail(lines 1341-1409) to test assigning an inactive key to an attribute definition.
- Updated
- service/integration/namespaces_test.go
- Updated
Test_AssociatePublicKeyToNamespace_Returns_Error_When_Key_Not_Found(lines 1170-1174) to assertdb.ErrNotFoundinstead ofdb.ErrForeignKeyViolationwhen a key is not found. - Added new test
Test_AssignPublicKeyToNamespace_NotActiveKey_Fail(lines 1176-1255) to test assigning an inactive key to a namespace. - Moved the
TestNamespacesSuitefunction definition (lines 1342-1347).
- Updated
- service/internal/fixtures/policy_fixtures.yaml
- Changed the
key_statusofkas_key_2fromKEY_STATUS_INACTIVE(2) toKEY_STATUS_ACTIVE(1) (line 559).
- Changed the
- service/policy/db/attribute_values.go
- Added a call to
c.verifyKeyIsActive(lines 348-350) at the beginning ofAssignPublicKeyToValueto check if the key is active before proceeding.
- Added a call to
- service/policy/db/attributes.go
- Added a call to
c.verifyKeyIsActive(lines 605-607) at the beginning ofAssignPublicKeyToAttributeto check if the key is active before proceeding.
- Added a call to
- service/policy/db/key_access_server_registry.go
- Added a new private helper function
verifyKeyIsActive(lines 920-933) to fetch a key and return an error if its status is notKEY_STATUS_ACTIVE.
- Added a new private helper function
- service/policy/db/namespaces.go
- Added a call to
c.verifyKeyIsActive(lines 368-370) at the beginning ofAssignPublicKeyToNamespaceto check if the key is active before proceeding.
- Added a call to
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's status, active or not,
Determines where it can be put.
Assign with care,
Only keys that are there,
Or an error will surely be got.
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 effectively addresses the issue of preventing non-active keys from being assigned, which is a solid improvement for security and data integrity. The core logic is implemented clearly by introducing the verifyKeyIsActive check in the relevant assignment functions (AssignPublicKeyToValue, AssignPublicKeyToAttribute, AssignPublicKeyToNamespace).
The new integration tests (Test_..._NotActiveKey_Fail) are comprehensive and directly verify the intended behavior. Additionally, the corrections made to existing tests by changing the expected error from db.ErrForeignKeyViolation to db.ErrNotFound in 'key not found' scenarios improve test accuracy.
A few points to consider:
- PR Checklist: The checklist in the PR description (unit tests, documentation) is currently unchecked. Could you please review and update these items? If documentation changes are needed (e.g., for API error responses or behavior changes), they should be included. Similarly, if specific unit tests for the new
verifyKeyIsActivemethod or the modified DB client methods are customary or beneficial despite the thorough integration tests, please consider adding them.
Overall, the changes are well-implemented and significantly enhance the robustness of the key assignment process. Addressing the checklist items would further solidify this PR.
Summary of Findings
- Core Logic Implementation: The primary goal of ensuring non-active keys cannot be assigned has been successfully implemented by adding a verification step (
verifyKeyIsActive) to the respective assignment functions. This is a crucial fix for security and correctness. - Integration Test Coverage: New integration tests have been added for attribute values, attribute definitions, and namespaces, specifically testing the scenario where an attempt is made to assign an inactive key. These tests are well-structured and cover the new functionality effectively.
- Test Accuracy Improvements: Existing tests that check for errors when a key is not found have been updated to expect
db.ErrNotFoundinstead ofdb.ErrForeignKeyViolation. This makes the tests more accurate in reflecting the expected behavior. - PR Checklist Completion: The checklist items in the pull request description (e.g., for unit tests and documentation) are currently unchecked. These should be reviewed and updated as necessary.
Merge Readiness
This pull request introduces an important fix and is generally well-implemented with good new integration tests. The core logic preventing the assignment of non-active keys is sound. The updates to existing tests also improve their accuracy.
Before merging, I recommend that the author review and complete the checklist items in the PR description, particularly concerning unit tests (if deemed necessary beyond the current integration tests) and any required documentation updates. Addressing these points will ensure the PR is fully complete.
As an AI, I am not authorized to approve pull requests. Please ensure this PR is reviewed and approved by other maintainers before merging.
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.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 <35498075+elizabethhealy@users.noreply.github.com>
### Proposed Changes 1.) Ensure non-active keys cannot be assigned. ### 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
🤖 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 <35498075+elizabethhealy@users.noreply.github.com>
Proposed Changes
1.) Ensure non-active keys cannot be assigned.
Checklist
Testing Instructions