-
Notifications
You must be signed in to change notification settings - Fork 24
feat(policy): Add obligation support to KAS #2786
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
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
/gemini review |
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 introduces support for obligations in KAS. The changes include retrieving fulfillable obligations from a new request header, passing them to the authorization service during access checks, and populating the required obligations from the PDP decision into the KAS response metadata. The implementation looks solid, with new logic to handle obligation context and good test coverage for the new functionality. I've identified a critical issue that could lead to a panic and a minor suggestion for performance improvement.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
/gemini review |
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 introduces support for policy obligations in KAS. It adds logic to parse fulfillable obligations from the X-Rewrap-Additional-Context
request header and includes required obligations in the X-Required-Obligations
metadata of the rewrap response. The changes are well-structured, and the new helper functions getAdditionalRewrapContext
and populateRequiredObligationsOnResponse
are accompanied by thorough unit tests.
However, I've identified a critical bug in the nanoTDFRewrap
function due to a copy-paste error, which could lead to incorrect behavior. Additionally, the core logic changes within the tdf3Rewrap
and nanoTDFRewrap
functions, which integrate this new obligation handling, are not covered by unit tests. It's crucial to add tests for these changes to ensure the feature works as expected and prevent future regressions. I've also left a suggestion to simplify some of the new logic for improved readability.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Proposed Changes
1.) Add logic to retrieve obligations from request header during rewrap
2.) Add logic to populate required obligations on kas response object
Smoke tests tried manually (no key split or bulk rewrap):
Checklist
Testing Instructions