Enable cross-platform package signature verification for restore #3458

heng-liu · 2020-06-17T01:39:32Z

Bug

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

Fix

Details:
1.Target NuGet.Packaging and projects depend on it to net5.0
2.Enable/implement cross-platform(net5.0 code path) package signing APIs (used in verification tests)
3.Enable/Implement cross-platform(net5.0 code path) package verification APIs.
4.Enable/fix related tests.

Testing/Validation

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

zivkan

There's a breaking API change that concerns me, otherwise it's just little things which could be improved in other PRs.

zivkan · 2020-06-17T02:03:18Z

build/common.targets

+    <SigningNotSupported Condition=" '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'netcoreapp2.1' OR '$(TargetFramework)' == 'netstandard2.1' OR '$(TargetFramework)' == 'netcoreapp3.1'">true</SigningNotSupported>
+    <SigningNotSupported Condition=" '$(SigningNotSupported)' != 'true'">false</SigningNotSupported>


Can be done in another PR, but we should avoid nous with negative names, particularly when they represent a boolean type, as the double negative when the value is false can be confusing.

Also comparing against a list of TFMs is hard to read and maintain. We can use a condition something like '$(TargetFrameworkIdentifier)' == '.NETFramework' or ( '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and [MSBuild]::VersionGreaterThanOrEquals('$(TargetFrameworkVersion)', '5.0') )

Finally, this SigningNotSupported property appears to be used only to change the DefineConstants property below. Just use the condition on DefineConstant's PropertyGroup. We don't need SigningNotSupported.

We should create a follow up to clean it up.

I tried the following, but the build failed, as TargetFrameworkIdentifier and TargetFrameworkVersion are empty.

<PropertyGroup Condition=" '$(IsDesktop)' == 'true' or ( '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals('$(TargetFrameworkVersion)', '5.0')) ) "> <DefineConstants>$(DefineConstants);IS_SIGNING_SUPPORTED</DefineConstants> </PropertyGroup>

I then tested two projects with TargetFrameworks and TargetFramework, looks like the later one got TargetFrameworkIdentifier and TargetFrameworkVersion correctly, the former one didn't.

scripts/funcTests/runFuncTests.sh

zivkan · 2020-06-17T03:11:13Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/IRfc3161TimestampTokenInfo.cs

+{
+    internal interface IRfc3161TimestampTokenInfo
+    {
+#if IS_SIGNING_SUPPORTED


very minor considering the type is internal, but why not put the #if outside the interface definition, so the interface only exists on TFMs that support signing?

Thanks. Will fix in #3571

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampProvider.cs

zivkan · 2020-06-17T03:20:47Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampRequestNetstandard21Wrapper.cs

+#if IS_SIGNING_SUPPORTED && IS_CORECLR
+using System.Net.Http;
+using System.Net.Http.Headers;
+#endif


The class defined in this function is wrapped in the same #if #endif block. Why are only these using statements excluded when the entire class hidden? Just have a single #if on line 3/4.

Thanks. Will fix in #3571

zivkan · 2020-06-17T03:29:12Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/Dotnet.Integration.Test.csproj

    <TestProject>true</TestProject>
    <Description>Integration tests for NuGet-powered dotnet CLI commands such as pack/restore/list package and dotnet nuget.</Description>
  </PropertyGroup>
-
+  


nitpick: trailing whitespace goes against coding guideline 23, and our .editorconfig file also instructs compatible editors to remove trailing whitespace, meaning someone is using an editor that does not respect .editorconfig.

Thanks. Will fix in #3571

zivkan · 2020-06-17T03:50:59Z

build/common.project.props

-    <TargetFrameworksLibrary>$(NETFXTargetFramework);$(NetStandardVersion)</TargetFrameworksLibrary>
+    <TargetFrameworksExe>$(NETFXTargetFramework);$(NETCoreTargetFrameworks)</TargetFrameworksExe>
+    <TargetFrameworksLibrary  Condition=" '$(RequiresSigningXplatAPIs)' != 'true' ">$(NETFXTargetFramework);$(NetStandardVersion)</TargetFrameworksLibrary>
+    <TargetFrameworksLibrary  Condition=" '$(RequiresSigningXplatAPIs)' == 'true' ">$(NETFXTargetFramework);$(NetStandardVersion);netcoreapp5.0</TargetFrameworksLibrary>


I think I've talked to you about when I worked on the dotnet.integration.tests multi-tfm testing: I'm not a fan of defining RequiresSigningXplatAPIs in the project, importing this props file, then using TargetFrameworksLibrary in the csproj's TargetFrameworks element.

In my opnion, either:

set properties in the project, import this file, and this file sets the TargetFrameworks

or

import this file, which defines a bunch of different properties, and the project file uses the one it likes in its own definition of TargetFrameworks.

I prefer the second option, which is how NuGet.Client worked before the dev-feature-xplatVerification branch, but this current approach where some of the TFM logic is in the project file and other TFM logic is in the common project props file, it makes it harder to understand.

However, it's something we can fix after this branch is merged into dev.

Thanks. Will fix in #3571

nkolev92

A few nits.

It looks great beyond that.

A product of lots of work over the last year.

Great job @heng-liu , @zkat & @kartheekp-ms.

Some kudos go to @zivkan too I guess.

build/build.proj

nkolev92 · 2020-06-18T21:59:37Z

build/common.targets

+    <SigningNotSupported Condition=" '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'netcoreapp2.1' OR '$(TargetFramework)' == 'netstandard2.1' OR '$(TargetFramework)' == 'netcoreapp3.1'">true</SigningNotSupported>
+    <SigningNotSupported Condition=" '$(SigningNotSupported)' != 'true'">false</SigningNotSupported>


We should create a follow up to clean it up.

build/config.props

global.json

nkolev92 · 2020-06-18T22:11:59Z

src/NuGet.Core/NuGet.Commands/NuGet.Commands.csproj

@@ -7,7 +11,7 @@
    <TargetFrameworks>$(TargetFrameworksLibrary)</TargetFrameworks>
    <TargetFramework />
    <NoWarn>$(NoWarn);CS1591;CS1574;CS1573;CS1584;CS1658</NoWarn>
-    <NoWarn Condition="'$(TargetFramework)' == '$(NetStandardVersion)'">$(NoWarn);CS1998</NoWarn>
+    <NoWarn Condition=" $(TargetFramework.StartsWith('netstandard')) OR $(TargetFramework.StartsWith('netcoreapp')) ">$(NoWarn);CS1998</NoWarn>


Does IsCore not work here?

IsCore doesn't work.
Properties are evaluated in the order in which they appear in the project file.
The assignment of property IsCore is in common.targets.
And the NuGet.commands.csproj imports the common.targets at the end, that's after the NoWarn.

We can probably move IsCore to common.props...I can look into that clean-up.

nkolev92 · 2020-06-18T22:20:50Z

...uGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampTokenInfoNetstandard21Wrapper.cs

+namespace NuGet.Packaging.Signing
+{
+#if IS_SIGNING_SUPPORTED && IS_CORECLR
+    internal sealed class Rfc3161TimestampTokenInfoNetstandard21Wrapper : IRfc3161TimestampTokenInfo


Do we need this? We are only using net5 aren't we?

Or maybe we do need and should name it so something better :)

Versions change :)
Future Pr would be ok.

I think the logic of #if IS_SIGNING_SUPPORTED && IS_CORECLR is more clear than #if NET5_0.
We can tell the first one, is related to signing, and when it's the netcore code path.
The second one may be related to signing, or may be related to net5.0 API change unrelated to signing.

nkolev92

I'd wait for @zivkan's sign off before merging (well that and a green build)

heng-liu · 2020-06-19T01:50:19Z

A few nits.

It looks great beyond that.

A product of lots of work over the last year.

Great job @heng-liu , @zkat & @kartheekp-ms.

Some kudos go to @zivkan too I guess.

Yes, @zivkan helps a lot :)
And we also would like to thank for @dtivel
You and @rrelyea also helped a lot when targeting to netstandard2.1 ;)

nkolev92 · 2020-06-19T18:49:41Z

test/NuGet.Core.Tests/NuGet.Frameworks.Test/NuGet.Frameworks.Test.csproj

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), 'README.md'))\build\common.test.props" />
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

  <PropertyGroup>
    <TargetFrameworks>$(TargetFrameworksExe)</TargetFrameworks>
-    <TargetFrameworks Condition=" '$(IsXPlat)' == 'true' ">$(NETCoreTargetFramework)</TargetFrameworks>
+    <TargetFrameworks Condition=" '$(IsXPlat)' == 'true' ">$(NETCoreTargetFrameworks)</TargetFrameworks>


I don't think we need to test for 2 different frameworks in a project like NuGet.Frameworks.

There is no signing code in here.

Same probably apllies to a bunch of the other projects.

Thanks. Will fix in #3571

nkolev92 · 2020-06-19T18:50:42Z

test/NuGet.Core.Tests/NuGet.LibraryModel.Tests/NuGet.LibraryModel.Tests.csproj

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), 'README.md'))\build\common.test.props" />
  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

  <PropertyGroup>
    <TargetFrameworks>$(TargetFrameworksExe)</TargetFrameworks>
-    <TargetFrameworks Condition=" '$(IsXPlat)' == 'true' ">$(NETCoreTargetFramework)</TargetFrameworks>
+    <TargetFrameworks Condition=" '$(IsXPlat)' == 'true' ">$(NETCoreTargetFrameworks)</TargetFrameworks>


Likely requiresSigningXplatAPIs is not needed here either.

Thanks. Will fix in #3571

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)

…cate_RaisesErrorsOnceAsync (#3342) * enable a test, change the comments; extend test limitation on Linux

…om JSON file (#3372)

* fix 2 warnings in CodeAnalysis

* fix string warnings raised by CodeAnalysis

* retarget netcoreapp5.0 (a workaround for net5.0)

* add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects

This reverts commit d8a52e2.

…no (#3451) * disable verification during extraction on mono,disable a mono test on Mac

* get TimestampTests working again Fixes: NuGet/Home#8935

zivkan

There are several things I would love to have cleaned up, but we can do that after this is merged in dev.

heng-liu · 2020-06-19T23:05:12Z

Thanks.
Yes, there're some comments I haven't addressed. Will do that after this merged in dev.

heng-liu requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners June 17, 2020 01:39

zivkan reviewed Jun 17, 2020

View reviewed changes

nkolev92 reviewed Jun 18, 2020

View reviewed changes

nkolev92 approved these changes Jun 18, 2020

View reviewed changes

nkolev92 reviewed Jun 19, 2020

View reviewed changes

heng-liu and others added 14 commits June 19, 2020 12:45

Enable test ExecuteCommandAsync_WithMultiplePackagesAndInvalidCertifi…

affa633

…cate_RaisesErrorsOnceAsync (#3342) * enable a test, change the comments; extend test limitation on Linux

Updated SDK version and removed the hardcoded version when reading fr…

2d8566f

…om JSON file (#3372)

update Pkcs package to the preview3 version (#3382)

66bd5a2

fix 2 warnings in CodeAnalysis (#3388)

4938494

* fix 2 warnings in CodeAnalysis

disable dynamic revoke tests on Mac (#3373)

7068ae9

fix 1 test (#3371)

fe279ff

Fix more warnings reported by CodeAnalysis (#3393)

27cb541

* fix string warnings raised by CodeAnalysis

Retarget xplat verification to netcoreapp5.0 (#3386)

4283f4b

* retarget netcoreapp5.0 (a workaround for net5.0)

fix all the rest warnings raised by code analysis (#3401)

2d6b9ba

Suppress CA1307 warning for string.GetHashCode() method (#3404)

3ec1aa5

Linux test failure due to generic exception (#3405)

3713243

add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects (#3419)

43182cd

* add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects

add PIN prompt for netcore (#3389)

00c8195

zivkan and others added 16 commits June 19, 2020 12:45

Run dotnet nuget tests on multiple TFMs (#3387)

59795bb

enabled mac assertions

37ea829

Revert "enabled mac assertions"

c94757f

This reverts commit d8a52e2.

Enable test GetCertificateChain_WithUntrustedRoot_Throws on Mac (#3428)

e38eb3d

run signing integration tests cross plat (#3415)

51c1391

added GetNonce() to IRfc3161TimestampRequest interface (#3427)

6946542

disable test for netcore (#3445)

56763e2

log Status and StatusInformation, fix 2 tests (#3268)

279296e

Disable verification during extraction for Mono, disable a test on Mo…

92999ab

…no (#3451) * disable verification during extraction on mono,disable a mono test on Mac

adapt 2 newly added dotnet integration tests for multiple TFMs (#3460)

0f84aee

get TimestampTests working again (#3218)

b1342aa

* get TimestampTests working again Fixes: NuGet/Home#8935

remove useless Sdk2x.targets

ae1e51d

change GetTimestampAsync into internal

3ad50c4

fix runFuncTests.sh and InstallCLIforBuild.sh

03a2479

address feedback

212ac0a

upgrade dotnet 5 to preview4 as preview5 is broken

97f4b70

heng-liu force-pushed the dev-feature-xplatVerification branch from 5f89ade to 97f4b70 Compare June 19, 2020 19:46

Fix exception thrown in SigningTestFixture during disposing (#3464)

4e33b3b

zivkan approved these changes Jun 19, 2020

View reviewed changes

aortiz-msft merged commit 784274c into dev Jun 20, 2020

aortiz-msft deleted the dev-feature-xplatVerification branch June 20, 2020 19:23

campersau mentioned this pull request Jun 28, 2020

Package signing with timestamp server does not work on .NET 5.0 NuGet/Home#9725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable cross-platform package signature verification for restore #3458

Enable cross-platform package signature verification for restore #3458

heng-liu commented Jun 17, 2020

zivkan left a comment

zivkan Jun 17, 2020

nkolev92 Jun 18, 2020

heng-liu Aug 10, 2020 •

edited

Loading

zivkan Jun 17, 2020

heng-liu Aug 10, 2020

zivkan Jun 17, 2020

heng-liu Aug 10, 2020

zivkan Jun 17, 2020 •

edited

Loading

heng-liu Aug 10, 2020

zivkan Jun 17, 2020

heng-liu Aug 10, 2020

nkolev92 left a comment

nkolev92 Jun 18, 2020

nkolev92 Jun 18, 2020

heng-liu Aug 7, 2020 •

edited

Loading

nkolev92 Aug 8, 2020

nkolev92 Jun 18, 2020

nkolev92 Jun 18, 2020

heng-liu Aug 10, 2020

nkolev92 left a comment •

edited

Loading

heng-liu commented Jun 19, 2020

nkolev92 Jun 19, 2020

heng-liu Aug 10, 2020

nkolev92 Jun 19, 2020

heng-liu Aug 10, 2020

zivkan left a comment

heng-liu commented Jun 19, 2020

		<SigningNotSupported Condition=" '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'netcoreapp2.1' OR '$(TargetFramework)' == 'netstandard2.1' OR '$(TargetFramework)' == 'netcoreapp3.1'">true</SigningNotSupported>
		<SigningNotSupported Condition=" '$(SigningNotSupported)' != 'true'">false</SigningNotSupported>

Enable cross-platform package signature verification for restore #3458

Enable cross-platform package signature verification for restore #3458

Conversation

heng-liu commented Jun 17, 2020

Bug

Fix

Testing/Validation

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heng-liu Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment • edited Loading

Choose a reason for hiding this comment

heng-liu commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan left a comment

Choose a reason for hiding this comment

heng-liu commented Jun 19, 2020

heng-liu Aug 10, 2020 •

edited

Loading

zivkan Jun 17, 2020 •

edited

Loading

heng-liu Aug 7, 2020 •

edited

Loading

nkolev92 left a comment •

edited

Loading