fix(auth): align mTLS discovery and enforce fail-fast transport configuration.#17470
fix(auth): align mTLS discovery and enforce fail-fast transport configuration.#17470vverman wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces auto-enablement of mTLS when workload certificates are discovered (including at a well-known SPIFFE path) and changes the library's behavior to fail fast with a MutualTLSChannelError instead of silently falling back to standard TLS when mTLS is expected but certificates cannot be loaded. The review feedback suggests several improvements: aligning the environment variable checks in _agent_identity_utils.py to treat any value other than 'true' as disabled, correcting a misleading exception message in sessions.py when a custom request handler is used, and making the SPIFFE certificate path check more robust by using path.isfile and handling potential OSError exceptions.
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | ||
| if use_client_cert and use_client_cert.lower() == "false": | ||
| return None |
There was a problem hiding this comment.
If GOOGLE_API_USE_CLIENT_CERTIFICATE is set to an invalid value (e.g., 'foo'), check_use_client_cert() will treat it as False (disabled). However, the current check only returns None if it is explicitly 'false'. To ensure perfect alignment with check_use_client_cert(), we should treat any value other than 'true' as an opt-out/disabled state when the environment variable is set.
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | |
| if use_client_cert and use_client_cert.lower() == "false": | |
| return None | |
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | |
| if use_client_cert is not None and use_client_cert.lower() != "true": | |
| return None |
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | ||
| if use_client_cert and use_client_cert.lower() == "false": | ||
| return False |
There was a problem hiding this comment.
Similar to the check in get_and_parse_agent_identity_certificate, if GOOGLE_API_USE_CLIENT_CERTIFICATE is set to an invalid value, it should be treated as disabled to prevent mismatched bound tokens. We should check if the variable is set and is not 'true'.
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | |
| if use_client_cert and use_client_cert.lower() == "false": | |
| return False | |
| use_client_cert = os.environ.get(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | |
| if use_client_cert is not None and use_client_cert.lower() != "true": | |
| return False |
| raise exceptions.MutualTLSChannelError( | ||
| "mTLS was configured/enabled but client certificate or private key could not be loaded." | ||
| ) |
There was a problem hiding this comment.
The exception message here is misleading. If this block is reached, it means a custom request handler (not an AiohttpRequest) was provided, which cannot be configured for mTLS. The current message states that the client certificate or private key could not be loaded, which will confuse users trying to debug their configuration.
| raise exceptions.MutualTLSChannelError( | |
| "mTLS was configured/enabled but client certificate or private key could not be loaded." | |
| ) | |
| raise exceptions.MutualTLSChannelError( | |
| "mTLS was configured/enabled but a custom request handler was provided, which is not supported for mTLS configuration." | |
| ) |
| if ( | ||
| path.exists(_WELL_KNOWN_SPIFFE_CERT_PATH) | ||
| and path.getsize(_WELL_KNOWN_SPIFFE_CERT_PATH) > 0 | ||
| ): | ||
| return True |
There was a problem hiding this comment.
Using path.exists can return True for directories, and calling path.getsize on a directory or a file without read permissions can raise an OSError (such as PermissionError). To make this auto-detection robust and prevent unexpected crashes on startup, we should use path.isfile and wrap the check in a try-except OSError block.
| if ( | |
| path.exists(_WELL_KNOWN_SPIFFE_CERT_PATH) | |
| and path.getsize(_WELL_KNOWN_SPIFFE_CERT_PATH) > 0 | |
| ): | |
| return True | |
| try: | |
| if ( | |
| path.isfile(_WELL_KNOWN_SPIFFE_CERT_PATH) | |
| and path.getsize(_WELL_KNOWN_SPIFFE_CERT_PATH) > 0 | |
| ): | |
| return True | |
| except OSError: | |
| pass |
This PR resolves two mTLS issues:
(1) it synchronizes the transport and token discovery paths to check for workload certs and library support, preventing mismatched bound tokens, and
(2) it enforces fail-fast error behavior in all transports if mTLS configuration fails, avoiding silent fallback to insecure TLS. These align token and transport states securely.