Skip to content

Add client certificate loopback authentication - #4556

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

Add client certificate loopback authentication#4556
JustinMDotNet wants to merge 8 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. Supported on every target framework, with both managed and native 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.
  • Managed SNI attaches the certificate to the client SslStream. Native SNI supplies it through SNIAuthProviderInfo.pfnCertificateCallback, which the shipping SNI binaries already honor; no SNI change is required.
  • 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.
  • 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. Certificate load failures surface as a SqlException on both SNI implementations.

Design notes

The API shape follows the JDBC driver (clientCertificate / clientKey / clientKeyPassword) rather than the single Certificate keyword prototyped in CTAIP, per guidance from the SNI/driver team, to keep the .NET and Java surfaces aligned. The CTAIP form accepts only subject: and sha1: store lookups and therefore cannot express the Linux loopback file scenario this issue asks for.

How native SNI is driven. An earlier revision of this PR assigned the certificate handle to SNIAuthProviderInfo.pCertContext. That was wrong, and it failed silently. In the SNI sources that produce the pinned Microsoft.Data.SqlClient.SNI 6.0.2 package, Ssl::AcquireCredentialsForClient gates solely on pwszCertId, and pCertContext is an output parameter that receives the resolved certificate; the field is never read as an input. SNI resolves a certificate by identifier — searching LocalMachine\MY, then CurrentUser\MY, and invoking pfnCertificateCallback only when both lookups miss.

This PR therefore supplies the certificate through that callback, which is live in the shipping provider. Two consequences are worth calling out:

  • The identifier passed to SNI is deliberately unmatchable (an all-zero SHA-1) rather than the certificate's real thumbprint. The caller named a file, so the file must be authoritative. Passing the real thumbprint would let a store entry with the same thumbprint win — and that entry may have been imported without its private key, which would break the handshake.
  • TdsParser previously requested the shared Schannel credential cache on every connection. SNI skips credential acquisition entirely when SNI_SSL_USE_SCHANNEL_CACHE is set, so the connection reused a cached credential that carried no certificate. That flag is now cleared when, and only when, a client certificate is configured, since such a credential is connection specific. Connections without a client certificate are unaffected, and managed SNI does not read the flag.

The managed SqlClientCertificateDelegate was a one-argument stub left over from the code removed in #2831 and did not match the native callback, so it has been redefined against the real signature. The certificate context handed to SNI is duplicated with CertDuplicateCertificateContext, because SNI releases it with CertFreeCertificateContext.

Scope and limitations

  • .NET Framework accepts PKCS#12 only. net462 has X509CertificateLoader through Microsoft.Bcl.Cryptography but not RSA.ImportFromPem, ImportPkcs8PrivateKey, or PemEncoding. Supporting a detached PEM key there would require a hand-written ASN.1 RSA key parser, which is not justified for a scenario whose target host is SQL Server on Linux. A detached Client Key therefore throws an error on net462 that names PKCS#12 as the supported alternative; a PFX or P12 containing the key works on all frameworks.
  • Native SNI presents the leaf certificate only. The callback returns a single PCCERT_CONTEXT, and SNIAuthProviderInfo has no field for an issuer chain. This matches the JDBC driver, whose PEM/DER path also builds a single-element chain (SQLServerCertificateUtils.readPKCS8Certificate calls keyStore.setKeyEntry(..., new Certificate[] {clientCertificate})). Managed SNI presents the full chain, which is a superset. Documented in the API remarks.
  • No Windows certificate store syntax. The ODBC subject: and sha1: forms are not accepted; that scaffolding was removed in Removed CTAIP, certificate authentication #2831. The JDBC driver does not support them either, so omitting them is the parity-preserving choice. They remain additive later — native SNI already resolves them through pwszCertId / fCertHash — but would need new managed-SNI code.
  • 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.

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, the keywords parse and round-trip identically to .NET, and a detached Client Key produces the documented "requires .NET" error.

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. The simulated server sets ClientCertificateRequired and asserts the exact thumbprint it observed, so the certificate is verified to reach the TLS layer rather than merely being configured.
  • The same matrix is run a second time over native SNI, which is what caught the pCertContext defect described above: those six cases failed with a null observed thumbprint against the previous implementation and pass now. Because the identifier passed to SNI is unmatchable by construction, a passing run proves the certificate arrived through the callback.
  • 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 contains the new properties for all four target frameworks.

End-to-end against a real SQL Server on Linux

