-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add more renegotiate tests #54609
Add more renegotiate tests #54609
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsWe disallow App data frame during renegotiation. Adding test to cover this scenario.
|
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.
generally looks reasonable. I left few comments.
clientOptions.LocalCertificateSelectionCallback = (sender, targetHost, localCertificates, remoteCertificate, acceptableIssuers) => | ||
{ | ||
//Assert.True(false, "Clent shouldn't send certificate in this test"); | ||
return clientCertificate; |
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.
do we even need the client certificate?
SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions() { ServerCertificate = serverCertificate }; | ||
serverOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => | ||
{ | ||
//Assert.True(false, "Server shouldn't receive certificate in this test"); |
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.
Do we even need the callback. It seems like we assert null certificate bellow.
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.
LGTM
seems like the failures are in the new tests. You should investigate @aik-jahoda |
|
||
Assert.Null(server.RemoteCertificate); | ||
|
||
Console.WriteLine("AA " + server.SslProtocol); |
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.
If anything, you should use _output.WriteLineI(). That would show up on test failure as well as it would be captured in testResults.xml
9cc6f06
to
4d31737
Compare
Merge with failures, those are unrelated to this PR |
We disallow App data frame during renegotiation. Adding test to cover this scenario.