Skip to content

Commit

Permalink
Fixing signature exception logging in install command (#2212)
Browse files Browse the repository at this point in the history
## Bug
Fixes: NuGet/Home#6903
Regression: No

## Fix
Details: As part of e91dbf3 we added support for verifying package signature on unzip. Any errors results in throwing of a new `SignatureException` with actual results inside `SignatureException.Results`. In install command that exception was not throws as is and this resulted in empty logging on the console - 

```
C:\>NuGet.exe install <package>
...
NU3000:
```

This PR fixes this - 

```
F:\validation\test>NuGet.exe install <package>
...
WARNING: NU3027: The signature should be timestamped to enable long-term signature validity after the certificate has expired.
NU3008: The package integrity check failed.
```

## Testing/Validation
Tests Added: Yes  
Validation done:  manual validation

I have opened the following issues - 
https://github.com/NuGet/Home/issues/6905: Add infrastructure to support mock https server
NuGet/Home#6906: Improve singing related error messages
  • Loading branch information
Ankit Mishra committed May 8, 2018
1 parent 61c9683 commit e95a67d
Show file tree
Hide file tree
Showing 6 changed files with 313 additions and 6 deletions.
36 changes: 33 additions & 3 deletions src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using NuGet.Frameworks;
using NuGet.Packaging;
using NuGet.Packaging.Core;
using NuGet.Packaging.Signing;
using NuGet.ProjectManagement;
using NuGet.ProjectManagement.Projects;
using NuGet.ProjectModel;
Expand Down Expand Up @@ -1578,7 +1579,7 @@ await PreviewBuildIntegratedProjectActionsAsync(buildIntegratedProject, actions,
var projectName = NuGetProject.GetUniqueNameOrName(nuGetProject);
var projectId = string.Empty;
nuGetProject.TryGetMetadata<string>(NuGetProjectMetadataKeys.ProjectId, out projectId);

var stopWatch = Stopwatch.StartNew();

var projectInstalledPackageReferences = await nuGetProject.GetInstalledPackagesAsync(token);
Expand Down Expand Up @@ -2339,6 +2340,35 @@ var projectUniqueNamesForBuildIntToUpdate
// Open readme file
await OpenReadmeFile(nuGetProject, nuGetProjectContext, token);
}
catch (SignatureException ex)
{
var errors = ex.Results.SelectMany(r => r.GetErrorIssues());
var warnings = ex.Results.SelectMany(r => r.GetWarningIssues());
SignatureException unwrappedException = null;

if (errors.Count() == 1)
{
// In case of one error, throw it as the exception
var error = errors.First();
unwrappedException = new SignatureException(error.Code, error.Message, ex.PackageIdentity);
}
else
{
// In case of multiple errors, wrap them in a general NU3000 error
var errorMessage = string.Format(CultureInfo.CurrentCulture,
Strings.SignatureVerificationMultiple,
$"{Environment.NewLine}{string.Join(Environment.NewLine, errors.Select(e => e.FormatWithCode()))}");

unwrappedException = new SignatureException(NuGetLogCode.NU3000, errorMessage, ex.PackageIdentity);
}

foreach (var warning in warnings)
{
nuGetProjectContext.Log(MessageLevel.Warning, warning.FormatWithCode());
}

exceptionInfo = ExceptionDispatchInfo.Capture(unwrappedException);
}
catch (Exception ex)
{
exceptionInfo = ExceptionDispatchInfo.Capture(ex);
Expand Down Expand Up @@ -2491,7 +2521,7 @@ var projectUniqueNamesForBuildIntToUpdate
}

var logger = new ProjectContextLogger(nuGetProjectContext);
var dependencyGraphContext = new DependencyGraphCacheContext(logger, Settings );
var dependencyGraphContext = new DependencyGraphCacheContext(logger, Settings);

// Get Package Spec as json object
var originalPackageSpec = await DependencyGraphRestoreUtility.GetProjectSpec(buildIntegratedProject, dependencyGraphContext);
Expand Down Expand Up @@ -3089,7 +3119,7 @@ public bool PackageExistsInPackagesFolder(PackageIdentity packageIdentity)
/// project <paramref name="nuGetProject" /> is also installed in any
/// other projects in the solution.
/// </summary>
public static async Task<bool> PackageExistsInAnotherNuGetProject(NuGetProject nuGetProject, PackageIdentity packageIdentity, ISolutionManager solutionManager, CancellationToken token, bool excludeIntegrated= false)
public static async Task<bool> PackageExistsInAnotherNuGetProject(NuGetProject nuGetProject, PackageIdentity packageIdentity, ISolutionManager solutionManager, CancellationToken token, bool excludeIntegrated = false)
{
if (nuGetProject == null)
{
Expand Down
9 changes: 9 additions & 0 deletions src/NuGet.Core/NuGet.PackageManagement/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/NuGet.Core/NuGet.PackageManagement/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -487,4 +487,8 @@
<data name="XdtError" xml:space="preserve">
<value>An error occurred while applying transformation to '{0}' in project '{1}'</value>
</data>
<data name="SignatureVerificationMultiple" xml:space="preserve">
<value>Signed package validation failed with multiple errors:{0}</value>
<comment>0 - new line separated list of errors</comment>
</data>
</root>
12 changes: 11 additions & 1 deletion src/NuGet.Core/NuGet.Packaging/PackageExtractor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,17 @@ public static class PackageExtractor
token,
parentId);

if (!verifyResult.Valid)
if (verifyResult.Valid)
{
// log any warnings
var warnings = verifyResult.Results.SelectMany(r => r.GetWarningIssues());

foreach (var warning in warnings)
{
packageExtractionContext.Logger.Log(warning);
}
}
else
{
throw new SignatureException(verifyResult.Results, package);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,254 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Security.Cryptography.X509Certificates;
using System.Threading.Tasks;
using FluentAssertions;
using NuGet.Test.Utility;
using Org.BouncyCastle.Crypto.Parameters;
using Org.BouncyCastle.Security;
using Test.Utility.Signing;
using Xunit;

namespace NuGet.CommandLine.FuncTest.Commands
{
/// <summary>
/// Tests install command
/// These tests require admin privilege as the certs need to be added to the root store location
/// </summary>
[Collection(SignCommandTestCollection.Name)]
public class InstallCommandTests
{
private const string _NU3008 = "NU3008: The package integrity check failed.";
private const string _NU3012 = "NU3012: The author primary signature found a chain building issue: The certificate is revoked.";
private const string _NU3018 = "NU3018: The author primary signature found a chain building issue: A certificate chain processed, but terminated in a root certificate which is not trusted by the trust provider.";
private const string _NU3027 = "NU3027: The signature should be timestamped to enable long-term signature validity after the certificate has expired.";

private SignCommandTestFixture _testFixture;
private TrustedTestCert<TestCertificate> _trustedTestCert;
private string _nugetExePath;

public InstallCommandTests(SignCommandTestFixture fixture)
{
_testFixture = fixture ?? throw new ArgumentNullException(nameof(fixture));
_trustedTestCert = SigningTestUtility.GenerateTrustedTestCertificate();
_nugetExePath = _testFixture.NuGetExePath;
}

[CIOnlyFact]
public async Task Install_AuthorSignedPackage_SucceedsAsync()
{
// Arrange
var nupkg = new SimpleTestPackageContext("A", "1.0.0");

using (var context = new SimpleTestPathContext())
using (var testCertificate = new X509Certificate2(_trustedTestCert.Source.Cert))
{
await SignedArchiveTestUtility.AuthorSignPackageAsync(testCertificate, nupkg, context.WorkingDirectory);

var args = new string[]
{
nupkg.Id,
"-Version",
nupkg.Version,
"-DirectDownload",
"-NoCache",
"-Source",
context.WorkingDirectory,
"-OutputDirectory",
Path.Combine(context.WorkingDirectory, "packages")
};

// Act
var result = RunInstall(_nugetExePath, context, expectedExitCode: 0, additionalArgs: args);

// Assert
result.ExitCode.Should().Be(0);
result.AllOutput.Should().Contain($"WARNING: {_NU3027}");
}
}


[CIOnlyFact]
public async Task Install_RepoSignedPackage_SucceedsAsync()
{
// Arrange
var nupkg = new SimpleTestPackageContext("A", "1.0.0");

using (var context = new SimpleTestPathContext())
using (var testCertificate = new X509Certificate2(_trustedTestCert.Source.Cert))
{
await SignedArchiveTestUtility.RepositorySignPackageAsync(testCertificate, nupkg, context.WorkingDirectory, new Uri("https://v3serviceIndex.test/api/index.json"));

var args = new string[]
{
nupkg.Id,
"-Version",
nupkg.Version,
"-DirectDownload",
"-NoCache",
"-Source",
context.WorkingDirectory,
"-OutputDirectory",
Path.Combine(context.WorkingDirectory, "packages")
};

// Act
var result = RunInstall(_nugetExePath, context, expectedExitCode:0, additionalArgs: args);

// Assert
result.ExitCode.Should().Be(0);
result.AllOutput.Should().Contain($"WARNING: {_NU3027}");
}
}

[CIOnlyFact]
public async Task Install_UntrustedCertSignedPackage_WarnsAsync()
{
// Arrange
var nupkg = new SimpleTestPackageContext("A", "1.0.0");

using (var context = new SimpleTestPathContext())
using (var testCertificate = new X509Certificate2(_testFixture.UntrustedSelfIssuedCertificateInCertificateStore))
{
await SignedArchiveTestUtility.AuthorSignPackageAsync(testCertificate, nupkg, context.WorkingDirectory);

var args = new string[]
{
nupkg.Id,
"-Version",
nupkg.Version,
"-DirectDownload",
"-NoCache",
"-Source",
context.WorkingDirectory,
"-OutputDirectory",
Path.Combine(context.WorkingDirectory, "packages")
};

// Act
var result = RunInstall(_nugetExePath, context, expectedExitCode: 0, additionalArgs: args);

// Assert
result.ExitCode.Should().Be(0);
result.AllOutput.Should().Contain($"WARNING: {_NU3018}");
result.AllOutput.Should().Contain($"WARNING: {_NU3027}");
}
}

[CIOnlyFact]
public async Task Install_TamperedPackage_FailsAsync()
{
// Arrange
var nupkg = new SimpleTestPackageContext("A", "1.0.0");

using (var context = new SimpleTestPathContext())
using (var testCertificate = new X509Certificate2(_trustedTestCert.Source.Cert))
{
var signedPackagePath = await SignedArchiveTestUtility.AuthorSignPackageAsync(testCertificate, nupkg, context.WorkingDirectory);
SignedArchiveTestUtility.TamperWithPackage(signedPackagePath);

var args = new string[]
{
nupkg.Id,
"-Version",
nupkg.Version,
"-DirectDownload",
"-NoCache",
"-Source",
context.WorkingDirectory,
"-OutputDirectory",
Path.Combine(context.WorkingDirectory, "packages")
};

// Act
var result = RunInstall(_nugetExePath, context, expectedExitCode: 1, additionalArgs: args);

// Assert
result.ExitCode.Should().Be(1);
result.Errors.Should().Contain(_NU3008);
result.AllOutput.Should().Contain($"WARNING: {_NU3027}");
}
}

[CIOnlyFact]
public async Task Install_TamperedAndRevokedCertificateSignaturePackage_FailsAsync()
{
// Arrange
var nupkg = new SimpleTestPackageContext("A", "1.0.0");
var testServer = await _testFixture.GetSigningTestServerAsync();
var certificateAuthority = await _testFixture.GetDefaultTrustedCertificateAuthorityAsync();
var issueOptions = IssueCertificateOptions.CreateDefaultForEndCertificate();
var bcCertificate = certificateAuthority.IssueCertificate(issueOptions);

using (var context = new SimpleTestPathContext())
using (var testCertificate = new X509Certificate2(bcCertificate.GetEncoded()))
{
testCertificate.PrivateKey = DotNetUtilities.ToRSA(issueOptions.KeyPair.Private as RsaPrivateCrtKeyParameters);

var signedPackagePath = await SignedArchiveTestUtility.AuthorSignPackageAsync(testCertificate, nupkg, context.WorkingDirectory);
SignedArchiveTestUtility.TamperWithPackage(signedPackagePath);
certificateAuthority.Revoke(
bcCertificate,
RevocationReason.KeyCompromise,
DateTimeOffset.UtcNow);

var args = new string[]
{
nupkg.Id,
"-Version",
nupkg.Version,
"-DirectDownload",
"-NoCache",
"-Source",
context.WorkingDirectory,
"-OutputDirectory",
Path.Combine(context.WorkingDirectory, "packages")
};

// Act
var result = RunInstall(_nugetExePath, context, expectedExitCode: 1, additionalArgs: args);

// Assert
result.ExitCode.Should().Be(1);
result.Errors.Should().Contain(_NU3008);
result.Errors.Should().Contain(_NU3012);
result.AllOutput.Should().Contain($"WARNING: {_NU3027}");
}
}

public static CommandRunnerResult RunInstall(string nugetExe, SimpleTestPathContext pathContext, int expectedExitCode = 0, params string[] additionalArgs)
{
// Store the dg file for debugging
var envVars = new Dictionary<string, string>()
{
{ "NUGET_HTTP_CACHE_PATH", pathContext.HttpCacheFolder }
};

var args = new string[] {
"install",
"-Verbosity",
"detailed"
};

args = args.Concat(additionalArgs).ToArray();

// Act
var r = CommandRunner.Run(
nugetExe,
pathContext.WorkingDirectory,
string.Join(" ", args),
waitForExit: true,
environmentVariables: envVars);

// Assert
Assert.True(expectedExitCode == r.ExitCode, r.AllOutput);

return r;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public static class SignedArchiveTestUtility
HashAlgorithmName timestampHashAlgorithm = HashAlgorithmName.SHA256)
{
var testLogger = new TestLogger();
var signedPackagePath = Path.Combine(dir, Guid.NewGuid().ToString());
var signedPackagePath = Path.Combine(dir, $"{nupkg.Id}.{nupkg.Version}.nupkg");
var tempPath = Path.GetTempFileName();

using (var packageStream = await nupkg.CreateAsStreamAsync())
Expand Down Expand Up @@ -91,7 +91,7 @@ public static class SignedArchiveTestUtility
HashAlgorithmName timestampHashAlgorithm = HashAlgorithmName.SHA256)
{
var testLogger = new TestLogger();
var signedPackagePath = Path.Combine(dir, Guid.NewGuid().ToString());
var signedPackagePath = Path.Combine(dir, $"{nupkg.Id}.{nupkg.Version}.nupkg");
var tempPath = Path.GetTempFileName();

using (var packageStream = await nupkg.CreateAsStreamAsync())
Expand Down

0 comments on commit e95a67d

Please sign in to comment.