[MGPG-79] fix handling of external pinentry programs in case the passphrase is not given #9

Syquel · 2021-02-26T11:24:11Z

Fixes MGPG-79 by using the default "pinentry-mode" if no passphrase for the GPG key is given.
Only in case a passphrase for the GPG key is given and shall be passed to GPG via stdin, the "pinentry-mode" is set to "loopback".

Additionally the "--batch" option is set as long as Maven is running in "non-interactive" mode to prevent other GPG input prompts (e.g. prompting to overwrite an already existing signature file), which would cause the command to hang indefinitely.

Syquel · 2021-03-01T14:06:46Z

Unfortunately I am not able to provide a proper test for this scenario, because the bug only occurs if Maven is being run in "interactive" mode and the maven-invoker-plugin always activates the Maven batch mode.
I tried to set invoker.mavenOpts = -DinteractiveMode=true in invoker.properties and <interactiveMode>true</interactiveMode> in settings.xml without any success (as expected).

Does someone else have an idea how to implement a test?

elharo

Perhaps you could test this by extracting the generation of the command line to a separate package protected method that is visible for testing. You can test that the command line is as expected without actually executing it.

elharo · 2021-03-01T16:23:45Z

running through jenkins: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-gpg-plugin/job/MGPG-79/

Syquel · 2021-03-01T16:24:52Z

My problem is that I only know that it behaves correctly now, because I did a lot of manual testing.
Optimally we could define a behavioral test to ensure that it will behave in the same way in the future even if GPG changes its behavior again.

I am thinking of implementing a JUnit based maven-invoker test (without maven-invoker-plugin) so I can set Maven to "interactive".

famod · 2021-03-01T20:45:40Z

Fix confirmed locally on my Windows box. Thanks!

My 2 cents on the test: Although I'm all for "test first" and high code coverage, sometimes it doesn't make sense to try to test everything automatically.

Syquel · 2021-03-02T11:31:27Z

I added an integration test for the Maven interactive mode use case via a JUnit-based maven-invoker test.
Hope it does not break the scope of this PR. We could move it to a separate PR otherwise.

I think it is important to have such a behavioral test so the next one who wants to fix a bug or even refactor the code has a clearly defined expected behavior and does not have to figure out on his own how the plugin should behave (as I had to).

Syquel · 2021-03-10T12:48:56Z

@elharo it would be great if you could review this PR again / trigger a new Jenkins build.

elharo

let me push it to jenkins

elharo · 2021-03-10T13:28:50Z

running at https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-gpg-plugin/job/mgpg-79/

elharo · 2021-03-10T14:39:36Z

GpgSignAttachedMojoIT.testInteractiveWithoutPassphrase appears to be failing

elharo

GpgSignAttachedMojoIT.testInteractiveWithoutPassphrase

Syquel · 2021-03-10T16:24:55Z

I was able to reproduce the issue with GpgSignAttachedMojoIT.testInteractiveWithoutPassphrase locally on Linux by unsetting the environment variable "GPG_TTY".

I pushed a new commit, which sets an invalid pinentry program for gpg-agent (instead of letting an actual one time out).
That should enable the test to run on Jenkins.

Sorry for that. I did not take Jenkins into account.

ghost · 2021-03-15T20:35:31Z

While the previous commit failed, the latest one 8171fa1 also works on macOS 11.2.3 (Apple Silicon, gpg (GnuPG/MacGPG2) 2.2.24). Java reports as: OS name: "mac os x", version: "10.16", arch: "x86_64", family: "mac"

elharo · 2021-03-16T10:57:12Z

let's see what Jenkins says:

https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-gpg-plugin/job/MGPG-79B/

elharo · 2021-03-16T13:46:59Z

Jenkins is not well designed so it's hard to find the output, but there seem to still be test failures.

Syquel · 2021-03-16T14:03:43Z

This time it seems that there was an issue in the JUnit maven-invoker test while connecting to Maven Central.
The ITs which are execute via maven-invoker-plugin are configured to use the the local repository I think.
Maybe to prevent the ITs from connecting to Maven Central?

Do you know if I am correct?

elharo · 2021-03-16T14:48:37Z

No, I don't know. You could ask on the dev mailing list. Jenkins is not well designed from a UI perspective. I'll try just rerunning and see what happens.

