Skip to content
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

ENH: Use cryptography for encryption/decryption as a fallback for PyCryptodome #2000

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

exiledkingcc
Copy link
Contributor

@exiledkingcc exiledkingcc commented Jul 22, 2023

Use the cryptography package (maintained by the Python Cryptographic Authority, pyca) as a fallback if PyCryptodome (maintained by Legrandin / Helder Eijs) is not installed.

Closes #1998

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Patch coverage: 98.72% and project coverage change: +0.14 🎉

Comparison is base (6df64af) 93.92% compared to head (f59f75f) 94.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2000      +/-   ##
==========================================
+ Coverage   93.92%   94.06%   +0.14%     
==========================================
  Files          33       38       +5     
  Lines        7011     7081      +70     
  Branches     1394     1394              
==========================================
+ Hits         6585     6661      +76     
+ Misses        272      269       -3     
+ Partials      154      151       -3     
Impacted Files Coverage Δ
pypdf/_crypt_providers/_fallback.py 95.23% <95.23%> (ø)
pypdf/_crypt_providers/__init__.py 100.00% <100.00%> (ø)
pypdf/_crypt_providers/_base.py 100.00% <100.00%> (ø)
pypdf/_crypt_providers/_cryptography.py 100.00% <100.00%> (ø)
pypdf/_crypt_providers/_pycryptodome.py 100.00% <100.00%> (ø)
pypdf/_encryption.py 95.92% <100.00%> (+1.00%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pubpub-zz
Copy link
Collaborator

@exiledkingcc
canyou upgrade the workflow to add test with cryptography?

@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Jul 25, 2023
@MartinThoma MartinThoma changed the title ENH: support Cryptography for encryption/decryption ENH: Use cryptography for encryption/decryption as a fallback for PyCryptodome Jul 25, 2023
@MartinThoma
Copy link
Member

Could you please also adjust https://github.com/py-pdf/pypdf/blob/main/docs/user/encryption-decryption.md (the comment about pycryptodome)?

Comparison of the two libraries:

It seems to me that we should primarily use cryptography and only use pycryptodome as a fallback. What do you think @exiledkingcc ?

@exiledkingcc
Copy link
Contributor Author

your opinion sounds very reasonable,i agree with it. but should contain it in the commit? then how about pip install pypdf[crypto] and ralated documents, will they should be changed in the commit also?

@MartinThoma
Copy link
Member

It's ok for me if you leave it as it is. That is not a blocker.

The change of the default (from pycryptodome to cryptography) should probably be done AFTER I merge this one. Just to keep this PR fairly stable. I want to merge it on Friday evening / early Saturday.

@MartinThoma MartinThoma merged commit 277643f into py-pdf:main Jul 27, 2023
@exiledkingcc exiledkingcc deleted the crypto branch July 29, 2023 01:46
MartinThoma added a commit that referenced this pull request Jul 29, 2023
## What's new

### New Features (ENH)
-  Accelerate image list keys generation (#2014)
-  Use `cryptography` for encryption/decryption as a fallback for PyCryptodome (#2000)
-  Extract LaTeX characters (#2016)
-  ASCIIHexDecode.decode now returns bytes instead of str (#1994)

### Bug Fixes (BUG)
-  Add RunLengthDecode filter (#2012)
-  Process /Separation ColorSpace (#2007)
-  Handle single element ColorSpace list (#2026)
-  Process lookup decoded as TextStringObjects (#2008)

### Robustness (ROB)
-  Cope with garbage collector during cloning (#1841)

### Maintenance (MAINT)
-  Cleanup of annotations (#1745)

[Full Changelog](3.13.0...3.14.0)
@xilopaint
Copy link
Contributor

xilopaint commented Jul 29, 2023

@exiledkingcc @MartinThoma I can't understand, v3.14.0 release mentions this PR but pip install "pypdf[crypto]" still installs PyCryptodome instead of cryptography. What did I not understand well?

Also, could you guys clarify if cryptography is a pure-python library and if it handles AES encryption?

@exiledkingcc
Copy link
Contributor Author

exiledkingcc commented Jul 30, 2023

for now, cryptography is a fallback for PyCryptodome. it will change in the future, since cryptography seems more official.

Also, could you guys clarify if cryptography is a pure-python library and if it handles AES encryption?

cryptography is a wrapper of openssl, not pure-python.

@xilopaint
Copy link
Contributor

for now, cryptography is a fallback for PyCryptodome. it will change in the future, since cryptography seems more official.

Does it mean for now we need pip install cryptography and pip install pypdf[crypto] will install cryptography in the future?

cryptography is a wrapper of openssl, not pure-python.

Does it handle AES encryption?

@exiledkingcc
Copy link
Contributor Author

for now,you can pip install pypdf cryptography to use cryptography in pypdf. in the future, pip install pypdf[crypto] will install cryptography automatically.

cryptography can handle AES, and many other algorithms.

@MartinThoma
Copy link
Member

The latest cryptography does not support Python 3.6 anymore since 41.0.0. I'm not sure if that might be a blocker from changing the default.

There are still older versions, but I don't know if they are secure as they might have unfixed known bugs.

That is a pretty strong reason for me to drop Python 3.6 support.

@MartinThoma
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
soon PRs that are almost ready to be merged, issues that get solved pretty soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider migrating to Cryptography
4 participants