Backport to branch(3) : Fix certificate and secret key version check and messages #213
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @feeblefakie, 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!
I've backported a critical fix that refines how certificate and secret key versions are handled within the client configuration. The primary goal of these changes is to prevent the use of zero as a version number, which was previously allowed but could lead to issues. This involves adjusting the configuration loading priority, enforcing stricter validation rules, and updating error messages to provide clearer guidance. These updates ensure more robust and predictable behavior when configuring client identities.
Highlights
- Configuration Loading Logic: I've refined the logic for loading certificate versions in
ClientConfigto ensure that theDS_CERT_VERSION(digital signature certificate version) is correctly prioritized and that a value of zero for any certificate or secret key version will now explicitly trigger an error. - Strict Version Validation: I've updated the validation rules for both certificate and secret key versions to strictly enforce that their values must be greater than zero. Previously, a value of zero was permitted, which could lead to unexpected behavior.
- Enhanced Error Handling and Testing: I've added comprehensive unit tests to
ClientConfigTestto confirm that providing a zero value for any certificate or secret key version now correctly throws anIllegalArgumentException, covering both current and deprecated configuration properties. - Improved Error Messages: I've updated the associated error messages in
CommonErrorto clearly communicate that certificate and secret key versions must be 'greater than zero', aligning with the new validation rules.
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 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 or fill out our survey 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.
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 backports a fix that tightens the validation for certificate and secret key versions, requiring them to be greater than zero. The logic for loading these versions from configuration properties has also been improved to correctly prioritize new properties over deprecated ones. The changes are well-supported by new and updated tests. My review focuses on improving the clarity and maintainability of the test code by removing some redundant property settings.
| Properties props = new Properties(); | ||
| props.put(ClientConfig.ENTITY_ID, SOME_ENTITY_ID); | ||
| props.put(ClientConfig.CERT_VERSION, "0"); | ||
| props.put(ClientConfig.CERT_HOLDER_ID, SOME_CERT_HOLDER_ID); |
There was a problem hiding this comment.
The property ClientConfig.CERT_HOLDER_ID is redundant here because ClientConfig.ENTITY_ID is set on line 596. The configuration loading logic gives precedence to ENTITY_ID, so CERT_HOLDER_ID will be ignored for determining the entity ID. Removing this line will make the test cleaner.
| props.put(ClientConfig.DS_CERT_VERSION, "0"); | ||
| props.put(ClientConfig.DS_CERT_PEM, SOME_CERT_PEM); | ||
| props.put(ClientConfig.DS_PRIVATE_KEY_PEM, SOME_PRIVATE_KEY_PEM); | ||
| props.put(ClientConfig.CERT_HOLDER_ID, SOME_CERT_HOLDER_ID); |
There was a problem hiding this comment.
This line is redundant. ClientConfig.ENTITY_ID is set on line 779, and it has precedence over the deprecated ClientConfig.CERT_HOLDER_ID. The configuration loading logic will use the value from ENTITY_ID and ignore CERT_HOLDER_ID for determining the entity ID. Removing this line will make the test cleaner and avoid confusion.
This is an automated backport of the following:
Please merge this PR after all checks have passed.