Fix the case of SSL_CERT_DIR containing number of paths by arilou · Pull Request #187 · rustls/rustls-native-certs

arilou · 2025-05-26T13:00:44Z

According to OpenSSL docs SSL_CERT_DIR is an env seperated list of directories (for Unix it's colon on Windows it's semi-colon).

Reference:
https://www.openssl.org/docs/man1.0.2/man1/c_rehash.html

cpu · 2025-05-26T13:08:18Z

According to OpenSSL docs SSL_CERT_DIR is an env seperated list of directories (for Unix it's colon on Windows it's semi-colon).

Have you verified that OpenSSL on Windows uses ; for this purpose (e.g. through experimentation, or from source code analysis)? Their documentation doesn't mention it at all and just says the var is colon separated without further segmentation by OS.

(I also checked the 3.5 documentation says the same thing as the older 1.0.2 docs you linked)

cpu

Some initial comments from a quick review pass. Seems worth some unit test coverage as well.

src/lib.rs

arilou · 2025-05-26T13:12:34Z

According to OpenSSL docs SSL_CERT_DIR is an env seperated list of directories (for Unix it's colon on Windows it's semi-colon).

Have you verified that OpenSSL on Windows uses ; for this purpose (e.g. through experimentation, or from source code analysis)? Their documentation doesn't mention it at all and just says the var is colon separated without further segmentation by OS.

(I also checked the 3.5 documentation says the same thing as the older 1.0.2 docs you linked)

You can see here how LIST_SEPARATOR_CHAR is defined for the split, in Windows it's semi-colon
https://github.com/openssl/openssl/blob/master/include/internal/e_os.h#L142

cpu · 2025-05-26T13:19:23Z

You can see here how LIST_SEPARATOR_CHAR is defined for the split, in Windows it's semi-colon

Fun, looks like there's both a C implementation of c_rehash and a Perl one. The C version does use LIST_SEPARATOR_CHAR as you point out, and the Perl version appears to have similar logic.

@arilou Are you open to filing an upstream OpenSSL bug to fix their documentation of SSL_CERT_DIR to mention the platform specific details of the separator char?

arilou · 2025-05-26T13:24:06Z

You can see here how LIST_SEPARATOR_CHAR is defined for the split, in Windows it's semi-colon

Fun, looks like there's both a C implementation of c_rehash and a Perl one. The C version does use LIST_SEPARATOR_CHAR as you point out, and the Perl version appears to have similar logic.

@arilou Are you open to filing an upstream OpenSSL bug to fix their documentation of SSL_CERT_DIR to mention the platform specific details of the separator char?

Done
openssl/openssl#27698

src/lib.rs

arilou · 2025-05-28T14:46:32Z

Alright I'll try to get to changing it to a Vec from Option<Vec..
Will probably get to it during the weekend

djc

Looking good modulo a few remaining nits. Thanks!

src/lib.rs

cpu

Thanks for sticking with our feedback. I think the diff looks good :-)

Seems worth some unit test coverage as well.

Bumping this earlier comment I made. I won't block an approval on it, but it would be nice to have a small test or two to prevent accidental regression in the future. WDYT?

arilou · 2025-05-30T15:42:59Z

What type of test did you have in mind?

According to OpenSSL docs SSL_CERT_DIR is an env seperated list of directories (for Unix it's colon on Windows it's semi-colon). Reference: https://docs.openssl.org/3.5/man1/openssl-rehash/#options Signed-off-by: Jon Doron <jond@wiz.io>

cpu · 2025-05-30T16:56:23Z

What type of test did you have in mind?

Ideally something that shows that specifying multiple SSL_CERT_DIR values influences the path building outcome correctly. Maybe a variation on badssl_with_dir_from_env() where the BadSSL trust anchor is located in the 2nd dir in a : delim'd list in the SSL_CERT_DIR env var?

Adds a test to ensure that SSL_CERT_DIR correctly handles multiple paths separated by the platform-specific separator (: on Unix, ; on Windows). The test sets up two temporary directories: * The first is left empty. * The second contains a copy of the test certificate. It then sets SSL_CERT_DIR to include both directories and calls check_site() to verify that the certificate is successfully loaded from the second path. Signed-off-by: Jon Doron <jond@wiz.io>

cpu

Thanks, the new test looks good. I ran it locally since the smoketests are in a CI job that doesn't run for PRs:

$ git rev-parse HEAD
1438c63107b79309c958241ed08fa42f3cb790bc

$ cargo test ssl_cert_dir_multiple_paths_are_respected -- --ignored --exact
<snipped>

running 1 test
test ssl_cert_dir_multiple_paths_are_respected ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 11 filtered out; finished in 0.15s

## Summary Closes #16414 Adds support for the standard [SSL_CERT_DIR](https://docs.openssl.org/3.6/man3/SSL_CTX_load_verify_locations) which has gained recent proper support from [rustls-native-certs](rustls/rustls-native-certs#187) in v0.8.2. In addition, this PR clarifies documentation around `SSL_CERT_FILE` and `SSL_CERT_DIR` when used in combination with `UV_NATIVE_TLS` as mentioned in #16412 (comment) ## Test Plan Manually tested with custom cert chains in multiple directories and loading them via SSL_CERT_DIR. We didn't have tests for `SSL_CERT_FILE` or `SSL_CERT_DIR` environment variables so I added a basic one using our own test-only certificate generation and dummy https server. I also moved some things around for better reuse.

cpu reviewed May 26, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

arilou force-pushed the fix_ssl_cert_dir branch from d7d0cbb to d3df689 Compare May 26, 2025 13:17

arilou force-pushed the fix_ssl_cert_dir branch from d3df689 to db3326f Compare May 26, 2025 13:25

djc reviewed May 26, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

djc reviewed May 26, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

arilou force-pushed the fix_ssl_cert_dir branch from db3326f to c3e6516 Compare May 26, 2025 14:15

djc reviewed May 26, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

arilou force-pushed the fix_ssl_cert_dir branch from c3e6516 to d013ed8 Compare May 30, 2025 13:06

djc approved these changes May 30, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

cpu approved these changes May 30, 2025

View reviewed changes

arilou force-pushed the fix_ssl_cert_dir branch from d013ed8 to 5589d6e Compare May 30, 2025 15:42

arilou force-pushed the fix_ssl_cert_dir branch from 5589d6e to 32ed410 Compare May 30, 2025 16:23

cpu approved these changes May 31, 2025

View reviewed changes

djc merged commit 93c893e into rustls:main May 31, 2025
13 of 14 checks passed

samypr100 mentioned this pull request Oct 27, 2025

Add support for SSL_CERT_DIR astral-sh/uv#16473

Merged

Conversation

arilou commented May 26, 2025

Uh oh!

cpu commented May 26, 2025

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arilou commented May 26, 2025

Uh oh!

cpu commented May 26, 2025

Uh oh!

arilou commented May 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arilou commented May 28, 2025

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

arilou commented May 30, 2025

Uh oh!

cpu commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cpu commented May 30, 2025 •

edited

Loading