[MRESOLVER-301] Artifact generators SPI and GnuPG signature generator #432

cstamas · 2024-02-19T14:01:32Z

Artifact generators and one implementation: GnuPG Signer.

The "signer" generator is simple module for signing artifacts with GnuPG Signer.

https://issues.apache.org/jira/browse/MRESOLVER-301

slawekjaranowski · 2024-02-20T20:34:22Z

At first time looks a complicated a bit.

I can test it in next weekend - I need more time for it.

first questions:

What is difference about gpg singnature and md5, sha checksums - why not use the same same existing implementation - we simply want generate a checksum or signature
What future will be for m-gpg-p when we move generating of signatures to resolver and finaly to Maven core ... yes I see that it is a extension but ...

By the way I think we should discuss such (for me big) architecture change on ML.

cstamas · 2024-02-20T20:54:22Z

Q1: checksums: they are tightly coupled into "basic connector". Historically, "connector" was the transport abstraction, but later "connector + transport" was introduced, and connector deals with checksums ONLY. Original issue was "deal with signatures as with checksums" but this is a no-go, as connector is (theoretically) swappable, while true, we have only "basic" connector and no other implementation. IMHO, we should eventually remove the abstraction from connector and accept that "basic" is THE connector we use.
Q2: First, it is an extension, yes. Second, I see no problem with resolver signing, as to me, it seems like "natural fit". If you think about, signer plugins usually do something and "attach" the result. Am fine with both, and as far I am concerned, the two can coexists even (maybe add some logic to signer to remain dormant if signatures discovered).

cstamas · 2024-02-20T21:03:30Z

Also, "artifact generator" is one thing, and its use for "signing" is another.

If we remain at "publishing to Central" domain, where PGP signature is enforced, and signing, I am not satisfied with any of existing solutions:

maven-sign-plugin uses gpg executable
takari-sign-plugin cannot do ED25519 (but have cool ideas)
s4u sign plugin unused in ASF (but have cool ideas)

