Skip to content
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

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

VaronisContributor
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

vcsjones commented Dec 11, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

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

@vcsjones vcsjones enabled auto-merge December 13, 2023 16:27
@vcsjones vcsjones merged commit c0c8b77 into vcsjones:main Dec 13, 2023
1 check passed
@vcsjones
Copy link
Owner

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

@clairernovotny
Copy link
Collaborator

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

@VaronisContributor VaronisContributor deleted the main-append-signature branch December 14, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants