add public key algorithm

[jgreeer]

Summary

This PR splits the SPKI public key identification of SignatureAlgorithm into its own separate type, PublicKeyAlgorithm. Bundling them together is fine for key generation, but it leads to us making incorrect assumptions when parsing. We also move closer to removing KeyPair for #450, since we move the signing algorithm from KeyPair to the SigningKey trait. This change is inspired by the crypto/x509 package in go.

No DER output changes for anything that worked before. write_alg_id is byte-identical to the old write_oids_sign_alg for every algorithm, which the unchanged openssl/webpki/botan verify-tests cover.

Changes

sign_algo.rs

• Added the PublicKeyAlgorithm struct with three methods: from_alg_id(), write_alg_id(), and iter()
• SignatureAlgorithm changes
  • Removed oids_sign_alg and replaced with key_alg, which holds a PublicKeyAlgorithm
  • Removed from_oid(), we've replaced the only usage in csr.rs. For ECDSA, some of the values have the same OIDs, so they were being shadowed. from_oid(ecdsa-with-SHA256) always returned ECDSA_P256_SHA256, so ECDSA_P521_SHA256 could never be returned at all.
• Added the key_alg module, which stores the static public key types
• Removed write_oids_sign_alg(), this is now done in write_alg_id()

key_pair.rs

• Removed algorithm() from KeyPair. It lives in the SigningKey trait as algorithm()
• PublicKeyData::algorithm() is renamed to key_algorithm() and now returns a PublicKeyAlgorithm instead of a SignatureAlgorithm. The names have to differ because SigningKey: PublicKeyData, so a shared algorithm() is ambiguous at the call site.
• The SubjectPublicKeyInfo struct now stores a PublicKeyAlgorithm, since it implements PublicKeyData
• from_der() in the SPKI struct now uses the from_alg_id method of PublicKeyAlgorithm
• serialize_public_key_der() now uses the write_alg_id method of PublicKeyAlgorithm

csr.rs

• The PublicKey structs alg field is now a PublicKeyAlgorithm instead of a SignatureAlgorithm
• In from_der(), we read the public key algorithm from the SPKI's AlgorithmIdentifier using the from_alg_id() method instead of using the csr's signature algorithm.
• Two side effects worth noting.
  • CSRs signed with sha1WithRSAEncryption or RSASSA-PSS now parse instead of erroring, since from_oid was acting as a second gate and x509-parser accepts both.
  • We now read the SPKI's algorithm parameters, which the old code ignored. A nonconforming RSA key with absent parameters instead of the required NULL is now rejected.
  • Not sure what we want to do about these, the first outcome seems unwanted, but I think the second is good.

crl.rs

• changed the doc example for the new trait shape

error.rs

• New Error::UnsupportedPublicKeyAlgorithm variant

rustls-cert-gen/src/cert.rs

• test imports SigningKey to reach algorithm()

lib.rs

• exports PublicKeyAlgorithm and key_alg; new algo_hash_agrees_with_eq test

verify-tests

• openssl.rs - Added a test for the issue CertificateSigningRequestParams::from_der can parse the wrong key type #448
• webpki.rs - Fixed its PublicKeyData implementation to return a PublicKeyAlgorithm

Fixes #448

[djc]

I think there's probably too much going on in the single commit here.

Can it be split into two or three commits? Maybe start by introducing the new algorithm type and replacing only one (the simplest) usage site, then do one commit per additional usage site?

@jgreeer do you think you can figure that out? There's a little more guidance on what we typically look for here.

[jgreeer]

I think there's probably too much going on in the single commit here.

Can it be split into two or three commits? Maybe start by introducing the new algorithm type and replacing only one (the simplest) usage site, then do one commit per additional usage site?

@jgreeer do you think you can figure that out? There's a little more guidance on what we typically look for here.

sure thing, I split the changes into 4 commits. let me know if we should split these changes further or not

[djc]

This is looking much better, but I'd like to get a second opinion from @cpu!

[djc]

Nit: since this calls iter(), it should be defined before it.

[cpu]

I'd like to get a second opinion from @cpu!

I'll be tied up at a conference for the next few days but this is on my list.