Add ActiveDirectory Interactive (browser) auth to mssql-odbc (T3) - #145
Add ActiveDirectory Interactive (browser) auth to mssql-odbc (T3)#145Vahid-b wants to merge 9 commits into
Conversation
Implement the ActiveDirectory Interactive method behind the existing EntraIdTokenFactory seam by hand-rolling the OAuth2 authorization-code flow with PKCE (the Azure SDK ships no interactive credential). Bind a loopback listener, open the system browser at the /authorize endpoint, receive the redirect, and exchange the code at /token as a public client. The redirect handler requires a matching state before honoring any code or error, reads the request line incrementally under a per-connection timeout, and ignores unrelated local callbacks. Empty access tokens are rejected so they are not cached. The login-connect deadline is disabled for interactive so the browser/MFA flow has time to complete. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds browser-based Active Directory Interactive authentication to the ODBC driver using OAuth2 authorization code flow with PKCE.
Changes:
- Implements browser launch, loopback redirect handling, token exchange, and caching.
- Registers interactive authentication with the TDS FedAuth pipeline.
- Adds dependencies and unit tests for PKCE, redirects, URLs, and token parsing.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
mssql-odbc/src/auth/mod.rs |
Registers the interactive authentication module. |
mssql-odbc/src/auth/interactive.rs |
Implements and tests the interactive OAuth2 flow. |
mssql-odbc/src/auth/entra.rs |
Configures the interactive token factory and shared helpers. |
mssql-odbc/src/api/driver_connect.rs |
Updates authentication support and unsupported-method testing. |
mssql-odbc/Cargo.toml |
Adds networking, PKCE, randomness, and JSON dependencies. |
| // The browser sign-in (with MFA) can take minutes, so disable the | ||
| // login-connect deadline; the flow keeps its own inner bound | ||
| // (REDIRECT_TIMEOUT) and stays cancellable. See AB#46067. | ||
| context.connect_timeout = 0; |
| let token = self | ||
| .token | ||
| .get_or_try_init(|| async move { |
| // The browser sign-in (with MFA) can take minutes, so disable the | ||
| // login-connect deadline; the flow keeps its own inner bound | ||
| // (REDIRECT_TIMEOUT) and stays cancellable. See AB#46067. | ||
| context.connect_timeout = 0; |
| fn open_browser(url: &str) -> std::io::Result<()> { | ||
| std::process::Command::new("open") | ||
| .arg(url) | ||
| .spawn() | ||
| .map(|_| ()) |
| fn open_browser(url: &str) -> std::io::Result<()> { | ||
| std::process::Command::new("xdg-open") | ||
| .arg(url) | ||
| .spawn() | ||
| .map(|_| ()) |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-odbc/src/api/driver_connect.rsmssql-odbc/src/api/exports.rsmssql-odbc/src/api/get_connect_attr.rsmssql-odbc/src/auth/interactive.rsmssql-odbc/src/handles/dbc.rsmssql-tds/src/connection_provider/tds_connection_provider.rs🔗 Quick Links |
- connect_timeout=0 gave every TCP connect a 0ms budget (timeout(0ms, ...) fires immediately), failing the connection before the browser opened. Raise it to a bounded 330s (CONNECT_TIMEOUT_SECS) so the login stays bounded and the TCP connect keeps a real budget, mirroring SqlClient's enlarged Connect Timeout for interactive auth. - Stop caching the access token in a OnceCell: like the service-principal path, acquire a fresh token each login so session recovery cannot reuse an expired token. Caching/refresh is tracked in AB#46409. - Drop the misleading "open this URL manually" hint from the browser-launch error; the loopback listener is gone once the error returns. - Reap the macOS open / Linux xdg-open child on a detached thread so it does not linger as a zombie. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Split the mssql-tds single connect_timeout into a network-connect cap (connect_timeout) and an overall login deadline (login_timeout), matching msodbcsql's separate connection vs. login timeouts. login_timeout is an Option<u32> that falls back to connect_timeout when unset, so every existing caller keeps identical behavior. Wire the ODBC SQL_ATTR_LOGIN_TIMEOUT attribute to login_timeout and have the interactive (T3) browser flow raise login_timeout instead of connect_timeout. An unreachable server now still fails fast on the per-TCP-connect cap while the browser/MFA round-trip has time to complete. An app-set SQL_ATTR_LOGIN_TIMEOUT takes precedence over the interactive default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Pushed mssql-tds core (additive, back-compatible):
mssql-odbc:
Net effect: the black-hole-server concern from the earlier review is resolved properly rather than papered over. Validation: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
mssql-odbc/src/auth/interactive.rs:315
READ_TIMEOUTis restarted for every read, so a local client that sends one byte every few seconds can monopolize this serial handler until the full redirect timeout and prevent the real browser callback from being processed. Apply one timeout to the entire request-line read (or track an absolute deadline), rather than timing each individual read.
let n = match tokio::time::timeout(READ_TIMEOUT, stream.read(&mut chunk)).await {
Ok(Ok(n)) if n > 0 => n,
_ => break, // EOF, read error, or timeout
};
mssql-odbc/src/auth/interactive.rs:435
- The token response is aggregated without any size limit. Because this endpoint is derived from server-provided FEDAUTHINFO, a malicious or malfunctioning HTTPS authority can stream an arbitrarily large body and exhaust the host application's memory before the outer login timeout fires. Read chunks with a small explicit cap appropriate for an OAuth JSON response and reject responses that exceed it.
let status = response.status();
let bytes = response
.bytes()
.await
.map_err(|e| Error::ConnectionError(format!("failed to read token response: {e}")))?;
mssql-odbc/src/auth/interactive.rs:422
- The client uses reqwest's default redirect policy, which follows cross-origin and HTTPS-to-HTTP redirects. A 307/308 response from the token endpoint would therefore resend the authorization code and PKCE verifier to the redirect target, bypassing the module's HTTPS-only endpoint check. OAuth token endpoints do not need redirects here; disable them (or validate every redirect target before following it).
let client = reqwest::Client::builder()
.build()
.map_err(|e| Error::ConnectionError(format!("failed to build HTTP client: {e}")))?;
| let login_timeout = context.login_timeout.unwrap_or(context.connect_timeout); | ||
| let deadline = match login_timeout { | ||
| 1.. => Some(Instant::now() + Duration::from_secs(login_timeout.into())), | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
Fixed in 7b55b3c. The login deadline is now computed at the top of execute_action_chain, before the shared-memory shortcut and SSRP/LocalDB resolution, so it bounds those phases too. The SM attempt uses the remaining login budget (deadline - now) instead of connect_timeout, so interactive sign-in against a local named instance is no longer cancelled after the default 15s. connect_timeout still bounds each individual TCP-connect attempt, and the change is back-compatible for callers that only set connect_timeout (login_timeout falls back to it).
| })?; | ||
|
|
||
| info!("launching browser for interactive Entra sign-in"); | ||
| debug!(%authorize_url, "interactive authorize URL"); |
There was a problem hiding this comment.
Fixed in 7b55b3c. The debug log now emits only scheme/host/path; the query string (CSRF state, PKCE challenge, and login_hint) is no longer logged.
One note: the additional locations flagged (lines 312/420/431) don't actually log anything — they're the request read-timeout, the reqwest client builder, and a response.status() read respectively. Line 235 was the only statement logging the URL, so that's the only change needed.
| let secs = value_ptr as usize as u32; | ||
| state.login_timeout = Some(secs); | ||
| debug!(secs, "SQLSetConnectAttrW: login timeout stored"); |
There was a problem hiding this comment.
Fixed in 7b55b3c. Implemented SQLGetConnectAttrW (new get_connect_attr.rs) backed by DbcState.login_timeout: for SQL_ATTR_LOGIN_TIMEOUT it writes the stored value into the caller's SQLUINTEGER buffer, returning the driver default of 15 when unset and 0 when the app set "wait indefinitely". Added set/get round-trip, default-when-unset, and null-pointer rejection tests.
…deadline ordering - Implement SQLGetConnectAttrW for SQL_ATTR_LOGIN_TIMEOUT so a set/get round-trip returns the stored value (driver default 15 when unset); add round-trip, default, and null-pointer tests. - Redact the interactive authorize URL debug log: drop the query string (CSRF state, PKCE challenge, login_hint) and log only scheme/host/path. - Compute the login deadline before the Windows shared-memory shortcut so it bounds the SM/SSRP/LocalDB phases too; the SM attempt now uses the remaining login budget instead of connect_timeout, so interactive sign-in against a local named instance isn't cancelled after the default 15s. Back-compatible for callers that only set connect_timeout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
mssql-tds/src/connection_provider/tds_connection_provider.rs:150
- This deadline still does not bound every phase as claimed. Both SSRP calls at lines 223/227 await their independent
ssrp_timeout_ms, and LocalDB resolution at line 266 performs a blockingLocalDBStartInstancecall without consultingdeadline; for example,login_timeout = 1withssrp_timeout_ms = 10_000can run for roughly ten seconds. Apply the remaining login budget to these resolution phases too, and add a regression test with distinct login and SSRP timeouts.
let login_timeout = context.login_timeout.unwrap_or(context.connect_timeout);
let deadline = match login_timeout {
1.. => Some(Instant::now() + Duration::from_secs(login_timeout.into())),
_ => None,
};
| let code = tokio::time::timeout(REDIRECT_TIMEOUT, wait_for_redirect(&listener, &state)) | ||
| .await | ||
| .map_err(|_| { | ||
| Error::ConnectionError("timed out waiting for interactive sign-in".into()) | ||
| })??; |
There was a problem hiding this comment.
Fixed in 2382c55c. The browser-redirect wait is no longer a fixed 300s.
InteractiveTokenFactory now carries the effective login_timeout (set on the context by the interactive arm — the app's SQL_ATTR_LOGIN_TIMEOUT, or the 330s default when unset), and redirect_wait_cap derives the wait from it:
Some(n)→nseconds (honors app values above or below the old fixed cap),Some(0)→ no cap (honors an infinite login timeout),None→ theREDIRECT_TIMEOUTfallback.
The provider already wraps create_token in timeout(deadline) where deadline is derived from the same login_timeout, so the inner (browser-wait) and outer (login-deadline) bounds now agree instead of the inner 300s silently winning. Added 3 unit tests for the cap derivation.
| let bytes = response | ||
| .bytes() | ||
| .await | ||
| .map_err(|e| Error::ConnectionError(format!("failed to read token response: {e}")))?; |
There was a problem hiding this comment.
Fixed in 2382c55c. read_body_capped replaces response.bytes(): it rejects an advertised Content-Length over the cap, then streams the body via chunk() enforcing a 1 MiB MAX_TOKEN_RESPONSE_BYTES limit (real token responses are a few KB). Since the STS authority is taken from the server's FEDAUTHINFO, this closes the unbounded-read / memory-exhaustion exposure you flagged.
| unsafe fn sql_get_connect_attr_w_impl( | ||
| connection_handle: SqlHandle, | ||
| attribute: SqlInteger, | ||
| value_ptr: SqlPointer, | ||
| _buffer_length: SqlInteger, |
There was a problem hiding this comment.
Refactored in 2382c55c. get_connect_attr.rs now follows the same panic-boundary → unsafe-shim → safe-core split as get_data.rs:
sql_get_connect_attr_w— publicunsafe fn, all-argumentdebug!+ffi_entry!panic boundary.sql_get_connect_attr_w_impl—unsafe fn, null-handle →SQL_INVALID_HANDLE,handle_from_raw::<DbcHandle>,debug_assert_eq!on the object type.sql_get_connect_attr_w_safe(&DbcHandle, …)— safe core: lock,free_errors, attribute match.
Good catch — the file had mirrored the older set_connect_attr.rs, which predates this convention.
| // buffer. `Some(0)` reflects an app-set "wait indefinitely"; an | ||
| // unset attribute reports the driver default. | ||
| let secs = state.login_timeout.unwrap_or(DEFAULT_LOGIN_TIMEOUT_SECS); | ||
| unsafe { (value_ptr as *mut u32).write_unaligned(secs) }; |
There was a problem hiding this comment.
Done in 2382c55c. The value write now goes through write_if_some(value_ptr as *mut u32, secs) (util.rs) instead of a raw write_unaligned. The explicit null-pointer check stays ahead of it (returns SQL_ERROR with a diagnostic), so write_if_some only ever runs on a validated non-null pointer.
| debug!( | ||
| ?connection_handle, | ||
| attribute, | ||
| ?value_ptr, | ||
| "SQLGetConnectAttrW called", | ||
| ); |
There was a problem hiding this comment.
Fixed in 2382c55c. The entry debug! now logs buffer_length and string_length_ptr alongside the handle, attribute, and value pointer, matching get_data.rs's all-argument trace.
| /// detached thread so the sign-in flow is not blocked. | ||
| #[cfg(unix)] | ||
| fn reap(mut child: std::process::Child) { | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
Fixed in 2382c55c. reap now uses std::thread::Builder::new().name(…).spawn(…) and handles the Err: on an OS thread-creation failure it logs and skips the wait (the child detaches — a rare zombie) instead of panicking. That keeps the shared library from unwinding across the FFI boundary, per the no-panic rule.
| post_sql_error( | ||
| &mut state, | ||
| SQLSTATE_HY009, | ||
| 0, | ||
| "SQL_ATTR_LOGIN_TIMEOUT value pointer is null", |
There was a problem hiding this comment.
Fixed in 2382c55c. The null-value_ptr case now calls post_diag(&mut state, ERR_INVALID_NULL_POINTER) (sqlstate.rs) instead of an inline post_sql_error(SQLSTATE_HY009, …) literal, so the SQLSTATE and message come from the shared diagnostic constant.
Getter (SQLGetConnectAttrW): refactor get_connect_attr.rs to the crate's canonical panic-boundary -> unsafe shim -> safe-core layering (mirroring get_data.rs); write the value via write_if_some; report a null value pointer with post_diag(ERR_INVALID_NULL_POINTER); log buffer_length and string_length_ptr in the entry trace. Interactive auth (interactive.rs): - Bound the browser-redirect wait by the effective login_timeout instead of a fixed 300s, so an app SQL_ATTR_LOGIN_TIMEOUT above/below the default (or 0 = infinite) is honored. The factory now carries the effective login timeout and derives the cap via redirect_wait_cap; REDIRECT_TIMEOUT stays as the fallback. - Cap the /token response body at 1 MiB (read_body_capped) so a hostile FEDAUTHINFO-selected authority cannot stream an unbounded body into memory. - Reap the browser-launcher child via thread::Builder::spawn and handle the error instead of thread::spawn, which panics on OS thread-creation failure (a shared library must not unwind across the FFI boundary). Tests: +3 redirect_wait_cap unit tests. fmt + clippy (-D warnings) clean; 352 mssql-odbc lib tests and 8 connection_provider tests pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
mssql-tds/src/connection_provider/tds_connection_provider.rs:150
- This only calculates a deadline; it does not bound the SSRP and LocalDB awaits below (lines 223/227 and 266). For example, an app-set 1-second login timeout can still wait for a longer SSRP timeout before the retry loop ever checks
deadline, so the new overall-login contract is not enforced. Wrap those resolution phases in the remaining budget, or apply the timeout around the whole action-chain future.
let login_timeout = context.login_timeout.unwrap_or(context.connect_timeout);
let deadline = match login_timeout {
1.. => Some(Instant::now() + Duration::from_secs(login_timeout.into())),
_ => None,
};
| async fn read_request_query(stream: &mut tokio::net::TcpStream) -> Option<String> { | ||
| let mut buf = Vec::new(); | ||
| let mut chunk = [0u8; 1024]; | ||
| loop { | ||
| let n = match tokio::time::timeout(READ_TIMEOUT, stream.read(&mut chunk)).await { |
There was a problem hiding this comment.
Good catch. READ_TIMEOUT was applied per read() call, so a client that dribbles bytes (or just holds the socket open) reset the 5s timer on every chunk and could hold this sequential loopback handler for the entire login budget — and indefinitely once the login timeout is infinite (the Some(0) path added in the previous round). Because wait_for_redirect accepts one connection at a time, that starves the real browser callback.
Fixed in 2a1415b: read_request_query now bounds the whole request-line read with a single tokio::time::timeout_at(now + READ_TIMEOUT, …) deadline instead of a per-read timeout, so total read time is capped regardless of how the bytes arrive. I split out read_request_query_within(stream, budget) and added two unit tests — a complete request line parses, and a one-byte-then-stall client is cut off well within the budget.
| return Err(Error::ConnectionError(format!( | ||
| "interactive sign-in failed: {detail}" | ||
| ))); |
There was a problem hiding this comment.
Correct, and this was an active bug rather than theoretical. RedirectOutcome::Failed (user clicks Cancel / access_denied, consent_required, …) returned Error::ConnectionError, which is_transient_connect_error (mssql-tds/src/error/mod.rs:402-407) classifies as transient. The ODBC connect path uses ClientContext's default connect_retry_count = 1 (ODBC parses ConnectRetryCount but doesn't wire it into the context), so the provider loops 0..=1 = two attempts and re-runs create_token on the retry — the browser pops a second time before the connection finally fails.
Fixed in 2a1415b: a state-matched authorization error now maps to Error::Security(SecurityError::AuthenticationDenied(..)), which is non-transient (it falls through the _ => false arm of is_transient_connect_error; pinned by the new authentication_denied_is_not_transient test). Genuinely transient transport faults around the sign-in stay ConnectionError. I reused the existing SecurityError variant instead of adding one, so there's no change to the public error surface.
| if !status.is_success() { | ||
| let detail = serde_json::from_str::<TokenErrorResponse>(body.as_ref()) | ||
| .map(|e| format!("{}: {}", e.error, e.error_description)) | ||
| .unwrap_or_else(|_| body.into_owned()); | ||
| return Err(Error::ConnectionError(format!( | ||
| "interactive token exchange failed ({status}): {detail}" | ||
| ))); |
There was a problem hiding this comment.
Same root cause as the redirect-failure comment. A non-2xx from the /token endpoint returned Error::ConnectionError (transient), so the connect-retry loop relaunched the whole browser flow — even for permanent RFC 6749 §5.2 failures such as invalid_grant.
Fixed in 2a1415b: the !status.is_success() path now returns Error::Security(SecurityError::AuthenticationDenied(..)) (non-transient), so a token-exchange failure is surfaced to the caller instead of silently re-prompting. I classify the entire non-2xx case as non-transient deliberately: create_token is atomic (a fresh browser + consent on every call, no token cache), so a provider-level retry is never a valid recovery for a token-exchange failure — msodbcsql likewise surfaces the error rather than reopening the browser.
Address Copilot review on PR #145: - Map interactive auth denials (user cancel / access_denied, and permanent token-endpoint failures) to Error::Security(SecurityError::AuthenticationDenied), which is non-transient, so the connect-retry loop no longer relaunches the browser on a terminal auth outcome. - Bound the loopback request-line read by a single overall deadline instead of a per-read timeout, so a slow local client cannot hold the sequential callback handler open and starve the real browser redirect. Add tests: bounded request-line read behavior (mssql-odbc) and a regression guard that AuthenticationDenied is not a transient connect error (mssql-tds). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
mssql-tds/src/connection_provider/tds_connection_provider.rs:142
- This deadline does not actually bound every phase named here. The SSRP calls at lines 223/227 use only
ssrp_timeout_ms, and LocalDB resolution at line 266 is also awaited without the remaining login budget. For example, a caller can setlogin_timeout = 1andssrp_timeout_ms = 30, yet named-instance resolution can run for 30 seconds before the deadline is checked. Wrap these resolution phases in the remaining deadline (and move blocking LocalDB work off the runtime thread) sologin_timeoutis truly an overall cap.
// Compute the overall login deadline up front so it bounds every
// phase of login — the shared-memory shortcut below, SSRP/LocalDB
// resolution, and the connect/retry loop — not just the final
// transport attempts. `login_timeout` falls back to `connect_timeout`
// for callers that only set the historical single knob; `0` means
| /// Overall login deadline in seconds, covering the network connect, the TDS | ||
| /// handshake, and any auth token acquisition (e.g. the interactive browser | ||
| /// flow). `None` falls back to [`Self::connect_timeout`] (preserving the | ||
| /// historical single-knob behavior); `Some(0)` disables the deadline (wait | ||
| /// indefinitely). Maps to the ODBC `SQL_ATTR_LOGIN_TIMEOUT` attribute. | ||
| pub login_timeout: Option<u32>, |
The deadline bounds the shared-memory shortcut and the connect/retry loop, but not name resolution: SSRP uses its own ssrp_timeout_ms and LocalDB resolution is a local pipe lookup. Describe that accurately instead of claiming it bounds every phase of login. Addresses a low-confidence Copilot note on PR #145. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
mssql-odbc/src/auth/interactive.rs:565
- A successful HTTP status with malformed JSON or an empty token is converted to
ConnectionError, whichis_transient_connect_errorretries. That retry restarts the entire connection and opens a second browser even though the authority already returned a definitive invalid response. Classify response-validation failures as a non-transient protocol error.
let token = serde_json::from_str::<TokenResponse>(body)
.map(|token| token.access_token)
.map_err(|e| Error::ConnectionError(format!("failed to parse token response: {e}")))?;
if token.is_empty() {
return Err(Error::ConnectionError(
"token response contained an empty access token".into(),
));
| // Bound the shared-memory attempt (which wraps the full TDS/auth | ||
| // handshake) by the remaining login budget rather than | ||
| // `connect_timeout`, so interactive sign-in against a local named | ||
| // instance isn't cancelled after the default 15s. |
There was a problem hiding this comment.
Good catch — this is a direct consequence of the round-3 change that made interactive-auth denials non-transient. The shared-memory shortcut runs the full TDS/auth handshake, so a cancelled interactive sign-in against a local named instance now surfaces as AuthenticationDenied, and the old Err arm fell through to SSRP/TCP for every error — relaunching the browser.
Fixed in 95bc1c9: the SM Err arm now only falls through when the failure is transient (err.is_transient_connect_error() — a transport-level problem such as SM not being available). A non-transient failure (the cancelled/denied login, or any permanent login error) returns immediately. That matches SNI semantics — a definitive login failure would recur on every transport — and stops the second browser.
| // Attributes not backed by stored state report success without writing, | ||
| // matching the historical stub behavior. | ||
| _ => SQL_SUCCESS, |
There was a problem hiding this comment.
Agreed — returning SQL_SUCCESS without writing value_ptr leaves the caller's buffer undefined while claiming success, and it was inconsistent with the set-side, which already rejects unknown attributes with HYC00.
Fixed in 95bc1c9: the _ arm now posts HYC00 ("Connection attribute not supported") and returns SQL_ERROR, mirroring sql_set_connect_attr_w. Added an unsupported_attribute_returns_error test to cover it.
Address Copilot round-4 review on PR #145: - tds_connection_provider: the shared-memory shortcut runs the full handshake, so it can now return a non-transient AuthenticationDenied (interactive sign-in cancelled). Only fall through to SSRP/TCP on transient failures; return non-transient errors immediately so a cancelled login does not relaunch the browser. - get_connect_attr: unsupported attributes now return SQL_ERROR/HYC00 instead of SQL_SUCCESS without writing, matching the set-side. - interactive: a 200 OK with a malformed body or empty access_token is a protocol failure, not a transient blip; map both to ProtocolError so they stay out of the connect retry loop. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
mssql-tds/src/connection_provider/tds_connection_provider.rs:142
- The new “overall” deadline does not actually bound the name-resolution phase. It is created here, but
resolve_ssrp(...).awaitandresolve_localdb_instance(...).awaitrun without the remaining-budget timeout; the latter synchronously callsLocalDBStartInstance(connection/transport/localdb.rs:441-449), soSQL_ATTR_LOGIN_TIMEOUTcan expire whileSQLDriverConnectremains blocked. Wrap these phases (or the whole action-chain future) in the overall budget; LocalDB’s blocking call also needs to run off the async worker for a Tokio timeout to take effect.
// Compute the overall login deadline up front. It bounds the
// shared-memory shortcut below and the connect/retry loop — the phases
// that run the TDS/auth handshake. Name resolution is bounded
// separately and is not covered by this deadline: SSRP uses its own
// `ssrp_timeout_ms` (default 1s) and LocalDB resolution is a local pipe
| /// ActiveDirectory Interactive. It is registered with an `http://localhost` | ||
| /// loopback redirect for native clients, so reusing it matches the classic | ||
| /// driver and needs no per-app registration. | ||
| const PUBLIC_CLIENT_ID: &str = "a94f9c62-97fe-4d19-b06d-472bed8d2bcb"; |
There was a problem hiding this comment.
I didnt find this client id in msodbcsql repo. Is this a different client id that's intentionally used here? Just curious, as the comment says it's the client id used by msodbcsql.
There was a problem hiding this comment.
Switched to msodbcsql's actual ODBC client id, 2c1229aa-16c5-4ff5-b46b-4f7fe2a2a9c8 (tds/Parse.cpp:3608 , aadauth/AzureADAuth.cpp:839 ), so both drivers present one identity to Entra.
One gap remains, now documented on PUBLIC_CLIENT_ID : msodbcsql only signs in through the Windows WAM broker, so that app is registered for broker redirects only — http://localhost isn't registered yet. Until it is, the flow fails late, after the user has already completed MFA (AADSTS50011). The redirect timeout now points at the browser and names that error instead of expiring silently.
T3 is split into three work items so this PR isn't blocked on the registration turnaround:
| Work item | Scope |
|---|---|
| AB#46067 — Part 1 (this PR) | OAuth2 auth-code + PKCE loopback flow on all platforms; separate SQL_ATTR_LOGIN_TIMEOUT and new SQLGetConnectAttr for msodbcsql parity |
| AB#46684 — Part 2 | Windows interactive via mssql-auth (OneAuth/WAM), matching msodbcsql. Its broker redirect is already registered, so it isn't blocked by Part 3. This PR's flow stays as the Windows fallback for contexts WAM can't serve, e.g. Session 0. |
| AB#46683 — Part 3 | Add http://localhost to app 2c1229aa. Unblocks Linux/macOS, where loopback is the only option — msodbcsql compiles its WAM path out off Windows, and this is what SqlClient uses there. |
On Windows this PR ships loopback as an interim, superseded by Part 2. A caller-supplied client_id override would sidestep the registration dependency, but msodbcsql has no such keyword and it would let a connection string redirect sign-in to an arbitrary app — out of scope in AB#46067.
I plan to merge Part 1 (current PR) now, documented as non-functional until AB#46683 lands, or hold it until the registration is in place.
The previous id a94f9c62-97fe-4d19-b06d-472bed8d2bcb does not exist in Entra (AADSTS700016), so interactive sign-in could never have succeeded. Present msodbcsql's ODBC client id instead so both drivers are a single identity in Entra. That app is registered for Windows broker redirects only, so the loopback flow stays blocked until http://localhost is added (AB#46683). Document the pending registration and the late AADSTS50011 failure, and make the redirect timeout name the browser-side cause instead of expiring silently. Also document the per-platform mechanism: loopback is the only option on Linux/macOS and matches SqlClient, while Windows moves to mssql-auth (OneAuth/WAM) under AB#46684 with this flow as the fallback. Add mock token-endpoint tests covering the request form, OAuth error mapping, malformed and empty responses, and an unreachable endpoint, plus coverage for the redirect-wait timeout arm. AB#46067 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
Implements T3 of the mssql-odbc authentication plan:
Authentication=ActiveDirectoryInteractive(browser sign-in). The Azure SDK for Rust ships no interactive credential, so this hand-rolls the OAuth2 authorization-code + PKCE flow behind the sameEntraIdTokenFactoryseam introduced by T2 (#110). The parity reference is the classic C++msodbcsqldriver.This PR also lands the separate login timeout (
SQL_ATTR_LOGIN_TIMEOUT) for full msodbcsql parity — see Timeout model below.Methods
Authentication=ActiveDirectoryInteractive, optionalUID→login_hint. No secret is read from or written to the connection context.How it works
A new
mssql-odbc/src/auth/interactive.rsprovidesInteractiveTokenFactory(implements the mssql-tdsEntraIdTokenFactorytrait).configure_authresolves the method, registers the factory inClientContext.auth_method_map, and mssql-tds invokes it during the FedAuth handshake. The factory:S256challenge and a 256-bitstate.127.0.0.1:0; advertiseshttp://localhost:{port}as the redirect (matching MSAL.NET / the well-known public client's registered loopback)./authorizeendpoint (authority + tenant derived from the server's FEDAUTHINFO STS URL).state, and exchanges thecode+ verifier at/tokenas a public client (no secret).Timeout model (msodbcsql parity)
Previously mssql-tds had a single
connect_timeoutthat bounded both the overall login deadline and each TCP-connect attempt — so making room for a multi-minute browser/MFA sign-in also let an unreachable host hang for that long. This PR splits them, matching msodbcsql's separate connection vs. login timeouts:connect_timeout(unchanged default 15s) — the per-TCP-connect cap; an unreachable server still fails fast.login_timeout: Option<u32>(new) — the overall login deadline covering network connect + TDS handshake + auth token acquisition.Nonefalls back toconnect_timeout, so every existing caller behaves identically;Some(0)means wait indefinitely.The ODBC
SQL_ATTR_LOGIN_TIMEOUTattribute is now honored and maps tologin_timeout. Interactive installs a generous default login timeout (330s) only when the app hasn't set its ownSQL_ATTR_LOGIN_TIMEOUT, and leavesconnect_timeoutat its default. Follow-up (separateConnection Timeoutconnection-string keyword) can build on this seam.Security
S256) + single-usestatebind the code to this process and guard against CSRF. The redirect handler requires a matchingstatebefore honoring anycodeorerror, reads the request line incrementally under a per-connection timeout (with a size cap), and ignores unrelated local callbacks so a strayhttp://localhostrequest can neither inject a code nor abort the sign-in.https— the same trust model as T2/SP (msodbcsql via azure-identity-cpp). Residual risk on a non-certificate-validated channel (TrustServerCertificate=yes): a rogue server could point sign-in at an attacker authority — useEncrypt=Strictor a validated certificate. Documented in the module; flagged below for a team-wide decision.explorer.exe <url>(no shell) — avoidscmd's%VAR%expansion (which corrupts the percent-encoded URL) and any injection from the server-provided authority. On macOS/Linux the spawnedopen/xdg-openchild is reaped so it can't linger as a zombie.Tests
Unit tests (CI-safe, no network): PKCE derivation (RFC 7636 vector) + verifier charset/length; authorize-URL construction (incl.
login_hintpresent/absent); token-endpoint shape; redirect classification (code on state-match; state-mismatch-with-code → ignored; missing state → ignored; server error surfaced only on state-match; error without state-match → ignored); token-response parsing incl. empty-token rejection;configure_authdispatch (registers factory, hides credentials, raiseslogin_timeoutwhile leavingconnect_timeoutat its default, and preserves an app-set login timeout). Timeout wiring:SQL_ATTR_LOGIN_TIMEOUTis stored (incl.0→ infinite) andClientContextdefaultslogin_timeouttoNone(back-compat).The full browser flow needs a human + a real tenant, so only pure logic is unit-tested (matches the wiki's manual-test plan).
Decisions flagged for review
a94f9c62-…(the msodbcsql/SqlClient AD-Interactive app,http://localhostloopback). No per-connection override yet.login_timeout(330s default) rather than overloadingconnect_timeout, so an unreachable server still fails fast on the per-connect cap. An app-setSQL_ATTR_LOGIN_TIMEOUTtakes precedence. Touches mssql-tds core (ClientContext) — additive and back-compatible (login_timeout: Nonefalls back toconnect_timeout).SQL_DRIVER_NOPROMPT— msodbcsql parity: that flag governs the DSN dialog, not the AAD sign-in.127.0.0.1bind,localhostredirect); hosts resolvinglocalhostto::1only are a documented fast-follow.Verification
cargo bfmtclean;cargo bclippy(workspace,-D warnings) clean. Rancargo test -p mssql-odbc --lib→ 346 passed; the changed mssql-tds modules (client_context,connection_provider) pass. Localcargo-nextest/llvm-covunavailable, so fullcargo btest+ coverage runs in CI.Related Issues
AB#46067
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses — rancargo test -p mssql-odbc --lib(346 pass) locally; full nextest/coverage runs in CI