Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.29%. Comparing base (08bc10d) to head (1326247).

@@           Coverage Diff           @@
##             main    #1954    /-   ##
=======================================
  Coverage   94.28%   94.29%           
=======================================
  Files          97       97           
  Lines       21818    21841    23     
=======================================
  Hits        20572    20595    23     
  Misses       1246     1246           

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

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

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.

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.

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?

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, 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).

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?

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.

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

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.

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!

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

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?

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.

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.

@lvkv Apologies, I think we lost some of the earlier momentum we had going here. Have you become busy (very reasonable!) or are there remaining points of uncertainty we should try and hash out to unblock progress?

Thanks for checking in. I'll be much more available after a few days! (Apartment hunting moving in NYC)

Coming back to this now! It's not clear to me, based on the discussion above, that there's a consensus yet on what the API for this should look like and why 🙂.

One idea to unstick the conversation: how do we feel about merging what we have right now (after cleanup)? I've also just added a commit to bring code coverage up to 100%. The plan would be:

• Clean up and land this PR with its current API
• Begin working on public_key implementations
• (Optional) Revise the API as needed for the next minor release

Edit: typo

Benchmark results

Instruction counts

Significant differences

There are no significant instruction count differences

Other differences

Wall-time

Significant differences

⚠️ There are significant wall-time differences

Other differences

Additional information

Historical results

Checkout details:

• Base repo: https://github.com/rustls/rustls.git
• Base branch: main (08bc10d)
• Candidate repo: https://github.com/lvkv/rustls.git
• Candidate branch: lvkv-verify-cert-key (1326247)

Cool, I'll add these in today

Home stretch! This one felt a bit slow, so I'm down to increase the pace for whatever work follows this PR