Do not fail verification if owners don't match (#2550)

NuGet · Dec 7, 2018 · e2c545a · e2c545a
1 parent f5e7eab
commit e2c545a
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 6 deletions.
diff --git a/src/NuGet.Core/NuGet.Packaging/Signing/Verification/AllowListVerificationProvider.cs b/src/NuGet.Core/NuGet.Packaging/Signing/Verification/AllowListVerificationProvider.cs
@@ -86,10 +86,15 @@ private bool IsSignatureAllowed(
                         {
                             if (ShouldVerifyOwners(certificateHashEntry as TrustedSignerAllowListEntry, signature as IRepositorySignature, out var allowedOwners, out var actualOwners))
                             {
-                                return allowedOwners.Intersect(actualOwners).Any();
+                                if (allowedOwners.Intersect(actualOwners).Any())
+                                {
+                                    return true;
+                                }
+                            }
+                            else
+                            {
+                                return true;
                             }
-
-                            return true;
                         }
                     }
 
@@ -108,10 +113,15 @@ private bool IsSignatureAllowed(
                             {
                                 if (ShouldVerifyOwners(certificateHashEntry as TrustedSignerAllowListEntry, repositoryCountersignature.Value as IRepositorySignature, out var allowedOwners, out var actualOwners))
                                 {
-                                    return allowedOwners.Intersect(actualOwners).Any();
+                                    if (allowedOwners.Intersect(actualOwners).Any())
+                                    {
+                                        return true;
+                                    }
+                                }
+                                else 
+                                {
+                                    return true;
                                 }
-
-                                return true;
                             }
                         }
                     }

diff --git a/...ore.FuncTests/NuGet.Packaging.FuncTest/SigningTests/AllowListVerificationProviderTests.cs b/...ore.FuncTests/NuGet.Packaging.FuncTest/SigningTests/AllowListVerificationProviderTests.cs
@@ -858,6 +858,61 @@ public async Task GetTrustResultAsync_RepositoryCountersignedPackage_PackageSign
             }
         }
 
+
+        [CIOnlyFact]
+        public async Task GetTrustResultAsync_RepositoryCountersignedPackage_PackageSignedWithCertFromAllowList_WithOwnerNotInOwnersList_AuthorInList_RequireMode_SuccessAsync()
+        {
+            var nupkg = new SimpleTestPackageContext();
+
+            // Arrange
+            using (var dir = TestDirectory.Create())
+            using (var primaryCertificate = new X509Certificate2(_trustedAuthorTestCert.Source.Cert))
+            using (var counterCertificate = new X509Certificate2(_trustedRepoTestCert.Source.Cert))
+            {
+                var hashAlgorithmName = HashAlgorithmName.SHA256;
+                var authorFingerprint = SignatureTestUtility.GetFingerprint(primaryCertificate, hashAlgorithmName);
+                var repositoryFingerprint = SignatureTestUtility.GetFingerprint(counterCertificate, hashAlgorithmName);
+
+                var signedPackagePath = await SignedArchiveTestUtility.AuthorSignPackageAsync(primaryCertificate, nupkg, dir);
+                var countersignedPackagePath = await SignedArchiveTestUtility.RepositorySignPackageAsync(
+                    counterCertificate,
+                    signedPackagePath,
+                    dir,
+                    v3ServiceIndex: new Uri("https://v3serviceIndex.test/api/index.json"),
+                    timestampService: null,
+                    packageOwners: new List<string>() { "owner4", "owner5" });
+
+                var allowList = new List<CertificateHashAllowListEntry>()
+                {
+                    new TrustedSignerAllowListEntry(VerificationTarget.Repository, SignaturePlacement.Any, repositoryFingerprint, hashAlgorithmName, owners: new List<string>() { "owner1", "owner2", "owner3" }),
+                    new TrustedSignerAllowListEntry(VerificationTarget.Author, SignaturePlacement.PrimarySignature, authorFingerprint, hashAlgorithmName)
+                };
+
+                var signedPackageVerifierSettings = SignedPackageVerifierSettings.GetRequireModeDefaultPolicy();
+                var signedPackageVerifier = new PackageSignatureVerifier(new ISignatureVerificationProvider[]
+                {
+                    new AllowListVerificationProvider(allowList, requireNonEmptyAllowList: true)
+                });
+
+                using (var packageReader = new PackageArchiveReader(countersignedPackagePath))
+                {
+                    // Act
+                    var result = await signedPackageVerifier.VerifySignaturesAsync(packageReader, signedPackageVerifierSettings, CancellationToken.None);
+                    var resultsWithErrors = result.Results.Where(r => r.GetErrorIssues().Any());
+                    var resultsWithWarnings = result.Results.Where(r => r.GetWarningIssues().Any());
+                    var totalErrorIssues = resultsWithErrors.SelectMany(r => r.GetErrorIssues());
+                    var totalWarningIssues = resultsWithWarnings.SelectMany(r => r.GetWarningIssues());
+
+                    // Assert
+                    result.IsValid.Should().BeTrue();
+                    resultsWithErrors.Count().Should().Be(0);
+                    resultsWithWarnings.Count().Should().Be(0);
+                    totalErrorIssues.Count().Should().Be(0);
+                    totalWarningIssues.Count().Should().Be(0);
+                }
+            }
+        }
+
         [CIOnlyFact]
         public async Task GetTrustResultAsync_RepositoryCountersignedPackage_PackageSignedWithCertFromAllowList_WithNoOwnersInPackage_RequireMode_ErrorAsync()
         {