Update verify command #1825

PatoBeltran · 2017-11-29T01:25:50Z

Fixes: NuGet/Home#6181
Fixes: NuGet/Home#6073
Fixes: NuGet/Home#6069
Refactored code, added missing verification steps, added more detailed
printing for verbose and debug mode, cleaned up strings

Also
Fixes: NuGet/Home#6212

Functional tests for verify flow are missing

dnfclas · 2017-11-29T01:25:55Z

@PatoBeltran,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

emgarten · 2017-11-29T19:18:12Z

src/NuGet.Clients/NuGet.CommandLine/NuGetCommand.resx

@@ -5375,7 +5375,7 @@ nuget verify-Signatures  .\*.nupkg</value>
    <comment>Please don't localize this string</comment>
  </data>
  <data name="VerifyCommandSignaturesDescription" xml:space="preserve">
-    <value>Specifies the type of verification to be done. Currently only -Signatures verification is supported.</value>
+    <value>Specifies that a verification of the signature of the package(s) should be performed.</value>


Would this be more clear? Specifies that package signature verification should be performed

emgarten · 2017-11-29T19:23:24Z

src/NuGet.Core/NuGet.Commands/Strings.resx

  </data>
  <data name="SignCommandOutputPath" xml:space="preserve">
    <value>Signed package(s) output path: {0}</value>
    <comment>0 - output directory path</comment>
  </data>
+  <data name="Error_CommandFailedWithException" xml:space="preserve">
+    <value>NuGet Command failed. {0} threw exception: {1}</value>


I would avoid threw exception here, just print the message from the exception.

If this is debug only then the rule is: don't localize debug strings (if we ask someone to send us diag output we shouldn't have to translate it, and diag output isn't really for the user)

emgarten · 2017-11-29T20:43:36Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs

-            var packagesToVerify = LocalFolderUtility.ResolvePackageFromPath(verifyArgs.PackagePath);
-            LocalFolderUtility.EnsurePackageFileExists(verifyArgs.PackagePath, packagesToVerify);
+                var packagesToVerify = LocalFolderUtility.ResolvePackageFromPath(verifyArgs.PackagePath);
+                LocalFolderUtility.EnsurePackageFileExists(verifyArgs.PackagePath, packagesToVerify);


Glad to see these APIs being used here 🥇 🏆

emgarten · 2017-11-29T20:49:00Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs

+                    catch (InvalidDataException e)
+                    {
+                        verifyArgs.Logger.LogError(string.Format(CultureInfo.CurrentCulture, Strings.VerifyCommand_PackageIsNotValid, package));
+                        verifyArgs.Logger.LogDebug(string.Format(CultureInfo.CurrentCulture, Strings.Error_CommandFailedWithException, nameof(VerifySignatureForPackageAsync), e.Message));


Don't localize this just log the full stack if it would be useful, also use ExceptionUtilities if it makes sense here

emgarten · 2017-11-29T20:52:50Z

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

-                SignedPackageArchiveUtility.VerifySignedZipIntegrity(reader, hashAlgorithm, expectedHash);
+                if (!SignedPackageArchiveUtility.VerifySignedZipIntegrity(reader, hashAlgorithm, expectedHash))
+                {
+                    throw new SignatureException(Strings.SignatureFailurePackageTampered);


Include the path if possible. This could come from checking if the stream is a FileStream and casting it to get the path. During a large restore with hundreds of packages it can be really confusing if there is a generic error message that doesn't specify the source.

emgarten · 2017-11-29T20:55:06Z

src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveIOUtility.cs

+            {
+                return false;
+            }
+
            for (var i = 0; i < byteSignature.Length; ++i)
            {
                var @byte = stream.ReadByte();


just use b for byte instead of an @

emgarten · 2017-11-29T20:56:44Z

src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveIOUtility.cs

+                throw new Exception(Strings.ErrorByteSignatureTooBig);
+            }
+
+            while (stream.Position != 0)


I'm curious how perf is for this. It seems like this should use a buffer and check within the buffer instead of going byte by byte. This can be profiled later, it shouldn't be changed in this PR

emgarten · 2017-11-29T22:51:20Z

src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveIOUtility.cs

+                metadata.IsZip64 = true;
+                metadata.Zip64EndOfCentralDirectoryRecordPosition = reader.BaseStream.Position;
+            }
+            catch { }


Add a comment for why this is ignored

emgarten · 2017-11-29T22:57:26Z

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

+            certStringBuilder.AppendLine($"{indentation}{string.Format(Strings.CertUtilityCertificateSubjectName, cert.Subject)}");
+            certStringBuilder.AppendLine($"{indentation}{string.Format(Strings.CertUtilityCertificateHash, cert.Thumbprint)}");
+            certStringBuilder.AppendLine($"{indentation}{string.Format(Strings.CertUtilityCertificateIssuer, cert.IssuerName.Name)}");
+            certStringBuilder.AppendLine($"{indentation}{string.Format(Strings.CertUtilityCertificateValidity, cert.NotBefore, cert.NotAfter)}");


String format needs to use CurrentCulture for user facing strings (this applies everywhere)

emgarten · 2017-11-29T22:58:03Z

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

@@ -51,7 +51,7 @@ private static void X509Certificate2ToString(X509Certificate2 cert, StringBuilde
        /// </summary>
        /// <param name="certCollection">X509Certificate2Collection to be converted to string.</param>
        /// <returns>string representation of the X509Certificate2Collection.</returns>
-        public static string X509Certificate2CollectionToString(X509Certificate2Collection certCollection)
+        public static string X509Certificate2CollectionToString(X509Certificate2Collection certCollection, string indentation = "")


avoid default parameters like this (nuget coding guideline)

emgarten · 2017-11-29T22:59:01Z

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

+
+    static class SignatureTypeMethods
+    {
+        public static string GetString(this SignatureType type)


Does type.ToString() work? I believe there is a way to let .NET handle this

dtivel · 2017-11-29T18:33:47Z

src/NuGet.Core/NuGet.Commands/Strings.resx

  </data>
  <data name="SignCommandOutputPath" xml:space="preserve">
    <value>Signed package(s) output path: {0}</value>
    <comment>0 - output directory path</comment>
  </data>
+  <data name="Error_CommandFailedWithException" xml:space="preserve">
+    <value>NuGet Command failed. {0} threw exception: {1}</value>


Lower case "command".

dtivel · 2017-11-29T18:34:14Z

src/NuGet.Core/NuGet.Commands/Strings.resx

+    <value>Finished with {0} errors and {1} warnings.</value>
+  </data>
+  <data name="VerifyCommand_Success" xml:space="preserve">
+    <value>Successfully verified package.</value>


dtivel · 2017-11-29T18:36:36Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs

-            if (verifyArgs.Type != VerifyArgs.VerificationType.Signatures)
+            var executedSuccessfully = false;
+
+            if (ShouldExecuteType(verifyArgs, VerificationType.Signatures))


I think it would read more clearly if you removed Type everywhere (e.g.: VerificationType, ShouldExecuteType, etc.). This could be simply ShouldExecuteVerification.

dtivel · 2017-11-29T19:00:39Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs


-            var trustProviders = SignatureVerificationProviderFactory.GetSignatureVerificationProviders();
-            var verifier = new PackageSignatureVerifier(trustProviders, SignedPackageVerifierSettings.RequireSigned);
+                var trustProviders = SignatureVerificationProviderFactory.GetSignatureVerificationProviders();


Seems this should be renamed toverificationProviders. If verification includes a trust check, no problem.

dtivel · 2017-11-30T15:12:23Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs

                }
+                executedSuccessfully = errorCount == 0;


This doesn't need to be evaluated after each package verification and can instead be evaluated once near the return statement below.

dtivel · 2017-11-30T23:28:43Z

src/NuGet.Core/NuGet.Packaging/Signing/Archive/SignedPackageArchiveUtility.cs


-                SignedPackageArchiveIOUtility.ReadAndHashUntilPosition(reader, hashAlgorithm, zipMetadata.Zip64EndOfCentralDirectoryLocatorPosition + 16);
+                    var relativeOffset = (uint)(reader.ReadUInt32() - metadata.SignatureFileEntryTotalSize);


relativeOffsetOfLocalFileHeader

dtivel · 2017-11-30T23:37:28Z

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

@@ -51,7 +51,7 @@ private static void X509Certificate2ToString(X509Certificate2 cert, StringBuilde
        /// </summary>
        /// <param name="certCollection">X509Certificate2Collection to be converted to string.</param>
        /// <returns>string representation of the X509Certificate2Collection.</returns>
-        public static string X509Certificate2CollectionToString(X509Certificate2Collection certCollection)
+        public static string X509Certificate2CollectionToString(X509Certificate2Collection certCollection, string indentation = "")


Update XML doc comments.

dtivel · 2017-11-30T23:39:58Z

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

        public static Signature Load(SignedCms cms)
        {
-            var signerInfoCollection = cms.SignerInfos;
-            if (signerInfoCollection.Count != 1)
+            if (cms.SignerInfos.Count < 1)


For the "root" signed CMS, the SignedData.signerInfos count MUST be 1.

dtivel · 2017-11-30T23:41:44Z

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

+        /// <summary>
+        /// Create a signature based on a valid byte stream to be decoded as a signed cms
+        /// </summary>
+        /// <param name="stream">signature data</param>
        public static Signature Load(Stream stream)


Add a null check on stream.

dtivel · 2017-11-30T23:43:11Z

src/NuGet.Core/NuGet.Packaging/Signing/SigningUtility.cs

-        {
-            return IsCertificateValid(certificate, out var chain, allowUntrustedRoot, checkRevocationStatus: true);
-        }
-
        public static bool IsCertificatePublicKeyValid(X509Certificate2 certificate)


Add XML doc comments.

PatoBeltran · 2017-12-01T18:45:12Z

@emgarten @dtivel addressed CR

Refactored code, added missing verification steps, added more detailed printing for verbose and debug mode, cleaned up strings

emgarten

🚀

dtivel · 2017-12-01T20:43:20Z

src/NuGet.Core/NuGet.Packaging/PackageArchiveReader.cs

-                SignedPackageArchiveUtility.VerifySignedZipIntegrity(reader, hashAlgorithm, expectedHash);
+                if (!SignedPackageArchiveUtility.VerifySignedZipIntegrity(reader, hashAlgorithm, expectedHash))
+                {
+                    throw new SignatureException(Strings.SignatureFailurePackageTampered, GetIdentity());


"Package integrity check failed." seems like a better message.

dtivel · 2017-12-02T00:20:05Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampVerificationUtility.cs

+            }
+
+            // everything to UTC
+            var timestampUpperGenTimeUtcTicks = tstInfoGenTime.AddTicks(tstInfoAccuracyInTicks).UtcTicks;


There's no need to convert into ticks all over the place.

should using milliseconds be better?

You can compare DateTimeOffset objects. tstInfo.Timestamp is one.

var tstInfoGenTime = tstInfo.Timestamp; var tstInfoAccuracy = tstInfo.AccuracyInMicroseconds; long tstInfoAccuracyInSeconds; if (tstInfoAccuracy.HasValue) { tstInfoAccuracyInSeconds = Math.Abs(tstInfoAccuracy.Value) / 1000000; } else if (string.Equals(tstInfo.PolicyId, Oids.BaselineTimestampPolicyOid)) { tstInfoAccuracyInSeconds = 1; } else { tstInfoAccuracyInSeconds = 0; } // everything to UTC var timestampUpperGenTimeUtc = tstInfoGenTime.AddSeconds(tstInfoAccuracyInSeconds); var timestampLowerGenTimeUtc = tstInfoGenTime.AddSeconds(-tstInfoAccuracyInSeconds); DateTimeOffset signerCertExpiryUtc = signerCertificate.NotAfter.ToUniversalTime(); DateTimeOffset signerCertBeginUtc = signerCertificate.NotBefore.ToUniversalTime(); return timestampUpperGenTimeUtcTicks < signerCertExpiryUtcTicks && timestampLowerGenTimeUtcTicks > signerCertBeginUtcTicks;

dtivel · 2017-12-02T15:15:17Z

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

+
+            var result = SignatureVerificationStatus.Invalid;
+
+            using (var chain = new X509Chain(useMachineContext: true))


Should be false based on last discussion with you and Zhi. You can just use X509Chain's default constructor which already passes false.

dtivel · 2017-12-04T18:24:46Z

src/NuGet.Core/NuGet.Commands/Strings.resx

  </data>
  <data name="SignCommandOutputPath" xml:space="preserve">
    <value>Signed package(s) output path: {0}</value>
    <comment>0 - output directory path</comment>
  </data>
+  <data name="VerifyCommand_FinishedWithErrors" xml:space="preserve">
+    <value>Finished with {0} errors and {1} warnings.</value>


Add a comment indicating what the arguments to {0} and {1} are.

dtivel · 2017-12-04T18:25:20Z

src/NuGet.Core/NuGet.Commands/Strings.resx

+    <value>Successfully verified package(s).</value>
+  </data>
+  <data name="VerifyCommand_VerifyingPackage" xml:space="preserve">
+    <value>Verifying {0}</value>


Add a comment indicating what an argument to {0} represents.

dtivel · 2017-12-04T18:31:53Z

src/NuGet.Core/NuGet.Commands/VerifyCommand/VerifyCommandRunner.cs

-                        var errors = logMessages.Where(m => m.Level == LogLevel.Error).Count();
-                        var warnings = logMessages.Where(m => m.Level == LogLevel.Warning).Count();
+                    var errors = logMessages.Where(m => m.Level == LogLevel.Error).Count();
+                    var warnings = logMessages.Where(m => m.Level == LogLevel.Warning).Count();


You can replace Where(...) with Count(...):

var errors = logMessages.Count(m => m.Level == LogLevel.Error); var warnings = logMessages.Count(m => m.Level == LogLevel.Warning);

dtivel · 2017-12-04T18:37:24Z

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

@@ -7,6 +7,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using NuGet.Common;
+using System.Diagnostics;


dtivel · 2017-12-04T18:37:47Z

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

@@ -60,7 +61,7 @@ public static string X509Certificate2CollectionToString(X509Certificate2Collecti
            for (var i = 0; i < Math.Min(_limit, certCollection.Count); i++)
            {
                var cert = certCollection[i];
-                X509Certificate2ToString(cert, collectionStringBuilder);
+                X509Certificate2ToString(cert, collectionStringBuilder, "");


indentation: ""

dtivel · 2017-12-04T18:40:51Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampProvider.cs


                byte[] timestampByteArray;

                using (var timestampNativeCms = NativeCms.Decode(timestampCms.Encode(), detached: false))
+                using (var timestampCertChain = new X509Chain(useMachineContext: true))


Use the default constructor: new X509Chain().

dtivel · 2017-12-04T18:41:38Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampProvider.cs

+                    var policy = timestampCertChain.ChainPolicy;
+
+                    policy.ApplicationPolicy.Add(new Oid(Oids.TimeStampingEkuOid));
+                    policy.VerificationFlags = X509VerificationFlags.IgnoreNotTimeValid;


Great, but we should probably fail if:

DateTime.NowUtc < timestampSignerCertificate.NotBefore

The certificate's validity date would be in the future.

PatoBeltran · 2017-12-04T20:24:18Z

@dtivel addressed comments

dtivel · 2017-12-04T20:46:22Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampVerificationUtility.cs

+            }
+
+            // everything to UTC
+            var timestampUpperGenTimeUtcTicks = tstInfoGenTime.AddTicks(tstInfoAccuracyInTicks).UtcTicks;


You can compare DateTimeOffset objects. tstInfo.Timestamp is one.

var tstInfoGenTime = tstInfo.Timestamp; var tstInfoAccuracy = tstInfo.AccuracyInMicroseconds; long tstInfoAccuracyInSeconds; if (tstInfoAccuracy.HasValue) { tstInfoAccuracyInSeconds = Math.Abs(tstInfoAccuracy.Value) / 1000000; } else if (string.Equals(tstInfo.PolicyId, Oids.BaselineTimestampPolicyOid)) { tstInfoAccuracyInSeconds = 1; } else { tstInfoAccuracyInSeconds = 0; } // everything to UTC var timestampUpperGenTimeUtc = tstInfoGenTime.AddSeconds(tstInfoAccuracyInSeconds); var timestampLowerGenTimeUtc = tstInfoGenTime.AddSeconds(-tstInfoAccuracyInSeconds); DateTimeOffset signerCertExpiryUtc = signerCertificate.NotAfter.ToUniversalTime(); DateTimeOffset signerCertBeginUtc = signerCertificate.NotBefore.ToUniversalTime(); return timestampUpperGenTimeUtcTicks < signerCertExpiryUtcTicks && timestampLowerGenTimeUtcTicks > signerCertBeginUtcTicks;

dtivel · 2017-12-04T21:51:44Z

src/NuGet.Core/NuGet.Packaging/Signing/Timestamp/Rfc3161TimestampVerificationUtility.cs

+            }
+
+            // everything to UTC
+            var timestampUpperGenTimeUtcTicks = tstInfoGenTime.AddMilliseconds(accuracyInMilliseconds);


Remove Ticks from all these names.

clairernovotny · 2017-12-04T22:46:14Z

Running this code throws for me:

This is throwing trying to load the timestampedSignature.

mishra14 · 2017-12-06T01:46:21Z

@onovotny it seems that the data timestamped SignedCms is not being loaded correctly by the underlying timestamp implementation. What is the data and the source?

clairernovotny · 2017-12-06T02:57:28Z

@mishra14 this was fixed by @dtivel in a different PR. There were some issues in the code.

mishra14 · 2017-12-06T19:14:35Z

@onovotny glad to know! :)

PatoBeltran self-assigned this Nov 29, 2017

PatoBeltran requested review from rohit21agrawal, nkolev92, emgarten, zhili1208, mishra14, dtivel and jainaashish November 29, 2017 01:25

dnfclas added the cla-already-signed label Nov 29, 2017

emgarten reviewed Nov 29, 2017

View reviewed changes

emgarten suggested changes Nov 29, 2017

View reviewed changes

dtivel reviewed Nov 30, 2017

View reviewed changes

PatoBeltran added 4 commits December 1, 2017 12:24

Update verify command

74a9912

Refactored code, added missing verification steps, added more detailed printing for verbose and debug mode, cleaned up strings

Make tests compile

9e4e743

Update verify to address spec

0e3c7b6

Addressed CR

e980b08

PatoBeltran force-pushed the dev-patobeltran-verify branch 2 times, most recently from 2d1b1b2 to 6bb36fe Compare December 1, 2017 22:09

fix build

37f779d

PatoBeltran force-pushed the dev-patobeltran-verify branch from 6bb36fe to 37f779d Compare December 1, 2017 22:23

emgarten approved these changes Dec 1, 2017

View reviewed changes

zip64 should not be supported

6890ed9

PatoBeltran force-pushed the dev-patobeltran-verify branch from 3d99d6c to 5236cec Compare December 4, 2017 19:16

fix integrity check

4539fcb

PatoBeltran force-pushed the dev-patobeltran-verify branch from 5236cec to 4539fcb Compare December 4, 2017 19:18

dtivel approved these changes Dec 4, 2017

View reviewed changes

addressed cr

5693822

dtivel approved these changes Dec 4, 2017

View reviewed changes

PatoBeltran merged commit 74b4f8c into dev-feature-signing Dec 4, 2017

PatoBeltran deleted the dev-patobeltran-verify branch December 4, 2017 22:32


		SignedPackageArchiveIOUtility.ReadAndHashUntilPosition(reader, hashAlgorithm, zipMetadata.Zip64EndOfCentralDirectoryLocatorPosition + 16);
		var relativeOffset = (uint)(reader.ReadUInt32() - metadata.SignatureFileEntryTotalSize);


		var result = SignatureVerificationStatus.Invalid;

		using (var chain = new X509Chain(useMachineContext: true))

Update verify command #1825

Update verify command #1825

Conversation

PatoBeltran commented Nov 29, 2017 • edited Loading

dnfclas commented Nov 29, 2017

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

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 Dec 1, 2017

emgarten left a 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

PatoBeltran commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairernovotny commented Dec 4, 2017

mishra14 commented Dec 6, 2017

clairernovotny commented Dec 6, 2017

mishra14 commented Dec 6, 2017

PatoBeltran commented Nov 29, 2017 •

edited

Loading