feat(java): support user-defined Maven mirrors in trivy.yaml - #11006
feat(java): support user-defined Maven mirrors in trivy.yaml#11006DmitriyLewen wants to merge 16 commits into
Conversation
|
|
||
| var mavenMirrors map[string][]string | ||
| if f.MavenMirrors != nil { | ||
| mavenMirrors = f.MavenMirrors.Value() |
There was a problem hiding this comment.
OCI mirror configuration can use a map safely because its keys are registry hostnames, which are case-insensitive. Here the keys are full URLs, whose paths may be case-sensitive. Viper lowercases YAML map keys, so /CaseSensitive/ will no longer match mirrorKey(repo.url). Could we consider a different schema that avoids full URLs as map keys, for example a list such as mirrors: [{ source: "...", targets: ["..."] }], and test it through actual YAML parsing?
There was a problem hiding this comment.
Good catch, thanks — I confirmed that viper lowercases map keys, so a case-sensitive path in a source URL would indeed break the match.
Before moving to a list schema, did you consider simply comparing in lowercase — i.e. normalizing both the config key and mirrorKey(repo.url) with strings.ToLower (a one-line change)?
The impact on users should be small:
- Scheme and host are case-insensitive anyway, so lowercasing them is harmless.
- The key is the repository base URL (
.../maven2/,.../repository/maven-public/), which is lowercase by convention — not the case-sensitive artifact path, which is appended only after matching. - A repository whose base URL differs only by path case is very unlikely in practice.
- For that rare case, the user can configure the mirror in
settings.xmlinstead — the same fallback we already recommend for credentials.
Advantages of keeping the map + lowercasing:
- Significantly simpler: no need to add a non-primitive type to
FlagType, a custom cast/decode for it, schema-generator support, and a list-to-map conversion. - Stays consistent with
RegistryMirrorsFlag(same map schema), which is less confusing for users who configure both.
Either way, I'll add a test that goes through actual YAML/viper parsing to lock the behavior down, as you suggested.
Would the lowercase approach work for you, or do you prefer the explicit list?
There was a problem hiding this comment.
I agree that the artifact path is appended after matching, but the repository base URL itself can contain a case-sensitive path. RFC 3986 defines the scheme and host as case-insensitive and otherwise assumes generic URI components are case-sensitive. Lowercasing the entire URL therefore changes URL identity. The practical impact may be small, but accepting behavior that differs from the URL semantics should be a deliberate decision.
Viper provides WithDecoderRegistry, so a wrapper decoder could preserve scan.maven.mirrors as a typed map[string][]string. However, Viper still calls insensitiviseMap after the decoder. The workaround only works because the current insensitiviseVal implementation does not recurse into map[string][]string. This depends on an internal implementation detail, while Viper explicitly documents that case-sensitive keys are unsupported, so I don't think it is a good long-term solution.
An explicit []struct schema avoids that dependency, but as you noted, it requires changes to FlagType, casting, and schema generation. Given these trade-offs, if you still prefer lowercasing for implementation simplicity, I'll leave the final decision to you. In that case, please add the real YAML/Viper test and document that source repository URLs are matched case-insensitively. More broadly, deviations from standardized URL semantics can become security-relevant when different components interpret the same URL differently, so I think this trade-off deserves careful consideration.
There was a problem hiding this comment.
I thought about it again and found one more advantage in your idea — extensibility.
If we do end up wanting to add some fields (an id from settings.xml, or user/password/token), we'll be able to do it without breaking changes to the flag's signature.
Updated in 77e2a0f
| repoPaths[len(repoPaths)-1] = pomFileName | ||
| fetched, err := p.fetchPOMFromRemoteRepository(ctx, candidate.url, repoPaths) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("fetch repository error: %w", err) |
There was a problem hiding this comment.
Since multiple mirrors can be configured in order, they should work as fallbacks. A 429 currently returns immediately here, and similarly for snapshot metadata, without trying the remaining mirrors. Could we continue with the next mirror and skip the rate-limited one for subsequent POM requests, honoring Retry-After if appropriate? The retained 429 can still be returned if all mirrors fail.
There was a problem hiding this comment.
One additional consideration: could the rate-limited mirror state be shared across the entire filesystem/repository scan, including multi-module projects, rather than being local to a single POM parser? Otherwise, each new pom.xml may retry the same mirror.
For Retry-After, it may be preferable not to block the scan by sleeping. Trivy could mark the mirror unavailable until that time and immediately continue with the next candidate, bounded by the scan context/deadline
There was a problem hiding this comment.
@knqyf263 -make sense.
Update in 1b5aec6
@mjshastha Thanks for the suggestion. We should consider it, but in a separate PR — since it would require significant code changes, and we need to think through the architecture so that it's applicable to all parsers, etc. This PR only adds support for mirrors from the config file.
| func (a pomAnalyzer) Analyze(ctx context.Context, input analyzer.AnalysisInput) (*analyzer.AnalysisResult, error) { | ||
| filePath := filepath.Join(input.Dir, input.FilePath) | ||
| p := pom.NewParser(filePath, pom.WithOffline(input.Options.Offline)) | ||
| p := pom.NewParser(filePath, |
There was a problem hiding this comment.
Since a new Parser is created for each pom.xml, any rate-limited/unhealthy mirror state stored on Parser will reset for every POM. In a multi-module repository, Trivy could therefore continue hitting the same 429ing mirror once per POM.
Could the mirror health or Retry-After state be shared at the repository-scan scope rather than only the individual parser scope? It would also be useful to add a test with multiple POM files to verify that a rate-limited mirror is not retried repeatedly during the same scan.
| continue | ||
| } | ||
| return repository{ | ||
| repo = repository{ |
There was a problem hiding this comment.
When this mapping matches, only the configured targets are returned and the original repository is never tried. Is that intentional when every configured mirror returns 404 or is unavailable?
For the Maven Central 429 use case, disabling source fallback may be desirable, but could we document this explicitly and add an all-mirrors-fail test? If both behaviors are needed, an explicit fallback-to-source option would avoid ambiguity.
There was a problem hiding this comment.
This is expected behavior. When mirrors are used, the original URL shouldn't be used.
| Distro: distro, | ||
| SkipVersionCheck: f.SkipVersionCheck.Value(), | ||
| DisableTelemetry: f.DisableTelemetry.Value(), | ||
| MavenMirrors: mavenMirrors, |
There was a problem hiding this comment.
A source configured with an empty target list currently passes validation. resolveMirrors then drops the entry, so Trivy silently continues using the original repository.
Since the acceptance criteria require one or more mirrors and an empty Maven Central mapping could unintentionally re-enable direct Central requests, could we reject empty target lists at startup and add both a validation test and minItems: 1 to the config schema?
There was a problem hiding this comment.
I think a check for emptiness is enough at this stage.
Added it in b43e591
Viper lowercases the map keys it reads from a config file, so a case-sensitive path in a repository URL used as a key was broken. `scan.maven.mirrors` is now a list of `source`/`targets` entries, which keeps a repository URL a value and preserves its case. The mirrors are still indexed by repository downstream, and a repository configured twice is now rejected.
Trivy embeds the credentials of a matching <server> into the repository URL, and `mirrorKey` derived the lookup key from the whole URL, so a mirrored repository with credentials never matched its configured entry — silently. The key is now built from the parts that identify a repository: the credentials are dropped and the host is lower-cased, as RFC 3986 defines it as case-insensitive, while the case-sensitive path is kept. `scan.maven.mirrors` rejects entries that differ only by those, since they collapse into a single key.
Go 1.26 added `errors.AsType`, which checks the error type in place, so the `isRateLimit` helper that only hid the `errors.As` boilerplate is no longer needed.
knqyf263
left a comment
There was a problem hiding this comment.
nit: The PR description and live-verification YAML still show the old URL-keyed map. Could you update them to the current list schema?
| Source string `mapstructure:"source"` | ||
|
|
||
| // Targets are the URLs of the mirrors serving Source, tried in order. | ||
| Targets []string `mapstructure:"targets"` |
There was a problem hiding this comment.
nit: One naming option is mirrors rather than targets. targets is understandable, but generic. OpenShift uses a similar one-to-many schema:
imageDigestMirrors:
- mirrors:
- example.io/example/ubi-minimal
- example.com/example2/ubi-minimal
source: registry.access.redhat.com/ubi9/ubi-minimal
mirrorSourcePolicy: AllowContactingSourceConfiguring image registry repository mirroring
That said, this would become scan.maven.mirrors[].mirrors, so keeping targets to avoid repeating mirrors also seems reasonable. I'll leave the naming choice to you.
| func mirrorKey(u url.URL) string { | ||
| u.User = nil | ||
| u.Host = strings.ToLower(u.Host) | ||
| return strings.TrimRight(u.String(), "/") |
There was a problem hiding this comment.
nit: TrimRight only removes trailing slashes; it does not normalize ./, ../, or repeated slashes, and it can also remove a slash from the query or fragment. Would normalizing u.Path with path.Clean, or using u.JoinPath("."), be appropriate here? Trivy already normalizes the repository base with path.Join when constructing artifact URLs, so applying the same path semantics to mirror keys seems reasonable. This is not general URL canonicalization, since servers may distinguish /repo from /repo/, as well as repeated slashes. RawPath and percent-encoding may also need a quick check, but this may be more than this PR needs, so I'll leave the final decision to you.
| - **Populate `~/.m2` before scanning.** Run `mvn dependency:resolve` (or any build step that resolves dependencies) so that every POM is cached locally. In CI, cache the `~/.m2` directory between runs (e.g. keyed on `pom.xml` checksums) so subsequent runs reuse the artifacts. | ||
| - **Configure mirrors** of the rate-limited repository, so that POM lookups go to a host that isn't blocking you. There are two ways to do it: | ||
| - `<mirrors>` in Maven's [settings.xml][maven-mirror-settings] — the standard mechanism, honored by `mvn` itself as well. A repository is served by a single mirror, so a mirror that is rate-limited too leaves nothing to fall back on. | ||
| - [scan.maven.mirrors][maven-mirrors] in `trivy.yaml` — Trivy-specific, and takes an ordered list of mirrors per repository. A mirror that returns `429` is skipped in favor of the next one, and the scan fails only once every mirror of an artifact is rate-limited. |
There was a problem hiding this comment.
nit: The retained 429 is also returned when one mirror responds with 429 and the remaining mirrors return 404, not only when every mirror is rate-limited. The behavior seems correct because the rate-limited mirror leaves the result unknown, but could the code comment and this sentence be updated to match it?
Description
This PR adds a
scan.maven.mirrorsconfig-file option — a map from a source repository URL to an ordered list of mirror URLs — so users can configure Maven repository mirrors intrivy.yamlwithout modifying the sharedsettings.xml(for example, in CI).Behavior:
settings.xmlresolution.settings.xmlrepo1 -> repo2followed bytrivy.yamlrepo2 -> repo3resolvesrepo1torepo3. On a conflictsettings.xmlwins.RegistryMirrorsFlag.Credentials for config-file mirrors are supported only inline in the URL (
https://user:password@host/...);settings.xml<server>entries are not used. For a secure setup, configure the mirror insettings.xmlinstead. This is documented indocs/guide/coverage/language/java.md.Live verification
A local proxy that logs each request and forwards it to Maven Central, with
trivy.yamlpointing Maven Central at the proxy and an empty local repository to force remote resolution:Every POM in the transitive chain is fetched through the mirror (
localhost:8099), nothing hits Maven Central directly:The artifact resolves and
CVE-2025-48924is reported fororg.apache.commons:commons-lang3:3.14.0.Related issues
Checklist