-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Decide on a policy for duplicate attributes on PKCS12 values #103924
Comments
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
Personally, as you'd expect, I would vote for reject, and then allow a loosening of that by the user. |
@blowdart Well, which flavor of rejection? 😄
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"). |
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. |
If I had to chose...
would be the winner. Let's rule out the rest.
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.
That's a worse variation of the first one. Nondeterministic behavior is at best, buggy and users might find edge cases in it.
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 On the other hand, "Why did you not ignore something when I set ignore to true" is also a valid concern.
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".
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). |
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) |
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 |
Meaning you want us to put something like |
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:
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.
The text was updated successfully, but these errors were encountered: