Add an ENV Variable for Package Signing Verification on .NET 5+ Linux/MAC #3986
+939
−7
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,6 +265,337 @@ await SimpleTestPackageUtility.CreateFolderFeedV3Async( | |
| } | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("true")] | ||
| [InlineData("TRUE")] | ||
| [InlineData("TRUe")] | ||
| public async Task DotnetRestore_WithUnSignedPackageAndSignatureValidationModeAsRequired_OptInEnvVar_True_FailsAsync(string envVar) | ||
| { | ||
| using (var pathContext = _msbuildFixture.CreateSimpleTestPathContext()) | ||
| { | ||
| //Arrange | ||
| var envVarName = "DOTNET_OPT_IN_SECURE_PACKAGE_VERIFICATION"; | ||
| var envVarValue = envVar; | ||
| //Setup packages and feed | ||
| var packageX = new SimpleTestPackageContext() | ||
| { | ||
| Id = "x", | ||
| Version = "1.0.0" | ||
| }; | ||
| packageX.Files.Clear(); | ||
| packageX.AddFile("lib/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("ref/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("lib/net472/x.dll"); | ||
| packageX.AddFile("ref/net472/x.dll"); | ||
|
|
||
| await SimpleTestPackageUtility.CreateFolderFeedV3Async( | ||
| pathContext.PackageSource, | ||
| PackageSaveMode.Defaultv3, | ||
| packageX); | ||
|
|
||
| // Set up solution, and project | ||
| var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot); | ||
|
|
||
| var projectName = "ClassLibrary1"; | ||
| var workingDirectory = Path.Combine(pathContext.SolutionRoot, projectName); | ||
| var projectFile = Path.Combine(workingDirectory, $"{projectName}.csproj"); | ||
|
|
||
| _msbuildFixture.CreateDotnetNewProject(pathContext.SolutionRoot, projectName, "classlib"); | ||
|
|
||
| using (FileStream stream = File.Open(projectFile, FileMode.Open, FileAccess.ReadWrite)) | ||
| { | ||
| XDocument xml = XDocument.Load(stream); | ||
|
|
||
| var attributes = new Dictionary<string, string>() { { "Version", "1.0.0" } }; | ||
|
|
||
| ProjectFileUtils.AddItem( | ||
| xml, | ||
| "PackageReference", | ||
| packageX.Id, | ||
| string.Empty, | ||
| new Dictionary<string, string>(), | ||
| attributes); | ||
|
|
||
| ProjectFileUtils.WriteXmlToFile(xml, stream); | ||
| } | ||
|
|
||
| //set nuget.config properties | ||
| var doc = new XDocument(); | ||
| var configuration = new XElement(XName.Get("configuration")); | ||
| doc.Add(configuration); | ||
|
|
||
| var config = new XElement(XName.Get("config")); | ||
| configuration.Add(config); | ||
|
|
||
| var signatureValidationMode = new XElement(XName.Get("add")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("key"), "signatureValidationMode")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("value"), "require")); | ||
| config.Add(signatureValidationMode); | ||
|
|
||
| File.WriteAllText(Path.Combine(workingDirectory, "NuGet.Config"), doc.ToString()); | ||
|
|
||
| // Act | ||
| CommandRunnerResult result = _msbuildFixture.RunDotnet( | ||
| workingDirectory, "restore", | ||
| ignoreExitCode: true, | ||
| additionalEnvVars: new Dictionary<string, string>() | ||
| { | ||
| { envVarName, envVarValue } | ||
| } | ||
| ); | ||
|
|
||
| result.AllOutput.Should().Contain($"error NU3004: Package '{packageX.Id} {packageX.Version}' from source '{pathContext.PackageSource}': signatureValidationMode is set to require, so packages are allowed only if signed by trusted signers; however, this package is unsigned."); | ||
| result.Success.Should().BeFalse(); | ||
| result.ExitCode.Should().Be(1, because: "error text should be displayed as restore failed"); | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. Yes, technically they're same. I just copied from another already existing integration test. There was a problem hiding this comment.
The following works too. |
||
| } | ||
| } | ||
|
|
||
| [PlatformFact(Platform.Linux, Platform.Darwin)] | ||
| public async Task DotnetRestore_WithUnSignedPackageAndSignatureValidationModeAsRequired_OptInEnvVar_False_SucceedAsync() | ||
| { | ||
| using (var pathContext = _msbuildFixture.CreateSimpleTestPathContext()) | ||
| { | ||
| //Arrange | ||
| var envVarName = "DOTNET_OPT_IN_SECURE_PACKAGE_VERIFICATION"; | ||
| var envVarValue = "false"; | ||
| //Setup packages and feed | ||
| var packageX = new SimpleTestPackageContext() | ||
| { | ||
| Id = "x", | ||
| Version = "1.0.0" | ||
| }; | ||
| packageX.Files.Clear(); | ||
| packageX.AddFile("lib/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("ref/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("lib/net472/x.dll"); | ||
| packageX.AddFile("ref/net472/x.dll"); | ||
|
|
||
| await SimpleTestPackageUtility.CreateFolderFeedV3Async( | ||
| pathContext.PackageSource, | ||
| PackageSaveMode.Defaultv3, | ||
| packageX); | ||
|
|
||
| // Set up solution, and project | ||
| var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot); | ||
|
|
||
| var projectName = "ClassLibrary1"; | ||
| var workingDirectory = Path.Combine(pathContext.SolutionRoot, projectName); | ||
| var projectFile = Path.Combine(workingDirectory, $"{projectName}.csproj"); | ||
|
|
||
| _msbuildFixture.CreateDotnetNewProject(pathContext.SolutionRoot, projectName, "classlib"); | ||
|
|
||
| using (FileStream stream = File.Open(projectFile, FileMode.Open, FileAccess.ReadWrite)) | ||
| { | ||
| XDocument xml = XDocument.Load(stream); | ||
|
|
||
| var attributes = new Dictionary<string, string>() { { "Version", "1.0.0" } }; | ||
|
|
||
| ProjectFileUtils.AddItem( | ||
| xml, | ||
| "PackageReference", | ||
| packageX.Id, | ||
| string.Empty, | ||
| new Dictionary<string, string>(), | ||
| attributes); | ||
|
|
||
| ProjectFileUtils.WriteXmlToFile(xml, stream); | ||
| } | ||
|
|
||
| //set nuget.config properties | ||
| var doc = new XDocument(); | ||
| var configuration = new XElement(XName.Get("configuration")); | ||
| doc.Add(configuration); | ||
|
|
||
| var config = new XElement(XName.Get("config")); | ||
| configuration.Add(config); | ||
|
|
||
| var signatureValidationMode = new XElement(XName.Get("add")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("key"), "signatureValidationMode")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("value"), "require")); | ||
| config.Add(signatureValidationMode); | ||
|
|
||
| File.WriteAllText(Path.Combine(workingDirectory, "NuGet.Config"), doc.ToString()); | ||
|
|
||
| // Act | ||
| CommandRunnerResult result = _msbuildFixture.RunDotnet( | ||
| workingDirectory, "restore", | ||
| ignoreExitCode: true, | ||
| additionalEnvVars: new Dictionary<string, string>() | ||
| { | ||
| { envVarName, envVarValue } | ||
| } | ||
| ); | ||
|
|
||
| result.AllOutput.Should().NotContain($"error NU3004"); | ||
| result.Success.Should().BeTrue(); | ||
| result.ExitCode.Should().Be(0); | ||
| } | ||
| } | ||
|
|
||
| [PlatformFact(Platform.Linux, Platform.Darwin)] | ||
| public async Task DotnetRestore_WithUnSignedPackageAndSignatureValidationModeAsRequired_OptInEnvVar_Null_SucceedAsync() | ||
| { | ||
| using (var pathContext = _msbuildFixture.CreateSimpleTestPathContext()) | ||
| { | ||
| //Arrange | ||
| var envVarName = "DOTNET_OPT_IN_SECURE_PACKAGE_VERIFICATION"; | ||
| string envVarValue = null; | ||
| //Setup packages and feed | ||
| var packageX = new SimpleTestPackageContext() | ||
| { | ||
| Id = "x", | ||
| Version = "1.0.0" | ||
| }; | ||
| packageX.Files.Clear(); | ||
| packageX.AddFile("lib/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("ref/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("lib/net472/x.dll"); | ||
| packageX.AddFile("ref/net472/x.dll"); | ||
|
|
||
| await SimpleTestPackageUtility.CreateFolderFeedV3Async( | ||
| pathContext.PackageSource, | ||
| PackageSaveMode.Defaultv3, | ||
| packageX); | ||
|
|
||
| // Set up solution, and project | ||
| var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot); | ||
|
|
||
| var projectName = "ClassLibrary1"; | ||
| var workingDirectory = Path.Combine(pathContext.SolutionRoot, projectName); | ||
| var projectFile = Path.Combine(workingDirectory, $"{projectName}.csproj"); | ||
|
|
||
| _msbuildFixture.CreateDotnetNewProject(pathContext.SolutionRoot, projectName, "classlib"); | ||
|
|
||
| using (FileStream stream = File.Open(projectFile, FileMode.Open, FileAccess.ReadWrite)) | ||
| { | ||
| XDocument xml = XDocument.Load(stream); | ||
|
|
||
| var attributes = new Dictionary<string, string>() { { "Version", "1.0.0" } }; | ||
|
|
||
| ProjectFileUtils.AddItem( | ||
| xml, | ||
| "PackageReference", | ||
| packageX.Id, | ||
| string.Empty, | ||
| new Dictionary<string, string>(), | ||
| attributes); | ||
|
|
||
| ProjectFileUtils.WriteXmlToFile(xml, stream); | ||
| } | ||
|
|
||
| //set nuget.config properties | ||
| var doc = new XDocument(); | ||
| var configuration = new XElement(XName.Get("configuration")); | ||
| doc.Add(configuration); | ||
|
|
||
| var config = new XElement(XName.Get("config")); | ||
| configuration.Add(config); | ||
|
|
||
| var signatureValidationMode = new XElement(XName.Get("add")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("key"), "signatureValidationMode")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("value"), "require")); | ||
| config.Add(signatureValidationMode); | ||
|
|
||
| File.WriteAllText(Path.Combine(workingDirectory, "NuGet.Config"), doc.ToString()); | ||
|
|
||
| // Act | ||
| CommandRunnerResult result = _msbuildFixture.RunDotnet( | ||
| workingDirectory, "restore", | ||
| ignoreExitCode: true, | ||
| additionalEnvVars: new Dictionary<string, string>() | ||
| { | ||
| { envVarName, envVarValue } | ||
| } | ||
| ); | ||
|
|
||
| result.AllOutput.Should().NotContain($"error NU3004"); | ||
| result.Success.Should().BeTrue(); | ||
| result.ExitCode.Should().Be(0); | ||
| } | ||
| } | ||
|
|
||
| [PlatformFact(Platform.Linux, Platform.Darwin)] | ||
| public async Task DotnetRestore_WithUnSignedPackageAndSignatureValidationModeAsRequired_WrongName_OptInEnvVar_SucceedAsync() | ||
| { | ||
| using (var pathContext = _msbuildFixture.CreateSimpleTestPathContext()) | ||
| { | ||
| //Arrange | ||
| var envVarName = "dOTNET_OPT_IN_SECURE_PACKAGE_VERIFICATIOn"; | ||
| var envVarValue = "true"; | ||
| //Setup packages and feed | ||
| var packageX = new SimpleTestPackageContext() | ||
| { | ||
| Id = "x", | ||
| Version = "1.0.0" | ||
| }; | ||
| packageX.Files.Clear(); | ||
| packageX.AddFile("lib/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("ref/netcoreapp2.0/x.dll"); | ||
| packageX.AddFile("lib/net472/x.dll"); | ||
| packageX.AddFile("ref/net472/x.dll"); | ||
|
|
||
| await SimpleTestPackageUtility.CreateFolderFeedV3Async( | ||
| pathContext.PackageSource, | ||
| PackageSaveMode.Defaultv3, | ||
| packageX); | ||
|
|
||
| // Set up solution, and project | ||
| var solution = new SimpleTestSolutionContext(pathContext.SolutionRoot); | ||
|
|
||
| var projectName = "ClassLibrary1"; | ||
| var workingDirectory = Path.Combine(pathContext.SolutionRoot, projectName); | ||
| var projectFile = Path.Combine(workingDirectory, $"{projectName}.csproj"); | ||
|
|
||
| _msbuildFixture.CreateDotnetNewProject(pathContext.SolutionRoot, projectName, "classlib"); | ||
|
|
||
| using (FileStream stream = File.Open(projectFile, FileMode.Open, FileAccess.ReadWrite)) | ||
| { | ||
| XDocument xml = XDocument.Load(stream); | ||
|
|
||
| var attributes = new Dictionary<string, string>() { { "Version", "1.0.0" } }; | ||
|
|
||
| ProjectFileUtils.AddItem( | ||
| xml, | ||
| "PackageReference", | ||
| packageX.Id, | ||
| string.Empty, | ||
| new Dictionary<string, string>(), | ||
| attributes); | ||
|
|
||
| ProjectFileUtils.WriteXmlToFile(xml, stream); | ||
| } | ||
|
|
||
| //set nuget.config properties | ||
| var doc = new XDocument(); | ||
| var configuration = new XElement(XName.Get("configuration")); | ||
| doc.Add(configuration); | ||
|
|
||
| var config = new XElement(XName.Get("config")); | ||
| configuration.Add(config); | ||
|
|
||
| var signatureValidationMode = new XElement(XName.Get("add")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("key"), "signatureValidationMode")); | ||
| signatureValidationMode.Add(new XAttribute(XName.Get("value"), "require")); | ||
| config.Add(signatureValidationMode); | ||
|
|
||
| File.WriteAllText(Path.Combine(workingDirectory, "NuGet.Config"), doc.ToString()); | ||
|
|
||
| // Act | ||
| CommandRunnerResult result = _msbuildFixture.RunDotnet( | ||
| workingDirectory, "restore", | ||
| ignoreExitCode: true, | ||
| additionalEnvVars: new Dictionary<string, string>() | ||
| { | ||
| { envVarName, envVarValue } | ||
| } | ||
| ); | ||
|
|
||
| result.AllOutput.Should().NotContain($"error NU3004"); | ||
| result.Success.Should().BeTrue(); | ||
| result.ExitCode.Should().Be(0); | ||
| } | ||
| } | ||
|
|
||
| [PlatformFact(Platform.Windows)] | ||
| public void DotnetRestore_WithAuthorSignedPackageAndSignatureValidationModeAsRequired_Succeeds() | ||
| { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you made the old constructors call the new constructor, passing in
EnvironmentVariableWrapper.Instanceto the new parameter, then_environmentVariableReaderwill never be null. (well, assuming you implement null checking and throw ArgumentNullException).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was not to change existing code. Also there're 6 public constructors vs 2 internal constructors for unit tests.
Currently
_environmentVariableReaderis 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 throwingArgumentNullExceptionis 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
_environmentVariableHelpernever being null, and having slightly less complex code in the methods that use the field.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.