fix(auth): exit early when agent cert config is outside well-known directory#17762
Conversation
…ectory To prevent unnecessary startup latency, this updates the agent certificate retrieval to only poll when the config points to the well-known workload directory (where Cloud Run dynamically mounts files). For all other paths, it exits early. - Extracts config parsing into a dedicated helper function - Removes the hardcoded fallback to the well-known path when no config is provided - Adds test coverage for new logic
![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 agent identity certificate path retrieval logic in _agent_identity_utils.py by extracting configuration parsing and polling into dedicated helper functions, and conditionally disabling polling for paths outside the well-known directory to avoid startup delays. The review feedback recommends specifying encoding="utf-8" when opening the configuration file to ensure consistent cross-platform behavior.
nbayati
changed the title
parthea
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:run
lsirac
changed the title
| # delays. | ||
| well_known_dir = os.path.dirname(_WELL_KNOWN_CERT_PATH) | ||
| has_well_known_dir = os.path.exists(well_known_dir) | ||
| should_poll = cert_config_path.startswith(well_known_dir) |
There was a problem hiding this comment.
Using string prefix matching here is not safe and can create both false positives and false negatives (e.g. when paths contain symlinks, but not sure if that is something we need to handle).
There was a problem hiding this comment.
Good point. I did some digging into the Cloud Run implementation, and it seems like CR doesn't use symlinks for these mounts (unlike Kubernetes), and it supplies a strict logical path to the environment variable. So we can safely fix the prefix vulnerability purely via string manipulation using os.path.abspath and os.path.commonpath without needing to hit the filesystem to resolve symlinks! I've updated the logic.
| cert_config_path = os.environ.get(environment_vars.GOOGLE_API_CERTIFICATE_CONFIG) | ||
|
|
||
| # Check if the well-known workload directory is mounted. | ||
| if not cert_config_path: |
There was a problem hiding this comment.
We now return None when env var is not set. Is this intentional? Will this break anything?
There was a problem hiding this comment.
Yes, this is intentional. The goal of this change is to completely skip any filesystem checks (and polling) unless the feature is explicitly enabled via the GOOGLE_API_CERTIFICATE_CONFIG environment variable. Cloud Run injects this environment variable, so relying on it exclusively is safe. Removing the hardcoded fallback prevents us from introducing unnecessary I/O or startup delays for users who aren't using this feature (see b/522957573)
There was a problem hiding this comment.
Let's test when the first attempt has the file missing, second attempt succeeds, and cert path is returned
| return None | ||
| except (IOError, ValueError, KeyError) as e: | ||
| if cert_config_path and os.path.exists(cert_config_path): | ||
| if os.path.exists(cert_config_path): |
There was a problem hiding this comment.
Can we confirm if this is the correct behavior? Gemini says this breaks polling when Cloud Run rotates secret volumes.
There was a problem hiding this comment.
Not sure what Gemini means by that, but the actual behavior didn't change, I just removed the redundant cert_config_path and truthiness check since we now guarantee it's not None at the top of the function.
So I don't think this would break or make any functional difference for CR.
lsirac
left a comment
lsirac
left a comment
There was a problem hiding this comment.
Just missing tests for the absolute path / startsWith change
4 participants
To prevent unnecessary startup latency, this updates the agent certificate retrieval to only poll when the config points to the well-known workload directory (where Cloud Run dynamically mounts files). For all other paths, it exits early.
fixes b/522957573