More granular X.509 certificate loader

[bartonjs]

Currently, loading a certificate from memory or a file is performed by the X509Certificate2 constructors or the X509Certificate2Collection.Import methods. These existing routines support many different formats (for single certificates: X509Certificate, PKCS#7 SignedCms, Windows Serialized Certificate, Authenticode-signed assets, and PKCS#12/PFX; for collections: X509Certificate, PKCS#7 SignedCms (interpreted differently than the single certificate case), Windows Serialized Store, and PKCS#12/PFX). Since many of those formats themselves support multiple encodings (e.g. X509Certificate-PEM and X509Certificate-DER), these members are very complicated.

While sometimes convenient to a caller, the design has proven lacking in multiple ways:

• When a protocol or file format indicates the presence of a certificate, new X509Certificate2(data) will unexpectedly allow several other file formats, making the most obvious code load data that other systems correctly reject as invalid.
• Of all the file formats these member support, only PKCS#12/PFX requires more options. These options are ignored when the input data/file is not a PKCS#12/PFX, leading to user confusion.
• PKCS#12/PFX requires more options... but the overloads that do not accept those options will provide defaults. Since PKCS#12/PFX is the only file format supported by these members that can also load private keys into memory, it isn't possible to understand the full security implications of new X509Certificate2(bytes).
• PKCS#12/PFX is a very complicated format which can be very expensive to load. Many .NET users have expressed desire for some control knobs to limit the total amount of work attempted.
• Authenticode-signed assets, Windows Serialized Certificates, and Windows Serialized Stores are only supported on Windows, but there's no way to mark that with [SupportedOS]

This proposal puts loader methods on a new type, both to avoid "do I want the ctor or the static?" but also so that this type can be made available to .NET Standard 2.0/.NET Framework.

The expected packaging is inbox for .NET 9+, and Microsoft.Bcl.Cryptography for .NET Standard 2.0/.NET Framework/.NET 8-.

namespace System.Security.Cryptography.X509Certificates
{
    public static partial class X509CertificateLoader
    {
        // A single X509Certificate value, PEM or DER
        // No collection variant needed.
        public static partial X509Certificate2 LoadCertificate(byte[] data);
        public static partial X509Certificate2 LoadCertificate(ReadOnlySpan<byte> data);
        public static partial X509Certificate2 LoadCertificate(string path);

        // Load "the best" certificate from a PFX: first-cert-with-privkey ?? first-cert ?? throw.
        // equivalent to the certificate from `new X509Certificate2(data, password, keyStorageFlags)`
        public static X509Certificate2 LoadPkcs12(
            byte[] data,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadPkcs12(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2 LoadPkcs12(
            string path,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadPkcs12(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);

        // Load a PFX as a collection.
        // null loaderLimits means Pkcs12LoaderLimits.Default
        public static X509Certificate2Collection LoadPkcs12Collection(
            byte[] data,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12Collection(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2Collection LoadPkcs12Collection(
            string path,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12Collection(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        // Add into an existing collection
        // equivalent to `X509Certificate2Collection.Import(data, password, keyStorageFlags)`
        public static X509Certificate2Collection LoadPkcs12Collection(
            byte[] data,
            string password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12Collection(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2Collection LoadPkcs12Collection(
            string path,
            string password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12Collection(
            string path,
            ReadOnlySpan<char> password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
    }

    public sealed class Pkcs12LoaderLimits
    {
        public static Pkcs12LoaderLimits Defaults { get; } = new Pkcs12LoaderLimits();

        public static Pkcs12LoaderLimits DangerousNoLimits { get; } = new Pkcs12LoaderLimits
        {
            MacIterationLimit = null,
            IndividualKdfIterationLimit = null,
            TotalKdfIterationLimit = null,
            MaxKeys = null,
            MaxCerts = null,
            PreserveStorageProvider = true,
            PreserveKeyName = true,
            PreserveCertificateAlias = true,
            PreserveUnknownAttributes = true,
        };

        public Pkcs12LoaderLimits();
        public Pkcs12LoaderLimits(Pkcs12LoaderLimits copyFrom);

        public bool IsReadOnly { get; }
        public void MakeReadOnly();

        public int? MacIterationLimit { get; set; } = 300_000;
        public int? IndividualKdfIterationLimit { get; set; } = 300_000;
        public int? TotalKdfIterationLimit { get; set; } = 1_000_000;
        public int? MaxKeys { get; set; } = 200;
        public int? MaxCertificates { get; set; } = 200;

        public bool PreserveStorageProvider { get; set; } // = false;
        public bool PreserveKeyName { get; set; } // = false;
        public bool PreserveCertificateAlias { get; set; } // = false;
        public bool PreserveUnknownAttributes { get; set; } // = false;

        public bool IgnorePrivateKeys { get; set; } // = false;
        public bool IgnoreEncryptedAuthSafes { get; set; } // = false;
    }

    public sealed class Pkcs12LoadLimitExceededException : CryptographicException
    {
        public Pkcs12LoadLimitExceededException(string propertyName)
            : base($"The PKCS#12/PFX violated the '{propertyName}' limit.")
        {
        }

        private Pkcs12LoadLimitExceededException(SerializationInfo info, StreamingContext context)
            : base(info, context)
        {
        }
    }

    // .NET 9+
    public partial class X509Certificate2
    {
        // mark all byte[] and fileName ctors as [Obsolete]
    }
}

[ghost]

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

Issue Details

---

Currently, loading a certificate from memory or a file is performed by the X509Certificate2 constructors or the X509Certificate2Collection.Import methods. These existing routines support many different formats (for single certificates: X509Certificate, PKCS#7 SignedCms, Windows Serialized Certificate, Authenticode-signed assets, and PKCS#12/PFX; for collections: X509Certificate, PKCS#7 SignedCms (interpreted differently than the single certificate case), Windows Serialized Store, and PKCS#12/PFX). Since many of those formats themselves support multiple encodings (e.g. X509Certificate-PEM and X509Certificate-DER), these members are very complicated.

While sometimes convenient to a caller, the design has proven lacking in multiple ways:

• When a protocol or file format indicates the presence of a certificate, new X509Certificate2(data) will unexpectedly allow several other file formats, making the most obvious code load data that other systems correctly reject as invalid.
• Of all the file formats these member support, only PKCS#12/PFX requires more options. These options are ignored when the input data/file is not a PKCS#12/PFX, leading to user confusion.
• PKCS#12/PFX requires more options... but the overloads that do not accept those options will provide defaults. Since PKCS#12/PFX is the only file format supported by these members that can also load private keys into memory, it isn't possible to understand the full security implications of new X509Certificate2(bytes).
• PKCS#12/PFX is a very complicated format which can be very expensive to load. Many .NET users have expressed desire for some control knobs to limit the total amount of work attempted.
• Authenticode-signed assets, Windows Serialized Certificates, and Windows Serialized Stores are only supported on Windows, but there's no way to mark that with [SupportedOS]

This proposal puts loader methods on a new type

namespace System.Security.Cryptography.X509Certificates
{
    public static partial class X509CertificateLoader
    {
        // A single X509Certificate value, PEM or DER
        // No collection variant needed.
        public static partial X509Certificate2 LoadX509(byte[] data);
        public static partial X509Certificate2 LoadX509(ReadOnlySpan<byte> data);
        public static partial X509Certificate2 LoadX509(string path);

        // A single Windows SerializedCert value
        // For high enough TFMs, will say [SupportedOS(Windows)].
        // No collection variant needed.
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadSerializedCert(byte[] data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadSerializedCert(ReadOnlySpan<data> data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadSerializedCert(string path);

        // Whatever new X509Certificate2("some.exe") does.
        // I think it extracts the SignedCms and then is the same as the next set.
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadAuthenticodeSigner(byte[] data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadAuthenticodeSigner(ReadOnlySpan<byte> data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2 LoadAuthenticodeSigner(string path);

        // Both the X509Certificate2 ctor and collection import can read PKCS7.
        // The single cert version returns the certificate used by the first SignerInfo in the file.
        // The collection one is very different, gets a new name.
        // PEM or BER
        public static partial X509Certificate2 LoadPkcs7PrimarySigner(byte[] data);
        public static partial X509Certificate2 LoadPkcs7PrimarySigner(ReadOnlySpan<byte> data);
        public static partial X509Certificate2 LoadPkcs7PrimarySigner(string path);

        // The collection version of PKCS7 returns all of the embedded X509Certificate values.
        // It doesn't care if they're referenced by a SignerInfo, or not.
        // Generally there isn't even a SignerInfo in a p7b collection.
        // PEM or BER
        public static partial X509Certificate2Collection LoadPkcs7EmbeddedCertificates(byte[] data);
        public static partial X509Certificate2Collection LoadPkcs7EmbeddedCertificates(ReadOnlySpan<byte> data);
        public static partial X509Certificate2Collection LoadPkcs7EmbeddedCertificates(string path);

        // Adds to an existing collection instead of allocating and returning a new one.
        public static partial void LoadPkcs7EmbeddedCertificates(
            byte[] data,
            X509Certificate2Collection collection);
        public static partial void LoadPkcs7EmbeddedCertificates(
            ReadOnlySpan<byte> data,
            X509Certificate2Collection collection);
        public static partial void LoadPkcs7EmbeddedCertificates(
            string path,
            X509Certificate2Collection collection);

        // SerializedStore versions of the PKCS7 embedded certs
        [SupportedOS(Windows)]
        public static partial X509Certificate2Collection LoadSerializedStore(byte[] data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2Collection LoadSerializedStore(ReadOnlySpan<byte> data);
        [SupportedOS(Windows)]
        public static partial X509Certificate2Collection LoadSerializedStore(string path);

        [SupportedOS(Windows)]
        public static partial void LoadSerializedStore(
            byte[] data,
            X509Certificate2Collection collection);
        [SupportedOS(Windows)]
        public static partial void LoadSerializedStore(
            ReadOnlySpan<byte> data,
            X509Certificate2Collection collection);
        [SupportedOS(Windows)]
        public static partial void LoadSerializedStore(
            string path,
            X509Certificate2Collection collection);

        // Load a PFX as a collection.
        // null loaderLimits means 
        public static X509Certificate2Collection LoadPkcs12(
            byte[] data,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2Collection LoadPkcs12(
            string path,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadUPkcs12(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        // Add into an existing collection
        // equivalent to `X509Certificate2Collection.Import(data, password, keyStorageFlags)`
        public static X509Certificate2Collection LoadPkcs12(
            byte[] data,
            string password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2Collection LoadPkcs12(
            string path,
            string password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2Collection LoadUPkcs12(
            string path,
            ReadOnlySpan<char> password,
            X509Certificate2Collection collection,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        
        // Load "the best" certificate from a PFX: first-cert-with-privkey ?? first-cert ?? throw.
        // equivalent to the certificate from `new X509Certificate2(data, password, keyStorageFlags)`
        public static X509Certificate2 LoadPkcs12SingleCertificate(
            byte[] data,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadUntrustedPkcs12SingleCertificate(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2 LoadUntrustedPkcs12SingleCertificate(
            string path,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadUntrustedPkcs12SingleCertificate(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
    }

    public sealed class Pkcs12LoaderLimits
    {
        public static readonly Pkcs12LoaderLimits Defaults = new Pkcs12LoaderLimits();

        public static readonly Pkcs12LoaderLimits DangerousNoLimits = new Pkcs12LoaderLimits
        {
            MacIterationLimit = null,
            IndividualKdfIterationLimit = null,
            TotalKdfIterationLimit = null,
            MaxKeys = null,
            MaxCerts = null,
            RespectStorageProvider = true,
            RespectKeyName = true,
            AllowMultipleEncryptedSegments = true,
            PreserveUnknownAttributes = true,
        };

        // NOTE: The object has a hidden read-only state so that (e.g.)
        // Pkcs12LoaderLimits.Defaults.MaxCerts cannot be changed.

        public int? MacIterationLimit { get; set; } = 300_000;
        public int? IndividualKdfIterationLimit { get; set; } = 300_000;
        public int? TotalKdfIterationLimit { get; set; } = 1_000_000;
        public int? MaxKeys { get; set; } = 200;
        public int? MaxCerts { get; set; } = 200;
        public bool RespectStorageProvider { get; set; } // = false;
        public bool RespectKeyName { get; set; } // = false;
        public bool AllowMultipleEncryptedSegments { get; set; } // = false;
        public bool PreserveUnknownAttributes { get; set; } // = false;

        public bool IgnorePrivateKeys { get; set; } // = false;
        public bool IgnoreEncryptedSegments { get; set; } // = false;
    }

    [Serializable]
    public sealed class Pkcs12LoadLimitExceededException : CryptographicException
    {
        public Pkcs12LoadLimitExceededException(string propertyName)
            : base($"The PKCS#12/PFX violated the '{propertyName}' limit.")
        {
        }

        private Pkcs12LoadLimitExceededException(SerializationInfo info, StreamingContext context)
            : base(info, context)
        {
        }
    }
}

Author:	bartonjs
Assignees:	-
Labels:	api-suggestion, area-System.Security
Milestone:	9.0.0

[morganbr]

This looks like a great improvement -- people generally know what file format to expect and lots of unexpected things can happen if they get something else instead (with many years of security and non-security bugs to prove it). Some minor thoughts on the proposed API (in no particular order):

1. Consider LoadX509Certificate instead of LoadX509 to match the type name
2. Are the serialized certificate and authenticode features actually useful to anyone? If so, do they belong here? (I've occasionally wanted to know what cert something was authenticode signed with, but I would have expected it to be on an api about module inspection. I'm not familiar with the serialized certificate format)
3. Are the PKCS7 method necessary vs getting the information from SignedCMS?
4. What's the scenario for the overloads that change password from a string to a span? Is this about keeping passwords off the GC heap?
5. Is LoadUPkcs12 a typo or a different format?
6. Are the "add cert to an existing collection" overloads necessary vs loading the certs and adding them yourself? If they are, consider naming them differently than the ones that make a new collection (e.g. LoadPkcs12 vs AddPkcs12ToCollection)
7. Is defaulting X509KeyStorageFlags a good idea? People being confused about where their keys are is a pretty common source of bugs, so maybe they should have to choose
8. Consider a default of 1 for MaxKeys and MaxCerts in Pkcs12LoaderLimits -- most people think they're loading a single certificate
9. What does passing a null Pkcs12LoaderLimits do? No limits? That sounds like a security risk, consider making it non-nullable and passing Pkcs12LoaderLimits.Defaults by default
10. I'm not familiar with all of the knobs in Pkcs12LoaderLimits. Maybe they're all necessary, but I wonder if there's any way to simplify it for those not deeply versed on the format (time/memory limits?)
11. LoadX509 breaks the "one method family per format" rule, but maybe that's ok? It'd just be nice to be sure we don't have to come back and split it later
12. The setters on Pkcs12LoaderLimits that you shouldn't call in certain cases feel a little weird. Consider making Pkcs12LoaderLimits have all readonly properties that get set in a constructor or with a builder
13. Are there any other key storage options that should be available (e.g. so you can import from a pfx into an HSM or know that you can access a CngKey?)

[vcsjones]

Are the serialized certificate and authenticode features actually useful to anyone? If so, do they belong here?

I’ve seen a few people want something like this. But I have a slightly different concern: we don’t have a 1:1 of that today. We have “Get whatever CryptQueryObject will give us” which can include up to the authenticode cert. If this API is meant to do just the authenticode cert, we’ll have to use the appropriate Win32 API.

If so, do they belong here?

If there is ever a chance of us deprecating the constructors (which is a separate discussion), we need some kind of parity.

What's the scenario for the overloads that change password from a string to a span? Is this about keeping passwords off the GC heap?

We have constructors that take span for the password already. Generally, we don’t mix-and-match arrays and spans. Using a derived password on the stack which isn’t subject to GC compaction is one possible reason.

What does passing a null Pkcs12LoaderLimits do? No limits? That sounds like a security risk, consider making it non-nullable and passing Pkcs12LoaderLimits.Defaults by default

null means “use Pkcs12LoaderLimits.Defaults“. Optional parameters for reference types can only be null, so we can’t make the default value Pkcs12LoaderLimits.Defaults.

If we think the optional parameter nullability is confusing we can switch to overloads. I think the null is reasonable and explainable in the <param> doc.

LoadX509 breaks the "one method family per format" rule, but maybe that's ok? It'd just be nice to be sure we don't have to come back and split it later

One concern that I had was we are going from a “do all the things” API to something much more specific. I had expressed some concern that if the APIs are so hyper-specific folks will just keep using the constructors because they load it without much effort.

Consider a library that wants to load a certificate from disk created by a customer. Some customers might have it in DER, some might have it in PEM. As the library author, I would have to do some try / catch attempts.

From my perspective, its allow multiple encoding (DER, PEM) but still one format (X.509). So it’s still just one thing if you squint right.

I'm not familiar with all of the knobs in Pkcs12LoaderLimits. Maybe they're all necessary, but I wonder if there's any way to simplify it for those not deeply versed on the format (time/memory limits?)

My gut feeling is that most people will never instantiate this class. They will use the defaults or the YOLO option. I expect that the only people that would are the people that know what all these knobs mean.

(time/memory limits?)

I think that would be difficult, not just implementation, but doing it correctly. The largest time issue is PBKDF. We don’t have a good way to cancel an in-flight PBKDF operation while it is iterating. Estimating the time needed by iterations is going to be very dependent on hardware.

[bartonjs]

We have “Get whatever CryptQueryObject will give us” which can include up to the authenticode cert.

I believe that "Authenticode signer" is CERT_QUERY_CONTENT_PKCS7_SIGNED_EMBED. It worked in my prototype, at least. If it matches more than just Authenticode then the method name would warrant an update (but I feel like the last time I went spelunking in the object loader, CERT_QUERY_CONTENT_PKCS7_SIGNED_EMBED == Authenticode)

[bartonjs]

Is LoadUPkcs12 a typo or a different format?

A typo. A previous offline iteration of the proposal had "LoadTrustedPkcs12" and "LoadUntrustedPkcs12", and apparently one of them only removed "ntrusted" 😄. The distinction was removed when @vcsjones pointed out that it's unclear if the caller is an untrusted context or the file is untrusted/potentially-hostile. So they got merged with a defaulted loader limits.

I'm not familiar with the serialized certificate format

It's a Windows proprietary format of the certificate and any custom-applied properties from CertSetCertificateProperty. It's only here because a) it already works, and b) we allow certs to be exported as it via cert.Export.

Are the "add cert to an existing collection" overloads necessary vs loading the certs and adding them yourself?

Probably not. "add to an existing collection" is the best match for coll.Import, but "give me a collection" is probably what more people want. Since the overload is fairly cheap, and does save a modicum of GC work tracking an undesired intermediate collection, I went ahead and included it. I'm happy to be talked down from it, but since I expect API Review to ask about that variant if it's missing I think I want to leave it in and let it get talked away during the meeting.

Are the PKCS7 method necessary vs getting the information from SignedCMS?

Arguably not. The concern would be people who cared about it, and then go "uh, it was one line, and now it's 3. I'm going to keep calling this Obsolete thing." (The same argument is why we've not felt like super incredibly terrible people for punting this on Linux since, well, .NET Core 1.0 (still doesn't work)... instead, we (I) merely feel "kinda bad")

SignedCms cms = new SignedCms();
cms.Decode(contents);
X509Certificate2Collection coll = cms.Certificates;
X509Certificaet2 cert = cms.SignerInfos[0].Certificate;

Consider LoadX509Certificate instead of LoadX509 to match the type name

That's fair, and Certificate (vs Cert) is the name of the structure in the RFC ASN. And probably the X.509 document, but I don't have it quickly at hand on this computer.

Is defaulting X509KeyStorageFlags a good idea?

I -wanted- to default it to EphemeralKeySet. But that doesn't work on macOS (/me glares at Keychain + SecIdentityRef). And since it's different it would probably confuse people.

Not defaulting it sounds quite reasonable.

Consider a default of 1 for MaxKeys and MaxCerts in Pkcs12LoaderLimits -- most people think they're loading a single certificate

My gut says there are too many "full chain" PFXes and they would get negatively impacted (since the API would throw for them exceeding the limit). And a default of 1 would be pretty cumbersome on the collection-based load. (We could have Defaults and CollectionDefaults, or SingleCertDefaults/CollectionDefaults, whatever). 200/200 is, I believe, the Win32 defaults. Or maybe just a number that I picked that sounded like "ye gods, if you need to raise this you're doing something really wrong" 😄.

The setters on Pkcs12LoaderLimits that you shouldn't call in certain cases feel a little weird.

The setters are all fine, unless you are trying to change the values on one of the two shared instances.

Consider making Pkcs12LoaderLimits have all readonly properties that get set in a constructor or with a builder

Like @vcsjones, I don't expect a lot of customization here, despite creating a bunch of knobs. The most likely thing I'd expect someone to change is to enable RespectStorageProvider, when they find that the old API made a cert work (because it ended up in CAPI) and the new one doesn't (because it ends up in CNG). Needing to specify all of the options redundantly to change the one feels awkward.

A With-style builder would create quite a lot of garbage copies if someone really cared about customizing things (e.g. someone in a security team having Opinions). I'm not sure that's what you meant, or if you had something more friendly in mind. (Note that this is a class, not a struct, because it's bigger than struct guidelines (especially with all those nullable ints))

I'm not familiar with all of the knobs in Pkcs12LoaderLimits. Maybe they're all necessary,

Most of the knobs exist in one form or another in Windows, though many are regkeys. This is a bit of demystification, and a bit of allowing similar control on macOS/Linux as Windows already has (and, even better, doing it per-load instead of per-system).

For the usability concern, I expect people to generally just use Defaults, or maybe Defaults with the CSP preserved; aside from a few dozen people around the globe who use all of them.

Are there any other key storage options that should be available (e.g. so you can import from a pfx into an HSM or know that you can access a CngKey?)

Windows just has "do what it says", "force the software CNG provider", and "try CNG but if that doesn't work do what it says". This API is "CNG" (RespectKeyStorageProvider == false) and "what it says" (true).

Someone clever could use Pkcs12Builder to build a PFX that says to use the "platform storage provider" (the TPM provider) or a specific HSM provider, but I don't think we really want to do anything weird for that here.

[filipnavara]

If there is ever a chance of us deprecating the constructors (which is a separate discussion), we need some kind of parity.

For the sake of completeness, we do have the requirement to parse whatever supported format is thrown at us (majority of the cases being X509 or PFX). This is used in our UI for certificate management for the import functionality. We load the data and then decide where it belong (has private key => Personal certificates, rest depending on X509 attributes => Other people certificates or Certificate authorities).

On other places (eg. inside S/MIME or CMS containers) we know the format and we would actually prefer these specific APIs.

The former use case can be covered by usage of X509Certificate2.GetCertContentType and then calling the appropriate loader method. I guess I would welcome a guidance whether it is preferred to switch to this pattern, why / why not, and what are the possible performance implications.

[filipnavara]

I -wanted- to default it to EphemeralKeySet. But that doesn't work on macOS (/me glares at Keychain + SecIdentityRef). And since it's different it would probably confuse people.

We have some leeway when it comes to .NET 9. The iOS-style keychain APIs should enable this scenario if we iron out the quirks and compatibility problems.

(Not sure how you plan to handle down-level compatibility for Microsoft.Bcl.Crytography if we need to use the native bits though. We currently don't have any pre-existing solution for this scenario.)

[vcsjones]

Consider LoadX509Certificate instead of LoadX509 to match the type name
That's fair, and Certificate (vs Cert) is the name of the structure in the RFC ASN.

I think Jeremy had this in his original proposal, and I pointed out that X509CertificateLoader.LoadX509Certificate is repetitious. Load is too ambiguous, LoadX509 seemed okay to me, and LoadX509Certificate is repeating what the class already says it does.

A With-style builder would create quite a lot of garbage copies if someone really cared about customizing things (e.g. someone in a security team having Opinions). I'm not sure that's what you meant, or if you had something more friendly in mind. (Note that this is a class, not a struct, because it's bigger than struct guidelines (especially with all those nullable ints))

I wrestled with the class too, but think the mutable properties make the most sense*. If there are people that care about one particular knob, like what KSP it lands in, but have no idea what the rest of the knobs do, people would need to know how to pass the defaults for the constructor args they don't care about.

There is, at least according to the comments, a notion of some kind of internal immutability so our pre-instantiated ones are not mutated.

* Actually I think this is a rare place I would use a record, but since we need to support down level compilers, we can't. It would look like this:

Pkcs12LoaderLimits myCustomLimit = Pkcs12LoaderLimits.Defaults with { IgnorePrivateKeys = true, IgnoreEncryptedSegments = true };

---

Not sure how you plan to handle down-level compatibility for Microsoft.Bcl.Crytography if we need to use the native bits though. We currently don't have any pre-existing solution for this scenario.

Yeah, largely I think we need to be able to implement this loader in terms of managed code, at least on non-Windows (Windows is always just p/invoke). If someone uses this package on .NET 6, we can't depend on the native shims having a particular behavior.

---

@bartonjs Should the pre-constructed loader limits be fields or properties with a get only? The JIT will typically inline those simple property gets, and I think we more typically like to expose properties.

Also wondering for AllowMultipleEncryptedSegments if "segments" is the right word. BagItems is probably a... technically... better name, but I don't know that it makes any more sense to other folks. I also don't know if that knob is necessary at all. If there are multiple encrypted bags, I would still expect the MacIterationLimit to stop a runaway from too much PBKDF work.

[bartonjs]

Should the pre-constructed loader limits be fields or properties with a get only?

Yeah, they should've been properties. Updated.

Also wondering for AllowMultipleEncryptedSegments if "segments" is the right word. BagItems is probably a... technically... better name, but I don't know that it makes any more sense to other folks.

In usage, it's actually "AuthSafes". I forget exactly what I was going for there, other than that 9-9s of PFXes won't have more than one, and if there's more than one someone's probably trying to be mean (or test some compatibility thing, I guess). Since I don't have a numeric AuthSafes limit, I probably did this in its place. But it's probably cuttable as it's generally covered by the KDF limit.

[vcsjones]

But [AllowMultipleEncryptedSegments is] probably cuttable as it's generally covered by the KDF limit.

I would cut it, but if you want to save it for API discussion, that's fine.

TotalKdfIterationLimit

Does this include the MAC limit? If there is a MAC with 299,999 iterations, and a key with 2 iterations, and TotalKdfIterationLimit is 300,000, will it blow up?

If yes, then we should probably strike Kdf from the name. If no, I wonder if it should be.

Is there any reasonable scenario in which these limits should be exposed as a long? I don't know that anyone would ever want to open a P12 with > int.MaxValue work factor. I guess I just want it written down for posterity that it doesn't make sense, but, I guess it's possible.

I have convinced myself this is not sane. If you have that many iterations you might as well set the work limit to null.

// .NET 9+
public partial class X509Certificate2
{
// mark all byte[] and fileName ctors as [Obsolete]
}

Should we strike that from this proposal and and open a new one once this is in? I suspect this will be a hot topic of debate. Maybe not.

RespectStorageProvider / RespectKeyName

I would consider replacing "Respect" with "Preserve". We have several other APIs with "Preserve" but none with "Respect".

[bartonjs]

Does TotalKdfIterationLimit include the MAC limit? If there is a MAC with 299,999 iterations, and a key with 2 iterations, and TotalKdfIterationLimit is 300,000, will it blow up?

No, there are three limits:

1. A MAC iteration limit.
2. An individual KDF iteration limit.
3. A cumulative KDF (non-MAC) iteration limit.

Since there's only zero or one MACs I felt like it was good enough being a completely separate work bucket. It's best paired with the cumulative KDF limit, since together they're "how much crypto work are we willing to do?" (vs the cert/key limits, which are about how expensive the pairwise matching can be). The individual KDF limit is also there because, well, we already had one as a "bogus value" detector. But the proposed defaults aren't total_kdf_limit = individual_kdf_limit * keys, it's way less than that.

I would consider replacing "Respect" with "Preserve".

Sounds good.

Should we strike [deprecation] from this proposal?

My intent is to say that's where we want to go, and if the meeting doesn't immediately agree then defer that to a later issue. Unless you're thinking it will be a hot debate in the issue, vs the review meeting?

[vcsjones]

My intent is to say that's where we want to go, and if the meeting doesn't immediately agree then defer that to a later issue. Unless you're thinking it will be a hot debate in the issue, vs the review meeting?

No, the meeting - I just don't want to block this getting approved on sorting the deprecation.

[bartonjs]

Video

API Review comments:

Pkcs12LoaderLimits:

• Add MakeReadOnly, IsReadOnly and a copy constructor
• Remove AllowMultipleEncryptedSegments
• IgnoreEncryptedSegments => IgnoreEncryptedItems if it's all items, IgnoreEncryptedAuthSafes if it's auth safes.
• MaxCerts => MaxCertificates

X509CertificateLoader:

• Remove SerializedCert, SerializedStore, AuthenticodeSigner, Pkcs7PrimarySigner, and Pkcs7EmbeddedCertificates
• Rename LoadX509Certificate to LoadCertificate
• LoadPkcs12 => LoadPkcs12Collection
• LoadPkcs12SingleCertificate => LoadPkcs12

To be continued...

[krwq]

Sorry I'm a bit late here with review - I thought about this a bit and I'm trying to think of a case where you'd actually need all those knobs on the limiter in real life scenario and I think it only really makes sense to set this up once and you likely don't need to touch this ever again. In my ideal scenario this API should be taking CancellationToken rather than limiter but this is currently impossible to do on Windows without doing cert parsing entirely in managed code and it would probably be way slower to do KDF with managed (perf is not super important for KDF but we're probably talking several times slower).

Since that's unrealistic I think it would make more sense to do just a single uint value with some arbitrary unit which user doesn't need to concern themselves about - they only should care if this is large enough for their scenario or not. I don't see individual/total limit that useful - it should always be total IMO.

Most of the time I'd imagine people be like: my cert doesn't load, let me increase the limit.

For the types I think most of the people won't understand which format they have and they will blindly guess until they find or (hopefully not) they will use some online tool to find out potentially risking exposing their keys. I think most of the people should only care about "Public" and "Private" and not PKCS7/PKCS12/PEM combination but we can provide that control.

I'd imagine the shape of APIs look somewhat similar to:

public static partial class X509CertificateLoader
{
    public static partial X509Certificate2 LoadCertificate(ReadOnlySpan<byte> data, X509ContentType2 contentType = X509ContentType2.PublicOnly, X509CertificateLoaderLimits limits = null);
    // + byte[] and password overloads

    public static partial IEnumerable<X509Certificate2> EnumerateCertificates(ReadOnlySpan<byte> data, X509ContentType2 contentType = X509ContentType2.PublicOnly, X509CertificateLoaderLimits limits = null);
    // + byte[] and password overloads

    // None for invalid data?
    // bool flag is so that we can skip checking for presence of private/public keys
    public static partial X509CertificateType GetCertificateType(ReadOnlySpan<byte> data, bool checkKeys = false, X509CertificateLoaderLimits limits = null);
}

// Being explicit makes sense but there is quite a bit of combinations here
// we could argue you only need Public, Private, None, Any
[Flags]
public enum X509CertificateType
{
  None = 0,

  Any = Public | Private,
  Public = NoPrivateKey | Pkcs7 | Pkcs12 | DER | PEM,
  Private = WithPrivateKey | Pkcs7 | Pkcs12 | DER | PEM,

  // Those don't need to be public but they're building flags
  NoPrivateKey   = 1 << 0, // also implies no work allowed if WithPrivateKey not specified
  WithPrivateKey = 1 << 1, // implies default limit

  Pkcs7          = 1 << 2,
  Pkcs12         = 1 << 3,

  DER            = 1 << 4,
  PEM            = 1 << 5,
}

// I don't like using PKCS12 in the name because it looks weird in the LoadCertificate overload
// also we can reuse that in the future or in other contexts if this remains fairly
public class X509CertificateLoaderLimits
{
    // these don't need to be public but I think it makes stuff easier, also we can expose uint directly as const
    public static X509CertificateLoaderLimits Default = new() { AllowedWork = 12314124; } // some sane default
    public static X509CertificateLoaderLimits NoWork = new() { AllowedWork = 0; }
    public static X509CertificateLoaderLimits Unlimited = new() { AllowedWork = null; }
    public static X509CertificateLoaderLimits IncreasedLimit = new() { AllowedWork = 12314124 * 5; } // some sane default for people who use larger KDF iterations but don't want forever

    // arbitrary unit of work user would need to play around with to find minimum number for their scenario
    // null = unlimited
    public required uint? AllowedWork { get; init; }
}

I'd also skip on File overloads and all Windows specific stuff to be honest.

I'm ok with adding extra options defining which types of keys are allowed (i.e. blocking/allowing DPAPI) and perhaps other things but honestly for other than DPAPI stuff I expect this will have usage close to zero.

[bartonjs]

I'd also skip ... all Windows specific stuff to be honest.

Yeah, that was all cut in the first review session, along with Pkcs7 (since you can just use SignedCms for that).

I'm trying to think of a case where you'd actually need all those knobs on the limiter in real life scenario

Generally, one wouldn't. 95% of callers will want the defaults, 1% will want DangerousNoLimits, 4% will want to toggle one of the preserve or ignore options, and the rest is mainly so that if we ever feel justified in ratcheting a default tighter that someone can undo us breaking a file that used to work.

with some arbitrary unit which user doesn't need to concern themselves about

That's pretty hard to document. The numbers in the limiter correspond to numbers in the spec. So at least someone somewhere can have Opinions and tie them in to things in RFCs.

I'd imagine the shape of APIs look somewhat similar to...

Unless I've missed something, you don't collect a password for Pkcs12. And I think that the flags enum for what formats are supported is a pit of failure... people will specify .Any because it always works, and then they may as well use the current API.

DER = 1 << 4, PEM = 1 << 5

@vcsjones and I had chatted about PEM-or-DER, and decided it was too annoying for most people to want to have to deal with as separate methods.

I was in the process of saying it might be helpful on API that are part of a protocol that expect only PEM or DER(BER) in context, but I think that's only true if the API rejects extraneous data (which we said it won't) -- trailing data for BER/DER, or anything other than whitespace outside the PEM encapsulation boundary for PEM.

[bartonjs]

Video

• Added [UnsupportedOSPlatform("browser")] to match the current X509Certificate2 ctors.
• Made all the string password parameters be string? password because that's more correct
• Added "FromFile" to the file-loading ones
• We cut the "Load into" collection methods, we can always add them back later.
• We should also obsolete the collection Import methods

namespace System.Security.Cryptography.X509Certificates
{
    [UnsupportedOSPlatform("browser")]
    public static partial class X509CertificateLoader
    {
        // A single X509Certificate value, PEM or DER
        // No collection variant needed.
        public static partial X509Certificate2 LoadCertificate(byte[] data);
        public static partial X509Certificate2 LoadCertificate(ReadOnlySpan<byte> data);
        public static partial X509Certificate2 LoadCertificateFromFile(string path);

        // Load "the best" certificate from a PFX: first-cert-with-privkey ?? first-cert ?? throw.
        // equivalent to the certificate from `new X509Certificate2(data, password, keyStorageFlags)`
        public static X509Certificate2 LoadPkcs12(
            byte[] data,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadPkcs12(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2 LoadPkcs12FromFile(
            string path,
            string password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static partial X509Certificate2 LoadPkcs12FromFile(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);

        // Load a PFX as a collection.
        // null loaderLimits means Pkcs12LoaderLimits.Default
        public static X509Certificate2Collection LoadPkcs12Collection(
            byte[] data,
            string? password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12Collection(
            ReadOnlySpan<byte> data,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
        public static X509Certificate2Collection LoadPkcs12CollectionFromFile(
            string path,
            string? password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits loaderLimits = null);
        public static partial X509Certificate2Collection LoadPkcs12CollectionFromFile(
            string path,
            ReadOnlySpan<char> password,
            X509KeyStorageFlags keyStorageFlags = X509KeyStorageFlags.DefaultKeySet,
            Pkcs12LoaderLimits? loaderLimits = null);
    }

    public sealed class Pkcs12LoaderLimits
    {
        public static Pkcs12LoaderLimits Defaults { get; } = new Pkcs12LoaderLimits();

        public static Pkcs12LoaderLimits DangerousNoLimits { get; } = new Pkcs12LoaderLimits
        {
            MacIterationLimit = null,
            IndividualKdfIterationLimit = null,
            TotalKdfIterationLimit = null,
            MaxKeys = null,
            MaxCertificates = null,
            PreserveStorageProvider = true,
            PreserveKeyName = true,
            PreserveCertificateAlias = true,
            PreserveUnknownAttributes = true,
        };

        public Pkcs12LoaderLimits();
        public Pkcs12LoaderLimits(Pkcs12LoaderLimits copyFrom);

        public bool IsReadOnly { get; }
        public void MakeReadOnly();

        public int? MacIterationLimit { get; set; } = 300_000;
        public int? IndividualKdfIterationLimit { get; set; } = 300_000;
        public int? TotalKdfIterationLimit { get; set; } = 1_000_000;
        public int? MaxKeys { get; set; } = 200;
        public int? MaxCertificates { get; set; } = 200;

        public bool PreserveStorageProvider { get; set; } // = false;
        public bool PreserveKeyName { get; set; } // = false;
        public bool PreserveCertificateAlias { get; set; } // = false;
        public bool PreserveUnknownAttributes { get; set; } // = false;

        public bool IgnorePrivateKeys { get; set; } // = false;
        public bool IgnoreEncryptedAuthSafes { get; set; } // = false;
    }

    public sealed class Pkcs12LoadLimitExceededException : CryptographicException
    {
        public Pkcs12LoadLimitExceededException(string propertyName)
            : base($"The PKCS#12/PFX violated the '{propertyName}' limit.")
        {
        }
    }

    // .NET 9+
    public partial class X509Certificate2
    {
        // mark all byte[] and fileName ctors as [Obsolete]
    }

        // .NET 9+
    public partial class X509Certificate2Collection
    {
        // mark all byte[] and fileName Import methods as [Obsolete]
    }
}

[Jozkee]

Sorry if this was discussed already and I missed it but... why don't ignore private keys by default?

public sealed class Pkcs12LoaderLimits
{
+    public bool PreservePrivateKeys { get; set; } // = false;
-    public bool IgnorePrivateKeys { get; set; } // = false;

        public static Pkcs12LoaderLimits DangerousNoLimits { get; } = new Pkcs12LoaderLimits
        {
            MacIterationLimit = null,
            IndividualKdfIterationLimit = null,
            TotalKdfIterationLimit = null,
            MaxKeys = null,
            MaxCertificates = null,
            PreserveStorageProvider = true,
            PreserveKeyName = true,
            PreserveCertificateAlias = true,
            PreserveUnknownAttributes = true,
+           PreservePrivateKeys = true,
        };
}

EDIT: Hmm now that I think about it, it could be too annoying to have to init Pkcs12LoaderLimits just to allow private keys since you want to load them in most cases? Adding it to DangerousNoLimit may lure users to set and forget dangerous limits.

[bartonjs]

why don't ignore private keys by default?

99.99% of the time someone wants them, that's why they have a PFX instead of a cert. The other 0.01% is that someone just wants to inspect a PFX to say what's in it, and not load (or even decrypt) private keys.