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

New cert loader should load into CNG by default #107005

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

bartonjs
Copy link
Member

When no provider attribute is present on a key, Windows loads the key into the CAPI Base provider unless PKCS12_PREFER_CNG_KSP is set. So, set that flag.

On .NET Framework (or .NET Standard running on .NET Framework) we don't have the power to set that flag (without completely redefining how the PFX load loads), so inject a synthetic attribute to force keys into the CNG KSP when PreserveStorageProvider isn't set.

Technically these two approaches differ when the incoming PFX has no name and PreserveStorageProvider is set (CoreFX: CNG, NetFX: CAPI Base), but that's unlikely, and consistent with .NET Framework imports.

Fixes the problem identified in #104487.

When no provider attribute is present on a key, Windows loads the key into the
CAPI Base provider unless PKCS12_PREFER_CNG_KSP is set.  So, set that flag.

On .NET Framework (or .NET Standard running on .NET Framework) we don't
have the power to set that flag (without completely redefining how the PFX load
loads), so inject a synthetic attribute to force keys into the CNG KSP when
PreserveStorageProvider isn't set.

Technically these two approaches differ when the incoming PFX has no name
and PreserveStorageProvider is set (CoreFX: CNG, NetFX: CAPI Base), but that's
unlikely, and consistent with .NET Framework imports.
Copy link
Contributor

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

@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Aug 27, 2024
@teo-tsirpanis
Copy link
Contributor

BTW I've heard that the NCrypt APIs perform an RPC call to a system process and I was wondering if the same happens with a key obtained from PFXImportCertStore and if yes, whether passing PKCS12_NO_PERSIST_KEY and/or using the earlier Crypto API to get the certificate's private key would change anything.

Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Some non-blocking feedback for clarity in a test, but functionally looks good.

@vcsjones
Copy link
Member

vcsjones commented Aug 27, 2024

I've heard that the NCrypt APIs perform an RPC call to a system process and I was wondering if the same happens with a key obtained from PFXImportCertStore

Yes it does.

whether passing PKCS12_NO_PERSIST_KEY and/or using the earlier Crypto API to get the certificate's private key would change anything.

It does. If that is the behavior people want, they can specify X509KeyStorageFlags.EphemeralKeySet when loading the PKCS12. That gets mapped to PKCS12_NO_PERSIST_KEY (among other things).

Hedging against likely follow up question..

Why is that not the default?

I think the idea was entertained when X509CertificateLoader` was being spiked out, but it can't work for two reasons.

  1. macOS does not support ephemeral key loads. X509KeyStorageFlags.EphemeralKeySet throws on that platform, and it doesn't make sense for the API to do ephemeral on one OS, and persisted on another, as a default behavior.

  2. It changes the behavior of certain Windows APIs. For example, an X509Certificate2 that is used by SslStream must have a persisted key. That's a Win32 thing.

So, if you know your scenarios work with ephemeral keys, you can opt-in to it.

@bartonjs
Copy link
Member Author

/ba-g The tests appear to have run successfully on all legs (infrastructure issue preventing them from uploading results?)

@bartonjs bartonjs merged commit 5d3bac1 into dotnet:main Aug 27, 2024
80 of 84 checks passed
@bartonjs bartonjs deleted the correct_ksp branch August 27, 2024 21:49
@bartonjs
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10586588721

@ergunr
Copy link

ergunr commented Aug 28, 2024 via email

@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
@bartonjs bartonjs added this to the 9.0.0 milestone Oct 23, 2024
@bartonjs bartonjs added the tracking This issue is tracking the completion of other related issues. label Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. tracking This issue is tracking the completion of other related issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants