Repository navigation
Conversation
Include the token logon session in Windows pool identities for both networking modes and prevent channel pool warmup under a mismatched identity. Add regression coverage for both pools, sync and async checkout, and real-server invalid-credential isolation, with documentation and an impersonation sample. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
LogonUsercan fail without initializing theout SafeAccessTokenHandle token(it may be null),… · NewDbConnectionPoolIdentity.GetCurrent()can be relatively expensive on Windows (token queries + SID… · NewEqualsnow includes_isRestrictedand_isNetwork, but the cached_hashCodedoes not,… · New10). Introduce a small private enum (e.g.,… · NewlogonTypeandlogonProviderarguments are passed as raw integers (9,3). Use named… · New
What changed in this PR
Fixes Windows netonly impersonation pooling so connection pools (and background replenishment) are keyed by logon session (AuthenticationId) rather than only by local SID, preventing cross-session reuse and poisoned pools.
Changes:
- Add
AuthenticationIdtoDbConnectionPoolIdentityequality/hash/caching and use Win32 token queries on Windows. - Add a logon-session guard to channel pool warmup to skip background creation under mismatched identities.
- Add unit + manual regression coverage and documentation/sample for impersonation + pooling behavior.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/DbConnectionPoolIdentityTest.cs | Adds Windows unit tests validating identity distinction, pool selection, replenishment behavior, and token-query error surfacing. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/IntegratedAuthenticationTest/PoolIdentityImpersonationTest.cs | Adds real-server manual regression tests for invalid netonly credentials and replenishment/login reversion scenarios. |
| src/Microsoft.Data.SqlClient/tests/Common/WindowsImpersonationHelper.cs | Adds helper to create NEW_CREDENTIALS (netonly) tokens for tests without storing credentials. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolIdentity.cs | Extends identity to include AuthenticationId and switches Windows identity acquisition to token-statistics based approach. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | Adds identity check to prevent warmup under the wrong logon session. |
| src/Microsoft.Data.SqlClient/src/Interop/Windows/Kernel32/Kernel32.cs | Adds P/Invoke for SetLastError used to disambiguate IsTokenRestricted failures. |
| src/Microsoft.Data.SqlClient/src/Interop/Windows/Advapi32/Advapi32.cs | Adds token-statistics and restricted-token queries with Win32 error propagation. |
| doc/samples/ConnectionPool_Impersonation.cs | Adds sample demonstrating how to open a pooled integrated-security connection within impersonation scope. |
| doc/connection-pooling-impersonation.md | Documents pooling identity behavior under impersonation and replenishment tradeoffs; links to tests/sample. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Guard failed netonly token creation, name the native token/logon constants, include all identity fields in the hash, and query the warmup identity once per pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Windows authentication pooling and native token interop warrant human review, especially because successful alternate-account scenarios were skipped.
0 open findings
5 resolved since last review
LogonUsercan fail without initializing theout SafeAccessTokenHandle token(it may be null),…Equalsnow includes_isRestrictedand_isNetwork, but the cached_hashCodedoes not,…DbConnectionPoolIdentity.GetCurrent()can be relatively expensive on Windows (token queries + SID…logonTypeandlogonProviderarguments are passed as raw integers (9,3). Use named…10). Introduce a small private enum (e.g.,…
🧠 Review effort: Balanced
CI agents accept placeholder network-only credentials over loopback, so a successful open cannot prove pool sharing there. Verify the precondition with an unpooled open and skip when it does not hold. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4814 +/- ##
==========================================
- Coverage 72.18% 64.73% -7.45%
==========================================
Files 292 287 -5
Lines 45341 68627 +23286
==========================================
+ Hits 32728 44425 +11697
- Misses 12613 24202 +11589
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
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.
Description
Windows network-only logons retain the process account's local SID while using different outbound credentials. Pooling by SID alone can therefore reuse connections authenticated under another logon session or cache its login failures, including during background replenishment.
Include the token's
AuthenticationIdin Windows pool identity equality, hashing, and identity-cache lookup for both native and managed networking. The existing wait-handle pool replenishment guard now distinguishes logon sessions; the channel pool gains the equivalent guard before background connection creation. Token-query failures surface as Win32 errors.Duplicated tokens from the same logon session still share a pool. Separate logon sessions for the same account now use separate pools. Background replenishment skips mismatched identities, so
Min Pool Sizeis not guaranteed across impersonation boundaries; opens under the owning identity can create connections on demand. There are no public API changes. Documentation and an impersonation sample describe the behavior and tradeoffs.Issues
Fixes #4717
Testing
Added Windows unit coverage for distinct network-only identities, pool selection, mismatched replenishment in both directions, sync/async checkout, native/managed networking, and invalid token handles.
Guidelines
Contributing
Code of Conduct
Best Practices
Coding Style
Review Process
Tests added or updated
Public API changes documented (no public API changes)
Verified against customer repro (related local real-server regression reproduced; exact customer setup not run)
Ensure no breaking changes introduced (intentional pool partitioning and replenishment behavior changes documented above)