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

Add more renegotiate tests #54609

Merged
merged 4 commits into from
Jun 30, 2021
Merged

Conversation

aik-jahoda
Copy link
Contributor

We disallow App data frame during renegotiation. Adding test to cover this scenario.

@ghost
Copy link

ghost commented Jun 23, 2021

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

Issue Details

We disallow App data frame during renegotiation. Adding test to cover this scenario.

Author: aik-jahoda
Assignees: -
Labels:

area-System.Net.Security

Milestone: -

Copy link
Member

@wfurt wfurt left a 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;
Copy link
Member

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");
Copy link
Member

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.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt
Copy link
Member

wfurt commented Jun 25, 2021

seems like the failures are in the new tests. You should investigate @aik-jahoda


Assert.Null(server.RemoteCertificate);

Console.WriteLine("AA " + server.SslProtocol);
Copy link
Member

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

@aik-jahoda
Copy link
Contributor Author

Merge with failures, those are unrelated to this PR

@aik-jahoda aik-jahoda merged commit 5b58501 into dotnet:main Jun 30, 2021
@aik-jahoda aik-jahoda deleted the jajajhoda/tlsrenegotests branch June 30, 2021 13:41
@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
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.

3 participants