Add support for 1.4 to cyclonedx-bom

[tokcum]

Testing if this PR meets the expectations.

[hone]

👋 i'm interested in using SBOMs with some 1.4 features and was curious how things were coming along with 1.4 support/this PR? Are there things others can do to help or just waiting on more reviews?

[ctron]

I would love to understand what the holdup is on this one? If help is needed, I am willing to.

[tokcum]

Hi, this PR has three aspects. First one is obviously to support 1.4. The second one is related to the ability to support more than one version. This involves some refactoring. I tried to keep this to a minimum. The third one is to add tests with real world SBOMs, e.g. from trivy.

I already spent many hours on this and I'm willing to put more effort in. However, so far, I got absolutely no feedback from the maintainer. I didn't expect to get much advice. A confirmation that my approach is okay would suffice, something like "Looks good, go ahead!" or "Sorry, you have to find a different way for ... .".

[stevespringett]

Hi @tokcum appreciate all the effort that went into this. Would it be possible to resolve the conflicts? I'd like to test locally prior to merging.

[tokcum]

@stevespringett, thank you for your support. Sure, I'll bring this into shape and update the PR. I've some free time at the end of Aug and early in Sept.

[tokcum]

Hi @stevespringett , it would be great if you find time for a quick review of this draft. I resolved all the conflicts and improved the integration testing differentiating between verification and validation. The approach is described in cyclonedx-bom/tests/README.md.

I will continue to work on the parts of the 1.4 spec not yet supported, see cyclonedx-bom/tests/spec/1.4/not_yet_supported.

Let me know what you think and how to proceed. Thank you.

[stevespringett]

FYI @lfrancke

[lfrancke]

Thank you!
I hope that @Shnatsel can help review this once you've marked it as ready for review

[tokcum]

Great! I marked the PR as ready for review. Let me know what to improve.

[Shnatsel]

I'll take a look and try to get this reviewed on Monday.

[Shnatsel]

There are some nits like outstanding TODOs and commented out code. I pointed them out where I noticed them. I don't see any major issues.

I appreciate the thorough testing against the spec, and the doc comments!

This is quite a lot of code! In the interest of being able to reproduce this for future versions, how was this file generated? This is a lot of code. Did you use templating of some sort, or is it entirely handwritten?

The big question here is whether we want to keep 1.3 support around, or remove it and go all in on 1.4 (and soon 1.5). I think supporting older versions would create too much of a maintenance burden, and I am not aware of compelling use cases them, so I prefer to gut 1.3 support. Is there a good reason to keep it?

[Shnatsel]

Why is this a TODO? Are there any blockers to actually doing this?

[tokcum]

Yes, there was a detail I was not sure how to handle it appropriately. Spec 1.4 introduced a vulnerability rating with a decimal score. In rust this is a f32 which does not allow to derive Eq.

Today I've worked on a solution. I'm not sure though, that my approach is okay. It would be great if you could check it and share your thoughts. Thanks.

[Shnatsel]

I like the approach you have taken. Thanks!

[Shnatsel]

Another unfulfilled TODO

[tokcum]

I've looked into this. I had to switch from into() to try_into() because of the different handling of version in Component in 1.3 vs. 1.4. In 1.3 it is mandatory, in 1.4 is is optional. So, a conversion into 1.3 might fail when the Component has no version.
The thing is that try_into returns either Ok(Bom) or Error(BomError), while output_as_json_v1_3() returns a Ok(()) or a Error(JsonWriteError) but not a Error(BomError).

So, how can we handle this. I see three options:

1. We do not change the interface of output_as_json_v1_3() and if a BomError occurs we write the error as a JSON document.
2. We do not change the interface of output_as_json_v1_3() and if a BomError occurs we translate the BomError somehow into a JsonWriteError.
3. We change the interface of output_as_json_v1_3(), which would introduce a breaking change.

We face the same challenge for the output_as_xml_v1_3(). The output_as_*_v1_4() are not affected because there we are able to use into(). However, we could also change it to try_into() to align it.

Are there other options I'm not aware of and which approach should we follow? Please advice. Thanks.

[Shnatsel]

We are making a field optional to match the spec, which is going to introduce breaking changes no matter if it is pub or not. I think it's a good idea to change the signature to reflect reality while we're making breaking changes anyway.

I've looked at the types and BomError represents the possible failure modes better than a hypothetical modification of JsonWriteError, so let's go with option 3.

