Skip to content

Conversation

@zivkan
Copy link
Member

@zivkan zivkan commented Jul 3, 2023

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2372

Regression? no

Description

  • System.Security.Cryptography.Pkcs has a known vulnerability, and needs to be upgraded. Or at least, it's easier to upgrade than to figure out if we're using the package in a way that's vulnerable or not.
  • System.Security.Cryptography.Cng hasn't been getting newer versions since 5.0.0, but is listed as a dependency of System.Security.Cryptography.Pkcs, and we only used .Cng where were also used .Pkcs, so I removed the .Cng` references
  • Although CryptographyPackagesVersion is now only used by 1 PackageVersion (Pkcs), I didn't rename the property, because I assume that source-build uses it to override the version
  • The newer version of .Pkcs (or its dependencies) now bring System.Buffer into the restore graph for NuGet.Packaging's netstandard2.0 build, so the internal copy of ArrayPool was causing compiler errors. Hence, stop using it in netstandard2.0, since we can reference it from the .NET library.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@zivkan zivkan requested a review from a team as a code owner July 3, 2023 10:25
@nkolev92
Copy link
Member

nkolev92 commented Jul 3, 2023

cc @NikolaMilosavljevic @MichaelSimons can confirm what this change does for source build.

We might be able to change the property name

@MichaelSimons
Copy link
Contributor

cc @NikolaMilosavljevic @MichaelSimons can confirm what this change does for source build.

We might be able to change the property name

I think this upgrade is going to cause a prebuilt in source-build. System.Security.Cryptography.Pkcs 6.0.4 will need to be added to https://github.com/dotnet/source-build-reference-packages. Instructions for adding new packages are here.

@zivkan
Copy link
Member Author

zivkan commented Jul 6, 2023

That doesn't address the question about the MSBuild property name used to override the package version in this repo, but ok: dotnet/source-build-reference-packages#738

However, after creating the PR, I realize that the version of the package we're currently using, 5.0.0, isn't in the repo at the moment anyway 🤷

@zivkan zivkan merged commit 622b83b into dev Jul 6, 2023
@zivkan zivkan deleted the dev-zivkan-upgrade-pkcs-package branch July 6, 2023 08:34
@MichaelSimons
Copy link
Contributor

That doesn't address the question about the MSBuild property name used to override the package version in this repo

The property name doesn't matter here because source-build is able to utilize a reference package. The property name becomes important if the package reference has to be lifted during source-build to the latest. In that case, it has to follow a naming convention.

However, after creating the PR, I realize that the version of the package we're currently using, 5.0.0, isn't in the repo at the moment anyway 🤷

Isn't it here: https://github.com/dotnet/source-build-reference-packages/tree/main/src/referencePackages/src/system.security.cryptography.pkcs/5.0.0?

@zivkan
Copy link
Member Author

zivkan commented Jul 6, 2023

Isn't it here

Yes, yes it is.

I have no idea what was wrong with me this morning. Trying to rush to finish too many things I have time for before the end of the day. 🤦‍♂️

aortiz-msft pushed a commit that referenced this pull request Jul 25, 2023
…#5332)

* Address component government alerts (#5308)

* Override vulnerable transitive dependency to fix GC alerts

* Update System.Security.Cryptography.Pkcs version (#5289)

---------

Co-authored-by: Erick Yondon <[email protected]>
Co-authored-by: Andy Zivkovic <[email protected]>
martinrrm added a commit that referenced this pull request Aug 14, 2023
…#5332)

* Address component government alerts (#5308)

* Override vulnerable transitive dependency to fix GC alerts

* Update System.Security.Cryptography.Pkcs version (#5289)

---------

Co-authored-by: Erick Yondon <[email protected]>
Co-authored-by: Andy Zivkovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants