[Windows] Consider adding other store names than "ROOT"

[ginger51011]

Hello, thanks for the good work on rustls!

I'm writing from inside the corporate world where certificates are pushed out on user machines, much like #22. I recently found out that while rustls-native-certs can detect certificates in the Root certificate store, it fails to detect all that are needed in my environment.

I can open a PR for adding some other stores that are required for my use case, but I wanted to ask the question if it is desirable? Basically a change would look like this (in my case only CA needs to be added, but the others might be a good idea):

// windows.rs
/// See <https://learn.microsoft.com/en-us/dotnet/framework/configure-apps/file-schema/wcf/certificate-of-clientcertificate-element?redirectedfrom=MSDN#attributes>
const CERTIFICATE_STORE_NAMES: &[&str] = [
    "My",   // -> Certificate store for personal certificates
    "Root", // -> Trusted Root Certification Authorities
    "Trust", // -> I'm honestly not quite sure; I believe trusted third parties. There doesn't seem to be a good list.
    "CA",   // -> Intermediate Certification Authorities
];

pub fn load_native_certs() -> CertificateResult {
    let mut result = CertificateResult::default();

    for store_name in CERTIFICATE_STORE_NAMES {
        let current_user_store = match CertStore::open_current_user(store_name) {
            Ok(store) => store,
            Err(err) => {
                result.os_error(
                    err.into(),
                    format!("failed to open current user certificate store '{store_name}'"),
                );
                return result;
            }
        };

        for cert in current_user_store.certs() {
            if usable_for_rustls(cert.valid_uses().unwrap()) && cert.is_time_valid().unwrap() {
                result
                    .certs
                    .push(CertificateDer::from(cert.to_der().to_vec()));
            }
        }
    }

    result
}

The docs for CertStore::open_current_user mentions the store names I have included here, and the source above the CERTIFICATE_STORE_NAMES go into some detail.

[cpu]

Out of curiosity, does rustls-platform-verifier pick up your custom roots without issue?

[ctz]

I think we'd need to deeply understand what the various store names actually mean. eg, "Intermediate Certification Authorities" rings alarm bells for me, as trusting those directly would shorten the paths to remove the actually intended issuers.

[ginger51011]

Out of curiosity, does rustls-platform-verifier pick up your custom roots without issue?

Good question, I will try to check once I have access to the environment tomorrow.

I think we'd need to deeply understand what the various store names actually mean. eg, "Intermediate Certification Authorities" rings alarm bells for me, as trusting those directly would shorten the paths to remove the actually intended issuers.

Agreed. However, we should consider how the current setup compares to the unix and macos implementations; is the Windows implementation actually comparable to checking the ca-certificates file (and looking for it using openssl-probe)? From my understanding this file aggregates the many different CA certificates found at different places (but I might very well be wrong). The man page for update-ca-certificates basically tells us that it just joins together all files in a directory. What I basically want to do is to look for CAs in more places on Windows as well, but I'm not sure if Windows is more free. There is even one cert store named Disallowed...

Either way, another thing to consider is to perhaps add an environment variable to allow the setting of additional stores, like RUSTLS_NATIVE_CERTS_STORE_NAMES to allow the user to bypass settings. Just a thought, might be a bad idea as well (polluted environment and the like).

[ginger51011]

@cpu I wrote the following small program to test, and in my environment passing PV=1 (using rustls-platform-verifier) passes, but not doing so (using rustls-native-certs) fails (assuming I am using things correctly; my real use-case is using rustls via the reqwest rustls-tls-native-roots feature). Perhaps this is a reqwest issue?

The test was done using an internal service on SERVER signed by a specific certificate CORP-1, signed by CORP-ROOT. CORP-ROOT is available in the Root cert store, but CORP-1 is not.

use std::io::{Write, Read};
use std::sync::Arc;
use rustls::ClientConfig;
use rustls_platform_verifier;
use rustls_native_certs;

fn main() {
    let pv = std::env::var("PV");
    let server = std::env::var("SERVER").unwrap();

    let config = if let Ok("1") = pv.as_deref() {
        println!("Using platform verifier");
        ClientConfig::builder()
            .dangerous()
            .with_custom_certificate_verifier(Arc::new(rustls_platform_verifier::Verifier::new()))
            .with_no_client_auth()
    } else {
        println!("Using native tls verifier (change by passing setting PV=1)");
        let mut roots = rustls::RootCertStore::empty();
        let native_certs = rustls_native_certs::load_native_certs().unwrap();
        for cert in native_certs {
            roots.add(cert).unwrap();
        }

        ClientConfig::builder()
            .with_root_certificates(roots)
            .with_no_client_auth()
    };
    let rc_config = Arc::new(config);
    let host = server.clone().try_into().unwrap();
    let mut client = rustls::ClientConnection::new(rc_config, host).unwrap();

    let mut socket = std::net::TcpStream::connect(&format!("{server}:443")).unwrap();
    let mut stream = rustls::Stream::new(&mut client, &mut socket); // Create stream

    stream
    .write(b"GET / HTTP/1.1\r\nConnection: close\r\n\r\n")
    .unwrap();
    let mut plaintext = Vec::new();
    stream.read_to_end(&mut plaintext).unwrap();
    std::io::stdout().write_all(&plaintext).unwrap();
}

[djc]

Specifically for Windows (and macOS) I think the platform verifier crate always provides a better solution so unless you have a specific use case for the particular functionality in native-certs, I think you should just use that instead.

[cpu]

(using rustls-platform-verifier) passes,

Thanks for testing! That's good news.

my real use-case is using rustls via the reqwest rustls-tls-native-roots feature). Perhaps this is a reqwest issue?

I think the right fix is for reqwest to adopt rustls-platform-verifier. There's an outstanding issue to watch for this: seanmonstar/reqwest#2159

Specifically for Windows (and macOS) I think the platform verifier crate always provides a better solution so unless you have a specific use case for the particular functionality in native-certs, I think you should just use that instead.

Agreed.

[ginger51011]

(using rustls-platform-verifier) passes,

Thanks for testing! That's good news.

my real use-case is using rustls via the reqwest rustls-tls-native-roots feature). Perhaps this is a reqwest issue?

I think the right fix is for reqwest to adopt rustls-platform-verifier. There's an outstanding issue to watch for this: seanmonstar/reqwest#2159

Specifically for Windows (and macOS) I think the platform verifier crate always provides a better solution so unless you have a specific use case for the particular functionality in native-certs, I think you should just use that instead.

Agreed.

Thanks for linking that issue, it does indeed seem like the solution. I do believe that's the behaviour I needed all along.

I'm gonna check a bit more next week, but I simply did not know rustls-platform-verifier existed.

[djc]

I think the right fix is for reqwest to adopt rustls-platform-verifier. There's an outstanding issue to watch for this: seanmonstar/reqwest#2159

To be clear, you can use reqwest with the platform verifier crate today:

Client::builder()
.use_preconfigured_tls(
     rustls_platform_verifier::tls_config_with_provider(Arc::new(
           aws_lc_rs::default_provider(),
     ))
     .expect("failed to initialize pre-configured rustls backend"),
)

[cpu]

you can use reqwest with the platform verifier crate today:

Thanks for clarifying, that's a good call out. I meant just in terms of offering features to do this out-of-box ala rustls-tls-native-roots and native-roots.

[cpu]

I'm going to close this issue since I think we've reached a conclusion where rustls-platform-verifier is confirmed to work and we're generally in favour of investing resources in that repo vs adding more complexity to rustls-native-certs. If I'm mistaken and folks believe there's more to discuss here we can re-open.

Thanks!