Search cert file and dir returned by openssl-probe #109
Conversation
pgerber
force-pushed
the
freebsd
branch
4 times, most recently
from
b8bc610 to
0266775
Compare
There was a problem hiding this comment.
Thanks for your work on improving rustls-native-certs!
Unfortunately in its current form this PR is quite hard to review. When we merge code we like make sure the PR consists of a series of well-defined changes, whereas this PR contains a bunch of changes all squashed together. Please read through https://github.com/rustls/rustls/blob/main/CONTRIBUTING.md for more guidance.
In particular, it would be good to avoid moving existing code around in the same commit as adding a bunch of new code, it would be good to split out addition of documentation for existing code from any other changes (in a separate commit). Then, further commits that change the behavior should update the documentation.
It would also be good to add a PR description that briefly describes which conceptual changes you're trying to make here.
|
So, the change should now be in a more reviewable state. I wonder if I'm just too used to people squashing commits and using commit message like "fixes #NNN". I should remind myself more often to work with small, digestible commits, even found I few issues splitting things down into pieces. |
|
@cpu, updated commits will follow soon. I'm afraid I had to leave before I managed to do a final review before pushing the changes. |
pgerber
force-pushed
the
freebsd
branch
2 times, most recently
from
5958180 to
e4fa787
Compare
src/lib.rs
Outdated
| @@ -63,7 +63,9 @@ const ENV_CERT_FILE: &str = "SSL_CERT_FILE"; | |||
| /// Returns None if SSL_CERT_FILE is not defined in the current environment. | |||
| /// | |||
| /// If it is defined, it is always used, so it must be a path to a real | |||
| /// file from which certificates can be loaded successfully. | |||
| /// file from which certificates can be loaded successfully. However, | |||
There was a problem hiding this comment.
I'd skip the "However" here, which doesn't seem entirely appropriate? Maybe rephrase as:
While parsing, [
rustls_pemfile::certs()] parser will ignore parts of the file which are not part of a certificate.
There was a problem hiding this comment.
"However" I chose because I'm trying to say certificate loading needs to be successful but the fact that parts of the file may be ignored (e.g. because the wrong certificate format is used or a certificate is corrupted) can mean that we report success/Ok(_) when really the certificate failed to parse, at least from a user's perspective.
What about this:
While parsing, [rustls_pemfile::certs()] parser will ignore parts of the file which are not considered part of a certificate. Certificates which are not in the right format (PEM) or are otherwise corrupted may get ignored silently.
I also wonder if this should we document this in the public part of the documentation too. Perhaps we could clarify what format is expected to make it less likely people will use the wrong format. Something like:
Be sure certificates are in PEM format. Example:
<insert example cert in PEM format>
There was a problem hiding this comment.
What's an error and what's ignored seems a bit arbitrary to me.
This is ignored
----BEGIN CERTIFICATE-----
^ missing '-' at start
<content>
-----END CERTIFICATE-----
This is an error:
-----BEGIN CERTIFICATE-----
<content>
-----END CERTIFICATE----
^ missing '-'
This is ignored:
-----BEGIN CERTIFICATE-----
<content>
-----END CERTIFICATE----
^ missing '-' AND '\n'
This is ignore:
-----BEGIN CERTIFICATE-----
<content>
-----END CERTIFICATE-----
^ missing '\n'
There was a problem hiding this comment.
I think we'd probably take PRs to make all of those errors, but the main goal in that crate was to properly ignore things that don't look like PEM objects at all which has been sighted in the wild. I guess these cases really haven't shown up.
All of your documentation suggestions sound good to me!
There was a problem hiding this comment.
PEM appears to use the encoding defined in RFC7468. Ignoring invalid sections of the file is part of the encoding but, if I read the grammar right, the line feed at the end of -----END CERTIFICATE----- is explicitly optional in "lax mode". So, perhaps it should be made optional in RusTLS parser too. I'm not sure.
There was a problem hiding this comment.
Thanks for looking into that! Would be good to file a followup issue and/or PR for that.
There was a problem hiding this comment.
Added it to my todo list.
Avoid catching panic without truly knowing what kind of error caused it. There could have been an network error or an error loading certificates triggering a panic. (Unlikely with the current test but there is more tests for failure conditions coming.)
• Add test to make this explicit. • Update (private) documentation to match status quo.
• Change wording of (private) documentation. I find "real" to be an ambiguous description. It could for instance be interpreted to mean it needs to be what Unix calls regular file (as opposed to a symlink, FIFO, etc.) • Add tests to show that file must exists but may be a non-regular file.
• On Unix, certificates were loaded from the OpenSSL certificate store but only the file based-certificate store (known as CAfile) in OpenSSL. Load certs from dir-based store too (CAdir). • On all platforms, accept new SSL_CERT_DIR, which is also accepted by OpenSSL.
Cargo sets these env. vars. internally, at least here, on Linux:
$ cargo init --bin env-conflict
$ cd env-conflict
$ cat >src/main.rs <<EOF
fn main() {
let _ = dbg!(std::env::var("SSL_CERT_FILE"));
let _ = dbg!(std::env::var("SSL_CERT_DIR"));
}
EOF
$ cargo -q run
[src/main.rs:2:13] std::env::var("SSL_CERT_FILE") = Ok(
"/usr/lib/ssl/cert.pem",
)
[src/main.rs:3:13] std::env::var("SSL_CERT_DIR") = Ok(
"/usr/lib/ssl/certs",
)
|
Thanks for all your work on this! Will do a follow-up which bumps the version and has some very minor tweaks. |
|
My pleasure :-) Thanks again for the contribution! |
Fixes #28