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 1 commit 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.
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" |
| 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) |
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