Refactor signature object to add support for repository countersignatures #2006

PatoBeltran · 2018-02-06T22:13:29Z

This work is being done in multiple PRs to avoid having one huge and difficult PR to review.

This PR refactors the current signature object to be able to have repository primary signatures, repository counter signatures in addition to the current author primary signatures.

mishra14 · 2018-02-06T23:41:23Z

Had an offline discussion with @PatoBeltran.

We should consider the following approach -
Have the class structure as it is in the PR i.e. AuthorPrimarySignature implements Signature etc.
But in such a case, we should consider moving the verification logic to the individual classes to warrant such a finer control on the types.
For instance, Signature should be an abstract class and implement all the common verification logic in Signature.Verify() and the child classes can then override the base verify method to add extra verification and call base.Verify().

Or if we want to keep the implementation simple then -
Signature should represent the complete signature of a package.
Signature.Type will then indicate if this is a repo or an author signature. It should contain a counter signature object - Signature.CounterSignature of type Signature.
That will satisfy the requirements as well, while keeping this simpler.

dtivel · 2018-02-07T16:42:12Z

src/NuGet.Core/NuGet.Common/Errors/NuGetLogCode.cs

@@ -364,6 +364,11 @@ public enum NuGetLogCode
        /// </summary>
        NU3029 = 3029,

+        /// <summary>
+        /// The package signature cointains multiple repository countersignatures.


typo: contains

dtivel · 2018-02-07T16:43:39Z

src/NuGet.Core/NuGet.Packaging/Signing/Authoring/X509SignatureProvider.cs

        {
            throw new NotSupportedException();
        }

-        private Task<Signature> TimestampSignature(SignPackageRequest request, ILogger logger, Signature signature, CancellationToken token)
+        private Task<PrimarySignature> TimestampPrimarySignature(SignPackageRequest request, ILogger logger, PrimarySignature signature, CancellationToken token)


TimestampPrimarySignatureAsync

dtivel · 2018-02-07T16:45:24Z

src/NuGet.Core/NuGet.Packaging/Signing/Authoring/X509SignatureProvider.cs

        }

-        private Task<Signature> TimestampSignature(SignPackageRequest request, ILogger logger, Signature signature, CancellationToken token)
+        private Task<PrimarySignature> TimestampPrimarySignature(SignPackageRequest request, ILogger logger, PrimarySignature signature, CancellationToken token)


TimestampPrimarySignatureAsync

dtivel · 2018-02-07T16:45:52Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/AuthorPrimarySignature.cs

+
+namespace NuGet.Packaging.Signing
+{
+    public class AuthorPrimarySignature : PrimarySignature


dtivel · 2018-02-07T16:49:49Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryPrimarySignature.cs

+
+namespace NuGet.Packaging.Signing
+{
+    public class RepositoryPrimarySignature : PrimarySignature, IRepositorySignature


dtivel · 2018-02-07T17:03:22Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryCounterSignature.cs

+                    potentialNuGetV3ServiceIndexUrl = AttributeUtility.GetNuGetV3ServiceIndexUrl(counterSignature.SignedAttributes);
+                }
+                // This counter signature is not a valid repository countersignature
+                catch (SignatureException) { }


After you determine it's a repository countersignature, do not catch this exception. Let the exception bubble up to indicate it's an invalid repository countersignature.

dtivel · 2018-02-07T17:03:48Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryCounterSignature.cs

+                throw new SignatureException(NuGetLogCode.NU3030, Strings.Error_NotOneRepositoryCounterSignature);
+            }
+
+            var respoSignature = repositoryCounterSignatures.FirstOrDefault();


typo: repoCountersignature

dtivel · 2018-02-07T17:09:08Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryCounterSignature.cs

+        public static RepositoryCounterSignature GetRepositoryCounterSignature(PrimarySignature primarySignature)
+        {
+            var counterSignatures = primarySignature.SignerInfo.CounterSignerInfos;
+            var repositoryCounterSignatures = new List<SignerInfo>();


You don't need a list.

Create a local:

RepositoryCounterSignature repositoryCountersignature = null;

When examining countersignatures, assign this local only if null; otherwise throw.

dtivel · 2018-02-07T17:10:20Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/UnknownPrimarySignature.cs

+
+namespace NuGet.Packaging.Signing
+{
+    public class UnknownPrimarySignature : PrimarySignature


dtivel · 2018-02-07T18:18:40Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/AuthorPrimarySignature.cs

+            VerifySigningTimeAttribute(SignerInfo);
+        }
+
+        private static void VerifySigningTimeAttribute(SignerInfo signerInfo)


This verification applies to author signatures, repository signatures and repository countersignatures.

…ures

PatoBeltran · 2018-02-07T22:12:18Z

@mishra14 @dtivel pushed some new commits, rebased from dev and addressed feedback 😄

PatoBeltran · 2018-02-07T22:17:06Z

src/NuGet.Core/NuGet.Packaging/Signing/Utility/CertificateUtility.cs

@@ -28,7 +28,7 @@ public static class CertificateUtility
        public static string X509Certificate2ToString(X509Certificate2 cert, HashAlgorithmName fingerprintAlgorithm)
        {
            var certStringBuilder = new StringBuilder();
-            X509Certificate2ToString(cert, certStringBuilder, fingerprintAlgorithm, indentation: "");
+            X509Certificate2ToString(cert, certStringBuilder, fingerprintAlgorithm, indentation: "  ");


I added indentation to all certs to match the output from signtool, see example in NuGet/Home#6005 (comment)

PatoBeltran · 2018-02-07T22:18:48Z

...t.Core/NuGet.Packaging/Signing/Verification/SignatureTrustAndValidityVerificationProvider.cs

-                                string.Join(", ", chainStatusList.Select(x => x.Status.ToString())))));
-                }
-            }
+            signatureIssues.AddRange(timestampIssues);


This is to display first the details of the signature and afterwards for its timestamp (if present). This matches signtool and makes more sense to display first the signature. (example output of signtool NuGet/Home#6005 (comment))

dtivel · 2018-02-08T00:01:42Z

src/NuGet.Core/NuGet.Common/Errors/NuGetLogCode.cs

+        NU3032 = 3032,
+
+        /// <summary>
+        /// A repository primary signature should not have a repository countersignatures.


A repository primary signature must not have a repository countersignature.

dtivel · 2018-02-08T23:43:44Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/AuthorPrimarySignature.cs

+        {
+        }
+
+        internal override SignatureVerificationStatus Verify(


Can't we just pass a settings object instead of a bunch of flags? It's easier to maintain over time.

dtivel · 2018-02-08T23:43:59Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/AuthorPrimarySignature.cs

+        internal override SignatureVerificationStatus Verify(
+            Timestamp timestamp,
+            bool allowUntrusted,
+            bool allowUntrustedSelfSignedCertificate,


allowUntrustedSelfIssuedCertificate

dtivel · 2018-02-08T23:44:32Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/IRepositorySignature.cs

+#if IS_DESKTOP
+        Uri NuGetV3ServiceIndexUrl { get; }
+
+        IReadOnlyList<string> NuGetPackageOwners { get; }


PackageOwners

dtivel · 2018-02-08T23:44:45Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/IRepositorySignature.cs

+    public interface IRepositorySignature
+    {
+#if IS_DESKTOP
+        Uri NuGetV3ServiceIndexUrl { get; }


V3ServiceIndexUrl

dtivel · 2018-02-08T23:51:47Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryCounterSignature.cs

+            ThrowForInvalidRepositoryCounterSignature();
+        }
+
+        private static void ThrowForInvalidRepositoryCounterSignature()


You don't need this method. Just move the method body into ThrowForInvalidSignature().

dtivel · 2018-02-08T23:52:58Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/RepositoryPrimarySignature.cs

+            X509Certificate2Collection certificateExtraStore,
+            List<SignatureLog> issues)
+        {
+            issues?.Add(SignatureLog.InformationLog(string.Format(CultureInfo.CurrentCulture, Strings.SignatureType, Type.ToString())));


Why allow issues to be null here and in other files?

I though it was a good idea to allow in the case you want to have the verification but don't care about logs, because even if there are no issues the same validation will be performed and you would get the same result. Do you think we should enforce the existence of issues?

dtivel · 2018-02-08T23:53:53Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/SigningTests/SignatureTests.cs

@@ -29,7 +29,7 @@ public SignatureTests(CertificatesFixture fixture)
        public void Load_WhenDataNull_Throws()
        {
            var exception = Assert.Throws<ArgumentNullException>(
-                () => Signature.Load(data: null));
+                () => PrimarySignature.Load(data: null));


Rename file to PrimarySignatureTests.

dtivel · 2018-02-08T23:56:17Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/SigningTests/SignatureUtilityTests.cs

@@ -80,25 +80,25 @@ public void GetPrimarySignatureCertificates_WithAuthorSignature_ReturnsCertifica
        public void GetPrimarySignatureTimestampSignatureCertificates_WhenSignatureNull_Throws()
        {
            var exception = Assert.Throws<ArgumentNullException>(
-                () => SignatureUtility.GetPrimarySignatureTimestampSignatureCertificates(signature: null));
+                () => SignatureUtility.GetPrimarySignatureTimestampCertificates(signature: null));


This test (and others) were testing the method GetPrimarySignatureTimestampSignatureCertificates(...). The method name begins with the name of the member under test. Since you renamed the name of the member under test, update the test name too. This feedback applies globally.

<MemberName>_<TestCondition>_<ExpectedResult>

dtivel · 2018-02-08T23:58:31Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/PrimarySignature.cs

+            ThrowForInvalidPrimarySignature();
+        }
+
+        protected static void ThrowForInvalidPrimarySignature()


Not sure this method is needed. Just move the body into ThrowForInvalidSignature().

I decided to have this method not as part of ThrowForInvalidSignature() because it is usefull as a static method in some of the static methods in this class, and because ThrowForInvalidSignature() is an abstract method it cannot be static. Therefore I decided to have another method that is static with the desired implementation and just let ThrowForInvalidSignature() call this method.

mishra14

Consider moving signed cms into signature.... otherwise looks good.

mishra14 · 2018-02-09T01:14:50Z

src/NuGet.Core/NuGet.Packaging/Signing/Signatures/PrimarySignature.cs

+        /// <summary>
+        /// A SignedCms object holding the signature and SignerInfo.
+        /// </summary>
+        public SignedCms SignedCms { get; }


This can be moved into Signature, since all signatures need to atleast be a SignedCms

Countersignatures are represented as SignerInfo, so they don't have their own SignedCms only Primary signatures will have it, that's why I choose to leave it in this class. If we see the need for the primary signature's SignedCms in the countersignature we should consider moving it, but in the mean time I think it should stay here.

PatoBeltran self-assigned this Feb 6, 2018

PatoBeltran requested review from rohit21agrawal, nkolev92, emgarten, zhili1208, mishra14, dtivel and jainaashish February 6, 2018 22:13

PatoBeltran mentioned this pull request Feb 7, 2018

Signing: fix incorrect timestamp hash verification #2010

Merged

dtivel reviewed Feb 7, 2018

View reviewed changes

PatoBeltran added 4 commits February 7, 2018 13:13

Refactor signature object to add support for repository countersignat…

139f67c

…ures

move verification logic to signature classes

d8a665c

Add null checks and update timestamp integrity check

68f3f07

Address feedback

5b0840c

PatoBeltran force-pushed the dev-patobeltran-signing-repo-first branch from 8334989 to 5b0840c Compare February 7, 2018 22:11

PatoBeltran commented Feb 7, 2018

View reviewed changes

fix failing test

6b84d8d

dtivel approved these changes Feb 9, 2018

View reviewed changes

address feedback

8105baf

PatoBeltran force-pushed the dev-patobeltran-signing-repo-first branch from cb14d1f to 8105baf Compare February 9, 2018 00:40

enforce issues

b54815a

mishra14 approved these changes Feb 9, 2018

View reviewed changes

PatoBeltran merged commit 894388a into dev Feb 9, 2018

PatoBeltran deleted the dev-patobeltran-signing-repo-first branch February 9, 2018 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor signature object to add support for repository countersignatures #2006

Refactor signature object to add support for repository countersignatures #2006

PatoBeltran commented Feb 6, 2018

mishra14 commented Feb 6, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

dtivel Feb 7, 2018

PatoBeltran commented Feb 7, 2018

PatoBeltran Feb 7, 2018

PatoBeltran Feb 7, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

PatoBeltran Feb 9, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

dtivel Feb 8, 2018

PatoBeltran Feb 9, 2018

mishra14 left a comment

mishra14 Feb 9, 2018

PatoBeltran Feb 9, 2018

Refactor signature object to add support for repository countersignatures #2006

Refactor signature object to add support for repository countersignatures #2006

Conversation

PatoBeltran commented Feb 6, 2018

mishra14 commented Feb 6, 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

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

PatoBeltran commented Feb 7, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment