Skip to content

Refactor: Remove rsa and make cryptography a core dependency #1771

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sai-sunder-s
Copy link
Contributor

This commit removes the rsa library as a dependency and makes the cryptography library a required, core dependency.

Previously, cryptography was an optional dependency, and the library would fall back to a pure Python RSA implementation using the rsa library if cryptography was not installed.

Changes made:

  • Modified setup.py to remove rsa from dependencies and add cryptography with version constraints.
  • Updated google/auth/crypt/rsa.py to directly use the cryptography-based RSA implementation (_cryptography_rsa.py) and remove the fallback mechanism.
  • Removed the pure Python RSA implementation file (google/auth/crypt/_python_rsa.py).
  • Removed the corresponding tests for the pure Python RSA implementation (tests/crypt/test__python_rsa.py).

Core unit tests pass after these changes.

This commit removes the `rsa` library as a dependency and makes the `cryptography` library a required, core dependency.

Previously, `cryptography` was an optional dependency, and the library would fall back to a pure Python RSA implementation using the `rsa` library if `cryptography` was not installed.

Changes made:
- Modified `setup.py` to remove `rsa` from dependencies and add `cryptography` with version constraints.
- Updated `google/auth/crypt/rsa.py` to directly use the `cryptography`-based RSA implementation (`_cryptography_rsa.py`) and remove the fallback mechanism.
- Removed the pure Python RSA implementation file (`google/auth/crypt/_python_rsa.py`).
- Removed the corresponding tests for the pure Python RSA implementation (`tests/crypt/test__python_rsa.py`).

Core unit tests pass after these changes.
@sai-sunder-s sai-sunder-s added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 23, 2025
@sai-sunder-s sai-sunder-s added the owlbot:run Add this label to trigger the Owlbot post processor. label May 28, 2025
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 28, 2025
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

LGTM. We should mark this as fix instead of refactor so that the change to setup.py is visible in release notes.

fix: add dependency on cryptography
fix: drop dependency on rsa


def autodoc_skip_member_handler(app, what, name, obj, skip, options):
"""
Skips members from internal modules (like _cryptography_rsa or base)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could you file a bug for issue that requires this docs workaround, even if it will be closed with the changes in this PR. Add a link to the issue in this comment.

if public_obj is obj:
return True # Skip this internal one
except ImportError:
pass # Should not happen if the library is installed
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is not expected to happen, can we just let the error bubble up?

pass # Should not happen if the library is installed

# Handle Signer and Verifier from base
elif name in ("Signer", "Verifier") and hasattr(obj, "__module__"):
Copy link
Contributor

Choose a reason for hiding this comment

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

This code under this block is similar to the above if statement. Is it possible to refactor it?

Comment on lines 25 to 26
# rsa==4.5 is the last version to support 2.7
# https://github.com/sybrenstuvel/python-rsa/issues/152#issuecomment-643470233
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# rsa==4.5 is the last version to support 2.7
# https://github.com/sybrenstuvel/python-rsa/issues/152#issuecomment-643470233

Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

Also remove rsa here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants