SslStreamCertificateContext has public fields in its implementation that aren't in the contract

[stephentoub]

SslStreamCertificateContext has two public fields in its implementation that aren't in the contract:

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

Lines 11 to 12 in 617de07

	public readonly X509Certificate2 Certificate;
	public readonly X509Certificate2[] IntermediateCertificates;

This was done for System.Net.Quic to be able to use these without actually making public API for them. But there are a variety of issues with this:

1. These kinds of ref vs src tricks are harder to maintain (our tooling for ensuring we don't break our contracts doesn't enforce such suppressed violations of the ref/src contract tooling). The fewer we have, the better.

2. Being public in the implementation means things like dynamic will by default allow access to them, making them accessible anyway but without tooling to guarantee we're not breaking them, e.g.

using System.Net;
using System.Net.Security;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;

dynamic ctx = SslStreamCertificateContext.Create(CreateServerSelfSignedCertificate("localhost"), null);
Console.WriteLine(ctx.IntermediateCertificates.Length);

static X509Certificate2 CreateServerSelfSignedCertificate(string name)
{
    using (RSA root = RSA.Create())
    {
        CertificateRequest req = new CertificateRequest($"CN={name}", root, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);

        req.CertificateExtensions.Add(new X509BasicConstraintsExtension(true, false, 0, true));
        req.CertificateExtensions.Add(new X509SubjectKeyIdentifierExtension(req.PublicKey, false));
        req.CertificateExtensions.Add(new X509KeyUsageExtension(X509KeyUsageFlags.DigitalSignature | X509KeyUsageFlags.KeyEncipherment | X509KeyUsageFlags.DataEncipherment, false));
        req.CertificateExtensions.Add(new X509EnhancedKeyUsageExtension(new OidCollection() { new Oid("1.3.6.1.5.5.7.3.1", null), }, false));

        SubjectAlternativeNameBuilder builder = new SubjectAlternativeNameBuilder();
        builder.AddDnsName(name);
        builder.AddIpAddress(IPAddress.Loopback);
        builder.AddIpAddress(IPAddress.IPv6Loopback);
        req.CertificateExtensions.Add(builder.Build());

        DateTimeOffset start = DateTimeOffset.UtcNow.AddMinutes(-5);
        DateTimeOffset end = start.AddMonths(1);
        X509Certificate2 cert = req.CreateSelfSigned(start, end);
        if (OperatingSystem.IsWindows())
        {
            cert = new X509Certificate2(cert.Export(X509ContentType.Pfx));
        }

        return cert;
    }
}

3. If both SslStream and Quic need access, that's an indicator other protocols will as well.

4. There's currently no way to properly clean up after an SslStreamCertificateContext. It's not disposable and this state isn't exposed, which means everything created (e.g. all the intermediate certs) and stored is left for finalization when one is no longer used. Thankfully this shouldn't be a big deal for most servers, where there should be relatively few instances that are all long-lived.

We should consider designing real public API around these.

[ghost]

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

Issue Details

---

SslStreamCertificateContext has two public fields in its implementation that aren't in the contract:

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

Lines 11 to 12 in 617de07

	public readonly X509Certificate2 Certificate;
	public readonly X509Certificate2[] IntermediateCertificates;

This was done for System.Net.Quic to be able to use these without actually making public API for them. But there are a variety of issues with this:

1. These kinds of ref vs src tricks are harder to maintain (our tooling for ensuring we don't break our contracts doesn't enforce such suppressed violations of the ref/src contract tooling). The fewer we have, the better.

2. Being public in the implementation means things like dynamic will by default allow access to them, making them accessible anyway but without tooling to guarantee we're not breaking them, e.g.

using System.Net;
using System.Net.Security;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;

dynamic ctx = SslStreamCertificateContext.Create(CreateServerSelfSignedCertificate("localhost"), null);
Console.WriteLine(ctx.IntermediateCertificates.Length);

static X509Certificate2 CreateServerSelfSignedCertificate(string name)
{
    using (RSA root = RSA.Create())
    {
        CertificateRequest req = new CertificateRequest($"CN={name}", root, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1);

        req.CertificateExtensions.Add(new X509BasicConstraintsExtension(true, false, 0, true));
        req.CertificateExtensions.Add(new X509SubjectKeyIdentifierExtension(req.PublicKey, false));
        req.CertificateExtensions.Add(new X509KeyUsageExtension(X509KeyUsageFlags.DigitalSignature | X509KeyUsageFlags.KeyEncipherment | X509KeyUsageFlags.DataEncipherment, false));
        req.CertificateExtensions.Add(new X509EnhancedKeyUsageExtension(new OidCollection() { new Oid("1.3.6.1.5.5.7.3.1", null), }, false));

        SubjectAlternativeNameBuilder builder = new SubjectAlternativeNameBuilder();
        builder.AddDnsName(name);
        builder.AddIpAddress(IPAddress.Loopback);
        builder.AddIpAddress(IPAddress.IPv6Loopback);
        req.CertificateExtensions.Add(builder.Build());

        DateTimeOffset start = DateTimeOffset.UtcNow.AddMinutes(-5);
        DateTimeOffset end = start.AddMonths(1);
        X509Certificate2 cert = req.CreateSelfSigned(start, end);
        if (OperatingSystem.IsWindows())
        {
            cert = new X509Certificate2(cert.Export(X509ContentType.Pfx));
        }

        return cert;
    }
}

3. If both SslStream and Quic need access, that's an indicator other protocols will as well.

4. There's currently no way to properly clean up after an SslStreamCertificateContext. It's not disposable and this state isn't exposed, which means everything created (e.g. all the intermediate certs) and stored is left for finalization when one is no longer used. Thankfully this shouldn't be a big deal for most servers, where there should be relatively few instances that are all long-lived.

We should consider designing real public API around these.

Author:	stephentoub
Assignees:	-
Labels:	area-System.Net.Security
Milestone:	8.0.0

[ManickaP]

#67552 could be just reopened for the API suggestion.

[wfurt]

fixed by #85402