Make SignTool accept a list of files to be signed

[JohnTortugo]

As per talk with @maririos she said that we are going to patch the SignToolTask to receive a list of directories where the SignTool will look for container files (only .nupkg for now) that need to be signed.

Cc: @maririos @tmat @weshaggard

[maririos]

Just to clarify, we are going to add this new functionality.

From the discussion in #58 we decided to have:

• Signtool will (batch) sign from a manifest (list of files)
• This manifest can be checked in (explicit)
• This manifest can be generated during the build (implicit)

[tmat]

Do we still need the explicit manifest?

Passing list of files in an item group in to be build task should be sufficient for all scenarios, no?

[maririos]

@jaredpar was advocating for it.

Jared, do you still want to have the explicit manifest?

[JohnTortugo]

I am a bit confused now. Is the usual SignToolData.json what you are referring to as manifest? If so:

Signtool will (batch) sign from a manifest (list of files)
This manifest can be checked in (explicit)

mean that we are going to keep what we have today, i.e., support for SignToolData.json and

This manifest can be generated during the build (implicit)

the SignToolTask will accept an ItemGroup with list of *.nupkgs to sign.

I want to make this issue + PR short. Thus, I'd like to scope it to only receiving the list of files to sign. Previous behavior will be maintained for now. Does that work for you?

[maririos]

mean that we are going to keep what we have today, i.e., support for SignToolData.json

Yes. Unless @jaredpar consider is it not necessary anymore.

The two ways to call the Sign task, as I understood from the last meeting, are:

1. Pass required parameters plus the location of the SignToolData.json file
2. Pass required parameters plus the list of directories which content needs to be signed (i.e. folder called shipping).

For the 1st scenario, the SignTool is in charge of reading the list of files to sign and the list of files to exclude, creating an ItemGroup with:

• Name/location of the file
• Strong name
• Certificate

=>No change to what we have today.

For the 2nd scenario, the SignTool is in charge of looking at each directory. If there are NuGet packages, unpack them to extract the dll that needs to be signed, extract the strong name and the certificate and create an ItemGroup with that information.
In this scenario, we will probably need to consolidate information against an exclusionList of files that don't need to be signed and a mapping table for the certificates.

=> Work to be done.

At the end, from both entry points we have an ItemGroup with the list of files that need to be signed.

@tmat please correct where neccesary

[JohnTortugo]

Just to confirm some things:

• The task is going to receive a list of directories.
• Should the sub-directories be recursively processed?
• Is it possible for .nupkg contain other .nupkgs and should they be expanded recursively as well?

[tmat]

The task itself accepts a list of files, not directories.
Arcade.SDK will populate the list using a glob like so:

<ItemGroup>
  <FilesToSign Include="$(ArtifactsShippingPackagesDir)**\*.nupkg">
  <!-- include other directories as needed (VSSetup, etc.) -->
</ItemGroup>

Containers (NuPkgs, VSIX) may contain other containers. The current task already handles that.

[JohnTortugo]

The task itself accepts a list of files, not directories.

Got it. Thanks.

Containers (NuPkgs, VSIX) may contain other containers. The current task already handles that.

Currently SignTool process containers recursively, but it does so only to find the files and all files will be signed with the same certificate + strong name. What I am thinking is that I'll need to do that search also, but for extracting the strong names + certificate(?), since each may use different values. What do you think?

[tmat]

I don't understand what the problem is. Each file is signed separately. For managed .dlls, .exes the certificate to be used for each file is determined from the public key token of the assembly strong name. For VSIX and NuGet containers the certificate is given.

In rare cases the default certificate that we associate with the PKT won't be the right one. If repo needs to override the certificate it would specify something like the following item group in its Versions.prop file:

<ItemGroup>
  <FileSignInfo Include="Microsoft.DiaSymReader.dll" PublicKeyToken="31bf3856ad364e35" TargetFramework=".NETFramework,Version=v2.0" CertificateName="MicrosoftSHA1Win8WinBlue" />
  <FileSignInfo Include="Microsoft.DiaSymReader.dll" PublicKeyToken="31bf3856ad364e35" TargetFramework=".NETStandard,Version=v1.1" CertificateName="WindowsPhone623" />
</ItemGroup>

The task would match PKT and TargetFramework against the respective values in assembly metadata.

[jaredpar]

@maririos

Jared, do you still want to have the explicit manifest?

I'm fine with it being inferred as part of the build but the manifest should still be a visible artifact output of build. Having it around makes it trivial to audit how changes to build impacted the set of signed binaries.

@JohnTortugo

Is it possible for .nupkg contain other .nupkgs and should they be expanded recursively as well?

Yes. The same nesting issue exists for VSIX. The tool is designed to handle this exact case.

[tmat]

Having it around makes it trivial to audit how changes to build impacted the set of signed binaries.

The build task now outputs the list of files it signs into binlog as messages. Is that sufficient?

[jaredpar]

Is it done in a single location or spread out? What is really needed here is a way to concisely view the signing state of a build. If it's easy to see it as a single element in binlog that's great. But If i have to hunt through a bunch of different messages that makes it harder to validate.

[tmat]

Is it done in a single location or spread out?

Single location

[JohnTortugo]

For VSIX and NuGet containers the certificate is given.

How is that information passed?

[jaredpar]

@JohnTortugo that would need to be determined. You would have to essentially specify a single one for each file type in a given directory.

[tmat]

All VSIXes are signed by VsixSHA2 certificate
All NuGet pakcages are signed by NuGet certificate.

[JohnTortugo]

I think I've code for a PR but first I'd like to check a few things:

• A list of files is received as parameter. Those can be .nupkg, .vsix, .exe and .dll
• For non-containers the public key token is forwarded as certificate
• For containers the certificate is hard-coded (at least for now) as NuGet and VsixSHA2
• For now, there is still support for the SignToolData.json file; and both configurations can work together

Questions:

• I couldn't figure out what should be the Strong Name value for each case.. I'm currently using NULL for all cases;

• It's not yet clear to me how signing of files nested into containers work. Which certificate will be used to sign them? Should all nested (non-containers) files be listed in the sign or exclude list. I think yes, but I'm not sure.

• I'm puzzled by the line linked below. Should I remove/patch this check ..?

  arcade/src/Microsoft.DotNet.SignTool/src/BatchSignUtil.cs

  Line 344 in e20e845

  if (fileSignInfo.StrongName != null || fileSignInfo.Certificate != null)

[tmat]

1. Determine whether .exe and .dll is managed. Use PEReader to open the file and check if it's native or managed.
2. If it's not managed then StrongName is null and certificate is MicrosoftSHA2.
3. For managed files, get MetadataReader from PEReader and read assembly name and PublicKeyToken from it.
4. Add a parameter to the build task that takes an item group that maps strong name to PublicKeyToken and Certificate name, so that we don't need to hardcode the values to the task. Sign.proj would then pass this item group to the task:

<ItemGroup>
  <StrongNameSignInfo Include="MsSharedLib72" PublicKeyToken="31BF3856AD364E35" CertificateName="MicrosoftSHA2" />
</ItemGroup>

[jaredpar]

All VSIXes are signed by VsixSHA2 certificate
All NuGet pakcages are signed by NuGet certificate.

Indeed that's the case at this time. It's not like managed binaries where we use different certificates for mysterious, but likely valid, reasons.

Aren't native EXEs the same way: one certificate to rule them all?

[JohnTortugo]

I'm having a hard time trying to use PEReader - barely any documentation.
I only managed to extract the TargetFramework using this:

Assembly assembly = System.Reflection.Assembly.LoadFrom(file);

var targetFramework = "Unknown";
var targetFrameworkAttributes = assembly.GetCustomAttributes(typeof(System.Runtime.Versioning.TargetFrameworkAttribute), true);
if (targetFrameworkAttributes.Length > 0)
{
    var targetFrameworkAttribute = (TargetFrameworkAttribute)targetFrameworkAttributes.First();
    targetFramework = (targetFrameworkAttribute.FrameworkName);
}
Console.WriteLine($"Target framework is: {targetFramework}");

which I assume is not acceptable due the LoadFrom(file) part. Can you guys give me any hint how to extract the TargetFrameworkAttribute using PEReader?

[rainersigwald]

@JohnTortugo check out https://github.com/Microsoft/msbuild/blob/a75c5a9e4f9ea6f2de24df4bfc2d60c09e684395/src/Tasks/AssemblyDependency/AssemblyInformation.cs#L353-L425

[JohnTortugo]

Thanks a lot @rainersigwald!

[JohnTortugo]

In rare cases the default certificate that we associate with the PKT won't be the right one. If repo needs to override the certificate it would specify something like the following item group in its Versions.prop file:

