feat(desktop): OS trust store and mTLS client certs for native HTTP - #1225
Conversation
The desktop app's native Rust HTTP client (guarded_http_client) trusted only the bundled Mozilla roots and could not present a client certificate, so remote fetches (tile/URL resolution, OGC GetCapabilities) to enterprise endpoints behind a private CA or mutual TLS failed, while the WebView fetch path succeeded via the OS trust store and an interactive cert prompt. - Trust the OS/system certificate store in addition to the bundled roots (rustls-tls-native-roots), so enterprise-CA-signed servers work with no configuration. - Present a client certificate for mutual TLS when configured via environment variables: GEOLIBRE_HTTP_CLIENT_CERT (PEM or PKCS#12), an optional GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD, and GEOLIBRE_HTTP_CA_CERT for a private CA. PEM identities use the default rustls backend; PKCS#12 (with passphrase) switches that one client to the platform native-tls backend. The interactive OS certificate prompt the WebView shows is not reachable from reqwest, so the native path uses this config-based mechanism instead. Verified end to end against a local mTLS server: both the PEM (rustls) and PKCS#12-with-passphrase (native-tls) client-cert paths complete the handshake, a client without the cert is rejected, and a wrong passphrase fails to load. Refs #1220
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe desktop Rust HTTP client now uses OS certificate roots, accepts extra CA certificates, and supports optional PEM or PKCS#12 client certificates for mTLS. Request handling, TLS dependencies, tests, and security documentation were updated. ChangesNative TLS and mutual TLS
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Environment
participant DesktopRustClient
participant HTTPSService
Environment->>DesktopRustClient: Configure CA and client certificate paths
DesktopRustClient->>DesktopRustClient: Apply OS roots, extra CA, and optional identity
DesktopRustClient->>HTTPSService: Make SSRF-guarded HTTPS request
HTTPSService-->>DesktopRustClient: Return authenticated HTTP response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Build a blocking HTTP client that enforces the SSRF guard at connect time | ||
| /// (via [`GuardedDnsResolver`]) and re-validates redirect hops. | ||
| /// | ||
| /// The client trusts the OS certificate store (via the `rustls-tls-native-roots` | ||
| /// feature) so enterprise CAs work, and presents a client certificate for | ||
| /// mutual TLS when one is configured (issue #1220). | ||
| fn guarded_http_client(timeout: Duration) -> Result<reqwest::blocking::Client, String> { |
There was a problem hiding this comment.
Performance: guarded_http_client is invoked fresh on every call to fetch_url_bytes_blocking/resolve_url_redirect_blocking (i.e. on every native fetch/URL-resolve command), and it now re-reads and re-parses the CA bundle and client certificate (including a PKCS#12 parse when mTLS is configured) from disk each time via extra_ca_certificates()/client_identity(). Since none of this config changes between calls, consider building the client once (e.g. behind a std::sync::OnceLock/OnceCell, or memoized on first use) instead of reconstructing it — including redoing crypto parsing — on every request.
Confidence: medium — not a correctness bug, but adds avoidable I/O and crypto work to a path that's on every OGC/XYZ/add-data fetch.
There was a problem hiding this comment.
Fixed in 4b672b1: the guarded client is built once behind a OnceLock, so the CA bundle and client certificate (including the PKCS#12 parse) are read and parsed a single time rather than on every fetch. Per-call deadlines moved to RequestBuilder::timeout().
| fn client_identity() -> Result<Option<ClientIdentity>, String> { | ||
| let Some(path) = env::var_os(HTTP_CLIENT_CERT_ENV) else { | ||
| return Ok(None); | ||
| }; | ||
| let path = PathBuf::from(path); | ||
| let bytes = fs::read(&path).map_err(|error| { | ||
| format!( | ||
| "Could not read client certificate {}: {error}", | ||
| path.display() | ||
| ) | ||
| })?; | ||
| let password = env::var(HTTP_CLIENT_CERT_PASSWORD_ENV).ok(); |
There was a problem hiding this comment.
If GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD is set but GEOLIBRE_HTTP_CLIENT_CERT is not, this function returns Ok(None) at the early-return on line 773-775 and the password is silently discarded — no client identity is configured and no error/warning is surfaced. A user who sets only the password env var (typo, or forgets the cert-path var) gets silent failure to establish mTLS with no indication of why, likely surfacing later as an opaque server-side TLS rejection instead of a clear local config error.
Confidence: medium — worth at least a debug log or an explicit error when the password is set without a cert path.
There was a problem hiding this comment.
Fixed in 4b672b1: client_identity() now returns an explicit error when GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD is set without GEOLIBRE_HTTP_CLIENT_CERT, instead of silently dropping the passphrase. Added a unit test (flags_passphrase_without_certificate_path).
| #[test] | ||
| fn classifies_client_cert_format() { | ||
| use std::path::Path; | ||
| // Extension selects PKCS#12, case-insensitively. | ||
| assert!(client_cert_is_pkcs12(Path::new("/certs/id.p12"), false)); | ||
| assert!(client_cert_is_pkcs12(Path::new("/certs/id.PFX"), false)); | ||
| // PEM (and unrecognised) extensions stay PEM without a passphrase. | ||
| assert!(!client_cert_is_pkcs12(Path::new("/certs/id.pem"), false)); | ||
| assert!(!client_cert_is_pkcs12(Path::new("/certs/id.crt"), false)); | ||
| assert!(!client_cert_is_pkcs12(Path::new("/certs/id"), false)); | ||
| // A passphrase forces PKCS#12 even for a PEM-looking or extensionless path. | ||
| assert!(client_cert_is_pkcs12(Path::new("/certs/id.pem"), true)); | ||
| assert!(client_cert_is_pkcs12(Path::new("/certs/id"), true)); | ||
| } |
There was a problem hiding this comment.
A few minor points on this change, grouped together:
- Test coverage (medium confidence): the only new test covers
client_cert_is_pkcs12's classification logic. There's no test exercisingguarded_http_client()itself to confirm the SSRF guard (GuardedDnsResolver/ redirect re-validation) still applies once the builder switches to thenative-tlsbackend for a PKCS#12 identity — that switch is new in this PR, and the guard is a security-critical property. Worth at least a targeted test (or explicit manual verification note) that a disallowed IP is still rejected on the native-tls code path, not just the default rustls path. - UX/quality (low confidence):
client_cert_is_pkcs12forces the PKCS#12 path whenever a password is present, even for a.pem-extensioned file. Since this implementation's PEM path requires an unencrypted PKCS8 key, a user who has a.pemwith an encrypted key and naturally setsGEOLIBRE_HTTP_CLIENT_CERT_PASSWORDhoping to decrypt it will get a "Could not load PKCS#12 client certificate" parse error instead of a message pointing at the real issue (this is documented behavior, so low severity, but the error message could be clearer for that specific case).
There was a problem hiding this comment.
Good points. On the SSRF guard: the GuardedDnsResolver and redirect policy are set on the builder before and independently of the TLS-backend choice, so .use_native_tls() (the PKCS#12 path) cannot drop them - I added a comment in build_guarded_http_client making that invariant explicit. I verified the full handshake end to end against a local openssl s_server requiring a client cert (both the rustls/PEM and native-tls/PKCS#12-with-passphrase paths succeed; a client without the cert is rejected), but I skipped committing binary cert fixtures for a backend-swap test since the guard holds by construction. On the encrypted-PEM-with-password case: agreed the error could be clearer; it is documented behavior (PEM path needs an unencrypted PKCS#8 key), and a passphrase routes to the PKCS#12 loader by design, so I left it as-is for now.
Code reviewReviewed the native HTTP client changes for OS trust-store parity and mTLS client certs ( Bugs
Security
Performance
Quality
CLAUDE.md
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src-tauri/src/lib.rs`:
- Around line 804-833: Cache the shared client configuration created by
guarded_http_client behind a OnceLock so extra_ca_certificates and
client_identity are loaded only once. Remove the fixed builder timeout and apply
each caller’s requested deadline with RequestBuilder::timeout() in
fetch_url_bytes_blocking and resolve_url_redirect_blocking, preserving their
distinct timeout values while reusing the cached guarded client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c01ae8a-5c03-4250-900a-cc29ce4eb7c2
⛔ Files ignored due to path filters (1)
apps/geolibre-desktop/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
apps/geolibre-desktop/src-tauri/Cargo.tomlapps/geolibre-desktop/src-tauri/src/lib.rsdocs/architecture.md
…sphrase Address review feedback on the native mTLS change: - Build the SSRF-guarded HTTP client once and cache it behind a OnceLock instead of rebuilding it (re-reading and re-parsing the CA bundle and client certificate from disk) on every native fetch. Callers now set their own per-request deadline with RequestBuilder::timeout(), so the shared client keeps its connection pool and parsed TLS material across fetch and URL-resolve calls. - Surface GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD set without GEOLIBRE_HTTP_CLIENT_CERT as a clear error rather than silently discarding the passphrase. - Note in guarded_http_client that the SSRF guard is applied independent of the TLS backend, so it holds on the native-tls (PKCS#12) path too. Adds a unit test for the stray-passphrase guard.
|
|
||
| /// Load the mutual-TLS client identity named by [`HTTP_CLIENT_CERT_ENV`], if any. | ||
| fn client_identity() -> Result<Option<ClientIdentity>, String> { | ||
| let password = env::var(HTTP_CLIENT_CERT_PASSWORD_ENV).ok(); |
There was a problem hiding this comment.
Bug (medium confidence): env::var(...).ok() treats an explicitly-empty-string passphrase the same as "a passphrase is set." Deployment tooling (Docker/K8s env interpolation, .env templating like GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD=${SECRET:-}) commonly produces an empty-string value rather than leaving the var unset when no passphrase is configured. With the current code that:
- forces
client_cert_is_pkcs12down the PKCS#12 path even for a plain PEM cert, or - if
GEOLIBRE_HTTP_CLIENT_CERTisn't set either, trips the new "stray passphrase" error (client_cert_password_without_path) even though the user never intentionally set a passphrase.
Consider treating an empty string as "not set":
| let password = env::var(HTTP_CLIENT_CERT_PASSWORD_ENV).ok(); | |
| let password = env::var(HTTP_CLIENT_CERT_PASSWORD_ENV) | |
| .ok() | |
| .filter(|value| !value.is_empty()); |
There was a problem hiding this comment.
Good catch, fixed in ec6877a: the passphrase is now read with .filter(|v| !v.is_empty()), so an empty-string GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD (common from ${SECRET:-}-style interpolation) counts as unset and neither forces the PKCS#12 path nor trips the stray-passphrase error.
| fn guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| static CLIENT: std::sync::OnceLock<Result<reqwest::blocking::Client, String>> = | ||
| std::sync::OnceLock::new(); | ||
| CLIENT.get_or_init(build_guarded_http_client).clone() | ||
| } |
There was a problem hiding this comment.
Design note (medium confidence): caching the client (and its build error) in a OnceLock for the whole process lifetime means GEOLIBRE_HTTP_CA_CERT/GEOLIBRE_HTTP_CLIENT_CERT/_PASSWORD are effectively read exactly once, on the first fetch_url_bytes/resolve_url_redirect call. That's fine for the documented "static launch-time config" use case, but two consequences worth confirming are intended:
- A single bad/expired cert on the first call permanently poisons every subsequent fetch for the rest of the process's life (previously each call rebuilt the client, so a transient misconfiguration wasn't "sticky").
- Any future test that exercises
guarded_http_client()with different env-var setups in the same test binary (tests run in one process) will observe whatever got cached by whichever test happened to run first — there's no reset hook, so this is a bit of a landmine for later test authors even though the current tests only touch the pure helper functions.
There was a problem hiding this comment.
Both consequences are intended. The env-driven cert config is static launch-time configuration, so reading it once and caching (including a load error) is acceptable - a bad cert means the user fixes the config and relaunches, same as any other launch-time setting. On the test landmine: noted; the current tests only exercise the pure helpers (client_cert_is_pkcs12, client_cert_password_without_path) and never call guarded_http_client(), precisely to avoid depending on process-global cached state. If a future test needs to drive the cached client under different env, it should run as its own test binary or use a serial/reset harness.
| const HTTP_CLIENT_CERT_ENV: &str = "GEOLIBRE_HTTP_CLIENT_CERT"; | ||
| /// Passphrase for a PKCS#12 client certificate. Its presence also forces the | ||
| /// PKCS#12 code path for a client cert that lacks a recognised extension. | ||
| const HTTP_CLIENT_CERT_PASSWORD_ENV: &str = "GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD"; |
There was a problem hiding this comment.
Security note (low confidence): the PKCS#12 passphrase is read from a plaintext environment variable. On Linux/macOS that's readable by other processes running as the same user (e.g. via /proc/<pid>/environ) and may end up in process-manager logs or crash dumps more readily than a file. Not a new class of risk for this codebase, but since this is a fresh secret-handling surface it may be worth supporting a _FILE-suffixed alternative (read the passphrase from a file path) the way many tools do, so the secret isn't sitting directly in the environment.
There was a problem hiding this comment.
Fair point on the fresh secret surface. For this PR I kept the documented env-var mechanism (the approved scope), which is the same channel already used for other config here. A GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD_FILE variant (read the passphrase from a file, Docker-secrets style) is a sensible follow-up and easy to add on top of the current client_identity() - happy to do it in a follow-up if the maintainer wants it, keeping this PR focused.
Code reviewOverview: Adds OS trust-store parity ( Bugs
Security
Performance
Quality
CLAUDE.md
Also reviewed and found correct: root-cert/identity ordering relative to |
Env interpolation in Docker/K8s/.env tooling (e.g. PASSWORD=${SECRET:-})
commonly yields an empty string rather than leaving the variable unset. An empty
GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD now counts as no passphrase, so it neither
forces the PKCS#12 code path for a PEM certificate nor trips the
stray-passphrase error.
|
|
||
| /// Load extra CA certificate(s) named by [`HTTP_CA_CERT_ENV`], if any. | ||
| fn extra_ca_certificates() -> Result<Vec<reqwest::Certificate>, String> { | ||
| let Some(path) = env::var_os(HTTP_CA_CERT_ENV) else { |
There was a problem hiding this comment.
Bug (high confidence): env::var_os returns Some("") when GEOLIBRE_HTTP_CA_CERT is set but empty — exactly the Docker/K8s/.env templating scenario (CA_CERT=${SECRET:-}) that the very next commit in this PR fixes for GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD. An empty path here isn't filtered, so it falls through to fs::read(""), which fails, and build_guarded_http_client()'s error propagates up.
Because guarded_http_client() caches its Result in a OnceLock (line 838-840), that failure is permanent for the process lifetime — it breaks every native fetch (fetch_url_bytes, resolve_url_redirect) app-wide, not just requests that need the extra CA. A stray empty env var turns into a total native-fetch outage rather than a no-op.
| let Some(path) = env::var_os(HTTP_CA_CERT_ENV) else { | |
| let Some(path) = env::var_os(HTTP_CA_CERT_ENV).filter(|value| !value.is_empty()) else { |
There was a problem hiding this comment.
Fixed in d56b453: extra_ca_certificates() now uses env::var_os(HTTP_CA_CERT_ENV).filter(|v| !v.is_empty()), so a set-but-empty CA path is treated as unset instead of failing on fs::read("") and poisoning the cached client.
| let password = env::var(HTTP_CLIENT_CERT_PASSWORD_ENV) | ||
| .ok() | ||
| .filter(|value| !value.is_empty()); | ||
| let Some(path) = env::var_os(HTTP_CLIENT_CERT_ENV) else { |
There was a problem hiding this comment.
Bug (high confidence): same empty-string gap as extra_ca_certificates() (see other comment), here for GEOLIBRE_HTTP_CLIENT_CERT. env::var_os treats a set-but-empty value as Some(""), so:
- It bypasses the
client_cert_password_without_pathcheck right below (sincehas_cert_pathbecomes effectively "true"), silently defeating the misconfiguration guard this PR just added for the analogous case. - It then fails later at
fs::readwith an opaque "No such file or directory" on an empty path, instead of the clear "cert path is not set" message. - Per the sibling comment, that error is cached forever in the
OnceLock-backed client, so this permanently breaks all native fetches, not just the mTLS ones.
| let Some(path) = env::var_os(HTTP_CLIENT_CERT_ENV) else { | |
| let Some(path) = env::var_os(HTTP_CLIENT_CERT_ENV).filter(|value| !value.is_empty()) else { |
There was a problem hiding this comment.
Fixed in d56b453 with the same .filter(|v| !v.is_empty()) on HTTP_CLIENT_CERT_ENV, so an empty cert path no longer bypasses the stray-passphrase guard or fails later on an empty path.
| fn guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| static CLIENT: std::sync::OnceLock<Result<reqwest::blocking::Client, String>> = | ||
| std::sync::OnceLock::new(); | ||
| CLIENT.get_or_init(build_guarded_http_client).clone() | ||
| } | ||
|
|
||
| fn build_guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| // The SSRF guard (GuardedDnsResolver + redirect re-validation) is applied | ||
| // here, independent of the TLS backend chosen below, so it holds on both the | ||
| // rustls and native-tls paths. | ||
| let mut builder = reqwest::blocking::Client::builder() | ||
| .connect_timeout(Duration::from_secs(REMOTE_TILE_CONNECT_TIMEOUT_SECS)) | ||
| .redirect(guarded_redirect_policy()) | ||
| .dns_resolver(std::sync::Arc::new(GuardedDnsResolver)) | ||
| .user_agent("GeoLibre Desktop") | ||
| .user_agent("GeoLibre Desktop"); | ||
|
|
||
| for certificate in extra_ca_certificates()? { | ||
| builder = builder.add_root_certificate(certificate); | ||
| } | ||
|
|
||
| builder = match client_identity()? { |
There was a problem hiding this comment.
Security (medium confidence): guarded_http_client() is a single process-wide singleton used by both fetch_url_bytes and resolve_url_redirect for any URL the app fetches natively — arbitrary tile/style/OGC hosts a user adds, not only the intended mTLS endpoint. Once GEOLIBRE_HTTP_CLIENT_CERT is configured, this same client (with the identity baked in at build time) is reused for every native request, so it will present the configured client certificate during the TLS handshake to any HTTPS server that issues a CertificateRequest — including unrelated public tile/style hosts a user points the app at, not just the enterprise host the cert was meant for.
TLS client certs are only sent when the peer asks for one, so this isn't an unconditional leak, but it does mean an enterprise client identity configured for one host can be exposed to any other HTTPS server the app talks to that happens to request a client cert. Worth at least calling out in the docs, or consider whether the identity can be scoped to the configured host.
There was a problem hiding this comment.
Good call. Documented in d56b453: the docs now note that the configured client certificate is held on the shared native client and is presented to any HTTPS server that requests one (not only the enterprise endpoint), so it should only be configured when the app's hosts are trusted. Per-host scoping (an allowlist of hosts the identity may be sent to) is a reasonable follow-up if needed, but that is a larger change than this PR's scope.
Code reviewReviewed the native mTLS/OS-trust-store change ( Bugs
Security
Performance
Quality
CLAUDE.md
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/geolibre-desktop/src-tauri/src/lib.rs (2)
3115-3137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for empty-passphrase normalization.
The new
.filter(|value| !value.is_empty())behavior is not exercised by the added tests, which only test downstream boolean helpers. Add a focused test for the empty environment value, or extract password normalization into a pure helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src-tauri/src/lib.rs` around lines 3115 - 3137, The tests cover downstream helpers but not the new empty-passphrase normalization. Add a focused regression test around the environment-password normalization flow, such as the relevant configuration-loading function, asserting that an empty environment value is treated as absent; alternatively extract the normalization into a pure helper and test that helper directly.
803-819: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPKCS#12 path drops the bundled Mozilla roots
The native-tls branch still gets the OS store and
GEOLIBRE_HTTP_CA_CERT, but it no longer uses the bundled Mozilla roots documented forguarded_http_client. PKCS#12 users can see different server certificate acceptance than the PEM/rustls path; keep the same CA set on both paths or avoid switching backends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src-tauri/src/lib.rs` around lines 803 - 819, Update the PKCS#12 handling in the client-identity flow around client_cert_is_pkcs12 and reqwest::Identity::from_pkcs12_der so the native-tls path preserves the bundled Mozilla roots used by guarded_http_client, while retaining the OS store and GEOLIBRE_HTTP_CA_CERT behavior. Ensure PKCS#12 and PEM/rustls paths use the same effective CA set, or avoid the backend switch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src-tauri/src/lib.rs`:
- Around line 780-786: Update the password retrieval near
HTTP_CLIENT_CERT_PASSWORD_ENV to distinguish env::VarError::NotUnicode from
VarError::NotPresent: treat only missing or empty values as unset, and return a
configuration error for non-UTF-8 values before certificate-path selection and
stray-passphrase validation proceed.
---
Outside diff comments:
In `@apps/geolibre-desktop/src-tauri/src/lib.rs`:
- Around line 3115-3137: The tests cover downstream helpers but not the new
empty-passphrase normalization. Add a focused regression test around the
environment-password normalization flow, such as the relevant
configuration-loading function, asserting that an empty environment value is
treated as absent; alternatively extract the normalization into a pure helper
and test that helper directly.
- Around line 803-819: Update the PKCS#12 handling in the client-identity flow
around client_cert_is_pkcs12 and reqwest::Identity::from_pkcs12_der so the
native-tls path preserves the bundled Mozilla roots used by guarded_http_client,
while retaining the OS store and GEOLIBRE_HTTP_CA_CERT behavior. Ensure PKCS#12
and PEM/rustls paths use the same effective CA set, or avoid the backend switch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7876715-7386-4938-8355-4e02b7d2fbab
📒 Files selected for processing (1)
apps/geolibre-desktop/src-tauri/src/lib.rs
…sphrase
More review follow-ups on the native mTLS config:
- Filter set-but-empty GEOLIBRE_HTTP_CA_CERT and GEOLIBRE_HTTP_CLIENT_CERT the
same way as the passphrase, so ${SECRET:-}-style empty values are treated as
unset instead of read as the path "" (which would fail on fs::read and, since
the guarded client is cached, permanently break every native fetch).
- Surface a non-UTF-8 GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD as a clear error rather
than silently dropping it, since the PKCS#12 loader takes a &str passphrase.
- Document that the configured client certificate is held on the shared native
HTTP client and is presented to any host that requests one during the TLS
handshake, so it should only be configured when the app's hosts are trusted.
| fn guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| static CLIENT: std::sync::OnceLock<Result<reqwest::blocking::Client, String>> = | ||
| std::sync::OnceLock::new(); | ||
| CLIENT.get_or_init(build_guarded_http_client).clone() | ||
| } |
There was a problem hiding this comment.
Robustness (medium confidence): guarded_http_client() caches an Err in the OnceLock forever once build_guarded_http_client() fails. Since this same client backs plain (non-mTLS) tile fetches and URL resolution (fetch_url_bytes_blocking, resolve_url_redirect_blocking), a misconfiguration that has nothing to do with a user's intent to use mTLS — e.g. a stray non-UTF‑8 byte in GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD left over in a shell profile, with GEOLIBRE_HTTP_CLIENT_CERT never set — will now permanently break all native tile/URL fetching for the entire process lifetime (see the NotUnicode branch in client_identity() around line 791, which errors unconditionally before even checking whether a cert path was configured).
Previously this failure mode didn't exist at all, since there was no cert/CA config to misconfigure. Given the cache-forever design (intentional per the "perf" commit), it might be worth at least logging clearly which env var caused the failure so it's diagnosable, or considering whether an env-var-driven mTLS misconfiguration should really take down basic tile loading rather than degrading gracefully (e.g. falling back to no client cert with a warning).
| # rustls-tls stays the default backend for every request. rustls-tls-native-roots | ||
| # also trusts the OS/system certificate store (enterprise CAs), and native-tls is | ||
| # used only on the opt-in mTLS path for PKCS#12 client certificates (issue #1220). | ||
| reqwest = { version = "0.12", default-features = false, features = ["blocking", "rustls-tls", "rustls-tls-native-roots", "native-tls"] } |
There was a problem hiding this comment.
Quality (low-medium confidence, already flagged by the author in the PR description): native-tls and rustls-tls-native-roots are enabled unconditionally, so every desktop build — including Linux, where native-tls pulls in openssl/openssl-sys and links libssl — now carries this dependency even for the overwhelming majority of users who will never set GEOLIBRE_HTTP_CLIENT_CERT_PASSWORD to use a PKCS#12 identity. That's a real increase in build complexity (system OpenSSL dependency on Linux) and attack surface (an extra TLS stack) for a feature most installs won't use. Worth a deliberate call on whether to gate native-tls behind a Cargo feature flag (off by default) or accept the always-on cost, rather than defaulting it on silently.
| fn build_guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| // The SSRF guard (GuardedDnsResolver + redirect re-validation) is applied | ||
| // here, independent of the TLS backend chosen below, so it holds on both the | ||
| // rustls and native-tls paths. | ||
| let mut builder = reqwest::blocking::Client::builder() | ||
| .connect_timeout(Duration::from_secs(REMOTE_TILE_CONNECT_TIMEOUT_SECS)) | ||
| .redirect(guarded_redirect_policy()) | ||
| .dns_resolver(std::sync::Arc::new(GuardedDnsResolver)) | ||
| .user_agent("GeoLibre Desktop") | ||
| .user_agent("GeoLibre Desktop"); | ||
|
|
||
| for certificate in extra_ca_certificates()? { | ||
| builder = builder.add_root_certificate(certificate); | ||
| } | ||
|
|
||
| builder = match client_identity()? { | ||
| // PKCS#12 identities are only understood by native-tls, which also reads | ||
| // the OS trust store on every platform; switch this one client over. | ||
| Some(ClientIdentity::Pkcs12(identity)) => builder.use_native_tls().identity(identity), | ||
| Some(ClientIdentity::Pem(identity)) => builder.use_rustls_tls().identity(identity), | ||
| None => builder.use_rustls_tls(), | ||
| }; | ||
|
|
||
| builder | ||
| .build() | ||
| .map_err(|error| format!("Could not create HTTP client: {error}")) | ||
| } |
There was a problem hiding this comment.
Minor nits (low confidence, non-blocking):
-
No default request timeout on the shared client. Previously
guarded_http_client(timeout)baked a total-request timeout into the client itself. Nowbuild_guarded_http_client()only setsconnect_timeout; the two current call sites (fetch_url_bytes_blocking,resolve_url_redirect_blocking) correctly chain.timeout(...)on theRequestBuilder, but there's no client-level fallback. A future caller ofguarded_http_client()that forgets to set a per-request timeout would have no read/total timeout at all (only the connect phase is bounded), and could hang indefinitely on a slow/stalled server. -
Test coverage gap.
classifies_client_cert_format/flags_passphrase_without_certificate_pathcover the extracted pure predicates well, but the actual env-var plumbing inclient_identity()/extra_ca_certificates()— empty-string-treated-as-unset, and the non-UTF-8 password error path — has no test exercising the realenv::var/env::var_osreads (understandably, since mutating process env in parallel tests is racy). Consider at least a#[test]gated to run serially, or an internal helper that takes the env lookups as parameters so it's testable without touching real process env.
Code reviewReviewed the native Rust HTTP client changes (OS trust store + mTLS client certs) in Bugs
Security
Performance
Quality
CLAUDE.md
|
| fn guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| static CLIENT: std::sync::OnceLock<Result<reqwest::blocking::Client, String>> = | ||
| std::sync::OnceLock::new(); | ||
| CLIENT.get_or_init(build_guarded_http_client).clone() | ||
| } |
There was a problem hiding this comment.
Bugs / Robustness (medium confidence): Caching a failed build in the OnceLock means a single bad mTLS/CA env-var value (wrong passphrase, corrupt PEM, stale path) permanently breaks every native fetch for the life of the process — not just requests that need the client cert. Tile loading, URL resolution, and OGC GetCapabilities calls that have nothing to do with mTLS will all fail with a certificate-loading error message once this is cached, until the app is restarted with a fixed/unset env var.
Before this PR there was no cert config, so this single point of failure is new. Consider building the base client independently of extra_ca_certificates()/client_identity(), and only failing (or falling back) the requests that actually need the extra trust/identity — or at minimum surface a one-time startup diagnostic so a misconfigured cert doesn't read as "tile loading is broken."
| fn build_guarded_http_client() -> Result<reqwest::blocking::Client, String> { | ||
| // The SSRF guard (GuardedDnsResolver + redirect re-validation) is applied | ||
| // here, independent of the TLS backend chosen below, so it holds on both the | ||
| // rustls and native-tls paths. | ||
| let mut builder = reqwest::blocking::Client::builder() | ||
| .connect_timeout(Duration::from_secs(REMOTE_TILE_CONNECT_TIMEOUT_SECS)) | ||
| .redirect(guarded_redirect_policy()) | ||
| .dns_resolver(std::sync::Arc::new(GuardedDnsResolver)) | ||
| .user_agent("GeoLibre Desktop") | ||
| .user_agent("GeoLibre Desktop"); |
There was a problem hiding this comment.
Quality (low-medium confidence): The client builder no longer sets a default .timeout(...); a request timeout is now opt-in per call site via RequestBuilder::timeout(). Both current callers (fetch_url_bytes_blocking, resolve_url_redirect_blocking) do set it, so there's no active bug, but the shared client has no backstop timeout anymore — a future caller that forgets .timeout() gets unbounded read/write time (only connect_timeout still applies), risking an indefinite hang instead of a clear timeout error. Worth a comment at the call sites, or a fallback default on the builder, so this can't be silently reintroduced.
| The configured client certificate is held on the shared native HTTP client and | ||
| is therefore **presented to any HTTPS server that requests one** during a native | ||
| fetch (tile, style, and OGC hosts a project points at), not only the endpoint the | ||
| certificate was issued for. TLS sends a client certificate only when the server | ||
| asks for one, so this is not an unconditional disclosure, but configure a client | ||
| certificate only when the hosts the app talks to are trusted. |
There was a problem hiding this comment.
Security (medium confidence, already partly disclosed here): Because the client certificate lives on the single shared guarded_http_client, it's presented to any HTTPS host that asks for one during a native fetch — including arbitrary tile/style/OGC URLs a user (or a shared .geolibre.json project, or a plugin) can point the app at. This is a confused-deputy-style risk: once mTLS is configured for a trusted enterprise endpoint, an attacker-controlled tile URL added to a project can also elicit the org's client certificate during its own TLS handshake, since nothing scopes cert presentation to a specific host/host-pattern.
The doc is honest about this, which is good, but given GeoLibre's data flow explicitly allows arbitrary remote tile/service URLs (see CLAUDE.md's Add Data flow), it may be worth scoping the client identity to an allow-listed set of hosts rather than presenting it on every native request.
Code reviewReviewed the diff ( Bugs
Security
Performance
Quality
CLAUDE.md
|
Summary
Addresses #1220. The desktop app's native Rust HTTP client trusted only the bundled Mozilla roots and could not present a client certificate, so remote fetches issued from the native process (tile/URL resolution, OGC GetCapabilities, and similar) failed against enterprise endpoints behind a private CA or mutual TLS (mTLS), even though the WebView
fetchpath succeeded (it uses the OS trust store and an interactive certificate prompt).What this delivers
OS trust store parity (server verification).
guarded_http_clientnow trusts the OS/system certificate store in addition to the bundled roots (rustls-tls-native-roots), so a server signed by an enterprise CA installed on the machine is accepted with no configuration. The existing SSRF guard (GuardedDnsResolver, redirect re-validation) is unchanged.Client certificates for mTLS (config-based). When configured via environment variables, the native client presents a client certificate:
GEOLIBRE_HTTP_CA_CERTGEOLIBRE_HTTP_CLIENT_CERT.pem(chain + unencrypted PKCS#8 key) or a PKCS#12.p12/.pfx.GEOLIBRE_HTTP_CLIENT_CERT_PASSWORDPEM identities use the default rustls backend; PKCS#12 identities (which is what Windows exports and what carries a passphrase) switch that one client to the platform native-tls backend (SChannel / Secure Transport / OpenSSL), which also reads the OS trust store.
Why not the interactive prompt
The reporter's preferred solution is an interactive OS certificate prompt like the WebView shows. That prompt is driven by WebView2/SChannel; reqwest exposes no interactive OS client-cert selection on any backend. True prompt parity would require custom Windows SChannel
unsafecode. This PR implements the reporter's documented secondary option (config-based certs) instead, which is the mechanism the native path can actually support. See the issue comment for the full assessment.Note on dependencies
Enabling the
native-tlsfeature (for PKCS#12 + passphrase) adds a platform TLS dependency: SChannel on Windows and Secure Transport on macOS (both OS-provided), and OpenSSL on Linux. Linux desktop builds therefore linklibssl. If you would rather avoid the Linux OpenSSL dependency, I can dropnative-tlsand support PEM client certs only (users convert.p12with a one-lineopensslcommand); happy to adjust.Verification
Cannot exercise real enterprise mTLS here (Linux, no mTLS endpoint), so I verified the exact reqwest plumbing end to end against a local
openssl s_serverrequiring a client certificate:Plus a unit test for the PEM-vs-PKCS#12 classification, and
cargo test(16 pass),cargo fmt --check,cargo check, andpre-commitall green. The reporter offered to verify on real Windows enterprise mTLS, which is the remaining validation this can't cover here, so I left the issue open rather than auto-closing.Docs: added a "Native HTTP trust store and mutual TLS" section to
docs/architecture.md.Summary by CodeRabbit