fix 2 warnings in CodeAnalysis #3388

heng-liu · 2020-05-12T03:50:20Z

Bug

Fixes: NuGet/Home#9546
Regression: No

Last working version:
How are we preventing it in future:

Fix

Details: fix 2 warnings raised by CodeAnalysis.FxCopAnalyzer

Testing/Validation

Tests Added: No
Reason for not adding tests:
Validation:

zivkan · 2020-05-12T21:32:15Z

src/NuGet.Core/NuGet.Packaging/Signing/Cms/ManagedCmsWrapper.cs

+                using (var certificate = new X509Certificate2(cert))
+                {
+                    _signedCms.AddCertificate(certificate);
+                }


By putting this is a using block, it means the certificate that was added to _signedCms is now disposed. Are you sure that's ok?

Thanks for reviewing!
Looks like the AddCertificate method will create a copy of the certificate in following code:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignedCms.cs#L639-L671

~~Consider adding a comment saying that AddCertificates copies the cert.~~

As you pointed out, there's no point making a new cert.

So it should be ok not new any certificate, just use the one from parameter.
I'll fix it. Thanks!

zivkan · 2020-05-12T22:16:47Z

src/NuGet.Core/NuGet.Packaging/Signing/Cms/ManagedCmsWrapper.cs

+                using (var certificate = new X509Certificate2(cert))
+                {
+                    _signedCms.AddCertificate(certificate);
+                }


~~Consider adding a comment saying that AddCertificates copies the cert.~~

As you pointed out, there's no point making a new cert.

…ild" This reverts commit 99d99a0.

* fix 2 warnings in CodeAnalysis

* Signing: Support signature verification on .NET Core Remove 2nd N.B.T.P package (#2984) remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable (#3161) * remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable version, fix error; change LockSDKVersion default value from empty string to false; apply Linux workaround in my change Retarget to netcore5.0 (#3162) * retarget to netcore5.0; Linux script workaround need to apply for downloading SDK for build add pkcs and cng packages Temporary fix on patching SDK, add System.Security.Cryptography.Pkcs.dll and update deps.json, use dll from .nuget/packages Address PR comments implement add cert to store on Mac; display openssl version (#3166) Implement timestamp integrity verification; Implement an equivalent NativeCms; enable signing APIs and tests (#3168) * implement timestamp integrity verification; implement ManagedCms; enable signing APIs and tests fix xplat verification branch after rebase (#3191) Fixes: NuGet/Home#9012 check different cross-platform error messages for self-signed cert test (#3208) Fixes: NuGet/Home#8933 Xplat Signing/Verification : Fix broken tests for setting privatekey of a X509Certificate2 (#3195) use workaround to set private key for X509Certificate2 (netcore), change ToRSA to workaround for netcore(especially for non-windows) enable PathTooLongException test (#3216) Fixes: NuGet/Home#8920 enable TrustedSignerActionsProvider on netcore, along with tests (#3213) Fixes: NuGet/Home#8921 Enable SignedPackageIntegrityVerificationTests for netcore5.0 (#3220) Enable SignedPackageIntegrityVerificationTests for netcore, remove extra preprocessors Enable timestamp provider test and fix 3 broken tests (#3210) enable all unit tests; enable 8 more tests; enable timestampProviderTest; fix 3 broken tests in TimestampProviderTests Fix 4 tests in SignatureTrustAndValidityVerificationProviderTests (#3242) fix untrusted msg for different platforms fixed broken tests using utility methods (#3263) disable outdated tests (#3274) enable 4 tests on Mac (#3275) fix 5 broken tests in SignatureTrustAndValidityVerificationProviderTests, add workaround for Linux (#3252) fix broken test case ExecuteCommandAsync_WithAmbiguousMatch_ThrowsAsync (#3291) * Enable test ExecuteCommandAsync_WithMultiplePackagesAndInvalidCertificate_RaisesErrorsOnceAsync (#3342) * enable a test, change the comments; extend test limitation on Linux * Updated SDK version and removed the hardcoded version when reading from JSON file (#3372) * update Pkcs package to the preview3 version (#3382) * fix 2 warnings in CodeAnalysis (#3388) * fix 2 warnings in CodeAnalysis * disable dynamic revoke tests on Mac (#3373) * fix 1 test (#3371) * Fix more warnings reported by CodeAnalysis (#3393) * fix string warnings raised by CodeAnalysis * Retarget xplat verification to netcoreapp5.0 (#3386) * retarget netcoreapp5.0 (a workaround for net5.0) * fix all the rest warnings raised by code analysis (#3401) * Suppress CA1307 warning for string.GetHashCode() method (#3404) * Linux test failure due to generic exception (#3405) * add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects (#3419) * add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects * add PIN prompt for netcore (#3389) * Run dotnet nuget tests on multiple TFMs (#3387) * enabled mac assertions * Revert "enabled mac assertions" This reverts commit d8a52e2. * Enable test GetCertificateChain_WithUntrustedRoot_Throws on Mac (#3428) * run signing integration tests cross plat (#3415) * added GetNonce() to IRfc3161TimestampRequest interface (#3427) * disable test for netcore (#3445) * log Status and StatusInformation, fix 2 tests (#3268) * Disable verification during extraction for Mono, disable a test on Mono (#3451) * disable verification during extraction on mono,disable a mono test on Mac * adapt 2 newly added dotnet integration tests for multiple TFMs (#3460) * remove useless Sdk2x.targets * get TimestampTests working again (#3218) * get TimestampTests working again Fixes: NuGet/Home#8935 * change GetTimestampAsync into internal * fix runFuncTests.sh and InstallCLIforBuild.sh * address feedback * upgrade dotnet 5 to preview4 as preview5 is broken * Fix exception thrown in SigningTestFixture during disposing (#3464) Co-authored-by: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com> Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com> Co-authored-by: Kat Marchán <kzm@zkat.tech>

fix 2 warnings in CodeAnalysis

46b2bde

heng-liu requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners May 12, 2020 03:50

zivkan reviewed May 12, 2020

View reviewed changes

zivkan approved these changes May 12, 2020

View reviewed changes

heng-liu added 3 commits May 12, 2020 15:18

fix AddCertificate

36f5372

temporarily set TreatWarningsAsErrors to false to pass the build

99d99a0

Revert "temporarily set TreatWarningsAsErrors to false to pass the bu…

f47238b

…ild" This reverts commit 99d99a0.

heng-liu merged commit f5cab2a into dev-feature-xplatVerification May 13, 2020

heng-liu deleted the dev-hengliu-xplat-fixWarnings branch May 13, 2020 04:05

heng-liu added a commit that referenced this pull request May 20, 2020

fix 2 warnings in CodeAnalysis (#3388)

637f5b4

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request May 26, 2020

fix 2 warnings in CodeAnalysis (#3388)

8e6910f

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 12, 2020

fix 2 warnings in CodeAnalysis (#3388)

2081500

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 16, 2020

fix 2 warnings in CodeAnalysis (#3388)

aa49e4d

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 17, 2020

fix 2 warnings in CodeAnalysis (#3388)

0a6f483

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 19, 2020

fix 2 warnings in CodeAnalysis (#3388)

cea5460

* fix 2 warnings in CodeAnalysis

heng-liu added a commit that referenced this pull request Jun 19, 2020

fix 2 warnings in CodeAnalysis (#3388)

4938494

* fix 2 warnings in CodeAnalysis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix 2 warnings in CodeAnalysis #3388

fix 2 warnings in CodeAnalysis #3388

heng-liu commented May 12, 2020

zivkan May 12, 2020

heng-liu May 12, 2020 •

edited

Loading

zivkan May 12, 2020 •

edited

Loading

heng-liu May 12, 2020

zivkan May 12, 2020 •

edited

Loading

fix 2 warnings in CodeAnalysis #3388

fix 2 warnings in CodeAnalysis #3388

Conversation

heng-liu commented May 12, 2020

Bug

Fix

Testing/Validation

zivkan May 12, 2020

Choose a reason for hiding this comment

heng-liu May 12, 2020 • edited Loading

Choose a reason for hiding this comment

zivkan May 12, 2020 • edited Loading

Choose a reason for hiding this comment

heng-liu May 12, 2020

Choose a reason for hiding this comment

zivkan May 12, 2020 • edited Loading

Choose a reason for hiding this comment

heng-liu May 12, 2020 •

edited

Loading

zivkan May 12, 2020 •

edited

Loading

zivkan May 12, 2020 •

edited

Loading