Skip to content

KDB-155: fix: cross-platform NTLM + Windows root store for vendored-openssl - #1

Merged
kihm-keeper merged 4 commits into
mainfrom
fix/windows-root-store-vendored-openssl
Jul 27, 2026
Merged

KDB-155: fix: cross-platform NTLM + Windows root store for vendored-openssl#1
kihm-keeper merged 4 commits into
mainfrom
fix/windows-root-store-vendored-openssl

Conversation

@kihm-keeper

Copy link
Copy Markdown
Collaborator

Summary

  • Cross-platform NTLM (AuthMethod::Windows): Upstream gates NTLM behind cfg(windows), but the underlying winauth NTLMv2 client is pure Rust. This lifts the cfg(windows) requirement to cfg(feature = "winauth") so DOMAIN\user logins work from Linux/macOS servers. The Windows-native SSPI path (NtlmSspiBuilder) stays gated to cfg(all(windows, ...)).
  • Windows ROOT store loading for vendored-openssl: openssl-probe only checks Unix filesystem paths for CA bundles — on Windows the trust anchors live in registry-backed certificate stores, so the probe finds nothing and TrustConfig::Default validates against an empty trust store. This loads the Windows ROOT certificate store (current-user composite view) into the OpenSSL connector via the schannel crate, scoped to cfg(windows) + vendored-openssl feature + TrustConfig::Default only. Unparseable certs are skipped with a warning; a failure to open the store degrades to existing behavior (empty trust store).

Motivation

KeeperDB uses tiberius with vendored-openssl + winauth for its MSSQL driver. Two problems:

  1. NTLM auth only compiled on Windows, but KeeperDB runs on Linux/macOS servers connecting to SQL Server with domain credentials.
  2. Entra (AadToken) auth to Azure SQL from Windows desktop builds always failed with "unable to get local issuer certificate" because the vendored OpenSSL had zero trusted roots.

Changes

File What
Cargo.toml winauth moved out of cfg(windows) deps; schannel added as optional cfg(windows) dep wired into vendored-openssl; version bumped to 0.12.3
src/client/auth.rs cfg(all(windows, feature = "winauth"))cfg(feature = "winauth") on WindowsAuth, Debug impl, AuthMethod::Windows
src/client/connection.rs TokenSspi import + flush_sspi + Windows match arm cfg gates widened; NextBytes import separated from Windows-only NtlmSspiBuilder
src/tds/codec/login.rs integrated_security() cfg adds feature = "winauth"
src/tds/codec/token/token_sspi.rs TokenSspi::new() cfg adds feature = "winauth"
src/tds/context.rs spn() cfg adds feature = "winauth"
src/tds/stream/token.rs flush_sspi() cfg adds feature = "winauth"
src/client/tls_stream/opentls_tls_stream.rs Windows ROOT store loading in TrustConfig::Default branch

Blast radius

Gate Effect
#[cfg(windows)] on root store code macOS/Linux builds unchanged
vendored-openssl feature native-tls and rustls backends untouched
TrustConfig::Default match arm only TrustAll and CaCertificateLocation paths untouched
cfg(feature = "winauth") on NTLM Only active when feature is opted in

Test plan

  • cargo check --no-default-features --features tds73,winauth,vendored-openssl,chrono,rust_decimal on macOS — compiles; Windows block cfg'd out
  • schannel usage cross-checked against x86_64-pc-windows-msvc target — compiles
  • KeeperDB Windows desktop build: Entra connect to Azure SQL succeeds (merge gate)
  • KeeperDB macOS: SQL-auth + NTLM connections unaffected (smoke)

craiglurey and others added 3 commits July 13, 2026 17:05
The winauth crate's NtlmV2Client is pure Rust (its own MD4/HMAC-MD5/RC4,
no SSPI) and works on every platform; only its 'windows' SSPI module is
Windows-specific. Lift the cfg(windows) gates so explicit-credential
Windows authentication (DOMAIN\user + password over NTLMv2) is available
from Linux and macOS clients, matching what FreeTDS, jTDS, and pytds
have long supported.

- Cargo.toml: winauth becomes a cross-platform optional dependency
- AuthMethod::Windows / WindowsAuth / AuthMethod::windows(): gate on
  feature = "winauth" alone
- login helpers (Context::spn, LoginMessage::integrated_security,
  TokenSspi::new, TokenStream/Connection::flush_sspi, NextBytes import):
  widened to include feature = "winauth"
- AuthMethod::Integrated is unchanged: SSPI on Windows, GSSAPI on Unix

Based on v0.12.3.
The v0.12.3 tag was cut without the version bump; crates.io has 0.12.3.
Matching it lets [patch.crates-io] consumers replace the registry crate.
The vendored OpenSSL discovers roots via openssl-probe, which only
checks Unix filesystem paths — on Windows it finds nothing, so
TrustConfig::Default validates against an empty trust store and every
strict-validation handshake fails with 'unable to get local issuer
certificate' (e.g. Azure SQL with AAD token auth).

