elharo commented on issue #91:
URL:
https://github.com/apache/maven-artifact-plugin/issues/91#issuecomment-5847080275
## Implementation plan for `artifact:verify-checksum`
Plan for implementing the goal described in the previous comment. This
supersedes the `-D"sha512[classifier:extension]=..."` interface proposed there:
the two comments on the issue were right that the properties form does not
scale, and the reasons turn out to be structural rather than stylistic, so the
plan is built around a checksum file as the primary input. Details and the
measurements behind each decision are below.
### What the feedback changed
| Draft | Now | Why |
| --- | --- | --- |
| primary input: `-D"sha512[classifier:extension]=..."` | primary input: a
checksum file | measured: the property name cannot be declared in a POM, cannot
be enumerated, and a mojo can only bind statically declared names, so "verify
these 7 files" is not expressible |
| `<checksums>` list in POM `<configuration>` | removed | the list would
have to be keyed by the same unsupportable names |
| attached artifacts identified by `classifier:extension` | identified by
published file name | that is the name that appears in every checksum file the
user can actually obtain, so no translation is needed |
| output shown as `sha512 matches <file>` | same, plus absolute path and the
offset of the first differing digit on mismatch | "precise diff ... alongside
the absolute file path" |
| reactor behaviour undecided | non-aggregator, per project, artifacts of
the current project | one invocation covers the whole reactor, and per-module
manifests work |
### Measurements that settle the interface
Run against Maven 3.9.9 with `-Dsha512[source-release:zip]=abc123`:
| Probe | Result |
| --- | --- |
| `<sha512[source-release:zip]>abc</sha512[source-release:zip]>` in POM
`<properties>` | `Non-parseable POM ... start tag unexpected character [` |
| same name as a plugin `<configuration>` element | parses, but is
unreachable: a parameter's property is only ever resolved from POM
`<properties>`, `-D`, system properties and `${session...}`, never from plugin
configuration, and a parameter name containing `[` cannot be declared |
| `${sha512[source-release:zip]}` expression lookup | resolves to `abc123` —
so expression resolution does un-escape |
| `project.getProperties()` | `<properties/>` — CLI `-D` values are *user*
properties and never reach `project.getProperties()` |
| `session.getUserProperties()` key | `sha512_.005bsource-release:zip_.005d`
— Maven escapes `[` to `_.005b` and `]` to `_.005d` |
So the name only survives if a mojo declares it as a `@Parameter` in
advance. That caps the properties interface at a fixed set of artifact
coordinates chosen when the plugin is compiled, gives users no way to add their
own, and prevents the goal from ever reporting what it did and did not cover.
Combined with the bracket-glob quoting hazard already noted, that is enough to
drop it rather than keep it as a secondary path. The only piece worth keeping
is the single simplest case, and a checksum file expresses that too.
### Interface
```
$ mvn artifact:verify-checksum
-Dverify.checksumFiles=target/maven-artifact-plugin-3.7.0-source-release.zip.sha512
```
or, when the checksum sidecars have been downloaded next to the artifacts
into one directory:
```
$ mvn artifact:verify-checksum -Dverify.checksumDirectory=/home/rm/downloads
```
No input at all is not an error: the goal reports that there is nothing to
verify and returns, so it can be left bound to a phase before the values are
known.
Not bound to a lifecycle phase by default, like `artifact:compare` and
`artifact:describe-build-output`; bindable to `verify` or `deploy` through
`<executions>` when a repeatable check is wanted.
### Checksum file formats accepted
All three are already in circulation, so all three are parsed. Blank lines
and `#` comments are ignored, files are read as UTF-8, and a trailing newline
is never significant.
1. **Bare sidecar, the ASF and Central form.** The file contains only the
digest. The artifact is taken from the file name, the algorithm from the
suffix. This is what `maven-artifact-plugin-3.7.0-source-release.zip.sha512` in
the dist area contains, and what `maven-artifact-plugin-3.7.0.jar.md5` on
Central contains:
```
aa97e7a9b11043e49c3a2f85e2e9e97fcc9732da0fdc2f9c6293be6e3c93c32d316872d6bed11e5fe3877cfcd019535adae7469c6bd29e8515c63d2663a23f1
```
2. **GNU coreutils output**, the format of `sha512sum` / `sha1sum` /
`md5sum` with either the text or binary marker, so a user can generate a
manifest with the tools they already have:
```
aa97e7a9...a23f1 maven-artifact-plugin-3.7.0-source-release.zip
f8c9a275...d9f39 *maven-artifact-plugin-3.7.0.jar
```
3. **POM-style `checksum:` prefix**, for anyone who wants to keep the list
in the POM, `<type>` mapping to the extension:
```
checksum:sha512 maven-artifact-plugin-3.7.0-source-release.zip
```
Algorithm resolution, in order: explicit `checksum:<algorithm>` prefix, else
the checksum file name suffix, else the digest length. The four lengths are
unambiguous — 32 `md5`, 40 `sha1`, 64 `sha256`, 128 `sha512` — so a `sha512sum`
manifest needs no extra information. When a suffix and a length are both
present and disagree, that is a hard error rather than a silent choice.
This format choice is also what makes the goal interoperate rather than
overlap: GNU coreutils format is what Resolver's own trusted-checksum sources
consume (see the scope note below), so a user can produce one file and use it
for both purposes.
### Deciding which file to verify
For every entry, the artifact file name from the manifest is looked up in a
map built in this order:
1. the artifacts of the **current project**: the main artifact, the project
file itself as `<artifactId>-<version>.pom`, the build/consumer pom where Maven
4 produces one, then `project.getAttachedArtifacts()`, keyed with the existing
`BuildInfoWriter.getArtifactFilename` helper so the names are identical to the
names `artifact:describe-build-output` prints;
2. the plain files inside `verify.checksumDirectory`, matched by file name.
Step 2 is what lets a release manager verify the copies downloaded from the
dist area, not only the local build output, without the goal needing to know
anything about remote repositories.
### Outcomes
| Situation | Log | Counted | Fails the build |
| --- | --- | --- | --- |
| digest matches | INFO `sha512 matches <file>` | `verified` | no |
| digest differs | ERROR with algorithm, file name, absolute path, expected,
actual, and the zero-based offset of the first differing character |
`mismatched` | yes, unless `verify.failOnMismatch=false` |
| manifest entry matches no artifact | WARN naming the entry and the
expected value | `unresolved` | yes, unless `verify.failOnUnresolved=false` |
| project artifact that no entry covers | WARN listing the uncovered file
names | `uncovered` | only if `verify.failOnUncovered=true` |
| malformed manifest line, or unusable input | ERROR | — | yes |
| no manifest given | INFO `no checksums to verify, skipping` | — | no |
Reporting the offset of the first differing character is a small addition
aimed straight at the original complaint: a single transposed digit out of 128
is otherwise effectively invisible, and the offset turns "these two strings
differ somewhere" into a pointer.
One summary line at the end, in the style `artifact:compare` already uses
for its `N files match, M differ` line:
```
[INFO] Checksum verification result: 7 verified, 0 mismatched, 0 unresolved,
3 uncovered
```
The three outcome counts are kept separate rather than collapsed, because
"unresolved" means the user made a mistake in the manifest while "uncovered"
means the manifest is simply incomplete, and the two deserve different
reactions.
### Parameters
| Parameter | Type | Default | Notes |
| --- | --- | --- | --- |
| `verify.checksumFiles` | `List<String>` | — | manifest files; each may
also name a directory, in which case it is treated as `checksumDirectory` |
| `verify.checksumDirectory` | `File` | — | searched for sidecars and for
the files to verify |
| `verify.failOnMismatch` | `boolean` | `true` | mirrors `compare.fail` |
| `verify.failOnUnresolved` | `boolean` | `true` | fail-fast on manifest
entries that match nothing |
| `verify.failOnUncovered` | `boolean` | `false` | opt in to requiring
complete coverage |
Property names follow the existing convention of the plugin's first goal
word (`buildinfo.*`, `check.*` from `check-buildplan`, `compare.*`).
`checksums.*` was the other candidate; `verify.*` was chosen for consistency,
and the names are specific enough not to collide with the generic `verify.*` of
the enforcer or failsafe plugins.
No `requiresDependencyResolution`: the goal only reads the current project's
own artifacts, so resolving the dependency graph would be wasted work and would
make the goal fail on an unresolvable graph for no reason. `threadSafe = true`,
`requiresProject = true`, not an aggregator, `@since 3.7.1`.
No `skip` parameter: `compare` and `check-buildplan` have none, and "no
manifest given" already covers the need to neutralise the goal.
### Scope relative to Maven Resolver trusted checksums
Worth recording explicitly, since the second comment raised it. The two are
complementary, not alternatives:
- `aether.artifactResolver.postProcessor.trustedChecksums` is an
`ArtifactResolverPostProcessor`. It only sees artifacts that go **through the
resolver**, that is things downloaded or read from a repository. The artifacts
the current build just produced are never resolved, so trusted checksums
structurally cannot check that a build output matches its published digest.
- This goal reads the **current build output** and compares it against a
digest published elsewhere. That is the direction of trust the issue is about,
and the plugin already draws this line: `artifact:compare` fetches the
*previous* release to compare against the *current* build.
- Resolver's own documentation is also careful that checksums "only provide
integrity verification. They do not provide security or trust", and points at
signatures for that. The goal is documented the same way; it answers "is this
the file that was published", not "is this file trustworthy".
Overlap is limited to the file format, and that is deliberate, as noted
above.
### Files added
| File | Purpose |
| --- | --- |
| `src/main/java/.../buildinfo/VerifyChecksumMojo.java` | the goal |
| `src/main/java/.../buildinfo/ChecksumManifest.java` | parses one checksum
file into `(fileName, algorithm, expected)` entries; format detection,
comments, the suffix/length conflict check |
| `src/test/java/.../buildinfo/ChecksumManifestTest.java` | the parser,
which is where all the fiddly logic lives |
| `src/test/java/.../buildinfo/VerifyChecksumMojoTest.java` | outcome matrix
against temp files and a hand-built `MavenProject` |
| `src/it/verify-checksum-manifest/` | end-to-end IT |
| `src/it/verify-checksum-uncovered/` | IT for the coverage warning and
`failOnUncovered` |
The parser is separated from the mojo so that the format handling is
unit-testable without a `MavenProject`, and so the format support can grow
without touching goal logic.
### Files changed
| File | Change |
| --- | --- |
| `src/site/site.xml` | one `<item name="artifact:verify-checksum"
href="verify-checksum-mojo.html"/>`, alphabetically last under Plugin
Documentation |
| `src/site/markdown/index.md` | new bullet; also fix the existing "has 4
goals currently", which lists five |
| `src/site/markdown/usage.md.vm` | a "Verifying published checksums"
section with a console transcript, matching the style of the existing
`artifact:compare` example |
`pom.xml` needs no change: `commons-codec` 1.22.1 and `commons-io` 2.22.0
are already compile-scoped, and `commons-codec` is what supplies the digests,
as it already does for `buildinfo` and `describe-build-output`.
### Tests
Unit, on `ChecksumManifest`: bare sidecar with each of the four suffixes;
`sha512sum` text mode and `*` binary mode; blank lines and comments; CRLF
input; digest and algorithm mismatch between suffix and length; a bare digest
with no file name inside a multi-line manifest; empty file; non-hex digest;
digest of an unsupported length; file that does not exist; directory passed
where a file was expected.
Unit, on `VerifyChecksumMojo`: one match; one mismatch; main artifact, POM,
build POM, an attached artifact, and a file found in `checksumDirectory`; an
entry that matches nothing; an artifact nothing covers; each of the three
`failOn*` toggles; no input at all; and that a wrong digest reports the correct
first-differing offset.
IT, `verify-checksum-manifest`: a project that attaches a
`source-release:zip` via `build-helper:attach-artifact`, and whose main jar and
POM are covered too. Step 1 runs the goal against a correct manifest and
expects success. Step 2 runs it against a manifest with one digit altered and
expects failure, with `invoker.buildResult.2=failure`. `verify.groovy` asserts
on the `build.log` lines: the summary line, the per-file match lines, and for
the failing run the absolute path and the expected/actual pair. Generating the
manifest from the build itself, via `checksum-plugin3` as the `apache-release`
profile does, keeps the "correct" step honest without hardcoding a digest in
the IT. The attached zip is created in-project rather than by depending on a
released version of this plugin, so the IT stays self-contained and does not
need a specific past release in the IT repository.
IT, `verify-checksum-uncovered`: a manifest covering only the main jar while
sources and javadoc are also produced, asserting the uncovered warning, success
under the default, and failure under `-Dverify.failOnUncovered=true`.
### Out of scope for this change
- **Downloading the published sidecar from a remote repository.** Tempting,
and `artifact:compare` already has the resolution machinery for it, but it
needs an artifact resolver and turns a pure local check into a networked one.
`verify.checksumDirectory` covers the same need for a release manager, who has
the file already. A natural follow-up.
- **Signature verification** (`.asc`). Out of scope by design, for the
reason quoted from Resolver's own documentation above: integrity is not
authenticity, and `maven-gpg-plugin` is the right tool for the latter.
- **A report file.** `buildinfo` and `compare` write `.buildinfo` and
`.buildcompare` because those are formats others consume. There is no consumer
for a checksum verification report, so the build log is enough; revisit if one
appears.
- **Recording checksums.** Resolver's `trustedChecksums.record` already does
this well for the incoming direction, and `checksum-plugin3` already does it
for the ASF release artifacts in the `apache-release` profile.
### Remaining questions
1. Should the default when no input is given be "scan for sidecars next to
the artifacts in `target/`"? Measured: there are none to find in a normal Maven
3 build, because Resolver does not download Central's `.sha1`/`.md5` sidecars
into the local repository. It would be a no-op default, so the plan is to
report and return instead. Worth confirming that a no-op is preferable to
looking like it did something.
2. Multi-module: the plan is a plain non-aggregator goal, so `mvn
artifact:verify-checksum -Dverify.checksumFiles=...` from the root verifies
every module, and each module picks up its own `<checksums>` configuration if
given. The alternative is an aggregator goal reading one manifest for the whole
reactor, which centralises the file but moves per-module configuration out of
the module POMs. The non-aggregator form follows `artifact:check-buildplan`;
the aggregator form follows `artifact:describe-build-output`. Preference is for
the non-aggregator form, since the release artifacts of a multi-module project
are mostly per-module and belong next to the module that produced them.
3. Anything here that should go to `src/site/markdown/faq.md` rather than
`usage.md.vm`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
