Skip to content
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

Verify that SigningKey matches public key within certificate #1918

Open
1 task done
lvkv opened this issue Apr 24, 2024 · 6 comments
Open
1 task done

Verify that SigningKey matches public key within certificate #1918

lvkv opened this issue Apr 24, 2024 · 6 comments

Comments

@lvkv
Copy link

lvkv commented Apr 24, 2024

👋 Hello!

Checklist

  • I've searched the issue tracker for similar requests

Apologies if this has already been asked and I didn't notice!

Is your feature request related to a problem? Please describe.

Right now, it looks like it's possible to create a CertifiedKey with inconsistent public and private keys, and there doesn't seem to be any functionality out of the box to check if this is the case.

Describe the solution you'd like

Ideally, I'm looking for a Rustls-compatible equivalent for OpenSSL's X509_check_private_key, which verifies that the given public and private keys are consistent 1:

DESCRIPTION
X509_check_private_key() function checks the consistency of private key pkey with the public key in cert.

This would be great even it functions in the same way that X509_check_private_key does:

BUGS
The X509_check_private_key() and X509_REQ_check_private_key() functions do not check if pkey itself is indeed a private key or not. They merely compare the public materials (e.g., exponent and modulus of an RSA key) and/or key parameters (e.g. EC params of an EC key) of a key pair. So they also return success if pkey is a matching public key.

Describe alternatives you've considered
I suppose I could write something myself that does some ASN.1/DER parsing. I feel like this is useful to have!
An external crate that has this functionality would also be great!

Additional context

Footnotes

  1. https://www.openssl.org/docs/manmaster/man3/X509_check_private_key.html

@cpu
Copy link
Member

cpu commented Apr 24, 2024

👋 Hi there, thanks for opening an issue

I think we're open to adding functionality for this. Previously (rustls/webpki#67) I had started implementing something similar in webpki but the conclusion at the time was that it'd be a better fit in rustls with a slightly different design. I haven't had time to come back around to that but it's been on my mind because we stubbed out X509_check_private_key with a TODO in some ongoing OpenSSL compatibility work.

Would you be interested in working on a PR?

@lvkv
Copy link
Author

lvkv commented May 1, 2024

Would you be interested in working on a PR?

Yeah! I still have a lot to learn in this space, though. Are you guys are okay with nurturing a newbie?

Naively, the interface I would want is one of:

  • A method on CertifiedKey that verifies consistency between the CertifiedKey's underlying end_entity_cert (of type CertificateDer) and key (Arc<dyn SigningKey>):

    /// I think this might not work, though. See the paragraph below.
    verify_private_key(&self) -> Result<(), Error>
  • Or maybe just a free function, like:

    /// Ditto-ish. See below also.
    verify_private_key(cert: &SomeCertType, key: &SomeKeyType) -> Result<(), Error>

Looking at rustls/webpki#67, it looks like one tricky part of this will be finding certificate and key types (types meaning Rust types) that are abstract, but not so much so that we lose distinctive information like cryptographic key types—which also must be consistent, aside from the actual public key bytes we'd like to compare.

For example, take the CertifiedKey function I pitched above. CertifiedKey's end_entity_cert() is CertificateDer, not webpki::EndEntityCert. But even webpki::EndEntityCert doesn't give us the SubjectPublicKeyInfo I assume we want—that would mean accessing its private inner cert::Cert. It also doesn't look like SigningKey gives us, well, anything here. That would also need to be figured out.

I'll have some contiguous time over the weekend to get started on this, along with a few hours here and there during the week. If you guys have any pointers in the meantime, I'd greatly appreciate it!

@ctz
Copy link
Member

ctz commented May 3, 2024

The pieces I had in mind for this were:

  • add a function to webpki::EndEntityCert that exposes the certificate's public key as a SPKI. Note that webpki's internal representation of an SPKI (ie Cert::spki) lacks the length prefix; this would need to be reconstituted for the encoding to be correct. We could possibly add this type to pki-types to give it a name?
  • add a function to rustls::sign::SigningKey::public_key that returns the public key, again in SPKI format. This should be optional, and have a default that opts-out in a distinctive way. Perhaps that is Result<Option<Vec<u8>>, Error>, or maybe we add a distinctive new Error. Both of those options seem alright.
  • add a function to CertifiedKey, naming TBD, which: a) parses end_entity_cert() to extract the certificate SPKI, b) gets the other SPKI from the private key, and c) errors in a distinctive way if they do not match.
  • call that new function from the various set_single_cert functions we have around the place.
  • see about providing public_key() for the various SigningKey impls we have (ring, aws-lc-rs, provider-example). A bit of research here to see what is possible in ring/aws-lc-rs's API.

@cpu
Copy link
Member

cpu commented May 3, 2024

Are you guys are okay with nurturing a newbie?

I would be happy to help you work through the above if you're still interested. You can find us in Discord in the #rustls room if you want to have a place to ask one-off questions.

Ctz's plan sounds good to me. WDYT about starting with the first bullet point and working up a pki-types and webpki branch? I think the last bullet might be the most involved, we can work towards it starting with some of the easier pieces.

@lvkv
Copy link
Author

lvkv commented May 4, 2024

Ctz's plan sounds good to me. WDYT about starting with the first bullet point and working up a pki-types and webpki branch? I think the last bullet might be the most involved, we can work towards it starting with some of the easier pieces.

This sounds great, and thank you! I'll drop a few questions in your Discord channel soon.

@briansmith
Copy link
Contributor

add a function to CertifiedKey, naming TBD, which: a) parses end_entity_cert() to extract the certificate SPKI, b) gets the other SPKI from the private key, and c) errors in a distinctive way if they do not match.

Perhaps stating the obvious: the crypto library may read in a (private key, public key) pair but never check that the privacy key is consistent with the public key. Then the crypto library might expose the potentially-mismatched public key to others. Many crypto libraries don't check pairwise consistency at all, and/or they offer options or separate APIs that optionally do it. Ideally Rustls would extract the SPKI from the EE certificate and then ask the crypto provider to do a pairwise consistency check as part of the construction of a CertifiedKey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants