Skip to content

Add client certificate loopback authentication - #4556

Draft
JustinMDotNet wants to merge 7 commits into
dotnet:mainfrom
JustinMDotNet:dev/automation/client-certificate-auth
Draft

Add client certificate loopback authentication#4556
JustinMDotNet wants to merge 7 commits into
dotnet:mainfrom
JustinMDotNet:dev/automation/client-certificate-auth

Conversation

@JustinMDotNet

@JustinMDotNet JustinMDotNet commented Aug 19, 2026

Copy link
Copy Markdown

Description

Adds client certificate authentication for SQL Server on Linux loopback connections, matching the documented behavior of the ODBC and JDBC drivers. Implemented in managed SNI.

  • Adds the Client Certificate / ClientCertificate, Client Key / ClientKey, and Client Key Password / ClientKeyPassword connection-string keywords and the matching SqlConnectionStringBuilder properties. Paths are plain file paths, as in the JDBC driver.
  • Signals certificate authentication with the TDS PRELOGIN CLIENT_CERT (0x80) bit and sends LOGIN7 with an empty user name and password. Authentication completes during the TLS handshake, so no new SqlAuthenticationMethod value is introduced.
  • Determines the certificate container format from the file contents, never from the file extension. A PKCS#12 (PFX or P12) file supplies its own private key, including ECDSA. A PEM or DER certificate requires Client Key, which accepts unencrypted RSA PKCS#1 and RSA PKCS#8 keys, or encrypted PKCS#8 when Client Key Password is set.
  • Selects the end-entity certificate from a bundle by content rather than by position, and retains the issuer chain so intermediates are presented during the handshake.
  • Requires an encrypted connection. If the server declines encryption the connection fails instead of silently sending an unauthenticated, anonymous LOGIN7 record. Encrypt=Optional, Mandatory, and Strict are all supported when the server negotiates encryption, on both the sync and async open paths.
  • Treats Client Key Password as sensitive connection information and always redacts it from tracing, regardless of Persist Security Info.
  • Rejects conflicting configuration: SQL credentials, integrated security, Microsoft Entra authentication, access tokens, SqlCredential, a custom SSPI context provider, and SqlConnection.ChangePassword. Conflict detection is value-based, so an empty User ID= or Integrated Security=false does not conflict.
  • Reports actionable errors for the ODBC file: path prefix, password-protected PKCS#1 keys, non-RSA detached keys, and a Client Key combined with a PKCS#12 certificate.

Scope and limitations

  • .NET only. The keywords, the builder properties, and the reference-assembly surface are #if NET gated. On .NET Framework the keywords are unknown and the connection string is rejected by the parser, because managed networking is unavailable there.
  • Managed SNI only. On .NET with native SNI, a configured client certificate throws PlatformNotSupportedException rather than being silently ignored. Native SNI ships as an external NuGet package with no source in this repository, so native support has to be a separate cross-repo change.
  • No Windows certificate store syntax. The ODBC subject: and sha1: forms are not accepted; that scaffolding was removed in Removed CTAIP, certificate authentication #2831.
  • Certificate rotation. The certificate is read when a pooled physical connection is created and cached for that connection's lifetime. Picking up a rotated certificate requires SqlConnection.ClearPool or Pooling=false.
  • Sample. doc/samples/SqlConnection_ClientCertificateAuthentication.cs is guarded with #if false because the Samples project compiles against the released Microsoft.Data.SqlClient package, which does not yet expose these properties. The guard should be removed once a package containing them ships.

Release note: Added Client Certificate, Client Key, and Client Key Password connection-string support for SQL Server on Linux loopback certificate authentication on .NET when using managed networking.

Issues

Fixes #4551

Testing

Connection-string and API surface

  • Keyword and alias parsing, key-without-certificate validation, credential conflict detection, empty-credential-keyword non-conflict, and confirmation that the container format is not inferred from the file extension.
  • Client Key Password redaction from both the public connection string and the trace string.
  • SqlConnectionStringBuilder round-trip and Clear.
  • Conflicts with SqlCredential, AccessToken, AccessTokenCallback, and both ChangePassword overloads.
  • On .NET Framework, all six keyword spellings are rejected by both SqlConnectionOptions and the builder indexer.

Certificate loader

  • Encrypted PFX, PEM certificate with an encrypted PKCS#8 key, PEM chain, PFX chain, and ECDSA PFX.
  • PKCS#12 content loaded from .pem, .cer, and extensionless paths, and PEM content loaded from a .pfx path, confirming content-based detection.
  • Deterministic leaf selection from a PKCS#12 bundle in which an issuer also carries a private key.
  • Rejection of detached ECDSA keys (PKCS#8 and SEC1), password-protected PKCS#1 keys, a Client Key paired with a PKCS#12 certificate, the ODBC file: prefix, expired certificates, incorrect passwords, missing keys, and unsupported PEM key labels.

Simulated TDS/TLS server

  • PRELOGIN 0x80, client certificate presentation, and empty LOGIN7 user name and password across Optional, Mandatory, and Strict, sync and async.
  • Certificate load failure surfaced as SqlException wrapping AuthenticationException, sync and async.
  • Server declining encryption fails the connection, asserting that no bytes at all are sent after PRELOGIN, sync and async.

Builds and runs

  • Full solution build (39 projects), 0 warnings and 0 errors, including the Samples project and the GenAPI-generated "not supported" assemblies. The generated surface correctly contains the new properties for net8.0 and net9.0 and omits them for net462 and netstandard2.0.
  • Unified product built for Windows (net462, net8.0, net9.0) and Unix (net8.0, net9.0), plus the reference assemblies.
  • Unit tests: 1144 passed on net8.0, 1145 on net9.0, and 1148 on net462; 0 failed on all three.
  • Functional tests run in full and compared against an origin/main worktree on the same machine: identical pre-existing failure counts (2 on net9.0, 86 on net462, all Always Encrypted certificate-store and SqlDataRecordTest.GetUdt_ReturnsValue environmental failures). No regressions; this branch passes six more tests than the baseline.

Not yet validated

  • A real SQL Server on Linux loopback environment was not available locally, so end-to-end login has not been demonstrated. That remains a merge gate for CI or maintainer testing.

Review

  • Reviewed with Claude Sonnet 5, then independently with GPT-5.6 Sol and Claude Opus 5, then GPT-5.6 Luna on the remediation, then a codebase-pattern conformance pass. All findings were addressed or answered in the review threads.

  • Tests added or updated

  • Public API changes documented

  • Verified against customer repro (requires a SQL Server on Linux loopback certificate environment)

  • Ensure no breaking changes introduced

Guidelines

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 19, 2026 22:28
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Aug 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds client-certificate loopback authentication support (managed SNI) by introducing new connection-string options, wiring them through TDS prelogin/TLS enablement, and updating validation/redaction, documentation, and test coverage.

Changes:

  • Adds Client Certificate / ClientCertificate, Client Key / ClientKey, and Client Key Password / ClientKeyPassword options (plus builder properties + ref docs).
  • Implements managed-SNI client certificate loading and passes the certificate context into TLS authentication; updates PRELOGIN encryption flags and LOGIN7 credential suppression.
  • Adds unit + functional + simulated-server tests, plus documentation snippet updates and a sample.

Reviewed changes

Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ClientCertificateAuthenticationTests.cs New simulated-server coverage for PRELOGIN flag + TLS client-cert presentation + empty LOGIN7 credentials.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs Validates parsing/aliases/conflicts and verifies redaction behavior for client-key passwords.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoaderTests.cs New unit coverage for certificate/key loading paths (PFX, PEM/DER, chains, algorithm handling, error normalization).
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniPacketTests.cs Updates test SNI handle stub to match new EnableSsl signature.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionTest.cs Adds functional validation for conflicts with SqlCredential / AccessToken / callback; updates invalid keyword list.
src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs Adds keyword parsing coverage and round-trip verification for new builder properties.
src/Microsoft.Data.SqlClient/src/Resources/Strings.resx Adds new user-facing strings for conflicts, platform gating, and certificate-load failures.
src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs Updates strongly-typed resource accessors for the new strings.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs Adds client-cert parameters to EnableSsl and explicitly rejects client cert auth on native SNI.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.netcore.cs Plumbs new client-cert parameters through to managed SNI EnableSsl.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs Extends abstract EnableSsl contract to include client-certificate inputs.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserHelperClasses.cs Ensures CLIENT_CERT prelogin flag is available across TFMs (while runtime-gated elsewhere).
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs Sets/propagates client-cert handshake state and updates encryption negotiation handling.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs Adds a helper exception factory for client-certificate auth conflicts.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionStringBuilder.cs Adds new connection-string builder properties and keyword/synonym wiring.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs Adds option parsing/validation (including conflicts + redaction support) for client cert + key + password.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs Enforces conflict rules when mixing client cert auth with credential/token/SSPI mechanisms.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoader.netcore.cs Adds managed-SNI certificate/key loading implementation with chain support and error normalization.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniTcpHandle.netcore.cs Passes client-certificate context into TLS client authentication and disposes cert context on handle dispose.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniNpHandle.netcore.cs Same as TCP handle changes, for NP transport.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniMarsHandle.netcore.cs Updates MARS wrapper to forward new EnableSsl signature.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniMarsConnection.netcore.cs Updates MARS connection to forward new EnableSsl signature.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniHandle.netcore.cs Adds client cert context caching and new TLS authentication helpers used by ManagedSNI.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs Omits username/password and credential object from LOGIN7 when using client certificate auth.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringSynonyms.cs Adds no-space synonyms for the new client certificate keywords.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringKeywords.cs Adds canonical keyword strings for the new connection-string options.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringDefaults.cs Adds default values for the new connection-string options.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionString.netfx.cs Extends sensitive-value masking to cover ClientKeyPassword in redacted strings.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Adds new exception helpers for mixed-usage client certificate authentication scenarios.
src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.cs Updates reference surface for new SqlConnectionStringBuilder properties.
doc/snippets/Microsoft.Data.SqlClient/SqlConnectionStringBuilder.xml Documents new builder properties and their behavioral constraints.
doc/samples/SqlConnection_ClientCertificateAuthentication.cs Adds an environment-variable-based sample showing new connection-string usage.
Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs Outdated
- Replace sync-over-async TLS handshake with the synchronous
  AuthenticateAsClient(SslClientAuthenticationOptions) overload.
- Fail the connection when a server declines encryption instead of
  silently sending an empty, anonymous LOGIN7 record.
- Reject ChangePassword for certificate-authenticated connections.
- Detect the certificate container format from file contents rather
  than the file extension.
- Base credential conflict detection on values rather than keyword
  presence so empty keywords no longer conflict.
- Gate the keywords, builder properties, and reference assembly
  surface behind #if NET; the keywords are unknown on .NET Framework.
- Select the end-entity certificate from a PKCS#12 bundle by content.
- Reject the ODBC 'file:' path syntax and report encrypted PKCS#1
  private keys with an actionable error.
- Revert the unrelated ServerCertificate copy-constructor fix.
- Expand tests and documentation to cover the above.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 00:02
@JustinMDotNet

Copy link
Copy Markdown
Author

Follow-up commit ddec2da4 addresses the review feedback from the full-PR reviews.

Behavior changes since the initial commit

Area Change
TLS handshake Replaced AuthenticateAsClientAsync(...).GetAwaiter().GetResult() with the synchronous AuthenticateAsClient(SslClientAuthenticationOptions) overload. No sync-over-async on the open path.
Encryption negotiation If the server declines encryption (negotiated encryption is neither ON nor LOGIN and TLS is not first), the connection now fails with SQL_ClientCertificateRequiresEncryption instead of silently sending an empty, anonymous LOGIN7 record.
ChangePassword Both static overloads now throw when the connection string uses client certificate authentication.
Container format The certificate format is detected from file contents, not the file extension. ClientCertificate=client.pem without ClientKey is no longer rejected on the basis of its name.
Conflict detection Credential conflicts are now value-based. User ID=, Password=, and Integrated Security=false no longer conflict with certificate authentication.
.NET Framework The three keywords, the builder properties, and the reference-assembly surface are #if NET gated. On net462 the keywords are simply unknown.
PKCS#12 bundles The end-entity certificate is selected by content (basic constraints plus in-bundle issuer relationships) rather than by position.
Error reporting The ODBC file: path prefix and password-protected PKCS#1 private keys now produce specific, actionable errors.
Split out The unrelated ServerCertificate copy-constructor fix was reverted and will ship as its own PR.

Validation

  • Product builds: Windows net462/net8.0/net9.0, Unix net8.0/net9.0, and the reference assemblies — 0 warnings, 0 errors.
  • Unit tests: 52 passed on net8.0 and net9.0, 32 passed on net462.
  • Functional tests: net9.0 and net462 pass apart from one pre-existing, unrelated failure (ConnectionString_AttachDbFileName_DataDirectory_NoLinuxRootFolder).

Still gating merge

A successful login against a real SQL Server on Linux loopback instance has not yet been demonstrated. The simulated TDS/TLS server tests cover the PRELOGIN flag, the presented certificate, the empty LOGIN7 record, and the encryption-degradation failure, but they are not a substitute for an end-to-end run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs:180

  • PR description says the connection-string API is available across target frameworks and that .NET Framework should fail explicitly with PlatformNotSupportedException when certificate authentication is used. However, these keywords are only added to the keyword map under #if NET, which means on .NET Framework the connection string fails earlier with KeywordNotSupported/ArgumentException and the API surface is not available.
#if NET
            // Client certificate authentication is implemented by managed SNI only, so the keywords
            // are not recognized on .NET Framework where managed networking is unavailable.
            AddKeywordToMap(DbConnectionStringKeywords.ClientCertificate,
                            DbConnectionStringSynonyms.ClientCertificate);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKey,
                            DbConnectionStringSynonyms.ClientKey);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKeyPassword,
                            DbConnectionStringSynonyms.ClientKeyPassword);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoader.netcore.cs:164

  • LoadPkcs12 throws a CryptographicException when the PKCS#12 bundle contains no private key, but Load(...) catches CryptographicException and wraps it as AuthenticationException(SQL_ClientCertificateLoadFailed). That loses the more actionable SQL_ClientCertificateMissingPrivateKey message at the top level (it only survives as an inner exception).
                int leafCertificateIndex = FindLeafCertificateIndex(certificates);
                if (leafCertificateIndex < 0)
                {
                    throw new CryptographicException(StringsHelper.GetString(Strings.SQL_ClientCertificateMissingPrivateKey));
                }

@cheenamalhotra

Copy link
Copy Markdown
Member

Hey @JustinMDotNet - looks like there are some conflicts that need resolution.

A PKCS#12 bundle already contains its private key, so pairing it with
Client Key is a misconfiguration. The detached-key path previously
reported this as an unspecified certificate-load failure. Detect the
container format from the file contents and report the conflict
explicitly instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 16:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs:180

  • The client-certificate keywords are only added to the parser keyword map under #if NET, so on net462 these keywords are rejected during connection-string parsing (ArgumentException) rather than being accepted and failing at open-time with PlatformNotSupportedException. This contradicts the PR description’s stated scope for .NET Framework behavior; either update the PR description to match the current implementation, or remove the conditional keyword gating and instead throw PlatformNotSupportedException when attempting to use client-certificate auth on unsupported platforms.
#if NET
            // Client certificate authentication is implemented by managed SNI only, so the keywords
            // are not recognized on .NET Framework where managed networking is unavailable.
            AddKeywordToMap(DbConnectionStringKeywords.ClientCertificate,
                            DbConnectionStringSynonyms.ClientCertificate);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKey,
                            DbConnectionStringSynonyms.ClientKey);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKeyPassword,
                            DbConnectionStringSynonyms.ClientKeyPassword);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoader.netcore.cs:164

  • LoadPkcs12 throws a CryptographicException when no certificate in the PKCS#12 bundle has a private key. That exception is immediately wrapped by Load(...) into a generic AuthenticationException(SQL_ClientCertificateLoadFailed), which hides the more specific SQL_ClientCertificateMissingPrivateKey message that the rest of the loader uses. Throw AuthenticationException(SQL_ClientCertificateMissingPrivateKey) directly here so callers get the intended actionable error.
                int leafCertificateIndex = FindLeafCertificateIndex(certificates);
                if (leafCertificateIndex < 0)
                {
                    throw new CryptographicException(StringsHelper.GetString(Strings.SQL_ClientCertificateMissingPrivateKey));
                }

