Arcade: [Signing] Represent a file and its certificate in one item group instead of three

[maririos]

Request from @natemcmaster .

Discussion from https://github.com/dotnet/arcade/pull/488#issuecomment-414760012

From @natemcmaster :

Design question: is there any reason why separate item groups for signing info and file paths is required? I'm trying to figure out why I need two or three item groups to represent a file and its certificate. I was expecting to be able to do something like this:

<ItemGroup>
  <ItemsToSign Include="file.dll" CertificateName="Microsoft402" />
</ItemGroup>

From @tmat :

We don't want to have to explicitly list all files that needs to be signed and their certificates. Instead we specify default certificates based on PKT and optionally target framework. Only when files need to be signed by a non-default certificate we use an explicit list.

Cc: @weshaggard @jaredpar

[JohnTortugo]

@natemcmaster @tmat Currently you'll need to use at least 2 ItemGroups to configure the SignTool. ItemsToSign to specify which files need to be signed and FileSignInfo to specify which information to use for the file.

What do you think about adding a new CertificateName attribute to ItemsToSign to configure the default certificate for a file? That way the user will only need to use FileSignInfo when there is a need to override the signing info based on other attributes like PKT and TargetFramework.

[natemcmaster]

+1 for a single MSBuild item.

<FilesToSign Include="path/to/MyFile.dll" Certificate="Microsoft400" />

[JohnTortugo]

Seems like we can cover that scenario with what we currently have. I'll close this issue for now but I've the feeling that we might eventually need to talk about the original issue ..

[weshaggard]

Seems like we can cover that scenario with what we currently have.

Can you please describe in this issue how to accomplish this scenario without requiring we list out all files twice?

[natemcmaster]

I'm working around this with an MSBuild transform:

    <ItemGroup>
      <_FileSignInfo Include="%(FilesToSign.FileName)%(FilesToSign.Extension)" CertificateName="%(FilesToSign.Certificate)" />
      <_FileSignInfo Include="%(FilesToExcludeFromSigning.FileName)%(FilesToExcludeFromSigning.Extension)" CertificateName="None" />
      <_ItemsToSign Include="@(FilesToSign)" />
    </ItemGroup>

    <Microsoft.DotNet.SignTool.SignToolTask
      DryRun="$(SignToolDryRun)"
      TestSign="$(SignToolTestSign)"
      ItemsToSign="@(_ItemsToSign)"
      FileSignInfo="@(_FileSignInfo)"
      StrongNameSignInfo=""
      FileExtensionSignInfo="@(SignableFileExtension)"
      TempDir="$(IntermediateDir)"
      LogDir="$(LogOutputDir)"
      MSBuildPath="$(MSBuildx86Path)"
      MicroBuildCorePath="$(NuGetPackageRoot)microbuild.core\$(MicroBuildCorePackageVersion)" />

[weshaggard]

We could very easily have support for this in the sdk.

[tmat]

The workaround shouldn't be necessary once we fix a couple of bugs in the current impl

[weshaggard]

Are we tracking those bugs in other issues? What would the code look like once those are fixed?

[tmat]

Yes, they are tracked.

The file would look like so:

  <ItemGroup>
      <FileExtensionSignInfo Remove="@(FileExtensionSignInfo)"/>

      <FileSignInfo Include="file1.dll" CertificateName="Microsoft400" />
      <FileSignInfo Include="file2.dll" CertificateName="Microsoft400" />
      <FileSignInfo Include="file3.dll" CertificateName="None" />
      ...
      <FileSignInfo Include="fileN.nupkg" CertificateName="NuGet" />
    </ItemGroup>

Assuming all packages to sign are under $(ArtifactsPackagesDir). If there are files that are expected to be signed in another directory then also add:

  <ItemsToSign Include="another_directory\**\*.*"/>

[weshaggard]

Yes, they are tracked.

For completeness can you link to the tracking issue? The closest issue I could find is https://github.com/dotnet/arcade/issues/977 which I think is slightly different but I could see supporting this there depending on the implementation we land on.

[tmat]

Yes, that's the issue.

[JohnTortugo]

Looks good to close this issue @weshaggard @natemcmaster ?

[natemcmaster]

It's still my opinion that three items groups is unnecessary when one would do, and that makes for confusing usage. But I have lost interest in battling @tmat on this point. So sure, feel free close.

[JohnTortugo]

I'll close the issue and if we find that something is blocking or should be improve we can always reopen it / create another issue. Thanks.