Skip to content

ROB: Improve RC4 fallback when cryptography can't provide it (#3398)#3853

Open
msylw wants to merge 11 commits into
py-pdf:mainfrom
msylw:add-rc4-fallback-when-missing-in-openssl
Open

ROB: Improve RC4 fallback when cryptography can't provide it (#3398)#3853
msylw wants to merge 11 commits into
py-pdf:mainfrom
msylw:add-rc4-fallback-when-missing-in-openssl

Conversation

@msylw

@msylw msylw commented Jun 12, 2026

Copy link
Copy Markdown

When OpenSSL is compiled without RC4 cryptography will not provide this as well. This breaks the fallback local RC4 implementation.

This change adds import time verification to cryptography's ability to handle RC4. If it fails, the existing fallback is used.

Only Applies to RC4, AES is not affected by this change.

AI (claude/sonnet) was used to debug the issue and draft the code. It was later edited for scope/simplicity.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.87%. Comparing base (efc511b) to head (496e171).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3853   +/-   ##
=======================================
  Coverage   97.86%   97.87%           
=======================================
  Files          57       57           
  Lines       10738    10777   +39     
  Branches     2006     2011    +5     
=======================================
+ Hits        10509    10548   +39     
  Misses        127      127           
  Partials      102      102           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stefan6419846

Copy link
Copy Markdown
Collaborator

Thanks for the PR.

I am tempted to not touch the central imports for this, but detect this specific error when actually doing the problematic calls in the cryptography-based implementation (after having it detected, we can store the state in a suitable manner for mass processing). This avoids the import-time overhead of the availability check. Additionally, I guess we should issue a warning at least once when we use the fallback.

@msylw

msylw commented Jun 15, 2026

Copy link
Copy Markdown
Author

I gave it another try, with the lazy detection approach. Is a little more complex than I expected, mostly as Cipher won't trigger the exception itself making CryptRC4 a little more complex than I hoped.

Just found cipher_supported . It could be used to initialize the _rc4_supported at init time, which could be a nice middle ground between the 2 solutions - probably lower overhead than the first version, but slightly higher than the 2nd one. Would make for much cleaner code, as there would be no need for the try blocks.

@msylw

msylw commented Jun 15, 2026

Copy link
Copy Markdown
Author

Just found cipher_supported . It could be used to initialize the _rc4_supported at init time, which could be a nice middle ground between the 2 solutions - probably lower overhead than the first version, but slightly higher than the 2nd one. Would make for much cleaner code, as there would be no need for the try blocks.

Never mind, it was an old version of cryptography, I can't find it in current docs...

@msylw
msylw marked this pull request as draft June 16, 2026 14:24
@msylw
msylw marked this pull request as ready for review June 17, 2026 09:28
Comment thread pypdf/_crypt_providers/_cryptography.py Outdated


def _disable_rc4() -> None:
global _rc4_supported # noqa: PLW0603

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of relying on a global variable, couldn't we make it a private member of CryptRC4 instead, id est using CryptRC4._rc4_supported for storage?

Comment thread pypdf/_crypt_providers/_cryptography.py Outdated
from cryptography.hazmat.primitives.ciphers.modes import CBC, ECB
from cryptography.hazmat.primitives.padding import PKCS7

from pypdf._crypt_providers import _fallback

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are we able to only import this if we really need it? I would assume that the default case is that we do not have issues, thus importing it here should not be required in these cases.

@msylw

msylw commented Jul 7, 2026

Copy link
Copy Markdown
Author

I made the suggested modifications, feel free to take a look.

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.

2 participants