…-certificate-auth

# Conflicts:
#	src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionOptionsTest.cs
Copilot AI review requested due to automatic review settings August 20, 2026 17:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoader.netcore.cs:164

  • When a PKCS#12 bundle contains no private key, LoadPkcs12 throws a CryptographicException which is caught by Load(...) and rethrown as SQL_ClientCertificateLoadFailed. This prevents callers from seeing the specific SQL_ClientCertificateMissingPrivateKey message that you already use for other missing-key cases.
                int leafCertificateIndex = FindLeafCertificateIndex(certificates);
                if (leafCertificateIndex < 0)
                {
                    throw new CryptographicException(StringsHelper.GetString(Strings.SQL_ClientCertificateMissingPrivateKey));
                }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionStringBuilder.cs:55

  • The PR description states the connection-string API is available across target frameworks and that .NET Framework fails explicitly with PlatformNotSupportedException. However these new keywords/properties are compiled under #if NET, which makes them unavailable on net462 (and related tests assert ArgumentException for netfx). Either update the PR description/scope, or remove the compile-time gating and allow parsing on .NET Framework with a runtime PlatformNotSupportedException during Open/EnableSsl.
            Encrypt,
            HostNameInCertificate,
            ServerCertificate,
#if NET
            ClientCertificate,
            ClientKey,
            ClientKeyPassword,
