replace pycrypto with pycryptodomex [2017.7] #54119
Conversation
pycrypto is unmaintained and has open security issues. Fixes: saltstack#54115
ghost
requested a review
from 
|
@JanZerebecki There is a plan to make Salt cryptography a plug-able entity. Currently pycryptodomex is not available on all platforms that Salt provides for. Currently there is a hierarchy as follows, in preferred order: Hence, if pycryptodomex is already installed, it will be preferred over pycrypto. Lastly this only addresses Suse spec file, and pip requirements, actual packaging occurs with https://github.com/saltstack/salt-pack and https://github.com/saltstack/salt-pack-py3 |
|
The main goal of this PR was fixing this for pip requirements, the changes under pkg were just easy to add while I was at it. pycrypto should not be installed on a platform that has one of your stated alternatives. Do you agree with that? @dmurphy18 I couldn't read from your comment if you think something about this PR needs to be changed or if it is acceptable as is. If it needs to be changed, please point out how. |
|
@JanZerebecki re: pycrypto should not be installed on a platform that has one of your stated alternatives. Do you agree with that? I personally prefer that a customer use a better alternative than pycrypto when it is available, however cannot dictate customer circumstances, hence the alternative of providing a preference for now with Python 2. As for the PR I cannot see it passing all tests since as previously stated, pycryptodomex is not available on all platforms that SaltStack supports. Also note that the 2017.7 branch is only supported for CVE's and this would be better placed in a later branch if it was to be considered. |
|
I'm not saying that salt should stop working with pycrypto. But it should only stop preferring it over the others when being installed using pip. Currently with pip it installs pycrypto even when pycryptodomex is already installed. On which platforms is pycryptodomex not available via pip, where pycrypto is? It is hard to read the CI log, but the only failure seems unrelated https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py3/job/PR-54119/1/consoleText (same one on py2/centos7 and py2/ubuntu1604):
Any idea regarding the failure? |
|
@JanZerebecki The package does not build on Debian/Ubuntu is the most probable cause. Understand your point with pip preferences but you need to ensure works on Debian 8/9, Ubuntu 16.04/18.04, Redhat 6/7 |
|
Earlier in the log for all the platforms CI runs on one can see that installing pycryptodomex worked just fine on each of them. So that is not the cause for the CI failure. |
|
@JanZerebecki Need to review if M2Crypto is the preferred crypto library, have to discuss this within the core team, next week, and shall get back to you |
This might help understand what is happening when test_issue_2731_masterless fails and stdout of the test process is empty.
|
@JanZerebecki pycryptodomex shall be the pip install preferred library, will take a deeper look into this PR and see why tests failing. Can you re-submit these changes against the 2018.3 branch since that is the branch which is currently supported. The 2017.7 branch is only for CVE fixes at this point. Thanks |
JanZerebecki
changed the title
|
The main commit cherry picked on 2018.3: #54241 |
pycrypto is unmaintained and has open security issues.
Fixes: #54115
What does this PR do?
replace pycrypto with pycryptodomex, like it already is on many platforms
What issues does this PR fix or reference?
#54115
Previous Behavior
require pycrpto
New Behavior
require pycryptodomex
Tests written?
No, drop in replacement of functionality.
Commits signed with GPG?
Yes