[API Proposal]: Make some System.Net.Security functions/properties public for System.Net.Quic

[ManickaP]

Background and motivation

1. SslStreamCertificateContext
   In System.Net.Quic, we're handling certificates from SslServerAuthenticationOptions and need to pass them to msquic. Since SslServerAuthenticationOptions allows to specify certificate with its intermediates via SslStreamCertificateContext, we need to be able to extract both properties from the class in order to convert them and pass them to msquic.

2. SslClientHelloInfo
   We'll also be adding our version of ServerOptionsSelectionCallback:
   

   runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

   Line 38 in 4f60211

   public delegate ValueTask<SslServerAuthenticationOptions> ServerOptionsSelectionCallback(SslStream stream, SslClientHelloInfo clientHelloInfo, object? state, CancellationToken cancellationToken);

   
   And we'll need to be able to construct SslClientHelloInfo struct. Otherwise, we'll be left with reflection here as well.

3. Current SslStreamCertificateContext takes list of additional certificates. But when Create method is called it is not clear if X509Chain build succeeds and how the actual list looks.
   Exposing the IntermediateCertificates would make any investigations easier

4. SslStreamCertificateContext is not iDisposable (unfortunately).
   Exposing the inner certificates possibly allows some cleanup if needed. (also creates risk of mishandling)

API Proposal

1. SslStreamCertificateContext

// Public ctors, but getters are internal.
public class SslStreamCertificateContext
{
-   internal readonly X509Certificate2 Certificate;
+   public readonly X509Certificate2 TargetCertificate { get; }
+   public ReadOnlyMemory<X509Certificate2> IntermediateCertificates { get; }
    // public ctors
}

Create method calls the certificate target and that seems too ambiguous. The proposal is to call it TaregtCertificate to make it somewhat closer or we can use the current internal Certificate (that is pretty generic as well)

We also talk about ReadOnlySpan<X509Certificate2> for the IntermediateCertificates but the consensus was to use Memory as more flexible. Current implementation use array under the cover.

2. SslClientHelloInfo

// It has public getters, but cannot be constructed with values outside of System.Net.Security.
public readonly struct SslClientHelloInfo
{
    public readonly string ServerName { get; }
    public readonly SslProtocols SslProtocols { get; }

-   internal SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
+   public SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
}

API Usage

1. SslStreamCertificateContext

if (sslOptions.ServerCertificateContext is not null) 
{
    certificate = sslOptions.ServerCertificateContext.ContextCertificate;
    intermediaries = sslOptions.ServerCertificateContext.ContextChain;
}

2. SslClientHelloInfo

SslClientHelloInfo sslClientHelloInfo = new SslClientHelloInfo(targetHost, SslProtocols.Tls13);

Alternative Designs

1. SslStreamCertificateContext
   Move the whole logic around certificates to System.Net.Security and get out of it only ASN1 blob (which we need for msquic).

2. SslClientHelloInfo
   Not much options here, possibly static create method.

For both, InternalsVisibleTo. I'm aware it's "forbidden" in runtime, but I don't know why.
For both, merging System.Net.Security with System.Net.Quic. This was voted against in our team discussion, unless we want to merge whole lot more and create omnipotent System.Net.dll.

3. Use reflection
   That was done in early 7.0. Not great for obvious reason.

4. Use semi-public methods (current state)
   Needed properties and methods are public but not in ref assembly.
   Noted in SslStreamCertificateContext has public fields in its implementation that aren't in the contract #72226 .

Risks

Not much. We'd be only exposing readonly getters and struct ctor.

[ghost]

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

1. SslStreamCertificateContext
   In System.Net.Quic, we're handling certificates from SslServerAuthenticationOptions and need to pass them to msquic. Since SslServerAuthenticationOptions allows to specify certificate with its intermediates via SslStreamCertificateContext, we need to be able to extract both properties from the class in order to convert them and pass them to msquic.
   For the time being, we're using reflection:
   

   runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

   Lines 18 to 19 in 6ffb7f0

   	private static readonly FieldInfo _contextCertificate = typeof(SslStreamCertificateContext).GetField("Certificate", BindingFlags.NonPublic | BindingFlags.Instance)!;
   	private static readonly FieldInfo _contextChain = typeof(SslStreamCertificateContext).GetField("IntermediateCertificates", BindingFlags.NonPublic | BindingFlags.Instance)!;

   
   But that is fragile and hacky solution.

2. SslClientHelloInfo
   We'll also be adding our version of ServerOptionsSelectionCallback:
   

   runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

   Line 38 in 4f60211

   public delegate ValueTask<SslServerAuthenticationOptions> ServerOptionsSelectionCallback(SslStream stream, SslClientHelloInfo clientHelloInfo, object? state, CancellationToken cancellationToken);

   
   And we'll need to be able to construct SslClientHelloInfo struct. Otherwise, we'll be left with reflection here as well.

cc: @wfurt

API Proposal

1. SslStreamCertificateContext

// Public ctors, but getters are internal.
public class SslStreamCertificateContext
{
-   internal readonly X509Certificate2 Certificate;
+   public readonly X509Certificate2 Certificate;
-   internal readonly X509Certificate2[] IntermediateCertificates;
+   internal readonly X509Certificate2[] IntermediateCertificates;
    // public ctors
}

2. SslClientHelloInfo

// It has public getters, but cannot be constructed with values outside of System.Net.Security.
public readonly struct SslClientHelloInfo
{
    public readonly string ServerName { get; }
    public readonly SslProtocols { get; }

-   internal SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
+   public SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
}

API Usage

1. SslStreamCertificateContext

if (sslOptions.ServerCertificateContext is not null) 
{
    certificate = sslOptions.ServerCertificateContext.ContextCertificate;
    intermediaries = sslOptions.ServerCertificateContext.ContextChain;
}

2. SslClientHelloInfo

SslClientHelloInfo sslClientHelloInfo = new SslClientHelloInfo(targetHost, SslProtocols.Tls13);

Alternative Designs

1. SslStreamCertificateContext
   Move the whole logic around certificates to System.Net.Security and get out of it only ASN1 blob (which we need for msquic).

2. SslClientHelloInfo
   Not much options here, possibly static create method.

For both, InternalsVisibleTo. I'm aware it's "forbidden" in runtime, but I don't know why.
For both, merging System.Net.Security with System.Net.Quic. This was voted against in our team discussion, unless we want to merge whole more and create omnipotent System.Net.dll.

Risks

Not much. We'd be only exposing readonly getters and struct ctor.

Author:	ManickaP
Assignees:	-
Labels:	api-suggestion, area-System.Net.Security, untriaged
Milestone:	-

[rzikm]

For the SslStreamCertificateContext.IntermediateCertificates field/property, I think it would be better to change the type to something immutable (ReadOnlyMemory/ReadOnlyList?) so that we don't need to worry about it changing under our hands.

[wfurt]

I would suggest

public X509Certificate2 TargetCertificate { get; }
public ReadOnlySpan<X509Certificate2> IntermediateCertificates { get; }

[ManickaP]

Yeah, I only took the fields as they are now and made them public. Getters are fine choice here as well, I'll update the proposal to reflect that.

[rzikm]

Do we still want to keep this since #68189 was merged?

[ManickaP]

Closing in favor of #68189

[ManickaP]

I know this is closed, but food for thought: If we enable custom QUIC implementations, i.e. make Quic* classes abstract, the solution with ref in #68189 won't be enough. But obviously we can revisit this once/if that happens.

cc: @wfurt

[bartonjs]

Video

We weren't feeling good about the ReadOnlyMemory property, and want to have a discussion about it.

// Public ctors, but getters are internal.
public class SslStreamCertificateContext
{
-   internal readonly X509Certificate2 Certificate;
+   public X509Certificate2 TargetCertificate { get; }
+   public ReadOnlyMemory<X509Certificate2> IntermediateCertificates { get; }
    // public ctors
}

// It has public getters, but cannot be constructed with values outside of System.Net.Security.
public readonly struct SslClientHelloInfo
{
    public readonly string ServerName { get; }
    public readonly SslProtocols SslProtocols { get; }

-   internal SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
+   public SslClientHelloInfo(string serverName, SslProtocols sslProtocols);
}

[terrajobst]

• We'd prefer ReadOnlyCollection<T>

namespace System.Net.Security;

public partial class SslStreamCertificateContext
{
    public X509Certificate2 TargetCertificate { get; }
    public ReadOnlyCollection<X509Certificate2> IntermediateCertificates { get; }
}