fix(auth): Use HTTPS/mTLS with MDS only if adapter is mounted#16693
fix(auth): Use HTTPS/mTLS with MDS only if adapter is mounted#16693nolanleastin wants to merge 2 commits intomainfrom
Conversation
Modify the request mounting so that we return whether an adapter is mounted or not. From that, decide whether we should use https or http.
![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 refactors the GCE metadata server mTLS implementation to improve robustness and validation. Key changes include updating _validate_gce_mds_configured_environment to verify the HTTPS scheme in STRICT mode, refactoring request preparation into _try_mount_mds_mtls_adapter to track adapter mounting status, and updating URL generation logic to dynamically select the scheme based on the mTLS mode and mounting status. Review feedback suggests refining the scheme selection in _get_metadata_root and _get_metadata_ip_root to ensure HTTPS is only applied to default hosts when the adapter is mounted, preventing potential connection failures for custom metadata hosts in non-strict modes.
| scheme = "http" | ||
| if mds_mtls_adapter_mounted or mds_mtls_mode == _mtls.MdsMtlsMode.STRICT: | ||
| scheme = "https" |
There was a problem hiding this comment.
In DEFAULT mTLS mode, the scheme should only be set to https if the metadata host is one of the default hosts for which the mTLS adapter was mounted. If the host has been overridden to a custom value (via the GCE_METADATA_HOST environment variable), the adapter is not mounted for it, and using https will likely result in a connection failure. Checking if the host is in _GCE_DEFAULT_MDS_HOSTS ensures that https is only used when the mTLS adapter is actually applicable.
| scheme = "http" | |
| if mds_mtls_adapter_mounted or mds_mtls_mode == _mtls.MdsMtlsMode.STRICT: | |
| scheme = "https" | |
| scheme = "http" | |
| if mds_mtls_mode == _mtls.MdsMtlsMode.STRICT or ( | |
| mds_mtls_adapter_mounted and _GCE_METADATA_HOST in _GCE_DEFAULT_MDS_HOSTS | |
| ): | |
| scheme = "https" |
There was a problem hiding this comment.
@nolanleastin what do you think of this? It does seem a bit confusing to me that the mtls state is being set in multiple different places, with different validation checks
There was a problem hiding this comment.
I actually do not understand what the gemini code assist is saying here. It is ... just explaining what the code is doing.
| scheme = "http" | ||
| if mds_mtls_adapter_mounted or mds_mtls_mode == _mtls.MdsMtlsMode.STRICT: | ||
| scheme = "https" | ||
| return "{}://{}".format( | ||
| scheme, os.getenv(environment_vars.GCE_METADATA_IP, _GCE_DEFAULT_MDS_IP) | ||
| ) |
There was a problem hiding this comment.
Similar to _get_metadata_root, we should only use https in DEFAULT mode if the metadata IP is one of the default hosts. If GCE_METADATA_IP is overridden to a custom value, the code should fall back to http (unless in STRICT mode) because the mTLS adapter is not mounted for custom IPs. This suggestion also consolidates the os.getenv call for efficiency.
| scheme = "http" | |
| if mds_mtls_adapter_mounted or mds_mtls_mode == _mtls.MdsMtlsMode.STRICT: | |
| scheme = "https" | |
| return "{}://{}".format( | |
| scheme, os.getenv(environment_vars.GCE_METADATA_IP, _GCE_DEFAULT_MDS_IP) | |
| ) | |
| mds_ip = os.getenv(environment_vars.GCE_METADATA_IP, _GCE_DEFAULT_MDS_IP) | |
| scheme = "http" | |
| if mds_mtls_mode == _mtls.MdsMtlsMode.STRICT or ( | |
| mds_mtls_adapter_mounted and mds_ip in _GCE_DEFAULT_MDS_HOSTS | |
| ): | |
| scheme = "https" | |
| return "{}://{}".format(scheme, mds_ip) |
| @@ -200,8 +239,12 @@ def ping( | |||
| Returns: | |||
| bool: True if the metadata server is reachable, False otherwise. | |||
| """ | |||
There was a problem hiding this comment.
should this method declare that it can raise MutualTLSChannelError?
There was a problem hiding this comment.
Yes. I added that!
| scheme = "http" | ||
| if mds_mtls_adapter_mounted or mds_mtls_mode == _mtls.MdsMtlsMode.STRICT: | ||
| scheme = "https" |
There was a problem hiding this comment.
@nolanleastin what do you think of this? It does seem a bit confusing to me that the mtls state is being set in multiple different places, with different validation checks
| mds_mtls_config = _mtls.MdsMtlsConfig() | ||
| should_mount_adapter = _mtls.should_use_mds_mtls( | ||
| mode, mds_mtls_config=mds_mtls_config | ||
| ) |
There was a problem hiding this comment.
nit: it looks like this line doesn't need to be executed if the request doesn't have a session. I'm not sure how expensive it is to check, but worth considering
There was a problem hiding this comment.
Yes that's true. I modified this method and moved some of the checks earlier
| # Mount the adapter for all default GCE metadata hosts. | ||
| for host in _GCE_DEFAULT_MDS_HOSTS: | ||
| request.session.mount(f"https://{host}/", adapter) | ||
| mds_mtls_adapter_mounted = True |
There was a problem hiding this comment.
Do we also need to mount to _GCE_METADATA_HOST?
There was a problem hiding this comment.
No. It's kind of confusing but _GCE_METADATA_HOST is the "effective" host we will be connecting to.
The variable resolves as follows:
- It checks environment variable
GCE_METADATA_HOST(users can set this) - It checks environment variable
GCE_METADATA_ROOT(users can set this) - Otherwise, defaults to
_GCE_DEFAULT_HOST(this is 'metadata.google.internal')
Since mTLS should only be used with default MDS (case 3), we only mount the adapter to the metadata.googe.internal value.
2 participants
Modify the request mounting so that we return whether an adapter is mounted or not. From that, decide whether we should use https or http.
Fixes #16090