feat(kas): add configurable SRT skew tolerance and diagnostics #2886
Conversation
Summary of ChangesHello @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! This pull request enhances the KAS service by introducing a configurable clock skew tolerance for Signed Request Tokens (SRTs). This allows for more flexible handling of time differences between clients and the KAS during token validation. Additionally, it significantly improves the diagnostic capabilities for SRT validation failures by providing detailed logging, which will assist in troubleshooting and maintaining system reliability related to time-sensitive operations. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Time's gentle drift, a minute's grace, For tokens signed, in digital space. Skewed clocks may err, but KAS will see, With watchful logs, for you and me. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable skew tolerance for Signed Request Token (SRT) validation and enhances the diagnostic logging for validation failures. The changes are well-structured, with the SRT validation logic refactored into smaller, more testable functions. The new configuration option is also properly documented.
My review focuses on the correctness of the new validation and logging logic. I've found a minor issue in the diagnostic logging that could lead to misleading information when debugging clock skew problems. Overall, this is a great improvement for the service's robustness and maintainability.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors Signed Request Token (SRT) validation to make clock skew tolerance configurable and adds detailed logging for validation failures. The change replaces a hardcoded 30-second acceptable skew with a configurable value (default: 1 minute) and restructures the SRT parsing/verification flow to be more modular while providing better diagnostics when tokens fail validation.
Key changes:
- Introduces configurable
srt_acceptable_skewparameter (defaults to 1 minute, previously hardcoded at 30 seconds) - Refactors SRT validation into separate parsing, validation, and signature verification steps with enhanced logging
- Adds comprehensive test coverage for new configuration and logging behavior
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| service/kas/kas.go | Updates calls to use new ApplyConfig method instead of direct field assignment |
| service/kas/access/rewrap_test.go | Adds test utilities and test cases for the new configuration method and validation logging |
| service/kas/access/rewrap.go | Refactors SRT verification into modular methods with detailed failure logging |
| service/kas/access/provider.go | Adds ApplyConfig method, acceptableSkew helper, and SRTAcceptableSkew configuration field |
| opentdf-kas-mode.yaml | Documents the new optional configuration parameter with example |
| docs/Configuring.md | Documents the srt_acceptable_skew configuration option in the main documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable skew tolerance for Signed Request Tokens (SRTs) and enhances diagnostic logging, which are excellent improvements for security and debuggability. The refactoring of the SRT validation logic in rewrap.go into smaller, more focused methods significantly improves code clarity and maintainability. The new tests are thorough and correctly validate the new functionality.
I have a couple of suggestions to improve the clarity of log messages in service/kas/access/rewrap.go for cases where token parsing or validation fails when signature verification is not required. Overall, this is a high-quality contribution.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
strantalis
force-pushed
the
dspx-1804/kas-srt-skew
branch
from
8ca905f to
bdfd5e5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable clock skew tolerance for Signed Request Tokens (SRT), which is a valuable feature for deployments where clock drift is a concern. The addition of detailed diagnostic logs for token validation failures is particularly well-executed and will be a great help for troubleshooting. The overall refactoring of the token validation logic in rewrap.go improves code clarity and maintainability. One thing to note is that the default acceptable skew has been increased from 30 seconds to 1 minute; this seems reasonable but is a change in behavior to be aware of. I've left a couple of suggestions for improvement, one related to making the new configuration more robust and another to improve logging diagnostics.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
strantalis
force-pushed
the
dspx-1804/kas-srt-skew
branch
from
3d36b62 to
5b980f7
Compare
strantalis
requested review from
dmihalcik-virtru,
elizabethhealy and
jentfoo
strantalis
force-pushed
the
dspx-1804/kas-srt-skew
branch
from
5b980f7 to
95ac79e
Compare
|
/backport |
### Proposed Changes * ### 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 (cherry picked from commit 1a57227)
|
Successfully created backport PR for |
### Proposed Changes * ### 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 (cherry picked from commit 1a57227)
|
/backport |
### Proposed Changes * ### 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 (cherry picked from commit 1a57227)
|
Successfully created backport PR for |
### Proposed Changes * ### 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 (cherry picked from commit 1a57227)
7 participants
Proposed Changes
Checklist
Testing Instructions