Skip to content

refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976

Open
nbayati wants to merge 4 commits into
googleapis:mainfrom
nbayati:remove-pyopenssl-dependency
Open

refactor(auth): replace pyOpenSSL with standard ssl and cryptography#16976
nbayati wants to merge 4 commits into
googleapis:mainfrom
nbayati:remove-pyopenssl-dependency

Conversation

@nbayati

@nbayati nbayati commented May 7, 2026

Copy link
Copy Markdown
Contributor

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.

Fixes #16920
Closes #16921
fixes #17412

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +109 to +130
if sys.platform == "linux" and hasattr(os, "memfd_create"):
cm = _memfd_cert_key_paths(cert_bytes, key_bytes)
try:
cert_path, key_path = cm.__enter__()
except OSError:
pass # Fallback to Tier 3 on failure.
else:
try:
# Handle cases where path exists but might be restricted.
if (cert_path is None or os.path.exists(cert_path)) and (
key_path is None or os.path.exists(key_path)
):
yield cert_path or cert, key_path or key, passphrase
return
finally:
import sys

exc_info = sys.exc_info()
cm.__exit__(
*(exc_info if exc_info[0] is not None else (None, None, None))
)
# If verification failed, fall through to Tier 3.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The manual invocation of __enter__ and __exit__ for the _memfd_cert_key_paths context manager is unnecessarily complex and error-prone. You can achieve the same fallback logic much more cleanly using a standard with statement inside a try...except block. This ensures that the context is properly managed and closed even if exceptions occur during the yield or in the caller's block.

    if sys.platform == "linux" and hasattr(os, "memfd_create"):
        try:
            with _memfd_cert_key_paths(cert_bytes, key_bytes) as (cert_path, key_path):
                # Handle cases where path exists but might be restricted.
                if (cert_path is None or os.path.exists(cert_path)) and (
                    key_path is None or os.path.exists(key_path)
                ):
                    yield cert_path or cert, key_path or key, passphrase
                    return
        except OSError:
            pass  # Fallback to Tier 3 on failure.

if cert_bytes is not None:
# MFD_CLOEXEC prevents FD leaks to spawned subprocesses.
fd_cert = os.memfd_create("mtls_cert", os.MFD_CLOEXEC)
os.write(fd_cert, cert_bytes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using os.write directly can lead to partial writes, although unlikely with in-memory files. A more robust and "Pythonic" way to write the entire byte string to the file descriptor without closing it prematurely is to use os.fdopen with closefd=False.

Suggested change
os.write(fd_cert, cert_bytes)
with os.fdopen(fd_cert, "wb", closefd=False) as f:
f.write(cert_bytes)


if key_bytes is not None:
fd_key = os.memfd_create("mtls_key", os.MFD_CLOEXEC)
os.write(fd_key, key_bytes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the certificate write, using os.fdopen with closefd=False is safer and cleaner than a direct os.write call.

Suggested change
os.write(fd_key, key_bytes)
with os.fdopen(fd_key, "wb", closefd=False) as f:
f.write(key_bytes)

@parthea

parthea commented May 26, 2026

Copy link
Copy Markdown
Contributor

@nbayati , Please could you address the failing presubmits?

@nbayati nbayati marked this pull request as ready for review June 10, 2026 05:59
@nbayati nbayati requested review from a team as code owners June 10, 2026 05:59
@nbayati nbayati added kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:run Add this label to force Kokoro to re-run the tests. labels Jun 10, 2026
nbayati and others added 4 commits June 10, 2026 06:07
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.
@nbayati nbayati force-pushed the remove-pyopenssl-dependency branch from f7e5146 to 1704dd8 Compare June 10, 2026 06:11
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants