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.

Sign up for GitHub

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

do not call RemoteCertificateValidationCallback on the same certificate again #42836

Merged
merged 4 commits into from
Sep 30, 2020
Merged

do not call RemoteCertificateValidationCallback on the same certificate again #42836

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a2492b
do not call RemoteCertificateValidationCallback on the same certifica…
wfurt Sep 29, 2020
3742b5d
feedback from review
wfurt Sep 30, 2020
833e55b
feedback from review
wfurt Sep 30, 2020
fea66b1
make sure we do not use Linq
wfurt Sep 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.SslProtocols.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,12 @@ public static IEnumerable<object[]> GetAsync_AllowedSSLVersion_Succeeds_MemberDa
[MemberData(nameof(GetAsync_AllowedSSLVersion_Succeeds_MemberData))]
public async Task GetAsync_AllowedSSLVersion_Succeeds(SslProtocols acceptedProtocol, bool requestOnlyThisProtocol)
{
int count = 0;
using (HttpClientHandler handler = CreateHttpClientHandler())
using (HttpClient client = CreateHttpClient(handler))
{
handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
handler.ServerCertificateCustomValidationCallback =
(request, cert, chain, errors) => { count++; return true; };

if (requestOnlyThisProtocol)
{
Expand Down Expand Up @@ -131,6 +133,8 @@ await TestHelper.WhenAllCompletedOrAnyFailed(
server.AcceptConnectionSendResponseAndCloseAsync(),
client.GetAsync(url));
}, options);

Assert.Equal(1, count);
}
}

Expand Down
12 changes: 11 additions & 1 deletion src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot

try
{
_remoteCertificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
X509Certificate2? remoteCertificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
wfurt marked this conversation as resolved.
Show resolved Hide resolved

if (remoteCertificate != null && _remoteCertificate != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

What if both are null? Is this possible? Should we call the callback again in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case we will fall-through and hit the "normal" case. SslPolicyErrors.RemoteCertificateNotAvailable will be set.
If peer certificate was required (and that is true on client side) that would lead to negotiation failure unless that was override by the callback for some reason. To prevent this form happening twice, we would probably need to add another property to track if this was already called. I simply use presence of certificate to detect that now but it can still probably happen on server side. It feels like unlikely corner case and it should not impact the SocketHttpClient nor be TLS 13 specific. The callback is only one way for app to know "something" e.g. renegotiation happened. So I think it is ok to be called multiple times in general case.

The interesting scenario IMHO would be TLS resume where session can be established without full exchange. I don't know if Schannel remembers the certificates from the previous session and if that generally works same way with OpenSSL. (resume currently not working on Linux but I'm hoping to fix that)

remoteCertificate.Equals(_remoteCertificate))
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
// This is renegotiation or TLS 1.3 and the certificate did not change.
// There is no reason to process callback again as we already established trust.
return true;
}

_remoteCertificate = remoteCertificate;

if (_remoteCertificate == null)
{
Expand Down
7 changes: 6 additions & 1 deletion src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ public async Task SslStream_NestedAuth_Throws()
public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
{
string targetName = useEmptyName ? string.Empty : Guid.NewGuid().ToString("N");
int count = 0;

(Stream clientStream, Stream serverStream) = TestHelper.GetConnectedStreams();
using (clientStream)
Expand All @@ -248,7 +249,7 @@ public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
{
SslStream stream = (SslStream)sender;
Assert.Equal(targetName, stream.TargetHostName);

count++;
return true;
};

Expand All @@ -265,8 +266,12 @@ public async Task SslStream_TargetHostName_Succeeds(bool useEmptyName)
await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
client.AuthenticateAsClientAsync(clientOptions),
server.AuthenticateAsServerAsync(serverOptions));

await TestHelper.PingPong(client, server);

Assert.Equal(targetName, client.TargetHostName);
Assert.Equal(targetName, server.TargetHostName);
Assert.Equal(1, count);
}
}

Expand Down
33 changes: 33 additions & 0 deletions src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using System.Security.Cryptography.X509Certificates.Tests.Common;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading.Tasks;
using Xunit;

namespace System.Net.Security.Tests
{
Expand All @@ -30,6 +32,9 @@ public static class TestHelper
private static readonly X509BasicConstraintsExtension s_eeConstraints =
new X509BasicConstraintsExtension(false, false, 0, false);

private static readonly byte[] s_ping = Encoding.UTF8.GetBytes("PING");
private static readonly byte[] s_pong = Encoding.UTF8.GetBytes("PONG");

public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams()
{
(Stream clientStream, Stream serverStream) = GetConnectedStreams();
Expand Down Expand Up @@ -151,5 +156,33 @@ internal static (X509Certificate2 certificate, X509Certificate2Collection) Gener

return (endEntity, chain);
}

internal static async Task PingPong(SslStream client, SslStream server)
{
byte[] buffer = new byte[s_ping.Length];
ValueTask t = client.WriteAsync(s_ping);

int remains = s_ping.Length;
while (remains > 0)
{
int readLength = await server.ReadAsync(buffer, buffer.Length - remains, remains);
Assert.True(readLength > 0);
remains -= readLength;
}
Assert.Equal(s_ping, buffer);
await t;

t = server.WriteAsync(s_pong);
remains = s_pong.Length;
while (remains > 0)
{
int readLength = await client.ReadAsync(buffer, buffer.Length - remains, remains);
Assert.True(readLength > 0);
remains -= readLength;
}

Assert.Equal(s_pong, buffer);
await t;
}
}
}