PEM parsing is broken

[lukpueh]

High-level problem
We currently have an incomplete view on PEM formats. Several functions in the modules pycrypto_keys, pyca_crypto_keys, ecdsa_keys and keys expect PEM header and footer to be one of:

-----BEGIN PUBLIC KEY----- ... -----END PUBLIC KEY-----
-----BEGIN RSA PUBLIC KEY----- ... -----END RSA PUBLIC KEY-----
-----BEGIN RSA PRIVATE KEY----- ... -----END RSA PRIVATE KEY-----
-----BEGIN EC PRIVATE KEY----- ... -----END EC PRIVATE KEY-----

However, there are many values for VALUE in -----BEGIN {VALUE} ... -----END {VALUE}-----, as can be seen e.g. in openssl's pem.h.

Breaking example
Securesystemlib's default crypto library cryptography has changed the PEM header for encrypted RSA private keys from PKSC#5 to PKSC#8 (without our noticing):

# PKCS#5 header example
-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
DEK-Info: DES-EDE3-CBC,7D3EC0DB6DFBB404 ....

# PKCS#8 header example
-----BEGIN ENCRYPTED PRIVATE KEY----- ....

As a consequence a newly created encrypted RSA private key is not recognized as such anymore.

from securesystemslib import keys
rsa_key = keys.generate_rsa_key()
rsa_private = rsa_key["keyval"]["private"]
private_pem = keys.create_rsa_encrypted_pem(rsa_private, "1234")
keys.is_pem_private(private_pem)
# Returns `False` (should be `True`)

Note: The unit tests use a key that was generated before the update and therefor pass.

Solutions

1. Update our PEM parsing functions to account for all formats
   Pro: Quick/Easy fix
   Con: IMHO bad documentation of PEM formats, seems like a shortcut that can easily break

2. Remove PEM parsing functions in securesystemslib and rely on crypto libraries
   Pro: Seems like a more stable approach
   Con: We support 2 crypto libraries (cryptography and pycrypto) and 3 key types (rsa, ecdsa, ed25519). And the affected functions are currently used to decide which crypto library functions should be called. Hence, this fix would require several larger changes.

Despite all, I would prefer solution (2).

Further readings
Below are random sources about PEM, PKCS5 and PKCS8. I didn't find the related RFC's very helpful, but I'll probably have to dig deeper:

• PKCS#8 on wikipedia
• More info on PKCS#5 and 8
• Interesting discussion about PKCS in pyca
• pycrypto PEM loading
• cryptography PEM loading
• PEM formats in openssl

[SantiagoTorres]

++ on 2)

[awwad]

Even if we did 2) (which seems to me, at first, the more attractive solution), it can still cause keys we use to break when the libraries are updated. And code will still break if we write code to parse all PEM formats and there are any changes to what PEM formats libraries support.

In 1), 2), and the status quo, changes to the crypto libraries we use can result in causing parsing of either new or old keys to break (depending on the chosen scenario).

The highest level solution to this problem is dependency pinning (which we use already) and good testing. If there were a test that generated new keys and then used them, then when the dependency pinnings were updated, the tests would have failed, and that update would never have been merged into develop, and we wouldn't be here.

In all cases, we need testing for the key formats we support, so we may as well start with that, I suppose. Beyond that, I prefer 2) if it's readily feasible, and if it's not readily feasible, then either 1) or, I suppose, neither (and make it explicit somewhere what PEM formats we support, and add support for the new one).

[lukpueh]

@awwad, thanks for your take on this. I definitely agree with you on better testing and dependency pinning. Let me try to figure out how much effort approach (2) is.

[SantiagoTorres]

Even if we did 2) (which seems to me, at first, the more attractive solution), it can still cause keys we use to break when the libraries are updated

Although I agree in principle, I don't think this would be a scenario we would run in real life. The key formats are standardized, and the underlying library developers have a (likely bigger) group of people providing support for these formats. At the end of the day, this issue boils down to us trying to reinvent the wheel at the moment of parsing PEM files and thus adding a new layer of code from which bugs can emerge.

If SSLib is an easy-to-use crypto frontend to different (pluggable) cryptographic libraries it should remain like that. I think we need to have a clear focus on what is the goal of SSLib and not deviate from it.

[lukpueh]

Btw. I found out what happened.

The problem was not that pyca's cryptography had mysteriously changed its PEM format, nor was it the way we were calling pyca's crytpography.

The problem was that, until recently, we never actually called pyca's cryptography to create encrypted RSA PEMs.

This actually dates back to when securesystemslib was still part of theupdateframework.

A quickfix would be to change the format parameter so that pyca's cryptography generates encrypted RSA PEM's the way pycrypto used to do it:

diff --git securesystemslib/pyca_crypto_keys.py securesystemslib/pyca_crypto_keys.py
index ceef7c0..291c027 100755
--- securesystemslib/pyca_crypto_keys.py
+++ securesystemslib/pyca_crypto_keys.py
@@ -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,
     encryption_algorithm=serialization.BestAvailableEncryption(passphrase.encode('utf-8')))
 
   return encrypted_pem.decode()

[SantiagoTorres]

The problem was that, until recently, we never actually called pyca's cryptography to create encrypted RSA PEMs.

Makes me wonder if we should have two test suites with the different crypto providers in isolation

[lukpueh]

Makes me wonder if we really benefit from supporting two libraries.

[lukpueh]

While we're at it, we might want to reconsider the names of some of the affected functions:
create_rsa_encrypted_pem --> e.g.: create_encrypted_rsa_pem or encrypt_rsa_pem
extract_pem --> strip_pem

[lukpueh]

Re isolated test suites:
Currently some of the unittests use constructs like:

for general_crypto_library in ['pycrypto', 'pyca-cryptography']:
  KEYS._GENERAL_CRYPTO_LIBRARY = general_crypto_library
  # Test routes
  ...

Maybe we can remove those and run all the tests for each setting via the test aggregator script? Provided that we still want to support interchangeable crypto libraries. (see #56)

[lukpueh]

Closing as outdated.