Merged
Conversation
stephentoub
reviewed
Mar 27, 2020
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs
Outdated
Show resolved
Hide resolved
Contributor
|
Is the test here intending to be testing sync over async? If not, can it just be made async? |
stephentoub
approved these changes
Mar 30, 2020
| } | ||
| else | ||
| { | ||
| return !IsOSX && (OpenSslVersion.CompareTo(new Version(1,1,1)) >= 0); |
Member
There was a problem hiding this comment.
macOS doesn't support TLS 1.3 on any version?
Member
There was a problem hiding this comment.
In that case can you please change this to something like:
else if (IsOSX)
{
// [ActiveIssue("https://github.com/dotnet/runtime/issues/1979")]
return false;
}
else
{
return OpenSslVersion >= new Version(1,1,1);
}? Thanks.
| } | ||
| else | ||
| { | ||
| return !IsOSX && (OpenSslVersion.CompareTo(new Version(1,1,1)) >= 0); |
Member
There was a problem hiding this comment.
Might be a little bit more understandable as:
Suggested change
| return !IsOSX && (OpenSslVersion.CompareTo(new Version(1,1,1)) >= 0); | |
| return !IsOSX && OpenSslVersion >= new Version(1,1,1); |
Member
Author
|
I don't think the IO matters much @scalablecory. I would not touch it but before the test change the tests were just hanging with broken tts13. I did not know if there is easier way how to make the original GetByte() within given timeout. I also feel it is good to have variety in test coverage. If we make everything Async, we may miss some bugs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is follow-up on #32925. While that PR made it generally safe e.g. avoided concurrent decrypt/encrypt beyond OpenSSL it did not address few remaining issues.
With TLS1.3 "renegotiation" can happen without any read. That left framing
unknownand caused exception visible in #1720. That also left some test hanging because of incorrect logic inForceAuthenticationAsync.We do not have CI to cover this but all tests are passing on my insider preview build.
TLS13 test should light-up when we have newer Windows versions in CI and when registry is set. (right now TLS13 is opt-in feature)
fixes #1720