The task would match PKT and TargetFramework against the respective values in assembly metadata.

Does this only override the certificate? That is, the strong name will still be the one informed in @(StrongNameSignInfo) ?

[tmat]

StrongNameSignInfo associates PKT with the name of the full key that signing server uses to sign the binary. It also specifies the default certificate that should be used to Authenticode-sign any assembly that has this PKT.

The key to use for strong name signing (the StrongName parameter to the signing server) of any binary must match its PKT.

The only reason why we need to check TargetFrameworkAttribute is when 2 binaries that have the same strong name (which includes PKT) need to use a different Authenticode certificate for some reason.

[JohnTortugo]

Can it occur that for some random binary we can't extract the TargetFramework? I'm thinking something like striped down version of the binary.. I've a file here (named Microsoft.VisualStudio.CodeCoverage.Shim.dll TFM=net461) that I've tried a few options and none worked. I am thinking if it's possible that the information isn't present in the file at all.

[JohnTortugo]

Regarding having or not an exclusion list of files: after some changes I made today files nested in containers will be automatically found. Doesn't that make necessary (or desirable) to have some way to exclude some files from the signing process? Otherwise, we'll be signing files nested in many "system" and "microsoft" namespaces, like System., Microsoft.VisualStudio., etc. Do we need to do that?

[tmat]

Can it occur that for some random binary we can't extract the TargetFramework?

This feature is only needed for binaries built by the repo. Binaries not built by the repo should already be signed or excluded. We can make sure that all binaries built by the repo that need special cert have the attribute.

Note that TargetFramework in FileSignInfo is optional. If not specified the info applies to any dll of the specified name and PKT.

[tmat]

Doesn't that make necessary (or desirable) to have some way to exclude some files from the signing process? Otherwise, we'll be signing files nested in many "system" and "microsoft" namespaces, like System., Microsoft.VisualStudio., etc. Do we need to do that?

We need to avoid signing already signed assemblies. Checking that an assembly is already signed can be done using the PEReader.

That said, you are right we might need to exclude some unsigned assemblies from signing. To do so, we can use FileSignInfo with a special value of CertificateName. Something like:

<FileSignInfo Include="Foo.dll" PublicKeyToken="31bf3856ad364e35" CertificateName="None" />

[JohnTortugo]

Thanks for answer. That make things much clearer. Only thing that I wanted to confirm is about checking if a random binary that I find in a package is from the current repo or not. That is, let's say a container file contains a file named "Foo.Bar.dll". How do I know if that file was produced by the current repo or not?

My current idea is to check if there is a bin/Foo.Bar/{TFM}/Foo.Bar.dll file in the $(OutputDir) folder. Maybe we can even compare the content of the files. Does this approach make sense?

[tmat]

How do I know if that file was produced by the current repo or not?

You should not need to know. Either it's signed or not. If it is signed then there is nothing we need to do for this file. Otherwise, check if there is a FileSignInfo for the file. If so, use the cert specified by this info (if none then skip the file). Otherwise, use the default certificate based on matching StrongNameSignInfo. Otherwise, report an error.

[tmat]

My current idea is to check if there is a bin/Foo.Bar/{TFM}/Foo.Bar.dll file in the $(OutputDir) folde

We shouldn't be touching files from bin directory at all, or any other directory. All the assets we need are already included in the containers.

[JohnTortugo]

It seems there are some files in our current SignToolData.json that aren't contained in of our Arcade packages. What's the idea for handling that?

"bin/Microsoft.DotNet.SignTool.Tests/netcoreapp2.0/Microsoft.DotNet.SignTool.Tests.dll",
"bin/Microsoft.AspNetCore.ApiVersioning/netstandard2.0/Microsoft.AspNetCore.ApiVersioning.dll",
"bin/Microsoft.AspNetCore.ApiVersioning.Analyzers/netstandard1.3/Microsoft.AspNetCore.ApiVersioning.Analyzers.dll",
"bin/Microsoft.AspNetCore.ApiVersioning.Swashbuckle/netstandard2.0/Microsoft.AspNetCore.ApiVersioning.Swashbuckle.dll" 

[tmat]

We don't want to sign or package test assemblies.

[weshaggard]

@JohnTortugo is there an issue/pr tracking the consumption of your new changes in #410 in arcade?

[JohnTortugo]

There is issue #464 where I plan to track the work of making Arcade use the refactored SignTool. I'll create a PR for it today.