Skip to content
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

remove support for Ssl2 #64322

Merged
merged 4 commits into from
Jan 28, 2022
Merged

remove support for Ssl2 #64322

merged 4 commits into from
Jan 28, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jan 26, 2022

fixes #35942

we may add test to verify we throw something reasonable if only SslProtocols.Ssl2 is requested but I'm not sure if it is worth of the effort.

@wfurt wfurt requested a review from a team January 26, 2022 09:21
@wfurt wfurt self-assigned this Jan 26, 2022
@ghost
Copy link

ghost commented Jan 26, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #35942

we may add test to verify we throw something reasonable if only SslProtocols.Ssl2 is requested but I'm not sure if it is worth of the effort.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security

Milestone: -

Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

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

One question, otherwise LGTM


if (_lastFrame.Header.Length < 0)
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, "invalid TLS frame size");
throw new IOException(SR.net_frame_read_size);
throw new AuthenticationException(SR.net_frame_read_size);
Copy link
Member

Choose a reason for hiding this comment

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

Why to change this exception when you leave IOException in GetFrameSize?
In other words: What is the rule to throw IOException vs. AuthenticationException?

Copy link
Member Author

Choose a reason for hiding this comment

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

ServerAsyncAuthenticate_InvalidHello_Throws checks for the exception type. In the past we can get one or the other depending on connection management. So I had to choose. Since getting invalid length did not feel like IO problem, I decided to use AuthenticationException

@wfurt
Copy link
Member Author

wfurt commented Jan 28, 2022

failing cryptography test is #64389

@wfurt wfurt merged commit 964b1d7 into dotnet:main Jan 28, 2022
@wfurt wfurt deleted the sslv2 branch January 28, 2022 23:45
@ghost ghost locked as resolved and limited conversation to collaborators Feb 28, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for SSLv2 should be removed from SslStream
2 participants