Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

use ROSpan in SslStreamCertificateContext #83485

Merged
merged 2 commits into from
Mar 29, 2023
Merged

use ROSpan in SslStreamCertificateContext #83485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a59a4b7
use ROSpan in SslStreamCertificateContext
wfurt Mar 15, 2023
68ed9be
feedback from review
wfurt Mar 18, 2023
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
2 changes: 1 addition & 1 deletion 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 @@ -262,7 +262,7 @@ internal static unsafe void SslStapleOcsp(SafeSslHandle ssl, ReadOnlySpan<byte>
}
}

internal static bool AddExtraChainCertificates(SafeSslHandle ssl, X509Certificate2[] chain)
internal static bool AddExtraChainCertificates(SafeSslHandle ssl, ReadOnlySpan<X509Certificate2> chain)
{
// send pre-computed list of intermediates.
for (int i = 0; i < chain.Length; i++)
Expand Down
2 changes: 1 addition & 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 @@ -35,7 +35,7 @@ internal static partial class Ssl
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetCaching")]
internal static unsafe partial int SslCtxSetCaching(SafeSslContextHandle ctx, int mode, int cacheSize, int contextIdLength, Span<byte> contextId, delegate* unmanaged<IntPtr, IntPtr, int> neewSessionCallback, delegate* unmanaged<IntPtr, IntPtr, void> removeSessionCallback);

internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, X509Certificate2[] chain)
internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, ReadOnlySpan<X509Certificate2> chain)
{
// send pre-computed list of intermediates.
for (int i = 0; i < chain.Length; i++)
Expand Down
13 changes: 8 additions & 5 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public static MsQuicSafeHandle Create(QuicClientConnectionOptions options)
}
}

return Create(options, flags, certificate, intermediates: null, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy);
return Create(options, flags, certificate, ReadOnlySpan<X509Certificate2>.Empty, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy);
}

public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, string? targetHost)
Expand All @@ -85,7 +85,7 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin
}

