[8.0.0 Cherry-pick] Connection encryption info – Slice A: public API surface by github-actions[bot] · Pull Request #5037 · dotnet/SqlClient

github-actions · 2026-11-11T13:32:31Z

Cherry-pick of #4830 (d8a60de) into release/8.0.

Update: This PR now also includes b457732 and 93d5fd3, cherry-picks of 4114d5c and b172dc5 from #5043 (Slice A review follow-up on main), which addresses the review feedback on this PR.

* Add connection encryption info public API surface (Slice A) Adds SqlConnectionEncryptionInfo, SqlConnectionEncryptionMode, and SqlServerCertificateValidationErrors, plus SqlConnection.EncryptionInfo and SqlException.EncryptionInfo getters. In this slice both getters always return an empty snapshot; no TdsParser/SNI plumbing yet. Related to #2353. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Use SHA-256 certificate hash in SqlConnectionEncryptionInfo; add design note Replace PresentedServerCertificateThumbprint (string?) with PresentedServerCertificateHash256 (ReadOnlyMemory<byte>?), per the API agreed with the SSMS team. The internal constructor validates the hash is 32 bytes and keeps a private copy. Add doc/design-notes/2353 for reviewers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix doc include path and slice plan in design note Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Add NegotiatedCipherSuite to SqlConnectionEncryptionInfo Adds int? NegotiatedCipherSuite (IANA TLS cipher suite identifier selected by the server) to the public snapshot, ref assembly, XML docs, design note and unit tests. Proposal pending API review. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Cipher suite: NegotiatedCipherSuiteId (int?) on all TFMs; TlsCipherSuite? on .NET (review feedback) - Add NegotiatedCipherSuiteId (int?, CLS-compliant) on every target framework. It returns the IANA identifier unchanged, including unnamed/GREASE values. - On .NET only, NegotiatedCipherSuite exposes the same value as System.Net.Security.TlsCipherSuite? and is marked [CLSCompliant(false)]. - No NegotiatedCipherSuite member on net462/netstandard2.0, so the only shared member has an identical signature across the netstandard2.0 ref and the net10.0 implementation. - The internal constructor still takes int? (0..0xFFFF). The internal NegotiatedCipherSuiteValue is removed. - Update ref, XML docs, design note and unit tests. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Encryption info: replace certificate hash with GetServerCertificate() (threat model review) Remove PresentedServerCertificateHash256 and expose the server-presented leaf certificate via X509Certificate2? GetServerCertificate(). Each call returns a new caller-owned instance parsed from a private DER copy, or null when no certificate was captured or the bytes cannot be parsed. The internal ctor now takes byte[]? presentedServerCertificate, and PresentedServerCertificateRawData exposes the captured bytes internally. Update ref, XML docs, design note and unit tests. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Encryption info tests: register SqlErrorCollection known types for DataContract round trip Since #4622, SqlException.GetObjectData writes the error collection, so DataContractSerializer needs SqlErrorCollection, SqlError and List<object> as known types. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Encryption info: address Slice A review feedback - Sort SqlConnectionEncryptionInfo public members alphabetically in the implementation, reference assembly, XML doc snippets and design note. - Document how each SqlConnectionEncryptionMode value is negotiated and how much of the connection TLS covers. - Clarify that the internal PresentedServerCertificateRawData wraps the snapshot's private copy and must not be mutated. - Add XML docs to the test constants and helpers. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Docs: make SqlConnectionEncryptionInfo type remarks release-neutral Remove the statement that the driver never populates the snapshot; it becomes inaccurate once capture lands in later slices. Point to the getter remarks for when values are captured. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Docs: clarify negotiated protocol availability on failure Describe null as unavailable, not proof that no TLS handshake completed, and document that a protocol observed before a later handshake or connection failure is retained. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Docs: mark encryption info API approved Update the design note to show the approved API and remove outdated review caveats. Replace Slice A-only empty-snapshot wording in the getter remarks with the stable connection and exception snapshot contracts. Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c47d6846-090b-4240-9f75-efa50beb8e34

azure-pipelines · 2026-11-11T13:32:41Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

paulmedynski · 2026-11-11T15:19:11Z

+    /// <include file='../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnectionEncryptionInfo.xml' path='docs/members[@name="SqlConnectionEncryptionInfo"]/ValidationErrors/*'/>
+    public Microsoft.Data.SqlClient.SqlServerCertificateValidationErrors? ValidationErrors { get { throw null; } }
+    /// <include file='../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnectionEncryptionInfo.xml' path='docs/members[@name="SqlConnectionEncryptionInfo"]/GetServerCertificate/*'/>
+    public System.Security.Cryptography.X509Certificates.X509Certificate2 GetServerCertificate() { throw null; }


Missing nullable ? on the return type.

Good catch, fixed. The ref now declares X509Certificate2? GetServerCertificate(). Done in #5043 and cherry-picked here (b457732).

paulmedynski · 2026-11-11T15:33:14Z

+            byte[]? presentedServerCertificate,
+            SqlServerCertificateValidationErrors? validationErrors,
+            X509ChainStatusFlags? chainStatus,
+            int? negotiatedCipherSuite = null)


Prefer the TlsCipherSuite type on .NET, which keeps that value strongly-typed end-to-end. There shouldn't be any need for the .NET pathways to use an int.

Done. On .NET the internal constructor now takes TlsCipherSuite?, so the value can flow straight from SslStream.NegotiatedCipherSuite with no int round-trip. Only netfx keeps the int? id. In #5043, cherry-picked here.

paulmedynski · 2026-11-11T15:34:04Z

+        // enum member are preserved.
+        /// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnectionEncryptionInfo.xml' path='docs/members[@name="SqlConnectionEncryptionInfo"]/NegotiatedCipherSuite/*' />
+        [CLSCompliant(false)]
+        public System.Net.Security.TlsCipherSuite? NegotiatedCipherSuite =>


Prefer an auto-property here, and rely on the constructor to assign both properties consistently on .NET.

Changed to { get; }. On .NET the constructor assigns NegotiatedCipherSuite and NegotiatedCipherSuiteId from the same argument, so they can't disagree. #5043, cherry-picked here.

paulmedynski · 2026-11-11T15:44:58Z

+      </value>
+      <remarks>
+        <para>
+          This property never returns <see langword="null" /> and never throws, regardless of whether the connection has been opened, closed, or disposed. A <see langword="null" /> property value on the returned snapshot means the information is not available; it does not indicate that encryption or certificate validation succeeded.


How about this:

A null property value on the returned snapshot means the information was not available at the time the snapshot was obtained. Any snapshot may have any combination of null and non-null property values depending on the state of the underlying connection. The presence or absence of null property values does not indicate that encryption or certificate validation succeeded or failed."

Took your wording as-is. In #5043, cherry-picked here.

paulmedynski · 2026-11-11T15:49:03Z

+          This property never returns <see langword="null" /> and never throws, regardless of whether the connection has been opened, closed, or disposed. A <see langword="null" /> property value on the returned snapshot means the information is not available; it does not indicate that encryption or certificate validation succeeded.
+        </para>
+        <para>
+          After the connection opens successfully, the snapshot describes the physical connection in use, including a connection returned from the pool. Before opening and after the connection is closed or disposed, the snapshot has all properties set to <see langword="null" />. Closing the connection does not change a snapshot that was already obtained.


I think this is more clear:

Before the connection opens successfully, the snapshot is empty (all null properties) because it has no physical connection associated. Once opened, the snapshot describes the physical connection in use, including an existing physical connection obtained from a pool. Closing a connection resets the snapshot to its initial empty state.

Adopted. I also added that closing/disposing resets the property to the empty snapshot, and that a snapshot already obtained doesn't change since it's immutable. In #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:14:32Z

+      </value>
+      <remarks>
+        <para>
+          This property returns the same value as <see cref="P:Microsoft.Data.SqlClient.SqlConnectionEncryptionInfo.NegotiatedCipherSuiteId" />, using the type of <c>System.Net.Security.SslStream.NegotiatedCipherSuite</c>. An identifier that has no named member in the enumeration is preserved numerically and is not discarded. The <see langword="null" /> and failure semantics are the same as for <see cref="P:Microsoft.Data.SqlClient.SqlConnectionEncryptionInfo.NegotiatedCipherSuiteId" />.


There won't be any failure semantics when this becomes an auto-property.

Removed the failure-semantics text. The remark now just says the two properties are null together. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:21:20Z

+        A combination of <see cref="T:Microsoft.Data.SqlClient.SqlServerCertificateValidationErrors" /> values, or <see langword="null" /> if certificate validation did not run or its outcome is not available.
+      </value>
+      <remarks>
+        A value of <see cref="F:Microsoft.Data.SqlClient.SqlServerCertificateValidationErrors.None" /> means validation ran and no errors were observed, which is distinct from <see langword="null" />. When validation is bypassed (for example, with <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.TrustServerCertificate" />), this value can be <see langword="null" />.


Will this value always be null when validation is bypased? The term can is ambiguous.

Yes, it's always null when the driver doesn't validate. Managed SNI returns early under TrustServerCertificate without recording a verdict, and native only reports it when validation actually ran. The cert itself can still be captured, so I reworded it to "this value is null, even if the server presented a certificate". #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:24:39Z

+      </value>
+      <remarks>
+        <para>
+          This property never returns <see langword="null" />. A <see langword="null" /> property value on the returned snapshot means the information is not available; it does not indicate that encryption or certificate validation succeeded.


Same suggestion here related to null values as in other places in this PR.

Same wording as the other two, adapted to "depending on how far the connection attempt progressed". #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:25:55Z

+      <summary>
+        Specifies server certificate validation errors observed by the driver. This enumeration supports a bitwise combination of its member values.
+      </summary>
+      <remarks>


I don't think this remarks block is necessary, and it risks becoming stale if the Info object changes.

Removed the remarks block. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:27:43Z

+        A value of <see cref="F:Microsoft.Data.SqlClient.SqlServerCertificateValidationErrors.None" /> means validation ran and no errors were observed, which is distinct from <see langword="null" />. When validation is bypassed (for example, with <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.TrustServerCertificate" />), this value can be <see langword="null" />.
+      </remarks>
+    </ValidationErrors>
+    <GetServerCertificate>


Should we mention that this will throw fatal errors?

I didn't call out fatal errors since the BCL docs don't document those either. With the catch narrowed they just propagate. The remark now says undecodable bytes return null and drops the old "does not throw" claim. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:30:25Z

My review here will be addressed by @priyankatiwari08 on main and any resulting commit will be cherry-picked into this PR.

paulmedynski

Further comments related to the tests.

paulmedynski · 2026-11-11T16:34:00Z

+
+        #endregion
+
+        #region SqlConnection getter


These tests belong with the other SqlConnection tests - maybe in their own file if a sensible existing place doesn't exist.

Moved to a new SqlConnectionTests.cs, since there wasn't an existing SqlConnection unit test file. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:35:01Z

+            SqlConnectionEncryptionInfo info = new(null, null, null, null, null);
+
+            AssertEmpty(info);
+            Assert.True(info.IsEmpty);


I think AssertEmpty() should check this.

AssertEmpty() now also asserts IsEmpty. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:35:16Z

+            using SqlConnection connection = new();
+
+            Assert.NotNull(connection.EncryptionInfo);
+            Assert.True(connection.EncryptionInfo.IsEmpty);


Why not use AssertEmpty() ?

Switched to AssertEmpty(). #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:37:31Z

+
+        #endregion
+
+        #region SqlException getter


These belong with the othe SqlException tests.

Moved all the SqlException tests into SqlExceptionTests.cs. That file was entirely netfx-only, so now only the BinaryFormatter bits are under #if NETFRAMEWORK. #5043, cherry-picked here.

paulmedynski · 2026-11-11T16:38:50Z

+            Assert.Equal(SslProtocols.Tls12, info.NegotiatedEncryptionProtocol);
+            Assert.Equal(SampleCipherSuite, info.NegotiatedCipherSuiteId);
+            Assert.NotNull(info.PresentedServerCertificateRawData);
+            Assert.Equal(CreateSampleCertificate(), info.PresentedServerCertificateRawData!.Value.ToArray());


Why do we need the ! here? Assert.NotNull() should guarantee it isn't null.

Removed all of the !s. They're replaced by Assert.NotNull (which narrows) or the [NotNull] helper. #5043, cherry-picked here.

paulmedynski · 2026-11-11T18:19:09Z

+        {
+            SqlException ex = SqlException.CreateException(CreateErrorCollection(), ServerVersion);
+            ex.EncryptionInfo = CreatePopulated();
+            SqlException clone = ex.InternalClone();


Another test that belongs with the existing SqlException tests.

Moved to SqlExceptionTests.cs. #5043, cherry-picked here.

paulmedynski · 2026-11-11T18:20:26Z

+            Assert.Null(fromNull.PresentedServerCertificateRawData);
+            Assert.Null(fromEmpty.GetServerCertificate());
+            Assert.Null(fromEmpty.PresentedServerCertificateRawData);
+            Assert.True(fromEmpty.IsEmpty);


Do we expect fromNull to be empty as well? Not sure why fromNull specified the encryption mode, but fromEmpty didn't.

Made them consistent: fromNull passes all nulls and both are checked with AssertEmpty(). #5043, cherry-picked here.

paulmedynski · 2026-11-11T18:21:16Z

+            SqlConnectionEncryptionInfo info = new(null, null, bytes, null, null);
+
+            Assert.Null(info.GetServerCertificate());
+            Assert.False(info.IsEmpty);


Hmm... if the public API reports null for all properties and the cert, why wouldn't IsEmpty be true?

This one is deliberate. IsEmpty means "nothing was captured", while the bytes are only decoded lazily in GetServerCertificate(). Computing IsEmpty by parsing would make it expensive and able to fail. In practice the captured bytes always come from a completed handshake, so undecodable bytes only happen in this test. I renamed the test to UndecodableBytes and added a remark explaining this. Happy to change it if you'd rather IsEmpty decode. #5043, cherry-picked here.

paulmedynski · 2026-11-11T18:22:13Z

+
+            Assert.False(info.IsEmpty);
+            Assert.Null(info.NegotiatedEncryptionMode);
+            Assert.Null(info.ValidationErrors);


We should confirm that GetCertificate() doesn't return null.

Added Assert.NotNull. #5043, cherry-picked here.

paulmedynski · 2026-11-11T18:24:00Z

+        /// Asserts that a snapshot is non-null and that every field is unavailable.
+        /// </summary>
+        /// <param name="info">The snapshot to check.</param>
+        private static void AssertEmpty(SqlConnectionEncryptionInfo? info)


It might help to add the attribute to this method that says "When this returns, info is guaranteed to be non-null". That might help eliminate some ! in the tests.

AssertEmpty is now AssertEmpty([NotNull] SqlConnectionEncryptionInfo? info) (shared in Utilities/EncryptionInfoTestData.cs), so callers don't need !. #5043, cherry-picked here.

Follow-up to #4830 addressing review feedback on the release/8.0 cherry-pick (#5037). - Carry TlsCipherSuite end-to-end on .NET: the internal constructor takes TlsCipherSuite? and NegotiatedCipherSuite is an auto-property; .NET Framework keeps the range-checked int? identifier. - Make SqlConnectionEncryptionMode byte-backed. - Catch only CryptographicException in GetServerCertificate. - Annotate GetServerCertificate as returning X509Certificate2? in the ref. - Clarify null semantics and validation/decoding behavior in the docs and drop implementation details. - Move SqlConnection and SqlException tests into SqlConnectionTests and SqlExceptionTests, share helpers via EncryptionInfoTestData, and tighten assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Follow-up to #4830 addressing review feedback on the release/8.0 cherry-pick (#5037). - Carry TlsCipherSuite end-to-end on .NET: the internal constructor takes TlsCipherSuite? and NegotiatedCipherSuite is an auto-property; .NET Framework keeps the range-checked int? identifier. - Make SqlConnectionEncryptionMode byte-backed. - Catch only CryptographicException in GetServerCertificate. - Annotate GetServerCertificate as returning X509Certificate2? in the ref. - Clarify null semantics and validation/decoding behavior in the docs and drop implementation details. - Move SqlConnection and SqlException tests into SqlConnectionTests and SqlExceptionTests, share helpers via EncryptionInfoTestData, and tighten assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 4114d5c)

GenAPI reproduces the '?' on SqlConnectionEncryptionInfo.GetServerCertificate() but emitted no '#nullable' context into the auto-generated source (CS8669) and copied the compiler-reserved NullableContextAttribute on net10.0 (CS8335). Pass a header file that adds '#nullable enable annotations', matching the ref assembly, and exclude the Nullable/NullableContext attributes from the output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> (cherry picked from commit b172dc5)

github-actions Bot requested a review from a team as a code owner November 11, 2026 13:32

github-actions Bot added this to the 8.0.0 milestone Nov 11, 2026

github-project-automation Bot added this to SqlClient Board Nov 11, 2026

github-actions Bot assigned priyankatiwari08 Nov 11, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Nov 11, 2026

paulmedynski force-pushed the dev/automation/pr-4830-to-8.0.0 branch from b7ed861 to ecfb453 Compare November 11, 2026 15:28

mdaigle approved these changes Nov 11, 2026

View reviewed changes

paulmedynski requested changes Nov 11, 2026

View reviewed changes

github-project-automation Bot moved this from To triage to Waiting for customer in SqlClient Board Nov 11, 2026

paulmedynski requested changes Nov 11, 2026

View reviewed changes

priyankatiwari08 mentioned this pull request Nov 12, 2026

Connection encryption info – Slice A review follow-up #5043

Open

priyankatiwari08 and others added 2 commits November 12, 2026 12:44

Conversation

github-actions Bot commented Nov 11, 2026 • edited by priyankatiwari08 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines Bot commented Nov 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmedynski commented Nov 11, 2026

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions Bot commented Nov 11, 2026 •

edited by priyankatiwari08

Loading