elharo · 2021-03-16T14:49:21Z

new build running: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-gpg-plugin/job/MGPG-79B/2/

Syquel · 2021-03-16T16:09:06Z

The log of the failing test can be found here.

[ERROR] Plugin org.apache.maven.plugins:maven-source-plugin:2.0.4 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.apache.maven.plugins:maven-source-plugin:jar:2.0.4: Could not transfer artifact org.apache.maven.plugins:maven-source-plugin:pom:2.0.4 from/to central (https://repo.maven.apache.org/maven2): Received fatal alert: protocol_version -> [Help 1]
[...]
Caused by: javax.net.ssl.SSLException: Received fatal alert: protocol_version
[...]

I pushed a new commit which aligns the JUnit-based maven-invoker tests with the maven-invoker-plugin ITs by utilizing the local Maven repository as remote repository.

If that does not help I will ask on the dev mailing list. Thank's for the pointer.

elharo · 2021-03-16T18:27:11Z

Haven't tried the latest commit, but now I see this. Maven 3.2.5 is quite old, as is jdk1.7.0_80. It's possible it's having SSL problems connecting to Maven Central.

Maven execution failed because no pinentry program is available
Expected: a string containing "[GNUPG:] FAILURE sign 67108949"
     but: was "[DEBUG] Using ${maven.home} of: '/home/jenkins/tools/maven/apache-maven-3.2.5'.
[DEBUG] Executing: /bin/sh -c cd '/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2/linux-jdk7-m3.2.x_build/target/test-classes/it/sign-release-without-passphrase-interactive' && '/usr/local/asfpackages/maven/apache-maven-3.2.5/bin/mvn' '-X' '-V' '-D' 'maven.repo.local=/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2/linux-jdk7-m3.2.x_build/target/local-repo' '-s' '/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2/linux-jdk7-m3.2.x_build/target/test-classes/it/settings.xml' '-D' 'gpg.homedir=/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2/linux-jdk7-m3.2.x_build/target/test-classes/gnupg' 'clean' 'install'
Picked up JAVA_TOOL_OPTIONS: -Dmaven.ext.class.path="/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2@tmp/withMaven287550cf/pipeline-maven-spy.jar" -Dorg.jenkinsci.plugins.pipeline.maven.reportsFolder="/home/jenkins/workspace/en-box_maven-gpg-plugin_MGPG-79B@2@tmp/withMaven287550cf" 
Apache Maven 3.2.5 (12a6b3acb947671f09b81f49094c53f426d8cea1; 2014-12-14T17:29:23+00:00)
Maven home: /home/jenkins/tools/maven/apache-maven-3.2.5
Java version: 1.7.0_80, vendor: Oracle Corporation
Java home: /usr/local/asfpackages/java/jdk1.7.0_80/jre
Default locale: en_US, platform encoding: ISO-8859-1
OS name: "linux", version: "4.15.0-74-generic", arch: "amd64", family: "unix"

bmarwell · 2021-03-16T19:23:37Z

Haven't tried the latest commit, but now I see this. Maven 3.2.5 is quite old, as is jdk1.7.0_80. It's possible it's having SSL problems connecting to Maven Central.

That is correct. TLS 1.2 (and even 1.3!) was backported to Java 8, but Java 7 can only connect to TLS 1.0 endpoints iirc.
Java 8 can create Java 7 bytecode, so we are fine here.

rfscholte · 2021-03-16T21:57:30Z

Should not be an issue on Jenkins @ a.o , INFRA fixed this with https://issues.apache.org/jira/browse/INFRA-19861

Syquel · 2021-03-16T22:06:23Z

Thanks to your comments I was able to reproduce the issue locally with Oracle JDK 7.

Apparently the maven-invoker-plugin sets the system property https.protocols, which was missing in my JUnit-based maven-invoker test.
Now I am stuck at an issue with the certificate chain, but that will have to wait until tomorrow unfortunately.

I am not sure whether my testing approach with a JUnit-based maven-invoker test is still viable.
Maybe I should just go with the approach @elharo initially suggested?

Syquel · 2021-03-17T12:01:39Z

I pushed a new commit, which sets https.protocols if that System property exists.
Apparently Jenkins sets this property exclusively for JDK7 builds.

I could verify that it works on Windows and Linux with Oracle JDK 7.

elharo · 2021-03-17T13:45:29Z

Checking now. Meanwhile it might help if you can merge master and squash your commits. The git log is getting complex, and I'm not sure I'm testing what you're sending.

Syquel · 2021-03-17T13:56:41Z

The build looks good :)
Should I squash it into one single commit or just into less commits?

