Skip to content

s10: connection-string parser, TLS trust, timeouts, secret hardening, bulk & ergonomics - #451

Merged
MattJackson merged 13 commits into
stack/s9from
stack/s10
Sep 24, 2026
Merged

MattJackson merged 13 commits into
stack/s9from
stack/s10

Conversation

@MattJackson

Copy link
Copy Markdown
Contributor

Tenth slice of the stack (stacked on #449 / stack/s9); each fix is its own commit referencing the issue it closes. Unit tests run in every feature lane; new integration coverage runs in the SQL Server lanes.

Security / credential hygiene

TLS / certificates

Connection strings / config

Ergonomics / bulk

Quality: internal-audit comment cleanups (no behavior change); unit tests green across the matrix; fmt + clippy (--features=all and no-default) clean; MSRV 1.88.

Note (maintainers): the #313 parser is an ADO.NET superset — it keeps {…} brace quoting as an extension. A separate note asks whether to go strict-ADO instead.

@MattJackson

Copy link
Copy Markdown
Contributor Author

A heads-up on the connection-string parsing change in here (context: #313).

The connection-string crate we depend on looks unmaintained (0.2.0) and seems to diverge from ADO.NET on a fairly common case: it appears to reject an unquoted = inside a value, so base64/generated passwords with = padding don't parse. I've made a change to parse ADO strings ourselves, splitting each pair on the first = the way ADO.NET seems to. (JDBC still uses the crate for now.)

One thing worth a second opinion: I've made it a superset of ADO.NET — the documented ADO.NET rules, plus {...} brace quoting as an extension (the old crate supported braces, and we document/test them). The alternative would be strict ADO.NET, which would drop {...} braces (they're really an ODBC-ism, not SqlClient). Concretely, "strict" would change one thing: a value that begins with { is currently read as brace-quoted, whereas strict ADO.NET would treat the braces as literal characters.

I'd lean toward the superset since it seems backward-compatible and addresses #313, but I'm happy to go strict and drop braces if people prefer — wanted to flag it rather than decide it myself. We added tests across the special characters; it feels more solid, though I won't claim it's airtight.

MattJackson and others added 11 commits September 20, 2026 09:58
Adds the `secrecy` crate (minimal, default features) so in-memory
credentials can be stored as `SecretString`, which bundles
zeroize-on-drop, redacted `Debug`, and `expose_secret()`-gated access.
Migrate every in-memory credential-storage site from
zeroize::Zeroizing<String> to secrecy::SecretString, which combines
zeroize-on-drop, a redacted Debug ([REDACTED]), and expose_secret()-gated
plaintext access so a secret can no longer be read or logged by accident.

Sites migrated:
- client/auth.rs: SqlServerAuth.password, WindowsAuth.password and
  AuthMethod::AADToken now hold SecretString. Hand-written redacting Debug
  impls are dropped in favour of derive(Debug); PartialEq/Eq are
  hand-written (SecretString has no PartialEq) so the public
  AuthMethod: Eq bound and its equality behaviour are preserved.
- client/config.rs: ClientCertSource::Pkcs12.password now holds
  SecretString; its manual redacting Debug is replaced by derive(Debug).
- tds/codec/login.rs: LoginMessage.password and FedAuthExt.fed_auth_token
  now hold SecretString, exposed only where their bytes are written into
  the LOGIN7 buffer. FedAuthExt loses its lifetime (all fields now owned).
  Test-only PartialEq/Eq are hand-written for the encode/decode
  round-trips. The Zeroizing<Box<[u8]>> encoded buffer is left unchanged.
- client/connection.rs: the SqlServer, AADToken and unix/windows Windows
  credential handoffs expose the secret only at the boundary where an
  external API needs plaintext.
- tls_stream/{native_tls,opentls}: expose the PKCS#12 password only for
  the from_pkcs12 decryption call.

Freed-plaintext-leak fix: SecretString::from(String) routes through
String::into_boxed_str(), which reallocates and frees the source buffer
WITHOUT zeroizing when the string has spare capacity, leaving a
recoverable plaintext copy in freed heap. A pub(crate) helper
`secret_from_string` in auth.rs (reachable via `pub(crate) mod auth`)
copies the bytes into an exact-sized Box<str> and wipes the original
before wrapping. Every String-to-SecretString conversion routes through
it. The redaction marker is unified on secrecy's [REDACTED].

Public constructors keep their impl Into<String>/ToString signatures and
wrap into SecretString internally.

Tests: adapt auth/config/login tests to expose_secret; add tests
asserting Debug never leaks the secret for the SqlServer, Windows, AAD,
connection-password, PKCS#12, login-password and fed-auth-token paths and
shows [REDACTED]; that expose_secret() yields the original value; that
secret_from_string preserves the value for the spare-capacity shape; and
a compile-time proof that the stored credential type is ZeroizeOnDrop.
Adds SqlBulkCopyOptions (TABLOCK, CHECK_CONSTRAINTS, KEEP_NULLS,
FIRE_TRIGGERS, KEEP_IDENTITY) and ORDER hints via
Client::bulk_insert_with_options; identity columns are retained only
when KEEP_IDENTITY is set. Closes #302.

Co-authored-by: Adrian Ehrsam <adrian.ehrsam@bmsuisse.ch>
Config::lossy_utf16_decoding replaces invalid UTF-16 in NVARCHAR/NCHAR
and NTEXT values with U+FFFD; strict decoding remains the default and
framing guards stay unconditional. Closes #325.

Co-authored-by: Skyler <1156263951@qq.com>
…330)

Introduces a two-axis trust model (root source + additive extra CAs +
bypass): trust_cert_ca now accumulates and accepts multi-cert files,
new trust_cert_ca_bundle takes in-memory PEM/DER bundles, and new
trust_webpki_roots (rustls-webpki-roots feature) uses bundled Mozilla
roots. Cert loading is unified across all three TLS backends. Closes #290, #330.

Co-authored-by: main() <main@ehvag.de>
Co-authored-by: zlepper <rhdh@digizuite.com>
When compiled without any TLS backend (none of the rustls, native-tls or
vendored-openssl features), the connection-string parser silently ignored
an explicit encryption request: the no-TLS ConfigString::encrypt() returned
Ok(EncryptionLevel::NotSupported) unconditionally. As a result encrypt=true
(or strict) resolved to NotSupported, the check_tls_backend_available guard
never fired, and traffic went out in plaintext with no signal to the user.

Classify the encrypt token the same way the with-TLS parser does and turn an
explicit encryption-on request (true/yes/strict) into a clear Error::Tls at
parse time, matching the wording and variant of check_tls_backend_available.
Opting out (false/no/DANGER_PLAINTEXT) and an omitted keyword still resolve
to NotSupported, so existing no-TLS users are unaffected. Unrecognized tokens
(e.g. mandatory) keep the with-TLS parser's bad-boolean error for consistency.

Adds no-TLS-gated unit tests for both the ADO.NET and JDBC parsers covering
the on/strict/mandatory error cases and the off/plaintext/missing ok cases.
Follow-up to the s10 code audit. Comment-only cleanups, no code or test
behavior changes:

- Drop dangling "Rule N" cross-references (21 sites across the TLS
  backends, certs.rs and config.rs). They referenced a numbered rule
  list that exists in no shipped source or doc; each comment already
  states its own invariant, so the numbering only added drift risk.
- Remove the duplicated `azure-sql-edge on macOS` debugging anecdote
  from the two handshake-timeout doc comments (connection.rs, config.rs)
  in favour of the general failure class (a server that accepts TCP then
  stalls mid-handshake).
- Rewrite a bulk-identifier test comment that narrated a prior bug
  ("used to slip through saturating_sub") to state the current invariant.
- Replace inline "pre-0.13 ..." version asides in the trust-config and
  timeout docs with plain descriptions of current behavior; the release
  migration notes already live in CHANGELOG.md.
The `connection-string` 0.2.0 crate is unmaintained and diverges from
ADO.NET: its ADO tokenizer requires a `;` after a value, so a value that
itself contains `=` (for example a base64 or otherwise generated password
with `=` padding, `Password=Zm9vYmFy==`) fails to parse with
"Key-value pairs must be separated by a `;`" — the exact symptom reported
in #313. Reproduced across the full set of ASCII special characters, not
just `=`.

Parse ADO.NET connection strings in-house instead (a new hand-written
tokenizer, not a fork of the crate), as a documented superset of ADO.NET:

- split each pair on the FIRST `=`, so a value may contain further `=`
  (matching ADO.NET and fixing the base64-password case);
- `'…'` / `"…"` quoting with `''` / `""` doubling to embed the quote;
- whitespace preserved inside quotes, trimmed when unquoted (ADO.NET);
- `{…}` brace quoting kept as an explicit extension (not ADO.NET);
- ASCII only; a `==` in the key position is a literal `=`;
- duplicate keys are last-wins.

JDBC parsing continues to use the crate for now. Errors keep the existing
actionable quoting hint.

Tests:
- parser-level unit tests over keys, pairs, quoting, braces, errors, and an
  exhaustive sweep of every printable-ASCII character round-tripping through
  single and double quotes and (where legal) unquoted;
- auth-level matrix in `ado_net.rs` over every special char via both quote
  styles, doubled-quote embedding, the #313 base64 cases, and the ambiguous
  inputs that must error;
- a server-gated end-to-end test (`tests/special_char_password.rs`) that
  creates a login with a `=`-padded / `;` / braced password and authenticates
  as it, covering the whole parse -> LOGIN7 -> auth path in the CI
  integration lanes.

Updates two special-character tests from #333 to the ADO.NET-aligned
behavior (whitespace preserved in quotes; trailing junk after a braced value
is rejected rather than folded in).
`connect_with_full_encryption` (and its `ENCRYPTED_CONN_STR`) use
`encrypt=true`, which since #305 is a hard error on a build with no TLS
backend rather than silently degrading to plaintext. Gate both behind a TLS
feature so the `--no-default-features` integration lanes pass; full-encryption
coverage still runs in the rustls / native-tls / vendored-openssl lanes.
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.

1 participant