FEAT: Add ActiveDirectoryServicePrincipal support for bulk copy#576
Draft
bewithgaurav wants to merge 4 commits into
Draft
FEAT: Add ActiveDirectoryServicePrincipal support for bulk copy#576bewithgaurav wants to merge 4 commits into
bewithgaurav wants to merge 4 commits into
Conversation
bewithgaurav
changed the title
Wires a Python token-factory callback into the mssql-py-core connection
context so bulk copy can authenticate with `Authentication=
ActiveDirectoryServicePrincipal`. The callback is invoked by mssql-tds
mid-handshake (FedAuth workflow 0x02), receives the STS URL from the
server, parses tenant_id from it, and uses azure-identity's
ClientSecretCredential to acquire a JWT — matching what ODBC does for
the query path.
Why a callback rather than pre-acquire (Model A):
- ServicePrincipal needs `tenant_id` to build ClientSecretCredential.
- The connection string does not carry it; azure-identity does not
discover it. The only place we can learn tenant_id client-side is
from the STS URL the server hands back during pre-login (which is
exactly what ODBC does internally).
- A pre-acquire flow therefore can't work. Saurabh approved the
callback model on EXP--Drivers--Python (May 13 2026).
Scope: ServicePrincipal only. ActiveDirectoryPassword and
ActiveDirectoryIntegrated remain on their existing code paths (still
rejected in py-core today with a clear error). Separate follow-ups.
Changes:
- constants.py: Add AuthType.SERVICE_PRINCIPAL.
- auth.py:
- Add ServicePrincipalAuth.make_token_factory(client_id, secret)
that returns a (spn, sts_url, auth_method) -> bytes callable
matching the entra_id_token_factory contract py-core expects.
- Add _parse_tenant_id() helper.
- process_auth_parameters: detect SP and return auth_type=None
(let ODBC handle the query path natively, msodbcsql 17.3+ has
native SP support).
- extract_auth_type: propagate "serviceprincipal" so bulkcopy can
distinguish the SP path.
- cursor.py bulkcopy: when _auth_type=="serviceprincipal", build the
factory from connection-string UID/PWD and register it via the new
entra_id_token_factory dict key. Keep authentication/user_name/
password in pycore_context (py-core's auth validator + transformer
need them to resolve the method to ActiveDirectoryServicePrincipal
before the factory is dispatched). Existing Default/DeviceCode/
Interactive (Model A) flow unchanged.
Requires mssql-py-core 0.1.5+ which wires the entra_id_token_factory
dict key into ClientContext.auth_method_map.
Tests: 17 new in test_008_auth.py covering tenant parsing
(GUID/domain/query-string/trailing-slash/empty/etc), credential kwarg
forwarding, scope construction, UTF-16LE encoding, and error paths
(missing client_id/secret, bad STS URL, auth failure propagation).
Partial fix for #534.
bewithgaurav
force-pushed
the
bewithgaurav/gh534-bulkcopy-service-principal
branch
from
2e81b7f to
cd80d9c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds bulk copy support for Authentication=ActiveDirectoryServicePrincipal by registering an Entra ID token-factory callback (invoked mid-handshake) so the tenant can be derived from the STS URL returned by the server, aligning bulk copy behavior with ODBC’s native Service Principal handling for the normal query path.
Changes:
- Added
AuthType.SERVICE_PRINCIPALand updated auth-type extraction/processing to recognize Service Principal mode. - Implemented
ServicePrincipalAuth.make_token_factory()and_parse_tenant_id()to generate UTF-16LE JWTs during FedAuth handshake. - Updated
cursor.bulkcopy()to registerentra_id_token_factoryfor Service Principal auth; added tests and a changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
mssql_python/constants.py |
Adds SERVICE_PRINCIPAL enum value for connection-string auth detection. |
mssql_python/auth.py |
Adds tenant parsing + token-factory callback for Service Principal; updates auth detection/mapping. |
mssql_python/cursor.py |
Registers the callback for bulkcopy when _auth_type == "serviceprincipal"; preserves credentials for py-core auth resolution. |
tests/test_008_auth.py |
Adds unit tests for tenant parsing, factory behavior, and new auth-type detection. |
CHANGELOG.md |
Documents new bulk copy Service Principal support and notes py-core requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2951
to
+2964
| try: | ||
| factory = ServicePrincipalAuth.make_token_factory( | ||
| client_id, client_secret | ||
| ) | ||
| except (RuntimeError, ValueError) as e: | ||
| raise RuntimeError( | ||
| f"Bulk copy failed: unable to build ServicePrincipal " | ||
| f"token factory: {e}" | ||
| ) from e | ||
| pycore_context["entra_id_token_factory"] = factory | ||
| # Keep authentication/user_name/password in pycore_context — | ||
| # py-core's auth validator + transformer need them to resolve | ||
| # the auth method to ActiveDirectoryServicePrincipal before | ||
| # the factory is dispatched at handshake time. |
Comment on lines
+2966
to
+2967
| "Bulk copy: registered ServicePrincipal token factory for client_id=%s", | ||
| client_id, |
Comment on lines
+147
to
+155
| try: | ||
| parsed = urlparse(sts_url) | ||
| except (ValueError, AttributeError): | ||
| return None | ||
| path = (parsed.path or "").strip("/") | ||
| if not path: | ||
| return None | ||
| first_segment = path.split("/", 1)[0] | ||
| return first_segment or None |
Comment on lines
+2960
to
+2964
| pycore_context["entra_id_token_factory"] = factory | ||
| # Keep authentication/user_name/password in pycore_context — | ||
| # py-core's auth validator + transformer need them to resolve | ||
| # the auth method to ActiveDirectoryServicePrincipal before | ||
| # the factory is dispatched at handshake time. |
- black: reformat auth.py and cursor.py per project line-length=100 - _parse_tenant_id: reject non-URL inputs (no scheme/netloc) so a bare string from an unexpected STS URL shape can't silently become tenant_id - cursor.py debug log: drop client_id from log message (credential identifier) - cursor.py finally cleanup: also pop entra_id_token_factory so the closure (which captures client_secret) doesn't outlive the dict reference - cursor.py: collapse f"..." f"..." adjacent string with no second-half interpolation - tests: 2 new TestParseTenantId cases for non-URL rejection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…https-only - ServicePrincipalAuth: cache ClientSecretCredential per tenant inside the closure so azure-identity's per-instance token cache actually works across handshake retries / reconnects (previously the credential was rebuilt on every call, defeating the cache and forcing a fresh AAD round-trip) - ServicePrincipalAuth: reject empty SPN early (was silently constructing scope='/.default' and surfacing a confusing azure-identity error) - ServicePrincipalAuth: surfaced RuntimeError no longer echoes provider message; provider detail stays in debug logs only (the user-facing chain could otherwise leak provider-controlled text) - _parse_tenant_id: tighten to https-only (Azure AD STS URLs are always https) - tests: 4 new cases - http rejection, empty SPN, no provider-message leak, per-tenant credential caching Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # nothing). Lives for the lifetime of the closure (== lifetime of the | ||
| # bulkcopy connection on the Rust side). | ||
| # | ||
| # TODO(#573-followup): once the MSI PR (#573) merges, refactor to |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/auth.pyLines 145-154 145 from urllib.parse import urlparse
146
147 try:
148 parsed = urlparse(sts_url)
! 149 except (ValueError, AttributeError):
! 150 return None
151 # Reject anything that isn't an https URL with a netloc. ``urlparse`` will
152 # happily put a bare string like ``"tenant-guid"`` into ``path``, which
153 # would then look like a valid tenant. Azure AD STS URLs are always https.
154 if parsed.scheme != "https" or not parsed.netloc:Lines 201-210 201 # pylint: disable=import-outside-toplevel,unused-argument
202 try:
203 from azure.identity import ClientSecretCredential
204 from azure.core.exceptions import ClientAuthenticationError
! 205 except ImportError as e:
! 206 raise RuntimeError(
207 "Azure authentication libraries are not installed. "
208 "Please install with: pip install azure-identity azure-core"
209 ) from e📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.build._deps.simdutf-src.src.haswell.implementation.cpp: 0.4%
mssql_python.pybind.build._deps.simdutf-src.src.implementation.cpp: 6.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.implementation.h: 10.4%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.scalar.utf16_to_utf8.utf16_to_utf8.h: 25.3%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.internal.isadetection.h: 65.3%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.2%🔗 Quick Links
|
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
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.
Summary
Adds
Authentication=ActiveDirectoryServicePrincipalsupport for bulk copy. Partial fix for #534 — addresses the exact error in the issue report ("Authentication method 'ActiveDirectoryServicePrincipal' is not supported. No token provider was registered for this method.").Why a callback rather than pre-acquire
The other Entra ID modes already supported in bulk copy (Default, DeviceCode, Interactive) pre-acquire a JWT in Python and pass it as
access_token. ServicePrincipal can't work that way:ClientSecretCredential(tenant_id, client_id, client_secret)requirestenant_idas a constructor argument.tenant_idon the connection string).So bulk copy must register a callback that's invoked mid-handshake. mssql-py-core hands the callback
(spn, sts_url, auth_method)and we parse the tenant from the STS URL on every call.Saurabh approved the callback approach in the EXP--Drivers--Python thread (May 13 2026). The matching
mssql-py-corechange (Rust-side wiring of the callback intoauth_method_map) is in mssql-rs PR and ships in mssql-py-core 0.1.5+.What changes
mssql_python/constants.pyAuthType.SERVICE_PRINCIPAL = "activedirectoryserviceprincipal".mssql_python/auth.pyServicePrincipalAuth.make_token_factory(client_id, client_secret)that returns the callable, plus_parse_tenant_id(). Updateprocess_auth_parameters()to detect SP (and leave ODBC's native query path alone), updateextract_auth_type()to propagate"serviceprincipal"for bulkcopy.mssql_python/cursor.py_auth_type == "serviceprincipal", build the factory from connection-string UID/PWD and register it via the newentra_id_token_factorydict key. Keepauthentication/user_name/passwordinpycore_context(the auth validator + transformer in py-core need them to resolve the method before the factory is dispatched). Existing Model A flow for Default/DeviceCode/Interactive untouched.tests/test_008_auth.pyCHANGELOG.mdFlow
Test
17 new tests cover:
TestAuthType::test_auth_type_constantsextended forSERVICE_PRINCIPALTestProcessAuthParameters::test_service_principal_auth_leaves_odbc_path_alone,test_service_principal_auth_case_insensitiveTestExtractAuthType::test_serviceprincipalTestParseTenantId: GUID, GUID without trailing slash, domain, query string, extra path segments, empty string, no pathTestServicePrincipalAuth: factory returns callable, requires non-emptyclient_idandclient_secret, returns UTF-16LE bytes, forwards credentials toClientSecretCredential, builds scope from SPN (and keeps/.defaultsuffix when already present), errors on unparseable STS URL, propagatesClientAuthenticationErrorasRuntimeErrorConnection string example
Same connection string keeps working for regular queries (ODBC handles ServicePrincipal natively in the non-bulk-copy path).
Out of scope
ActiveDirectoryPassword— needs the same callback shape but a different azure-identity credential class. Separate PR; py-core still rejects it today with a clear error.ActiveDirectoryIntegrated— needs SSPI/Kerberos wiring at the Rust layer, not a Python callback. Separate work.Dependencies
mssql-py-core0.1.5+ (mssql-rs PRdev/gaurav/entra-id-token-factory-py-callback). This Python PR will fail at import or runtime against earlier py-core versions because theentra_id_token_factorydict key is silently ignored there. Will update the dependency pin once the mssql-rs PR merges and a new wheel is published.