fix: deploy-side integrity must fail closed

[gnodet]

Summary

Fixes 4 findings from the maven-resolver security audit (scan-maven-resolver-20260811):

Finding	Severity	Description
f022	LOW	Checksum upload failures swallowed; deploy succeeds without published checksums
f023	LOW	Provided/trusted checksums never cover metadata downloads
f029	LOW	First matching checksum algorithm accepts, skipping stronger provided checksums
f030	LOW	One pre-existing .asc silently disables signing of all deploy artifacts

Root cause: On the publication and strongest-verification paths, failures degrade silently: checksums that fail to upload are logged and forgotten, one pre-existing signature disables signing wholesale, and trusted-checksums has structural coverage gaps.

Fix: Deploy must be atomic across artifact bytes and integrity metadata: propagate failures, cover metadata, compare all checksums, per-artifact signature skip.

Test plan

• Existing tests pass
• Checksum upload failure propagation tested
• Metadata checksum coverage tested
• All-algorithm comparison tested
• Per-artifact signature skip tested

🤖 Generated with Claude Code

[cstamas]

Re "First matching checksum algorithm accepts, skipping stronger provided checksums": this was always the behaviour of Resolver (default SHA1, MD5). The idea is that when user configures aether.checksums.checksumAlgorithms or any related one, user should be aware that this is a list, and algorithms should be configured in descending order by their "strength".

[cstamas]

Also, by checking all checksums, Resolver "lazyness" is being removed, and also may trigger extra HTTP roundtrips (think about reposes that have "optional" checksums, like Central -- not all of it has SHA512).

Basically this change IMHO should then (would be logical) to trigger another change, to mark listed checksum "optional", as otherwise, "mixed reposes" (just like Central is), cannot be uniformly configured, OR, user is forced to use "least common denominator" (that is SHA1) in cases where they may be available stronger algorithms.

[cstamas]

So, checksums list were envisioned as:

• first match makes verification outcome OK
• the list should be descending "by alg strength"
• there are reposes like Central, where some (modern) artifacts have "more" than SHA1, while ancient old artifacts (like aopalliance, used by todays Maven even as Guice transitive dep) only SHA1

[gnodet]

@cstamas — thanks for the detailed review, both points are important. Let me clarify the scope of the change, because the implementation already splits the behaviour along exactly the line you're drawing:

PROVIDED / REMOTE_INCLUDED checksums (validateChecksums())

These are checksums that are already in hand — no HTTP roundtrips involved, just iterating over an in-memory map. Here the change is intentional: if a TrustedChecksumsSource provides both SHA-512 and MD5 for an artifact, and the SHA-512 mismatches but MD5 matches, the old code might accept on the MD5 match first (depending on map iteration order), silently masking the SHA-512 mismatch. The new code checks all comparable checksums and rejects if any one mismatches. This is purely in-memory — zero extra cost.

REMOTE_EXTERNAL checksums (validateExternalChecksums())

This is where your roundtrip concern applies — and the code already preserves the lazy early-return. The only change is adding a && !rejected guard:

// Old:
} else if (checksumPolicy.onChecksumMatch(factory.getName(), REMOTE_EXTERNAL)) {
    return true;  // early accept on first match
}

// New:
} else if (checksumPolicy.onChecksumMatch(factory.getName(), REMOTE_EXTERNAL)
        && !rejected) {
    return true;  // still early accept — unless a prior algorithm already mismatched
}

Happy path is identical: SHA-1 matches → early return, no extra fetches. The guard only kicks in when a prior stronger checksum already mismatched — which is either an attack or corruption. In that case the old code would have continued iterating too (it doesn't return on mismatch), but would then accept on the next weaker match — that's the security gap this fixes.

Regarding the "optional checksums" idea

You're right that a more general solution would be marking individual algorithms as optional vs. required, which would cleanly handle mixed repos like Central. That's a worthwhile enhancement but orthogonal to this fix — it would be a separate feature PR with its own configuration surface (e.g. aether.checksums.optionalAlgorithms). This PR only tightens the existing logic: "if you have a checksum comparison and it mismatches, don't let a weaker match override it."

tl;dr — no extra roundtrips in the normal case, and the documented first-match-wins contract for remote external checksums is preserved. The only behavioral change is: a mismatch on a stronger algorithm can no longer be masked by a match on a weaker one.

[cstamas]

Explanations aligns, I am okay now with PR

[gnodet]

Solid set of fail-closed security fixes for four audit findings (f022, f023, f029, f030). All implementations are correct and well-tested.

@since tag corrections (6 locations)

All six new @since tags reference 2.0.22, but that version is already released. They must be @since 2.0.23:

• BasicRepositoryConnectorConfigurationKeys.java — CONFIG_PROP_CHECKSUM_UPLOAD_FAIL_CLOSED
• FileTrustedChecksumsSourceSupport.java — getTrustedMetadataChecksums + doGetTrustedMetadataChecksums
• ProvidedChecksumsSource.java — getProvidedMetadataChecksums
• TrustedChecksumsSource.java — getTrustedMetadataChecksums + Writer.addTrustedMetadataChecksums

Minor observation (not blocking)

hasSignature and isSignatureArtifact are duplicated identically in GnupgSignatureArtifactGenerator and SigstoreSignatureArtifactGenerator. Consider extracting to a shared utility to reduce maintenance risk.

Positive observations

• f022: Checksum upload fail-closed defaults correctly with per-repository override
• f023: Metadata checksum SPI extension uses backward-compatible default methods
• f029: validateChecksums correctly returns accepted && !rejected (all-must-match)
• f030: Per-artifact signature check replaces all-or-nothing bail-out

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet