-
Notifications
You must be signed in to change notification settings - Fork 4.9k
SqlClient fix managed encryption connection failure #40732
Conversation
src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SslOverTdsStream.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SslOverTdsStream.cs
Show resolved
Hide resolved
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:
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. |
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 |
I'm re-testing things as I'm kind of confused with changesets.. ignore the last comment. |
There was a problem hiding this 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!
How can I use this fix today? The only place I found it applied was in |
@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 |
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