[tokcum]

Ok, great. I'll follow this approach. Thanks.

[Shnatsel]

TODO

[Shnatsel]

Another TODO. The validation seems to be implemented for vulnerabilities now, any blockers to actually wiring it up?

[Shnatsel]

This is not necessarily an issue with the code, could be my ignorance of the codebase and/or the format, but: is there any reason to keep the 1.3 methods around now that we have 1.4 ones? Isn't 1.4 a strict superset with no new mandatory fields, so that 1.3 data could be parsed by methods expecting 1.4?

[Shnatsel]

Is there anything to be done here? I don't think it should impact this struct.
Maybe validate that this field is present in the 1.3 parser but not 1.4?

[Shnatsel]

Commented out code, here and further down in this file. If it is not used, it should be removed - storing older code is what git is for.

[Shnatsel]

Out of scope of this PR, but a note to self:

We should also exclude the test folder from publishing to crates.io now that it is almost 11MB worth of snapshots, and start tagging releases in the repo (so that Linux distributions could still package it with tests). But our restricted access to the repo may make it harder.

[lfrancke]

@Shnatsel See the discussion in the issue #275

I did not fully follow them tbh.
Would the current code (in this PR) allow someone to choose a version of CycloneDX?
As in: I could create an old version if I wanted to?

[Shnatsel]

Yes.

In fact, this PR does not convert cargo cyclonedx to generate 1.4, it only changes the cyclonedx-bom crate, and cargo cyclonedx keeps generating v1.3.

[lfrancke]

Thanks. I'll do a survey on the CycloneDX slack what other libraries do.

There's arguments for both sides
Some downstream tools might expect an older version and can't deal with newer versions.

Oh the other hand the versions did avoid breaking changes so far I believe.

[lfrancke]

There is a good discussion about this in Slack now: https://cyclonedx.slack.com/archives/CV062H2GH/p1696319537238529

Summary:

• cdxgen supports 1.4 and 1.5
• PHP, JS, Python support multiple CDX versions
• Java has a dedicated major version per CDX schema version
• There are some differences between XML and JSON
• Adoption of 1.5 is not widespread yet so if we support 1.5 we should keep on supporting 1.4 as well
• Suggestion: Model for latest spec and then "downgrade" previous to serialization (Go and .NET do it like that - https://github.com/CycloneDX/cyclonedx-go/blob/master/convert.go )

What does it mean for us?
It'd be great if we could support older versions of CycloneDX but I - personally - would be fine with 1.4 as the minimum if it makes this PR easier. If we already have all the logic in place to keep 1.3 as well and you both think it's fine then I'm happy to keep it in as well.

I'd rather have a version of this library with 1.4 support this month than a version that supports multiple CycloneDX specs in three months.

[Shnatsel]

The summary of the technical considerations is:

• Every 1.3 document is a valid 1.5 document. This is an explicit design foal of the CycloneDX format.
• Not every 1.5 document is a valid 1.3 document. For example, some fields were made optional going from 1.3 to 1.4.

So there are use cases for writing in older format versions, to be compatible with older software. But not so much for reading from older format versions, since any 1.3 document can be loaded by a 1.5 parser.

[jkowalleck]

updated text snippet:

Please sign off your commits, to show that you agree to publish your changes under the current terms and licenses of the project
, and to indicate agreement with [Developer Certificate of Origin (DCO)](https://developercertificate.org/).

[jkowalleck]

Every 1.3 document is a valid 1.5 document. This is an explicit design foal of the CycloneDX format.

there are exceptions, especially bug fixes.

For example CycloneDX/specification#204: there was a bug in the CycloneDX json schema < 1.5 which allowed a mix of multiple licenses.

• This bug was fixed in CycloneDX 1.5
• This bug did not exist in CycloneDX xml schema < 1.5. The correct behaviour is: licenses are either a list of exactly one SPDX Expression or a list of unbound amount of (either named license or SPDX ID).

Since this wrong behavior was allowed in CDX 1.3 json, there might exist documents that are not valid CDX 1.5 json - for obvious reasons.

[tokcum]

There are some nits like outstanding TODOs and commented out code. I pointed them out where I noticed them. I don't see any major issues.

I appreciate the thorough testing against the spec, and the doc comments!

This is quite a lot of code! In the interest of being able to reproduce this for future versions, how was this file generated? This is a lot of code. Did you use templating of some sort, or is it entirely handwritten?

The big question here is whether we want to keep 1.3 support around, or remove it and go all in on 1.4 (and soon 1.5). I think supporting older versions would create too much of a maintenance burden, and I am not aware of compelling use cases them, so I prefer to gut 1.3 support. Is there a good reason to keep it?

Hi @Shnatsel,

thank you for the feedback. Great, that we get this going. I'll continue to work on it.

To answer your questions, when I started to work on this I started by comparing the official specs of 1.3 vs. 1.4 to find out the differences. The result was that there were only a few really new things to consider. Most was "more of the same". Then I discussed the approach with @amy-keibler. She told my to keep the existing model part untouched as far as possible and add the code to support 1.4 under specs.
My understanding from what Amy told me was, that cyclonedx-bom should be able to support multiple versions with a best effort approach, i.e. not getting to strict about the details. Here is her comment that made me think in this direction.

Having said that, I copied specs/1.3 to specs/1.4 and started by implementing the "new" things in the official 1.4 spec. As I said, from an implementation perspective, most was not really new just more of the same. To be honest, at that time, many things were new to me, such as snapshot testing and how the existing code handled serialization and deserialization of XML. No templating behind the scenes, just a thoroughly done copy and paste job. Not very smart, sorry. Maybe we have to overhaul the approach at some point in the future.

To answer your last question, the reason why I started to work on this is, I wanted to be able to deserialize SBOMs generated by trivy. At that time, trivy generated SBOMs in CycloneDX version 1.4. Meanwhile trivy uses 1.5. Anyway, I'm in charge of security at a software editor with many formerly created SBOMs which I would like to process with cyclonedx-bom. So, it would be great if cyclonedx-bom could support former versions as well.

[tokcum]

It'd be great if we could support older versions of CycloneDX but I - personally - would be fine with 1.4 as the minimum if it makes this PR easier. If we already have all the logic in place to keep 1.3 as well and you both think it's fine then I'm happy to keep it in as well.

I reviewed my conversion with Amy and think that we should try to support multiple versions. nscur0 also mentioned the approach in the Slack discussion.

I'm pretty confident we can make it and I think we should consider to add some tests to check the compatibility when serializing / de-serializing different versions.

[lfrancke]

Thanks!
If we can support multiple versions then I'm very happy. I'll leave the implementation discussions to you but am happy to merge as soon as @Shnatsel approves.

[lfrancke]

Just a heads-up that this now has a few merge conflicts so you'd need to rebase it.

[Shnatsel]

FYI we'd like to get this merged in the next week or so, and then start on implementing v1.5. Please let me know if I can help in any way.

[lfrancke]

Hi @tokcum, if we don't hear back from you we plan on taking this PR over the finish line at the beginning of next week.

Thank you for all your work so far and being so patient.

[tokcum]

@Shnatsel and @lfrancke

Sorry, I've been occupied by other projects during the last weeks. I've remotely followed all the other PRs you got done recently and I would be very happy to see this PR finished by you. Thanks.

[justahero]

There is a small crate called ordered-float that should cover this nicely. It provides implementations for Ord & Eq. The Score struct then can look like:

#[derive(Debug, PartialEq, Eq)]
pub struct Score(OrderedFloat<f32>);

impl Score {
    pub fn new_unchecked(score: f32) -> Self {
        Score(score.into())
    }

    pub fn from_f32(score: f32) -> Option<Self> {
        // or add range check here, maybe clamp between 0..=10?
        Some(Score(score.into()))
    }

    pub fn to_f32(&self) -> f32 {
        self.0.0
    }
}

impl From<f32> for Score {
    fn from(value: f32) -> Self {
        Score(value.into())
    }
}

impl From<Score> for f32 {
    fn from(value: Score) -> f32 {
        value.0.0
    }
}

This simplifies conversions & should provide easier handling of scores.

[ctron]

It would be great to see that merged. Is there any case I could help?

[lfrancke]

Someone from the team will pick this up soon (this week).

There are just a few minor todos to handle and we believe the Snapshot tests don't necessarily pull their weight.
They are pretty big so our current plan is to prune them from the git history and do a followup PR.

I hope to see some progress here this or next week.

[Shnatsel]

The PR based on this one has been merged: #575

I am going to go ahead and close this. Thanks for your contribution @tokcum !