Adding support for NuGet.exe sign command with fake signing #1755

mishra14 · 2017-10-18T02:20:54Z

Bug

Link: Contributes to NuGet/Home#5904
Regression: No

Fix

Details: Adds a top level command to NuGet.exe to allow authors to sign packages with fake signing. The behavior is documented in the spec.

Testing/Validation

Tests Added: Yes
Validation done: Manually tested NuGet.exe

emgarten · 2017-10-18T19:10:59Z

src/NuGet.Clients/NuGet.CommandLine/Commands/SignCommand.cs

+        public ISignCommandRunner SignCommandRunner { get; set; }
+
+        // List of possible values - https://github.com/Microsoft/referencesource/blob/master/mscorlib/system/security/cryptography/HashAlgorithmName.cs
+        private static string[] _acceptedHashAlgorithms = { "SHA256", "SHA384", "SHA512" };


note: this will probably need to come from the helper that verifies this later based on the package signing version, that isn't in yet though

emgarten · 2017-10-18T19:23:04Z

src/NuGet.Clients/NuGet.CommandLine/Commands/SignCommand.cs

+            {
+                throw new ArgumentException(NuGetCommand.SignCommandNoCertificateException);
+            }
+            else if (!string.IsNullOrEmpty(CertificatePath) &&


I think this would more clear if we had some static helper methods for these checks.

Verify required option exists

Verify no invalid option combinations

Enum parsing + verify valid result

With a way to throw the right message for all the above

Example: https://github.com/emgarten/NuGet.CatalogReader/blob/bcda3b86fa55a914aa0b7d3af7a3c366c0aee96e/src/NuGetMirror/Common/CmdUtils.cs#L17-L70

Using an else if for all of these also implies that these things are related, but they aren't. It would be the same logic if they were all separate ifs.

emgarten · 2017-10-18T19:27:14Z

src/NuGet.Clients/NuGet.CommandLine/Commands/SignCommand.cs

+        {
+        }
+
+        public ISignCommandRunner SignCommandRunner { get; set; }


It seems like it would be easier if we had a static helper that created SignArgs and passed it back instead of needing to catch the SignArgs result by passing a mock SignCommandRunner in to get that result.

emgarten · 2017-10-18T19:27:40Z

src/NuGet.Clients/NuGet.CommandLine/NuGet.CommandLine.csproj

    <EmbeddedResource Update="NuGetResources.resx">
      <!-- Strings are shared by other projects, use public strings. -->
      <Generator>PublicResXFileCodeGenerator</Generator>
      <LastGenOutput>NuGetResources.Designer.cs</LastGenOutput>
    </EmbeddedResource>
+    <Compile Update="NuGetCommand.Designer.cs">


nit: why did this move?

emgarten · 2017-10-18T19:31:31Z

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

+    <value>Signs a NuGet package.</value>
+  </data>
+  <data name="SignCommandTimestampHashAlgorithmDescription" xml:space="preserve">
+    <value>Hash algorithm to be used by the RFC 3161 time stamp server. Defaults to SHA256.</value>


Should time stamp be timestamp here?

emgarten · 2017-10-18T19:34:21Z

src/NuGet.Core/NuGet.Commands/NuGet.Commands.csproj

@@ -33,6 +33,9 @@
    <Reference Include="System.Xml" />
    <Reference Include="System.Xml.Linq" />
    <Reference Include="System.IO.Compression" />
+    <Reference Include="System.Security">
+      <HintPath>C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5\System.Security.dll</HintPath>


Remove hint path

emgarten · 2017-10-18T19:35:14Z

src/NuGet.Core/NuGet.Commands/SignCommand/CertificateFindOptions.cs

+    /// </summary>
+    internal class CertificateSourceOptions
+    {
+


nit: remove extra space

emgarten · 2017-10-18T19:37:26Z

src/NuGet.Core/NuGet.Commands/SignCommand/SignCommandRunner.cs

+            {
+                var package = new SignedPackageArchive(zip);
+                var signer = new Signer(package);
+                signer.SignAsync(request, logger, CancellationToken.None).Wait();


emgarten · 2017-10-18T19:40:40Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs

@@ -221,7 +221,7 @@ private static string GetSourceDisplayName(string source)
            return "'" + source + "'";
        }

-        private static IEnumerable<string> GetPackagesToPush(string packagePath)
+        public static IEnumerable<string> ResolvePackageFromPath(string packagePath)


Move these wildcard methods to a new static helper class. The Sign command shouldn't have a dependency on Push.

LocalFolderUtility might be a good place to add them.

emgarten · 2017-10-18T19:41:28Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+
+            // Act & Assert
+            var ex = Assert.Throws<ArgumentException>(() => signCommand.Execute());
+            Assert.Equal("No package was provided. For a list of accepted ways to provide a package, please visit http://docs.nuget.org/docs/reference/command-line-reference", ex.Message);


any reason this doesn't use the const above?

emgarten

Address minor feedback then

emgarten · 2017-10-19T22:41:10Z

src/NuGet.Clients/NuGet.CommandLine/Commands/SignCommand.cs

+            };
+        }
+
+        private void ValidateAndParseHashAlgorithm(string value, string name, out HashAlgorithmName hashAlgorithm)


these methods can just return the value, it doesn't look like the out is needed since there is only 1 return

emgarten · 2017-10-19T22:44:00Z

src/NuGet.Core/NuGet.Protocol/Utility/LocalFolderUtility.cs

@@ -949,6 +949,47 @@ public static IEnumerable<LocalPackageInfo> GetPackagesV3(string root, string id
            yield break;
        }

+
+        public static IEnumerable<string> ResolvePackageFromPath(string packagePath)


Add a /// summary here to make it clear to anyone else using this that the method is applying wild cards

dtivel · 2017-10-20T13:57:29Z

src/NuGet.Clients/NuGet.CommandLine/Commands/SignCommand.cs

+            }
+        }
+
+        private void ValidateCertificateInputs()


Is it an error to specify one but not both of CryptographicServiceProvider and KeyContainer?

dtivel · 2017-10-20T13:59:48Z

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

@@ -5345,4 +5345,92 @@ nuget locals global-packages -list</value>
  <data name="InstallCommandFrameworkDescription" xml:space="preserve">
    <value>Target framework used for selecting dependencies. Defaults to 'Any' if not specified.</value>
  </data>
+  <data name="SignCommandCertificatePasswordDescription" xml:space="preserve">
+    <value>Password for the certificate, if needed.
+This option can be used to specify the password for the certificate. If no password is provided, the user may be prompted for a password at run time, unless the -NonInteractive  option is passed.</value>


Remove the extra space between NonInteractive and option.

dtivel · 2017-10-20T14:03:24Z

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

@@ -5345,4 +5345,92 @@ nuget locals global-packages -list</value>
  <data name="InstallCommandFrameworkDescription" xml:space="preserve">
    <value>Target framework used for selecting dependencies. Defaults to 'Any' if not specified.</value>
  </data>
+  <data name="SignCommandCertificatePasswordDescription" xml:space="preserve">
+    <value>Password for the certificate, if needed.
+This option can be used to specify the password for the certificate. If no password is provided, the user may be prompted for a password at run time, unless the -NonInteractive  option is passed.</value>


Nowhere else do we say "the user". The user is reading this message. Just say, "If no password is provided, the command will prompt for a password at runtime, unless the -NonInteractive option is passed."

dtivel · 2017-10-20T14:07:23Z

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

+The certificate store can be specified by -CertificateStoreName and -CertificateStoreLocation options.</value>
+  </data>
+  <data name="SignCommandCertificateSubjectNameDescription" xml:space="preserve">
+    <value>Subject name of the certificate used to search a local certificate store for the certificate. 


Some of these option descriptions are quite long. Does this print nicely?

dtivel · 2017-10-20T14:36:56Z

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

+    <value>Signs a NuGet package with the specified certificate.</value>
+  </data>
+  <data name="SignCommandExamples" xml:space="preserve">
+    <value>nuget sign MyPackage.nupkg -CertificatePath /path/to/certificate


It looks like all our other examples use Windows-style paths (\ not /).

dtivel · 2017-10-20T14:56:08Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+        {
+            // Arrange
+            var packagePath = @"foo\bar.nupkg";
+            var timestamper = "http://foo.bar";


https://unit.test. Applies throughout this file.

dtivel · 2017-10-20T15:50:38Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+            {
+                Console = mockConsole.Object,
+                Timestamper = timestamper,
+                CertificatePath =certificatePath,


While you're in this document, format it.

dtivel · 2017-10-20T15:54:28Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+        [InlineData("Root")]
+        [InlineData("TrustedPeople")]
+        [InlineData("TrustedPublisher")]
+        [InlineData("AddreSSBook")]


I don't think you need to test the case-sensitivity of every StoreName member. Having one case-insensitivity test is completely sufficient.

dtivel · 2017-10-20T15:55:28Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+        [InlineData("CurrentUser")]
+        [InlineData("LocalMachine")]
+        [InlineData("currentuser")]
+        [InlineData("localmaChiNe")]


One case-insensitive test is sufficient.

dtivel · 2017-10-20T16:01:19Z

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetSignCommandTest.cs

+            // Act & Assert
+            var signArgs = signCommand.GetSignArgs();
+
+            Assert.True(


In this test and subsequent tests, would it be better to have multiple assertions? If this assertion fails, you don't know the reason without debugging. Bad!

mishra14 · 2017-10-20T18:10:09Z

@dtivel Thanks for the feedback, I will fix these in a separate PR.

## Bug Fixes: NA Regression: No ## Fix Details: This PR addresses the some PR feedback that came in after merging #1755 along with some additional cleanup. ## Testing/Validation Tests Added: Yes Validation done: manual validation + tests

Ankit Mishra added 12 commits October 13, 2017 16:23

Adding sign command, runner and args

b67c912

Cleaning up I/O params and adding some cert look up code

3bf7351

checking in non-building code as a checkpoint

c83087c

Cleaning up code and getting it to build

9b81d75

cleaning up the runner

e637e9e

Adding plumbing to generate test signing

8c5511a

fixing broken tests

87c1d71

fixing build break

b7a102d

Removing changes from signing libraries

784938f

Adjusting to fake sign

e6c1b4b

Adding unit tests for arg parsing

9a4510d

cleaning up UX and interning strings

636d398

mishra14 requested review from PatoBeltran, emgarten, zhili1208 and dtivel October 18, 2017 02:20

dnfclas added the cla-already-signed label Oct 18, 2017

emgarten suggested changes Oct 18, 2017

View reviewed changes

Ankit Mishra added 2 commits October 18, 2017 15:18

addressing pr feedback

471f234

addressing pr feedback and fixing tests

d1c16fc

emgarten approved these changes Oct 19, 2017

View reviewed changes

Addressing pr feedback

0edac8f

mishra14 merged commit fed8530 into dev-feature-signing Oct 20, 2017

mishra14 deleted the dev-anmishr-signcommand-minimal branch October 20, 2017 00:03

dtivel reviewed Oct 20, 2017

View reviewed changes

mishra14 restored the dev-anmishr-signcommand-minimal branch October 21, 2017 01:30

mishra14 mentioned this pull request Oct 21, 2017

Addressing late PR feedback for Sign Command #1762

Merged

mishra14 deleted the dev-anmishr-signcommand-minimal branch November 2, 2017 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for NuGet.exe sign command with fake signing #1755

Adding support for NuGet.exe sign command with fake signing #1755

mishra14 commented Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten Oct 18, 2017

emgarten left a comment

emgarten Oct 19, 2017

emgarten Oct 19, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

dtivel Oct 20, 2017

mishra14 commented Oct 20, 2017

Adding support for NuGet.exe sign command with fake signing #1755

Adding support for NuGet.exe sign command with fake signing #1755

Conversation

mishra14 commented Oct 18, 2017

Bug

Fix

Testing/Validation

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

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

mishra14 commented Oct 20, 2017