feat: added sspi ntlm without kerberos using sspi-rs - #408
Open
carlvoller wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds optional Unix SSPI support via a new Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/config.rs (1)
313-319:⚠️ Potential issue | 🟠 MajorFix
integratedsecurityrouting to enable SSPI NTLM on Unix withsspi-rs.The current routing has two problems:
unix +
sspi-rsonly (nointegrated-auth-gssapi):integratedsecurityis ignored. Line 313–316 doesn't execute (requiresintegrated-auth-gssapi), so the request falls through to SQL auth at line 319, never reaching the NTLM handler atconnection.rs:392.unix + both
sspi-rsandintegrated-auth-gssapi: Line 315 unconditionally returnsAuthMethod::Integrated(Kerberos), preventing use of the NTLM path that exists atconnection.rs:392for credentials-based flows.Route
integratedsecurity=truewith credentials toAuthMethod::windows()for unix+sspi-rs, and reserveAuthMethod::Integratedfor unix+integrated-auth-gssapiwhen credentials are absent.Proposed fix
- #[cfg(feature = "integrated-auth-gssapi")] - Some(val) if val.to_lowercase() == "sspi" || Self::parse_bool(val)? => { - Ok(AuthMethod::Integrated) - } - - // Should sspi-rs take over the default behaviour here if enabled? + #[cfg(all(unix, feature = "sspi-rs"))] + Some(val) if val.to_lowercase() == "sspi" || Self::parse_bool(val)? => match (user, pw) { + (Some(user), Some(pw)) => Ok(AuthMethod::windows(user, pw)), + #[cfg(feature = "integrated-auth-gssapi")] + (None, None) => Ok(AuthMethod::Integrated), + _ => Err(crate::Error::Conversion( + "Connection string: uid/pwd required for SSPI NTLM on Unix".into(), + )), + }, + #[cfg(all(unix, feature = "integrated-auth-gssapi", not(feature = "sspi-rs")))] + Some(val) if val.to_lowercase() == "sspi" || Self::parse_bool(val)? => { + Ok(AuthMethod::Integrated) + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1111a8b5-b377-4996-a533-7ad5bcbe319a
📒 Files selected for processing (9)
Cargo.tomlsrc/client/auth.rssrc/client/config.rssrc/client/connection.rssrc/error.rssrc/tds/codec/login.rssrc/tds/codec/token/token_sspi.rssrc/tds/context.rssrc/tds/stream/token.rs
Comment on lines
+30
to
+33
| #[cfg_attr( | ||
| feature = "docs", | ||
| doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))) | ||
| )] |
There was a problem hiding this comment.
Incorrect doc attribute syntax — missing cfg() wrapper.
The doc(any(...)) syntax is invalid. It should be doc(cfg(any(...))) to properly display the "Available on..." badge in rustdoc. Compare with the existing Integrated variant at line 76 which correctly uses doc(cfg(any(...))).
This same issue appears at lines 41-44, 62-65, and 97.
Proposed fix for all occurrences
#[cfg_attr(
feature = "docs",
- doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs")))
+ doc(cfg(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))))
)]Apply the same pattern to lines 41-44, 62-65, and fix line 97:
- #[cfg_attr(feature = "docs", doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))))]
+ #[cfg_attr(
+ feature = "docs",
+ doc(cfg(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))))
+ )]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[cfg_attr( | |
| feature = "docs", | |
| doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))) | |
| )] | |
| #[cfg_attr( | |
| feature = "docs", | |
| doc(cfg(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs")))) | |
| )] |
… sspi-rs parsing in ConfigString
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/client/auth.rs (1)
62-65:⚠️ Potential issue | 🟡 MinorUse
doc(cfg(...))instead ofdoc(any(...))for rustdoc availability annotations.Line 64 and Line 97 still use
doc(any(...)); these should bedoc(cfg(any(...)))for correct rustdoc cfg badges.Proposed fix
#[cfg_attr( feature = "docs", - doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))) + doc(cfg(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs")))) )]- #[cfg_attr(feature = "docs", doc(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs"))))] + #[cfg_attr( + feature = "docs", + doc(cfg(any(all(windows, feature = "winauth"), all(unix, feature = "sspi-rs")))) + )]#!/bin/bash # Verify unresolved rustdoc cfg syntax in src/client/auth.rs. rg -nP 'doc\(\s*any\(' src/client/auth.rs rg -nP 'doc\(\s*cfg\(' src/client/auth.rsExpected result: the first command returns no matches after the fix.
Also applies to: 97-97
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea811eef-e961-4a7b-85d5-08f3f441fbab
📒 Files selected for processing (3)
src/client/auth.rssrc/client/config.rssrc/error.rs
Comment on lines
+170
to
+175
| #[cfg(all(unix, feature = "sspi-rs"))] | ||
| impl From<sspi::Error> for Error { | ||
| fn from(err: sspi::Error) -> Self { | ||
| Error::SspiRs(format!("{}", err)) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add doc cfg attribute for consistency with GSSAPI impl.
The From<libgssapi::error::Error> impl (lines 159-168) includes a #[cfg_attr(feature = "docs", doc(cfg(...)))] attribute, but this impl is missing it.
📚 Proposed fix to add doc cfg attribute
#[cfg(all(unix, feature = "sspi-rs"))]
+#[cfg_attr(
+ feature = "docs",
+ doc(cfg(all(unix, feature = "sspi-rs")))
+)]
impl From<sspi::Error> for Error {
fn from(err: sspi::Error) -> Self {
Error::SspiRs(format!("{}", err))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[cfg(all(unix, feature = "sspi-rs"))] | |
| impl From<sspi::Error> for Error { | |
| fn from(err: sspi::Error) -> Self { | |
| Error::SspiRs(format!("{}", err)) | |
| } | |
| } | |
| #[cfg(all(unix, feature = "sspi-rs"))] | |
| #[cfg_attr( | |
| feature = "docs", | |
| doc(cfg(all(unix, feature = "sspi-rs"))) | |
| )] | |
| impl From<sspi::Error> for Error { | |
| fn from(err: sspi::Error) -> Self { | |
| Error::SspiRs(format!("{}", err)) | |
| } | |
| } |
…nd unix sspi behaviours in ConfigString
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Personally, I think it makes sense to replace winauth with sspi as well. This would also enable Kerberos on Windows. |
4 tasks
joelparkerhenderson
added a commit
to mssql-rust/mssql-rust
that referenced
this pull request
Aug 29, 2026
Mirrors tiberius-rs/tiberius#408, reconciled with the concerns tasks.md raised about it (no tests, duplicated GSSAPI connection-string parsing, password not zeroized) plus one wire-protocol bug of the same shape #351 already fixed elsewhere in this fork. New `sspi-rs` feature (Unix only, via the `sspi` crate, currently 0.21.3 - newer than the PR's 0.18, adapted to its current API) implements NTLM/Windows authentication without needing a real Kerberos ticket cache, unlike the existing `integrated-auth-gssapi` feature. `WindowsAuth`/ `AuthMethod::Windows`/`AuthMethod::windows()` and the wire-plumbing cfg gates (`TokenSspi::new`, `PacketHeader::sspi`, `LoginMessage:: integrated_security`, `Context::spn`) are extended to also apply under `all(unix, feature = "sspi-rs")`, alongside their existing `windows`/ `integrated-auth-gssapi` gates. Fixes applied beyond a direct patch: - `WindowsAuth.password` changed from a plain `String` to `Zeroizing<String>` (matching `SqlServerAuth`'s existing #411 pattern), since this feature makes `WindowsAuth` reachable outside the Windows-only `winauth` path for the first time. The existing `winauth`-backed `AuthMethod::Windows` arm in `connection.rs` needed a small restructure to keep the password alive exactly as long as `winauth`'s borrowing API needs it (verified this still compiles against the real `winauth` crate via `cargo check --target x86_64-pc-windows-msvc`). - `Config::from_ado_string`/`from_jdbc_string`'s `IntegratedSecurity` dispatch previously repeated its guard condition (`val.to_lowercase() == "sspi" || Self::parse_bool(val)?`) once per `#[cfg(...)]`-gated match arm, and the PR's own new arm would have repeated it a third time. Restructured to check it exactly once, then dispatch via `cfg_if!` (already a dependency, added for #308) - a bugfix to that guard now only needs to happen in one place. The `integrated-auth-gssapi`-only behavior (always `Integrated`, ignoring any given credentials) is unchanged from before this commit; the new combinations (sspi-rs alone, or alongside gssapi) prefer explicit credentials (NTLM) when given and fall back to Kerberos or SQL auth otherwise - not the original PR's `AuthMethod::windows("", "")` blank- credential fallback for the no-creds/no-gssapi case, which would have attempted NTLM with an empty username and password instead of failing more legibly via SQL auth. - The new arm's continuation-packet send used `PacketHeader::login(id)` in the original PR; changed to `PacketHeader::sspi(id)`, the same wire-format bug #351 fixed for the `winauth` and GSSAPI paths earlier this session (MS-TDS distinguishes the SSPI continuation packet type 0x11 from the initial LOGIN7's 0x10). Added to the `all` feature bundle so CI's live-database `cargo-test-linux` job (and the Windows job, where the unix-only `sspi` dependency simply doesn't apply, same as `integrated-auth-gssapi`/`libgssapi` already) compiles and runs it going forward. Testing: added unit tests for `WindowsAuth`'s domain/user splitting and password zeroizing, and for the deduplicated connection-string dispatch across all four sspi-rs/gssapi feature combinations (previously zero tests covered this dispatch logic at all, for any backend). What is *not* verified: the actual NTLM wire handshake against a real server. None of this session's test infrastructure (a plain SQL-auth Azure SQL Edge container) can exercise Windows/NTLM authentication, and this sandbox has no Windows domain or NTLM-capable SQL Server to test against - the same limitation the original PR shipped with. The handshake code was written and reviewed carefully and both cross-compiles (Windows target) and compiles across every feature combination, but should be treated as unverified beyond that until someone can test it against a real NTLM-auth SQL Server. Verified: - cargo check: default, --features=rustls, --features=vendored-openssl, --no-default-features, sspi-rs, integrated-auth-gssapi, both together, --features=all, and --target x86_64-pc-windows-msvc for both winauth and all - cargo clippy --all-targets -- -D warnings for the same combinations - cargo fmt --check - cargo test --lib (182 passed default, 190 with --features=all) - Live integration tests over rustls+sspi-rs (SQL-auth path, unaffected): 305 passed (102 bulk + 203 query) + 28 doctests Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0156Di1tRRLsJK8ctU1AmAJr
joelparkerhenderson
added a commit
to mssql-rust/mssql-rust
that referenced
this pull request
Aug 30, 2026
Mirrors tiberius-rs/tiberius#408, reconciled with the concerns tasks.md raised about it (no tests, duplicated GSSAPI connection-string parsing, password not zeroized) plus one wire-protocol bug of the same shape #351 already fixed elsewhere in this fork. New `sspi-rs` feature (Unix only, via the `sspi` crate, currently 0.21.3 - newer than the PR's 0.18, adapted to its current API) implements NTLM/Windows authentication without needing a real Kerberos ticket cache, unlike the existing `integrated-auth-gssapi` feature. `WindowsAuth`/ `AuthMethod::Windows`/`AuthMethod::windows()` and the wire-plumbing cfg gates (`TokenSspi::new`, `PacketHeader::sspi`, `LoginMessage:: integrated_security`, `Context::spn`) are extended to also apply under `all(unix, feature = "sspi-rs")`, alongside their existing `windows`/ `integrated-auth-gssapi` gates. Fixes applied beyond a direct patch: - `WindowsAuth.password` changed from a plain `String` to `Zeroizing<String>` (matching `SqlServerAuth`'s existing #411 pattern), since this feature makes `WindowsAuth` reachable outside the Windows-only `winauth` path for the first time. The existing `winauth`-backed `AuthMethod::Windows` arm in `connection.rs` needed a small restructure to keep the password alive exactly as long as `winauth`'s borrowing API needs it (verified this still compiles against the real `winauth` crate via `cargo check --target x86_64-pc-windows-msvc`). - `Config::from_ado_string`/`from_jdbc_string`'s `IntegratedSecurity` dispatch previously repeated its guard condition (`val.to_lowercase() == "sspi" || Self::parse_bool(val)?`) once per `#[cfg(...)]`-gated match arm, and the PR's own new arm would have repeated it a third time. Restructured to check it exactly once, then dispatch via `cfg_if!` (already a dependency, added for #308) - a bugfix to that guard now only needs to happen in one place. The `integrated-auth-gssapi`-only behavior (always `Integrated`, ignoring any given credentials) is unchanged from before this commit; the new combinations (sspi-rs alone, or alongside gssapi) prefer explicit credentials (NTLM) when given and fall back to Kerberos or SQL auth otherwise - not the original PR's `AuthMethod::windows("", "")` blank- credential fallback for the no-creds/no-gssapi case, which would have attempted NTLM with an empty username and password instead of failing more legibly via SQL auth. - The new arm's continuation-packet send used `PacketHeader::login(id)` in the original PR; changed to `PacketHeader::sspi(id)`, the same wire-format bug #351 fixed for the `winauth` and GSSAPI paths earlier this session (MS-TDS distinguishes the SSPI continuation packet type 0x11 from the initial LOGIN7's 0x10). Added to the `all` feature bundle so CI's live-database `cargo-test-linux` job (and the Windows job, where the unix-only `sspi` dependency simply doesn't apply, same as `integrated-auth-gssapi`/`libgssapi` already) compiles and runs it going forward. Testing: added unit tests for `WindowsAuth`'s domain/user splitting and password zeroizing, and for the deduplicated connection-string dispatch across all four sspi-rs/gssapi feature combinations (previously zero tests covered this dispatch logic at all, for any backend). What is *not* verified: the actual NTLM wire handshake against a real server. None of this session's test infrastructure (a plain SQL-auth Azure SQL Edge container) can exercise Windows/NTLM authentication, and this sandbox has no Windows domain or NTLM-capable SQL Server to test against - the same limitation the original PR shipped with. The handshake code was written and reviewed carefully and both cross-compiles (Windows target) and compiles across every feature combination, but should be treated as unverified beyond that until someone can test it against a real NTLM-auth SQL Server. Verified: - cargo check: default, --features=rustls, --features=vendored-openssl, --no-default-features, sspi-rs, integrated-auth-gssapi, both together, --features=all, and --target x86_64-pc-windows-msvc for both winauth and all - cargo clippy --all-targets -- -D warnings for the same combinations - cargo fmt --check - cargo test --lib (182 passed default, 190 with --features=all) - Live integration tests over rustls+sspi-rs (SQL-auth path, unaffected): 305 passed (102 bulk + 203 query) + 28 doctests Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0156Di1tRRLsJK8ctU1AmAJr
This was referenced Sep 1, 2026
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
2 participants
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.
Tiberius currently doesn't support SSPI NTLM authentication on Linux/MacOS systems without using Kerberos. This limits the use of Tiberius in older or more restrictive environments operated by some companies.
This PR adds the
sspi-rsfeature. When enabled, Linux and MacOS users can useAuthMethod::Windowsjust like Windows users currently can. This is done through the optional dependency on sspi-rs. Enabling this feature on Windows currently does nothing, and does not overwrite the existingwinauthbehaviour.There's no breaking changes with this PR (as far as I'm aware). I have tested these changes locally and was successfully able to authenticate with SSPI NTLM without Kerberos.
Summary by CodeRabbit
New Features
Bug Fixes