vcsjones / AzureSignTool

SignTool Library and Azure Key Vault Support
MIT License
267 stars 85 forks source link

Add support for append signature + tests #214

Closed VaronisContributor closed 8 months ago

VaronisContributor commented 9 months ago

Expose the existing Win API option to add a signature instead of overriding through the new optional command line argument: -as or --append-signature

vcsjones commented 9 months ago

Does this actually work? The last time I tested this on Windows 10, it produced invalid signatures but return SUCCESS. The README alludes to this:

Dual signing is not supported. This appears to be a limitation of the API used.

If this does indeed work, we might want restrict the usage of it to a minimum Windows version that it's been observed to work on.

VaronisContributor commented 9 months ago

It worked on Windows 11, didn't have a chance to test it on other Windows versions. Let me check and get back to you with the results.

VaronisContributor commented 9 months ago

By the way, the main version uses SignerSignEx3 which is supported only on Windows 10+

One more question, I would like to add a warning in SignFile function:

                    // E_INVALIDARG is expected from SignerSignEx3.
                    logger?.LogWarning("If you set the dwTimestampFlags parameter to SIGNER_TIMESTAMP_AUTHENTICODE, you cannot set the dwFlags parameter to SIG_APPEND.");

This warning will be logged before the error code is returned from the SignerSignEx3 (as mentioned in the comment). Is this an acceptable practice?

VaronisContributor commented 8 months ago

We tried it on Windows 10 - it doesn't work. If you're ok with adding Windows version verification and error reporting on incompatible arguments. I can implement it.

VaronisContributor commented 8 months ago

For our internal usage, we distributed the file mssign32.dll from Windows 11 and used it on Windows 10, and it worked. But I guess you won't like such a solution to be an official :)

vcsjones commented 8 months ago

If you're ok with adding Windows version verification and error reporting on incompatible arguments. I can implement it.

I would implement this in two way.

For the AzureSignTool CLI in OnValidate, add a check that does something like

if (AppendSignature && !OperatingSystem.IsWindowsVersionAtLeast(10, 0, 22000)) {
    return new ValidationResult("--append-signature requires Windows 11 or later.", new[] { nameof(AppendSignature) });
}

And in the actual SignFile

if (appendSignature && !OperatingSystem.IsWindowsVersionAtLeast(10, 0, 22000)) {
    throw new PlatformNotSupportedException("Appending signatures requires Windows 11 or later.");
}

That way people using the CLI get a nice validation error, and consumers of just the library itself get a nice exception instead of some Win32 error that is difficult to decode.

vcsjones commented 8 months ago

For our internal usage, we distributed the file mssign32.dll from Windows 11 and used it on Windows 10, and it worked. But I guess you won't like such a solution to be an official :)

There is... technically.. a way to do this. It was explored in the past and it adds a lot of complexity. My preference would just be to say "use Windows 11" instead of trying to carry a copy of mssign32.

Windows 10 goes out of mainstream support in 2 years, Windows 11 has been out for 2. Bumping the OS requirement seems more reasonable to me than trying to do all of the gymnastics needed to ship a native library.

VaronisContributor commented 8 months ago

Agree with you. I've updated it according to the requirements. Please review.

vcsjones commented 8 months ago

Thank for very much for the contribution and bearing with me during the review!

clairernovotny commented 8 months ago

For our internal usage, we distributed the file mssign32.dll from Windows 11 and used it on Windows 10, and it worked. But I guess you won't like such a solution to be an official :)

There is... technically.. a way to do this. It was explored in the past and it adds a lot of complexity. My preference would just be to say "use Windows 11" instead of trying to carry a copy of mssign32.

Windows 10 goes out of mainstream support in 2 years, Windows 11 has been out for 2. Bumping the OS requirement seems more reasonable to me than trying to do all of the gymnastics needed to ship a native library.

Please see https://github.com/dotnet/sign as this tool redistributes the necessary versions of the files to work on all platforms. We will release an update that pulls in the latest AzureSignTool.Core library with these updates.

/cc @dtivel