…phrase is not given.

…interactive mode

…ractive mode

elharo · 2021-03-17T14:28:35Z

Checking again: https://ci-builds.apache.org/job/Maven/job/maven-box/job/maven-gpg-plugin/job/MGPG-79C/

In general, just squash everything.

elharo

jenkins passed

elharo reviewed Mar 1, 2021

View reviewed changes

elharo approved these changes Mar 10, 2021

View reviewed changes

elharo requested changes Mar 10, 2021

View reviewed changes

Syquel requested a review from elharo March 16, 2021 09:02

Syquel added 3 commits March 17, 2021 14:59

[MGPG-79] fix handling of external pinentry programs in case the pass…

d8502dd

…phrase is not given.

[MGPG-79] add test for handling of external pinentry programs in non-…

f9e0450

…interactive mode

[MGPG-79] add test for handling of external pinentry programs in inte…

0768402

…ractive mode

elharo approved these changes Mar 17, 2021

View reviewed changes

elharo merged commit e4dc062 into apache:master Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MGPG-79] fix handling of external pinentry programs in case the passphrase is not given #9

[MGPG-79] fix handling of external pinentry programs in case the passphrase is not given #9

Syquel commented Feb 26, 2021

Syquel commented Mar 1, 2021 •

edited

elharo left a comment

elharo commented Mar 1, 2021

Syquel commented Mar 1, 2021

famod commented Mar 1, 2021

Syquel commented Mar 2, 2021 •

edited

Syquel commented Mar 10, 2021

elharo left a comment

elharo commented Mar 10, 2021

elharo commented Mar 10, 2021

elharo left a comment

Syquel commented Mar 10, 2021 •

edited

ghost commented Mar 15, 2021

elharo commented Mar 16, 2021

elharo commented Mar 16, 2021

Syquel commented Mar 16, 2021

elharo commented Mar 16, 2021

elharo commented Mar 16, 2021

Syquel commented Mar 16, 2021 •

edited

elharo commented Mar 16, 2021

bmarwell commented Mar 16, 2021

rfscholte commented Mar 16, 2021

Syquel commented Mar 16, 2021

Syquel commented Mar 17, 2021

elharo commented Mar 17, 2021

Syquel commented Mar 17, 2021

elharo commented Mar 17, 2021

elharo left a comment

[MGPG-79] fix handling of external pinentry programs in case the passphrase is not given #9

[MGPG-79] fix handling of external pinentry programs in case the passphrase is not given #9

Conversation

Syquel commented Feb 26, 2021

Syquel commented Mar 1, 2021 • edited

elharo left a comment

Choose a reason for hiding this comment

elharo commented Mar 1, 2021

Syquel commented Mar 1, 2021

famod commented Mar 1, 2021

Syquel commented Mar 2, 2021 • edited

Syquel commented Mar 10, 2021

elharo left a comment

Choose a reason for hiding this comment

elharo commented Mar 10, 2021

elharo commented Mar 10, 2021

elharo left a comment

Choose a reason for hiding this comment

Syquel commented Mar 10, 2021 • edited

ghost commented Mar 15, 2021

elharo commented Mar 16, 2021

elharo commented Mar 16, 2021

Syquel commented Mar 16, 2021

elharo commented Mar 16, 2021

elharo commented Mar 16, 2021

Syquel commented Mar 16, 2021 • edited

elharo commented Mar 16, 2021

bmarwell commented Mar 16, 2021

rfscholte commented Mar 16, 2021

Syquel commented Mar 16, 2021

Syquel commented Mar 17, 2021

elharo commented Mar 17, 2021

Syquel commented Mar 17, 2021

elharo commented Mar 17, 2021

elharo left a comment

Choose a reason for hiding this comment

Syquel commented Mar 1, 2021 •

edited

Syquel commented Mar 2, 2021 •

edited

Syquel commented Mar 10, 2021 •

edited

Syquel commented Mar 16, 2021 •

edited