[MJARSIGNER-41] Retry and delay feature when signing by schedin · Pull Request #13 · apache/maven-jarsigner-plugin

schedin · 2023-12-06T11:01:00Z

This pull request implements https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-41 that introduces a retry feature for when jar signing fails (typically for network issues). It also implements a delay feature between retries.

In this pull request I have:

Used the pull request Implement maxTries parameter #1 by @lasselindqvist as base along with the branch https://github.com/apache/maven-jarsigner-plugin/tree/MJARSIGNER-41 by @slachiewicz.
Also integrated the suggested change by adds maxTries and retryDelay #3 by @bdt-stru.
Fixed https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-68 (edit: made dedicated pull request [MJARSIGNER-68] Fixing integration test src/it/verify-fail #15 for this)
Written a huge number of tests.

I thought a lot about whether both signing and verification should have the retry feature. In the end I felt the code would be better if only the signing had the retry feature. I’m unsure if any verification may fail due to external circumstances. Perhaps if external Certificate Revocation Lists (CRLs) are used? But I’m unsure if jarsigner performs such a check to an external network resource?

I opted to use @since 3.1.0 declaration(s) for the added parameters. Feel free to change/suggest other values for this!

I spent some time thinking about if a linear or exponential backoff algorithm for sleeping should be used. In the end I made a simple exponential backoff implementation with only one configurable parameter (maxRetryDelay). It would be possible to also add configuration for initalDelay (hard coded to 1 second) and multiplier (hard coded to 2), but I choose not to do this to keep things simple.

I considered implementing support for multiple tsa lists. It was suggested by Thorsten Meinl as a patch to https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-59 and also by @jcompagner in #1 (comment). I think this could be a good idea, but I did not want to “pollute” my pull request. If it was to be implemented I think the way forward is to change type of JarsignerSignMojo.tsa from a String to String[] and let Maven default array handling handle this (it will then handle both using comma separation, or nested XML tags). But my guess is that an adding of tsa list would “force” to also use a list for -tsacert and also the non-existent parameter -tsapolicyid? Because tsa, tsacert and tsapolicyid all belong together as a triplet.

I implemented varargs for the getMessage() methods to make them cleaner.

With the combination of what bdt-stru and lasselindqvist added I ended up adding one log and one obscure exception (that will not happen often) that does not have internationalization (see JarsignerSignMojo.defaultWaitStrategy()). My own feeling is that if the Mojo configuration site documentation does not have internationalization, is it that important that some corner case log messages also does not have it?

I spend a lot of time writing test cases. My motivation for this is that I like tests and that I plan to continue working to implement https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-72. I suspect I will need some refactoring and a safety net when I begin with that.

While writing test case I saw that maven-jarsigner-plugin was missing support for the -signedjar parameter. But I did not have a direct need to have this feature, so I did not implement it. I cannot find any ticket for this on https://issues.apache.org/jira/projects/MJARSIGNER/issues so I guess there is no high need for it.

I tried using maven Maven Plugin Testing Harness (https://maven.apache.org/plugin-testing/maven-plugin-testing-harness/). I spent many hours on it. I felt that the documentation/implementation was not mature enough to use. I did not find any good way of injecting custom instances of JarSigner/MavenSession/MavenProject. It is possible to make Maven Plugin Testing Harness instantiate a class (that is runing the default constructor). But that was not enough for me: I needed to tailor the instances. The way to make it instantiate custom classes was too cumbersome in my opinion (that I had to go via a pom.xml file on disk). It was possible to create via Java methods only, but those methods were not documented. All in all, I decided to create my own mini-test hardness framework (see MojoTestCreator.java and PluginXmlParser.java).

Since slachiewicz, in the integration branch https://github.com/apache/maven-jarsigner-plugin/tree/MJARSIGNER-41, tried to use jacoco for code coverage I completed the configuration so that Jacoco will output HTML reports in target/site/jacoco/index.html. Unfortunately, I don’t have access or don’t fully understand how the CI (Jenkins?) server https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-jarsigner-plugin/ works to know what good values are for a code coverage report to be shown on a job build page.

Just as Elliotte Rusty has noted in https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-68 the integration test /src/it/verify-fail/pom.xml fails when I executed it (mvn clean install -P+run-its). I have made a fix for this and documented how I did to generate the tampered.jar in src/it/verify-fail/pom.xml.

slachiewicz · 2023-12-06T11:22:12Z

Amazing work, thank you.
let's see what CI will say

elharo

Looks good overall. If the fix for the CI can be separated out and landed first, that would be great.

elharo · 2023-12-07T12:20:44Z

+     * if any network operations are used during signing, for example using a Time Stamp Authority or network based
+     * PKCS11 HSM solution for storing code signing keys.
+     *
+     * The default value of 1 indicate that no retries should be made.


indicates
What if someone sets this to zero or a negative number?

Added method AbstractJarsignerMojo.validateParameters() and overridden in JarsignerSignMojo. Added maxTries and maxRetryDelaySeconds contraint check that will reset to default values and log if checks fail. Also added test cases to verify correct behaviour (see JarsignerSignMojoRetryTest and methods testInvalidMaxTries_zero(), testInvalidMaxTries_negative(), testMaxRetryDelaySeconds(), testInvalidMaxRetryDelaySeconds_negative()).

elharo · 2023-12-07T12:22:30Z

+            throws JavaToolException, MojoExecutionException {
+        Commandline commandLine = null;
+        int resultCode = 0;
+        for (int attempt = 0; attempt < maxTries; attempt++) {


this doesn't try even once if the value is 0 or less.

Fixed via JarsignerSignMojo.validateParameters().

elharo · 2023-12-07T12:24:15Z

+                waitStrategy.waitAfterFailure(attempt, Duration.ofSeconds(maxRetryDelaySeconds));
+            }
+        }
+        throw new MojoExecutionException(getMessage("failure", getCommandlineInfo(commandLine), resultCode));


This returns the last failure only. Perhaps good enough but there could be multiple different failures

I have made a minor refactor of the for-loop so that it is clearer in the source code that it is the last failure that is returned. I have also added test case JarsignerSignMojoRetryTest.testLastErrorReturned() to verify this behavior.

elharo · 2023-12-07T12:27:09Z

+    }
+
+    private void defaultWaitStrategy(int attempt, Duration maxRetryDelay) throws MojoExecutionException {
+        long delayMillis = (long) (Duration.ofSeconds(1).toMillis() * Math.pow(2, attempt));


possible overflow and wraparound here.
There should likely be a maximum retry length.

Refactored method to make it testable. Added test case JarsignerSignMojoRetryTest.testDefaultWaitStrategy() that tests/proves that there are no unexpected overflow/wraparound problems (at least not visible from the outside).

Maybe. Math.pow(2, attempt) still worries me. That can get very big very fast. Perhaps set a maximum on attempts here. That is, don't go above a few minutes. attempt <= 20. You can retry more, though I wouldn't wait more than a few minutes between attempts, no matter how many times.

One or two retries might be OK. But if it fails three times, do we really want to just keep trying?

schedin · 2023-12-07T17:03:45Z

Thanks for the feedback @elharo! I have fixed the easy things, and also created a dedicated pull request #15 for MJARSIGNER-68! I will look at and fix the rest of the feedback soon (1 to 2 working days)!

schedin · 2023-12-08T14:55:54Z

+        if (this.skip) {
+            getLog().info(getMessage("disabled"));
+            return;
+        }


I did an indentation change for this method that my IDE showed nicely, but GitHub diff goes bananas over it. I could change it back. However in the scope of https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-72 I still plan to refactor this method. I'm not sure if MJARSIGNER-72 will be accepted by the community, but I will still implement it because I have a direct need for it.

schedin · 2023-12-10T09:53:46Z

I have resolved/fixed all code review comments (at least from my perspective) and have now set pull request back to "ready for review".

elharo · 2023-12-10T22:44:20Z

+    }
+
+    private void defaultWaitStrategy(int attempt, Duration maxRetryDelay) throws MojoExecutionException {
+        long delayMillis = (long) (Duration.ofSeconds(1).toMillis() * Math.pow(2, attempt));


Maybe. Math.pow(2, attempt) still worries me. That can get very big very fast. Perhaps set a maximum on attempts here. That is, don't go above a few minutes. attempt <= 20. You can retry more, though I wouldn't wait more than a few minutes between attempts, no matter how many times.

One or two retries might be OK. But if it fails three times, do we really want to just keep trying?

Implementation of retry and delay feature when signing and test cases

schedin · 2023-12-11T08:42:42Z

I have fixed JUnit version.

I don't agree there is any problem in JarsignerSignMojo.waitAfterFailure() because it had test cases to test limit behaviors in addition to a maximum check and a negative check:

        delayMillis = Math.min(delayMillis, maxRetryDelay.toMillis());
        if (delayMillis > 0) {

However I have anyway added limit on what value (20) the exponent should have in the function and updated the test cases to reflect this.

VirtualTim · 2024-02-14T01:14:31Z

@elharo would it be possible to release a new version of this plugin containing this?
My signing seems to be pretty flaky, and this would really help me out. Plus #18 sounds like it would also be really beneficial.

schedin · 2024-02-14T08:25:47Z

I also think it would be good to release a new version of the plugin. For what it is worth I have been running a custom build from the state of master branch the 19th of December in my company. We have probably built over 1000 builds since then. The settings I use:

-Djarsigner.storetype=PKCS11 -Djarsigner.providerClass=sun.security.pkcs11.SunPKCS11 -Djarsigner.providerArg=/opt/Keylockertools-linux-x64/pkcs11properties.cfg -Djarsigner.storepass=NONE -Djarsigner.alias=${GLOBAL_CODE_SIGN_ALIAS} -Djarsigner.plugin.version=3.1.0-schedin-20231219 -Djarsigner.maxTries=10 -Djarsigner.threadCount=16

(also tagging @slachiewicz)

schedin · 2024-03-25T07:12:44Z

@elharo and @slachiewicz: Is there any plan on making a new release for maven-jarsigner-plugin? I think the features I have implemented would be beneficial for others as well.

VirtualTim · 2024-08-12T02:15:28Z

@elharo @slachiewicz @hboutemy
Sorry to bother you guys, but is there any plan on releasing a new version of this plugin?

slawekjaranowski · 2024-08-12T09:22:48Z

Ok I will do it 😄 in a few days

jira-importer · 2025-06-18T23:19:31Z

Resolve #66

slachiewicz requested a review from slawekjaranowski December 6, 2023 11:40

elharo changed the title ~~MJARSIGNER-41 retry and delay feature when signing~~ [MJARSIGNER-41] retry and delay feature when signing Dec 7, 2023

elharo changed the title ~~[MJARSIGNER-41] retry and delay feature when signing~~ [MJARSIGNER-41][MJARSIGNER-68] retry and delay feature when signing Dec 7, 2023

elharo reviewed Dec 7, 2023

View reviewed changes

schedin force-pushed the MJARSIGNER-41_maxTries_and_retryDelay branch 2 times, most recently from 1c46d0a to d35a8aa Compare December 7, 2023 16:52

schedin changed the title ~~[MJARSIGNER-41][MJARSIGNER-68] retry and delay feature when signing~~ [MJARSIGNER-41] Retry and delay feature when signing Dec 8, 2023

schedin force-pushed the MJARSIGNER-41_maxTries_and_retryDelay branch from 25f8e98 to 8e786d4 Compare December 8, 2023 08:22

schedin marked this pull request as draft December 8, 2023 08:46

schedin force-pushed the MJARSIGNER-41_maxTries_and_retryDelay branch from 8e786d4 to fd1fb4f Compare December 8, 2023 14:47

schedin commented Dec 8, 2023

View reviewed changes

schedin force-pushed the MJARSIGNER-41_maxTries_and_retryDelay branch from fd1fb4f to 2c8b205 Compare December 10, 2023 09:45

schedin marked this pull request as ready for review December 10, 2023 09:52

elharo reviewed Dec 10, 2023

View reviewed changes

[MJARSIGNER-41] Retry and delay feature when signing

529d2cf

Implementation of retry and delay feature when signing and test cases

schedin force-pushed the MJARSIGNER-41_maxTries_and_retryDelay branch from 2c8b205 to 529d2cf Compare December 11, 2023 08:36

elharo approved these changes Dec 11, 2023

View reviewed changes

elharo merged commit 26c070d into apache:master Dec 11, 2023

This was referenced Dec 11, 2023

adds maxTries and retryDelay #3

Closed

Implement maxTries parameter #1

Closed

Conversation

schedin commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slachiewicz commented Dec 6, 2023

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schedin Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elharo Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schedin commented Dec 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schedin commented Dec 10, 2023

Uh oh!

Uh oh!

elharo Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schedin commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VirtualTim commented Feb 14, 2024

Uh oh!

schedin commented Feb 14, 2024

Uh oh!

schedin commented Mar 25, 2024

Uh oh!

VirtualTim commented Aug 12, 2024

Uh oh!

slawekjaranowski commented Aug 12, 2024

Uh oh!

jira-importer commented Jun 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

schedin commented Dec 6, 2023 •

edited

Loading

schedin Dec 10, 2023 •

edited

Loading

elharo Dec 10, 2023 •

edited

Loading

elharo Dec 10, 2023 •

edited

Loading

schedin commented Dec 11, 2023 •

edited

Loading