fix: install the rustls CryptoProvider unconditionally before any TLS build (#90) - #112
Merged
Merged
Conversation
… build (#90) build_tls_connector only installed the process-wide rustls CryptoProvider as a side effect inside NoCertVerifier::new() / VerifyCaCertVerifier::new() (the require/verify-ca branches). The verify-full and prefer/allow/disable fallthrough branches build a ClientConfig/WebPkiServerVerifier directly, with no such side effect -- so provider installation depended on which TLS mode happened to run first in the process. The builtin driver avoids this by calling ensure_rustls_crypto_provider() unconditionally at the top of build_postgres_tls_connector, before any branch runs. Ported that exactly. Added 4 new unit tests locking in that all four previously-under-verified branches (verify-full with/without an explicit CA, and the prefer/disable fallthrough) build successfully. The existing TLS tests only ever exercised require/verify-ca, which install the provider as a side effect -- these are the first tests for the branches the issue is actually about. Investigated whether the panic this issue describes is actually reproducible against this repo's pinned dependency versions (rustls = "0.23" with only the "ring" feature). Read rustls 0.23.45's actual source: CryptoProvider::get_default_or_install_from_crate_features (crypto/mod.rs) is the function ClientConfig::builder() and WebPkiServerVerifier::builder() both call internally, and it already falls back to installing the provider named by crate features (ring, here) when no default is set -- present in every 0.23.x release checked locally (.37/.40/.43/.45). Confirmed empirically: spawned genuinely fresh processes (not just fresh test functions in the shared test-binary process, which can't isolate this) of both the pre-fix and post-fix binaries, making verify-full (both with and without an explicit ssl_ca) the very first TLS operation each process ever performed, against a real TLS-enabled PostgreSQL container. Neither panicked; both failed the same ordinary way (a self-signed test CA not chaining to the real server cert), with identical exit codes. This means the specific rustls 0.23 auto-install fallback already covers this repo's actual dependency graph, so the crash scenario the issue describes is not currently reproducible here -- but the guard is still the correct, defensive fix to carry: it makes the plugin's behavior independent of that rustls-internal fallback existing/continuing to exist, matches the builtin's own defensive posture (added for the same reason), and costs nothing (a single Once-guarded call). Documented this finding in the PR rather than overstating the current risk.
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build_tls_connectoronly installed the process-wide rustlsCryptoProvideras a side effect insideNoCertVerifier::new()/VerifyCaCertVerifier::new()(therequire/verify-cabranches). Theverify-fullandprefer/allow/disablefallthrough branches build aClientConfig/WebPkiServerVerifierdirectly, with no such side effect — so provider installation depended on which TLS mode happened to run first in the process.ensure_rustls_crypto_provider()unconditionally at the top ofbuild_postgres_tls_connector, before any branch runs. Ported that exactly.Fixes #90.
Test plan
verify-fullwith/without an explicit CA, and theprefer/disablefallthrough) build successfully. The existing TLS tests only ever exercisedrequire/verify-ca, which install the provider as a side effect — these are the first tests for the branches the issue is actually about.cargo test --lib— 331 passedcargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleanImportant finding — is the panic actually reproducible here?
I investigated whether the panic this issue describes is actually reproducible against this repo's pinned dependency versions (
rustls = "0.23"with only theringfeature), rather than taking the theoretical risk at face value.Read rustls 0.23.45's actual source:
CryptoProvider::get_default_or_install_from_crate_features(crypto/mod.rs) is the functionClientConfig::builder()andWebPkiServerVerifier::builder()both call internally, and it already falls back to installing the provider named by crate features (ring, here) when no default is set — present in every 0.23.x release I checked locally (.37/.40/.43/.45).Confirmed empirically: spawned genuinely fresh processes (not just fresh test functions in the shared test-binary process, which can't isolate this — a
Onceguard is process-global) of both the pre-fix and post-fix binaries, makingverify-full(both with and without an explicitssl_ca) the very first TLS operation each process ever performed, against a real TLS-enabled PostgreSQL container. Neither panicked; both failed the same ordinary way (a self-signed test CA not chaining to the real server cert), with identical exit codes.This means the specific rustls 0.23 auto-install fallback already covers this repo's actual dependency graph, so the crash scenario the issue describes is not currently reproducible here — but the guard is still the correct, defensive fix to carry: it makes the plugin's behavior independent of that rustls-internal fallback existing/continuing to exist across future dependency updates, matches the builtin's own defensive posture (added for the same reason), and costs nothing (a single
Once-guarded call). Documenting this rather than overstating the current risk — the fix is correct and worth having, just not currently load-bearing against an actual crash on this repo's dependency versions.