Enable GPG checks of channel artifacts #268

spyrkob · 2024-06-18T12:56:21Z

Initial SignatureValidator API
Pass GPG setting to dependant channels
Update validator signatur
Add SignatureValidator implementation
Use return status instead of exceptions
Report validation errors
Add keyserver support to the validator
Create channel version 2.1.0

spyrkob · 2024-08-08T12:26:12Z

@jmesnil could you review this please? It's a large one, if it makes it easy I can split it up into spec/API changes and implementation PRs.

jmesnil · 2024-08-12T11:52:25Z

core/src/main/java/org/wildfly/channel/Channel.java

+ }
+
+ @JsonIgnore
+ public boolean requiresGpgCheck() {


do we need this method as it kind of duplicates isGpgCheck?

I added it to avoid having to check isGpgCheck() for null every time, but I think it just made it more confusing, I'm happy to remove it.

+1, let's store the field as a boolean and deal only once with the nullability of the Boolean param in the constructor.

I ended up keeping the field as a Boolean to allow channels without gpg-check fields to be deserialized & serialized without changes. The public getter deals with the null check

jmesnil · 2024-08-12T11:53:48Z

core/src/main/java/org/wildfly/channel/Channel.java

- @JsonProperty(value = "resolve-if-no-stream") NoStreamStrategy noStreamStrategy) {
+ @JsonProperty(value = "resolve-if-no-stream") NoStreamStrategy noStreamStrategy,
+ @JsonProperty(value = "gpg-check") Boolean gpgCheck,
+ @JsonProperty(value = "gpg-keys") List<String> gpgUrls) {


there is a mismatch between the json property gpg-keys and the java field gpgUrls.
What are the String representing? keys or URLs?

The fields contains URLs where the keys can be downloaded from. Originally I called the field gpg-urls, later renamed it to gpg-keys to follow YUM naming, but I suppose that's not really required.

jmesnil · 2024-08-12T11:59:36Z

doc/spec.adoc

@@ -76,6 +77,8 @@ A channel is composed of several fields:
 ** `maven-latest` - a version marked as `latest` in the Maven metadata
 ** `maven-release` - a version marked as `release` in the Maven metadata
 ** `none` - do not attempt to resolve versions of artifact not listed in the `streams` collection. Default value if no strategy is provided.
+* Optional `gpg-check` boolean indicating if the artifacts resolved from this channel have to have a valid GPG signature.


s/have to have/must be checked with ?

Does this sound better?

"Optional gpg-check boolean indicating that during artifact resolution, this channel will verify a signature of every artifact. If the artifact signature cannot be found, or cannot be validated, the artifact will not be resolved from the channel. The channel repositories must contain a detached GPG signature paired with the artifact as described below."

jmesnil · 2024-08-12T12:01:50Z

doc/spec.adoc

@@ -76,6 +77,8 @@ A channel is composed of several fields:
 ** `maven-latest` - a version marked as `latest` in the Maven metadata
 ** `maven-release` - a version marked as `release` in the Maven metadata
 ** `none` - do not attempt to resolve versions of artifact not listed in the `streams` collection. Default value if no strategy is provided.
+* Optional `gpg-check` boolean indicating if the artifacts resolved from this channel have to have a valid GPG signature.
+* Optional `gpg-key` a list of URLs that the public GPG keys used to validate artifact signatures are resolved from.


s/gpg-key/gpg-keys

Optional `gpg-keys` a list of URLs corresponding to the public GPG keys used to validate artifact signatures.

If gpg-check is true, gpg-keys must be set too, right?

Not necessary. The client using wildfly-channels may have a local keystore that's used to resolve the required public keys, or they can use public keyservers.

The gpg-keys field was meant to be an optional way to provide the channel definition with a way to pass information were the key may be found.

jmesnil · 2024-08-12T12:02:15Z

doc/spec.adoc

 ### Changelog

+### Version 2.1.0
+
+* Adding ability to verify artifact signatures


could you please also add the fields added in this version?

jmesnil · 2024-08-12T12:03:13Z

pom.xml

@@ -31,10 +31,12 @@
 <version.org.jboss.logging>3.6.0.Final</version.org.jboss.logging>
 <version.org.apache.commons.commons-lang3>3.14.0</version.org.apache.commons.commons-lang3>
 <version.org.apache.httpcomponents.httpclient>4.5.14</version.org.apache.httpcomponents.httpclient>
+ <version.org.bouncycastle>1.76</version.org.bouncycastle>


Are the versions of the added dependencies aligned with WildFly Core (or WildFly)?

Looks like I'm a bit behind with the WildFly Core, updated now

jmesnil · 2024-08-12T12:05:35Z

pom.xml

 <version>${version.org.assertj}</version>
 </dependency>
+ <dependency>
+ <groupId>org.pgpainless</groupId>


is that a new dependency in the WildFly ecosystem? (it's the first I hear from it)

Yes it's a new dependency. I used it for writing tests only as it makes GPG key operations much easier. If that's a problem I can re-write the tests to remove it

jmesnil · 2024-08-12T12:06:53Z

core/src/main/java/org/wildfly/channel/ChannelImpl.java

+ * The order of returned URLs is the same as order of coordinates.
+ *
+ * @param coords - list of ChannelMetadataCoordinate.
+ * @param optional - if artifact is optional, the method will return an empty collection if no versions are found


it's not clear to me what "if artifact is optional" means? When would an artifact be optional?

This is about blocklist. According to the spec, if no blocklist can be resolved at specified coordinates, the channel should ignore this blocklist:

If no blocklist artifact can be resolved with supplied Maven coordinates, the channel treats it as an empty blocklist.

The idea behind it was to allow for a channel definition that would later add a blocklist.

Either way, this should probably be its own issue and PR, I'll create one

spyrkob · 2024-09-13T12:59:12Z

@jmesnil this should be ready for re-review, sorry about the delay. Again if it's easier I can break this down into smaller PRs (e.g. API/Spec changes vs implementation)

spyrkob force-pushed the mvn_verify branch from d6f4d79 to 54ccf06 Compare August 2, 2024 11:08

spyrkob marked this pull request as ready for review August 8, 2024 12:25

jmesnil requested changes Aug 12, 2024

View reviewed changes

spyrkob force-pushed the mvn_verify branch from 54ccf06 to 9c16f28 Compare August 13, 2024 10:19

spyrkob force-pushed the mvn_verify branch 3 times, most recently from cbb8103 to 005f85b Compare September 5, 2024 15:38

spyrkob mentioned this pull request Sep 13, 2024

Add ChannelManifest.Builder #244

Merged

spyrkob force-pushed the mvn_verify branch 3 times, most recently from 049320a to 5f1f025 Compare September 13, 2024 12:43

spyrkob mentioned this pull request Sep 13, 2024

Add option to use Signature Validation wildfly-extras/prospero-metadata#45

Open

spyrkob force-pushed the mvn_verify branch from 5f1f025 to 8a28fbe Compare September 13, 2024 13:15

This was referenced Sep 13, 2024

EAP7 2183 signature verification wildfly-extras/prospero#763

Draft

[WFMP-275] Add GPG checks to the channel provisioning wildfly/wildfly-maven-plugin#569

Draft

spyrkob added 10 commits October 15, 2024 16:46

Update channel schema to support signature verification

cb62d0b

Add SignatureValidator API

c248fc7

Add validation of URL manifests

6db8b46

Add implementation of SignatureValidator

a42cd47

Update validator implementation with SPI changes

97fe2b4

Add tests for ChannelImpl and ChannelSession

ff06f33

Update Javadocs

5989012

Add override for manifest signature URL

58e29c4

Move signature validation from ChannelSession to the VersionResolver

fd9339e

WiP add missing key to the exception information

148bb67

spyrkob force-pushed the mvn_verify branch from 8a28fbe to 148bb67 Compare October 15, 2024 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable GPG checks of channel artifacts #268

Enable GPG checks of channel artifacts #268

spyrkob commented Jun 18, 2024

spyrkob commented Aug 8, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024 •

edited

Loading

jmesnil Aug 12, 2024

spyrkob Aug 13, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

jmesnil Aug 12, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

jmesnil Aug 12, 2024

spyrkob Aug 12, 2024

spyrkob commented Sep 13, 2024

Enable GPG checks of channel artifacts #268

Are you sure you want to change the base?

Enable GPG checks of channel artifacts #268

Conversation

spyrkob commented Jun 18, 2024

spyrkob commented Aug 8, 2024

Choose a reason for hiding this comment

spyrkob Aug 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyrkob commented Sep 13, 2024

spyrkob Aug 12, 2024 •

edited

Loading