Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use a custom PFX reader/writer on Unix OSes - #42226

Merged
bartonjs merged 65 commits into
dotnet:masterfrom
bartonjs:custom_pfx_loader_rb
Nov 8, 2019
Merged

Use a custom PFX reader/writer on Unix OSes#42226
bartonjs merged 65 commits into
dotnet:masterfrom
bartonjs:custom_pfx_loader_rb

Conversation

@bartonjs

@bartonjs bartonjs commented Oct 30, 2019

Copy link
Copy Markdown
Member

This change moves PFX import and export primarily into managed code to work around inconsistencies across the operating systems.

Current issues:

  • Linux
    • Reading
      • PKCS12_parse doesn't support multiple cert-with-keys.
      • PKCS12_parse doesn't support reading a PFX with no MAC.
      • OpenSSL 1.0 had a weird bug where an ECDSA cert inexplicably didn't match to its key.
    • Writing
      • PKCS12_create doesn't support multiple cert-with-keys.
      • PKCS12_create doesn't support writing empty collections.
  • macOS
    • Reading
      • Either SecItemImport does not understand the NULL (vs Empty) password, or we called it wrong... cannot load a PFX which is MACd with the NULL password.
      • SecItemImport can only support "normalized" PFXes, where "normalized" means "how Windows XP would have written it":
        • PFX
          • SafeContents0 (no encryption) (won't load keys from an encrypted SafeContents, IIRC)
            • ShroudedKey0 (won't load keys from KeyBag (unencrypted), only ShroudedKeyBag (encrypted))
            • ...
            • ShroudedKeyN
          • SafeContents1 (encrypted) (won't load certs from an unencrypted SafeContents, IIRC)
            • Cert0
            • ...
            • CertM
        • MAC
          • AlgId: HMAC-SHA-1 (IIRC this was a requirement, but it's also the only allowed algorithm on Win7 or Win8.1...)
    • Writing
      • SecItemExport fails to create a PFX with only public keys (or, at least, with non-keychain-based certificates).
      • SecItemExport fails to create a PFX where some elements are in different keychains than others (including "some elements are not in a keychain").

This change moves the necessary ASN types from the Pkcs12 library into Common so they're shared between Pkcs12Info/Pkcs12Builder and X509Certificates, then uses a managed loader and managed writer.

Quirks:

  • SecItemImport(PKCS8) doesn't support marking keys as non-exportable, so non-exportable keyloads on macOS read a PFX, write a normalized PFX in memory, then call SecItemImport(PKCS12).
    • Because one of the failure modes of SecItemImport(PKCS12) is that it returns certs without private keys associated, it's not possible to call SecItemImport first and fall back to the managed loader.
  • Windows and Linux both will happily return the wrong private key with a cert if the PFX says to do so, but on macOS the SecIdentityRef creation fails and the cert comes back with no private key.
    • This isn't a very realistic situation outside of our tests, so it's not something worth doing heroics for right now. The easiest answer is to make HasPrivateKey be true but the GetPrivateKey methods throw... but that's still different than the other platforms, and would be very weird with SslStream.

As part of this change the X509Certificates SafePasswordHandle is now always UTF-16, because that's the encoding required for the PKCS12 KDF, so some platform-split code got to go away. Some helper routines for CFString were added to make the transition back to macOS P/Invokes easier from the SafePasswordHandle.

Fixes #40539.
Fixes #2743.
Fixes #2745.
Fixes #2746.
Fixes #11046.
Fixes #16705.
Fixes #24225.
Fixes #24226.
Fixes #26397.
Fixes #30946.
Fixes #31746.
Fixes #37599.
(And maybe others)

Have some todos, and Windows won't compile...
@maryamariyan

Copy link
Copy Markdown

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your corefx repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

@bartonjs

bartonjs commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

All of the Nano 1903 test failures are that it took the s_loaderFailsKeysEarly codepath, but the loader didn't fail early.

        private static readonly bool s_loaderFailsKeysEarly =
            RuntimeInformation.IsOSPlatform(OSPlatform.Windows) &&
            !PlatformDetection.IsWindows10Version1903OrGreater;

The build number used for detecting 1903 matches what I see in docs (18362) and works for non-Nano. And PlatformDetection's IsWindowsNanoServer starts off with the IsOSPlatform(Windows) check.

Weird.

Comment thread src/Common/src/Interop/OSX/Interop.CoreFoundation.CFData.cs
Comment thread src/Common/src/Interop/OSX/Interop.CoreFoundation.cs
Comment thread src/Common/src/System/Security/Cryptography/CryptoPool.cs
@bartonjs

bartonjs commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

K, this time OneCert_EncryptedEmptyPassword_OneKey_EncryptedNullPassword_NoMac hit a one in a million (okay, less unlikely than that...) chance where the padding worked out so decryption "succeeded" but ASN parsing failed. Added a second acceptable failure code to that one... if there are more sneaky one in a millions then I'll call it "overtesting" instead of "test case generation validation" and remove testing for the Win32 codes at all.

As for the Nano failures, apparently I can't read. It's 1809, not 1903. Since it has the "1903" behavior, but is 1809, my test was wrong... so I'm changing it to RS5/1809; and hopefully the full run doesn't say that Windows Client 1809 disagrees. (I forget where I came up with 1903 as the cutover, but I remember feeling like it was a guess when I did it)

@bartonjs

bartonjs commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

Yay, finally got all the edge cases on all the OSes at the same time! 🍾

}

return certs;
return ((ECDiffieHellmanOpenSsl)key).DuplicateKeyHandle();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: for what it's worth, you can simplify these kinds of patterns with switch expressions:

internal static SafeEvpPKeyHandle GetPrivateKey(AsymmetricAlgorithm key) =>
    key switch
    {
        RSAOpenSsl rsa => rsa.DuplicateKeyHandle(),
        DSAOpenSsl dsa => dsa.DuplicateKeyHandle(),
        _ => ((ECDiffieHellmanOpenSsl)key).DuplicateKeyHandle()
    };

@bartonjs

bartonjs commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

I can't tell if either @krwq or @stephentoub have made it to the end of the PR while reviewing, so I'm still waiting on a checkmark.

I'd like to control the squash message, so please don't hit squash and merge as signoff.

@bartonjs
bartonjs merged commit 1338e4e into dotnet:master Nov 8, 2019
@danmoseley

Copy link
Copy Markdown

Yay @bartonjs, that was a big change.

@krwq

krwq commented Nov 8, 2019

Copy link
Copy Markdown
Member

Yes, I've made it, new iterations LGTM as well

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.