Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[API Proposal]: X509Certificate.ExportPkcs12 with PBE parameters #80314

Open
vcsjones opened this issue Jan 6, 2023 · 10 comments
Open

[API Proposal]: X509Certificate.ExportPkcs12 with PBE parameters #80314

vcsjones opened this issue Jan 6, 2023 · 10 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Jan 6, 2023

Background and motivation

Right now Export(X509ContentType.Pkcs12, ...) assumes 3DES, SHA1, and 2000 rounds of PBKDF. These are not great in 2023 and there is no control over it today. Developers could use Pkcs12Builder themselves, but, that's a fairly low-level mechanism that requires understanding how PKCS12 works.

I propose we expose new APIs that allow stronger PKCS12 exports.

In the proposal, there are two overloads. An enum that provides simple options that match Windows behavior. The second is PbeParameters where people can have total control over the encryption, hash, and number ob PBES2 rounds.

Win32 by default does not give us the flexibility that is needed to implement PbeParameters, as it does not let us choose the number of PBES2 rounds. In that case, the export will be performed using an existing Win32 API, decoded, and re-encoded with the managed implementation using the specified parameters.

API Proposal

namespace System.Security.Cryptography.X509Certificates;

public partial class X509Certificate  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
    public byte[] ExportPkcs12(PbeParameters exportParameters, string? password);
}

public partial class X509Certificate2Collection  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
    public byte[] ExportPkcs12(PbeParameters exportParameters, string? password);
}


public enum Pkcs12ExportPbeParameters {
    // Initially will behave as `Pbes2TripleDesSha1`, but reserved to change behavior in the future.
    Default = 0,

    // TripleDes3KeyPkcs12, SHA1, 2000 PBKDF2 rounds. Matches Win32.
    Pbes2TripleDesSha1 = 1,

    // AES-256, SHA256, 2000 PBKDF2 rounds. Matches Win32.
    Pbes2Aes256Sha256 = 2,
}

API Usage

X509Certificate certificate = GetCert();
byte[] pkcs12 = certificate.ExportPkcs12(Pkcs12ExportPbeParameters.Pbes2Aes256Sha256, "potato");
@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Jan 6, 2023
@ghost
Copy link

ghost commented Jan 6, 2023

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

Issue Details

Background and motivation

Right now Export(X509ContentType.Pkcs12, ...) assumes 3DES, SHA1, and 2000 rounds of PBKDF. These are not great in 2023 and there is no control over it today. Developers could use Pkcs12Builder themselves, but, that's a fairly low-level mechanism that requires understanding how PKCS12 works.

Windows 10 1709 supports PKCS12_EXPORT_PBES2_PARAMS which doesn't allow total control over the PBE parameters, but it lets opting in to stronger AES+SHA256 PBE. I propose we expose this in a new method called ExportPkcs12.

Since this is only present on recent versions of Windows, this would throw PNSE on down level platforms. Non-Windows platforms implement this in managed code, so we can swap out s_windowsPbe as needed depending on the input.

API Proposal

namespace System.Security.Cryptography.X509Certificates;

public partial class X509Certificate  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
}

public enum Pkcs12ExportPbeParameters {
    Default = 0,
    Pbes2Aes256Sha256 = 1,
}

API Usage

X509Certificate certificate = GetCert();
byte[] pkcs12 = certificate.ExportPkcs12(Pkcs12ExportPbeParameters.Pbes2Aes256Sha256, "potato");

Alternative Designs

I originally had a more complicate design that attempted to be a "classenum" so that we have more flexibility in the future. A future version of this class might have a public constructor that accepted PbeParameters, for example. Or some kind of configuration that can't be expressed in an enum.

After thinking about it though, I deemed this too complicated. If, in the future, we need this kind of capability, we can overload ExportPkcs12. I'll leave it here for posterity, maybe someone can convince me it's actually useful.

namespace System.Security.Cryptography.X509Certificates;

public partial class X509Certificate  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
}

public sealed class Pkcs12ExportPbeParameters : IEquatable<Pkcs12ExportPbeParameters> {
    public static Pkcs12ExportPbeParameters Default { get; }
    public static Pkcs12ExportPbeParameters Pbes2Aes256Sha256 { get; }
    public static bool operator ==(Pkcs12ExportPbeParameters? left, Pkcs12ExportPbeParameters? right);
    public static bool operator !=(Pkcs12ExportPbeParameters? left, Pkcs12ExportPbeParameters? right);
    // Override Equals, GetHashCode, etc.
}

Risks

It's a Window 10 1709+ API. This will throw PNSE on down level Windows if Pbes2Aes256Sha256 is supplied.

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 6, 2023
@bartonjs
Copy link
Member

bartonjs commented Jan 6, 2023

Amusingly, I was just answering an SO question yesterday where someone had an AES-256/SHA-256 PFX that they couldn't load (because it was being loaded by Mono, which doesn't support the newer revision with PBES2).

The version with the enum is probably a good balance of allowing more modern algorithm choices without impeding a common operating too much... but I wonder if we should just go ahead and overload it with PbeParameters at the same time to allow the much richer control.

We should add TripleDes192Sha1 to the enum now, though, so that we can have "Default means we pick, but you have the option of specifying 'legacy' if we ever change the defaults".

It's a Window 10 1709+ API. This will throw PNSE on down level Windows if Pbes2Aes256Sha256 is supplied.

We could probably avoid the PNSE and just have both the OS-exporter and our custom-exporter available on Windows.

@vcsjones
Copy link
Member Author

vcsjones commented Jan 6, 2023

@bartonjs

We could probably avoid the PNSE and just have both the OS-exporter and our custom-exporter available on Windows.

I thought about that but.. I got a little concerned about all of the Win32-isms and Bag Attributes that get applied. It might be surprising to folks that using AES means "you don't get a keyAttributes on your PKCS12 bag anymore". But maybe that is acceptable for Windows 8 / Server 2012.

We should add TripleDes192Sha1 to the enum now, though, so that we can have "Default means we pick, but you have the option of specifying 'legacy' if we ever change the defaults".

That makes sense if we are okay with using a managed export on Windows. Win32 doesn't let us explicitly pick TripleDes192Sha1. It lets us pick "Whatever the OS decides" and "Aes256Sha256". Is there a world in which Windows changes their own default?

but I wonder if we should just go ahead and overload it with PbeParameters at the same time to allow the much richer control.

My concern is largely the same as the first one, where if on Windows you use the managed one, you might not get all of the bag attributes that Windows likes to shove in there.

If we're okay losing bag attributes on Windows... then... I guess we can do PbeParameters and 3DES on the enumeration.

@bartonjs
Copy link
Member

bartonjs commented Jan 7, 2023

Worst case: we could always let Windows export it, then we decompose and recompose it with the better algorithms...

@vcsjones
Copy link
Member Author

vcsjones commented Jan 7, 2023

we could always let Windows export it, then we decompose and recompose it with the better algorithms...

I am annoyed that did not occur to me. This sounds reasonable to me, and should make it (relatively) straight forward to make this work with any set of PbeParameters. In that sense then I'm not sure there is much value in using the new Win32 API at all. We can just use the old one, crack it open, and re-encode it.

I'll adjust the proposal.

It also occurs to me that we probably want symmetry with X509Certificate2Collection

@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 17, 2023
@bartonjs bartonjs added this to the 8.0.0 milestone Jan 17, 2023
@bartonjs
Copy link
Member

bartonjs commented Mar 7, 2023

Video

Looks good as proposed.

We discussed replacing the enum with prepopulated accelerators on PbeParameters, but the enum values don't specifically encode the iteration count (where we're following OS defaults).

namespace System.Security.Cryptography.X509Certificates;

public partial class X509Certificate  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
    public byte[] ExportPkcs12(PbeParameters exportParameters, string? password);
}

public partial class X509Certificate2Collection  {
    public byte[] ExportPkcs12(Pkcs12ExportPbeParameters exportParameters, string? password);
    public byte[] ExportPkcs12(PbeParameters exportParameters, string? password);
}


public enum Pkcs12ExportPbeParameters {
    // Initially will behave as `Pbes2TripleDesSha1`, but reserved to change behavior in the future.
    Default = 0,

    // TripleDes3KeyPkcs12, SHA1, 2000 PBKDF2 rounds. Matches Win32.
    Pbes2TripleDesSha1 = 1,

    // AES-256, SHA256, 2000 PBKDF2 rounds. Matches Win32.
    Pbes2Aes256Sha256 = 2,
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 7, 2023
@vcsjones vcsjones self-assigned this Mar 7, 2023
@vcsjones vcsjones modified the milestones: 8.0.0, Future May 3, 2023
@vcsjones vcsjones removed their assignment Jun 16, 2023
@vcsjones vcsjones marked this as a duplicate of #29200 Jan 7, 2025
@vcsjones
Copy link
Member Author

@bartonjs I have a prototype of this, but the question that arose from this is "Should we inbox Pkcs12Info and Pkcs12Builder?"

The strategy that ended up working the best is to do an export from the system and re-write it with the specified PBE parameters. This is more or less required for Windows because we need all of the attributes from the cert store. On other platforms we can just build the PKCS12 object from the respective pieces.

The reason for inboxing is it ends up being a lot of shared source between S.S.Cryptography and S.S.C.Pkcs. Anyone that has a package reference to S.S.C.Pkcs is going to be carrying around two copies of the PKCS12 reader / builder since they would be types with different identities.

@bartonjs
Copy link
Member

Inboxing the type would effectively stop it from evolving in the NuGet package... but I don't think either of those types (or their closure) has ever changed; so it's not like anyone would notice.

We'd still need to build them for downlevel platforms, so they'd have to move into shared source. At that point it's sort of "6 of one, half-dozen of the other" if they're shared as internal vs public or if it's inboxed (engineeringwise).

If you want to hedge, share it with a conditional access modifier. If you believe that there's benefit to fully moving the public types, and can articulate a reason, then I'll deal with the paperwork.

@vcsjones
Copy link
Member Author

vcsjones commented Jan 18, 2025

@bartonjs the main scenario I had in mind was trimmability. If you have a .NET 10 app that does:

X509Certificate2 cert = GetCert();
cert.Export(X509ContentType.Pkcs12);

and

Pkcs12Builder builder = new();

Because they are shared source, they have different type identities. Your trimmed application will carry two copies of Pkcs12Builder, one internal used by Export and one from System.Security.Cryptography.Pkcs. This applies to all of the supporting types as well, like Pkcs12Info, all of the Asn serializer types, the helpers, etc.

@vcsjones
Copy link
Member Author

vcsjones commented Jan 18, 2025

Alternatively, the easier thing to do is what we do with System.Formats.Asn1. Just inbox the assembly and then the OOB is no longer needed for .NET. If X509CertificateLoader is ever going to get PKCS7 support, then it will probably do so with SignedCms.

Nope, that would not solve the circular dependency problem. We would need to type forward, if we inbox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

No branches or pull requests

2 participants