feat: added sspi ntlm without kerberos using sspi-rs#408
feat: added sspi ntlm without kerberos using sspi-rs#408carlvoller wants to merge 4 commits intoprisma:mainfrom
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
| #[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.
| #[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
| #[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.
| #[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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
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