Add new with extra roots on macos/ios #133

stormshield-gt · 2024-08-26T08:39:08Z

Add support of extra roots for the moment on macos and ios. I plan to add windows and android but I believe this is already ready for initial feedback.
The work is mostly based on @complexspaces branch https://github.com/rustls/rustls-platform-verifier/tree/shared/extra-roots-additions.

I've changed the signature of new_with_extra_roots (breaking) to be uniform on all platforms.

Partially fix #58

cpu

Thanks for picking this up! 💕 Here's some initial feedback.

rustls-platform-verifier/src/tests/verification_mock/mod.rs

rustls-platform-verifier/src/verification/apple.rs

rustls-platform-verifier/Cargo.toml

rustls-platform-verifier/src/tests/verification_mock/mod.rs

rustls-platform-verifier/src/verification/others.rs

complexspaces · 2024-08-26T19:22:03Z

As a Git nit, would you mind adding me as a Co-authored-by: to 1bb8c1c since its based off my original branch?

I plan to add windows and android

I planned on working on overhauling the Android verifier this week since it has a lot of outstanding problems as-is, including not supporting this pattern very cleanly. Maybe we could split the Windows and Android work?

cpu · 2024-08-26T19:25:26Z

Maybe we could split the Windows and Android work?

That sounds like a good idea to me 👍

stormshield-gt · 2024-08-27T06:36:00Z

As a Git nit, would you mind adding me as a Co-authored-by: to 1bb8c1c since its based off my original branch?

Sure ! For what it's worth, I've already done it but it seems the change has been reverted, most likely because of a rebase

I planned on working on overhauling the Android verifier this week since it has a lot of outstanding problems as-is, including not supporting this pattern very cleanly. Maybe we could split the Windows and Android work?

All right, I will look into the windows implementation then. Maybe I will just do a rebase over #131 to start cleanly.

cpu

Thanks! This looks good to me. Did you want to keep it as a draft until #131 is merged?

stormshield-gt · 2024-08-28T17:10:44Z

I've rebased on top of #131 and tried to implement the new_with_extra_roots for Windows, thanks to the guidance of @complexspaces

We can create a custom CERT_CHAIN_ENGINE_CONFIG and set cAdditionalStore to a custom, in-memory certificate store. This engine and store are then included in our call to CertGetCertificateChain.

Sadly, it keeps considering the custom CA as untrusted. I've tried to tweak the flags and various parameters, and add debug traces but without success. Anyone has any idea or how we can solve this ?

On GitHub there are very few usages of rghAdditionalStore/cAdditionalStore, most of them seems to use the same parameters as me. For instance here and here

cpu · 2024-08-28T17:16:08Z

I've rebased on top of #131 and tried to implement the new_with_extra_roots for Windows

I think it would be better to land the MacOS code and handle Windows as a follow-up, especially if there are implementation issues to sort out. The MacOS code seemed ready mod any review comments from a 2nd reviewer. WDYT?

stormshield-gt · 2024-08-28T17:42:19Z

Yes sure, I will split the Windows work on an other PR

rustls-platform-verifier/src/verification/apple.rs

rustls-platform-verifier/src/verification/others.rs

complexspaces

Thank you for getting started on this implementation.

complexspaces · 2024-09-01T05:53:07Z

rustls-platform-verifier/src/verification/others.rs