#endif

- Guard the documentation sample with #if false so the Samples project,
  which builds against the released package, still compiles. Follow the
  sample conventions: file-scoped namespace, Snippet1 markers, no
  license header, and a reference from the ClientCertificate snippet.
- Throw AuthenticationException through ADP.SSLCertificateAuthenticationException
  so certificate failures are traced like other managed SNI failures,
  and add the inner-exception overload that requires.
- Order the client certificate snippet entries alphabetically, matching
  the rest of SqlConnectionStringBuilder.xml.
- Document the certificate loader types and members, matching the other
  ManagedSni files.
- Order [PasswordPropertyText] on ClientKeyPassword as on Password, and
  place ChangePasswordConflictsWithClientCertificate next to its sibling.
- Cover both the synchronous and asynchronous open paths in the
  certificate failure and encryption degradation tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SqlClientCertificateLoader.netcore.cs:193

  • LoadPkcs12 throws a CryptographicException for the “missing private key” case, but Load catches CryptographicException and rethrows a generic SQL_ClientCertificateLoadFailed AuthenticationException. This prevents the more actionable SQL_ClientCertificateMissingPrivateKey message from ever surfacing for PKCS#12 bundles that lack a private key.
                int leafCertificateIndex = FindLeafCertificateIndex(certificates);
                if (leafCertificateIndex < 0)
                {
                    throw new CryptographicException(StringsHelper.GetString(Strings.SQL_ClientCertificateMissingPrivateKey));
                }

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs:181

  • The PR description says the client-certificate connection-string API is available across target frameworks and that .NET Framework fails explicitly with PlatformNotSupportedException. In the current implementation the keywords are not recognized on .NET Framework (#if NET around AddKeywordToMap), so a netfx caller will instead get an ArgumentException (unsupported keyword) at parse time. Please align the PR description with this behavior, or remove the #if NET gating and ensure netfx reaches the intended PlatformNotSupportedException path.
#if NET
            // Client certificate authentication is implemented by managed SNI only, so the keywords
            // are not recognized on .NET Framework where managed networking is unavailable.
            AddKeywordToMap(DbConnectionStringKeywords.ClientCertificate,
                            DbConnectionStringSynonyms.ClientCertificate);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKey,
                            DbConnectionStringSynonyms.ClientKey);
            AddKeywordToMap(DbConnectionStringKeywords.ClientKeyPassword,
                            DbConnectionStringSynonyms.ClientKeyPassword);
#endif

Per team guidance, native SNI already ships client certificate support,
so the feature no longer requires managed networking.

SNI exposes three inputs through SNIAuthProviderInfo: a Windows
certificate store subject lookup, a store SHA1 lookup, and a caller
supplied CERT_CONTEXT. The declared client certificate callback is not
invoked by the shipping provider, so the certificate is handed over as
a CERT_CONTEXT, which preserves the JDBC style file path contract.

- Correct AuthProviderInfo.certContext and clientCertificateCallbackContext
  to IntPtr, matching the native CERT_CONTEXT* and PVOID fields.
- Move the certificate loader out of ManagedSni so both SNI
  implementations share it, and build it for every target framework.
- Load the certificate in TdsParserStateObjectNative.EnableSsl and pass
  X509Certificate2.Handle as certContext, releasing it on dispose.
- Drop the PlatformNotSupportedException and un-gate the connection
  string keywords, builder properties, and reference assembly surface.
- On .NET Framework support PKCS#12 containers, which carry their own
  private key, and report a specific error for a detached Client Key
  because RSA.ImportFromPem and ImportPkcs8PrivateKey do not exist there.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 21:42
Copilot stopped reviewing on behalf of JustinMDotNet due to an error August 20, 2026 22:02
- Root the certificate across the SNIAddProvider call. X509Certificate2.Handle
  does not keep its certificate alive, so the managed object could be collected
  while native SNI still held the PCCERT_CONTEXT.
- Dispose any certificate left by a previous attempt before loading another.
- Convert a certificate load failure on the native path into a SqlError that
  carries the original AuthenticationException, matching how managed SNI
  reports the same failure. It previously escaped as a raw
  AuthenticationException and bypassed the connection retry loop.
- Document that native SNI presents only the end-entity certificate, since
  SNIAuthProviderInfo has no field for the issuer chain.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

Support ClientCertificate keyword in Connection String

3 participants