Add integration tests for CertificateValidationMessageHandler #341

loic-sharma · 2018-02-16T01:50:42Z

No description provided.

loic-sharma · 2018-02-16T01:51:55Z

src/Validation.PackageSigning.ValidateCertificate/Primitives.cs

@@ -246,7 +246,7 @@ internal struct CERT_TRUST_STATUS
    [StructLayout(LayoutKind.Sequential)]
    internal unsafe struct CRL_ENTRY
    {
-        public CRYPTOAPI_BLOB SerializeNumber;
+        public CRYPTOAPI_BLOB SerialNumber;


This is the proper name for the property (docs here).

loic-sharma · 2018-02-16T01:52:55Z

src/Validation.PackageSigning.ValidateCertificate/SignatureDeciderFactory.cs

@@ -107,6 +107,12 @@ private SignatureDecider MakeDeciderForRevokedCodeSigningCertificate(DateTime? r
                    throw new InvalidOperationException($"Signature {signature.Key} has multiple trusted timestamps");
                }

+                // Revoked certificates invalidate all dependent signatures at ingestion.
+                if (signature.Status == PackageSignatureStatus.Unknown)


This is a bug fix. We do not accept signatures with revoked certificates at ingestion, even if the revocation date is after the signing time.

loic-sharma · 2018-02-16T01:54:48Z

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

@@ -52,7 +52,12 @@ public CertificateIntegrationTestFixture()

        public Task<SigningTestServer> GetTestServerAsync() => _testServer.Value;
        public Task<Uri> GetTimestampServiceUrlAsync() => _timestampServiceUrl.Value;
-        public Task<X509Certificate2> GetSigningCertificateAsync() => _signingCertificate.Value;
+
+        public async Task<X509Certificate2> GetSigningCertificateAsync()


Before, this certificate was shared and would be disposed by the message handler - this caused weird issues in integration tests. A new X509Certificate2 is created now, so you can dispose it willy-nilly without affecting other tests.

loic-sharma · 2018-02-16T01:55:32Z

tests/Validation.PackageSigning.ValidateCertificate.Tests/SignatureDeciderFactoryFacts.cs

@@ -230,7 +230,7 @@ public static IEnumerable<object[]> RevokedCodeSigningCertificateWithRevocationD
            {
                yield return new object[]
                {
-                    TimeSpan.FromDays(-1), SignatureDecision.Ignore, SignatureDecision.Ignore, SignatureDecision.Ignore
+                    TimeSpan.FromDays(-1), SignatureDecision.Reject, SignatureDecision.Ignore, SignatureDecision.Ignore


Signatures should be rejected at ingestion time if their certificate is revoked, regardless of the revocation time.

loic-sharma · 2018-02-16T02:02:09Z

...dation.PackageSigning.ValidateCertificate.Tests/Support/CertificateIntegrationTestFixture.cs

            var ca = await GetCertificateAuthority();
+            var ca2 = ca.CreateIntermediateCertificateAuthority();


Some weird caching of certificates is happening. Windows is sometimes able to build a chain without the CA Issuer attribute if the intermediate certificate has been seen before.

With this change, a new intermediate CA with no CA responders is issued every time a new end certificate is requested. Since the intermediate CA's certificate has never seen before, caching certificates cannot help build the chain.

dtivel · 2018-02-16T20:03:40Z

...kageSigning.ValidateCertificate.Tests/CertificateValidationMessageHandlerIntegrationTests.cs

+    {
+        public static readonly DateTime RevocationTime = DateTime.UtcNow.AddDays(-4);
+        public static readonly DateTime BeforeRevocationTime = RevocationTime.AddDays(-1);
+        public static readonly DateTime AfterRevocationTime = RevocationTime.AddDays(1);


Can these be private?

Also, I'm not sure what the default validity period is for your certificates, but I recommend minimizing the validity period of your test certificates.

Yup, I'll lower the visibility.

We will need to update Test.Utility APIs to customize the "not after" date. I think I'll do this in a separate PR once a newer version has been released.

dtivel · 2018-02-16T20:34:21Z

...kageSigning.ValidateCertificate.Tests/CertificateValidationMessageHandlerIntegrationTests.cs

+            var certificateValidationService = new CertificateValidationService(
+                _context.Object,
+                _telemetryService.Object,
+                new Mock<ILogger<CertificateValidationService>>().Object);


There's a shorthand for this:

Mock.Of<ILogger<CertificateValidationService>>()

Thanks for the suggestion, I didn't know about that :)

dtivel · 2018-02-16T20:52:34Z

...dation.PackageSigning.ValidateCertificate.Tests/Support/CertificateIntegrationTestFixture.cs

+
+            var (publicCertificate, certificate) = IssueCertificate(ca, "Revoked Timestamping", CustomizeAsSigningCertificate);
+
+            ca.Revoke(publicCertificate, reason: 0, revocationDate: revocationDate);


Revoke(...)'s second parameter is an int, because BouncyCastle does not have an enum for revocation reasons. However, BouncyCastle does have a class (Org.BouncyCastle.Asn1.X509.CrlReason) with some predefined revocation reasons. I could have created my own enum and map between the two, and maybe I should still do that. However, without that, I recommend passing CrlReason.Unspecified (0) or some other field instead of passing a constant literal 0, which is frustratingly obtuse.

I'll go ahead and use a const for this. The Test.Utility package has an enum for the reason now - after the latest version is released, we will switch over to the enum.

dtivel · 2018-02-16T21:03:13Z

...dation.PackageSigning.ValidateCertificate.Tests/Support/CertificateIntegrationTestFixture.cs

            var ca = await GetCertificateAuthority();
+            var ca2 = ca.CreateIntermediateCertificateAuthority();
+
+            responders.AddRange(testServer.RegisterResponders(ca2, addCa: false));


This is a suboptimal use of a IHttpResponder's. SigningTestServer.RegisterResponder(...) returns an IDisposable, which deregisters an IHttpResponder on disposal. Registered responders should be scoped as narrowly as possible. For the default, trusted chain (root, intermediate, end), it's reasonable to scope the responders for the lifetime of your test fixture. In all other cases, the registered responders should be deterministically disposed by each test, ideally with a using statement. Here is an example of a responder that is scoped to a single test.

I think this should be scoped at the test level, since this method always creates a new, single-use ca2 and end certificate anyway.

Fixed. I went ahead and made a return type too as the tuple wasn't the prettiest.

dtivel · 2018-02-16T21:04:38Z

...dation.PackageSigning.ValidateCertificate.Tests/Support/CertificateIntegrationTestFixture.cs


-            ca.Revoke(publicCertificate, reason: 0, revocationDate: DateTimeOffset.UtcNow);
+            ca2.Revoke(publicCertificate, reason: 0, revocationDate: DateTimeOffset.UtcNow);


Same feedback here for reason.

joelverhagen · 2018-02-20T17:17:42Z

...dation.PackageSigning.ValidateCertificate.Tests/Support/CertificateIntegrationTestFixture.cs

+            public X509Certificate2 EndCertificate { get; }
+            public X509Certificate2[] IntermediateCertificates { get; }
+
+            public void Dispose() { /*=> _responders.Dispose()*/; }


Should this be commented out? Or removed?

Good catch - this should not be commented out.

Progress on NuGet/NuGetGallery#6317.

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

Progress on NuGet/NuGetGallery#6317.

* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.

loic-sharma self-assigned this Feb 16, 2018

loic-sharma requested review from joelverhagen, xavierdecoster, dtivel and skofman1 February 16, 2018 01:50

loic-sharma commented Feb 16, 2018

View reviewed changes

dtivel approved these changes Feb 16, 2018

View reviewed changes

joelverhagen reviewed Feb 20, 2018

View reviewed changes

joelverhagen approved these changes Feb 20, 2018

View reviewed changes

Add integration tests for CertificateValidationMessageHandler

5f810f0

loic-sharma force-pushed the loshar-integration-tests branch from 2ed6ed2 to 5f810f0 Compare February 20, 2018 18:01

loic-sharma merged commit ffed70e into dev Feb 20, 2018

loic-sharma deleted the loshar-integration-tests branch February 20, 2018 21:30

joelverhagen pushed a commit that referenced this pull request Sep 27, 2019

Test: add registration tests (#341)

1907ccb

Progress on NuGet/NuGetGallery#6317.

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Add integration tests for CertificateValidationMessageHandler (#341)

7d341f6

joelverhagen pushed a commit that referenced this pull request Oct 26, 2020

Test: add registration tests (#341)

dc6d1b5

Progress on NuGet/NuGetGallery#6317.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests for CertificateValidationMessageHandler #341

Add integration tests for CertificateValidationMessageHandler #341

loic-sharma commented Feb 16, 2018

loic-sharma Feb 16, 2018

loic-sharma Feb 16, 2018

loic-sharma Feb 16, 2018

loic-sharma Feb 16, 2018

loic-sharma Feb 16, 2018 •

edited

dtivel Feb 16, 2018

loic-sharma Feb 17, 2018

dtivel Feb 16, 2018

loic-sharma Feb 17, 2018

dtivel Feb 16, 2018

loic-sharma Feb 17, 2018 •

edited

loic-sharma Feb 17, 2018

dtivel Feb 16, 2018 •

edited

loic-sharma Feb 17, 2018

dtivel Feb 16, 2018

loic-sharma Feb 17, 2018

joelverhagen Feb 20, 2018

loic-sharma Feb 20, 2018

loic-sharma Feb 20, 2018

		var ca = await GetCertificateAuthority();
		var ca2 = ca.CreateIntermediateCertificateAuthority();


		var (publicCertificate, certificate) = IssueCertificate(ca, "Revoked Timestamping", CustomizeAsSigningCertificate);

		ca.Revoke(publicCertificate, reason: 0, revocationDate: revocationDate);


		ca.Revoke(publicCertificate, reason: 0, revocationDate: DateTimeOffset.UtcNow);
		ca2.Revoke(publicCertificate, reason: 0, revocationDate: DateTimeOffset.UtcNow);

Add integration tests for CertificateValidationMessageHandler #341

Add integration tests for CertificateValidationMessageHandler #341

Conversation

loic-sharma commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtivel Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Feb 16, 2018 •

edited

loic-sharma Feb 17, 2018 •

edited

dtivel Feb 16, 2018 •

edited