ebourg / jsign

Java implementation of Microsoft Authenticode for signing Windows executables, installers & scripts
https://ebourg.github.io/jsign
Apache License 2.0
250 stars 107 forks source link

Allow blank tsaurl value #71

Closed tresf closed 4 years ago

tresf commented 4 years ago

Ant tasks are cumbersome in their own right, but one thing that's particularly frustrating about ant is the lack of good conditional support in a target.

As a side-effect, ugly patterns like this emerge...

<!-- Handle missing property using "unless" -->
<target name="my-target-condition-1" unless="some.property">
   <!-- ... --> 
</target>
<!-- Handle existing property using "if" -->
<target name="my-target-condition-2" if="some.property">
   <!-- ... --> 
</target>
<!-- Wrap conditional property into depends -->
<target name="my-target" depends="my-target-condition-1,my-target-condition-2">
   <!-- ... --> 
</target>

Although this pattern is a side-effect of ant (not jsign specifically), handling the parameter's lib-side can avoid this...

For example... if a build system has two build configurations:

It would make sense to have a way to provide the ant-task an empty value (e.g. for tsaurl=) instead of two separate targets.

Expanding this to JSign:

<target name="sign-exe">
   <taskdef name="jsign" classname="net.jsign.JsignTask" classpath="path/to/jsign-3.0-SNAPSHOT.jar"/>

   <jsign file="my-program.exe"
      name="My Program"
      url="https://ebourg.github.io/jsign/"
      keystore="my-keystore.pfx"
      alias="my-alias"
      storepass="P@ssw0rd"
      keypass="P@ssw0rd"
      #### a blank to skip timestamping? ####
      tsaurl="${signing.tsaurl}"
   />
</target>

Note, this same problem occurs for many command-line invocations; ant users are used to it. If the request is closed as wontfix, I would completely understand. πŸ˜„

ebourg commented 4 years ago

I don't mind ignoring blank values for the tsaurl parameter, but in your example if ${signing.tsaurl} isn't defined it won't be blank, it'll be the string "${signing.tsaurl}".

For the command line I don't think it's possible to ignore a blank value, because the parser expects a parameter after --tsaurl.

tresf commented 4 years ago

in your example if ${signing.tsaurl} isn't defined it won't be blank, it'll be the string "${signing.tsaurl}".

Good catch. For this, I would use a property getter/setter.

<!-- blank if not already set -->
<condition property="signing.tsaurl" value="">
   <not>
      <isset property="signing.tsaurl"/>
   </not>
</condition>

... or less obvious... because... ant πŸ˜†

<!-- blank if not already set -->
<property name="signing.tsaurl" value=""/>

For the command line I don't think it's possible to ignore a blank value, because the parser expects a parameter after --tsaurl.

This would depend on invocation, but '' or "" would work, no?

ebourg commented 4 years ago

This would depend on invocation, but '' or "" would work, no?

I don't know, this has to be tested.

Would you want to submit a PR implementing this? I can review and merge it into the upcoming version 3.0. I think a simple change in SignerHelper will do it.

tresf commented 4 years ago

I have the code staged here https://github.com/ebourg/jsign/compare/master...tresf:patch-1.

As I'm using it, I have some conflicts with its behavior...

  1. My <!-- blank if not already set --> technique is flawed, it modifies parameters that may be used elsewhere.
  2. Your point it'll be the string "${signing.tsaurl}". means I still need additional variables and/or conditional logic.
  3. Ant's own signjar task behaves the same way.

For these reasons, I'm inclined to say we're breaking a -- albeit very ugly -- ant paradigm and we should leave it as-is. πŸ™