Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986

erdembayar · 2021-04-05T18:11:13Z

Bug

Fixes: NuGet/Home#10742 https://github.com/NuGet/Client.Engineering/issues/882

Regression? Last working version:

Description

Add an environment variable to opt-in to the package signing verification on .NET 5.
This is to provide an opt-in / troubleshooting experience for Mac/Linux based on the reversion of:
#3979
We intend this ENV for testing and don't guarantee compatibility.

PR Checklist

nkolev92 · 2021-04-05T18:46:54Z

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

-                return false;
+                // Conditionally enable back package sign verification temporary disabled due to Mozilla drop Symantec as CA on Linux/MAC.
+                // Please note: Linux/MAC case sensitive for env var.
+                string signVerifyEnvVariable = Environment.GetEnvironmentVariable("DOTNET_OPT_IN_SECURE_PACKAGE_VERIFICATION");


Use the https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Common/EnvironmentVariableWrapper.cs so you can add a unit test.

This is an API that's easily unit testable.

I already checked that one. I don't see any actual benefit.
Does it prevent from cross test env var value set contamination? If yes then we're going to have more flaky unit tests?
Specially it's problem if parallel tests are running same time.

Yes it prevents issues with concurrent unit test as you can provide a mock to each test with values you control. For example:

NuGet.Client/test/NuGet.Core.Tests/NuGet.Configuration.Test/ProxyCacheTests.cs

Lines 96 to 107 in f24bad0

// Arrange

var settings = Mock.Of<ISettings>();

var environment = new Mock<IEnvironmentVariableReader>(MockBehavior.Strict);

environment.Setup(s => s.GetEnvironmentVariable("http_proxy")).Returns(proxyValue);

var proxyCache = new ProxyCache(settings, environment.Object);

// Act

var proxy = proxyCache.GetUserConfiguredProxy();

// Assert

Assert.Null(proxy);

Does it prevent from cross test env var value set contamination? If yes then we're going to have more flaky unit tests?

Yes, because as @loic-sharma showed, you can mock the specific instance of the wrapper.

I just realized problem with this approach. I have to make change to existing public api PackageExtractor.ExtractPackageAsync in order to pass IEnvironmentVariableReader as parameter.
Is it really good justification to make change to public api, just sake of additional unit testing working?

You can add an internal constructor.
We set internalsvisibleto to test projects.

NuGet.Client/src/NuGet.Core/NuGet.Packaging/Properties/AssemblyInfo.cs

Line 11 in 7e3b0e4

[assembly: InternalsVisibleTo("NuGet.Packaging.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]

@nkolev92
PackageExtractor is static class, so it doesn't have constructor.
If we introduce new field into static class then it'll cause problem in concurrent unit test context.

PackageExtractor is static class, so it doesn't have constructor.

The change is in PackageArchiveReader not PackageExtractor.

Ok. I create new internal constructors for PackageArchiveReader and created new tests in most places by creating duplicate of existing test with environmental variable change.

But there're 2 places I can't use it and I don't think it it's really important for now.

RestoreCommandTests.cs >> RestoreCommand_InvalidSignedPackageAsync_SuccessAsync()

PackageExtractorTests.cs>>InstallFromSourceAsyncByStream_InvalidSignPackageWithoutUnzipAsync

Please review again.

heng-liu · 2021-04-06T18:19:56Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs

+                result.Success.Should().BeFalse();
+                result.ExitCode.Should().Be(1, because: "error text should be displayed as restore failed");


According to https://github.com/NuGet/NuGet.Client/blob/dev/test/TestUtilities/Test.Utility/CommandRunnerResult.cs#L37-L39, my understanding is, the two assertions are the same?

Yes, technically they're same. I just copied from another already existing integration test.
2nd one actually tells you what went wrong in text.

NuGet.Client/test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs

Lines 191 to 192 in 7e3b0e4

result.Success.Should().BeFalse();

result.ExitCode.Should().Be(1, because: "error text should be displayed as restore failed");

result.Success.Should().BeFalse(because: "error text should be displayed as restore failed");

The following works too.
Existing code is not always perfect.

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

erdembayar · 2021-04-07T04:04:03Z

@NuGet/nuget-client
Ready to review.

zivkan · 2021-04-07T16:40:58Z

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

+                string signVerifyEnvVariable = _environmentVariableReader == null ?
+                    EnvironmentVariableWrapper.Instance.GetEnvironmentVariable(envVarName) :
+                    _environmentVariableReader.GetEnvironmentVariable(envVarName);


If you made the old constructors call the new constructor, passing in EnvironmentVariableWrapper.Instance to the new parameter, then _environmentVariableReader will never be null. (well, assuming you implement null checking and throw ArgumentNullException).

My intention was not to change existing code. Also there're 6 public constructors vs 2 internal constructors for unit tests.
Currently _environmentVariableReader is used only for prevent from flaky unit tests when many concurrent tests run same time and I'm expecting whole Linux/MAC specific changes are temporary (including this environmental var checking + all new unit tests) will be removed once we get long term solution resistant any 3rd party changes(like Mozilla did), so checking it and throwing ArgumentNullException is not really important. Since I'm expecting this changes are temporary I prefer to make less changes to existing code, so it would be easy to revert in the future.

You can have the public constructors chain into a private constructor.

I couldn't find any blog posts or stack overflow answers suggesting best practices, but a pattern I've often seen, especially in the NuGet repo, is that only a single constructor has a non-empty body and all other constructor overloads chain into this "one true constructor". This makes it easy to validate that all fields/properties that must not be null are checked and assigned a value. Otherwise it's very difficult reading the code to ensure that fields/properties that should be set are correctly assigned a value in all different constructors.

This would have the side-effect of _environmentVariableHelper never being null, and having slightly less complex code in the methods that use the field.

Currently _environmentVariableReader is used only for prevent from flaky unit tests

If it's "only for tests", then we need to consider whether Arcade need an opt-in mechanism to keep doing signature validation on Linux and Mac, and do API design, including a new public API if necessary, to allow them to keep doing so.

This is the problem with NuGet being both a tool and an API. Given that the .NET platform is built with the arcade SDK, if we disable package signature validation, we might risk breaking every github.com/dotnet/ repo since they're using these APIs (maybe we will "already have" when Arcade upgrades to the version of NuGet that includes the PR we merged a week or two ago).

Maybe we need to talk to DncEng to find out what exactly they're using our package validation APIs for, and test that it will still do what they need with all these changes.

My intention was not to change existing code. Also there're 6 public constructors vs 2 internal constructors for unit tests.

What's motivation behind not changing existing code?
Is there a risk inherent with that?
Having a single root constructor helps avoid many of the argument checking pitfalls.

My intention was not to change existing code. Also there're 6 public constructors vs 2 internal constructors for unit tests.

What's motivation behind not changing existing code?
Is there a risk inherent with that?
Having a single root constructor helps avoid many of the argument checking pitfalls.

Since I'm expecting this changes are temporary I prefer to make less changes to existing code, so it would be easy to revert in the future.

We have git for that.
I think we should strive for quality code, regardless whether we think the change is temp or not temp.

There's no public API changes here, so no future risk there.

Maybe we need to talk to DncEng to find out what exactly they're using our package validation APIs for, and test that it will still do what they need with all these changes.

I don't know if DncEng/Arcade would be only users of this env var, intent for now only for testing. There're many cases temporary things become permanent thing so I'll address this for better code quality.

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

nkolev92 · 2021-04-08T18:26:13Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetRestoreTests.cs

+                result.Success.Should().BeFalse();
+                result.ExitCode.Should().Be(1, because: "error text should be displayed as restore failed");


result.Success.Should().BeFalse(because: "error text should be displayed as restore failed");

The following works too.
Existing code is not always perfect.

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageExtractorTests.cs

erdembayar · 2021-04-08T21:08:39Z

@JonDouglas @nkolev92 @aortiz-msft @dtivel
Renamed to DOTNET_NUGET_SIGNATURE_VERIFICATION. I'll merge the PR today evening.

erdembayar added 3 commits April 1, 2021 19:54

Add environmental flag for enabling sign verification.

dd17e1f

Change env var name after Jon code review.

ce1e1bb

Change variable name to DOTNET_ENABLE_PACKAGE_SIGNATURE_VERIFICATION

dfc6c98

erdembayar requested a review from a team as a code owner April 5, 2021 18:11

erdembayar changed the title ~~Dev eryondon sign config env variable~~ Add an ENV Variable for Package Signing Verification on .NET 5 Linux/MAC Apr 5, 2021

nkolev92 reviewed Apr 5, 2021

View reviewed changes

erdembayar changed the title ~~Add an ENV Variable for Package Signing Verification on .NET 5 Linux/MAC~~ Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC Apr 5, 2021

erdembayar force-pushed the dev-eryondon-SignConfigEnvVariable branch from 93f5e0e to dfc6c98 Compare April 6, 2021 04:38

heng-liu reviewed Apr 6, 2021

View reviewed changes

erdembayar added 2 commits April 6, 2021 12:33

Address PR review comment by Nikolche and Loic.

6b37961

Make sure envVariableReader is passed correctly.

e1a59c0

erdembayar requested review from heng-liu and loic-sharma April 6, 2021 21:04

zivkan reviewed Apr 6, 2021

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

Fix dotnet integration test.

d7916ae

kartheekp-ms reviewed Apr 6, 2021

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

Address PR comment by Karteek.

834c5d8

erdembayar requested a review from kartheekp-ms April 6, 2021 23:28

zivkan reviewed Apr 7, 2021

View reviewed changes

Address PR comment by Andy.

6a3c789

dtivel approved these changes Apr 7, 2021

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

Address PR comment by Damon.

fb609f6

erdembayar force-pushed the dev-eryondon-SignConfigEnvVariable branch from 97a9715 to fb609f6 Compare April 7, 2021 21:48

nkolev92 reviewed Apr 7, 2021

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

erdembayar added 3 commits April 7, 2021 15:27

Remove edge cases.

770f899

Check env var value is true.

bcd3840

Typo fix

fd2259d

nkolev92 reviewed Apr 8, 2021

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs Outdated Show resolved Hide resolved

Address Nikolche code review.

fd43fce

erdembayar added 3 commits April 7, 2021 21:10

Value case sensitive

230bf61

Fix typo.

b454db5

Change back to .net internal type.

824008c

dtivel approved these changes Apr 8, 2021

View reviewed changes

nkolev92 approved these changes Apr 8, 2021

View reviewed changes

Rename opt-in env var name one more time.

51976c0

erdembayar added 2 commits April 8, 2021 14:13

Fix typo

50af84f

Minor typo fix.

747e5f2

erdembayar merged commit 936eff1 into dev Apr 9, 2021

erdembayar deleted the dev-eryondon-SignConfigEnvVariable branch April 9, 2021 01:25

dtivel mentioned this pull request Apr 20, 2022

Signing: enable custom trust roots and re-enable signed package verification on Linux under .NET 7 #4585

Closed

8 tasks

dtivel mentioned this pull request Jun 30, 2022

Signing: enable signed package verification on Linux by default in .NET 6 SDK #4706

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986

Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986

erdembayar commented Apr 5, 2021 •

edited

Loading

nkolev92 Apr 5, 2021

erdembayar Apr 5, 2021

loic-sharma Apr 5, 2021 •

edited

Loading

nkolev92 Apr 5, 2021

erdembayar Apr 6, 2021 •

edited

Loading

nkolev92 Apr 6, 2021

erdembayar Apr 6, 2021

nkolev92 Apr 6, 2021

erdembayar Apr 6, 2021

heng-liu Apr 6, 2021

erdembayar Apr 6, 2021

nkolev92 Apr 8, 2021

erdembayar commented Apr 7, 2021

zivkan Apr 7, 2021

erdembayar Apr 7, 2021 •

edited

Loading

zivkan Apr 7, 2021

nkolev92 Apr 7, 2021

erdembayar Apr 7, 2021

nkolev92 Apr 7, 2021

erdembayar Apr 7, 2021 •

edited

Loading

nkolev92 Apr 8, 2021

erdembayar commented Apr 8, 2021

	// Arrange
	var settings = Mock.Of<ISettings>();
	var environment = new Mock<IEnvironmentVariableReader>(MockBehavior.Strict);
	environment.Setup(s => s.GetEnvironmentVariable("http_proxy")).Returns(proxyValue);

	var proxyCache = new ProxyCache(settings, environment.Object);

	// Act
	var proxy = proxyCache.GetUserConfiguredProxy();

	// Assert
	Assert.Null(proxy);

		result.Success.Should().BeFalse();
		result.ExitCode.Should().Be(1, because: "error text should be displayed as restore failed");

Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986

Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986

Conversation

erdembayar commented Apr 5, 2021 • edited Loading

Bug

Description

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Apr 6, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Apr 7, 2021

Choose a reason for hiding this comment

erdembayar Apr 7, 2021 • 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

erdembayar Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar commented Apr 8, 2021

erdembayar commented Apr 5, 2021 •

edited

Loading

loic-sharma Apr 5, 2021 •

edited

Loading

erdembayar Apr 6, 2021 •

edited

Loading

erdembayar Apr 7, 2021 •

edited

Loading

erdembayar Apr 7, 2021 •

edited

Loading