Validated against SQL Server 2022 (16.0.4265.3) Developer Edition on Ubuntu 22.04, using a linux-x64 client built from this branch, running on the SQL Server host.

  • The certificate is presented and the server extracts its identity. Using the real launchpad-minted satellite credentials — /var/opt/mssql-extensibility/data/<launchpad-guid>/sqlsatellitecert.pem and sqlsatellitekey.pem, the exact files and formats the extensibility framework produces (PEM certificate plus PKCS#1 PEM key) — the server responded Login failed for user 'Microsoft Corporation'. That is the O of the satellite certificate's subject, so SQL Server received the certificate, parsed it, and derived the login identity from it. This confirms the certificate reaches the server and that LOGIN7 is interpreted as certificate authentication rather than as an anonymous login. Reproduced on Encrypt=Optional and Encrypt=Mandatory.
  • A second, independent path gives the same result. With a SQL-generated certificate (CREATE CERTIFICATE / CREATE LOGIN ... FROM CERTIFICATE, exported via BACKUP CERTIFICATE and converted from PVK to PEM), the server resolved the login as clientCertAuth and logged Error: 18456, State: 1 ... Reason: Infrastructure error occurred. Certificate-to-login resolution again succeeded; the rejection is the documented behavior that certificate-mapped logins cannot be used for ordinary connections.
  • The error paths behave correctly against a live server, surfacing SqlException wrapping AuthenticationException ("The client certificate or private key could not be loaded") for unreadable certificate material.

This exercises managed SNI, which is the implementation used in the real scenario, since the target host is Linux.

  • Unified product built for Windows (net462, net8.0, net9.0) and Unix (net8.0, net9.0), plus the reference assemblies.
  • Unit tests: net9.0 1151 passed / 0 failed; net462 1163 passed / 0 failed. The net462 run is split into two invocations because SimulatedServerTests leaves the net462 test host hanging after all of its tests pass — a pre-existing condition reproduced on an unmodified origin/main worktree, unrelated to this change.
  • 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.

Not yet validated

  • A fully authenticated session was not obtained. Reaching an accepted login requires the connection to originate from a genuine satellite process inside a live sp_execute_external_script session, where the launchpad has registered the session-to-certificate-to-user mapping. Standing that up needs a working R or Python runtime under the launchpad; on SQL Server 2022 the launchpad still binds the legacy mssql-mlservices launchers (libPythonLauncher.so / libRLauncher.so), which are not installable on 2022. Completing this would require a SQL Server 2019 host with the mssql-mlservices-* packages. The unverified remainder is the server's authorization decision, not the driver's presentation of the certificate.
  • Encrypt=Strict (TDS 8) was not confirmed against a live server. It fails on the test host during the TLS handshake — but so does sqlcmd with ODBC Driver 18 and -N s and no client certificate at all (TCP Provider: Error code 0x2746). That isolates it to the server instance's TDS 8 configuration rather than to this change. The Strict path, including tds/8.0 ALPN, remains covered by the simulated-server tests.

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, then GPT-5.6 Luna again on the native SNI work and once more on this fix. 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
JustinMDotNet and others added 2 commits August 20, 2026 17:25
- 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>
The native SNI path presented no client certificate: the TLS handshake
completed, but the server never received one. Two defects caused this.

First, `AuthProviderInfo.certContext` is never read as an input by the
shipping SNI provider. `Ssl::AcquireCredentialsForClient` gates only on
`pwszCertId`, and `pCertContext` is an output parameter, so assigning the
certificate handle to it was a silent no-op. SNI instead resolves a
certificate by identifier, searching the LocalMachine and CurrentUser
personal stores and falling back to `pfnCertificateCallback`. The callback
is now used, since the configured file is the authoritative source for the
connection. An unmatchable identifier is passed so that a store entry
sharing the certificate's thumbprint, possibly imported without its private
key, cannot take precedence over the file the caller named.

Second, `TdsParser` always requested the shared Schannel credential cache.
SNI skips acquiring credentials entirely when `SNI_SSL_USE_SCHANNEL_CACHE`
is set, so the connection reused a cached credential that carried no
certificate. That flag is now cleared when a client certificate is
configured, because such a credential is connection specific.

The managed `SqlClientCertificateDelegate` was a one-argument stub that did
not match the native callback, so it has been redefined, and the returned
certificate context is duplicated because SNI releases it.

The handshake test now also runs over native SNI, which is what caught
this. It fails against the previous implementation and passes now.

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