-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for append signature + tests #214
Add support for append signature + tests #214
Conversation
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:
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. |
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. |
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:
This warning will be logged before the error code is returned from the SignerSignEx3 (as mentioned in the comment). Is this an acceptable practice? |
We tried it on Windows 10 - it doesn't work. |
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 :) |
I would implement this in two way. For the AzureSignTool CLI in if (AppendSignature && !OperatingSystem.IsWindowsVersionAtLeast(10, 0, 22000)) {
return new ValidationResult("--append-signature requires Windows 11 or later.", new[] { nameof(AppendSignature) });
} And in the actual 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. |
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. |
Agree with you. I've updated it according to the requirements. Please review. |
Thank for very much for the contribution and bearing with me during the review! |
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 |
Expose the existing Win API option to add a signature instead of overriding through the new optional command line argument: -as or --append-signature