Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Switch to using "cryptography" Python lib #953

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

RJPercival
Copy link
Contributor

Replaces "ecdsa" and "pycrypto" libraries; the former is considered to be poorly implemented, whilst the latter is poorly maintained.

This library correctly raises exceptions for all forms of invalid ASN.1 in STHs, unlike the "ecdsa" library, so some tests have been modified to account for this and a TODO removed.

See: https://cryptography.io

"""
if (key_info.type != client_pb2.KeyInfo.ECDSA):
raise error.UnsupportedAlgorithmError(
"Expected ECDSA key, but got key type %d" % key_info.type)

# Will raise a PemError on invalid encoding
self.__der, _ = pem.from_pem(key_info.pem_key, self.__READ_MARKERS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is self.__der no longer required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because the cryptography library can load a public key directly from PEM, so there's no need to manually convert to DER first. It can also convert straight back to PEM, so self.__der isn't required in repr() either.

@phad phad added the LGTM label Sep 25, 2015
@eranmes
Copy link
Contributor

eranmes commented Sep 25, 2015

Per @ekasper 's comments on cryptography.io , undoing the LGTM until we further discuss how to proceed with it.

@RJPercival
Copy link
Contributor Author

@ekasper has rescinded her reservations about cryptography.io now. However, we probably want to sit on this PR a bit longer until other Googlers have been using cryptography.io successfully for a little while (and possibly worked out any kinks).

Replaces "ecdsa" and "pycrypto" libraries; the former is considered to be
poorly implemented, whilst the latter is poorly maintained.

See: https://cryptography.io

Fixes some broken test
@RJPercival
Copy link
Contributor Author

I think we've waited more than long enough to de-risk this now.

@eranmes
Copy link
Contributor

eranmes commented Dec 7, 2016

Re-adding original LGTM, approval.

@RJPercival RJPercival merged commit 3c34ae9 into google:master Dec 7, 2016
RJPercival pushed a commit to RJPercival/certificate-transparency that referenced this pull request Sep 6, 2018
We don't state a dependency on M2Crypto in requirements.txt, and it
creates a transitive dependency on a compatible version of OpenSSL being
installed too, so it'd make this script more portable if we used
cryptography.io instead (we started migrating to that with PR google#953).
RJPercival pushed a commit to RJPercival/certificate-transparency that referenced this pull request Sep 6, 2018
We don't state a dependency on M2Crypto in requirements.txt, and it
creates a transitive dependency on a compatible version of OpenSSL being
installed too, so it'd make this script more portable if we used
cryptography.io instead (we started migrating to that with PR google#953).
@RJPercival RJPercival deleted the crypt_lib branch September 6, 2018 14:51
RJPercival pushed a commit that referenced this pull request Sep 7, 2018
We don't state a dependency on M2Crypto in requirements.txt, and it
creates a transitive dependency on a compatible version of OpenSSL being
installed too, so it'd make this script more portable if we used
cryptography.io instead (we started migrating to that with PR #953).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants