I checked whether we could use rsa::RsaPrivateKey::from_pkcs1_pem here to remove more code, but the pkcs1 crate does not support parsing the legacy Encrypted PEM format, which we need to do here in order to tell users that it is unsupported. So this is the correct change.

More precisely, rsa::RsaPrivateKey::from_pkcs1_pem uses the pem-rfc7468 crate, and RFC 7468 specifically excludes the header portion:

   Unlike legacy PEM encoding [RFC1421], OpenPGP ASCII armor, and the
   OpenSSH key file format, textual encoding does *not* define or permit
   headers to be encoded alongside the data.  Empty space can appear
   between the pre-encapsulation boundary and the base64, but generators
   SHOULD NOT emit such any such spacing.  (The provision for this empty
   area is a throwback to PEM, which defined an "encapsulated header
   portion".)

We are specifically accepting the OpenSSH key file format here, so we need to parse the PEM ourselves.

Opened RustCrypto/formats#112 upstream to clarify documentation.

Yeah, I added RustCrypto/formats#13 Error::HeaderDisallowed with exactly this use case in mind. But I take your point here RustCrypto/formats#112 (comment) that you would still like to verify that there is not just some header, but the exact header that indicates it’s a legacy encrypted PKCS#1 RSA private key.

If you recall you also added decode_label to parse the label from the PEM encapsulation boundary https://docs.rs/pem-rfc7468/0.2.3/pem_rfc7468/fn.decode_label.html

Maybe it would be interesting to add a higher-level parser type which could retain the context of what it was successfully able to parse. It already exists internally in the crate as the Encapsulation struct

Yes, I suppose the question to @str4d is… how assured do you want to be about the validity of the unsupported key file?

If you parse the PEM label with pem-rfc7468 and you get ENCRYPTED RSA PRIVATE KEY is that sufficient to indicate to the user that this is an unsupported key file? Or do you also want to confirm not just the presence of the header but also its specifics?

If you parse the PEM label with pem-rfc7468 and you get ENCRYPTED RSA PRIVATE KEY is that sufficient to indicate to the user that this is an unsupported key file?

No, because ENCRYPTED RSA PRIVATE KEY is not what is used as the label in practice; you instead get the same PEM label as for unencrypted, but with the additional header. Here's what I get from openssl 1.1.1f:

$ openssl genrsa -out example.key 2048
Generating RSA private key, 2048 bit long modulus (2 primes)
...............+++++
....................+++++
e is 65537 (0x010001)
$ cat example.key | head -n 2
-----BEGIN RSA PRIVATE KEY-----
MIIEogIBAAKCAQEA9WDz0dl/aYSgxHso3rt9c1ljZ1qvmOUOCa1oyX0dAX3YQqlF
$ openssl rsa -des3 -in example.key -out protected.key
writing RSA key
Enter PEM pass phrase:
Verifying - Enter PEM pass phrase:
$ cat protected.key | head -n 5
-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
DEK-Info: DES-EDE3-CBC,9BC872773CF8C93B

U8d2d6DqZNX20UyrOr5fqc+eJCaOzj8G8fPm4gdmvESVzAW95BNQpRHodAEW6Nx5

I want to continue to distinguish three cases, as I do in the current parser:

• Data has no PEM header.
  • Great, we carry on parsing, and maybe return a supported key!
• Data has a PEM header matching the structure of the encrypted PEM header.
  • We stop parsing and return Identity::Unsupported(UnsupportedKey::EncryptedPem).
• Data has an unknown PEM header.
  • We return a parser error.

Per my comment in RustCrypto/formats#112 (comment), I think how we could achieve this is to match on the pem_rfc7468::Error::HeaderDisallowed error, and in that case we re-parse the data ourselves with the following small parser:

preceded(
    tuple((
        tag("-----BEGIN "),
        tag(pem_label),
        tag("-----"),
        line_ending,
    )),
    terminated(rsa_pem_encryption_header, line_ending),
)

This would result in the PEM header being parsed three separate times (once to get Error::HeaderDisallowed, once to get pem_label, once to detect the encryption header), but this would only happen in the error case where a header exists, so it shouldn't add any parsing cost to the hot path.

We could potentially add support for the encrypted PEM format format in the pkcs1 crate. It would likely require a custom PEM parser since the format isn't RFC7468-friendly, but that isn't the end of the world.

I'm going to leave this code as-is for now.