Repository navigation
s10: connection-string parser, TLS trust, timeouts, secret hardening, bulk & ergonomics - #451
Conversation
|
A heads-up on the connection-string parsing change in here (context: #313). The One thing worth a second opinion: I've made it a superset of ADO.NET — the documented ADO.NET rules, plus 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. |
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.
67b4529 to
1f6dec6
Compare
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).
1f6dec6 to
333c620
Compare
`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.
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
secrecy::SecretString— wrapped, zeroized, kept out ofDebug.encrypt=truedue to TLS feature flags disablement #305 — no silent plaintext: on a no-TLS build,encrypt=true/strictnow errors instead of quietly sending plaintext.TLS / certificates
trust_cert_ca(+ multi-cert files), newtrust_cert_ca_bundle, and feature-gatedtrust_webpki_roots(); one loader across all three backends, fail-closed on zero usable certs.Connection strings / config
Error: Conversion error: Conversion error: Key-value pairs must be separated by a ;#313 — in-house ADO.NET connection-string parser (replaces the unmaintainedconnection-stringcrate for ADO): splits each pair on the first=, so base64/=-padded passwords parse; full special-char matrix + server-gated end-to-end login test. Parsed as an ADO.NET superset (see maintainer note).handshake_timeoutandcommand_timeoutso a stalled server surfaces a timeout instead of hanging.Ergonomics / bulk
Row::builder().Quality: internal-audit comment cleanups (no behavior change); unit tests green across the matrix; fmt + clippy (
--features=alland 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.