feat(auth): Ensure new auth SDK versions are backwards compatible with previous SDKs to enable copying to G3#17407
feat(auth): Ensure new auth SDK versions are backwards compatible with previous SDKs to enable copying to G3#17407macastelaz wants to merge 8 commits into
Conversation
…nd circular import decoupling - Restore fail-fast validation in _mtls_helper.py - Add robustness guardrails to impersonated_credentials.py for universe_domain - Safeguard RSA Signer & key_id in rsa.py - Break circular import in credentials.py - Update unit tests in test__mtls_helper.py to match new behavior and run robustly in Google environments. TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
![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 introduces dynamic imports for regional access boundary lookups, adds robust environment variable validation for client certificates, and improves fallback handling for universe domains and key IDs. The review feedback highlights opportunities to make the code more robust and efficient, such as performing safe None checks before accessing object properties in rsa.py, avoiding falsy value issues when retrieving key_id, and eliminating redundant attribute lookups in impersonated_credentials.py.
…s module attribute TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
…ey_id TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
…impersonated credentials TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
| raise ValueError( | ||
| "Environment variable `GOOGLE_API_USE_CLIENT_CERTIFICATE` must be" | ||
| " either `true` or `false`" | ||
| ) |
There was a problem hiding this comment.
I don't think we should be reverting this kind of behaviour. It was an explicit decision to return False instead of raise an exception here
There was a problem hiding this comment.
Do you by chance have any additional context you could share on the original decision to return False here? From my view point, it feels like a security risk to not fail fast here. E.g. if a user sets this to ture trying to set the desire to use a client cert for an mtls connection and we silently "downgrade" this to false because of the typo, I'm not sure it's a great result. By raising here, we fail fast and close that "hole".
FWIW, there are internal tests for gapic generated clients that are asserting an error is raised so if we do stick with returning false, we'll likely need to update a lot of older APIs, etc.
FWIW, we currently have a discrepancy in
| elif module_str.startswith(RSA_KEY_MODULE_PREFIX): | ||
| elif private_key is None or private_key.__class__.__module__.startswith( | ||
| RSA_KEY_MODULE_PREFIX | ||
| ): |
There was a problem hiding this comment.
With this change, someone would get an ImportError if they called RSASigner(None) without the rsa library installed. That doesn't seem right
Can we make the class use the default _cryptography_rsa implementation if None is passed? Or otherwise refactor the class to support None?
There was a problem hiding this comment.
Good call - fixed this to use the default implementation in the case of None
| def key_id(self): | ||
| key_id = getattr(self, "_key_id", None) | ||
| if key_id is not None: | ||
| return key_id |
There was a problem hiding this comment.
Is this to support subclasses? (If so, maybe add a comment?)
| universe_domain | ||
| if isinstance(universe_domain, str) | ||
| else credentials.DEFAULT_UNIVERSE_DOMAIN | ||
| ) |
There was a problem hiding this comment.
This seems like it could be a pretty big behaviour change. Are you sure this is safe?
There was a problem hiding this comment.
It should be safe (I know, famous last words): the base credential class already sets this same default (
) and if the universe domain is set to something else, it would still be respected here. There are cases where you have an older custom credential class or what we encountered in mock credentials used in tests where the universe domain isn't set and the class doesn't extend the base, but I think even in these cases, using DEFAULT_UNIVERSE_DOMAIN is expected across Google client libraries.Thoughts?
…to avoid ImportError TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
…key_id TAG=agy CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6
2 participants
Incorporate universal bug fixes, robustness guardrails, and circular import decoupling
TAG=agy
CONV=39c1761f-e06b-4e3a-80b6-b4fbc70df0b6