Add function to verify CertifiedKey consistency

[lvkv]

Using the subject_public_key_info() function we added in rustls/webpki#253, this PR:

• Adds a function to trait SigningKey that returns its SubjectPublicKeyInfo, but only if the implementer opts in
• Adds a function to CertifiedKey that verifies the consistency of its public and private keys by comparing the SPKI values of its end-entity cert and its key

The motivation behind this is to make it possible to verify consistency for public and private keys (#1918).

[codecov]

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 94.07%. Comparing base (0c85c01) to head (8cce4de).
Report is 4 commits behind head on main.

Files	Patch %	Lines
rustls/src/crypto/signer.rs	71.42%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1954      +/-   ##
==========================================
- Coverage   94.09%   94.07%   -0.02%     
==========================================
  Files          96       96              
  Lines       20597    20614      +17     
==========================================
+ Hits        19380    19393      +13     
- Misses       1217     1221       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cpu]

Realized I was reviewing at the same time as Ctz so I submitted my partial review :-) I agree with his feedback.

[cpu]

Some (hopefully helpful) pointers for the failed CI tasks:

• Check for documentation errors: I think you need to use crate:: instead of rustls:: in the rustdoc for UnimplementedSubjectPublicKeyInfo (assuming that error type doesn't fall away based on other feedback)
• Check for minimum versions of direct dependencies - I think you need to bump the pki-types version alongside the webpki update (something like cpu@0eb3df6). (Note that Add support for enforcing CRL expiration #1922 might beat you to these updates :-))
• Clippy - The fuzz crate has a separate Cargo.lock that needs updating. You should be able to cd into fuzz, run cargo build and then commit the updated lockfile.
• Clippy (nightly) - ditto above.
• Validate external types appearing in public API - this will be resolved with ctz's suggestion.

[lvkv]

I'm converting this to a "ready for review" PR, as it's less of a draft now. Questions:

• It looks like we've agreed to return Ok(None) by default for SigningKey::public_key. This looks sensible, though it means that certificate_and_key_are_consistent will (as this PR is written) always return Error::InvalidCertificate(CertificateError::BadEncoding), since 1. it interprets the absence of a SPKI as an invalid cert and 2. nobody has overridden the default behavior of certificate_and_key_are_consistent yet. I assume the solution is to update the SigningKey implementers in additional PR(s) before putting this current change in a release?
• @djc / @cpu suggested that we updated the return type of SigningKey::public_key from Result<Option<Vec<u8>>, Error> to Result<Option<SubjectPublicKeyInfoDer>, Error> over at Add function to verify CertifiedKey consistency #1954 (comment). Does this count as an external type? I'm still poking cargo check-external-types, which I haven't gotten to work locally yet 🙂

Also: I can tidy up the commits like we did over at rustls/webpki#253. I'll wait for reviews to settle first, though.

[cpu]

It looks like we've agreed to return Ok(None) by default for SigningKey::public_key. This looks sensible, though it means that certificate_and_key_are_consistent will (as this PR is written) always return Error::InvalidCertificate(CertificateError::BadEncoding), since 1. it interprets the absence of a SPKI as an invalid cert and 2. nobody has overridden the default behavior of certificate_and_key_are_consistent yet. I assume the solution is to update the SigningKey implementers in additional PR(s) before putting this current change in a release?

Hmm, yeah, it does seem a little off that if SigningKey::public_key's default impl isn't overridden the new API produces a BadEncoding error. I could see that being confusing. I suggested the bad encoding error thinking it was used only when the SPKI was missing from a cert, but didn't think about the case where the SigningKey wasn't able to provide the information.

Perhaps it would be better to formulate certificate_and_key_are_consistent() (or whatever its name ends up being) to be infallible. Instead of returning a Result<bool>, maybe we could return an enum with variants like Consistent, Inconsistent, Unknown. We'd return Unknown if the SPKI can't be produced for either the key or the cert, Inconsistent if we compared SPKIs and they weren't byte-for-byte identical, and Consistent if they were.

My thinking is that even when we implement public_key() for the aws-lc-rs and ring signing keys there is still the possibility of external implementations of the trait not having caught up yet. I think treating that as an error instead of a variant like Unknown might be too heavy-weight given I see this more as a helper API than load-bearing. WDYT?

[lvkv]

Perhaps it would be better to formulate certificate_and_key_are_consistent() (or whatever its name ends up being) to be infallible. Instead of returning a Result, maybe we could return an enum with variants like Consistent, Inconsistent, Unknown. We'd return Unknown if the SPKI can't be produced for either the key or the cert, Inconsistent if we compared SPKIs and they weren't byte-for-byte identical, and Consistent if they were.

@cpu, I took a stab at rewriting the keys_match() function with this in mind. The "unknown" in particular feels a bit goofy to me, like Rustls is a magic 8 ball that wants me to "ask again later" (i.e. when we've gone around and implemented public_key on all the SigningKeys) 🙂

As a dev, I'm unsure if I'd ever handle Unknown differently from Inconsistent differently from Error. I basically only ever want Consistent, and anything else indicates misconfiguration—resulting in either a fast failure, alert or both for whatever system I'm developing.

My thinking is that even when we implement public_key() for the aws-lc-rs and ring signing keys there is still the possibility of external implementations of the trait not having caught up yet. I think treating that as an error instead of a variant like Unknown might be too heavy-weight given I see this more as a helper API than load-bearing. WDYT?

I think the design should reflect whether or not this check will happen by default during the creation of a CertifiedKey (going off of #1918 (comment)). Open to opinions!

[cpu]

@cpu, I took a stab at rewriting the keys_match() function with this in mind

Thanks! That looks close to what I was thinking, but I was imagining it wouldn't be a Result<KeyConsistency, Error> return, just KeyConsistency.

The "unknown" in particular feels a bit goofy to me, like Rustls is a magic 8 ball that wants me to "ask again later" (i.e. when we've gone around and implemented public_key on all the SigningKeys)
As a dev, I'm unsure if I'd ever handle Unknown differently from Inconsistent differently from Error. I basically only ever want Consistent, and anything else indicates misconfiguration—resulting in either a fast failure, alert or both for whatever system I'm developing.

Yeah :-/ It is awkward throwing a third variant in the mix.

If folks don't think my suggestion is a net improvement I'm not tied to it and open to alternatives. My main concern was avoiding a bad encoding error for the case where SigningKey::public_key() returns Ok(None). If that's fixed another way I'm also satisfied.

Maybe if the principle call-site we care about using that fn is going to turn it into an error anyway the default impl should be Err(General("unimplemented".into())) (bikeshedding the details as appropriate) instead of Ok(None).

[djc]

From #1918:

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.

Is this actually what we're doing? Currently this PR actually just does a byte-for-byte comparison of the SPKI data, right?

[djc]

Perhaps it would be better to formulate certificate_and_key_are_consistent() (or whatever its name ends up being) to be infallible. Instead of returning a Result<bool>, maybe we could return an enum with variants like Consistent, Inconsistent, Unknown. We'd return Unknown if the SPKI can't be produced for either the key or the cert, Inconsistent if we compared SPKIs and they weren't byte-for-byte identical, and Consistent if they were.

I think the core question is the intended behavior for crypto providers that don't implement this. I think in rustls 0.24 we might want to tighten this up to force crypto provider implementers to support this? If that's the case, I think probably it'd make sense to just yield Result<(), Error> and have the default impl yield Ok(()) for now.

[cpu]

Is this actually what we're doing?

Is that your preference? Without strong signal it seems better to follow the plan outlined by Ctz unless there's consensus it isn't the right approach. When that comment was posted I didn't understand the advantages (I'm still not sure I do with crystal clarity). I asked in the Discord thread and Ctz said:

yes, i think he's saying that crypto libraries asked for the public half of a key pair could fulfil the request from a public key they read in (alongside the private key; for example RSA keys in PKCS#1 format) rather than one they computed. i think that seems like a harder situation for a user to get in though, and likely a different problem

[djc]

Is this actually what we're doing?

Is that your preference? Without strong signal it seems better to follow the plan outlined by Ctz unless there's consensus it isn't the right approach. When that comment was posted I didn't understand the advantages (I'm still not sure I do with crystal clarity).

Ahh, sorry for potentially derailing this discussion from an agreed upon plan. To be clear, doing this is definitely better than doing nothing. I just think "doing SPKI bytes comparison" is probably a weaker guarantee than if we could ask the crypto provider to do curve/group/whatever magic math thing to check that the keys are actually compatible.

[lvkv]

My main concern was avoiding a bad encoding error for the case where SigningKey::public_key() returns Ok(None). If that's fixed another way I'm also satisfied.

Agree. @cpu How do you guys feel about the following options to avoid this? Ordered from least to most drastic:

1. Option 1: Change the default implementation of SigningKey::public_key to something along the lines of err(Error::Unimplemented). This would allow us to disambiguate None from unimplemented when implementing CertifiedKey::keys_match and, in turn, avoid us mistaking "unimplemented" for "bad encoding".

2. Option 2: Remove the default implementation of SigningKey::public_key altogether and frontload the work of implementing it for types that impl SigningKey. This avoids the "unimplemented" case altogether.

3. Option 3: All of Option 2, plus removing the Option from the return type of SigningKey::public_key, taking it from Result<Option<SubjectPublicKeyInfoDer>, Error> to Result<SubjectPublicKeyInfoDer, Error>. This would eliminate both the "unimplemented" problem, but also the "unknown" case when one of the SPKIs is None.

Also open to suggestions!

[cpu]

@lvkv From my perspective I think Option 1 is probably the most agreeable. Option 2 and 3 will be breaking API changes to the exported SigningKey trait and we're trying to avoid those while the ecosystem is catching up w/ our last release.

[djc]

Option 1 sounds good, maybe we should file an issue with a label to remind ourselves to remove the default impl for the next semver-breaking release?

[ctz]

I was thinking originally we could call the new consistency API CertifiedKey::keys_match in several places that accept CertifiedKeys and are already fallible. There are only a few such places, but doing that unprompted requires either a) we don't treat missing SigningKey::public_key as an error, or b) require SigningKey::public_key as a fundamental part of that trait.

(a) means we can add those calls, and land this PR before writing the wedge of encoding conversion code that would be needed for the impls of SigningKey just in this crate.

(b) I think is a tough sell, needing a lot of research. For example, can you get a public key given a windows NCRYPT_KEY_HANDLE? I have no idea, but it seems not, and the suggestion is to look up the certificate and extract the public key from that (which would be quite circular...)

The other option, of course, is that we don't insert those calls, and leave the API for others to call when they know or expect their SigningKey to support getting the public key.

[djc]

Right, so I think this probably is the right direction still:

I think the core question is the intended behavior for crypto providers that don't implement this. I think in rustls 0.24 we might want to tighten this up to force crypto provider implementers to support this? If that's the case, I think probably it'd make sense to just yield Result<(), Error> and have the default impl yield Ok(()) for now.