@@ -54,11 +54,17 @@ impl Verifier {
    /// WebPKI, using root certificates provided by the platform and augmented by
    /// the provided extra root certificates.
    pub fn new_with_extra_roots(
-        roots: impl IntoIterator<Item = pki_types::TrustAnchor<'static>>,
+        roots: impl IntoIterator<Item = pki_types::CertificateDer<'static>>,


Does anyone have thoughts on the future possibility of having a RootLike trait in pki-types that could be used here instead? That way we could abstract over the difference between webpki_root_certs for native platforms and webpki_roots in the other cases and save on binary size, parsing time, etc.

Parsing seems like it would need to handle an error, but core::convert::Infallible might let the compiler optimize that out entirely.

I think the tricky part here would be that the conversion from CertificateDer to TrustAnchor is a non-trivial amount of DER parsing that currently lives in webpki. So far, pki-types contains very little logic, and my intuition is that it might be better to keep it that way?

Maybe it could be an enum RootRepresentation that lives in rustls?

That seems like a reasonable middleground. With that this could be a bit cleaner and push the bulkiness of roots further away from this crate (and probably others)

@ctz @cpu thoughts?

I think I would prefer to allow composition of verifiers at the rustls level, rather than relax the type safety of TrustAnchor.

But, then, I think that's because I'm not 100% sold on the value of giving extra roots to the platform verifier (I previously thought it was a method for affixing the behaviour for testing purposes, but I guess not?)

rustls-platform-verifier/src/tests/verification_mock/mod.rs

rustls-platform-verifier/src/verification/apple.rs

djc · 2024-09-02T09:44:43Z

rustls-platform-verifier/src/verification/apple.rs

+    ///
+    /// See [Verifier::new] for the external requirements the verifier needs.
+    pub fn new_with_extra_roots(
+        roots: impl IntoIterator<Item = pki_types::CertificateDer<'static>>,


Sorry, one more nit: I think we should just take Vec here as the argument? We're going to store a Vec internally anyway, and IMO it's slightly better to allow the caller to provide that for the case where they already have a Vec allocated for some reason.

(Should then do the same thing for the Unix platform in the later commit.)

I see your point, I've updated the code

Hmm, I failed to consider that on Unix we have to iterate anyway to map from CertificateDer to TrustAnchor, so I guess the optimal scenario for macOS/iOS is different from the Unices? Not sure what the best choice here is. Going to merge this now -- thanks for all the work, I am happy to do a follow-up here to change it back it to iterators if people think that would be better.

Co-authored-by: ComplexSpaces <[email protected]>

The `new_with_extra_roots` function for other platforms now take a `CertificateDer` instead of a `TrustAnchor` which is a breaking change

ctz

oops, i didn't submit this (deleted comments as they were all addressed already!)

ctz · 2024-09-05T07:34:02Z

rustls-platform-verifier/src/verification/others.rs

@@ -54,11 +54,17 @@ impl Verifier {
    /// WebPKI, using root certificates provided by the platform and augmented by
    /// the provided extra root certificates.
    pub fn new_with_extra_roots(
-        roots: impl IntoIterator<Item = pki_types::TrustAnchor<'static>>,
+        roots: impl IntoIterator<Item = pki_types::CertificateDer<'static>>,


I think I would prefer to allow composition of verifiers at the rustls level, rather than relax the type safety of TrustAnchor.

But, then, I think that's because I'm not 100% sold on the value of giving extra roots to the platform verifier (I previously thought it was a method for affixing the behaviour for testing purposes, but I guess not?)

stormshield-gt marked this pull request as draft August 26, 2024 08:39

cpu reviewed Aug 26, 2024

View reviewed changes

complexspaces reviewed Aug 26, 2024

View reviewed changes

rustls-platform-verifier/src/tests/verification_mock/mod.rs Show resolved Hide resolved

rustls-platform-verifier/src/verification/others.rs Outdated Show resolved Hide resolved

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from 1c21fed to 5d19173 Compare August 27, 2024 09:55

cpu approved these changes Aug 27, 2024

View reviewed changes

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from 5d19173 to 223bbe7 Compare August 28, 2024 16:54

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch 2 times, most recently from 6b1ec76 to b5a5b4f Compare August 28, 2024 17:43

stormshield-gt changed the title ~~Add new with extra roots on other platforms~~ Add new with extra roots on macos/ios Aug 28, 2024

stormshield-gt marked this pull request as ready for review August 28, 2024 17:47

stormshield-gt mentioned this pull request Aug 28, 2024

Add new with extra roots on windows #135

Merged

cpu requested review from complexspaces, djc and ctz August 28, 2024 17:57

djc approved these changes Aug 29, 2024

View reviewed changes

rustls-platform-verifier/src/verification/apple.rs Outdated Show resolved Hide resolved

rustls-platform-verifier/src/verification/apple.rs Outdated Show resolved Hide resolved

rustls-platform-verifier/src/verification/others.rs Outdated Show resolved Hide resolved

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from b5a5b4f to 3e738cf Compare August 29, 2024 10:26

djc reviewed Aug 29, 2024

View reviewed changes

rustls-platform-verifier/src/verification/others.rs Outdated Show resolved Hide resolved

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from bf6a2ba to 621969b Compare August 29, 2024 15:02

complexspaces approved these changes Sep 1, 2024

View reviewed changes

complexspaces reviewed Sep 1, 2024

View reviewed changes

rustls-platform-verifier/src/verification/apple.rs Outdated Show resolved Hide resolved

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch 2 times, most recently from d690b3a to 4256a4e Compare September 2, 2024 09:28

djc approved these changes Sep 2, 2024

View reviewed changes

Add extra roots on macos and ios

2136496

Co-authored-by: ComplexSpaces <[email protected]>

stormshield-gt added 3 commits September 2, 2024 12:01

Link RFC 5280 AIA section to certificate instead of CRL

61ca4cc

Uniformize new_with_extra_roots

36cc3f5

Add test for new_with_extra_roots

beef74a

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from 4256a4e to fb85563 Compare September 2, 2024 10:02

Cargo: version 0.3.4 -> 0.4.0

094341e

The `new_with_extra_roots` function for other platforms now take a `CertificateDer` instead of a `TrustAnchor` which is a breaking change

stormshield-gt force-pushed the add_new_with_extra_roots_on_other_platforms branch from fb85563 to 094341e Compare September 2, 2024 10:10

djc merged commit 39adc94 into rustls:main Sep 2, 2024
17 of 18 checks passed

djc mentioned this pull request Sep 2, 2024

Fix FreeBSD #137

Merged

ctz reviewed Sep 5, 2024

View reviewed changes

tottoto mentioned this pull request Oct 5, 2024

feat(tls): Add rustls-platform-verifier support hyperium/tonic#1974

Draft

stormshield-gt mentioned this pull request Oct 14, 2024

Consider adding Verifier::new_with_extra_roots implementation to other platforms #58

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new with extra roots on macos/ios #133

Add new with extra roots on macos/ios #133

stormshield-gt commented Aug 26, 2024 •

edited

Loading

cpu left a comment

complexspaces commented Aug 26, 2024

cpu commented Aug 26, 2024

stormshield-gt commented Aug 27, 2024

cpu left a comment

stormshield-gt commented Aug 28, 2024

cpu commented Aug 28, 2024 •

edited

Loading

stormshield-gt commented Aug 28, 2024

complexspaces left a comment

complexspaces Sep 1, 2024

djc Sep 2, 2024 •

edited

Loading

complexspaces Sep 5, 2024

djc Sep 5, 2024

ctz Sep 5, 2024

djc Sep 2, 2024

stormshield-gt Sep 2, 2024

djc Sep 2, 2024

ctz left a comment •

edited

Loading

ctz Sep 5, 2024

Add new with extra roots on macos/ios #133

Add new with extra roots on macos/ios #133

Conversation

stormshield-gt commented Aug 26, 2024 • edited Loading

cpu left a comment

Choose a reason for hiding this comment

complexspaces commented Aug 26, 2024

cpu commented Aug 26, 2024

stormshield-gt commented Aug 27, 2024

cpu left a comment

Choose a reason for hiding this comment

stormshield-gt commented Aug 28, 2024

cpu commented Aug 28, 2024 • edited Loading

stormshield-gt commented Aug 28, 2024

complexspaces left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormshield-gt commented Aug 26, 2024 •

edited

Loading

cpu commented Aug 28, 2024 •

edited

Loading

djc Sep 2, 2024 •

edited

Loading

ctz left a comment •

edited

Loading