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

Jump to bottom

PoC TLS resume on Linux client #64369

Merged
merged 17 commits into from
Mar 25, 2022
Merged

PoC TLS resume on Linux client #64369

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e52b12f
TLS resume on client
wfurt Jan 27, 2022
1811028
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Jan 27, 2022
f9a2eab
shim functions introduced in 1.1.1
wfurt Feb 1, 2022
9b87b4e
add missing struct
wfurt Feb 1, 2022
fd5635d
disable resume on old OpenSSL
wfurt Feb 3, 2022
99240d4
feedback from review
wfurt Feb 4, 2022
342d916
fix source build
wfurt Feb 4, 2022
acd3b46
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Feb 18, 2022
51ec560
update comment
wfurt Feb 18, 2022
dc453d4
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Feb 25, 2022
2170ec4
feedback from review
wfurt Mar 3, 2022
929e5c7
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Mar 3, 2022
c2c8580
feedback from review
wfurt Mar 3, 2022
d2ab19b
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Mar 3, 2022
576d4d5
avoild client resume on old OpenSSL
wfurt Mar 5, 2022
876b5a0
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Mar 7, 2022
434ab42
Merge branch 'main' of https://github.com/dotnet/runtime into tlsClie…
wfurt Mar 24, 2022
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
133 changes: 110 additions & 23 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
Expand All @@ -21,6 +22,7 @@ internal static partial class OpenSsl
private const string DisableTlsResumeCtxSwitch = "System.Net.Security.DisableTlsResume";
private const string DisableTlsResumeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_DISABLETLSRESUME";
private static readonly IdnMapping s_idnMapping = new IdnMapping();
private static readonly ConcurrentDictionary<SslProtocols, SafeSslContextHandle> s_clientSslContexts = new ConcurrentDictionary<SslProtocols, SafeSslContextHandle>();

#region internal methods
internal static SafeChannelBindingHandle? QueryChannelBinding(SafeSslHandle context, ChannelBindingKind bindingType)
Expand Down Expand Up @@ -124,7 +126,7 @@ private static SslProtocols CalculateEffectiveProtocols(SslAuthenticationOptions
}

// This essentially wraps SSL_CTX* aka SSL_CTX_new + setting
internal static SafeSslContextHandle AllocateSslContext(SafeFreeSslCredentials credential, SslAuthenticationOptions sslAuthenticationOptions, SslProtocols protocols, bool enableResume)
internal static unsafe SafeSslContextHandle AllocateSslContext(SafeFreeSslCredentials credential, SslAuthenticationOptions sslAuthenticationOptions, SslProtocols protocols, bool enableResume)
{
SafeX509Handle? certHandle = credential.CertHandle;
SafeEvpPKeyHandle? certKeyHandle = credential.CertKeyHandle;
Expand Down Expand Up @@ -161,16 +163,13 @@ internal static SafeSslContextHandle AllocateSslContext(SafeFreeSslCredentials c

Debug.Assert(cipherSuites == null || (cipherSuites.Length >= 1 && cipherSuites[cipherSuites.Length - 1] == 0));

unsafe
fixed (byte* cipherListStr = cipherList)
fixed (byte* cipherSuitesStr = cipherSuites)
{
fixed (byte* cipherListStr = cipherList)
fixed (byte* cipherSuitesStr = cipherSuites)
if (!Ssl.SslCtxSetCiphers(sslCtx, cipherListStr, cipherSuitesStr))
{
if (!Ssl.SslCtxSetCiphers(sslCtx, cipherListStr, cipherSuitesStr))
{
Crypto.ErrClearError();
throw new PlatformNotSupportedException(SR.Format(SR.net_ssl_encryptionpolicy_notsupported, sslAuthenticationOptions.EncryptionPolicy));
}
Crypto.ErrClearError();
throw new PlatformNotSupportedException(SR.Format(SR.net_ssl_encryptionpolicy_notsupported, sslAuthenticationOptions.EncryptionPolicy));
}
}

Expand All @@ -183,14 +182,26 @@ internal static SafeSslContextHandle AllocateSslContext(SafeFreeSslCredentials c
// https://www.openssl.org/docs/manmaster/ssl/SSL_shutdown.html
Ssl.SslCtxSetQuietShutdown(sslCtx);

Ssl.SslCtxSetCaching(sslCtx, enableResume ? 1 : 0);

if (sslAuthenticationOptions.IsServer && sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0)
if (enableResume)
{
unsafe
if (sslAuthenticationOptions.IsServer)
{
Interop.Ssl.SslCtxSetAlpnSelectCb(sslCtx, &AlpnServerSelectCallback, IntPtr.Zero);
Ssl.SslCtxSetCaching(sslCtx, 1, null, null);
}
else
{
Ssl.SslCtxSetCaching(sslCtx, 1, &NewSessionCallback, &RemoveSessionCallback);
sslCtx.EnableSessionCache();
}
}
else
{
Ssl.SslCtxSetCaching(sslCtx, 0, null, null);
}

if (sslAuthenticationOptions.IsServer && sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0)
{
Interop.Ssl.SslCtxSetAlpnSelectCb(sslCtx, &AlpnServerSelectCallback, IntPtr.Zero);
}

bool hasCertificateAndKey =
Expand Down Expand Up @@ -266,25 +277,44 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia
SafeSslContextHandle? newCtxHandle = null;
SslProtocols protocols = CalculateEffectiveProtocols(sslAuthenticationOptions);
bool hasAlpn = sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0;
bool cacheSslContext = !DisableTlsResume && sslAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.RequireEncryption &&
sslAuthenticationOptions.IsServer &&
sslAuthenticationOptions.CertificateContext != null &&
sslAuthenticationOptions.CertificateContext.SslContexts != null &&
sslAuthenticationOptions.CipherSuitesPolicy == null;
bool cacheSslContext = !DisableTlsResume && sslAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.RequireEncryption && sslAuthenticationOptions.CipherSuitesPolicy == null &&
((sslAuthenticationOptions.IsServer &&
sslAuthenticationOptions.CertificateContext != null &&
sslAuthenticationOptions.CertificateContext.SslContexts != null) ||
(sslAuthenticationOptions.IsClient &&
// since SNI us our key, we don't wont to resume sessions without it.
!string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) &&
// on client avoid resume with certificates
sslAuthenticationOptions.CertificateContext == null &&
sslAuthenticationOptions.CertSelectionDelegate == null));
wfurt marked this conversation as resolved.
Show resolved Hide resolved

if (cacheSslContext)
{
sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (SslProtocols)(hasAlpn ? 1 : 0), out sslCtxHandle);
if (sslAuthenticationOptions.IsServer)
{
sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (SslProtocols)(hasAlpn ? 1 : 0), out sslCtxHandle);
}
else
{

wfurt marked this conversation as resolved.
Show resolved Hide resolved
s_clientSslContexts.TryGetValue(protocols, out sslCtxHandle);
}
}

if (sslCtxHandle == null)
{
// We did not get SslContext from cache
sslCtxHandle = newCtxHandle = AllocateSslContext(credential, sslAuthenticationOptions, protocols, cacheSslContext);

if (cacheSslContext && sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), newCtxHandle))
if (cacheSslContext)
{
newCtxHandle = null;
bool added = sslAuthenticationOptions.IsServer ?
sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), newCtxHandle) :
s_clientSslContexts.TryAdd(protocols, newCtxHandle);
wfurt marked this conversation as resolved.
Show resolved Hide resolved
if (added)
{
newCtxHandle = null;
}
}
}

Expand Down Expand Up @@ -316,7 +346,7 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia
}
}

if (!sslAuthenticationOptions.IsServer)
if (sslAuthenticationOptions.IsClient)
{
// The IdnMapping converts unicode input into the IDNA punycode sequence.
string punyCode = string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) ? string.Empty : s_idnMapping.GetAscii(sslAuthenticationOptions.TargetHost!);
Expand All @@ -327,6 +357,11 @@ internal static SafeSslHandle AllocateSslHandle(SafeFreeSslCredentials credentia
Crypto.ErrClearError();
}

if (cacheSslContext && !string.IsNullOrEmpty(punyCode))
{
sslCtxHandle.TrySetSession(sslHandle, punyCode);
}

// Set client cert callback, this will interrupt the handshake with SecurityStatusPalErrorCode.CredentialsNeeded
// if server actually requests a certificate.
Ssl.SslSetClientCertCallback(sslHandle, 1);
Expand Down Expand Up @@ -624,6 +659,58 @@ private static unsafe int AlpnServerSelectCallback(IntPtr ssl, byte** outp, byte
return Ssl.SSL_TLSEXT_ERR_ALERT_FATAL;
}

[UnmanagedCallersOnly]
// Invoked from OpenSSL when new session is created.
// We attached GCHandle to the SSL so we can find back SafeSslContextHandle holding the cache.
// New session ahs refCount of 1.
// If this function return 0, OpenSSL will drop the refCount and discard the session.
wfurt marked this conversation as resolved.
Show resolved Hide resolved
// If we return 1, the ownership is transfered to us and we will need to call SessionFree().
private static unsafe int NewSessionCallback(IntPtr ssl, IntPtr session)
{
IntPtr ptr = Ssl.SslGetData(ssl);
Debug.Assert(ptr != IntPtr.Zero);
GCHandle gch = (GCHandle)ptr;
wfurt marked this conversation as resolved.
Show resolved Hide resolved

SafeSslContextHandle? ctxHandle = gch.Target as SafeSslContextHandle;
Debug.Assert(ctxHandle != null);

if (ctxHandle != null && ctxHandle.TryAddSession(Ssl.SslGetServerName(ssl), session))
{
// offered session was stored in our cache.
return 1;
}

// OpenSSL will destroy session.
return 0;
Comment on lines +725 to +729
Copy link
Member

Choose a reason for hiding this comment

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

Are these interesting from a logging perspective (if so, that can easily be in a future change)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about logging but there are conditions that make it normal. If we hit this, there should be no functional change as we simply won't do the caching & resume.

}

[UnmanagedCallersOnly]
private static unsafe void RemoveSessionCallback(IntPtr ctx, IntPtr session)
{
Debug.Assert(ctx != IntPtr.Zero && session != IntPtr.Zero);

IntPtr ptr = Ssl.SslCtxGetData(ctx);
Debug.Assert(ptr != IntPtr.Zero);
GCHandle gch = (GCHandle)ptr;
wfurt marked this conversation as resolved.
Show resolved Hide resolved
if (!gch.IsAllocated)
{
return;
}

SafeSslContextHandle? ctxHandle = gch.Target as SafeSslContextHandle;
Debug.Assert(ctxHandle != null);
if (ctxHandle == null)
{
return;
}

string? name = Marshal.PtrToStringAnsi(Ssl.SessionGetHostname(session));
if (!string.IsNullOrEmpty(name))
{
ctxHandle.Remove(name, session);
}
}

private static int BioRead(SafeBioHandle bio, byte[] buffer, int count)
{
Debug.Assert(buffer != null);
Expand Down
18 changes: 18 additions & 0 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ internal static partial class Ssl
[return: MarshalAs(UnmanagedType.Bool)]
internal static partial bool SslSetTlsExtHostName(SafeSslHandle ssl, string host);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetServerName")]
internal static unsafe partial IntPtr SslGetServerName(IntPtr ssl);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSetSession")]
internal static unsafe partial int SslSetSession(SafeSslHandle ssl, IntPtr session);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGet0AlpnSelected")]
internal static partial void SslGetAlpnSelected(SafeSslHandle ssl, out IntPtr protocol, out int len);

Expand Down Expand Up @@ -163,6 +169,18 @@ internal static partial class Ssl
[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_Tls13Supported")]
private static partial int Tls13SupportedImpl();

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionGetHostname")]
internal static partial IntPtr SessionGetHostname(IntPtr session);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionFree")]
internal static partial void SessionFree(IntPtr session);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionSetHostname", CharSet = CharSet.Ansi)]
internal static partial int SessionSetHostname(IntPtr session, string name);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionSetHostname")]
internal static partial int SessionSetHostname(IntPtr session, IntPtr name);

internal static class Capabilities
{
// needs separate type (separate static cctor) to be sure OpenSSL is initialized.
Expand Down
116 changes: 115 additions & 1 deletion src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Net.Security;
using System.Runtime.InteropServices;
using System.Security.Cryptography.X509Certificates;
Expand All @@ -19,11 +21,20 @@ internal static partial class Ssl
[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxDestroy")]
internal static partial void SslCtxDestroy(IntPtr ctx);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxGetData")]
internal static partial IntPtr SslCtxGetData(IntPtr ctx);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetData")]
internal static partial int SslCtxSetData(SafeSslContextHandle ctx, IntPtr data);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetData")]
internal static partial int SslCtxSetData(IntPtr ctx, IntPtr data);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetAlpnSelectCb")]
internal static unsafe partial void SslCtxSetAlpnSelectCb(SafeSslContextHandle ctx, delegate* unmanaged<IntPtr, byte**, byte*, byte*, uint, IntPtr, int> callback, IntPtr arg);

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetCaching")]
internal static unsafe partial void SslCtxSetCaching(SafeSslContextHandle ctx, int mode);
internal static unsafe partial void SslCtxSetCaching(SafeSslContextHandle ctx, int mode, delegate* unmanaged<IntPtr, IntPtr, int> neewSessionCallback, delegate* unmanaged<IntPtr, IntPtr, void> removeSessionCallback);

internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, X509Certificate2[] chain)
{
Expand All @@ -50,6 +61,9 @@ namespace Microsoft.Win32.SafeHandles
{
internal sealed class SafeSslContextHandle : SafeHandle
{
private ConcurrentDictionary<string, IntPtr>? _sslSessions;
wfurt marked this conversation as resolved.
Show resolved Hide resolved
private GCHandle _gch;

public SafeSslContextHandle()
: base(IntPtr.Zero, true)
{
Expand All @@ -69,7 +83,107 @@ protected override bool ReleaseHandle()
{
Interop.Ssl.SslCtxDestroy(handle);
SetHandle(IntPtr.Zero);
if (_gch.IsAllocated)
{
//Interop.Ssl.SslCtxSetData(this, (IntPtr)_gch);
wfurt marked this conversation as resolved.
Show resolved Hide resolved
_gch.Free();
}

if (_sslSessions != null)
{
lock (_sslSessions)
{
foreach (string name in _sslSessions.Keys)
{
_sslSessions.Remove(name, out IntPtr session);
Interop.Ssl.SessionFree(session);
}
wfurt marked this conversation as resolved.
Show resolved Hide resolved
}
}

return true;
}

public void EnableSessionCache()
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
_sslSessions = new ConcurrentDictionary<string, IntPtr>();
_gch = GCHandle.Alloc(this);
wfurt marked this conversation as resolved.
Show resolved Hide resolved
// This is needed so we can find the handle from session remove callback.
Interop.Ssl.SslCtxSetData(this, (IntPtr)_gch);
}

public bool TryAddSession(IntPtr serverName, IntPtr session)
{
Debug.Assert(_sslSessions != null && session != IntPtr.Zero);

if (_sslSessions == null || serverName == IntPtr.Zero)
{
return false;
}

string? name = Marshal.PtrToStringAnsi(serverName);
if (!string.IsNullOrEmpty(name))
{
Interop.Ssl.SessionSetHostname(session, serverName);
Copy link
Member

Choose a reason for hiding this comment

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

It feels really weird to me that SetHostname would be inside TryAddSession.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need something to find the session in removal. Associating name with it allows me to get the string and then do lookup. It would be great if we can come up with something that allows to lookup by both IntPtr and Name.

Copy link
Member

Choose a reason for hiding this comment

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

Sure... it just feels like calling SetHostname'd be the responsibility of the caller. Doesn't it have to be done in the case where TryAddSession returns false?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is ony only for the lookup - there is no functional difference. And I tried to hide all this inside the handle.
If we can lookup/remove the entry just from the IntPtr session, we would not need to do that. But I'm not sure if there is good way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I put in comment and made changes to make it clear. I also moved complementing removal so both parts are done inside the SafeHandle.


lock (_sslSessions)
Copy link
Member

Choose a reason for hiding this comment

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

The locking around _sslSessions makes sense, since you're manipulating state depending on how the dictionary performed.

But, since you're already locking it, it feels like you want a non-Concurrent dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed. I was also thinking about grabbing extra reference on the session. That would allow me to use ConcurrentDictionary without locking as the session would never be released in the middle.
Do you have preference/recommendation @bartonjs ?

Copy link
Member

Choose a reason for hiding this comment

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

grabbing extra reference on the session

Something like

Interop.Ssl.SslSessionUpRef(session);

if (!_sslSessions.TryAdd(...))
{
    // Undo the upref since it's not in the dictionary
    Interop.Ssl.SslSessionFree(session);
}

? (Upref inside has a race condition with the cleanup in ReleaseHandle)

That would get a little weird since in the cleanup you'd need to call free twice, I think?

The fact that we wrote ConcurrentDictionary suggests that it gives better perf (on average) than manual locking... but if the code to interact with it is doing memory/lifetime management and it becomes unreadable with the gymnastics... then locking is better for maintainability. (If it's clean code and more performant, than by all means use upref+ConcurrentDictionary)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. and then call free twice. I'm inclined to good with the lock and better maintainability as the perf does not depend on this. This happens one in while - not even for each SSL session. I started with ConcurrentDictionary but you are right - we don't need it at this point.

{
IntPtr oldSession = _sslSessions.GetOrAdd(name, session);
if (oldSession != session)
{
_sslSessions.Remove(name, out oldSession);
Interop.Ssl.SessionFree(oldSession);
oldSession = _sslSessions.GetOrAdd(name, session);
Debug.Assert(oldSession == session);
}
}

return true;
}

return false;
}

public void Remove(string name, IntPtr session)
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
if (_sslSessions != null)
{
lock (_sslSessions)
{
if (!_sslSessions.Remove(name, out IntPtr oldSession))
wfurt marked this conversation as resolved.
Show resolved Hide resolved
{
Interop.Ssl.SessionFree(oldSession);
}
}
}
}

public bool TrySetSession(SafeSslHandle sslHandle, string name)
{
Debug.Assert(_sslSessions != null);

if (_sslSessions == null || string.IsNullOrEmpty(name))
{
return false;
}

// even if we don't have matching session, we can get new one and we need
// way how to link SSL back to `this`.
Interop.Ssl.SslSetData(sslHandle, (IntPtr)_gch);
wfurt marked this conversation as resolved.
Show resolved Hide resolved

lock (_sslSessions)
{
if (_sslSessions.TryGetValue(name, out IntPtr session))
{
// This will increase reference count on the session as needed.
// We need to hold lock here to prevent session being deleted before the call is done.
Interop.Ssl.SslSetSession(sslHandle, session);

return true;
}
}

return false;
}
}
}
Loading