Support for traditional OpenSSL and PKCS8 RSA private key serialization #1503
Conversation
|
|
||
| :param enctype: :class:`~cryptography.hazmat.primitives.serialization.EncryptionTypes` | ||
|
|
||
| :param bytes password: The password for encryption. |
There was a problem hiding this comment.
Hmm, needing both params makes me think this should be a class, there's no reason to have a password param if you're doing NoEncryption
|
Test FAILed. |
|
test failure relevant (use of unicode syntax in test_serialization.py) On Sat Nov 29 2014 at 4:01:37 PM jenkins-cryptography <
|
|
Test PASSed. |
|
I think we can do it simpler than that, just rename the interface, add a compatibility name, and be happy. That's the same thign we did with CMACContext -> MACContext. |
|
Done |
|
Test PASSed. |
|
Test PASSed. |
|
Test PASSed. |
1 similar comment
| class EncryptionTypes(Enum): | ||
| BestAvailable = "Best available to the backend" | ||
| AES256CBC = "256-bit AES in CBC mode" | ||
| TripleDESCBC = "Triple DES EDE in CBC mode" |
There was a problem hiding this comment.
These are a bit mis-leading; using the same value between PKCS8 and PKCS1 gets you totally different things, since they use differnt KDFs. I also think we should switch to using classes, instead of enums, so we vaoid issues like NoEncryption requiring you to pass an empty password
There was a problem hiding this comment.
So we'd need to define an interface and 4 classes (for now) that conform to it?
@six.add_metaclass(abc.ABCMeta)
class KeySerializationEncryption(object):
pass
@utils.register_interface(KeySerializationEncryption)
class BestAvailable(object):
def __init__(self, password):
self.password = password
@utils.register_interface(KeySerializationEncryption)
class AES256CBC(object):
def __init__(self, password):
self.password = password
@utils.register_interface(KeySerializationEncryption)
class TripleDESCBC(object):
def __init__(self, password):
self.password = password
@utils.register_interface(KeySerializationEncryption)
class NoEncryption(object):
passWhich would make calls to serialize look like:
key.dump_pem(PKCS8(AES256CBC("password"))
key.dump_pem(PKCS1(NoEncryption()))There was a problem hiding this comment.
(The interface would be so we can easily test to see if the passed object is permissible)
|
Test FAILed. |
|
Tests are very red :-) |
|
Holiday commit!
|
|
Test PASSed. |
|
|
||
| :param enctype: :class:`~cryptography.hazmat.primitives.asymmetric.serialization.KeySerializationEncryption` | ||
|
|
||
| .. class:: TraditionalOpenSSL(enctype) |
There was a problem hiding this comment.
Now that all the serializers just have their own encryption, should we just move this "up the stack" as it were, and onto the dump_pem method?
|
There's still a few outstanding comments here, but progress! |
3 similar comments
| .. method:: as_bytes(encoding, format, encryption_algorithm) | ||
|
|
||
| Allows serialization of the key to bytes. Encoding (PEM or DER), format | ||
| (TraditionalOpenSSL or PKCS8) and encryption algorithm (such as |
There was a problem hiding this comment.
Can we linkify PEM, DER, TraditionalOpenSSL, and PKCS8
|
Besides these two comments, this is ready as far as I'm concerned. Since it's pretty large and introducing a new API I'd like one person to double check me though :-) |
| :class:`~cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateNumbers` | ||
| instance. | ||
|
|
||
| .. method:: as_bytes(encoding, format, encryption_algorithm) |
There was a problem hiding this comment.
Why private_numbers but not private_bytes? This seems inconsistent.
Would private_as_numbers / private_as_bytes make sense?
|
I'm happy with this. Unless I hear otherwise I'll merge in the morning. |
|
Can someone explain why we are deprecating the WithNumbers names instead of 1) Using the existing class name or 2) Making a seperate set of interfaces for Serialization? |
|
Because both Numbers and PEM/DER are Serialization, and if you can do one On Sun, Mar 1, 2015 at 7:44 AM, Alex Stapleton notifications@github.com
"I disapprove of what you say, but I will defend to the death your right to |
|
Until we commit to depending on pyasn1 that doesnt quite seem accurate to me, not that I have much of a problem with that idea :) In any case I don't think "WithSerialization" is particularly a more descriptive class name than "WithNumbers". Would seem preferable to avoid deprecating yet another thing. Am I missing something? |
|
@public I do find WithSerialization more accurate describes what we're doing with this interface, and I don't think many people actually type this class name, so it's not super disruptive. Are you -1 on this, or are you ok with me merging? |
Support for traditional OpenSSL and PKCS8 RSA private key serialization
|
While this has been merged I should note that we do have a hard dependency on pyasn1. |
This temporarily uses the backend to do it until the alternate approach is ready. DSA/ECDSA and public key support will follow if this is deemed worthy.