-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
EndEntityCert
type 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_key
function supports the following private key algorithms and encodings (matching the supported formats used by Rustls):Key algorithms:
Encodings: