-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve handling of handshake failure #35549
Conversation
Tagging subscribers to this area: @dotnet/ncl |
I also enabled ServerAsyncAuthenticate_MismatchProtocols_Fails. The test no longer depends on timeout/IOException and it should be more stable. (time will tell) fixes #29642 |
src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs
Outdated
Show resolved
Hide resolved
public const SslProtocols AllProtocols = | ||
SslProtocols.Ssl2 | SslProtocols.Ssl3 | | ||
#pragma warning disable CS0618 // Ssl3 are obsolete | ||
public const SslProtocols AllProtocols = SslProtocols.Ssl3 | |
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.
Why was Ssl2 removed?
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.
Ssl2 does not have the framing protocol So determining length and type is somewhat more complicated. And I did not want to bring that to the TLS parser. With Ssl3 being deprecated and Ssl2 not supported by any of the current Ones I felt that would be ok.
rfc6176 from 2011 prohibts use of Ssl2.
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.
Does that mean your change removes Ssl2 support from SslStream?
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.
yes, I mention that in description. I can mark this as breaking change to make that more visible. I can also split this or roll back that part and I keep old processing around.
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.
I think we need to have a broader conversation about that and whether we want to explicitly permanently disable SSL2 in .NET 5. On the one hand, we don't want anyone using SSL2; it's insecure, deprecated, etc. On the other hand, we know we've gotten pushback in the past because of apps talking to, e.g., services and devices that are out of their control. For the most part then we've left it up to whether the underlying operating system / platform supports it. This would be explicitly disabling such support even when it's enabled in the OS. We can choose to make that decision, but we need to do so thoughtfully.
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.
We shouldn't remove it completely yet from .NET 5. We still seem to run into customer issues where they have some very old device that they can't change but it talks some legacy protocol. We don't want to prevent customers from using .NET 5 due to this.
At some point, we probably would drop support from it. But I think .NET 5 is not the release to do that.
It would be interesting to get other data points such as does OpenSsl still support SSL2 in some fashion?
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.
ok, I'll split it. maybe open new issue about retiring Sslv2 to be more explicit?
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.
does OpenSsl still support SSL2 in some fashion?
They deleted all the SSLv2 code for OpenSSL 1.1.0. So Ubuntu 16.04 can probably still do it (OpenSSL 1.0.2), but in another year I don't think there will be any supported Linux configurations that can do SSLv2.
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.
While Ubuntu16 has -ssl2, attempt to use it throws error so as forced run of ClientAsyncAuthenticate_Ssl2WithSelf_Success. It seems like !PlatformDetection.IsWindows10Version1607OrGreater
is only viable option.
I created #35942 to track this and rolled-back Ssl2 changes.
I agree that it should not be side-effect of this change (even if convenient in this case)
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs
Outdated
Show resolved
Hide resolved
contributes to #2359 |
I rolled back all the Ssl2 changes and all tests are green. |
This is done by paying more attention to the handshake it self. In the past, we would just buffer TLS frames and pass them to OS. When handshake fail, underlying OS libraries may or may not sent Alert. With this change, we will always sent alert and we will get consistent behavior across all supported OSes. (e.g. client will always get Authentication exception instead of sometimes Authentication and sometimes IO exception)
closes #914
The other part is that sometimes we can get pretty generic (or lame) error from OS.
Example from #33186 for cipher mismatch
With this, we would see the Alert coming and produce something more meaningful. Type and inner exceptions are identical, but the test should now be more useful.
closes #33186
To make it easier, I moved some things around and I created helper to do parsing in more consolidated way. This also removes craft added to support Sslv2. It is dead for a while as none of the supported OS supports such archaic version but the code lived there never less.
more cleanup, parsing and event tracing will probably come but I felt this is good stoping point with fix for two active issues.