Decide on a policy for duplicate attributes on PKCS12 values

[bartonjs]

As currently written, the new X509CertificateLoader class allows duplicate attributes. For Windows, the current filtering/inclusion algorithm will have the effect of reversing these attributes from their input order when compared to the DangerousNoLimits import.

Some possible outcomes:

• Allow duplicates, change to an order-preserving filter.
• Allow duplicates, permitting inconsistent ordering.
• Reject duplicates for items being imported (e.g. if IgnorePrivateKeys is true, don't check for duplicates in private keys)
• Reject duplicates for all importable objects (e.g. check private keys irrespective of IgnorePrivateKeys, but do not check SecretBag values)
• Reject duplicates for all objects
• Reject duplicates for imported/importable/all objects, but only for the few well-known OIDs

If the chosen outcome is "allow duplicates, order-preserving", then we need to check that we are interpreting the LocalKeyId with the same firstness/lastness/value-based-ness that Windows does.

[dotnet-policy-service]

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

[blowdart]

Personally, as you'd expect, I would vote for reject, and then allow a loosening of that by the user.

[bartonjs]

@blowdart Well, which flavor of rejection? 😄

allow a loosening of that by the user

That sounds like another property on Pkcs12LoaderLimits, which we'd have to shape/name.

If we go with "reject duplicates of the attributes we understand (cert friendly name, key persisted name, key CSP, local id) on things we're importing", I don't think we'd really have need for a knob. None of Windows/macOS/OpenSSL would write things with duplicates there, so anyone who has duplicates there is obviously malicious, or a tester ("white-hat malicious").

[bartonjs]

Yet another flavor of rejection is "only reject it if an attribute we care about is duplicated". So if PreserveKeyName isn't set we wouldn't care about duplicates there, but then if you set PreserveKeyName=true we move from importing it with a random key name to failing to import it.

[vcsjones]

If I had to chose...

Reject duplicates for all importable objects

would be the winner. Let's rule out the rest.

Allow duplicates, change to an order-preserving filter.

This one is... fine... and it's probably what some other PKCS12/PFX processors do. Its behavior is dependent on matching Windows though when we use DangerousNoLimits since that bypasses our decoding. I'm generally not a fan of having duplicate information that can only be meaningfully processed once.

Allow duplicates, permitting inconsistent ordering

That's a worse variation of the first one. Nondeterministic behavior is at best, buggy and users might find edge cases in it.

Reject duplicates for items being imported

This one is close to a good idea, and I don't dislike it. I slightly prefer the next one because I don't love the idea of a PKCS12 having different validation behavior depending on IgnorePrivateKeys is set to true or false.

On the other hand, "Why did you not ignore something when I set ignore to true" is also a valid concern.

Reject duplicates for all objects

This might as well be "for all importable objects". There is not a whole lot of innovation in the PFX space, and all of the items we understand are 99% of the time what is going to be in there. I have never seen a CRL in a PFX. So maybe just an outright "no duplicates".

Reject duplicates for imported/importable/all objects, but only for the few well-known OIDs

That risks an unknown OID becoming known one day. Again, unlikely, but a thought that occurred to me, and it seems unnecessary compared to just rejecting all dupes.

I don't think we need a new API to control any of this behavior. We can add one if needed based on feedback (which would surprise me).

[bartonjs]

There's one minor caveat, "all importable objects" changes depending on whether IgnoreEncryptedAuthSafes is on or off. So "IgnorePrivateKeys" would not change whether or not something threw, but "IgnoreEncryptedAuthSafes" would (since, by virtue of not decrypting it, we don't know it contained duplicate stuff)

[blowdart]

Yea, I lean onto reject all duplicates as the default, then allow the user to flag they're acceptable. The ignore if it's not a property being used I have mixed feelings about, as duplicates would presumably indicate a bad key file, and even if I'm not touching the bad bits in that operation I, personally, would rather know it's bad, because I may go on to use those properties later - so a specific flag saying, ok, I know, shut up because it doesn't affect my current operation would seem suitable

[bartonjs]

then allow the user to flag they're acceptable

Meaning you want us to put something like public bool AllowDuplicateAttributes { get; set; } on the Pkcs12LoaderLimits type? Which mode should it be set to when calling new X509Certificate2(bytes, pwd, flags)?