end_entity: add verify_private_key function. #67
Conversation
Codecov Report
@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 95.34% 95.45% +0.11%
==========================================
Files 13 13
Lines 2662 2819 +157
==========================================
+ Hits 2538 2691 +153
- Misses 124 128 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@djc I think this is ready for a review pass/your input when you have a chance. Thanks! |
|
This commit introduces a function on the `EndEntityCert` type that can be used to check if a DER encoded private key matches the certificate's subject public key. This is done by un-marshalling the encoded subject public key information of the EE certificate and performing a byte-for-byte comparison of the encoded public key with the encoded public key derived from the encoded private key. If the two match, we consider the private key usable for the certificate. Care is taken to match the algorithms and encoding formats supported by Rustls.
| extractor: impl Fn(&[u8]) -> Option<K>, | ||
| cert_pub_key: &[u8], | ||
| ) -> Option<Result<(), Error>> { | ||
| match extractor(private_key_der) { |
There was a problem hiding this comment.
You can replace the outer match with let key_pair = extractor(private_key_der)?;. Also consider extracting the Some from the inner match arms.
| /// * PKCS8v2 (Ed25519 only) | ||
| /// * PKCS1 (RSA only) | ||
| /// * Sec1 (ECDSA only) | ||
| pub fn verify_private_key(&self, private_key_der: &[u8]) -> Result<(), Error> { |
There was a problem hiding this comment.
Thinking aloud: if we restate this API as "is this the right public key for the certificate?" then we'd get some good advantages:
- it would unduplicate all the private key handling from here,
- webpki as a crate would maintain its "never deals with secrets" property,
- the result would be more linker friendly (eg, a program that otherwise only uses RSA but uses this function ends up with ECDSA and ed25519 inside)
I guess that would be on the rustls side we'd add a fn public_key(&self) -> &[u8] to SigningKey and then do the key identity validation in cross_check_end_entity_cert?
There was a problem hiding this comment.
Those are compelling advantages 👍 In retrospect I was probably overfitting Brian's suggestion on Rustls#618 by jumping straight to this design.
| cert_pub_key: &[u8], | ||
| ) -> Option<Result<(), Error>> { | ||
| match extractor(private_key_der) { | ||
| Some(keypair) => match cert_pub_key == keypair.public_key().as_ref() { |
There was a problem hiding this comment.
I'm not sure this is completely correct, because neither cert_pub_key nor keypair.public_key() expresses the key type. This would mean we could compare as equal (say) a nistp256 and ed25519 key as equal if they had the same encoding, even though they semantically are never equal because they're are not in the same domain.
There was a problem hiding this comment.
That's a good point. We should also be asserting the key types match somewhere in this logic. I'll give this more attention in the reworked formulation. Thanks for calling this out as a potential pitfall.
|
I think given ctz's feedback above I'm going to close this PR and look at replacing it with a reworked version that doesn't have Thanks for the input! |
This branch introduces a function on the
EndEntityCerttype that can be used to check if a DER encoded private key matches the EE certificate's subject public key. This resolves the user request made in rustls/rustls#618.This is done by un-marshalling the encoded subject public key information (SPKI) of the EE certificate and performing a byte-for-byte comparison of the encoded SPKI public key with the encoded public key derived from the private key. If the two match, we consider the private key usable for the certificate.
The
verify_private_keyfunction supports the following private key algorithms and encodings (matching the supported formats used by Rustls):Key algorithms:
Encodings: