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.

Sign up for GitHub

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

Jump to bottom

Create new X509CertificateLoader #102167

Merged
merged 21 commits into from
Jun 25, 2024
Merged

Create new X509CertificateLoader #102167

merged 21 commits into from
Jun 25, 2024

Conversation

bartonjs
Copy link
Member

The new certificate loader only loads one data type per method, unlike the previous loader mechanism (new X509Certiicate2(bytes, ...)). It also allows for caller configuration to control cost-of-work limits and some common usability gotchas around Windows PFX loading.

This change adds the new loader, and changes the X509Certificate2 ctors to use it; a followup will mark the ctors as Obsolete and update usage in the dotnet/runtime codebase.

Contributes to #91763.

@bartonjs
Create new X509CertificateLoader
85d2991 
The new certificate loader only loads one data type per method, unlike
the previous loader mechanism (new X509Certiicate2(bytes, ...)).  It also
allows for caller configuration to control cost-of-work limits and some
common usability gotchas around Windows PFX loading.

This change adds the new loader, and changes the X509Certificate2 ctors
to use it; a followup will mark the ctors as Obsolete and update usage in
the dotnet/runtime codebase.
@bartonjs bartonjs added this to the 9.0.0 milestone May 13, 2024
@bartonjs bartonjs self-assigned this May 13, 2024
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

stephentoub
stephentoub reviewed May 13, 2024
View reviewed changes
src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs Outdated
set
{
if (value < 0)
throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_NeedNonNegNum);
Copy link
Member

Choose a reason for hiding this comment

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

Earlier an ifdef was used to use ObjectDisposedException.ThrowIf if it's available. I assume we're not doing so here with AOORE just because there are a bunch of them and it'd be abnoxious?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the reason, yeah; but looking at it now in this file it's just repeating a negative check, so making a helper seems reasonable (it'll get re-used)

Copy link
Member Author

Choose a reason for hiding this comment

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

How "amusing"... I updated every instance except the one this comment was on. Fixed locally now.

stephentoub
stephentoub reviewed May 13, 2024
View reviewed changes
...ies/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs
{
if (!pfxAsn.VerifyMac(password, authSafeContents))
{
password = password.ContainsNull() ? "".AsSpan() : default;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not clear on the reasoning here. If it contains null we make it non-null and if it doesn't contain null we make it null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. From the API perspective null and empty are the same thing, but from the algorithm perspective they're different. So if we got null, try again with empty, and if we got empty, try again with null.

vcsjones
vcsjones reviewed May 13, 2024
View reviewed changes
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.

What I have so far. Scrollbar says I have a ways to go still.

src/libraries/Common/src/System/IO/MemoryMappedFiles/MemoryMappedFileMemoryManager.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/System/Security/Cryptography/Helpers.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/System/Security/Cryptography/PasswordBasedEncryption.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/System/Security/Cryptography/PasswordBasedEncryption.cs Outdated Show resolved Hide resolved
src/libraries/Common/src/System/Security/Cryptography/PasswordBasedEncryption.cs Show resolved Hide resolved
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/NetStandardShims.cs Show resolved Hide resolved
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/NetStandardShims.cs Outdated Show resolved Hide resolved
...soft.Bcl.Cryptography/src/System/Security/Cryptography/PbeEncryptionAlgorithm.netstandard.cs Outdated Show resolved Hide resolved
...ies/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PbeParameters.netstandard.cs Outdated Show resolved Hide resolved
src/libraries/Microsoft.Bcl.Cryptography/tests/X509Certificates/TestData.cs Outdated Show resolved Hide resolved
This was referenced May 13, 2024
Open
Open
vcsjones
vcsjones reviewed May 14, 2024
View reviewed changes
...em.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.cs Show resolved Hide resolved
...yptography/src/System/Security/Cryptography/X509Certificates/X509Certificate.LegacyLimits.cs Outdated Show resolved Hide resolved
...yptography/src/System/Security/Cryptography/X509Certificates/X509Certificate.LegacyLimits.cs Outdated Show resolved Hide resolved
...ptography/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.OpenSsl.cs Outdated Show resolved Hide resolved
...ptography/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.OpenSsl.cs Show resolved Hide resolved
src/libraries/System.Security.Cryptography/tests/X509Certificates/CertLoaderTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Security.Cryptography/tests/X509Certificates/CertLoaderTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Security.Cryptography/tests/X509Certificates/CertLoaderTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Security.Cryptography/tests/X509Certificates/CollectionImportTests.cs Outdated Show resolved Hide resolved
src/libraries/System.Security.Cryptography/tests/X509Certificates/CtorTests.cs Outdated Show resolved Hide resolved
This was referenced Jun 7, 2024
Closed
Closed
Closed
Closed
This was referenced Jun 19, 2024
Open
Open
@vcsjones
Copy link
Member

/azp run runtime-androidemulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bartonjs
Copy link
Member Author

/azp run runtime-androidemulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jun 24, 2024
Open
3 tasks
@bartonjs
Copy link
Member Author

/azp run runtime-androidemulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jun 25, 2024
Open
Closed
@vcsjones
Copy link
Member

/azp run runtime-ioslikesimulator

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bartonjs bartonjs merged commit d4fe4a7 into dotnet:main Jun 25, 2024
86 of 95 checks passed
@bartonjs bartonjs deleted the x509loader branch June 25, 2024 22:10
@pavelsavara
Copy link
Member

pavelsavara commented Jun 26, 2024 •
edited
Loading

this is preventing code flow into azdo internal repo because the security scanner detects credentials.

 ! [remote rejected]         main -> main (VS403654: The push was rejected because it contains one or more secrets.

Resolve the following secrets before pushing again. For help, see https://aka.ms/1ESSecretScanning.

Secrets:

commit: d4fe4a7e987584bfa4904dd1027a873029b2005d
paths:
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2710,1-26) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2723,1-26) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2745,1-29) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2752,1-30) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2802,1-26) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2881,1-30) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(2964,1-26) : SEC101/013 : PemPrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(57,149-13) : SEC101/055 : Pkcs12PrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(1476,41-55) : SEC101/055 : Pkcs12PrivateKey
/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs(3443,13-26) : SEC101/055 : Pkcs12PrivateKey)

See also #104021

@adamsitnik adamsitnik mentioned this pull request Jun 26, 2024
Closed
This was referenced Jun 26, 2024
Closed
Closed
@ellahathaway ellahathaway mentioned this pull request Jul 8, 2024
Merged
@ViktorHofer ViktorHofer mentioned this pull request Jul 15, 2024
Closed
@matouskozak matouskozak mentioned this pull request Jul 23, 2024
Open
@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2024
@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Aug 15, 2024
@bartonjs bartonjs added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Sep 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 12, 2024
@bartonjs
Copy link
Member Author

Breaking change docs written. dotnet/docs#42613

@bartonjs bartonjs removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 12, 2024
@bartonjs bartonjs added the tracking This issue is tracking the completion of other related issues. label Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@vcsjones vcsjones vcsjones approved these changes

Assignees

@bartonjs bartonjs

Labels
area-System.Security breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. new-api-needs-documentation tracking This issue is tracking the completion of other related issues.
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bartonjs @vcsjones @pavelsavara @stephentoub