-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add ALPN support for SslStream. #24389
Changes from all commits
14f807c
c90f684
c2df948
7e48bcc
0e77f0a
1ba2e72
23c34d9
87f9f50
2997476
0ece1ba
29a357f
c1eab3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ internal static partial class Interop | |
{ | ||
internal static partial class OpenSsl | ||
{ | ||
private static Ssl.SslCtxSetVerifyCallback s_verifyClientCertificate = VerifyClientCertificate; | ||
private static readonly Ssl.SslCtxSetVerifyCallback s_verifyClientCertificate = VerifyClientCertificate; | ||
private static readonly Ssl.SslCtxSetAlpnCallback s_alpnServerCallback = AlpnServerSelectCallback; | ||
|
||
#region internal methods | ||
|
||
|
@@ -47,7 +48,7 @@ internal static SafeChannelBindingHandle QueryChannelBinding(SafeSslHandle conte | |
return bindingHandle; | ||
} | ||
|
||
internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX509Handle certHandle, SafeEvpPKeyHandle certKeyHandle, EncryptionPolicy policy, bool isServer, bool remoteCertRequired) | ||
internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX509Handle certHandle, SafeEvpPKeyHandle certKeyHandle, EncryptionPolicy policy, SslAuthenticationOptions sslAuthenticationOptions) | ||
{ | ||
SafeSslHandle context = null; | ||
|
||
|
@@ -88,17 +89,32 @@ internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX50 | |
SetSslCertificate(innerContext, certHandle, certKeyHandle); | ||
} | ||
|
||
if (remoteCertRequired) | ||
if (sslAuthenticationOptions.IsServer && sslAuthenticationOptions.RemoteCertRequired) | ||
{ | ||
Debug.Assert(isServer, "isServer flag should be true"); | ||
Ssl.SslCtxSetVerify(innerContext, | ||
s_verifyClientCertificate); | ||
Ssl.SslCtxSetVerify(innerContext, s_verifyClientCertificate); | ||
|
||
//update the client CA list | ||
UpdateCAListFromRootStore(innerContext); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method call does not match the behavior on Windows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ayende This code is not related to the work done in this PR. Please file separate issue if there's a bug. |
||
} | ||
|
||
context = SafeSslHandle.Create(innerContext, isServer); | ||
if (sslAuthenticationOptions.ApplicationProtocols != null) | ||
{ | ||
if (sslAuthenticationOptions.IsServer) | ||
{ | ||
byte[] protos = Interop.Ssl.ConvertAlpnProtocolListToByteArray(sslAuthenticationOptions.ApplicationProtocols); | ||
sslAuthenticationOptions.AlpnProtocolsHandle = GCHandle.Alloc(protos); | ||
Interop.Ssl.SslCtxSetAlpnSelectCb(innerContext, s_alpnServerCallback, GCHandle.ToIntPtr(sslAuthenticationOptions.AlpnProtocolsHandle)); | ||
} | ||
else | ||
{ | ||
if (Interop.Ssl.SslCtxSetAlpnProtos(innerContext, sslAuthenticationOptions.ApplicationProtocols) != 0) | ||
{ | ||
throw CreateSslException(SR.net_alpn_config_failed); | ||
} | ||
} | ||
} | ||
|
||
context = SafeSslHandle.Create(innerContext, sslAuthenticationOptions.IsServer); | ||
Debug.Assert(context != null, "Expected non-null return value from SafeSslHandle.Create"); | ||
if (context.IsInvalid) | ||
{ | ||
|
@@ -314,6 +330,18 @@ private static int VerifyClientCertificate(int preverify_ok, IntPtr x509_ctx_ptr | |
return OpenSslSuccess; | ||
} | ||
|
||
private static unsafe int AlpnServerSelectCallback(IntPtr ssl, out IntPtr outp, out byte outlen, IntPtr inp, uint inlen, IntPtr arg) | ||
{ | ||
GCHandle protocols = GCHandle.FromIntPtr(arg); | ||
byte[] server = (byte[])protocols.Target; | ||
|
||
fixed (byte* sp = server) | ||
{ | ||
return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)sp, (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ? | ||
Interop.Ssl.SSL_TLSEXT_ERR_OK : Interop.Ssl.SSL_TLSEXT_ERR_NOACK; | ||
} | ||
} | ||
|
||
private static void UpdateCAListFromRootStore(SafeSslContextHandle context) | ||
{ | ||
using (SafeX509NameStackHandle nameStack = Crypto.NewX509NameStack()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Runtime.InteropServices; | ||
|
||
internal static partial class Interop | ||
{ | ||
internal enum ApplicationProtocolNegotiationStatus | ||
{ | ||
None = 0, | ||
Success, | ||
SelectedClientOnly | ||
} | ||
|
||
internal enum ApplicationProtocolNegotiationExt | ||
{ | ||
None = 0, | ||
NPN, | ||
ALPN | ||
} | ||
|
||
[StructLayout(LayoutKind.Sequential)] | ||
internal class SecPkgContext_ApplicationProtocol | ||
{ | ||
private const int MaxProtocolIdSize = 0xFF; | ||
|
||
public ApplicationProtocolNegotiationStatus ProtoNegoStatus; | ||
public ApplicationProtocolNegotiationExt ProtoNegoExt; | ||
public byte ProtocolIdSize; | ||
[MarshalAs(UnmanagedType.ByValArray, SizeConst = MaxProtocolIdSize)] | ||
public byte[] ProtocolId; | ||
public byte[] Protocol | ||
{ | ||
get | ||
{ | ||
return new Span<byte>(ProtocolId, 0, ProtocolIdSize).ToArray(); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Net.Security; | ||
using System.Runtime.InteropServices; | ||
|
||
internal static partial class Interop | ||
{ | ||
[StructLayout(LayoutKind.Sequential, Pack = 1)] | ||
internal struct Sec_Application_Protocols | ||
{ | ||
private static readonly int ProtocolListOffset = Marshal.SizeOf<Sec_Application_Protocols>(); | ||
private static readonly int ProtocolListConstSize = ProtocolListOffset - (int)Marshal.OffsetOf<Sec_Application_Protocols>(nameof(ProtocolExtenstionType)); | ||
public uint ProtocolListsSize; | ||
public ApplicationProtocolNegotiationExt ProtocolExtenstionType; | ||
public short ProtocolListSize; | ||
|
||
public static unsafe byte[] ToByteArray(List<SslApplicationProtocol> applicationProtocols) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephentoub, should we also have a method that takes in a span and puts the bytes into it, i.e. TryCopyTo(Span buffer)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is internal. Is the equivalent exposed publicly somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's exposed, but: a) maybe we should expose it, b) maybe the internal code that uses it would be written with less copies/allocations if we had such internal API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For b), at that point it's an implementation detail and we should do whatever's most efficient, assuming it matters. For a), what would it be used for? |
||
{ | ||
long protocolListSize = 0; | ||
for (int i = 0; i < applicationProtocols.Count; i++) | ||
{ | ||
if (applicationProtocols[i].Protocol.Length == 0 || applicationProtocols[i].Protocol.Length > byte.MaxValue) | ||
{ | ||
throw new ArgumentException(SR.net_ssl_app_protocols_invalid, nameof(applicationProtocols)); | ||
} | ||
|
||
protocolListSize += applicationProtocols[i].Protocol.Length + 1; | ||
|
||
if (protocolListSize > short.MaxValue) | ||
{ | ||
throw new ArgumentException(SR.net_ssl_app_protocols_invalid, nameof(applicationProtocols)); | ||
} | ||
} | ||
|
||
Sec_Application_Protocols protocols = new Sec_Application_Protocols(); | ||
protocols.ProtocolListsSize = (uint)(ProtocolListConstSize + protocolListSize); | ||
protocols.ProtocolExtenstionType = ApplicationProtocolNegotiationExt.ALPN; | ||
protocols.ProtocolListSize = (short)protocolListSize; | ||
|
||
Span<byte> pBuffer = new byte[protocolListSize]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: pBuffer kind of implies that the variable is a pointer. it might be better to call it protocolsBuffer. |
||
int index = 0; | ||
for (int i = 0; i < applicationProtocols.Count; i++) | ||
{ | ||
pBuffer[index++] = (byte)applicationProtocols[i].Protocol.Length; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is done, in the code just before in the same function, so a debug assert is not required here. |
||
applicationProtocols[i].Protocol.Span.CopyTo(pBuffer.Slice(index)); | ||
index += applicationProtocols[i].Protocol.Length; | ||
} | ||
|
||
byte[] buffer = new byte[ProtocolListOffset + protocolListSize]; | ||
fixed (byte* bufferPtr = buffer) | ||
{ | ||
Marshal.StructureToPtr(protocols, new IntPtr(bufferPtr), false); | ||
byte* pList = bufferPtr + ProtocolListOffset; | ||
pBuffer.CopyTo(new Span<byte>(pList, index)); | ||
} | ||
|
||
return buffer; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lot of allocation happening in these conversion routines: enumerators, byte arrays, etc. Not to block this PR, but it'd be good subsequently to see what kind of impact that has and whether there are ways to reduce it. |
||
} |
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.
What does it mean if RemoteCertRequired is true and IsServer is false? The assert here took care of identifying that as an invalid state.
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.
This state will not be hit with the current implementation. Previously, these internal APIs were passing the RemoteCertRequired=false for client and RemoteCertRequired=true for server. This is in direct contradiction with external meaning of RemoteCertRequired, where for client it is always true, and server is got from RemoteCertRequired parameter on the authenticate methods. Since we don't flip these values internally anymore, it doesn't make sense to have this assert.
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.
Oh, I get it. You've changed the client to be
true
for this instead offalse
. Really the client isN/A
, and that's what the assert was guarding (that it made no sense for a client to change that value during the handshake).This seems fine, now that I understand the change. There's a mild perf gain (ditching the
&&
) for having the client usefalse
, but it's not something worth adding confusion over.