From 40a67f45987f208f5d67b521c9abd4ab356bc867 Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 25 Jul 2018 15:55:31 -0700 Subject: [PATCH 1/4] Add option to suppress repository signature extractoin --- .../ProcessSignatureConfiguration.cs | 6 +++ .../Settings/dev.json | 3 +- .../Settings/int.json | 3 +- .../Settings/prod.json | 3 +- .../SignaturePartsExtractor.cs | 10 ++++ .../SignaturePartsExtractorFacts.cs | 52 +++++++++++++++++++ .../SignatureValidatorIntegrationTests.cs | 2 + 7 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs b/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs index 4d5bb7d5c..2639b9906 100644 --- a/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs +++ b/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs @@ -20,5 +20,11 @@ public class ProcessSignatureConfiguration /// repository signature is removed. /// public string V3ServiceIndexUrl { get; set; } + + /// + /// Whether repository signatures should be persisted to the database. Disable this if repository signing + /// is in test mode and repository signed packages are not published. + /// + public bool ExtractRepositorySignatures { get; set; } } } diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json b/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json index 11728b8a0..71bd4d37f 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json @@ -22,7 +22,8 @@ "AllowedRepositorySigningCertificates": [ "cf6ce6768ef858a3a667be1af8aa524d386c7f59a34542713f5dfb0d79acf3dd" ], - "V3ServiceIndexUrl": "https://apidev.nugettest.org/v3/index.json" + "V3ServiceIndexUrl": "https://apidev.nugettest.org/v3/index.json", + "ExtractRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/int.json b/src/Validation.PackageSigning.ProcessSignature/Settings/int.json index 1180c0816..ae54d270c 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/int.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/int.json @@ -22,7 +22,8 @@ "AllowedRepositorySigningCertificates": [ "cf6ce6768ef858a3a667be1af8aa524d386c7f59a34542713f5dfb0d79acf3dd" ], - "V3ServiceIndexUrl": "https://apiint.nugettest.org/v3/index.json" + "V3ServiceIndexUrl": "https://apiint.nugettest.org/v3/index.json", + "ExtractRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json b/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json index d195c2287..1e93ab4a9 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json @@ -22,7 +22,8 @@ "AllowedRepositorySigningCertificates": [ "cf7ac17ad047ecd5fdc36822031b12d4ef078b6f2b4c5e6ba41f8ff2cf4bad67" ], - "V3ServiceIndexUrl": "https://api.nuget.org/v3/index.json" + "V3ServiceIndexUrl": "https://api.nuget.org/v3/index.json", + "ExtractRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs index 3eccf8bca..644aa5d6c 100644 --- a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs +++ b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs @@ -8,6 +8,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using NuGet.Jobs.Validation.PackageSigning.Storage; using NuGet.Packaging.Signing; using NuGet.Services.Validation; @@ -18,15 +19,18 @@ public class SignaturePartsExtractor : ISignaturePartsExtractor { private readonly ICertificateStore _certificateStore; private readonly IValidationEntitiesContext _entitiesContext; + private readonly IOptionsSnapshot _configuration; private readonly ILogger _logger; public SignaturePartsExtractor( ICertificateStore certificateStore, IValidationEntitiesContext entitiesContext, + IOptionsSnapshot configuration, ILogger logger) { _certificateStore = certificateStore ?? throw new ArgumentNullException(nameof(certificateStore)); _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext)); + _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); } @@ -227,6 +231,12 @@ private async Task InitializePackageSignatureAndTrustedTimestampAsync( return; } + if (type == PackageSignatureType.Repository && !_configuration.Value.ExtractRepositorySignatures) + { + _logger.LogWarning("Skipping initialization of repository signature due to configuration!"); + return; + } + // Initialize the package signature record. var packageSignature = await InitializePackageSignatureAsync( packageKey, diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs index 0b4077061..f8d2ee962 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs @@ -10,7 +10,9 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Moq; +using NuGet.Jobs.Validation.PackageSigning; using NuGet.Jobs.Validation.PackageSigning.ProcessSignature; using NuGet.Jobs.Validation.PackageSigning.Storage; using NuGet.Packaging.Signing; @@ -116,6 +118,8 @@ public class ExtractAsync private readonly Mock _certificateStore; private readonly List _savedCertificates; private readonly Mock _entitiesContext; + private readonly Mock> _configAccessor; + private readonly ProcessSignatureConfiguration _config; private readonly Mock> _logger; private readonly SignaturePartsExtractor _target; @@ -152,11 +156,20 @@ public ExtractAsync() .Setup(x => x.TrustedTimestamps) .Returns(DbSetMockFactory.Create()); + _configAccessor = new Mock>(); + _config = new ProcessSignatureConfiguration + { + ExtractRepositorySignatures = true + }; + + _configAccessor.Setup(a => a.Value).Returns(_config); + _logger = new Mock>(); _target = new SignaturePartsExtractor( _certificateStore.Object, _entitiesContext.Object, + _configAccessor.Object, _logger.Object); } @@ -616,6 +629,45 @@ public async Task IgnoreExtraCertificates() signature.SignedCms.Certificates.Count + signature.Timestamps.Sum(x => x.SignedCms.Certificates.Count)); } + [Fact] + public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySignatureOnRepositorySignedPackage() + { + // Arrange + var signature = await TestResources.LoadPrimarySignatureAsync(TestResources.RepoSignedPackageLeaf1); + + _entitiesContext + .Setup(x => x.PackageSignatures) + .Returns(DbSetMockFactory.Create()); + + _config.ExtractRepositorySignatures = false; + + // Act + await _target.ExtractAsync(_packageKey, signature, _token); + + // Assert + Assert.Equal(0, _entitiesContext.Object.PackageSignatures.Count()); + } + + [Fact] + public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySignatureOnRepositoryCounterSignedPackage() + { + // Arrange + var signature = await TestResources.LoadPrimarySignatureAsync(TestResources.AuthorAndRepoSignedPackageLeaf1); + + _entitiesContext + .Setup(x => x.PackageSignatures) + .Returns(DbSetMockFactory.Create()); + + _config.ExtractRepositorySignatures = false; + + // Act + await _target.ExtractAsync(_packageKey, signature, _token); + + // Assert + Assert.Equal(1, _entitiesContext.Object.PackageSignatures.Count()); + Assert.Equal(PackageSignatureType.Author, _entitiesContext.Object.PackageSignatures.First().Type); + } + private void AssignIds() { var endCertificates = _entitiesContext.Object.EndCertificates.AsQueryable().ToList(); diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs index 742f2d98c..9981f6b3b 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs @@ -122,6 +122,7 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt _signaturePartsExtractor = new SignaturePartsExtractor( _certificateStore.Object, _validationEntitiesContext.Object, + _optionsSnapshot.Object, loggerFactory.CreateLogger()); _packageFileService = new Mock(); @@ -149,6 +150,7 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt { AllowedRepositorySigningCertificates = new List { "fake-thumbprint" }, V3ServiceIndexUrl = TestResources.V3ServiceIndexUrl, + ExtractRepositorySignatures = true, }; _optionsSnapshot = new Mock>(); _optionsSnapshot.Setup(x => x.Value).Returns(() => _configuration); From aea15e529ba398a9c7a81d4e1ef957797d4eef5f Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 25 Jul 2018 16:04:14 -0700 Subject: [PATCH 2/4] Update config name --- .../ProcessSignatureConfiguration.cs | 2 +- .../Settings/dev.json | 2 +- .../Settings/int.json | 2 +- .../Settings/prod.json | 2 +- .../SignaturePartsExtractor.cs | 2 +- .../SignaturePartsExtractorFacts.cs | 6 +++--- .../SignatureValidatorIntegrationTests.cs | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs b/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs index 2639b9906..ab0fce92b 100644 --- a/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs +++ b/src/Validation.PackageSigning.ProcessSignature/ProcessSignatureConfiguration.cs @@ -25,6 +25,6 @@ public class ProcessSignatureConfiguration /// Whether repository signatures should be persisted to the database. Disable this if repository signing /// is in test mode and repository signed packages are not published. /// - public bool ExtractRepositorySignatures { get; set; } + public bool CommitRepositorySignatures { get; set; } } } diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json b/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json index 71bd4d37f..ecbe7450d 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/dev.json @@ -23,7 +23,7 @@ "cf6ce6768ef858a3a667be1af8aa524d386c7f59a34542713f5dfb0d79acf3dd" ], "V3ServiceIndexUrl": "https://apidev.nugettest.org/v3/index.json", - "ExtractRepositorySignatures": false + "CommitRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/int.json b/src/Validation.PackageSigning.ProcessSignature/Settings/int.json index ae54d270c..3dd7a70fd 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/int.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/int.json @@ -23,7 +23,7 @@ "cf6ce6768ef858a3a667be1af8aa524d386c7f59a34542713f5dfb0d79acf3dd" ], "V3ServiceIndexUrl": "https://apiint.nugettest.org/v3/index.json", - "ExtractRepositorySignatures": false + "CommitRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json b/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json index 1e93ab4a9..2836e7501 100644 --- a/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json +++ b/src/Validation.PackageSigning.ProcessSignature/Settings/prod.json @@ -23,7 +23,7 @@ "cf7ac17ad047ecd5fdc36822031b12d4ef078b6f2b4c5e6ba41f8ff2cf4bad67" ], "V3ServiceIndexUrl": "https://api.nuget.org/v3/index.json", - "ExtractRepositorySignatures": false + "CommitRepositorySignatures": false }, "PackageDownloadTimeout": "00:10:00", diff --git a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs index 644aa5d6c..c5c3e7850 100644 --- a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs +++ b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs @@ -231,7 +231,7 @@ private async Task InitializePackageSignatureAndTrustedTimestampAsync( return; } - if (type == PackageSignatureType.Repository && !_configuration.Value.ExtractRepositorySignatures) + if (type == PackageSignatureType.Repository && !_configuration.Value.CommitRepositorySignatures) { _logger.LogWarning("Skipping initialization of repository signature due to configuration!"); return; diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs index f8d2ee962..9d3797b82 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs @@ -159,7 +159,7 @@ public ExtractAsync() _configAccessor = new Mock>(); _config = new ProcessSignatureConfiguration { - ExtractRepositorySignatures = true + CommitRepositorySignatures = true }; _configAccessor.Setup(a => a.Value).Returns(_config); @@ -639,7 +639,7 @@ public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySig .Setup(x => x.PackageSignatures) .Returns(DbSetMockFactory.Create()); - _config.ExtractRepositorySignatures = false; + _config.CommitRepositorySignatures = false; // Act await _target.ExtractAsync(_packageKey, signature, _token); @@ -658,7 +658,7 @@ public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySig .Setup(x => x.PackageSignatures) .Returns(DbSetMockFactory.Create()); - _config.ExtractRepositorySignatures = false; + _config.CommitRepositorySignatures = false; // Act await _target.ExtractAsync(_packageKey, signature, _token); diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs index 9981f6b3b..7a7ff91a2 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs @@ -150,7 +150,7 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt { AllowedRepositorySigningCertificates = new List { "fake-thumbprint" }, V3ServiceIndexUrl = TestResources.V3ServiceIndexUrl, - ExtractRepositorySignatures = true, + CommitRepositorySignatures = true, }; _optionsSnapshot = new Mock>(); _optionsSnapshot.Setup(x => x.Value).Returns(() => _configuration); From 291f2df2e28ba5267007aab9c0bcb78e8a76cdee Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 25 Jul 2018 16:10:34 -0700 Subject: [PATCH 3/4] Address feedback --- .../SignaturePartsExtractor.cs | 2 +- .../SignaturePartsExtractorFacts.cs | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs index c5c3e7850..7fb6056bd 100644 --- a/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs +++ b/src/Validation.PackageSigning.ProcessSignature/SignaturePartsExtractor.cs @@ -233,7 +233,7 @@ private async Task InitializePackageSignatureAndTrustedTimestampAsync( if (type == PackageSignatureType.Repository && !_configuration.Value.CommitRepositorySignatures) { - _logger.LogWarning("Skipping initialization of repository signature due to configuration!"); + _logger.LogInformation("Skipping initialization of repository signature due to configuration!"); return; } diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs index 9d3797b82..885f83c4c 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignaturePartsExtractorFacts.cs @@ -64,7 +64,7 @@ public class SignaturePartsExtractorFacts "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), }, - TimestampEndCertificate = new SubjectAndThumbprint( + TimestampEndCertificate = new SubjectAndThumbprint( "CN=Symantec SHA256 TimeStamping Signer - G2, OU=Symantec Trust Network, O=Symantec Corporation, C=US", TestResources.Leaf1TimestampThumbprint), TimestampParentCertificates = new[] @@ -646,6 +646,9 @@ public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySig // Assert Assert.Equal(0, _entitiesContext.Object.PackageSignatures.Count()); + + // The repository signature's certificate is still stored on blob storage. + VerifyStoredCertificates(Leaf1Certificates); } [Fact] @@ -666,6 +669,9 @@ public async Task IfRepositorySignatureExtractionIsDisabled_IgnoresRepositorySig // Assert Assert.Equal(1, _entitiesContext.Object.PackageSignatures.Count()); Assert.Equal(PackageSignatureType.Author, _entitiesContext.Object.PackageSignatures.First().Type); + + // The repository signature's certificate is still stored on blob storage. + VerifyStoredCertificates(AuthorAndRepoSignedCertificates); } private void AssignIds() From 52a2d8f64790241eb283899c90cb7057cdcb604b Mon Sep 17 00:00:00 2001 From: Loic Sharma Date: Wed, 25 Jul 2018 16:23:47 -0700 Subject: [PATCH 4/4] Fix tests --- .../SignatureValidatorIntegrationTests.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs index 7a7ff91a2..a4d899a8a 100644 --- a/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs +++ b/tests/Validation.PackageSigning.ProcessSignature.Tests/SignatureValidatorIntegrationTests.cs @@ -119,6 +119,17 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt _certificateStore = new Mock(); + // These dependencies are concrete. + _configuration = new ProcessSignatureConfiguration + { + AllowedRepositorySigningCertificates = new List { "fake-thumbprint" }, + V3ServiceIndexUrl = TestResources.V3ServiceIndexUrl, + CommitRepositorySignatures = true, + }; + _optionsSnapshot = new Mock>(); + _optionsSnapshot.Setup(x => x.Value).Returns(() => _configuration); + _formatValidator = new SignatureFormatValidator(_optionsSnapshot.Object); + _signaturePartsExtractor = new SignaturePartsExtractor( _certificateStore.Object, _validationEntitiesContext.Object, @@ -145,17 +156,6 @@ public SignatureValidatorIntegrationTests(CertificateIntegrationTestFixture fixt _corePackageService = new Mock(); - // These dependencies are concrete. - _configuration = new ProcessSignatureConfiguration - { - AllowedRepositorySigningCertificates = new List { "fake-thumbprint" }, - V3ServiceIndexUrl = TestResources.V3ServiceIndexUrl, - CommitRepositorySignatures = true, - }; - _optionsSnapshot = new Mock>(); - _optionsSnapshot.Setup(x => x.Value).Returns(() => _configuration); - _formatValidator = new SignatureFormatValidator(_optionsSnapshot.Object); - _telemetryClient = new Mock(); _telemetryService = new TelemetryService(_telemetryClient.Object);