elharo commented on code in PR #13:
URL:
https://github.com/apache/maven-jarsigner-plugin/pull/13#discussion_r1418878067
##########
pom.xml:
##########
@@ -128,6 +129,25 @@ under the License.
<artifactId>maven-jarsigner</artifactId>
<version>3.0.0</version>
</dependency>
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>4.13</version>
+ <scope>test</scope>
+ </dependency>
+ <!-- Used for test cases to perfor simple logging without any SLF4J
warning -->
Review Comment:
perform
##########
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java:
##########
@@ -503,23 +503,23 @@ private void processArchive(final File archive) throws
MojoExecutionException {
request.setStorepass(decrypt(storepass));
try {
- JavaToolResult result = jarSigner.execute(request);
-
- Commandline commandLine = result.getCommandline();
-
- int resultCode = result.getExitCode();
-
- if (resultCode != 0) {
- // CHECKSTYLE_OFF: LineLength
- throw new MojoExecutionException(getMessage("failure",
getCommandlineInfo(commandLine), resultCode));
- // CHECKSTYLE_ON: LineLength
- }
-
+ executeJarSigner(jarSigner, request);
} catch (JavaToolException e) {
throw new
MojoExecutionException(getMessage("commandLineException", e.getMessage()), e);
}
}
+ /**
+ * Executes jarsigner (execute signing or verification for a jar file).
+ *
+ * @param jarSigner the JarSigner execution interface.
Review Comment:
nit: no period at end of sentence fragment Javadoc, per Oracle guidelines
##########
src/it/verify-fail/pom.xml:
##########
@@ -63,6 +63,33 @@ under the License.
<version>@project.version@</version>
<configuration>
<archive>tampered.jar</archive>
+<!--
+The tampered.jar is a jar file the originally was correctly signed, but then
was "tampered with"
Review Comment:
the --> that
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -126,4 +155,62 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
request.setKeypass(decrypt(keypass));
return request;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * Will retry signing up to maxTries times if it fails.
+ *
+ * @throws MojoExecutionException If all signing attempts fail.
+ */
+ @Override
+ protected void executeJarSigner(JarSigner jarSigner, JarSignerRequest
request)
+ throws JavaToolException, MojoExecutionException {
+ Commandline commandLine = null;
+ int resultCode = 0;
+ for (int attempt = 0; attempt < maxTries; attempt++) {
+ JavaToolResult result = jarSigner.execute(request);
+ resultCode = result.getExitCode();
+ commandLine = result.getCommandline();
+ if (resultCode == 0) {
+ return;
+ }
+ if (attempt < maxTries - 1) { // If not last attempt
+ waitStrategy.waitAfterFailure(attempt,
Duration.ofSeconds(maxRetryDelaySeconds));
+ }
+ }
+ throw new MojoExecutionException(getMessage("failure",
getCommandlineInfo(commandLine), resultCode));
+ }
+
+ /** Set current WaitStrategy. Package private for testing. */
+ void setWaitStrategy(WaitStrategy waitStrategy) {
+ this.waitStrategy = waitStrategy;
+ }
+
+ /** Wait/sleep after a signing failure before the next re-try should
happen. */
+ @FunctionalInterface
+ interface WaitStrategy {
+ /**
+ * Will be called after a signing failure, if a re-try is about to
happen. May as a side effect sleep current
+ * thread for some time.
+ * @param attempt the attempt number (0 is the first).
Review Comment:
blank line before @param
##########
src/main/java/org/apache/maven/plugins/jarsigner/AbstractJarsignerMojo.java:
##########
@@ -533,33 +533,21 @@ protected String decrypt(String encoded) throws
MojoExecutionException {
* Gets a message for a given key from the resource bundle backing the
implementation.
*
* @param key The key of the message to return.
- * @param args Arguments to format the message with or {@code null}.
+ * @param args Arguments to format the message with.
Review Comment:
arguments, no period
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -126,4 +155,62 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
request.setKeypass(decrypt(keypass));
return request;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * Will retry signing up to maxTries times if it fails.
+ *
+ * @throws MojoExecutionException If all signing attempts fail.
+ */
+ @Override
+ protected void executeJarSigner(JarSigner jarSigner, JarSignerRequest
request)
+ throws JavaToolException, MojoExecutionException {
+ Commandline commandLine = null;
+ int resultCode = 0;
+ for (int attempt = 0; attempt < maxTries; attempt++) {
+ JavaToolResult result = jarSigner.execute(request);
+ resultCode = result.getExitCode();
+ commandLine = result.getCommandline();
+ if (resultCode == 0) {
+ return;
+ }
+ if (attempt < maxTries - 1) { // If not last attempt
+ waitStrategy.waitAfterFailure(attempt,
Duration.ofSeconds(maxRetryDelaySeconds));
+ }
+ }
+ throw new MojoExecutionException(getMessage("failure",
getCommandlineInfo(commandLine), resultCode));
+ }
+
+ /** Set current WaitStrategy. Package private for testing. */
+ void setWaitStrategy(WaitStrategy waitStrategy) {
+ this.waitStrategy = waitStrategy;
+ }
+
+ /** Wait/sleep after a signing failure before the next re-try should
happen. */
+ @FunctionalInterface
+ interface WaitStrategy {
+ /**
+ * Will be called after a signing failure, if a re-try is about to
happen. May as a side effect sleep current
+ * thread for some time.
+ * @param attempt the attempt number (0 is the first).
+ * @param maxRetryDelay The maximum duration to sleep (may be zero).
+ * @throws MojoExecutionException If the sleep was interrupted.
+ */
+ void waitAfterFailure(int attempt, Duration maxRetryDelay) throws
MojoExecutionException;
+ }
+
+ private void defaultWaitStrategy(int attempt, Duration maxRetryDelay)
throws MojoExecutionException {
+ long delayMillis = (long) (Duration.ofSeconds(1).toMillis() *
Math.pow(2, attempt));
Review Comment:
possible overflow and wraparound here.
There should likely be a maximum retry length.
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -90,6 +94,30 @@ public class JarsignerSignMojo extends AbstractJarsignerMojo
{
@Parameter(property = "jarsigner.certchain", readonly = true, required =
false)
private File certchain;
+ /**
+ * How many times to try to sign a jar (assuming each previous attempt is
a failure). This option may be desirable
+ * 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.
Review Comment:
indicates
What if someone sets this to zero or a negative number?
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -126,4 +155,62 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
request.setKeypass(decrypt(keypass));
return request;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * Will retry signing up to maxTries times if it fails.
+ *
+ * @throws MojoExecutionException If all signing attempts fail.
+ */
+ @Override
+ protected void executeJarSigner(JarSigner jarSigner, JarSignerRequest
request)
+ throws JavaToolException, MojoExecutionException {
+ Commandline commandLine = null;
+ int resultCode = 0;
+ for (int attempt = 0; attempt < maxTries; attempt++) {
Review Comment:
this doesn't try even once if the value is 0 or less.
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -126,4 +155,62 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
request.setKeypass(decrypt(keypass));
return request;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * Will retry signing up to maxTries times if it fails.
+ *
+ * @throws MojoExecutionException If all signing attempts fail.
+ */
+ @Override
+ protected void executeJarSigner(JarSigner jarSigner, JarSignerRequest
request)
+ throws JavaToolException, MojoExecutionException {
+ Commandline commandLine = null;
+ int resultCode = 0;
+ for (int attempt = 0; attempt < maxTries; attempt++) {
+ JavaToolResult result = jarSigner.execute(request);
+ resultCode = result.getExitCode();
+ commandLine = result.getCommandline();
+ if (resultCode == 0) {
+ return;
+ }
+ if (attempt < maxTries - 1) { // If not last attempt
+ waitStrategy.waitAfterFailure(attempt,
Duration.ofSeconds(maxRetryDelaySeconds));
+ }
+ }
+ throw new MojoExecutionException(getMessage("failure",
getCommandlineInfo(commandLine), resultCode));
Review Comment:
This returns the last failure only. Perhaps good enough but there could be
multiple different failures
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -126,4 +155,62 @@ protected JarSignerRequest createRequest(File archive)
throws MojoExecutionExcep
request.setKeypass(decrypt(keypass));
return request;
}
+
+ /**
+ * {@inheritDoc}
+ *
+ * Will retry signing up to maxTries times if it fails.
+ *
+ * @throws MojoExecutionException If all signing attempts fail.
+ */
+ @Override
+ protected void executeJarSigner(JarSigner jarSigner, JarSignerRequest
request)
+ throws JavaToolException, MojoExecutionException {
+ Commandline commandLine = null;
+ int resultCode = 0;
+ for (int attempt = 0; attempt < maxTries; attempt++) {
+ JavaToolResult result = jarSigner.execute(request);
+ resultCode = result.getExitCode();
+ commandLine = result.getCommandline();
+ if (resultCode == 0) {
+ return;
+ }
+ if (attempt < maxTries - 1) { // If not last attempt
+ waitStrategy.waitAfterFailure(attempt,
Duration.ofSeconds(maxRetryDelaySeconds));
+ }
+ }
+ throw new MojoExecutionException(getMessage("failure",
getCommandlineInfo(commandLine), resultCode));
+ }
+
+ /** Set current WaitStrategy. Package private for testing. */
+ void setWaitStrategy(WaitStrategy waitStrategy) {
+ this.waitStrategy = waitStrategy;
+ }
+
+ /** Wait/sleep after a signing failure before the next re-try should
happen. */
+ @FunctionalInterface
+ interface WaitStrategy {
+ /**
+ * Will be called after a signing failure, if a re-try is about to
happen. May as a side effect sleep current
+ * thread for some time.
+ * @param attempt the attempt number (0 is the first).
+ * @param maxRetryDelay The maximum duration to sleep (may be zero).
Review Comment:
the maximum
no period
##########
src/main/java/org/apache/maven/plugins/jarsigner/JarsignerSignMojo.java:
##########
@@ -90,6 +94,30 @@ public class JarsignerSignMojo extends AbstractJarsignerMojo
{
@Parameter(property = "jarsigner.certchain", readonly = true, required =
false)
private File certchain;
+ /**
+ * How many times to try to sign a jar (assuming each previous attempt is
a failure). This option may be desirable
+ * 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.
+ *
+ * @since 3.1.0
+ */
+ @Parameter(property = "jarsigner.maxTries", defaultValue = "1")
+ private int maxTries;
+
+ /**
+ * Maximum delay, in seconds, to wait after a failed attempt before
re-trying. The delay after a failed attempt
+ * follows an exponential backoff strategy, with increasing delay times.
+ *
+ * @since 3.1.0
+ */
+ @Parameter(property = "jarsigner.maxRetryDelaySeconds", defaultValue = "0")
+ private int maxRetryDelaySeconds;
Review Comment:
What if this is negative?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
