dotnet / sign

Code Signing CLI tool supporting Authenticode, NuGet, VSIX, and ClickOnce
MIT License
447 stars 84 forks source link

Question: Why description and description URL arguments are mandatory? #642

Closed SemyonL closed 2 weeks ago

SemyonL commented 9 months ago

The tool requires two mandatory arguments:

-d, --description <description> (REQUIRED)             Description of the signing certificate.
-u, --description-url <description-url> (REQUIRED)     Description URL of the signing certificate.

However, signing tools like azuresigntool, nuget sign, signtool do not require these arguments as mandatory. What's the reasoning of making those arguments required?

dtivel commented 7 months ago

These arguments are used for Authenticode and ClickOnce signing. If you aren't doing Authenticode or ClickOnce signing, it does seem weird for Sign CLI to require arguments anyway. The current problem is that Sign CLI doesn't know at the time of argument parsing if it will be doing Authenticode or ClickOnce signing. A signing operation could fail if these values were required but not supplied.

We should look into how to make this better. Ideas include:

dlemstra commented 4 weeks ago

The --description and --description-url options are only required by AzureSignToolSignatureProvider. ClickOnceSignatureProvider uses --description-url but this is optional.

I did some local testing and made them both null inside the AzureSignToolSignatureProvider:

using (var ctx = new Kernel32.ActivationContext(manifestFile))
{
    code = signer.SignFile(
        file.FullName,
        description: null,
        descriptionUrl: null,
        pageHashing: null,
        _logger);
    success = code == S_OK;
}

This seems to sign a file without any issues. I also double checked with GitHub Copilot and got this answer:

No, both pwszName and pwszDescription in the SIGNER_ATTR_AUTHCODE structure are not required fields.

The pwszName field is used to specify the name of the signed content, and pwszDescription is used to provide a description of the signed content. If either of these fields is set to NULL, the signing process will continue without these pieces of information.

I think we could remove setting IsRequired to true for both these fields? I can open up a PR for this if the team agrees with making them optional?

clairernovotny commented 2 weeks ago

These parameters have been made optional and will be reflected in the next release.