PEM format quickfix #54 #57
Conversation
This quickfix changes the PEM format from PKSC8 to PKSC5 (TraditionalOpenSSL) in pyca_crypto's variant of `create_rsa_encrypted_pem`. PKSC5 has the PEM headers expected by other PEM parsing functions, e.g. `is_pem_private` and `extract_pem`. See secure-systems-lab#54 for more details
Test the output of `create_rsa_encrypted_pem` in conjunction with PEM parsing functions `extract_pem` and `is_private_pem`.
securesystemslib/pyca_crypto_keys.py
Outdated
| @@ -540,7 +540,7 @@ def create_rsa_encrypted_pem(private_key, passphrase): | |||
|
|
|||
| encrypted_pem = \ | |||
| private_key.private_bytes(encoding=serialization.Encoding.PEM, | |||
| format=serialization.PrivateFormat.PKCS8, | |||
| format=serialization.PrivateFormat.TraditionalOpenSSL, | |||
There was a problem hiding this comment.
The docstring for this function should be updated, not only to mention the new PEM format but to correct it. The docstring incorrectly references PyCrypto and 3DES, etc. (this appears to be a copy+paste that wasn't edited).
tests/test_keys.py
Outdated
| # Test encrypted private pem | ||
| private_pem_encrypted = KEYS.create_rsa_encrypted_pem(private_pem, "pw") | ||
| private_pem_encrypted = KEYS.extract_pem(private_pem_encrypted, | ||
| private_pem=True) |
There was a problem hiding this comment.
I know that there are still many instances of this style of indentation in the codebase, but we should try to use the recently added indentation rules of the code style guidelines from now on.
Here is what the guidelines says about indentation for this particular case:
Make sure to indent the continued line appropriately; double-indentation (4 spaces) is prefered for line continuation, to distinguish it from functional indentation.
Correct me if I'm wrong, but this line should look as follows:
KEYS.extract_pem(private_pem_encrypted,
private_pem=True)
tests/test_keys.py
Outdated
| @@ -737,9 +745,11 @@ def test_is_pem_private(self): | |||
| # Test for a valid PEM string. | |||
| private_pem = self.rsakey_dict['keyval']['private'] | |||
| private_pem_ec = self.ecdsakey_dict['keyval']['private'] | |||
| private_pem_encrypted = KEYS.create_rsa_encrypted_pem(private_pem, "pw") | |||
There was a problem hiding this comment.
Maybe name it private_pem_encrypted_rsa to make it clear that it's an RSA PEM, similar to private_pem_ec?
securesystemslib/pyca_crypto_keys.py
Outdated
| is used to strengthen 'passphrase'. | ||
| Return a string in PEM format (TraditionalOpenSSL), where the | ||
| private part of the RSA key is encrypted using the best available | ||
| encryption for a given key’s backend. This is a curated encryption choice |
There was a problem hiding this comment.
A non-ASCII character is used on this line, but these modules don't declare a source code encoding. Probably easiest not to remove it.
As @vladimir-v-diaz points out, comments and docstrings for securesystemslib/pyca_crypto_keys.py's `create_rsa_encrypted_pem` are outdated. This commit updates docstring, comments and some indentation guideline mismatches.
Following @vladimir-v-diaz's review suggestions, this commit fixes some variable names and indentations in `test_keys.py`
lukpueh
force-pushed
the
pem-format-quickfix
branch
from
a6b9877 to
59f5876
Compare
|
Oops, removed non-ascii character, fixed wrong variable, rebased and force-pushed. Should be fine now. |
This quickfix changes the PEM format from PKSC8 to PKSC5 (TraditionalOpenSSL) in pyca_crypto's variant of
create_rsa_encrypted_pem.PKSC5 has the PEM headers expected by other PEM parsing functions, e.g.
is_pem_privateandextract_pem, which would break without this fix. The PR also updates the unittests.However, we should consider a smarter solution as discussed in #54.