Allow unavailable revocation information during extract and validate (#314)

Address NuGet/Engineering#785

Author: joelverhagen

src/Validation.PackageSigning.ExtractAndValidateSignature/PackageSignatureVerifierFactory.cs

@@ -25,8 +25,9 @@ public static IPackageSignatureVerifier CreateMinimal()
                 allowUnsigned: true,
                 allowUntrusted: false, // Invalid format of the signature uses this flag to determine success.
                 allowIgnoreTimestamp: true,
-                failWithMultipleTimestamps: false,
-                allowNoTimestamp: true);
+                allowMultipleTimestamps: true,
+                allowNoTimestamp: true,
+                allowUnknownRevocation: true);
 
             return new PackageSignatureVerifier(
                 verificationProviders,
@@ -48,8 +49,9 @@ public static IPackageSignatureVerifier CreateFull()
                 allowUnsigned: false,
                 allowUntrusted: false,
                 allowIgnoreTimestamp: false,
-                failWithMultipleTimestamps: true,
-                allowNoTimestamp: false);
+                allowMultipleTimestamps: false,
+                allowNoTimestamp: false,
+                allowUnknownRevocation: true);
 
             return new PackageSignatureVerifier(
                 verificationProviders,

src/Validation.PackageSigning.ExtractAndValidateSignature/SignatureValidator.cs

@@ -20,6 +20,9 @@ namespace NuGet.Jobs.Validation.PackageSigning.ExtractAndValidateSignature
 {
     public class SignatureValidator : ISignatureValidator
     {
+        private const string FormatVerificationName = "format verification";
+        private const string SignatureVerificationName = "signature integrity and trust verification";
+
         private readonly IPackageSigningStateService _packageSigningStateService;
         private readonly IPackageSignatureVerifier _minimalPackageSignatureVerifier;
         private readonly IPackageSignatureVerifier _fullPackageSignatureVerifier;
@@ -87,6 +90,7 @@ private async Task<SignatureValidatorResult> HandleSignedPackageAsync(
                 // First, detect format errors with a minimal verification. This doesn't even check package integrity. The
                 // minimal verification is expected to swallow any sort of signature format exception.
                 var invalidFormatResult = await GetVerifyResult(
+                    FormatVerificationName,
                     _minimalPackageSignatureVerifier,
                     packageKey,
                     signedPackageReader,
@@ -163,6 +167,7 @@ private async Task<SignatureValidatorResult> HandleSignedPackageAsync(
 
                 // Call the "verify" API, which does the main logic of signature validation.
                 var failureResult = await GetVerifyResult(
+                    SignatureVerificationName,
                     _fullPackageSignatureVerifier,
                     packageKey,
                     signedPackageReader,
@@ -205,6 +210,7 @@ private async Task<SignatureValidatorResult> HandleSignedPackageAsync(
         }
 
         private async Task<SignatureValidatorResult> GetVerifyResult(
+            string verificationName,
             IPackageSignatureVerifier verifier,
             int packageKey,
             ISignedPackageReader signedPackageReader,
@@ -214,26 +220,27 @@ private async Task<SignatureValidatorResult> GetVerifyResult(
             var verifyResult = await verifier.VerifySignaturesAsync(
                 signedPackageReader,
                 cancellationToken);
-            if (!verifyResult.Valid)
-            {
-                var errorIssues = verifyResult
-                    .Results
-                    .SelectMany(x => x.GetErrorIssues())
-                    .ToList();
 
-                var errorsForLogs = errorIssues
-                    .Select(x => $"{x.Code}: {x.Message}")
-                    .ToList();
-                var warningsForLogs = verifyResult
-                    .Results
-                    .SelectMany(x => x.GetWarningIssues())
-                    .Select(x => $"{x.Code}: {x.Message}")
-                    .ToList();
+            var errorIssues = verifyResult
+                .Results
+                .SelectMany(x => x.GetErrorIssues())
+                .ToList();
+            var warningsForLogs = verifyResult
+                .Results
+                .SelectMany(x => x.GetWarningIssues())
+                .Select(x => $"{x.Code}: {x.Message}")
+                .ToList();
+            var errorsForLogs = errorIssues
+                .Select(x => $"{x.Code}: {x.Message}")
+                .ToList();
 
+            if (!verifyResult.Valid)
+            {
                 _logger.LogInformation(
-                    "Signed package {PackageId} {PackageVersion} is blocked for validation {ValidationId} due to verify failures. Errors: {Errors} Warnings: {Warnings}",
+                    "Signed package {PackageId} {PackageVersion} is blocked during {VerificationName} for validation {ValidationId} . Errors: [{Errors}] Warnings: [{Warnings}]",
                     message.PackageId,
                     message.PackageVersion,
+                    verificationName,
                     message.ValidationId,
                     errorsForLogs,
                     warningsForLogs);
@@ -260,8 +267,19 @@ private async Task<SignatureValidatorResult> GetVerifyResult(
                     message,
                     errorValidationIssues);
             }
+            else
+            {
+                _logger.LogInformation(
+                   "Signed package {PackageId} {PackageVersion} passed {VerificationName} for validation {ValidationId}. Errors: [{Errors}] Warnings: [{Warnings}]",
+                   message.PackageId,
+                   message.PackageVersion,
+                   verificationName,
+                   message.ValidationId,
+                   errorsForLogs,
+                   warningsForLogs);
 
-            return null;
+                return null;
+            }
         }
 
         private async Task<SignatureValidatorResult> RejectAsync(

src/Validation.PackageSigning.ExtractAndValidateSignature/Validation.PackageSigning.ExtractAndValidateSignature.csproj

@@ -106,7 +106,7 @@
       <Version>1.1.2</Version>
     </PackageReference>
     <PackageReference Include="NuGet.Packaging">
-      <Version>4.7.0-preview1-4853</Version>
+      <Version>4.7.0-preview1-4870</Version>
     </PackageReference>
     <PackageReference Include="NuGet.Services.Configuration">
       <Version>2.12.0</Version>

tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/SignaturePartsExtractorFacts.cs

@@ -26,7 +26,7 @@ public class SignaturePartsExtractorFacts
         private const string BouncyCastleCollection = "Collection";
 
         private static readonly DateTime Leaf1TimestampValue = DateTime
-            .Parse("2018-01-16T20:31:09.0000000Z")
+            .Parse("2018-01-26T22:08:31.0000000Z")
             .ToUniversalTime();
 
         /// <summary>
@@ -42,7 +42,7 @@ public class SignaturePartsExtractorFacts
             {
                 new SubjectAndThumbprint(
                     "CN=NUGET_DO_NOT_TRUST.intermediate.test.test, OU=Test Organizational Unit Name, O=Test Organization Name, L=Redmond, S=WA, C=US",
-                    "d5949445cde4d80bc0c857dddb8520114a146d73de081a77404b0c17dda6a4b4"),
+                    "7358e4597696b1d02e7aa2b3cf30a7cf154f2c8ff0710fd0dc3ace17e3784054"),
                 new SubjectAndThumbprint(
                     "CN=NUGET_DO_NOT_TRUST.root.test.test, OU=Test Organizational Unit Name, O=Test Organization Name, L=Redmond, S=WA, C=US",
                     TestResources.RootThumbprint),

tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/SignatureValidatorIntegrationTests.cs

@@ -39,7 +39,7 @@ public class SignatureValidatorIntegrationTests : IDisposable
         private readonly List<string> _trustedThumbprints;
         private readonly IPackageSignatureVerifier _minimalPackageSignatureVerifier;
         private readonly IPackageSignatureVerifier _fullPackageSignatureVerifier;
-        private readonly ILogger<SignatureValidator> _logger;
+        private readonly RecordingLogger<SignatureValidator> _logger;
         private readonly int _packageKey;
         private SignedPackageArchive _package;
         private readonly SignatureValidationMessage _message;
@@ -69,7 +69,7 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt
 
             var loggerFactory = new LoggerFactory();
             loggerFactory.AddXunit(output);
-            _logger = loggerFactory.CreateLogger<SignatureValidator>();
+            _logger = new RecordingLogger<SignatureValidator>(loggerFactory.CreateLogger<SignatureValidator>());
 
             // Initialize data.
             _packageKey = 23;
@@ -120,6 +120,64 @@ public async Task AcceptsValidSignedPackage()
             Assert.Empty(result.Issues);
         }
 
+        [Fact]
+        public async Task RejectsUntrustedSigningCertificate()
+        {
+            // Arrange
+            AllowCertificateThumbprint(TestResources.Leaf1Thumbprint);
+            _package = TestResources.SignedPackageLeaf1Reader;
+
+            // Act
+            var result = await _target.ValidateAsync(
+                _packageKey,
+                _package,
+                _message,
+                _token);
+
+            // Assert
+            VerifyPackageSigningStatus(result, ValidationStatus.Failed, PackageSigningStatus.Invalid);
+            Assert.NotEmpty(result.Issues);
+            var clientIssues = result
+                .Issues
+                .OfType<ClientSigningVerificationFailure>()
+                .Where(x => x.ClientCode == "NU3021")
+                .ToList();
+            var untrustedIssue = Assert.Single(clientIssues);
+            Assert.Equal(
+                "A certificate chain processed, but terminated in a root certificate which is not trusted by the trust provider.",
+                untrustedIssue.ClientMessage);
+        }
+
+        [Fact]
+        public async Task AcceptsTrustedCertificateWithUnavailableRevocation()
+        {
+            // Arrange
+            using (var trustedTestCert = new TrustedTestCert<X509Certificate2>(
+                await TestResources.GetTestRootCertificateAsync(),
+                x => x,
+                StoreName.Root,
+                StoreLocation.LocalMachine,
+                maximumValidityPeriod: TimeSpan.MaxValue))
+            {
+                _package = TestResources.SignedPackageLeaf1Reader;
+                AllowCertificateThumbprint(TestResources.Leaf1Thumbprint);
+
+                // Act
+                var result = await _target.ValidateAsync(
+                    _packageKey,
+                    _package,
+                    _message,
+                    _token);
+
+                // Assert
+                VerifyPackageSigningStatus(result, ValidationStatus.Succeeded, PackageSigningStatus.Valid);
+                Assert.Empty(result.Issues);
+                var allMessages = string.Join(Environment.NewLine, _logger.Messages);
+                Assert.Contains("NU3018: The revocation function was unable to check revocation because the revocation server was offline.", allMessages);
+                Assert.Contains("NU3018: The revocation function was unable to check revocation for the certificate.", allMessages);
+            }
+        }
+
         [Fact]
         public async Task RejectsPackageWithAddedFile()
         {

tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/Support/CertificateIntegrationTestFixture.cs

@@ -111,6 +111,13 @@ await GetSignedPackageStreamAsync(reference, resourceName, certificate, output),
 
         private static async Task<byte[]> GenerateSignedPackageBytesAsync(string resourceName, TrustedTestCert<TestCertificate> certificate, ITestOutputHelper output)
         {
+            if (string.IsNullOrWhiteSpace(_testTimestampServer))
+            {
+                Assert.False(
+                    string.IsNullOrWhiteSpace(_testTimestampServer),
+                    "You must set a TIMESTAMP_SERVER_URL environment variable to an accessible timestamping authority URL.");
+            }
+
             var testLogger = new TestLogger(output);
             var timestampProvider = new Rfc3161TimestampProvider(new Uri(_testTimestampServer));
             var signatureProvider = new X509SignatureProvider(timestampProvider);
@@ -125,7 +132,7 @@ private static async Task<byte[]> GenerateSignedPackageBytesAsync(string resourc
                 signatureProvider,
                 x =>
                 {
-                    var request = new SignPackageRequest(certificate.TrustedCert, HashAlgorithmName.SHA256);
+                    var request = new AuthorSignPackageRequest(certificate.TrustedCert, HashAlgorithmName.SHA256);
                     return x.SignAsync(request, testLogger, CancellationToken.None);
                 });
 

tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/Support/RecordingLogger.cs

@@ -0,0 +1,38 @@
+// 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 Microsoft.Extensions.Logging;
+
+namespace Validation.PackageSigning.ExtractAndValidateSignature.Tests
+{
+    public class RecordingLogger<T> : ILogger<T>
+    {
+        private readonly ILogger<T> _inner;
+        private readonly List<string> _messages = new List<string>();
+
+        public RecordingLogger(ILogger<T> inner)
+        {
+            _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+        }
+
+        public IReadOnlyList<string> Messages => _messages;
+
+        public IDisposable BeginScope<TState>(TState state)
+        {
+            return _inner.BeginScope(state);
+        }
+
+        public bool IsEnabled(LogLevel logLevel)
+        {
+            return _inner.IsEnabled(logLevel);
+        }
+
+        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
+        {
+            _messages.Add(formatter(state, exception));
+            _inner.Log(logLevel, eventId, state, exception, formatter);
+        }
+    }
+}

tests/Validation.PackageSigning.ExtractAndValidateSignature.Tests/Support/TestResources.cs

@@ -1,14 +1,28 @@
 // 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.IO;
+using System.Linq;
+using System.Security.Cryptography.X509Certificates;
+using System.Threading;
+using System.Threading.Tasks;
 using NuGet.Packaging.Signing;
 
 namespace Validation.PackageSigning.ExtractAndValidateSignature.Tests
 {
     public static class TestResources
     {
         private const string ResourceNamespace = "Validation.PackageSigning.ExtractAndValidateSignature.Tests.TestData";
+        private static readonly Lazy<Task<byte[]>> _lazyTestRootCertificate = new Lazy<Task<byte[]>>(async () =>
+        {
+            using (var package = SignedPackageLeaf1Reader)
+            {
+                var signature = await package.GetSignatureAsync(CancellationToken.None);
+                var certificates = SignatureUtility.GetPrimarySignatureCertificates(signature);
+                return certificates.Last().RawData;
+            }
+        });
 
         public const string SignedPackageLeaf1 = ResourceNamespace + ".TestSigned.leaf-1.1.0.0.nupkg";
         public const string SignedPackageLeaf2 = ResourceNamespace + ".TestSigned.leaf-2.2.0.0.nupkg";
@@ -18,17 +32,17 @@ public static class TestResources
         /// <summary>
         /// This is the SHA-256 thumbprint of the root CA certificate for the signing certificate of <see cref="SignedPackageLeaf1"/>.
         /// </summary>
-        public const string RootThumbprint = "0e829fa17cfd9be513a41d9f205320f7d035f48d6c4cc7acbaa95f1744c1d6bb";
+        public const string RootThumbprint = "557276839c961df211cf267b318d880568676efa41e8b62d9bb38752c1d6214d";
 
         /// <summary>
         /// This is the SHA-256 thumbprint of the signing certificate in <see cref="SignedPackageLeaf1"/>.
         /// </summary>
-        public const string Leaf1Thumbprint = "56a23ed7c0ef80bd0269d4a3b41e3e2830243a9fc85594b6c311e27423df6023";
+        public const string Leaf1Thumbprint = "4456d5d38709876dcd20ef3d7ba98bfd79fcaee91141d153a55f10841ef909c6";
 
         /// <summary>
         /// This is the SHA-256 thumbprint of the signing certificate in <see cref="SignedPackageLeaf2"/>.
         /// </summary>
-        public const string Leaf2Thumbprint = "cd177f02cb88f6e6fb6b0dd67d68559b101c3e100fb19ebf4db43d9d082674e1";
+        public const string Leaf2Thumbprint = "a8cc70dbbd8bc61410231805b690cca7c5a8d07553c1c49b299a6aabaeb7ff9a";
 
         /// <summary>
         /// This is the SHA-256 thumbprint of the timestamp certificate in <see cref="SignedPackageLeaf1"/>.
@@ -38,6 +52,12 @@ public static class TestResources
         public static SignedPackageArchive SignedPackageLeaf1Reader => LoadPackage(SignedPackageLeaf1);
         public static SignedPackageArchive SignedPackageLeaf2Reader => LoadPackage(SignedPackageLeaf2);
 
+        public static async Task<X509Certificate2> GetTestRootCertificateAsync()
+        {
+            var bytes = await _lazyTestRootCertificate.Value;
+            return new X509Certificate2((byte[])bytes.Clone());
+        }
+
         /// <summary>
         /// Buffer the resource stream into memory so the caller doesn't have to dispose.
         /// </summary>