-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support for traditional OpenSSL and PKCS8 RSA private key serialization #1503
Changes from all commits
f83e25c
199dc27
4d23604
8aad028
858dd3f
6177cbe
20456e9
45be354
35194c9
223a8f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,8 @@ pseudorandom | |
pyOpenSSL | ||
Schneier | ||
scrypt | ||
Serializers | ||
serializer | ||
Solaris | ||
Tanja | ||
testability | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,13 @@ | |
from cryptography.hazmat.primitives.asymmetric.padding import ( | ||
AsymmetricPadding, MGF1, OAEP, PKCS1v15, PSS | ||
) | ||
from cryptography.hazmat.primitives.interfaces import ( | ||
RSAPrivateKeyWithNumbers, RSAPublicKeyWithNumbers | ||
from cryptography.hazmat.primitives.asymmetric.rsa import ( | ||
RSAPrivateKeyWithNumbers, RSAPrivateKeyWithSerialization, | ||
RSAPublicKeyWithNumbers | ||
) | ||
from cryptography.hazmat.primitives.serialization import ( | ||
BestAvailableEncryption, Encoding, Format, KeySerializationEncryption, | ||
NoEncryption | ||
) | ||
|
||
|
||
|
@@ -507,6 +512,7 @@ def _verify_pss(self, evp_md): | |
|
||
|
||
@utils.register_interface(RSAPrivateKeyWithNumbers) | ||
@utils.register_interface(RSAPrivateKeyWithSerialization) | ||
class _RSAPrivateKey(object): | ||
def __init__(self, backend, rsa_cdata): | ||
self._backend = backend | ||
|
@@ -559,6 +565,62 @@ def private_numbers(self): | |
) | ||
) | ||
|
||
def private_bytes(self, encoding, format, encryption_algorithm): | ||
if not isinstance(encoding, Encoding): | ||
raise TypeError("encoding must be an item from the Encoding enum") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Experience has taught me that any error message like this really ought to have an "… instead got …" portion which explains what object was received instead. Otherwise debugging can be agonizing - "I only get this exception in production, my unit tests aren't catching it, what am I doing? am I reversing the order of arguments, is something naming a variable wrong..." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't really do that /anywhere/ else right now. If we want to do that we On Sat, Feb 28, 2015 at 12:50 PM, Glyph notifications@github.com wrote:
"I disapprove of what you say, but I will defend to the death your right to |
||
|
||
if not isinstance(format, Format): | ||
raise TypeError("format must be an item from the Format enum") | ||
|
||
# This is a temporary check until we land DER serialization. | ||
if encoding is not Encoding.PEM: | ||
raise ValueError("Only PEM encoding is supported by this backend") | ||
|
||
if format is Format.PKCS8: | ||
write_bio = self._backend._lib.PEM_write_bio_PKCS8PrivateKey | ||
key = self._evp_pkey | ||
elif format is Format.TraditionalOpenSSL: | ||
write_bio = self._backend._lib.PEM_write_bio_RSAPrivateKey | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be clearer to write this as: if isinstance(s, PKCS8):
# ...
elif isinstance(s, TraditionalOpenSSL):
# ...
else:
raise TypeError("...") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still applicable. |
||
key = self._rsa_cdata | ||
|
||
if not isinstance(encryption_algorithm, KeySerializationEncryption): | ||
raise TypeError( | ||
"Encryption algorithm must be a KeySerializationEncryption " | ||
"instance" | ||
) | ||
|
||
if isinstance(encryption_algorithm, NoEncryption): | ||
password = b"" | ||
passlen = 0 | ||
evp_cipher = self._backend._ffi.NULL | ||
elif isinstance(encryption_algorithm, BestAvailableEncryption): | ||
# This is a curated value that we will update over time. | ||
evp_cipher = self._backend._lib.EVP_get_cipherbyname( | ||
b"aes-256-cbc" | ||
) | ||
password = encryption_algorithm.password | ||
passlen = len(password) | ||
if passlen > 1023: | ||
raise ValueError( | ||
"Passwords longer than 1023 bytes are not supported by " | ||
"this backend" | ||
) | ||
else: | ||
raise ValueError("Unsupported encryption type") | ||
|
||
bio = self._backend._create_mem_bio() | ||
res = write_bio( | ||
bio, | ||
key, | ||
evp_cipher, | ||
password, | ||
passlen, | ||
self._backend._ffi.NULL, | ||
self._backend._ffi.NULL | ||
) | ||
assert res == 1 | ||
return self._backend._read_mem_bio(bio) | ||
|
||
|
||
@utils.register_interface(RSAPublicKeyWithNumbers) | ||
class _RSAPublicKey(object): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,13 +42,30 @@ def public_key(self): | |
|
||
|
||
@six.add_metaclass(abc.ABCMeta) | ||
class RSAPrivateKeyWithNumbers(RSAPrivateKey): | ||
class RSAPrivateKeyWithSerialization(RSAPrivateKey): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there really a reason that this needs to be a subclass? Practically speaking, things which implement key serialization will also themselves be a key, but for testing or format-translation or other esoteric use-cases, mightn't there be a serialization provider with no actual There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really no. @alex, @public got opinions here? I'm happy to go either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leave the subclassing orthagonal IMO; no need to change more things than On Sat, Feb 28, 2015 at 4:45 PM, Paul Kehrer notifications@github.com
"I disapprove of what you say, but I will defend to the death your right to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough; In that case this PR is once again ready for review. I added additional cross-linking and changed the name of |
||
@abc.abstractmethod | ||
def private_numbers(self): | ||
""" | ||
Returns an RSAPrivateNumbers. | ||
""" | ||
|
||
@abc.abstractmethod | ||
def private_bytes(self, encoding, format, encryption_algorithm): | ||
""" | ||
Returns the key serialized as bytes. | ||
""" | ||
|
||
|
||
RSAPrivateKeyWithNumbers = utils.deprecated( | ||
RSAPrivateKeyWithSerialization, | ||
__name__, | ||
( | ||
"The RSAPrivateKeyWithNumbers interface has been renamed to " | ||
"RSAPrivateKeyWithSerialization" | ||
), | ||
utils.DeprecatedIn08 | ||
) | ||
|
||
|
||
@six.add_metaclass(abc.ABCMeta) | ||
class RSAPublicKey(object): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More words are needed here. Should probably specifically mention that where possible PKCS8 is prefered since it allows better encryption algorithms.