fix(core): Improves errs with misconfigured keys #2730
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 @dmihalcik-virtru, 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 significantly enhances the robustness and flexibility of key management within the system. It addresses critical error scenarios by preventing panics when encountering misconfigured or non-exportable keys and when decrypting malformed ciphertext. Furthermore, it expands the system's cryptographic capabilities by introducing dynamic detection and support for a wider array of RSA and Elliptic Curve key types for in-process keys, improving compatibility and future-proofing.
Highlights
- Improved Error Handling for Misconfigured Keys: The pull request introduces checks to prevent panics caused by misconfigured keys. Specifically, it now errors out if a basic wrapped KAS key has an empty private key or if a key is labeled non-exportable. Additionally, a check was added to prevent panics when decrypting ciphertext that is too short in AES-GCM operations.
- Extended Key Type Support for In-Process Keys: The system now supports a wider range of cryptographic key types for in-process keys. This includes larger RSA key sizes (e.g., RSA4096) and various Elliptic Curve types (e.g., P384, P521), enhancing flexibility and compatibility with diverse cryptographic requirements.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| 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 pull request 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. 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.
A key, misconfigured, No panic, just error clear, New types now embraced.
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 aims to improve error handling for misconfigured keys and add support for extended key types. The changes include adding a boundary check in AES-GCM decryption to prevent panics, introducing a KeyType method to better identify key characteristics, and adding a check for non-exportable keys. My review found a critical issue where a deprecated function is used, which breaks the functionality for finding EC keys by ID. I've also pointed out a minor grammatical error in a comment. Overall, the changes are beneficial, but the critical issue needs to be addressed.
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:
|
This allows extracting the exact key type, including bit depth for RSA and curve for EC.
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:
|
dmihalcik-virtru
force-pushed
the
DSPX-1630-improve-errors
branch
from
039c60e to
1eb1746
Compare
|
Blocked by #2735 |
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:
|
|
Pulled out ocrypto stuff to #1635 |
Proposed Changes
Checklist
Testing Instructions