”Permission denied“ since v0.7.1

[rustdesk]

I found this when we upgraded seanmonstar/reqwest#2391, one of our users reported this, rustdesk/rustdesk-server-pro#360. It is not easy to reproduce, I can only see this on his machine so far.

❯ ./test-master
Custom { kind: PermissionDenied, error: "could not load certs from dir /usr/lib/ssl/certs: Permission denied (os error 13)" }
❯ ./test-v0.6.2
ok
❯ ./test-v0.6.3
ok
❯ ./test-v0.7.0
ok
❯ ./test-v0.7.1
Os { code: 13, kind: PermissionDenied, message: "Permission denied" }

❯ sudo ./test-master
ok
❯ sudo ./test-v0.6.2
ok
❯ sudo ./test-v0.6.3
ok
❯ sudo ./test-v0.7.0
ok
❯ sudo ./test-v0.7.1
ok

The test is

use std::error::Error;
use x509_parser::prelude::*;

fn main() -> Result<(), Box<dyn Error>> {
    if let Err(err) = rustls_native_certs::load_native_certs() {
      println!("{:?}", err);
    }
    Ok(())
}

[djc]

Can you try with main? It should have a more precise error message.

[ChieloNewctle]

I have encountered the same problem. And the issue is gone after switch to git main:

rustls-native-certs = { git = "https://github.com/rustls/rustls-native-certs.git" }

I think it is #117 that fixed the issue.
Hope to have a new release. Thanks.

[ctz]

I think it is #117 that fixed the issue.
Hope to have a new release. Thanks.

-> #125

[rustdesk]

This is the git main.

./test-master
Custom { kind: PermissionDenied, error: "could not load certs from dir /usr/lib/ssl/certs: Permission denied (os error 13)" }

[ctz]

Is that unexpected given the permissions involved? Inspect these by running, for example namei -mo /usr/lib/ssl/certs/ISRG_Root_X1.pem and id as the user this crates code runs under.

This crate isn't doing much special when it comes to permissions. It needs to enumerate and read the files in this directory.

[rustdesk]

Below < v0.7.1 can get certs without permission issue.

use std::error::Error;
use x509_parser::prelude::*;

fn main() -> Result<(), Box<dyn Error>> {
    if let Err(err) = rustls_native_certs::load_native_certs() {
      println!("{:?}", err);
    }
    Ok(())
}

[djc]

Yes, in earlier releases we didn't inspect some of the paths that might have additional certificates installed on the user's system. (See the release notes for 0.7.1.)

[rustdesk]

I think the bug is here.

a0a7012

    fn load(&self) -> Result<Vec<CertificateDer<'static>>, Error> {
        let mut certs = match &self.file {
            Some(cert_file) => load_pem_certs(cert_file)?,
            None => Vec::new(),
        };

        if let Some(cert_dir) = &self.dir {
            certs.append(&mut load_pem_certs_from_dir(cert_dir)?);
        }

load_pem_certs(cert_file)? or load_pem_certs_from_dir(cert_dir)? having one success is ok, but now, any one fail, you will fail.

[rustdesk]

It can be like this.

        let mut err = None;
        let mut certs = match &self.file {
            Some(cert_file) => {
               match load_pem_certs(cert_file) {
                 Err(err) => { err = Some(err); Vec::new()}
                  Ok(res) => res
                }
            None => Vec::new(),
        };

        if let Some(cert_dir) = &self.dir {
               match load_pem_certs_from_dir(cert_dir) {load failure.
                 Err(err) => { err = Some(err) }
                  Ok(res) => certs.apend(res)
                }
        }

       if certs.is_empty() && err.is_some() {
            return Err(err.unwrap());
        }

load_pem_certs_from_dir should not also return earlier becauce of one failure.

[djc]

I think a reasonable expectation from users installing certificates in their filesystem is that those certificates get used, so when, for whatever reason, software is not actually capable of using the installed certificates, raising an error makes sense.

But perhaps this is too much of a policy decision that the application-level logic needs to customize, and we should have a different API that both yields an error and also any certificates that have been found?

[rustdesk]

another reasonable expectation is additional function does not break old function.

[djc]

I think signaling about error conditions that were previously ignored is generally an improvement, and this function was already fallible, so I don't think it's as simple as you make it out to be.

[rustdesk]

Yes, I just express my thought. It is your crate, you decide. :)
Thanks for your effort on this crate.

[djc]

Other issues:

• Problem with certificates after 0.7.1 #116
• UV 0.2.29 Permission error astral-sh/uv#5448
• Prepare 0.7.1 #113 (comment)

@ctz what do you think? We could

1. Do a semver-incompatible release to change the API to return (Vec<CertificateDer<'static>>, Vec<io::Error>)
2. Add a new function find_native_certs() with the new return type (and deprecate the old one?)
3. Swallow the error if we were able to find some certificates as proposed in ”Permission denied“ since v0.7.1 #124 (comment)

(There's a rough analogue here with the RootCertStore::add_parsable_certificates() API, which yields (usize, usize) for (added, ignored).)

[ctz]

Another option is to allow SSL_CERT_DIR to be used to disable the directory search behaviour altogether? Perhaps an empty string, or some sentinel value that would not overlap with a valid path?

edit: in fact, perhaps this is a sufficient workaround for those environments?

mkdir /tmp/no-certs-here
export SSL_CERT_DIR=/tmp/no-certs-here
export SSL_CERT_FILE=<...>

that leads to reading the certs from SSL_CERT_FILE, then a successful dummy directory search:

openat(AT_FDCWD, "/tmp/no-certs-here", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
fstat(3, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
getdents64(3, 0x645e4c178f70 /* 2 entries */, 32768) = 48
getdents64(3, 0x645e4c178f70 /* 0 entries */, 32768) = 0
close(3)                                = 0

[ChieloNewctle]

I apologize for the incorrect information provided earlier. 🙇🙇🙇
I ran a quick test actually on a different environment where the permissions of certificates are not aligned.
As for the solution, I suggest to have a backward-compatible update swallowing errors and another semver-incompatible update deprecating the old interfaces.

[djc]

Another option is to allow SSL_CERT_DIR to be used to disable the directory search behaviour altogether? Perhaps an empty string, or some sentinel value that would not overlap with a valid path?

edit: in fact, perhaps this is a sufficient workaround for those environments?

mkdir /tmp/no-certs-here
export SSL_CERT_DIR=/tmp/no-certs-here
export SSL_CERT_FILE=<...>

that leads to reading the certs from SSL_CERT_FILE, then a successful dummy directory search:

openat(AT_FDCWD, "/tmp/no-certs-here", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
fstat(3, {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
getdents64(3, 0x645e4c178f70 /* 2 entries */, 32768) = 48
getdents64(3, 0x645e4c178f70 /* 0 entries */, 32768) = 0
close(3)                                = 0

My worry is that setting this environment variable is a painful workaround when the user of the software is administratively far removed from the author of the software. For example, in the case of rustup (used by most Rust programmers around the world on whatever development machine) or, I imagine, in the case of rustdesk (where the software gets used by a large population on varied hardware/software environments). I think we should make it easy for authors of software like that to do the right thing (and spit out some logging), which might be to still continue with whatever certificates we did parse (probably typically the ones from SSL_CERT_FILE rather than SSL_CERT_DIR?).

[ctz]

Fair point. I think that's also an argument that introducing an API change that requests or allows best-effort behaviour has limited benefits?

So I think I'm down for (3). Further justification: golang/go@e83bcd9#diff-460ef6ed238924757954b5486f513fdc2f0a3d54c6e9d361e47c867e19f662bfR83-R87 which AFAICT is doing the same logic.

An additional detail: perhaps take an optional dep on log, and complain at warning level for errors that were swallowed because we returned some certs?

[djc]

Fair point. I think that's also an argument that introducing an API change that requests or allows best-effort behaviour has limited benefits?

Not sure... when I'm deploying containers at work that use rustls-native-certs (perhaps via rustls-platform-verifier) I'm both the author and the user and I would like to opt in to stricter error handling.

So I think I'm down for (3). Further justification: golang/go@e83bcd9#diff-460ef6ed238924757954b5486f513fdc2f0a3d54c6e9d361e47c867e19f662bfR83-R87 which AFAICT is doing the same logic.

An additional detail: perhaps take an optional dep on log, and complain at warning level for errors that were swallowed because we returned some certs?

I think we should do tracing instead of log, but yeah, issuing those would make sense to me.

[ctz]

https://crates.io/crates/rustls-native-certs/0.7.3 published with a fix for this.

[rustdesk]

Thanks for your great work.