Repository navigation
feat: support tlp enum conversion - #1334
bshek-xitaso wants to merge 8 commits into
Conversation
da9ce11 to
0dbe88e
Compare
0dbe88e to
ebd150c
Compare
|
Currently reviewing 👀 |
Gronner
left a comment
Gronner
left a comment
There was a problem hiding this comment.
Overall this looks good and working to me, I have some suggestion w.r.t to conversions happens and how the edge cases could be handled
| Ok(csaf2_0::schema::LabelOfTlp::Red) => (csaf2_1::schema::LabelOfTlp::Red, false), | ||
| Err(_) => { | ||
| diagnostics.warning("/document/distribution/tlp/label".into(), Warning::InvalidTlpLabel); | ||
| (csaf2_1::schema::LabelOfTlp::Clear, false) |
There was a problem hiding this comment.
While the default for a missing TLP label is explicitly TLP:CLEAR, I'd actually choose the most restrictive one for this case? As a label is set, but we can not identify it, this would assume that the information within the CSAF document is always public. So maybe in case of an invalid value TLP:RED or TLP:ABMER+STRICT would be a more secure default?
There was a problem hiding this comment.
Might be something to discuss with TC?
There was a problem hiding this comment.
I would keep CLEAR for now, but we can open a discussion with the TC.
| @@ -62,6 +66,14 @@ impl fmt::Display for Warning { | |||
| Warning::InvalidCsafVersion => "CSAF version node contained an invalid value", | |||
| Warning::InvalidTlpLabel => "TLP label contained an invalid value", | |||
| Warning::TlpIsMissingLabel => "TLP node was present but did not contain the `label` property", | |||
There was a problem hiding this comment.
Maybe add a hint, that if the TLP is contained in the distribution's text field, an option to detect this is available?
There was a problem hiding this comment.
🟡 Changes recommended
The required public Settings field introduces a breaking downstream source-compatibility issue.
1 open issue
What changed in this PR
Adds CSAF 2.0-to-2.1 TLP label conversion, optional distribution.text extraction, diagnostics, and conformance coverage.
Changes:
- Maps TLP v1.0 labels to v2.0 equivalents.
- Adds configurable text-based TLP overrides and CLI support.
- Adds warnings and default handling for missing labels.
- Enables related conformance tests.
A critical compatibility issue remains: the required public Settings field breaks downstream Settings {} usage.
| File | Description |
|---|---|
csaf-rs/tests/converter-conformance.rs |
Enables missing-TLP conformance coverage. |
csaf-rs/src/converter/update_tlp_label.rs |
Implements TLP conversion and text-based overrides. |
csaf-rs/src/converter/mod.rs |
Adds settings and warning variants. |
csaf-converter/src/main.rs |
Exposes the CLI option. |
Review effort: Lite
This PR resolves #1067.