Add new with extra roots on windows

[stormshield-gt]

Windows implementation of new_with_extra_roots, which have some problems for now as mentioned in #133 (comment).
Note that it's based on top of #133 to benefit the tests defined here.

Partially Fix #58

[stormshield-gt]

After experimenting using cAdditionalStore/rghAdditionalStore, I concluded that this store can only be used for intermediate certificates and not for roots.
Indeed, when configuring the certEngine, if I add the intermediates only to the additonalStore and the extra roots to the exclusiveStore, the validation is successful. However, if I add both the intermediates and the extra roots to the additionalStore, then the validation failed, saying it doesn't trust the root.

So we can't use cAdditionalStore to achieve what we want. Chromium seems to have also dropped this idea.

I've come up with 3 possible solutions:

1. Use hExclusiveRoot to load the system trust store and the extra roots. I got a POC locally that pass the tests that looks like this:

Click to expand the code

    fn new_with_extra_roots(
        roots: &[pki_types::CertificateDer<'static>],
    ) -> Result<Self, TlsError> {

        let mut inner = Self::new()?;

        // In memory store for the extra roots
        let mut additional_store = CertificateStore::new()?;
        for root in roots {
            additional_store.add_cert(root)?;
        }

        // System store
        let mut pvpara: Vec<_> = "root".encode_utf16().collect();
        pvpara.push(0);
        let system_store = unsafe { CertOpenSystemStoreW(0, pvpara.as_ptr()) };

        // We create a collection, and we add the two previous created stores.
        let collection = unsafe {
            CertOpenStore(
                CERT_STORE_PROV_COLLECTION,
                X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
                0,
                CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG,
                ptr::null(), //must be null in that case
            )
        };
        unsafe {
            CertAddStoreToCollection(
                collection,
                system_store,
                CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG,
                0,
            )
        };
        unsafe {
            CertAddStoreToCollection(
                collection,
                additional_store.inner.as_ptr(),
                CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG,
                0,
            )
        };

        let mut config = CERT_CHAIN_ENGINE_CONFIG::zeroed_with_size();
        config.hExclusiveRoot = collection;
        //..

However, this seems fragile as in Windows they are Root stores in various places. It will be hard to cover and maintain the roots that match the ones that are naturally looked by CertGetCertificateChain that don't use exclusiveStore.

2. Do the verification twice in case of extra roots and combine the result. One verification with only the system root store and one with only the extra roots. That's what's the Golang std seems to do, but for backward compatibility reasons. To cite the go issue :

In order not to break existing code that invokes x509.SystemCertPool and then adds extra custom certificates to it, we’ll have Verify perform two verification in that case, one with the platform and one with the custom roots, and return chains from both.

3. Use the same approach as schannel crate. Basically, they add the CERT_CHAIN_POLICY_ALLOW_UNKNOWN_CA_FLAG flag if the root of the chain is in the extra roots.

Other references

In the source code of curl and Dotnet, we can see that they only support using extra roots exclusively, not in combination with the system root store like we plan to support.

[stormshield-gt]

This MR is ready to review, I've opted for the golang approach that I discussed on discord.

The Android job failure seems unrelated, as it's already on main.

[complexspaces]

Thanks for the PR, I think this approach is the most reasonable we have, and the code looks good for the most part.

One open question I have is about the infallibility of new_with_extra_roots. After seeing how we need to implement this on Windows, I'm wondering if we want to return Result<Self, TlsError> instead so that we can do OS-level logic in the constructor instead of being restricted to infallible operations.

The reason I mention this is because I think the re-build logic in this is not great for efficiency if we assume that the case where extra roots are provided is required for most, if not all, chain building operations to succeed. Instead we are allocating a new CertificateStore, parsing all the DER provided by the user again, etc. Instead, you could envision that we keep an Option<CertificateStore> on the Verifier structure instead of Vec<CertificateDer<'static> and reuse that when encountering partial chain errors.

[cpu]

Apologies I had queued review comments I hadn't submitted from a few days ago. I think Complexspaces and I overlapped in some feedback as a result.

One open question I have is about the infallibility of new_with_extra_roots. After seeing how we need to implement this on Windows, I'm wondering if we want to return Result<Self, TlsError> instead so that we can do OS-level logic in the constructor instead of being restricted to infallible operations.

I would be in favour of this 👍

[cpu]

The Android job failure seems unrelated, as it's already on main.

#140

[stormshield-gt]

Thank you both for the thorough review!

One open question I have is about the infallibility of new_with_extra_roots. After seeing how we need to implement this on Windows, I'm wondering if we want to return Result<Self, TlsError> instead so that we can do OS-level logic in the constructor instead of being restricted to infallible operations.

I also totally agree this could be better this way. I've implemented a new constructor where I parse the certificates right away.

I now store the certificate engine in the verifier to avoid creating a store each time for the extra roots.

[cpu]

This looks good to me within my limited understanding of the windows_sys APIs. Thank you @stormshield-gt !

[complexspaces]

No action needed: I'm not 100% certain of this assertion FWIW, but I think there's enough supporting evidence to go with it. Windows handles are generally thread safe, the CertFreeCertificateChainEngine function doesn't say it invalidates the handle, and an engine is really just a set of parameters Windows needs to read.

[cpu]

@djc @ctz Do either of you want to give this a pass or should we merge?

[djc]

No need to block on me; I looked at it but didn't have time to dig into the details of the Windows API.

[cpu]

Thanks again for your work on this @stormshield-gt 🎉