refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976
refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976nbayati wants to merge 11 commits into
Conversation
![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 library to remove the pyOpenSSL dependency, migrating certificate and key management to the cryptography library and Python's standard ssl module. It introduces a secure three-tier fallback strategy for handling mTLS credentials via the new secure_cert_key_paths utility, which utilizes RAM-backed virtual files on Linux to minimize disk exposure. Review feedback suggests simplifying the manual context manager orchestration within this utility and using os.fdopen instead of os.write for more robust and idiomatic file descriptor operations.
|
@nbayati , Please could you address the failing presubmits? |
nbayati
added
kokoro:run
Replace pyOpenSSL with standard library ssl for mTLS transport and update key decryption to use cryptography library. This change also enhances security for handling private keys by: - Using Linux memfd_create for RAM-backed in-memory files to avoid writing secrets to physical storage. - Encrypting plaintext keys on-the-fly before writing to fallback temporary files on disk. - Securely wiping temporary files with null bytes before deletion.
…S cert loading fallbacks
nbayati
force-pushed
the
remove-pyopenssl-dependency
branch
from
f7e5146 to
1704dd8
Compare
yoshi-kokoro
removed
the
kokoro:force-run
… update tests accordingly
…on to include empty password for client_combined_cert_path
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the dependency on pyOpenSSL across the library, replacing it with Python's standard ssl library and the cryptography package. It also introduces a secure three-tier fallback strategy for handling client certificates and private keys. The review feedback highlights a potential NameError in urllib3.py due to an unimported module reference, suggests simplifying context manager exit logic in _mtls_helper.py, recommends verifying write permissions for /dev/shm to prevent failures in restricted environments, and advises defensively encoding leaf certificate callback values to bytes.
| return | ||
|
|
||
| cert_bytes = cert if isinstance(cert, bytes) else None | ||
| key_bytes = key if isinstance(key, bytes) else None |
There was a problem hiding this comment.
what if a user passed one as a string path, and one as bytes? It seems like that isn't supported? Should we mention that in the docstrings?
There was a problem hiding this comment.
It might be good to leave an in-line comment about how Nones are treated here too. After looking at some of the helpers, it seems like it will result in an empty output?
There was a problem hiding this comment.
Mixed inputs (e.g. one string path and one bytes) are currently supported. If one parameter is a string path or None, its bytes representation will be None, so only the bytes parameter is written to a temp file, and the string/None parameter is yielded back as-is. I've updated the docstrings to clarify this behavior, and added inline comments on how None values are handled.
| cleanup_fds.append(fd_key) | ||
| with os.fdopen(fd_key, "wb", closefd=False) as f: | ||
| f.write(key_bytes) | ||
| key_path = f"/proc/self/fd/{fd_key}" |
There was a problem hiding this comment.
nit: I wonder if making a helper or loop for these could be cleaner?
There was a problem hiding this comment.
Done! Added a loop.
| Yields: | ||
| Tuple[Optional[str], Optional[str]]: In-memory file paths pointing to | ||
| the active descriptors (e.g., '/proc/self/fd/3'). | ||
| """ |
There was a problem hiding this comment.
It seems like secure_cert_key_paths expects this one to raise OSErrors? If so, we should probably mention them in a Raises: section in the docstring
(although I had a separate suggestion about those OSErrors too)
| cert: Union[str, bytes], | ||
| key: Union[str, bytes], | ||
| passphrase: Optional[bytes] = None, | ||
| ) -> Generator[Tuple[str, str, Optional[bytes]], None, None]: |
There was a problem hiding this comment.
It might be useful to mention in the docstring that this was implemented as a context manager/generator to clean up after itself between yield and exit.
A lot of the clean-up code is hidden in delegated calls, so it may not be obvious to future readers
| with os.fdopen(fd, "wb") as f: | ||
| f.write(encrypted_key_bytes) | ||
| f.flush() | ||
| os.fsync(f.fileno()) |
There was a problem hiding this comment.
This might be worth replacing with a loop or helper
…actor file writing into loops
4 participants
Replace pyOpenSSL with standard library ssl for mTLS transport and update key decryption to use cryptography library.
This change also enhances security for handling private keys by:
Fixes #16920
Closes #16921
fixes #17412