Skip to content

[release/9.0] New cert loader should load into CNG by default #107060

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

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 27, 2024

Backport of #107005 to release/9.0

/cc @bartonjs

Customer Impact

  • Customer reported
  • Found internally

The new certificate loader is supposed to give private keys the best treatment it can, but it wasn't. This manifests in places like SslStream not successfully doing TLS 1.3.

Regression

  • Yes
  • No

Only impacts a new feature for 9

Testing

Tests were added.

Risk

Low, based on test coverage.

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.
@ghost ghost added the area-System.Security label Aug 27, 2024
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 requested a review from jeffhandley August 27, 2024 22:26
@jeffhandley jeffhandley requested a review from artl93 August 28, 2024 00:44
@jeffhandley
Copy link
Member

Tagging @artl93 for review (and merge) into release/9.0. This is a broken scenario in .NET 9 found internally a couple different ways, including updating more unit tests and end-to-end scenarios being updated to use the new cert loader. Once this is merged, #104487 should light up green and be unblocked.

@artl93 artl93 added the Servicing-approved Approved for servicing release label Aug 28, 2024
@artl93 artl93 merged commit d5e6369 into release/9.0 Aug 28, 2024
85 of 87 checks passed
@ericstj ericstj deleted the backport/pr-107005-to-release/9.0 branch August 29, 2024 02:44
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants