Repository navigation
Conversation
af4820f to
5722496
Compare
rh-jfuller
left a comment
There was a problem hiding this comment.
@gildub have you considered registering conforma as a semantic validator (like scheck and csaf validator) ... instead of bolting on into the endpoint itself ? That would mean we have clear abstraction and sets up conforma usage elsewhere.
575202f to
dfcdc79
Compare
|
Semantic validators (scheck, csaf-validator) are coupled to the ingest event, they run once when the document arrives and that's it. That's fine for validation only depending on document content itself, which never changes after ingest. But Conforma evaluations depend on two independent variables:
Because the policy can change independently of the SBOM, re-evaluation is a first-class operation, not an edge case. Embedding Conforma into the semantic validator pattern would make re-evaluation awkward, since you'd have to fake a re-ingestion to trigger it. Keeping it as a separate, triggerable evaluation service that can be invoked:
Meanwhile I have been thinking about future policy evaluators and the need to add abstract within the crypto/policy domain. The rationale is that although Conforma (Rego policy) is a mature policy validation solution, a future is coming with CEL (per-artifact checks), Datalog/graph-based (cross-artifact reasoning), CUE (niche but elegant schema-plus-constraint validation), Cedar and others. A PolicyEvaluator will allow Conforma to be swappable. Something like : The policy broker to sit on top of this: And the policies table gains an evaluator_kind field — so each stored policy declares not just what to evaluate (the Rego/policy content) but which engine evaluates it. The broker routes accordingly. This will also allow to connect directly to the coming policy management feature:
The current ConformaClient in the PR is the first concrete implementation of PolicyEvaluator. The question is about adding such abstract now to allow easy future evolution. |
|
@rh-jfuller, I added a commit with the policy evaluator abstract. |
c9f78b0 to
d6eab7a
Compare
Reviewer's GuideThis PR replaces the interim in-process crypto policy implementation with an optional Conforma/Podman evaluator, persists its per-algorithm verdicts in the database, exposes stored classifications and aggregate summaries through the API, and triggers evaluation asynchronously after SBOM ingestion while preserving normal operation when Conforma is not configured. Sequence diagram for asynchronous SBOM Conforma evaluationsequenceDiagram
participant Client
participant Upload as SBOM_upload
participant DB
participant Crypto as CryptoService
participant Conforma as ConformaClient
participant Podman
Client->>Upload: upload()
Upload->>DB: commit()
Upload-->>Client: HTTP 201
Upload->>Crypto: has_evaluator()
Crypto-->>Upload: evaluator configured
Upload->>Crypto: evaluate_policy(sbom_id, transaction)
Crypto->>Conforma: evaluate(algorithms)
Conforma->>Podman: podman run --network=host
Podman-->>Conforma: container_id
Conforma->>Conforma: wait_ready()
Conforma->>Podman: GET /ready
Conforma->>Podman: POST /v1/validate/input
Podman-->>Conforma: violations and warnings
Conforma->>Conforma: parse_finding()
Conforma-->>Crypto: EvaluatorReport
Crypto->>DB: update policy_verdict
Crypto-->>Upload: evaluation result
Upload->>DB: commit()
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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="modules/fundamental/src/crypto/service/conforma.rs" line_range="111-122" />
<code_context>
+ self.wait_ready(&base_url).await?;
+
+ let client = reqwest::Client::new();
+ let response = client
+ .post(format!("{base_url}/v1/validate/input"))
+ .json(&input)
+ .send()
+ .await
+ .context("failed to reach Conforma evaluation endpoint")?;
+
+ response
+ .json()
+ .await
+ .context("failed to parse Conforma response")
+ .map_err(|e| crate::Error::Internal(e.to_string()))
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The Conforma HTTP status is ignored before deserializing the response. An error response such as `{}` deserializes successfully because `RawConformaReport.filepaths` has a default empty value, causing every evaluated algorithm to fall through to `Compliant` and persist a false compliant verdict.
**Triggers:** When Conforma returns a non-success response with a JSON body that does not contain `filepaths`.
**Suggested fix:** Call `error_for_status()` before deserializing the response so HTTP failures are returned instead of treated as an empty successful report.
</issue_to_address>
### Comment 2
<location path="modules/fundamental/src/crypto/model.rs" line_range="30-32" />
<code_context>
#[derive(Serialize, Deserialize, Debug, Clone, ToSchema)]
pub struct CryptoSummary {
pub total_algorithms: i64,
- pub pqc_compliant: i64,
- pub classical_share_pct: f64,
- pub sboms_meeting_pqc: i64,
}
#[derive(Serialize, Deserialize, Debug, Clone, ToSchema)]
</code_context>
<issue_to_address>
**issue (broader_impact):** The existing `GET /v3/crypto/summary` response drops `pqc_compliant`, `classical_share_pct`, and `sboms_meeting_pqc`, returning only `total_algorithms`. Existing API consumers that read those fields receive missing values and the existing KPI contract is broken; the new policy summary endpoint does not preserve that response shape.
**Triggers:** When an existing client continues calling `GET /v3/crypto/summary`.
**Suggested fix:** Preserve the existing `CryptoSummary` fields and populate them from stored verdicts, or explicitly version the endpoint and update every consumer in the same change.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and a faulty Conforma policy, parser, or container integration can persist incorrect cryptographic verdicts and expose them through the list and summary APIs; launching a podman container also introduces a production runtime dependency. Reverting removes the new behavior, and the stored verdicts are bounded and can be repaired by rerunning evaluation.
Blocking findings: modules/fundamental/src/crypto/service/conforma.rs:122, modules/fundamental/src/crypto/model.rs:32
| let response = client | ||
| .post(format!("{base_url}/v1/validate/input")) | ||
| .json(&input) | ||
| .send() | ||
| .await | ||
| .context("failed to reach Conforma evaluation endpoint")?; | ||
|
|
||
| response | ||
| .json() | ||
| .await | ||
| .context("failed to parse Conforma response") | ||
| .map_err(|e| crate::Error::Internal(e.to_string())) |
There was a problem hiding this comment.
issue (bug_risk): The Conforma HTTP status is ignored before deserializing the response. An error response such as {} deserializes successfully because RawConformaReport.filepaths has a default empty value, causing every evaluated algorithm to fall through to Compliant and persist a false compliant verdict.
Triggers: When Conforma returns a non-success response with a JSON body that does not contain filepaths.
Suggested fix: Call error_for_status() before deserializing the response so HTTP failures are returned instead of treated as an empty successful report.
| pub pqc_compliant: i64, | ||
| pub classical_share_pct: f64, | ||
| pub sboms_meeting_pqc: i64, |
There was a problem hiding this comment.
issue (broader_impact): The existing GET /v3/crypto/summary response drops pqc_compliant, classical_share_pct, and sboms_meeting_pqc, returning only total_algorithms. Existing API consumers that read those fields receive missing values and the existing KPI contract is broken; the new policy summary endpoint does not preserve that response shape.
Triggers: When an existing client continues calling GET /v3/crypto/summary.
Suggested fix: Preserve the existing CryptoSummary fields and populate them from stored verdicts, or explicitly version the endpoint and update every consumer in the same change.
I think you are observing existing initial usage of schema validators and assuming its linked to just ingestion ... it is not.
then we should change semantic validators to enable that scenario instead of create a different codepath/abstraction.
I think all schema validators should be able to operate similarly ... lets discuss today how we can do that.
lets talk about this as well. thx! |
|
@gildub as per our recent discussion ... this PR is not going to work (for a few reasons) in terms of invoking a container to get CLI ... direct invoke of the conforma CLI was previously considered and something we want to avoid. So we have to either:
or
we are leaning towards the former (eg. separate pod running cli server). today registering a validation service means it can be used in ingestion validation as well as direct invoke though either of the above approaches predicates some work for the current validation registration extensibility already built in trustify (eg. scheck and csaf validator) ... I will try to do that work first to support where we land in this PR. the only other risk is possibility that any of these approaches break down with scale. |
|
#2733 implements registration for conforma validator |
|
Conforma does have API interface [1] and that's the one we're using in this PR : Therefore policy validation will still occur through Conforma API interface meanwhile through the incoming validator abstract update. That said and as discussed, the local container invocation is going to be removed and will be replaced with a compose script for dev and helm chart for prod. [1] https://github.com/conforma/cli/blob/main/cmd/validate/input.go#L120-L132. |
8d9441d to
2e8782c
Compare
jcrossley3
left a comment
There was a problem hiding this comment.
Looks good, but I agree there is some appeal to fitting this into the existing semantic validation framework as shown in #2733
2e8782c to
b3a60c0
Compare
Attached to the bottom of https://docs.google.com/document/d/1sY2CMFMtK-sF0-T538dImvV9ssAASX2XZvzLBfdoZgw/edit?usp=sharing Sample CBOM and policy file
Implements https://redhat.atlassian.net/browse/TC-5851
When CONFORMA_POLICY is not provided, the application degrades gracefully in stages:
So the application is fully usable without Conforma for SBOM ingestion and browsing; it just has no policy verdicts. The 500 on the evaluate endpoint is a clear signal that the operator needs to configure CONFORMA_POLICY to enable that feature.
Summary by Sourcery
Integrate configurable Conforma policy evaluation into cryptographic asset processing and persist its verdicts for API and UI consumption.
New Features:
Enhancements:
Build:
Deployment:
Documentation:
Tests:
Chores: