Strict encryption option #412, hostname_in_certificate option #340, t…#413
Open
olback wants to merge 9 commits intoprisma:mainfrom
Open
Strict encryption option #412, hostname_in_certificate option #340, t…#413olback wants to merge 9 commits intoprisma:mainfrom
olback wants to merge 9 commits intoprisma:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughAdds TDS 8.0 / Strict encryption support and wiring: new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
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)
380-390:⚠️ Potential issue | 🟠 MajorReject
encrypt=strictwhentds80is not compiled in.This branch is only gated on a TLS backend. A
default-features = falsebuild withnative-tlsorrustlsbut withouttds80will still acceptstrictfrom the connection string and hand an unsupported mode to the connection layer.🔧 Suggested fix
- Err(_) if val.eq_ignore_ascii_case("strict") => Ok(EncryptionLevel::Strict), + Err(_) if val.eq_ignore_ascii_case("strict") => { + #[cfg(feature = "tds80")] + { + Ok(EncryptionLevel::Strict) + } + #[cfg(not(feature = "tds80"))] + { + Err(crate::Error::Conversion( + "Connection string: `encrypt=strict` requires the `tds80` feature" + .into(), + )) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/config.rs` around lines 380 - 390, The encrypt() parsing currently accepts "strict" unconditionally; change the branch handling Err(_) if val.eq_ignore_ascii_case("strict") inside encrypt() so that it only returns Ok(EncryptionLevel::Strict) when the crate feature "tds80" is enabled (e.g. using cfg!(feature = "tds80") or #[cfg] gating), and otherwise return an Err parsed error (reject the value) so non-tds80 builds (default-features = false) do not accept "strict"; update the match in encrypt() that references Self::parse_bool and EncryptionLevel to perform this conditional check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/connection.rs`:
- Line 82: Remove the leftover commented-out dead code line that defines a
transport ("// let transport = Framed::new(MaybeTlsStream::Raw(tcp_stream),
PacketCodec);"); it is redundant because transport initialization is handled by
the subsequent match expression—delete that commented line so only the active
match-based transport creation remains (search for the transport variable
initialization and references to MaybeTlsStream and PacketCodec in connection.rs
to locate it).
- Around line 480-485: The code currently matches transport.into_inner() and
uses MaybeTlsStream::Raw(tcp) to call create_tls_stream(...,
TlsPreloginWrapper::new(tcp)).await? with an unreachable!() fallback; replace
that panic-prone fallback with a defensive error return (or at minimum a
debug_assert! plus a clear Err variant) so unexpected variants produce a
Result::Err instead of panicking. Locate the match around transport.into_inner()
in connection.rs and change the unreachable!() arm to return a meaningful error
(or debug_assert! then return a conversion error) so callers of this code
receive a graceful error when MaybeTlsStream is not Raw.
In `@src/client/tls_stream/native_tls_stream.rs`:
- Around line 17-20: The rustls backend currently uses
ClientConfig::builder().with_safe_defaults() without limiting TLS versions,
causing a mismatch with native-tls which caps to TLS 1.2 when the tds80 feature
is disabled; update the rustls client config (the ClientConfig constructed in
rustls_tls_stream.rs) to enforce a maximum of TLS1.2 in the non-tds80 case
(mirror the conditional used in native_tls_stream.rs) by setting the config's
supported protocol versions to only TLS1_2 (e.g., replace or override the
default versions on the created ClientConfig when cfg(not(feature = "tds80"))).
In `@src/tds.rs`:
- Around line 28-29: Prelogin ENCRYPTION currently writes the raw
EncryptionLevel discriminant (used in PreloginMessage::encode) so
EncryptionLevel::Strict ends up as 0x04 on the wire; update prelogin() to remap
Strict into a wire-compatible value (e.g., map EncryptionLevel::Strict to
EncryptionLevel::Required or the 0x01–0x03 equivalent) before
constructing/encoding the PreloginMessage so encode (which does
write_u8(self.encryption as u8)) never emits 0x04; locate the prelogin() method
and apply the mapping there (leave the internal config.encryption unchanged if
needed, only modify the value passed into PreloginMessage).
---
Outside diff comments:
In `@src/client/config.rs`:
- Around line 380-390: The encrypt() parsing currently accepts "strict"
unconditionally; change the branch handling Err(_) if
val.eq_ignore_ascii_case("strict") inside encrypt() so that it only returns
Ok(EncryptionLevel::Strict) when the crate feature "tds80" is enabled (e.g.
using cfg!(feature = "tds80") or #[cfg] gating), and otherwise return an Err
parsed error (reject the value) so non-tds80 builds (default-features = false)
do not accept "strict"; update the match in encrypt() that references
Self::parse_bool and EncryptionLevel to perform this conditional check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4d3682a-7245-4c64-b7a2-0e566d4ad112
📒 Files selected for processing (10)
Cargo.tomlsrc/client/config.rssrc/client/connection.rssrc/client/tls_stream/native_tls_stream.rssrc/client/tls_stream/opentls_tls_stream.rssrc/client/tls_stream/rustls_tls_stream.rssrc/lib.rssrc/tds.rssrc/tds/codec/login.rssrc/tds/codec/pre_login.rs
…re sent over the wire
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/tds/codec/login.rs (1)
199-200: 🧹 Nitpick | 🔵 TrivialMinor: Comment mentions "byte" but buffer is
u16array.The comment says "1 byte for the null terminator" but the buffer is
[0u16; 15 + 1], so it should say "1 element" or "1 WCHAR".📝 Suggested fix
- // MAX_COMPUTERNAME_LENGTH is 15 and we need 1 byte for the null terminator + // MAX_COMPUTERNAME_LENGTH is 15 and we need 1 WCHAR for the null terminator let mut buffer = [0u16; 15 + 1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tds/codec/login.rs` around lines 199 - 200, The comment above the buffer allocation incorrectly says "1 byte for the null terminator" even though the buffer is a u16 array; update the comment to reflect a 1-element/1 WCHAR null terminator instead. Locate the comment near the declaration let mut buffer = [0u16; 15 + 1] (related to MAX_COMPUTERNAME_LENGTH) in src/tds/codec/login.rs and replace "1 byte" with "1 element" or "1 WCHAR" so the comment accurately matches the u16 buffer type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 195-196: The default TLS backend was changed in Cargo.toml
(default = ["tds80","winauth","rustls"] and tds80 = ["tds73"]) which is a
breaking change for downstream users relying on native-tls; update the release
notes / CHANGELOG to call out that the default feature set now enables rustls
instead of native-tls, describe the consequences (different TLS behavior and
potential need to compile ring), and add clear migration guidance showing how
consumers can restore the old behavior by explicitly enabling the previous
feature (e.g., selecting native-tls and/or disabling rustls) in their Cargo.toml
or dependency feature list, plus any platform-specific build notes for ring.
In `@src/client/config.rs`:
- Around line 408-410: The parse path silently falls through to parse_bool and
yields a confusing "Not a valid boolean" when the user supplies "strict" but the
crate was compiled without the tds80 feature; update the parsing logic (the
FromStr/parse function that branches to EncryptionLevel::Strict and calls
parse_bool) to explicitly check for val.eq_ignore_ascii_case("strict") when
cfg!(not(feature = "tds80")) and return a clear Err variant/message indicating
that "strict" requires the tds80 feature (or suggest enabling that feature), so
users see a meaningful error instead of the generic parse_bool failure.
In `@src/client/tls_stream/native_tls_stream.rs`:
- Around line 17-19: The async-native-tls dependency is missing the "alpn"
feature required for builder.request_alpns to be available; update the
Cargo.toml dependency entry for async-native-tls to include the "alpn" feature
in its features list (e.g., add "alpn" alongside "runtime-async-std") so calls
like builder.request_alpns(&[super::TDS_ALPN_PROTOCOL_NAME]) compile
successfully.
---
Duplicate comments:
In `@src/tds/codec/login.rs`:
- Around line 199-200: The comment above the buffer allocation incorrectly says
"1 byte for the null terminator" even though the buffer is a u16 array; update
the comment to reflect a 1-element/1 WCHAR null terminator instead. Locate the
comment near the declaration let mut buffer = [0u16; 15 + 1] (related to
MAX_COMPUTERNAME_LENGTH) in src/tds/codec/login.rs and replace "1 byte" with "1
element" or "1 WCHAR" so the comment accurately matches the u16 buffer type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 501ec9e4-d7c6-4477-b96a-0ec12f4b8299
📒 Files selected for processing (11)
Cargo.tomlsrc/client/config.rssrc/client/config/ado_net.rssrc/client/connection.rssrc/client/tls_stream.rssrc/client/tls_stream/native_tls_stream.rssrc/client/tls_stream/opentls_tls_stream.rssrc/client/tls_stream/rustls_tls_stream.rssrc/tds.rssrc/tds/codec/login.rssrc/tds/codec/pre_login.rs
Comment on lines
+408
to
+410
| Err(_) if val.eq_ignore_ascii_case("strict") && cfg!(feature = "tds80") => { | ||
| Ok(EncryptionLevel::Strict) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider improving error message when tds80 feature is disabled.
When encrypt=strict is specified but the tds80 feature is not enabled, the code falls through to return a generic "Not a valid boolean" error from parse_bool. This may confuse users who expect strict to work.
♻️ Proposed improvement for clearer error handling
Err(_) if val == "DANGER_PLAINTEXT" => Ok(EncryptionLevel::NotSupported),
- Err(_) if val.eq_ignore_ascii_case("strict") && cfg!(feature = "tds80") => {
- Ok(EncryptionLevel::Strict)
- }
+ Err(_) if val.eq_ignore_ascii_case("strict") => {
+ #[cfg(feature = "tds80")]
+ {
+ Ok(EncryptionLevel::Strict)
+ }
+ #[cfg(not(feature = "tds80"))]
+ {
+ Err(crate::Error::Conversion(
+ "encrypt=strict requires the 'tds80' feature to be enabled".into(),
+ ))
+ }
+ }
Err(e) => Err(e),📝 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
| Err(_) if val.eq_ignore_ascii_case("strict") && cfg!(feature = "tds80") => { | |
| Ok(EncryptionLevel::Strict) | |
| } | |
| Err(_) if val.eq_ignore_ascii_case("strict") => { | |
| #[cfg(feature = "tds80")] | |
| { | |
| Ok(EncryptionLevel::Strict) | |
| } | |
| #[cfg(not(feature = "tds80"))] | |
| { | |
| Err(crate::Error::Conversion( | |
| "encrypt=strict requires the 'tds80' feature to be enabled".into(), | |
| )) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/config.rs` around lines 408 - 410, The parse path silently falls
through to parse_bool and yields a confusing "Not a valid boolean" when the user
supplies "strict" but the crate was compiled without the tds80 feature; update
the parsing logic (the FromStr/parse function that branches to
EncryptionLevel::Strict and calls parse_bool) to explicitly check for
val.eq_ignore_ascii_case("strict") when cfg!(not(feature = "tds80")) and return
a clear Err variant/message indicating that "strict" requires the tds80 feature
(or suggest enabling that feature), so users see a meaningful error instead of
the generic parse_bool failure.
Comment on lines
+17
to
+19
| if matches!(config.encryption, crate::EncryptionLevel::Strict) { | ||
| builder = builder.request_alpns(&[super::TDS_ALPN_PROTOCOL_NAME]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
async-native-tls 0.5 request_alpns method TlsConnector
💡 Result:
In async-native-tls 0.5.x, request_alpns is a builder-style method on async_native_tls::TlsConnector that lets you advertise ALPN protocols (e.g. h2, http/1.1) during the TLS handshake:
use async_native_tls::TlsConnector;
let tls = TlsConnector::new()
.request_alpns(&["h2", "http/1.1"]);request_alpns ultimately comes from native-tls and is only available when the native-tls crate is built with its alpn feature enabled. In native-tls it’s documented as “Available on crate feature alpn only.” [2] (Reqwest uses the same underlying request_alpns call on a TlsConnector::builder() for ALPN selection.) [3]
Sources: async-native-tls::TlsConnector docs (shows request_alpns(self, protocols: &[&str]) -> Self) [1], native-tls::TlsConnectorBuilder::request_alpns (feature-gated by alpn) [2], reqwest source using tls.request_alpns(...) [3].
🏁 Script executed:
#!/bin/bash
# Check Cargo.toml for async-native-tls dependency and alpn feature
find . -name "Cargo.toml" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 5 "async-native-tls" {}'Repository: prisma/tiberius
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Find the TDS_ALPN_PROTOCOL_NAME constant definition
rg "TDS_ALPN_PROTOCOL_NAME" -B 2 -A 2Repository: prisma/tiberius
Length of output: 1053
🏁 Script executed:
#!/bin/bash
# Check if there are any feature flags or default settings that might enable alpn
rg "alpn" -i --type tomlRepository: prisma/tiberius
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check async-native-tls documentation or source for default features
# Search for any build script or conditional compilation related to alpn
rg "request_alpns" -B 5 -A 5 src/client/tls_stream/native_tls_stream.rsRepository: prisma/tiberius
Length of output: 429
Add missing alpn feature to async-native-tls dependency.
The code at lines 17-19 calls builder.request_alpns(), which requires the alpn feature on the native-tls crate. However, the async-native-tls dependency in Cargo.toml only enables runtime-async-std:
[dependencies.async-native-tls]
version = "0.5"
features = ["runtime-async-std"]Update to:
[dependencies.async-native-tls]
version = "0.5"
features = ["runtime-async-std", "alpn"]Without this feature flag, the request_alpns method will not be available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/tls_stream/native_tls_stream.rs` around lines 17 - 19, The
async-native-tls dependency is missing the "alpn" feature required for
builder.request_alpns to be available; update the Cargo.toml dependency entry
for async-native-tls to include the "alpn" feature in its features list (e.g.,
add "alpn" alongside "runtime-async-std") so calls like
builder.request_alpns(&[super::TDS_ALPN_PROTOCOL_NAME]) compile successfully.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Around line 195-196: The change to the Cargo.toml default features now
enabling tds80 (default = ["tds80", "winauth", "native-tls"]) is a breaking
change for downstream users; update the CHANGELOG/release notes to clearly
document the migration: explain that tds80 is now default, that tds80 includes
tds73 but introduces new behavior (e.g., EncryptionLevel::Strict, ALPN/TLS
handshake differences and TDS 8.0 protocol capabilities), provide the exact
opt-out example using default-features = false and features = ["tds73",
"winauth", "native-tls"], and add a short guidance section listing likely
behavioral differences and how to test/restore previous behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+195
to
+196
| default = ["tds80", "winauth", "native-tls"] | ||
| tds80 = ["tds73"] |
There was a problem hiding this comment.
Document the breaking change in default features.
Changing the default feature set from tds73 to tds80 is a breaking change for downstream users. Users who relied on the previous defaults will now automatically receive TDS 8.0 behavior, including:
- New
EncryptionLevel::Strictsupport (requires TLS handshake with ALPN) - TDS 8.0 protocol capabilities
- Different connection behavior when using defaults
Users who need the previous behavior must explicitly opt out of default features and select tds73 in their Cargo.toml:
[dependencies]
tiberius = { version = "0.12", default-features = false, features = ["tds73", "winauth", "native-tls"] }This change should be prominently documented in the CHANGELOG/release notes with migration guidance.
Note: Since tds80 = ["tds73"], the tds80 feature is additive and preserves tds73 behavior while enabling additional functionality. However, the new code paths (Strict encryption, ALPN negotiation) may introduce behavioral differences that users should be aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` around lines 195 - 196, The change to the Cargo.toml default
features now enabling tds80 (default = ["tds80", "winauth", "native-tls"]) is a
breaking change for downstream users; update the CHANGELOG/release notes to
clearly document the migration: explain that tds80 is now default, that tds80
includes tds73 but introduces new behavior (e.g., EncryptionLevel::Strict,
ALPN/TLS handshake differences and TDS 8.0 protocol capabilities), provide the
exact opt-out example using default-features = false and features = ["tds73",
"winauth", "native-tls"], and add a short guidance section listing likely
behavioral differences and how to test/restore previous behavior.
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
1 participant
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.
Fixes
Config::trust_cert_cashould take Into<PathBuf> instead of ToString #336 - trust_cert_ca now takesInto<PathBuf>instead ofStringHostNameInCertificateproperty? #340 - Added option to setHostNameInCertificateproperty. Added support in connection string parser as well.New
LoginMessageStrictencryption optiontds80, extends the existing featuretds73Strictoption added toEncryptionLeveltds/8.0ALPN to TLS implementations that support it. ALPN is only requested whenEncryptionLevel = Strict. See note below.New/updated tests
encrypt=strictin connection stringhostname in certificatein connection stringWorkstation IDconnection stringQs
NOTE: While not required, ALPN should be set when negotiating TLS. SQL Server will assume tds/8.0 if unset.
As ffmpeg twitter once said:
talk is cheap, send patches