feat: MDS connections use mTLS

[nolanleastin]

Feature Gating
The GCE_METADATA_MTLS_MODE environment variable is introduced, which can be set to strict, none, or default.

The should_use_mds_mtls function determines whether to use mTLS based on the environment variable and the existence of the certificate files.

Using MDS mTLS
A custom MdsMtlsAdapter for requests is implemented to handle the SSL context for mTLS.

The create_session function creates a requests.Session with the mTLS adapter.

Integrating with existing code
compute_engine/_metadata.py:

• The metadata server URL construction is now dynamic, supporting both http and https schemes based on whether mTLS is enabled.
• ping and get functions are updated to use mTLS when it's enabled.

[daniel-sanche]

Nothing major, but left a few suggestions to consider

[daniel-sanche]

It seems stange that this IP is hardcoded in multiple places. Can this be GCE_MDS_HOSTS[-1]? Or can we make a DEFAULT_GCE_METADATA_IP constant?

[nolanleastin]

I can add a _DEFAULT_GCE_METADATA_IP constant

[daniel-sanche]

nit: the line break seems unnecessary here

[daniel-sanche]

use_mtls isn't mentioned here

[daniel-sanche]

I'm not familiar with how requests work in this library, but I find it surprising the request passed in is completely replaced with a new one when mtls is enabled. Wouldn't it be cleaner to create a different request at the caller side, instead of accepting one and then overriding it here?

If you do keep this behaviour, do the docstrings need to be updated to make it clear that the input request might not be used?

request (google.auth.transport.Request): A callable used to make HTTP requests.

Or am I thinking about all this wrong?

[nolanleastin]

I actually did initially implement it the way you suggested here (which is: the caller passes in a different request).

The problem @sai-sunder-s pointed out is that people using this library will create their own request object and then pass it in.

[daniel-sanche]

Isn't this a private file? Do users call these functions directly?

[daniel-sanche]

Also, is _prepare_request_for_mds really needed? It seems like this could just be a one-liner:

request = requests.Request(_mtls.create_session()) if use_mtls else request

[daniel-sanche]

These paths are a bit unweildy, especially since the directory is referenced multiple times in this file. I'd suggest doing something like:

WIN_BASE_PATH_COMPONENTS = ("C:\\", "ProgramData", "Google", "ComputeEngine")
BASE_PATH_COMPONENTS = ("/", "run", "google-mds-mtls")

def _get_mds_root_crt_path():
    if os.name == "nt":
        return os.path.join(*WIN_BASE_PATH_COMPONENTS, "mds-mtls-root.crt")
    else:
        return os.path.join(*BASE_PATH_COMPONENTS, "root.crt")

Or, preferably, consider using Path objects instead of strings. It could simplify some of the weirdness of paths on different platforms

from pathlib import Path

WIN_BASE_PATH = PATH("C:/ProgramData/Google/ComputeEngine")
BASE_PATH = PATH("/run/google-mds-mtls")

def _get_mds_root_crt_path():
    if os.name == "nt":
        return WIN_BASE_PATH / "mds-mtls-root.crt"
    else:
        return BASE_PATH / "root.crt"

[daniel-sanche]

nit: The "nt" string is used in a few places, and could be confusing if you're not familiar

Maybe consider making this a constant. Something like WINDOWS_OS_NAME="nt"

[daniel-sanche]

Looking at this a bit closer, it looks like this change would contradict the public API as it currently stands. Custom request objects constructed and passed in by users would be bypassed after the next release, which could break existing code. I think we need to consider that more before merging

Or let me know if I'm misunterstanding