Make SIGN001 suppressable for specific files

[natemcmaster]

For example, this repo has this:

  <ItemGroup>
    <!-- 
      Suppresses code signing on this file because it causes a false-positive on SIGN001. This assembly comes from https://www.nuget.org/packages/Microsoft.VisualStudio.OLE.Interop/, but isn't code signed.
      TODO: Make SIGN001 suppressable.
    -->
    <FileSignInfo Include="Microsoft.VisualStudio.OLE.Interop.dll" CertificateName="None" />
  </ItemGroup>

Without this, the build fails with an error:

error SIGN001: Signing 3rd party library 'C:\src\dotnet\arcade\artifacts\tmp\Debug\ContainerSigning\5D7431DF4BE7A006B818A2C5BD85D4189CAEA1A798FE6A422CCA2A4DBDA80D70\Microsoft.VisualStudio.OLE.Interop.dll' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: ''.

We could also completely suppress SIGN001, but that would lead us to miss valid instances of SIGN001. It would be good if we could make suppressions more granular.

[tmat]

Sounds like we need to fix the package. Seems like it should be simple to get this binary signed and new package published.

Until the package is fixed, what's wrong with specifying <FileSignInfo Include="Microsoft.VisualStudio.OLE.Interop.dll" CertificateName="None" /> ?

[natemcmaster]

Until the package is fixed, what's wrong with specifying

That's what we did to unblock getting a build of SignCheck. This would not be acceptable for a product repo, though. We wouldn't want to ship unsigned binaries just because the tool produces an error because its assembly copyright doesn't fit our criteria.

[tmat]

This would not be acceptable for a product repo, though

The root problem is that Microsoft.VisualStudio.OLE.Interop.dll is not signed. If this was a product dependency, the fix would be to fix this binary, not to suppress warning in repos that consume the binary.

That said I'm not against adding an attribute SuppressWarnings to FileSignInfo. It just seems like that shouldn't be the solution for this particular binary.

[JohnTortugo]

That said I'm not against adding an attribute SuppressWarnings to FileSignInfo. It just seems like that shouldn't be the solution for this particular binary.

My plan here is:

• Add the SupressWarnings attribute to FileSignInfo so that the user can specify a semicolon separated list of SIGNWXXX identifiers to suppress for an specific file. E.g.:
  <FileSignInfo Include="Microsoft.VisualStudio.OLE.Interop.dll" SuppressWarnings="SIGNW001;SIGNWXXX" CertificateName="Microsoft400" />
• Add a new ItemGroup parameter, SupressWarnings, where the user can specify SIGNWXXX identifiers to suppress in the scope of all files.

Does this meet you needs @natemcmaster ? @ericstj

[natemcmaster]

As long as you use "suppress" (two P's), I'm happy with this plan.

Just my two cents: also consider "NoWarn", which is a naming convention used in other places, like Csc and NuGet. <FileSignInfo ... NoWarn="SIGN001" />

[JohnTortugo]

As long as you use "suppress" (two P's), I'm happy with this plan.

Just my two cents: also consider "NoWarn", which is a naming convention used in other places, like Csc and NuGet. <FileSignInfo ... NoWarn="SIGN001" />

Well, that seems an even better name!

[tmat]

BTW, there should be no W in the diagnostic ID, it's just SIGNXXX.

[tmat]

@natemcmaster Has Microsoft.VisualStudio.OLE.Interop been fixed? As I said I'm not against adding NoWarn attribute, but at this point there seems to be no use case for it since Microsoft.VisualStudio.OLE.Interop package needs to be fixed.

[natemcmaster]

Microsoft.VisualStudio.OLE.Interop

No. This package hasn't been updated since Sep. 2017. I don't know who owns it.

[tmat]

Version 7.10.6071 contains signed binaries. It also seems this is an interop assembly and perhaps should be embedded, not referenced.

[tmat]

So the fix is simply to use 7.10.6071 instead of 7.10.6070: #1394

[JohnTortugo]

Just to bring some more info to the table. Other builds have seen this warning around and I think moving forward adding an option to suppress this message might still be useful.

One build where I saw the warning was CoreFX: here.

warning SIGN001: Signing 3rd party library 'E:\A\_work\7\s\corefx\artifacts\tmp\Release\ContainerSigning\56946CB703CD99FC868CA921B539AA005823CFE4AEA5E69534D1F5C487655CEE\netstandard.dll' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: '.NET Foundation and Contributors'.

I'm not sure if .NET Foundation and Contributors is considered third party.. but I think that the check for SIGN001 is quite limited in it's approach. I think we even had a discussion by e-mail about that, right? Just my two cents.

[tmat]

We should check for .NET Foundation and Contributors copyright as well when figuring out whether the certificate is allowed for the binary then. The purpose of the check is to warn about suspect certificate applications. If this is a copyright that we regularly use in Microsoft-signed binaries then let's update the heuristic to account for that.

As I said, I'm not against adding the NoWarn but let's have specific scenarios that support such addition. So far we have found two potential use-cases but each of them actually unearthed a problem that warranted another fix. So the heuristic is actually working!

[JohnTortugo]

We should check for .NET Foundation and Contributors copyright as well when figuring out whether the certificate is allowed for the binary then.

I'll add that to the condition and put this issue on hold until we see further evidence that we need it.

[dsplaisted]

I'm hitting this with a bunch of VSTest assemblies that don't have the Copyright set:

      <FilesNotToSign Include="Microsoft.TestPlatform.Extensions.BlameDataCollector.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.TestHostRuntimeProvider.dll"/>
      <FilesNotToSign Include="Microsoft.VisualStudio.TestPlatform.Extensions.Trx.TestLogger.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.Build.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.CommunicationUtilities.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.CoreUtilities.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.CrossPlatEngine.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.PlatformAbstractions.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.Utilities.dll"/>
      <FilesNotToSign Include="Microsoft.TestPlatform.VsTestConsole.TranslationLayer.dll"/>
      <FilesNotToSign Include="Microsoft.VisualStudio.TestPlatform.Client.dll"/>
      <FilesNotToSign Include="Microsoft.VisualStudio.TestPlatform.Common.dll"/>
      <FilesNotToSign Include="Microsoft.VisualStudio.TestPlatform.ObjectModel.dll"/>
      <FilesNotToSign Include="datacollector.dll"/>
      <FilesNotToSign Include="vstest.console.dll"/>

I agree that the right fix is to fix the root cause, but if that takes a while we would be blocked on shipping an arcade-built .NET Core SDK without the NoWarn capability.

[tmat]

Are they not signed?

[tmat]

Your repo shouldn't be signing these in the first place (unless your repo is VS test repo).

[dsplaisted]

@tmat We crossgen these so they need to be resigned

[tmat]

@dsplaisted I see. Makes sense.

You can also suppress SIGN001 completely as a workaround to unblock - please file an issue on VS test repo to add proper copyright to their assemblies though.

[Chrisboh]

We believe this is already resolved.