-
Notifications
You must be signed in to change notification settings - Fork 595
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
Add function to verify CertifiedKey consistency #1954
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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:
|
I'm converting this to a "ready for review" PR, as it's less of a draft now. Questions:
Also: I can tidy up the commits like we did over at rustls/webpki#253. I'll wait for reviews to settle first, though. |
Hmm, yeah, it does seem a little off that if Perhaps it would be better to formulate My thinking is that even when we implement |
@cpu, I took a stab at rewriting the As a dev, I'm unsure if I'd ever handle
I think the design should reflect whether or not this check will happen by default during the creation of a |
Thanks! That looks close to what I was thinking, but I was imagining it wouldn't be a
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 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 |
From #1918:
Is this actually what we're doing? Currently this PR actually just does a byte-for-byte comparison of the SPKI data, right? |
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 |
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:
|
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. |
Agree. @cpu How do you guys feel about the following options to avoid this? Ordered from least to most drastic:
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 |
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 (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 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 |
Right, so I think this probably is the right direction still:
|
@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) |
Using the
subject_public_key_info()
function we added in rustls/webpki#253, this PR:trait SigningKey
that returns itsSubjectPublicKeyInfo
, but only if the implementer opts inCertifiedKey
that verifies the consistency of its public and private keys by comparing the SPKI values of its end-entity cert and its keyThe motivation behind this is to make it possible to verify consistency for public and private keys (#1918).