So I just "brought" the best of all here. At least, that was my intent. And yes, IMO, "signing" is natural fit for "artifact generator" and IMO we should not complicate our build/POMs for something that is an expected requirement (is like we'd need to add a plugin to POM to create checksums, something also required to publish to Central).

Also, "signer" is extensible, so it does not have to get GnuPG, it could be something else as well... so in this way, it is not in Maven Core (wired in), but can progress and change, maybe as an extension.

...tor-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgAgentPasswordLoader.java

michael-o

I don't have any opinion and cannot object.

michael-o

I'll leave the decision to others.

cstamas · 2024-02-22T10:05:32Z

By the way I think we should discuss such (for me big) architecture change on ML.

@slawekjaranowski this is not "big architectural change" at all, just another option. I modified current PR to:

enabled GnuPG signer step back, if signatures detected among deployable artifacts (ie bui;d uses plugin that produces .asc signatures)
by default signer is disabled (even if present as extension)

gnodet

Mostly coding style issues

gnodet · 2024-02-22T17:12:11Z

...erator-signer/src/main/java/org/eclipse/aether/generator/signer/SignerArtifactGenerator.java

+
+import static java.util.Objects.requireNonNull;
+
+final class SignerArtifactGenerator implements ArtifactGenerator {


Why not public instead of final with a public constructor and protected fields ? Why would not people be allowed to extend it ?

gnodet · 2024-02-22T17:16:15Z

...rator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgEnvPasswordLoader.java

+@Named(GpgEnvPasswordLoader.NAME)
+@Priority(50)
+@SuppressWarnings("checkstyle:magicnumber")
+public final class GpgEnvPasswordLoader implements GpgSignerFactory.KeyPasswordLoader {


Is the final keyword needed ?

gnodet · 2024-02-22T17:16:31Z

...-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgFileRingMaterialLoader.java

+@Singleton
+@Named(GpgFileRingMaterialLoader.NAME)
+@Priority(10)
+public final class GpgFileRingMaterialLoader implements GpgSignerFactory.KeyRingMaterialLoader {


gnodet · 2024-02-22T17:16:55Z

...solver-generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSigner.java

+ * GnuPG {@link Signer} implementation that is kept and reused across session.
+ */
+@SuppressWarnings("checkstyle:magicnumber")
+public final class GpgSigner implements Signer {


gnodet · 2024-02-22T17:17:40Z

...solver-generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSigner.java

+        });
+    }
+
+    private void sign(InputStream content, OutputStream signature) throws IOException {


protected ?

gnodet · 2024-02-22T17:20:06Z

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultDeployer.java

@@ -151,7 +162,23 @@ private DeployResult deploy(SyncContext syncContext, RepositorySystemSession ses
            throw new DeploymentException("Failed to deploy artifacts/metadata: " + e.getMessage(), e);
        }

+        final List<Artifact> artifacts = new ArrayList<>(request.getArtifacts());


I don't see how final is useful. The static analysis very well knows if a variable is final or not.

gnodet · 2024-02-22T17:20:38Z

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java

@@ -95,53 +106,96 @@ private InstallResult install(SyncContext syncContext, RepositorySystemSession s

        RequestTrace trace = RequestTrace.newChild(request.getTrace(), request);

-        List<? extends MetadataGenerator> generators = getMetadataGenerators(session, request);
+        final List<Artifact> artifacts = new ArrayList<>(request.getArtifacts());


Once again ?

cstamas · 2024-02-22T18:32:38Z

For other a general answer: I do agree with all your comments, but

Resolver whole codebase is done as this, and we may want to change all these, but for now there are a lot of package protected and final classes, I did not want to disturb this coherence. But I do agree about non-final/public/protected fields.
BUT, extending JSR330 annotated component is a no-no (hence final)
I like how these classes are being prevented to be tampered with. If someone needs this, can copy, but I don't want to be hindered a year after, as somecome tells me I broke "compatibility" with his app (where he uses or extends these classes).

So, yes, I agree, but this is why not IMO.

cstamas · 2024-02-23T12:21:10Z

Added UT for signer (for now does not assert much). Also verified locally the whole signing workflow along with agent, and it all works nicely, while verified produced signatures with gpg CLI.

The test on purpose use Ed25519 key, as that is where the future moves to.

slawekjaranowski

There are many new Factories ... do we need all of them?

slawekjaranowski · 2024-02-24T11:08:23Z

...signer/src/main/java/org/eclipse/aether/generator/signer/SignerArtifactGeneratorFactory.java

+
+@Singleton
+@Named(SignerArtifactGeneratorFactory.NAME)
+public final class SignerArtifactGeneratorFactory implements ArtifactGeneratorFactory {


It is only one implementation of SignerArtifactGeneratorFactory
why we put it in new module ... maybe should be in maven-resolver-impl

slawekjaranowski · 2024-02-24T11:12:53Z

...solver-generator-signer/src/main/java/org/eclipse/aether/generator/signer/SignerFactory.java

+ *
+ * @since 2.0.0
+ */
+public interface SignerFactory {


next factory similar to ArtifactGeneratorFactory

slawekjaranowski · 2024-02-24T11:25:54Z

...erator-signer/src/main/java/org/eclipse/aether/generator/signer/SignerArtifactGenerator.java

+                        .map(a -> {
+                            HashMap<String, String> properties = new HashMap<>(a.getProperties());
+                            properties.put(
+                                    SignerArtifactGeneratorFactory.ARTIFACT_SIGNER_ID,


This property ARTIFACT_SIGNER_ID is only used in this place .... why we add it?

slawekjaranowski · 2024-02-24T11:30:31Z

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultDeployer.java

        try {
+            List<Artifact> generatedArtifacts = new ArrayList<>();
+            for (ArtifactGenerator artifactGenerator : artifactGenerators) {
+                Collection<? extends Artifact> generated = artifactGenerator.generate(generatedArtifacts);


generatedArtifacts is an empty list at beginning - so how we discover artifacts to sign in artifactGenerator.generate

ok - I see we heve materialized artifact list in org.eclipse.aether.generator.signer.SignerArtifactGenerator
It is look too complicated

slawekjaranowski · 2024-02-24T11:44:56Z

maven-resolver-demos/maven-resolver-demo-snippets/pom.xml

+    <!-- To make Jetty + generator-signer work -->
+    <javaVersion>17</javaVersion>


I only see that UnixDomainSocketAddress used in GpgAgentPasswordLoader requres JDK 16+
so I don't see connections with Jetty

Originally it was 11 as Jetty 10.x we use as client and as server (in http-test) required 11, but now upped to 17.
Will change it: "jetty needs 11, generator-signer needs 17"

slawekjaranowski · 2024-02-24T11:48:30Z

...generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSignerFactory.java

+@Named(GpgSignerFactory.NAME)
+public final class GpgSignerFactory implements SignerFactory {
+    public static final String NAME = GpgConfigurationKeys.NAME;
+    private static final String SIGNER_KEY = GpgSignerFactory.class.getName() + ".signer";


unused constant

slawekjaranowski · 2024-02-24T11:54:08Z

...generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSignerFactory.java

+        }
+
+        Long keyId = null;
+        String keyIdStr = ConfigUtils.getString(session, null, CONFIG_PROP_KEY_ID);


keyId should also be provided by environment value

fixed, now only exists "loaders" (3 of them: env, conf and agent)

cstamas · 2024-02-24T17:33:23Z

@slawekjaranowski general remark:

spi defines ArtifactGenerators, there can exist more of those in the system
generator-signer hooks into this (is one implementation of generators), and define Signers
gpg is one Signer implementation

But also:

there may be multiple generators, see how they are ordered, and the idea is that "signer generator" is last, and it should sign all the deployed but also "so far generated" artifacts (if they are relevant).
gpg may not be the only one signer either
moreover, another signer may sign differently than gpp. Gpg signs each artifact, but another signer may provide one signature for all "relevant" artifacts.

...generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSignerFactory.java

cstamas · 2024-02-24T22:09:55Z

@michael-o @slawekjaranowski @gnodet pls review again.

Reworked, as layout was "improper" for signer (is transport and remote repository related). What was actually missing was the ArtifactPredicate (that recognizes artifact hashes and also configured extensions without hashes). This config was never per-remote repository, so it felt right to be moved to new component. Maven2 repo layout updated accordingly. Finally, this change revealed a (doc) bug: remote repo checksums ARE configurable per remote repository, but doco did not reflect that.

cstamas · 2024-02-24T22:26:12Z

So to recap, what I tried to explain to Slawek. This PR:

defines Artifact Generator SPI (there may be multiple of those present in system)
defines Signer Artifact Generator that is one provider for SPI, that defines Signer API (there may be multiple of those present in system)
implements GpgSigner that is one Signer implementation provided out of the box

Naturally, as generators are ordered, signer generator is expected to be "among last" and sign not only artifacts in request (install/deploy) but also "so far generated artifacts" (if they are relevant).

gnodet · 2024-02-25T09:02:09Z

So to recap, what I tried to explain to Slawek. This PR:

defines Artifact Generator SPI (there may be multiple of those present in system)

defines Signer Artifact Generator that is one provider for SPI, that defines Signer API (there may be multiple of those present in system)

Now that you put it that way, I'm not really convinced we need a Signer SPI here. The code of the ArtifactGenerator which calls the signers is trivial, so it seems to me that the GpgSigner could directly implement the Artifact Generator SPI.

implements GpgSigner that is one Signer implementation provided out of the box

Naturally, as generators are ordered, signer generator is expected to be "among last" and sign not only artifacts in request (install/deploy) but also "so far generated artifacts" (if they are relevant).

cstamas · 2024-02-25T12:23:31Z

Now that you put it that way, I'm not really convinced we need a Signer SPI here. The code of the ArtifactGenerator which calls the signers is trivial, so it seems to me that the GpgSigner could directly implement the Artifact Generator SPI.

Agreed, will simplify it more, but the question begs: "generator-signer" module name becomes wrong, no? Should it be maybe "generator-gnupg (or gpg)" instead?

gnodet · 2024-02-25T13:18:51Z

Now that you put it that way, I'm not really convinced we need a Signer SPI here. The code of the ArtifactGenerator which calls the signers is trivial, so it seems to me that the GpgSigner could directly implement the Artifact Generator SPI.

Agreed, will simplify it more, but the question begs: "generator-signer" module name becomes wrong, no? Should it be maybe "generator-gnupg (or gpg)" instead?

Yes, I think that would make more sense.

cstamas · 2024-02-26T10:15:24Z

Collapsed code and renamed to generator-gnupg

--- https://issues.apache.org/jira/browse/MRESOLVER-301

cstamas self-assigned this Feb 19, 2024

cstamas marked this pull request as ready for review February 19, 2024 18:00

cstamas requested review from gnodet, slawekjaranowski and michael-o February 19, 2024 18:01

cstamas force-pushed the MRESOLVER-301 branch from 3fc4f4c to 8fd2fe1 Compare February 20, 2024 17:00

michael-o requested changes Feb 21, 2024

View reviewed changes

...tor-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgAgentPasswordLoader.java Outdated Show resolved Hide resolved

michael-o self-requested a review February 21, 2024 12:28

michael-o reviewed Feb 21, 2024

View reviewed changes

michael-o self-requested a review February 21, 2024 13:08

michael-o reviewed Feb 21, 2024

View reviewed changes

cstamas force-pushed the MRESOLVER-301 branch from febfac1 to a41067a Compare February 21, 2024 16:09

cstamas force-pushed the MRESOLVER-301 branch from a41067a to 1ad2b13 Compare February 22, 2024 15:49

gnodet approved these changes Feb 22, 2024

View reviewed changes

cstamas force-pushed the MRESOLVER-301 branch from 1ad2b13 to bec77d9 Compare February 22, 2024 18:29

slawekjaranowski reviewed Feb 24, 2024

View reviewed changes

cstamas commented Feb 24, 2024

View reviewed changes

...generator-signer/src/main/java/org/eclipse/aether/generator/signer/gpg/GpgSignerFactory.java Outdated Show resolved Hide resolved

cstamas force-pushed the MRESOLVER-301 branch from fdea33b to eb5796c Compare February 24, 2024 22:01

cstamas requested review from michael-o, slawekjaranowski and gnodet February 24, 2024 22:10

cstamas changed the title ~~[MRESOLVER-301] Artifact generators~~ [MRESOLVER-301] Artifact generators SPI and GnuPG signature generator Feb 25, 2024

[MRESOLVER-301] Artifact generators

e3e9602

--- https://issues.apache.org/jira/browse/MRESOLVER-301

cstamas force-pushed the MRESOLVER-301 branch from c5dd853 to e3e9602 Compare February 26, 2024 10:27

gnodet approved these changes Feb 26, 2024

View reviewed changes

cstamas merged commit e152bf3 into apache:master Feb 26, 2024
4 checks passed

cstamas deleted the MRESOLVER-301 branch February 26, 2024 17:05


		import static java.util.Objects.requireNonNull;

		final class SignerArtifactGenerator implements ArtifactGenerator {

		<!-- To make Jetty + generator-signer work -->
		<javaVersion>17</javaVersion>

[MRESOLVER-301] Artifact generators SPI and GnuPG signature generator #432

[MRESOLVER-301] Artifact generators SPI and GnuPG signature generator #432

Conversation

cstamas commented Feb 19, 2024 • edited

slawekjaranowski commented Feb 20, 2024

cstamas commented Feb 20, 2024

cstamas commented Feb 20, 2024

michael-o left a comment

Choose a reason for hiding this comment

michael-o left a comment

Choose a reason for hiding this comment

cstamas commented Feb 22, 2024

gnodet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Feb 22, 2024

cstamas commented Feb 23, 2024 • edited

slawekjaranowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas commented Feb 24, 2024 • edited

cstamas commented Feb 24, 2024 • edited

cstamas commented Feb 24, 2024

gnodet commented Feb 25, 2024

cstamas commented Feb 25, 2024 • edited

gnodet commented Feb 25, 2024

cstamas commented Feb 26, 2024

cstamas commented Feb 19, 2024 •

edited

cstamas commented Feb 23, 2024 •

edited

cstamas commented Feb 24, 2024 •

edited

cstamas commented Feb 24, 2024 •

edited

cstamas commented Feb 25, 2024 •

edited