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.

Sign up for GitHub

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

Jump to bottom

Replace PyCrypto with tinyaes for encryption support #4652

Merged
merged 1 commit into from May 31, 2020
Merged

Replace PyCrypto with tinyaes for encryption support #4652

merged 1 commit into from May 31, 2020

Conversation

naufraghi
Copy link
Contributor

@naufraghi naufraghi commented Jan 29, 2020 •
edited by Legorooj

tinyaes is a minimal AES-only library that wraps the C library tiny-AES-c.

Currently the library wraps only the CTR mode that can be used in PyInstaller instead of the CFB mode used before.

Reading various sources, CFB is somewhat similar to CBC and CTR is suggested as a go-to stream cipher (https://crypto.stackexchange.com/questions/6029/aes-cbc-mode-or-aes-ctr-mode-recommended and links) that can be used instead of CBC.

See the issue comments for more discussion #2365 (comment)

Closes #2365

@naufraghi naufraghi mentioned this pull request Jan 29, 2020
Closed
@naufraghi naufraghi force-pushed the 2365-replace-pycrypto-with-tinyaes branch 3 times, most recently from 2187fd2 to a8410a7 Compare January 29, 2020 16:09
@naufraghi naufraghi changed the title WIP: Replace PyCrypto with tinyaes for encryption support Replace PyCrypto with tinyaes for encryption support Jan 30, 2020
@BoboTiG
Copy link
Contributor

BoboTiG commented Feb 6, 2020

The new code is much simpler to read, and after so many months there is finally a possible replacement for that critical code part (that I failed to do it myself, twice ^^). 💪

@naufraghi
Copy link
Contributor Author

Do we need a similar PR for the master too?

@naufraghi naufraghi force-pushed the 2365-replace-pycrypto-with-tinyaes branch from a8410a7 to ce127b9 Compare February 7, 2020 08:19
@naufraghi
Copy link
Contributor Author

Hmm, I'll wait a bit more, I'm adding some test to the library, I'd prefer to point a stable release in tests/requirements-tools.txt

@naufraghi naufraghi changed the title Replace PyCrypto with tinyaes for encryption support WIP: Replace PyCrypto with tinyaes for encryption support Feb 7, 2020
@BoboTiG
Copy link
Contributor

BoboTiG commented Feb 7, 2020

Do we need a similar PR for the master too?

Nope, target develop only.

@naufraghi naufraghi requested review from Legorooj and removed request for htgoebel February 7, 2020 16:20
@Legorooj
Copy link
Member

Legorooj commented Feb 7, 2020 •
edited

@naufraghi will review this later today when on PC. This looks good at the moment. I will point out that I can't actually merge this - you need @bjones1 for that. Oh and don't target master. Only stable releases get pushed to that branch. We want develop.

@naufraghi
Copy link
Contributor Author

Sorry for the review requests, I was just inspecting the the GitHub gui 🙄

@Legorooj
Copy link
Member

Legorooj commented Feb 8, 2020

@naufraghi not a problem. I've got to say, this looks good. I'm going to suggest that seeing as you said you don't want the bounty, it goes to the PyInstaller team to fund @htgoebel.

@Legorooj Legorooj added the @high label Feb 9, 2020
Legorooj
Legorooj requested changes Feb 16, 2020
View reviewed changes
Copy link
Member

@Legorooj Legorooj left a comment •
edited

Choose a reason for hiding this comment

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

You need to add a changelog entry, as per https://pyinstaller.readthedocs.io/en/stable/development/changelog-entries.html. It can be a bit confusing - see the news folder for some examples. Also:

  • edit the README
  • And the docs. Remove PyCrypto references

tests/functional/test_basic.py Outdated Show resolved Hide resolved
@naufraghi naufraghi force-pushed the 2365-replace-pycrypto-with-tinyaes branch 2 times, most recently from bd95389 to 3c52c69 Compare February 17, 2020 10:29
@naufraghi
Copy link
Contributor Author

Added news/4652.bootloader.rst and updated both README and usage.rst, need to check that everything works as expected (is there some easy way using the CI?).

Legorooj
Legorooj requested changes Feb 17, 2020
View reviewed changes
Copy link
Member

@Legorooj Legorooj left a comment •
edited

Choose a reason for hiding this comment

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

You need to change the requirements version spec - https://travis-ci.org/pyinstaller/pyinstaller/jobs/651427755#L480.

As to your last comment, the when you push to origin, the CI runs automatically.

@naufraghi

doc/_common_definitions.txt Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@Legorooj
Copy link
Member

@bjones1 @htgoebel

@naufraghi
Copy link
Contributor Author

I use to let the reviewer resolve the discussions, but I can close myself the ones I think I have resolved, let me know the preferred workflow.

@bjones1
Copy link
Contributor

bjones1 commented Feb 21, 2020

Sorry, I lack the time to review this.

@Legorooj Legorooj removed the request for review from bjones1 February 21, 2020 22:44
@Legorooj
Copy link
Member

@htgoebel Can you please review?

@Legorooj Legorooj self-requested a review March 7, 2020 09:18
@Legorooj Legorooj added the bug label Mar 7, 2020
@Legorooj Legorooj added this to the PyInstaller 4.0 milestone Mar 7, 2020
@naufraghi naufraghi force-pushed the 2365-replace-pycrypto-with-tinyaes branch from 5b7c14e to ff6f3f5 Compare March 21, 2020 09:18
@naufraghi
Copy link
Contributor Author

naufraghi commented Mar 24, 2020 •
edited

Rebased, just to check everything still works, and we have some unrelated error on macOS and nightly.

@Legorooj
Copy link
Member

@naufraghi I've restarted the failing build. It's running. FYI we're just waiting for @htgoebel to review and merge.

@Legorooj
Copy link
Member

@htgoebel can you look at this PR now? It should be ready to merge, we just need final review.

@naufraghi @htgoebel
Replace PyCrypto with tinyaes for encryption support.
455c57f 
This re-enables encrypting the Python bytecode (option ``--key``).

`tinyaes <https://github.com/naufraghi/tinyaes-py>`_
is a minimal AES-only library that wraps the C library
`tiny-AES-c <https://github.com/kokke/tiny-AES-c>`_.

Currently the library wraps only the CTR mode that can be used in
PyInstaller instead of the CFB mode used before.

Reading various sources, CFB is `somewhat similar to CBC
<https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Cipher_Feedback_(CFB)>`_
and CTR is suggested as a go-to stream cipher
(see <https://crypto.stackexchange.com/questions/6029> and links)
that can be used instead of CBC.

Closes pyinstaller#2365.
@htgoebel htgoebel force-pushed the 2365-replace-pycrypto-with-tinyaes branch from ff6f3f5 to 455c57f Compare April 19, 2020 09:15
@Legorooj
Copy link
Member

@htgoebel, can I merge this?

@Legorooj Legorooj mentioned this pull request May 13, 2020
Closed
@brentvollebregt brentvollebregt mentioned this pull request May 20, 2020
Closed
@Legorooj Legorooj merged commit d914a23 into pyinstaller:develop May 31, 2020
@Legorooj
Copy link
Member

@naufraghi thanks for this feature/bugfix!

@naufraghi naufraghi deleted the 2365-replace-pycrypto-with-tinyaes branch May 31, 2020 07:10
@johnthagen johnthagen mentioned this pull request May 31, 2020
Closed
@naufraghi naufraghi restored the 2365-replace-pycrypto-with-tinyaes branch June 1, 2020 10:40
This was referenced Jun 6, 2020
Closed
Closed
cool-RR pushed a commit to cool-RR/pyinstaller that referenced this pull request Jun 20, 2020
@naufraghi @cool-RR
building/loader: replace PyCrypto with tinyaes for encryption support. (
781c2d0 
pyinstaller#4652)

This re-enables encrypting the Python bytecode (`--key`) by replacing PyCrypto with tinyaes.

tinyaes is a minimal AES-only library that wraps the C library tiny-AES-c. See
https://github.com/naufraghi/tinyaes-py for source code. 

Closes pyinstaller#2365.
@Legorooj Legorooj removed the request for review from htgoebel August 17, 2020 07:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@htgoebel htgoebel htgoebel left review comments

@rrfaria rrfaria rrfaria approved these changes

@Legorooj Legorooj Legorooj approved these changes

Assignees

@naufraghi naufraghi

@htgoebel htgoebel

@Legorooj Legorooj

Labels
bug @high
Projects
None yet
Milestone
PyInstaller 4.0
Development

Successfully merging this pull request may close these issues.

replace Pycrypto - which is unsupported and buggy
6 participants
@naufraghi @BoboTiG @Legorooj @bjones1 @htgoebel @rrfaria