[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!

[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.

[jgreeer]

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.

@cpu bumping so this is on your radar 😄

[cpu]

@cpu bumping so this is on your radar 😄

Thanks for the ping! I have to admit I forgot about this one 😅 I'll take a look shortly.

[cpu]

Here's a first pass of review. Sorry for the delay!

I think this is 90% of the way where we want, but I think there might still be something to pull at w.r.t the SignatureAlgorithm and SigningKey trait and whether it's possible to represent a valid case like a P-256 key signing messages using ECDSA/SHA-384 like the new openssl.rs test. In that case the signature algorithm identifier is ecdsca-with-SHA384 and names no specific curve, and the public key algorithm is ECDSA_P256.

If I were to implement a SigningKey for that scenario, I can correctly return key_alg::ECDSA_P256 from key_algorithm() but I think I'm stuck for an ECDSA_SHA384 signature constant. I'd have to use ECDSA_P384_SHA384 and that's claiming a curve that doesn't match the public key alg. It feels like we're still conflating the two.

I think we need to be looking at something more akin to the internal x509 logic in Go's signingParamsForKey? Does that make sense?

[cpu]

nit: might read a little clearer as something like:

The public key algorithm associated with public keys used for this signature algorithm.

[jgreeer]

tried to fix, but i think this depdends on what we do with signature algorithm

[cpu]

I think the way this worked previously we called from_oid(), then searched this algorithm list for a match, and returned UnsupportedSignatureAlgorithm for RSA-PSS after I removed them in f8ef6ee

We probably want to continue to reject RSA-PSS on this codepath right?

[jgreeer]

yep, good catch. worth noting since csr.verify_signature() was called before from_oid the error you'd get was InvalidCertificationRequestSignature instead of UnsupportedSignatureAlgorithm because RSA-PSS is not supported by verify_signature(). But since sha1 was supported by verify_signature() it would fall through to from_oid and give the correct error. I added supports_oid() to SignatureAlgorithm which happens before signature verifcation, so now both get the same error.

[jgreeer]

Here's a first pass of review. Sorry for the delay!

I think this is 90% of the way where we want, but I think there might still be something to pull at w.r.t the SignatureAlgorithm and SigningKey trait and whether it's possible to represent a valid case like a P-256 key signing messages using ECDSA/SHA-384 like the new openssl.rs test. In that case the signature algorithm identifier is ecdsca-with-SHA384 and names no specific curve, and the public key algorithm is ECDSA_P256.

If I were to implement a SigningKey for that scenario, I can correctly return key_alg::ECDSA_P256 from key_algorithm() but I think I'm stuck for an ECDSA_SHA384 signature constant. I'd have to use ECDSA_P384_SHA384 and that's claiming a curve that doesn't match the public key alg. It feels like we're still conflating the two.

I think we need to be looking at something more akin to the internal x509 logic in Go's signingParamsForKey? Does that make sense?

Hey, thanks for the review! That's a good point, should we drop the curve from SignatureAlgorithm then? That would align the struct closer to go and what is actually written into the cert. Certificate.signatureAlgorithm and tbsCertificate.signature are both ecdsa-with-SHA384, no curve. I assume this is what we want to take away from signingParamsForKey because they also allow you to decide the signatureAlgorithm at sign time, which is not something we want to do if i understand correctly.

I think the problem is that SignatureAlgorithim is overloaded, since it's also used as the argument to generate a key. You'd probably have to make a new struct that serves as an argument to generate. The scope of this PR would increase a bit. Am I understanding this correctly or do you have a smaller change in mind? Thanks!

[cpu]

Sorry about the lag 😓

I think the problem is that SignatureAlgorithim is overloaded, since it's also used as the argument to generate a key. You'd probably have to make a new struct that serves as an argument to generate. The scope of this PR would increase a bit. Am I understanding this correctly or do you have a smaller change in mind? Thanks!

Yeah, exactly. I think we want to break that overloading. Increasing the scope of the PR to achieve that seems reasonable to me. I'd rather do this properly in one go vs continue to try and mix concepts up across pub key identification, key generation, and signature alg identification/encoding.

[jgreeer]

Summary of changes:

• Removed key_alg and sign_alg fields from SignatureAlgorithm
  • key_alg: decoupling keyalg from sigalg so we don't have to infer a curve when reading a sigalg
  • sign_alg: it held ring/aws-lc-rs types; removing it makes SignatureAlgorithm independent of the backend, so the types can be ungated and reused by future backends
• Created a new public sig_alg module that has the curve free signature algorithms
• Added signing_alg() as a translation layer in key_pair.rs to the specific backend types; anything a backend doesn't support (previously a panic! in the from_* loaders) now returns UnsupportedSignatureAlgorithm
• Created a new struct: SigningKeyAlgorithm, which is a pairing of a SignatureAlgorithm and a PublicKeyAlgorithm.
  • This struct essentially takes the place of SignatureAlgorithm in key_pair.rs, most importantly the 12 top-level constants are now this type.
  • Ungated algo::ECDSA_P521_* and algo::ML_DSA_* so that future crypto backends can use these types
    • On ring you can now use them as generation arguments, but the translation layer will fail it with UnsupportedSignatureAlgorithm
• Since SignatureAlgorithm changed KeyPair::algorithm() now returns the curve-free signature algorithm
• Removed PublicKey from csr.rs since we have SubjectPublicKeyInfo

Tests:
- Added an openssl test test_openssl_p256_key_signing_with_sha384 in openssl.rs for the situation @cpu described
- Added an unsupported algorithm test for ring

I've added five commits on top of the existing 6 to describe these changes, some of them undo work from the previous commits (like removing key_alg from SignatureAlgorithm. I've left it this way for now so it's easier to review, but i can squash them into a smaller amount of commits once reviewed.

[djc]

I've added five commits on top of the existing 6 to describe these changes, some of them undo work from the previous commits (like removing key_alg from SignatureAlgorithm. I've left it this way for now so it's easier to review, but i can squash them into a smaller amount of commits once reviewed.

Suggest to squash them now, since that will be easier to review.

[jgreeer]

I've added five commits on top of the existing 6 to describe these changes, some of them undo work from the previous commits (like removing key_alg from SignatureAlgorithm. I've left it this way for now so it's easier to review, but i can squash them into a smaller amount of commits once reviewed.

Suggest to squash them now, since that will be easier to review.

Sure, squashed it back to 5 commits.