feat(grpc): add certificate rotation support for mTLS flow - #1
feat(grpc): add certificate rotation support for mTLS flow#1agrawalradhika-cell wants to merge 1 commit into
Conversation
This change introduces support for X.509 certificate rotation in gRPC connections by utilizing the certificate_configuration_fetcher parameter in grpcio. Fixes: b/494318856\nSee also: go/cert-rotation-in-pythonsdk-for-x509
Summary of ChangesHello, 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 gRPC transport layer to support X.509 certificate rotation for mTLS connections. By integrating gRPC's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. 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 support for X.509 certificate rotation for mTLS in gRPC connections. The implementation adds a new helper function _get_ssl_channel_credentials to handle creating credentials with rotation support if the installed grpcio version allows it, while providing a fallback for older versions. The changes are well-structured and include comprehensive tests for the new functionality. I have one suggestion to improve the exception handling to ensure consistency between the certificate rotation and fallback paths.
| try: | ||
| _, cert, key, _ = _mtls_helper.get_client_ssl_credentials() | ||
| self._ssl_credentials = grpc.ssl_channel_credentials( | ||
| certificate_chain=cert, private_key=key | ||
| ) | ||
|
|
||
| def client_cert_callback(): | ||
| _, cert, key, _ = _mtls_helper.get_client_ssl_credentials() | ||
| return cert, key | ||
|
|
||
| self._ssl_credentials = _get_ssl_channel_credentials(client_cert_callback) | ||
| except exceptions.ClientCertError as caught_exc: | ||
| new_exc = exceptions.MutualTLSChannelError(caught_exc) | ||
| raise new_exc from caught_exc |
There was a problem hiding this comment.
The current try...except block only catches ClientCertError during the initial channel creation, specifically when the fallback mechanism in _get_ssl_channel_credentials is used. If certificate rotation is active, the client_cert_callback is called later by gRPC, and any ClientCertError raised at that point will not be caught and converted to a MutualTLSChannelError.
Moving the exception handling inside the client_cert_callback ensures consistent error handling for both scenarios (with and without certificate rotation).
def client_cert_callback():
try:
_, cert, key, _ = _mtls_helper.get_client_ssl_credentials()
return cert, key
except exceptions.ClientCertError as caught_exc:
new_exc = exceptions.MutualTLSChannelError(caught_exc)
raise new_exc from caught_exc
self._ssl_credentials = _get_ssl_channel_credentials(client_cert_callback)
There was a problem hiding this comment.
Code Review
This pull request adds support for X.509 certificate rotation in gRPC by using the certificate_configuration_fetcher feature of grpcio. The implementation correctly feature-detects and provides a fallback for older versions of grpcio. The changes are well-tested. I found one potential issue in the implementation of the client_cert_callback where it doesn't correctly handle the case where client credentials can't be loaded, which could lead to unhandled exceptions. I've suggested a fix for this.
| _, cert, key, _ = _mtls_helper.get_client_ssl_credentials() | ||
| return cert, key |
There was a problem hiding this comment.
The client_cert_callback is ignoring the first element of the tuple returned by _mtls_helper.get_client_ssl_credentials(), which is a boolean indicating whether credentials were successfully obtained. If get_client_ssl_credentials() returns (False, None, None, None), this callback will return (None, None), which will likely cause an unhandled error deep inside grpcio when it tries to use these None values. The callback should check this boolean and raise a google.auth.exceptions.ClientCertError if credentials are not found. This will ensure that errors are handled gracefully within this library.
| _, cert, key, _ = _mtls_helper.get_client_ssl_credentials() | |
| return cert, key | |
| has_creds, cert, key, _ = _mtls_helper.get_client_ssl_credentials() | |
| if not has_creds: | |
| raise exceptions.ClientCertError( | |
| "Failed to load client credentials for mTLS." | |
| ) | |
| return cert, key |
1 participant
This change introduces support for X.509 certificate rotation in gRPC connections by utilizing the certificate_configuration_fetcher parameter in grpcio.
Fixes: b/494318856\nSee also: go/cert-rotation-in-pythonsdk-for-x509