Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

SqlClient fix managed encryption connection failure #40732

Merged
merged 3 commits into from Sep 4, 2019

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Aug 31, 2019

Fixes https://github.com/dotnet/corefx/issues/40476

Fixes a bug in encapsulation of ssl over tds when connecting to SqlServer using the managed implementation (non-windows and uap). This is a slight rework of the original code (not the buggy replacement).

It has been run through the manual tests with and without entyption enabled in the connection string and fails in no places that the unix manual tests do not already fail. When tested manually under a debugger to ensure it works it correctly allows connection and read of an entire table using the managed interface, this means that switch from encapsulation to streamed encryption works correctly.

/cc @wfurt @stephentoub @bartonjs @rmja

this will need porting to M.D.SqlClient when the PR that introduces the issue is ported @cheenamalhotra @David-Engel

@Wraith2 Wraith2 changed the title Sqlbug 40476 SqlClient fix managed encryption connection failure Aug 31, 2019
cheenamalhotra added a commit to cheenamalhotra/SqlClient that referenced this pull request Sep 3, 2019
@cheenamalhotra
Copy link
Member

@Wraith2

It has been run through the manual tests with and without entyption enabled in the connection string and fails in no places that the unix manual tests do not already fail.

Have you run the Manual Tests on Ubuntu and do they pass? Please share your execution commands, in case I'm missing something.

I couldn't get them passing, hence ported over the changes to M.D.S just to recheck my environment with something that I configured and know works with "master" branch in Ubuntu pipelines, but I can't make the ported PR #35363 (alongwith this PR changes) pass. I get the below error in all tests:

A connection was successfully established with the server, but then an error occurred during the pre-login handshake. (provider: SSL Provider, error: 31 - Encryption(ssl/tls) handshake failed)

I'm still trying to work with it if I can catch the actual issue, but if you can provide some details, that would be helpful. What I'm most concerned right now is M.D.S (master) works fine while this PR + old one ported over doesn't work.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Sep 4, 2019

I didn't try it on ubuntu. I use the debug version of the code and force it into managed mode using the System.Data.SqlClient.UseManagedSNIOnWindows environment variable. This method won't find problem that are caused by the platform/PAL but as CoreCLR has matured far fewer runtime problems are found than library problems. I ran the test suite in managed and unmanaged modes using encryption and not for master and my branch and got the same results in each run with this fix. I then stepped into the code in the reproduction application i'd used to confirm the problem and verified that the expected behaviour (you know, actually working) occured.

One thing to note is that if you're setting Encrypt=true in the connection string you must also set TrustServerCertificate=true or the ssl connection will fail, that might be the cause here.

I didn't attempt to port this fix to M.D.SqlClient because that doesn't contain the original PR which introduced the error. Since i'm still trying to establish goals on M.D.SqlClient i thought that porting that PR would take some time and wasn't urgent. It is urgent in S.D.SqlClient because it is blocking 3.0

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Sep 4, 2019

@Wraith2

I'm re-testing things as I'm kind of confused with changesets.. ignore the last comment.

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

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

The changes work well, tested with Encrypted connection strings on different clients!

@johnsimons
Copy link

How can I use this fix today? The only place I found it applied was in 5.0.0-alpha1.19465.2, is there going to be another preview package?

@danmoseley
Copy link
Member

@johnsimons if it missed the current preview it will be in the final release later this month. If you need to test on it now you could install the preview then build it yourself and hand patch this one binary

@karelz karelz added this to the 5.0 milestone Dec 19, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants