Fix | Perform ServerCertificate pin validation to verify with server provided cert

[cheenamalhotra]

Description

Refines the managed SNI certificate-validation helper so that the ServerCertificate connection-string keyword behaves consistently with its documented "exact match" semantic.

Address internal item AB#46371

Two small refinements in SniCommon.ValidateSslServerCertificate. The existing design is unchanged: an exact ServerCertificate match still satisfies certificate validation.

1. Short-circuit narrowed to the pin-less path. The early-return on policyErrors == SslPolicyErrors.None now also requires validationCertFileName to be null/empty. When the caller has supplied a ServerCertificate, the helper always compares it against the presented server certificate — previously it was skipped entirely whenever the platform reported no policy errors. The no-pin fast path is unchanged.
2. Load failure fails closed. If the file can't be loaded/parsed, the helper now throws SSLCertificateAuthenticationException naming the configured path, instead of silently discarding the option and falling back to host-name validation — matching the caller's explicit intent.

Additionally, two defects surfaced in review:

• The loaded X509Certificate is now disposed via a narrow using scope (it owns unmanaged resources).
• A null server certificate now throws AuthenticationException rather than a NullReferenceException from the raw-bytes comparison.

The TrustServerCertificate=true short-circuit on the transport handles is unchanged — when validation is disabled at that layer, ServerCertificate remains unconsulted, matching the existing semantic.

Backwards compatibility

• No ServerCertificate supplied → identical behavior.
• ServerCertificate supplied, well-formed, and matches → identical behavior, including when the presented certificate would otherwise fail chain validation (for example a self-signed or private-CA certificate). This scenario deliberately continues to work.
• ServerCertificate supplied and the file is missing/unreadable, or it doesn't match the server cert → now throws SSLCertificateAuthenticationException at handshake, aligning runtime with the documented "exact match" contract. Both cases previously risked accepting a connection the caller intended to restrict.

Testing

Added SniCommonValidateSslServerCertificateTest, which exercises SniCommon.ValidateSslServerCertificate directly. Both the TCP and Named Pipes transports call into this same helper, so no transport-specific parameterization is needed.

Scenario	Expected
No cert configured + PolicyErrors.None	returns true
Match + PolicyErrors.None	returns true
Match + PolicyErrors.RemoteCertificateChainErrors	returns true (exact match satisfies validation)
Mismatch + PolicyErrors.None	throws AuthenticationException
Mismatch + PolicyErrors.RemoteCertificateChainErrors	throws AuthenticationException
Configured + null server cert + PolicyErrors.RemoteCertificateNotAvailable	throws AuthenticationException
Configured + null server cert + PolicyErrors.None	throws AuthenticationException
Unreadable file + PolicyErrors.None	throws AuthenticationException

Local run on net9.0: 8 passed / 0 failed.

Guidelines

Reviewed:

• Contributing
• Best Practices
• Coding Style
• Review Process

[Copilot]

Pull request overview

This PR refines Managed SNI’s TLS server-certificate validation so the ServerCertificate connection-string keyword behaves as an explicit certificate pin (and updates docs/tests to reflect the intended semantics).

Changes:

• Tightens the “policyErrors == None” fast path so it only applies when no ServerCertificate pin is provided.
• Changes pin-load failures to fail closed by throwing an authentication exception.
• Adds unit tests for pin vs. no-pin behavior and updates documentation to clarify “additive” pinning semantics.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs	Adjusts certificate-validation control flow for pin handling and fail-closed behavior.
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ManagedSni/SniCommonValidateSslServerCertificateTest.cs	Adds unit coverage for SniCommon.ValidateSslServerCertificate pin/no-pin scenarios.
doc/snippets/Microsoft.Data.SqlClient/SqlConnectionStringBuilder.xml	Documents ServerCertificate as an additive check (pin + standard validation).
doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml	Expands connection-string keyword docs to clarify additive pinning and fail-closed behavior.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Files not reviewed (1)

• src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file

[paulmedynski]

What is platform trust? Is it a broadly understood industry term? How is it different than the checks we perform in ValidateSslServerCertificate?

[cheenamalhotra]

Good catch — "platform trust" was unnecessary jargon. Reworded to "chain-and-name validation", which is exactly what SslStream performs and reports back to us via SslPolicyErrors. ValidateSslServerCertificate does not re-do that work; it interprets the reported SslPolicyErrors and then applies the additional ServerCertificate pin check.

[paulmedynski]

Interesting, so it isn't possible to supply a self-signed server cert pin and use it to validate? I guess you could also load the server cert into the store and then the chain-and-name would validate as well, but that seems like an extra hoop to jump through. This is all existing behaviour though, so we're stuck with it I guess.

[cheenamalhotra]

Correct — this is pre-existing behavior and unchanged here. The pin only narrows which certificate is acceptable; it does not establish trust. A self-signed server cert still has to be trusted (installed in the OS store) or the connection must use TrustServerCertificate=true. I have added a sentence to both doc snippets making that explicit so users are not surprised.

[cheenamalhotra]

Following up: keeping the existing behavior here. An exact match does satisfy validation, so a self-signed server cert supplied via ServerCertificate works today without loading it into the store — I had briefly changed that and have reverted it, since it would have been a breaking change for exactly this scenario.

[paulmedynski]

Why must policyErrors be None? Do we mean that chain/name checks are only performed when policyErrors is None? What do we do for other values of policyErrors?

[cheenamalhotra]

Reworded the doc comment. policyErrors is the result of the chain/name validation SslStream already performed before invoking this callback — we do not perform those checks ourselves. None simply means SslStream found no chain or name problem. The docs now describe the two ordered steps: (1) inspect every set policyErrors flag and fail with a descriptive AuthenticationException unless it can be resolved (a name mismatch may still be satisfied by HostNameInCertificate), and (2) apply the ServerCertificate pin check on top.

[paulmedynski]

We can narrow the scope of this variable to inside the try block on line 95:

try
{
    using X509Certificate validationCertificate = ...;

    // Do span checks...
}
catch (Exception e)
{
    // Load failed...
    throw ...;
}

I think this also eliminates the try-finally on line 91.

[cheenamalhotra]

Done. Extracted ValidateCertificatePin and LoadValidationCertificate, so the certificate is now a using declaration in a narrow scope and the outer try/finally is gone. Splitting the load into its own method also keeps the load-failure catch from swallowing the mismatch AuthenticationException.

[paulmedynski]

You could avoid the try/finally in each test if this became a disposable class:

private class TempCertFile : IDisposable
{
  public string Path { get; }

  public TempCertFile(X509Certificate2 cert)
  {
    string tempPath = ...
    File.WriteAllBytes(tempPath, cert...);
    Path = tempPath;
  }

  public void Dispose()
  {
    File.Delete(Path);
  }
}

Then in the tests:

using TempCertFile pinFile = new(pinCert);

ValidateSslServerCertificate(..., pinFile.Path, ...);

[cheenamalhotra]

Done — added a TempCertFile : IDisposable helper; every test now uses using TempCertFile pinFile = new(cert); and the try/finally blocks are gone.

[paulmedynski]

What should happen if serverCert isn't null, but policyErrors is RemoveCertificateNotAvailable? Logically I think we're saying that we don't expect a serverCert for certain policy error values, but the method docs don't talk about that relationship, and I'm not sure the method enforces it.

[cheenamalhotra]

The relationship is now documented on the method, and the ordering change makes it moot for the normal path: RemoteCertificateNotAvailable is handled in step 1 and always throws, regardless of whether serverCert happens to be non-null. For the inverse (a pin supplied with serverCert == null and policyErrors == None, which should not happen), ValidateCertificatePin now has an explicit null guard that throws AuthenticationException instead of NREing. Both cases have regression tests.

[paulmedynski]

If this new pin checking is additive, then why are we performing it before all of the existing policy error checks? I think it would make more sense to perform this after the existing checks (i.e. below line 190).

[cheenamalhotra]

Agreed — reordered. The pin check now runs after all SslPolicyErrors handling and after the messageBuilder.Length > 0 throw, so the additive semantics are explicit in the control flow: policy errors always fail first, and the pin can only add a further failure, never rescue one. The existing MatchingPin_ChainErrors_Throws test still covers the invariant.

[cheenamalhotra]

Reverting this — I had reordered the pin check to run after the policy-error handling, but that turned out to be a real breaking change and is beyond the intended scope of this PR. Under the existing design an exact ServerCertificate match satisfies validation, which is what makes Encrypt=Mandatory;ServerCertificate=<path> work today against a self-signed or private-CA server certificate without TrustServerCertificate=true. Making the check additive would break those users. The design is now unchanged from main; this PR keeps only the two intended fixes: (1) the policyErrors == None fast path no longer skips a configured ServerCertificate, and (2) a ServerCertificate that cannot be loaded/parsed fails closed rather than being silently ignored.

[github-actions]

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 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 (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs:119

• The trace format string for the RemoteCertificateChainErrors path uses incorrect placeholder indices (it prints the connection id where targetServerName should be, and targetServerName where the policy error should be). This makes TLS troubleshooting logs misleading.

                    SqlClientEventSource.Log.TrySNITraceEvent(nameof(SniCommon), EventType.ERR, "Connection Id {0}, targetServerName {0}, SslPolicyError {1}, SSL Policy certificate chain has errors.", args0: connectionId, args1: targetServerName, args2: policyErrors);

[Copilot]

Pull request overview

Copilot reviewed 5 out of 6 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 (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ManagedSni/SniCommon.netcore.cs:121

• The log format string for the RemoteCertificateChainErrors branch uses incorrect placeholders (it repeats {0} for targetServerName and never prints policyErrors). This produces misleading traces when diagnosing TLS failures.

                if (policyErrors.HasFlag(SslPolicyErrors.RemoteCertificateChainErrors))
                {
                    SqlClientEventSource.Log.TrySNITraceEvent(nameof(SniCommon), EventType.ERR, "Connection Id {0}, targetServerName {0}, SslPolicyError {1}, SSL Policy certificate chain has errors.", args0: connectionId, args1: targetServerName, args2: policyErrors);