Repository navigation
feat(validators): persist validation reports and make queryable - #2752
rh-jfuller wants to merge 4 commits into
Conversation
The validator backends (scheck, CSAF spec, Conforma) were always compiled in eg. pulling scheck dependency tree into every binary. Add a 'semantic-validation' Cargo feature, on by default, gating the three backend modules and making an optional dependency.
Reviewer's GuideThis PR adds feature-gated compilation for the semantic validation subsystem, keeping it enabled by default while allowing dependency-free builds that reject configured semantic backends at runtime; CI now verifies the feature-off configuration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/ingestor/src/service/validation/config.rs" line_range="122-123" />
<code_context>
Ok(validators)
}
+/// Build a single validator from its configuration.
+fn build_one(validator: &ValidatorConfig) -> Result<Arc<dyn Validator>, anyhow::Error> {
+ match &validator.backend {
+ #[cfg(feature = "semantic-validation")]
</code_context>
<issue_to_address>
**Validation reports are not retained**
When an ingestion completes with validation reports, `build_one` only constructs validators; after validation, `IngestorService::ingest` attaches reports to the in-memory `IngestResult` and does not store them. Once the ingestion response is gone, callers cannot retrieve reports for querying.
Persist reports with the ingested document and add a query path for retrieving them.
</issue_to_address>
### Comment 2
<location path="modules/ingestor/Cargo.toml" line_range="10-15" />
<code_context>
rust-version.workspace = true
+[features]
+default = ["semantic-validation"]
+
+# The semantic validator subsystem (ADR 00020): scheck, CSAF spec and Conforma
+# backends. Disabling it drops the `scheck` dependency tree; the validator
+# config still parses, but building a configured validator fails. See ADR 00022.
+semantic-validation = ["dep:scheck"]
+
[dependencies]
</code_context>
<issue_to_address>
**Server cannot disable semantic validation**
When a server or workspace binary is built intending to omit semantic validation, cargo feature unification enables `semantic-validation` through workspace dependents that declare `trustify-module-ingestor` with default features. The server and other composed binaries therefore still compile the validator backends and `scheck` dependency tree, even when built with `--no-default-features`.
Disable default features on the ingestor dependency throughout the workspace and add a forwarded feature where consumers need to enable semantic validation.
</issue_to_address>
### Comment 3
<location path="modules/ingestor/Cargo.toml" line_range="15" />
<code_context>
+# The semantic validator subsystem (ADR 00020): scheck, CSAF spec and Conforma
+# backends. Disabling it drops the `scheck` dependency tree; the validator
+# config still parses, but building a configured validator fails. See ADR 00022.
+semantic-validation = ["dep:scheck"]
+
[dependencies]
</code_context>
<issue_to_address>
**CSAF dependency remains enabled**
When the ingestor is built with `--no-default-features`, with `semantic-validation` disabled, Cargo still compiles the unconditional `csaf-rs` dependency even though `service::validation::csaf` is gated off, so disabling the feature does not remove the CSAF backend’s dependency tree from builds.
Make `csaf-rs` optional and include `dep:csaf-rs` in the `semantic-validation` feature.
</issue_to_address>
### Comment 4
<location path="modules/ingestor/src/service/validation/config.rs" line_range="138-141" />
<code_context>
+ Backend::Conforma(conforma) => Ok(Arc::new(conforma::build(validator, conforma)?)),
+ #[allow(unreachable_patterns)]
+ backend => anyhow::bail!(
+ "validator '{}' uses backend {backend:?}, which is not compiled into this build \
+ (the 'semantic-validation' feature is disabled)",
+ validator.name,
+ ),
+ }
+}
</code_context>
<issue_to_address>
**Feature-off unit test fails**
When the ingestor unit tests are run with `--no-default-features`, `build` reaches this unsupported-backend error for the first validator before checking the duplicate name on the second, so `rejects_duplicate_validator_names` receives the feature-disabled error and fails its assertion when tests run without default features.
Make the duplicate-name test use a backend-independent setup or adjust the feature-off test/configuration so it can reach the duplicate check.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: modules/ingestor/src/service/validation/config.rs:123, modules/ingestor/Cargo.toml:15, modules/ingestor/Cargo.toml:15, modules/ingestor/src/service/validation/config.rs:141
Reuse unchanged verdicts, serialize report deduplication, preserve combined filters, and test the ingestor without semantic-validation.
jcrossley3
left a comment
There was a problem hiding this comment.
I think my main concern is that I can't see exactly how to map the report results to specific elements of the validated document.
I understand the desire to have the report schema generic enough to support all the different types of documents we might ingest, but for the Conforma report specifically, we need a way to map the results to an SBOM's components.
Maybe we punt and just brute-force search for specific algorithm id's within the findings value, but can we conceive of a more elegant solution?
it is a very good use case ... lets ignore the generic aspects and focus on the needful. for components (in sbom) the right way to handle identy would be pURL (because a pURL in a specific sbom is safe) or its checksum (but often that can not be supplied) ... using the sbom own internal ID is doable as well - can you provide an example of conforma report processing an entire sbom ? |
Just added in a commit to #2725 : https://github.com/gildub/trustify/tree/75decd040f12b4894903b15c8d0544de9906b10e/etc/test-data/cyclonedx/cryptographic |
Persist validation report (with configs to control persistence) which comes with new read.validation perm, semantic validators are now feature gated and we decided to implement a complementary REST API.
hitting these endpoints requires
read.validationperm:from document
everything, newest first, with a total
only the documents this instance refused
filter on any column of the report
by document name: an SBOM's describing node, or an advisory identifier
one document, by digest -- the only way to reach a rejected document
one document, by the ID of an ingested SBOM or advisory
Code
the primitives are usable from anywhere that already depends on the
ingestor. The by_* functions return a Select, so callers compose their own
ordering, filtering and pagination:
Configuration
While I was there I added a 'semantic-validation' Cargo feature, on by default, gating the three backend modules and making an optional dependency.
Summary by Sourcery
Persist semantic validation outcomes and make them securely queryable through a new REST API.
New Features:
read.validationpermission for validation report access.Enhancements:
semantic-validationCargo feature and verify feature-disabled builds in CI.CI:
Documentation:
Tests:
Chores: