Skip to content

allow access to crypto props from RemoteCertificateValidationCallback #37580

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

Merged
wfurt merged 1 commit into from
Jun 9, 2020
Merged

allow access to crypto props from RemoteCertificateValidationCallback #37580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 19 additions & 8 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ public virtual SslProtocols SslProtocol
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand Down Expand Up @@ -560,7 +560,7 @@ public virtual TlsCipherSuite NegotiatedCipherSuite
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
return _context!.ConnectionInfo?.TlsCipherSuite ?? default(TlsCipherSuite);
}
}
Expand All @@ -569,7 +569,7 @@ public virtual CipherAlgorithmType CipherAlgorithm
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand All @@ -583,7 +583,7 @@ public virtual int CipherStrength
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand All @@ -598,7 +598,7 @@ public virtual HashAlgorithmType HashAlgorithm
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand All @@ -612,7 +612,7 @@ public virtual int HashStrength
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand All @@ -627,7 +627,7 @@ public virtual ExchangeAlgorithmType KeyExchangeAlgorithm
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand All @@ -642,7 +642,7 @@ public virtual int KeyExchangeStrength
{
get
{
ThrowIfExceptionalOrNotAuthenticated();
ThrowIfExceptionalOrNotHandshake();
SslConnectionInfo? info = _context!.ConnectionInfo;
if (info == null)
{
Expand Down Expand Up @@ -863,6 +863,17 @@ private void ThrowIfExceptionalOrNotAuthenticated()
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Member

@stephentoub stephentoub Jun 8, 2020

Choose a reason for hiding this comment

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

Are the call sites very hot paths? Wondering if it's really beneficial to force inline it like this.

Copy link
Member Author

@wfurt wfurt Jun 8, 2020

Choose a reason for hiding this comment

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

I can try to measure it. For now, I just followed pattern from other helper methods.
Generally I would not expect them on hot path. The properties would either never be used or used once for logging after handshake is completed.

private void ThrowIfExceptionalOrNotHandshake()
{
ThrowIfExceptional();

if (!IsAuthenticated && _context?.ConnectionInfo == null)
{
ThrowNotAuthenticated();
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void ThrowIfExceptionalOrNotAuthenticatedOrShutdown()
{
Expand Down
25 changes: 23 additions & 2 deletions src/libraries/System.Net.Security/tests/FunctionalTests/ClientAsyncAuthenticateTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ public async Task ClientAsyncAuthenticate_ServerRequireEncryption_ConnectWithEnc
await ClientAsyncSslHelper(EncryptionPolicy.RequireEncryption);
}

[Fact]
public async Task ClientAsyncAuthenticate_ConnectionInfoInCallback_DoesNotThrow()
{
await ClientAsyncSslHelper(EncryptionPolicy.RequireEncryption, SslProtocols.Tls12, SslProtocolSupport.DefaultSslProtocols, AllowAnyServerCertificateAndVerifyConnectionInfo);
}

[Fact]
public async Task ClientAsyncAuthenticate_ServerNoEncryption_NoConnect()
{
Expand Down Expand Up @@ -139,7 +145,8 @@ private Task ClientAsyncSslHelper(SslProtocols clientSslProtocols, SslProtocols
private async Task ClientAsyncSslHelper(
EncryptionPolicy encryptionPolicy,
SslProtocols clientSslProtocols,
SslProtocols serverSslProtocols)
SslProtocols serverSslProtocols,
RemoteCertificateValidationCallback certificateCallback = null)
{
_log.WriteLine("Server: " + serverSslProtocols + "; Client: " + clientSslProtocols);

Expand All @@ -150,7 +157,7 @@ private async Task ClientAsyncSslHelper(
{
server.SslProtocols = serverSslProtocols;
await client.ConnectAsync(server.RemoteEndPoint.Address, server.RemoteEndPoint.Port);
using (SslStream sslStream = new SslStream(client.GetStream(), false, AllowAnyServerCertificate, null))
using (SslStream sslStream = new SslStream(client.GetStream(), false, certificateCallback != null ? certificateCallback : AllowAnyServerCertificate, null))
{
Task clientAuthTask = sslStream.AuthenticateAsClientAsync("localhost", null, clientSslProtocols, false);
await clientAuthTask.TimeoutAfter(TestConfiguration.PassingTestTimeoutMilliseconds);
Expand All @@ -173,6 +180,20 @@ private bool AllowAnyServerCertificate(
return true; // allow everything
}

private bool AllowAnyServerCertificateAndVerifyConnectionInfo(
object sender,
X509Certificate certificate,
X509Chain chain,
SslPolicyErrors sslPolicyErrors)
{
SslStream stream = (SslStream)sender;

Assert.NotEqual(SslProtocols.None, stream.SslProtocol);
Assert.NotEqual(CipherAlgorithmType.None, stream.CipherAlgorithm);

return true; // allow everything
}

#endregion Helpers
}
}