X509Certificate? certificate = null;
X509Certificate[]? intermediates = null;
ReadOnlySpan<X509Certificate2> intermediates = default;
if (authenticationOptions.ServerCertificateContext is not null)
{
certificate = authenticationOptions.ServerCertificateContext.Certificate;
Expand All @@ -101,7 +101,7 @@ public static MsQuicSafeHandle Create(QuicServerConnectionOptions options, strin
return Create(options, flags, certificate, intermediates, authenticationOptions.ApplicationProtocols, authenticationOptions.CipherSuitesPolicy, authenticationOptions.EncryptionPolicy);
}

private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, X509Certificate[]? intermediates, List<SslApplicationProtocol>? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy)
private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUIC_CREDENTIAL_FLAGS flags, X509Certificate? certificate, ReadOnlySpan<X509Certificate2> intermediates, List<SslApplicationProtocol>? alpnProtocols, CipherSuitesPolicy? cipherSuitesPolicy, EncryptionPolicy encryptionPolicy)
{
// Validate options and SSL parameters.
if (alpnProtocols is null || alpnProtocols.Count <= 0)
Expand Down Expand Up @@ -171,11 +171,14 @@ private static unsafe MsQuicSafeHandle Create(QuicConnectionOptions options, QUI

byte[] certificateData;

if (intermediates?.Length > 0)
if (intermediates.Length > 0)
{
X509Certificate2Collection collection = new X509Certificate2Collection();
collection.Add(certificate);
collection.AddRange(intermediates);
foreach (X509Certificate2 intermediate in intermediates)
{
collection.Add(intermediate);
}
Copy link
Member

Choose a reason for hiding this comment

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

This could be slower if we expected a lot of certs. I assume we don't?

Copy link
Member Author

Choose a reason for hiding this comment

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

typically this would be 1-3 items. I was looking at #1530 but it did not seems like I can use the AddRage for generic collection. We actually allocate it just to make the export easy e.g. produce the Pkcs12 blob. If needed we may suck in some crypto primitives directly.

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine as you have it.

Copy link
Member

Choose a reason for hiding this comment

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

Have we considered adding AddRange(RoS) overload into X509Certificate2Collection if we're introducing RoS type on SslStreamCertificateContext?

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'm not sure that would make measurable improvement. There was some discussion on the issue linked above.
If anything I would push to avoid allocation of the collection.

certificateData = collection.Export(X509ContentType.Pkcs12)!;
}
else
Expand Down
29 changes: 15 additions & 14 deletions src/libraries/System.Net.Security/src/CompatibilitySuppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
Expand All @@ -8,13 +9,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/android/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/android/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -26,13 +27,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/freebsd/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/freebsd/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -44,13 +45,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/ios/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/ios/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -62,13 +63,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/linux/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/linux/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -80,13 +81,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/osx/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/osx/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -98,13 +99,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/tvos/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/tvos/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand All @@ -116,13 +117,13 @@
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>F:System.Net.Security.SslStreamCertificateContext.IntermediateCertificates</Target>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/win/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
<Suppression>
<DiagnosticId>CP0002</DiagnosticId>
<Target>M:System.Net.Security.SslClientHelloInfo.#ctor(System.String,System.Security.Authentication.SslProtocols)</Target>
<Target>M:System.Net.Security.SslStreamCertificateContext.get_IntermediateCertificates</Target>
<Left>ref/net8.0/System.Net.Security.dll</Left>
<Right>runtimes/win/lib/net8.0/System.Net.Security.dll</Right>
</Suppression>
Expand Down
6 changes: 1 addition & 5 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,11 +727,7 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
// effectively disable caching as it would lead to creating new credentials for each connection. We attempt to recover by creating
// a temporary certificate context (which builds a new chain with hopefully more recent chain).
//
certificateContext = SslStreamCertificateContext.Create(
certificateContext.Certificate,
new X509Certificate2Collection(certificateContext.IntermediateCertificates),
trust: certificateContext.Trust);

certificateContext = certificateContext.Duplicate();
cred._expiry = GetExpiryTimestamp(certificateContext);
}

Expand Down
2 changes: 1 addition & 1 deletion ...raries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ public partial class SslStreamCertificateContext

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates, SslCertificateTrust? trust)
{
_intermediateCertificates = intermediates;
Certificate = target;
IntermediateCertificates = intermediates;
Trust = trust;
}

Expand Down
2 changes: 1 addition & 1 deletion ...ibraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ public partial class SslStreamCertificateContext

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates, SslCertificateTrust? trust)
{
_intermediateCertificates = intermediates;
Certificate = target;
IntermediateCertificates = intermediates;
Trust = trust;
SslContexts = new ConcurrentDictionary<SslProtocols, SafeSslContextHandle>();

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ public partial class SslStreamCertificateContext

private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[] intermediates, SslCertificateTrust? trust)
{
_intermediateCertificates = intermediates;
Certificate = target;
IntermediateCertificates = intermediates;
Trust = trust;
}

Expand Down
2 changes: 1 addition & 1 deletion ...raries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ private SslStreamCertificateContext(X509Certificate2 target, X509Certificate2[]
}
}

_intermediateCertificates = intermediates;
Certificate = target;
IntermediateCertificates = intermediates;
Trust = trust;
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ namespace System.Net.Security
public partial class SslStreamCertificateContext
{
public readonly X509Certificate2 Certificate;
public readonly X509Certificate2[] IntermediateCertificates;
public ReadOnlySpan<X509Certificate2> IntermediateCertificates => _intermediateCertificates;

private readonly X509Certificate2[] _intermediateCertificates;
internal readonly SslCertificateTrust? Trust;

[EditorBrowsable(EditorBrowsableState.Never)]
Expand Down Expand Up @@ -131,7 +133,7 @@ internal static SslStreamCertificateContext Create(

internal SslStreamCertificateContext Duplicate()
{
return new SslStreamCertificateContext(new X509Certificate2(Certificate), IntermediateCertificates, Trust);
return new SslStreamCertificateContext(new X509Certificate2(Certificate), _intermediateCertificates, Trust);
}
}
}