Load the Windows ROOT certificate store (current-user view, a
composite that includes the local-machine store) into the connector
via schannel, the same crate rustls-native-certs uses for this.
Scoped to cfg(windows) + the vendored-openssl feature + the Default
trust branch; TrustAll, CaCertificateLocation, other TLS backends,
and non-Windows targets are unchanged.
@kihm-keeper kihm-keeper changed the title fix: cross-platform NTLM + Windows root store for vendored-openssl KDB-155: fix: cross-platform NTLM + Windows root store for vendored-openssl Jul 27, 2026
@pcwhite

pcwhite commented Jul 27, 2026

Copy link
Copy Markdown

Reviewed. No blockers. Merging this into main is the right call and it resolves the concern I raised on keeperdb#305 about pinning to a topic branch that could be deleted or force-pushed.

Worth stating what is and isn't new product risk here, because it's less than the diff size suggests.

Two of the three commits are already in production

40c5e67a (cross-platform NTLM) and f012b794 (version align) are what keeperdb's [patch.crates-io] already pins today, so they shipped in v2.3.0 and have real mileage. The only unproven product code in this PR is 912bbcb9.

The base change to main pulls in 6 upstream commits the current pin never had

This is the part that usually hides surprises, so I checked each one rather than assuming:

Commit Assessment
c34fab2e Bump v0.12.3 Matches the version this PR sets. No conflict.
51f0cbb3 retroactive v0.11.7/v0.11.8 CHANGELOG only.
406ad278 vec reallocations (tiberius-rs#370) Purely TokenRow::new() to TokenRow::with_capacity(n) in into_row.rs. Vec pre-allocation on the parameter-encoding path, semantically identical. No risk.
59db5796 libgssapi 0.8.1 (tiberius-rs#372) Touches connection.rs, but entirely inside cfg(all(unix, feature = "integrated-auth-gssapi")). KeeperDB does not enable that feature, so none of it compiles for us. Inert.
a5dffa0d remove SECURITY.md No product code.
a6b4fcda CI security workflow No product code.

So the upstream drift is benign. Its only overlap with this PR is connection.rs, and the two touch disjoint regions, which is consistent with GitHub reporting the PR as mergeable.

cfg-widening correctness

This is the class of bug I was actually looking for, since asymmetric cfg gates break exactly one feature combination and nothing else:

  • AuthMethod::Windows widening does not create a duplicate match arm on Windows. AuthMethod::Integrated (line 318, the SSPI path) and AuthMethod::Windows (line 387, explicit-cred NTLM) are distinct variants, so cfg(all(windows, feature = "winauth")) and cfg(feature = "winauth") coexist cleanly. This was my main compile-break worry and it's fine.
  • NtlmSspiBuilder correctly stays cfg(all(windows, feature = "winauth")) while NextBytes widens to cfg(feature = "winauth"). Both are used on Windows, neither goes unused.
  • flush_sspi has a single positively-gated definition with no paired not(any(...)) to fall out of sync. The nearby not(any(...)) is for post_login_encryption and the TLS backends, unrelated.

The root store fix

  • Scoped to cfg(windows) + vendored-openssl + TrustConfig::Default only, so TrustAll (SQL auth, NTLM, RDS) is untouched and macOS/Linux compile to identical code. That is why this supersedes the native-tls approach, and why the 6x macOS RDS trust-cert re-run I set as a gate on keeperdb#305 is no longer required.
  • schannel is MIT, verified against steffengy/schannel-rs and the registry, so it clears our dependency license rule.
  • Skipping unparseable certs with a WARN rather than failing the connection matches rustls-native-certs. Degrading to an empty trust store when the store can't be opened preserves today's behavior. Both are the right defaults.

Three asks, none blocking

  1. Merge with a merge commit or rebase, not squash. Squashing collapses 40c5e67a into a single commit under one author and drops Craig's authorship of the NTLM work.
  2. open_current_user("ROOT") deserves a comment. It's correct for what we ship on Windows (the desktop MSI), and the current-user view is a composite that picks up machine and GP-pushed roots such as Zscaler. It would be the wrong store for a Windows service build, which would silently get no roots. Cheap to note now while the reasoning is fresh.
  3. I cannot verify the Windows compile from here. Your x86_64-pc-windows-msvc cross-check is the only evidence on that path, and the real gate stays the KeeperDB Windows desktop build reaching Azure SQL over Entra.

After this merges

keeperdb#305 should become a pin bump only: keep the release dependency line as-is (default-features = false, tds73, vendored-openssl, winauth) and point [patch.crates-io] at the merge commit on main. The comment above that patch entry needs updating too, since it currently explains only the cfg(windows) NTLM lift and cites tiberius-rs#408. It should also mention the Windows root store and that the fork branch is now main.

… services

The current-user ROOT store is correct for user-session processes
(desktop app / MSI). A Windows service running in session 0 would
need open_local_machine("ROOT") instead. Add a comment while the
reasoning is fresh.
@kihm-keeper
kihm-keeper merged commit 781fb71 into main Jul 27, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants