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

Swapping PyCrypto for pyOpenSSL. #1338

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jan 2, 2016

This was done because PyCrypto does not install easily on Windows. pyOpenSSL is managed by PyCA (the Python crypto authority) and has a mature release process.

This change was influenced by discussions about #1009.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 2, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Jan 2, 2016

To "confirm" this implementation matches our current one I did the following:

>>> import json
>>> from oauth2client.client import GoogleCredentials
>>> from OpenSSL import crypto
>>> from gcloud import credentials
>>> creds = GoogleCredentials.get_application_default()  # Env. var. -> path to JSON key
>>> pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, creds._private_key_pkcs8_text)
>>> data = b'foo'
>>> new_signed_bytes = crypto.sign(pkey, data, 'SHA256')
>>> curr_signed_bytes = credentials._get_signature_bytes(creds, data)
>>> new_signed_bytes == curr_signed_bytes
True

@dhermes
Copy link
Contributor Author

dhermes commented Jan 2, 2016

H/T to @tseaver for pointing out that cryptography was managed by the PyCA which led me to discover that pyOpenSSL was too.

@@ -20,6 +20,8 @@ class _Monkey(object):

def __init__(self, module, **kw):
self.module = module
if len(kw) == 0: # pragma: NO COVER
raise ValueError('_Monkey was used with nothing to monkey-patch')

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This was done because PyCrypto does not install easily on Windows.
pyOpenSSL is managed by PyCA (the Python crypto authority) and
has a mature release process.

This change was influenced by discussions about googleapis#1009.
@dhermes
Copy link
Contributor Author

dhermes commented Jan 7, 2016

@jonparrott Is pyOpenSSL supported on App Engine?

@theacodes
Copy link
Contributor

I don't think so; only pycrypto and the core ssl library.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 7, 2016

OK. PyCrypto has a pretty crappy install story (latest version isn't on PyPI, pip install fails on Windows).

It may be best to take the oauth2client approach and just implement using both pyOpenSSL and PyCrypto but require neither in setup.py.

@theacodes
Copy link
Contributor

Sounds reasonable.

@tseaver
Copy link
Contributor

tseaver commented Jan 7, 2016

I'd really hate to have our codebase cluttered with both the current PyCrypto-based implementation and the new pyOpenSSL-based one.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 7, 2016

  1. The original goal: better Windows experience, is not really possible with PyCrypto since the maintainers don't have great windows support.
  2. pyOpenSSL not working on Google App Engine is something we can avoid if we allow PyCrypto as a fallback since it is supported on GAE
  3. I wouldn't exactly call it cluttered. We already have both implementations and they are both short (and only used for signing bytes). We've mentioned before we wanted to push the signing upsteam into oauth2client anyhow.

Maybe now is the time to bite the bullet and get both implementations into oauth2client?

Then the question becomes

  • Are we OK having a component that depends on one of pyOpenSSL and PyCrypto being installed if we don't include either one in our setup.py?

@tseaver
Copy link
Contributor

tseaver commented Jan 7, 2016

Are we OK having a component that depends on one of pyOpenSSL and PyCrypto being installed if we don't include either one in our setup.py?

I really don't want to introduce imperative platform-based variations in the requirements (breaking wheel generation, for instance). Is there a PEP 508-supported way to detect GAE?

@theacodes
Copy link
Contributor

Is there a PEP 508-supported way to detect GAE?

No.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 7, 2016

@jonparrott Wrote https://github.com/jonparrott/Darth-Vendor which probably makes him the foremost expert on packaging for GAE 😀

@dhermes
Copy link
Contributor Author

dhermes commented Jan 8, 2016

@jonparrott After some digging I realized this can be done in pure Python using pyasn1, pyasn1-modules and rsa. Is there any vendor-ing issue for these libraries on GAE?

@theacodes
Copy link
Contributor

@dhermes if they have any native components, then yes. Let me verify.

@theacodes
Copy link
Contributor

@dhermes those should be fine. Go for it. 👍

@dhermes
Copy link
Contributor Author

dhermes commented Jan 8, 2016

Good deal. Thanks for doing it for me (I know I could've RTFM instead of wasting your time).

@theacodes
Copy link
Contributor

I know I could've RTFM instead of wasting your time

Considering how much of your time I've monopolized elsewhere, I'd say I owe you.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 8, 2016

As I was starting to do the rsa/pyasn1 implementation I realized GAE users are [covered] by using oauth2client.appengine.AppAssertionCredentials so we don't need to worry if they have support for either pyOpenSSL or PyCrypto.

@jonparrott What does darth vendor do if a package can't be installed / imported in GAE?

@theacodes
Copy link
Contributor

@dhermes nothing. pip handles the installation, not the vendor tool. Pip will happily stage a binary package into the lib directory. :(

@dhermes
Copy link
Contributor Author

dhermes commented Jan 8, 2016

OK. I suppose we could try/except ImportError on the pyOpenSSL import. Though it seems strange to do for something in setup.py.

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

Would the try:..except: in setup.py be to make the dependency on PyCrypto be "soft" FBO the GAE environment?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

What did you have in mind?

i.e. Try and except what import? How would it help GAE?

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

I'm responding to:

OK. I suppose we could try/except ImportError on the pyOpenSSL import. Though it seems strange to do for something in setup.py.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

I meant try/except in our code, not in setup.py.

@theacodes
Copy link
Contributor

setup.py doesn't run on app engine, it runs on the user's machine when they vendor the package.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

@jonparrott Do you think it's fine to just leave pyopenssl as a dep since on GAE the user has the fancy http://oauth2client.readthedocs.org/en/stable/source/oauth2client.appengine.html module (now moved to contrib)

@theacodes
Copy link
Contributor

I think you're in the clear as long as you don't ever try to use the library within GAE code.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

@tseaver GAE is "no longer" a blocker for using pyopenssl. Remaining issues?

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

@dhermes

GAE is "no longer" a blocker for using pyopenssl. Remaining issues?

I thought GAE was the only reason to keep the pyopenssl codepath around at all. Am I mistaken?

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

Nope its Windows. Check out the description of this PR.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

I just scared myself into thinking that it'd be a blocker for GAE but forgot that GAE and GCE had custom credentials types

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

Oops, I misspoke: I meant I thought the old codepath (PyCrypto) was being kept around FBO GAE. If that isn't so, then lets just punt and make pyOpenSSL the only way we do this.

@dhermes
Copy link
Contributor Author

dhermes commented Jan 14, 2016

Got it. All good to merge?

(PS I am planning on pushing this upstream to oauth2client at some point.)

@tseaver
Copy link
Contributor

tseaver commented Jan 14, 2016

LGTM. I had lost track of the fact that you already dropped the old codepath.

dhermes added a commit that referenced this pull request Jan 14, 2016
Swapping PyCrypto for pyOpenSSL.
@dhermes dhermes merged commit 30acf33 into googleapis:master Jan 14, 2016
@dhermes dhermes deleted the swap-crypto-libs branch January 14, 2016 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth cla: yes This human has signed the Contributor License Agreement. packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants