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 all usage of PyCrypto with python-cryptography #1280

Merged
merged 8 commits into from
Mar 19, 2019
Merged

Replace all usage of PyCrypto with python-cryptography #1280

merged 8 commits into from
Mar 19, 2019

Conversation

ryanpetrello
Copy link
Contributor

@ryanpetrello ryanpetrello commented Mar 11, 2019 •
edited
Loading

PyCrypto is dead (the last release to PyPI was in October 2013) and insecure:

pycrypto/pycrypto#173

see: https://issues.apache.org/jira/browse/LIBCLOUD-1015

@codecov-io
Copy link

codecov-io commented Mar 11, 2019 •
edited
Loading

Codecov Report

Merging #1280 into trunk will increase coverage by 0.03%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##            trunk    #1280      +/-   ##
==========================================
+ Coverage   85.91%   85.95%   +0.03%     
==========================================
  Files         359      359              
  Lines       73777    73774       -3     
  Branches     6689     6690       +1     
==========================================
+ Hits        63386    63411      +25     
+ Misses       7719     7688      -31     
- Partials     2672     2675       +3
Impacted Files Coverage Δ
libcloud/test/common/test_google.py 97.23% <ø> (+2.76%) ⬆️
libcloud/common/google.py 65.33% <0%> (+3.33%) ⬆️
libcloud/test/compute/test_softlayer.py 92.74% <0%> (+2.66%) ⬆️
libcloud/utils/py3.py 46.25% <0%> (ø) ⬆️
libcloud/compute/drivers/softlayer.py 84.79% <0%> (-1.09%) ⬇️
libcloud/test/test_utils.py 95.13% <100%> (+0.21%) ⬆️
libcloud/utils/publickey.py 56% <58.33%> (+30.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8735a7...633289e. Read the comment docs.

@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 15, 2019 •
edited
Loading

@wrigri @Kami @tonybaloney any chance you've got some time to look over this change?

@wrigri
Copy link
Contributor

wrigri commented Mar 15, 2019

Looks good to me. (It's been a long time since I touched this code though.)

@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 15, 2019 •
edited
Loading

For what it's worth, we use libcloud in https://github.com/ansible/awx for generating cloud inventory from hosts in your GCE account, and I was able to step through the code in this PR w/ a Python debugger and see it properly sign and auth and pull down hostnames from Google's API.

This was referenced Mar 18, 2019
Closed
Closed
@ryanpetrello ryanpetrello changed the title Use cryptography (not PyCrypto) for GCE service account authentication Replace all usage of PyCrypto with python-cryptography Mar 19, 2019
@Kami Kami self-requested a review March 19, 2019 18:39
@Kami
Copy link
Member

Kami commented Mar 19, 2019

Thank you for this contribution 👍

I'm +1 for it, because as you've said, PyCrypto in not actively maintained anymore.

In the past, we had more code relying and using PyCrypto, but that's not the case anymore so the change is relatively contained and straight forward.

I will look into failing tests, fix them and merge this into master.

ryanpetrello
ryanpetrello commented Mar 19, 2019
View reviewed changes
libcloud/test/test_utils.py Outdated

def test_pubkey_openssh_fingerprint(self):
fp = get_pubkey_openssh_fingerprint(self.PUBKEY)
assert fp == '35:22:13:5b:82:e2:5d:e1:90:8c:73:74:9f:ef:3b:d8'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got this value from what PyCrypto code generated prior to this PR.

ryanpetrello
ryanpetrello commented Mar 19, 2019
View reviewed changes
libcloud/test/test_utils.py Outdated

def test_pubkey_ssh2_fingerprint(self):
fp = get_pubkey_ssh2_fingerprint(self.PUBKEY)
assert fp == '11:ad:5d:4c:5b:99:c9:80:7e:81:03:76:5a:25:9d:8c'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one, too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's related to an edge case in hexadigits utility function - I'm looking into it.

ryanpetrello
ryanpetrello commented Mar 19, 2019
View reviewed changes
libcloud/utils/py3.py
@@ -126,7 +126,7 @@ def tostring(node):

def hexadigits(s):
# s needs to be a byte string.
return [format(x, "x") for x in s]
return [format(x, "02x") for x in s]
Copy link
Contributor Author

@ryanpetrello ryanpetrello Mar 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ended up noticing a discrepancy between py2 and py3 here for integers 0-9 i.e.,

:3: vs :03:

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

This should indeed fix it and ensure leading 0 is always present.

@Kami Kami merged commit 5c93852 into apache:trunk Mar 19, 2019
@Kami
Copy link
Member

Kami commented Mar 19, 2019

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kami Kami Kami approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanpetrello @codecov-io @wrigri @Kami