feat: add cyclonedx schema version selection #2123
Conversation
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch |
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
spiffcs
changed the title
syft/formats/cyclonedxxml/format.go
Outdated
| @@ -7,14 +7,66 @@ import ( | |||
| "github.com/anchore/syft/syft/sbom" | |||
| ) | |||
|
|
|||
| const ID sbom.FormatID = "cyclonedx-xml" | |||
| const ID sbom.FormatID = "cyclonedx" | |||
There was a problem hiding this comment.
I think this is a consequential change, can you expound on this a bit? (I think there is an alias for cyclonedx already, which I want to say defaults to json, but I haven't verified this)
There was a problem hiding this comment.
actually, this doesn't seem right: this is changing the ID for this format to something more generic, but the ID should be as specific as possible. SPDX does this by having a list of IDs, where the first is the ID and the remaining are unique aliases
Then that list is used in the format object
syft/syft/formats/spdxtagvalue/format.go
Line 18 in 9de4129
This PR seems to be reversing this, where the aliases are more specific
Since all of these instances are the same can you make a list and share that similar to SPDX?
There was a problem hiding this comment.
Ahh - I think this was just an unexpected change on my part thanks for the catch - I looked back at the diff and the original format had the alias of cyclonedx. The original ID was cyclonedx-xml. I think a unit test was failing and to fix it I put the wrong alias back in the wrong spot. I'll fix this now. The default for cyclonedx is not json but actually xml - see here on main:
syft/syft/formats/cyclonedxxml/format.go
Lines 12 to 20 in 9de4129
There was a problem hiding this comment.
@wagoodman this should now be reverted to where there are no ID changes and aliases are consistent across the new formats
syft/formats/formats.go
Outdated
| @@ -71,6 +81,13 @@ func ByNameAndVersion(name string, version string) sbom.Format { | |||
| for _, f := range Formats() { | |||
| for _, n := range f.IDs() { | |||
| if cleanFormatName(string(n)) == name && versionMatches(f.Version(), version) { | |||
| if version == sbom.AnyVersion && strings.Contains(string(n), "cyclonedx") { | |||
There was a problem hiding this comment.
This is looking for the string cyclonedx but we might introduce an alias like cdx. I think this should more formally ask the format for the latest supported version.
At the same time, I feel that this format object is about to be rethought anyway. I think there are strong signs to split up for encoding vs decoding use cases, which means trying to settle this detail in this PR wouldn't be worth it. I think it's worth at least adding a comment here about the limitations of this approach and a TODO to refactor later.
There was a problem hiding this comment.
also, how does ByName work for spdx for the latest version (I didn't read up on that yet)?
There was a problem hiding this comment.
I feel that this format object is about to be rethought anyway.
I think there are strong signs to split up for encoding vs decoding use cases,
which means trying to settle this detail in this PR wouldn't be worth it.
^ Yea that sounds good to me
I think this should more formally ask the format for the latest supported version
I had tried just setting up a default version for each format rather than latest so we had more flexibility, but that would constitute more of a refactor for the format object that I didn't want to include in this PR.
The comment for this line should be below it I can move above and add a TODO:
// if the version is not specified and the format is cyclonedx, then we want to return the most recent version up to 1.4
// https://github.com/CycloneDX/cyclonedx-go/pull/90
For SPDX if you pass in syft -o spdx alpine:latest then no version is detected. mostRecentFormat will eventually become 2.3
There was a problem hiding this comment.
I would probably like to add a Default field to the format object. It could be accomplished without a huge refactor (see related comment below). At some point in the future, when we support SPDX 3, we will probably also want to continue to default to SPDX 2.3.
There was a problem hiding this comment.
I can follow this PR up with a stab at that. I really want to keep this PR small and focused on integrating the released cyclonedx library. Other changes on how we handle formats, their defaults or how the latest is selected can be their own change set. The reason this line had to be included was because we don't necessarily want to be on latest for cyclonedx at this moment. Rather than rewrite ByNameAndVersion into a totally new thing I thought a simple edge case with comment would be the correct change here
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
| func Format() sbom.Format { | ||
| var Format = Format1_4 | ||
|
|
||
| func Format1_0() sbom.Format { |
There was a problem hiding this comment.
It might be nice to programmatically build these formats, something like this could live in the common/cyclonedxhelpers package:
func Formats(format cyclonedx.BOMFileFormat, ids ...sbom.FormatID) (out []sbom.Format) {
versions := []cyclonedx.SpecVersion{
cyclonedx.SpecVersion1_0,
cyclonedx.SpecVersion1_1,
cyclonedx.SpecVersion1_2,
cyclonedx.SpecVersion1_3,
cyclonedx.SpecVersion1_4,
cyclonedx.SpecVersion1_5,
}
for i := range versions {
version := versions[i]
encoder := func(output io.Writer, s sbom.SBOM) error {
return encodeCycloneDX(output, s, format, version)
}
if version == cyclonedx.SpecVersion1_4 {
// I had another comment about maybe adding a `Default` field to the format struct, which seems to have been lost.
// assuming that this was something we wanted to do, it could look like this without a huge refactor:
out = append(out, sbom.NewFormatDefault(
version.String(),
encoder,
cyclonedxhelpers.GetDecoder(format),
cyclonedxhelpers.GetValidator(format),
ids...,
))
} else {
out = append(out, sbom.NewFormat(
version.String(),
encoder,
cyclonedxhelpers.GetDecoder(format),
cyclonedxhelpers.GetValidator(format),
ids...,
))
}
}
}This way there only needs to be a single encodeCycloneDX() function and adding new versions is just updating the list, which would update both XML and JSON variants. E.g. the XML variant would have:
func Formats() []sbom.Format {
return cyclonedxhelpers.Formats(cyclonedx.BOMFileFormatXML, ID, "cyclonedx", "cyclone")
}There was a problem hiding this comment.
This is a good suggestion, but I think we wanted to save this kind of work for when we go back and take a look at Formats before syft v1.0. The purpose of this PR was to just keep this as close to the same pattern as we have with SPDX. Getting creative here and trying to build these programmatically (different from the current SPDX pattern) would eventually get swallowed by that new work.
There was a problem hiding this comment.
agreed to the both of you -- it's a good suggestion, but it would be better to be consistent with the current pattern first, then refactor to "the next" pattern.
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
syft/formats/formats.go
Outdated
| // formats to inform this function what their default version is | ||
| // TODO: remove this check when 1.5 is stable or default formats are designed. PR below should be merged. | ||
| // https://github.com/CycloneDX/cyclonedx-go/pull/90 | ||
| if version == sbom.AnyVersion && strings.Contains(string(n), "cyclonedx") { |
There was a problem hiding this comment.
This check should probably be strings.Contains(string(n), "cyclone") to use the same default when -o cyclone is used.
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
--------- Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Summary
This PR moves the cyclone-dx format section closer to how we do spdx version selection.
The above command should now provide the header:
Users can also look forward and try v1.5. Syft will move to this as the default version when this milestone is complete:
CycloneDX/cyclonedx-go#90
With the latest update to the cyclone-dx golang library v1.5 is minimally supported (v0.7.2). This PR keeps the default output of cyclone-dx (xml/json) to be v1.4, but allows users to specify if they'd like to try and generate
v1.5or any of the older specification versions.TODO
AllIDscovers newly added schema version options https://github.com/anchore/syft/blob/main/test/cli/all_formats_expressible_test.go