Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backward/forward compatibility for Tools (cdx 1.5) #115

Closed
manifestori opened this issue Aug 13, 2023 · 11 comments · Fixed by #90
Closed

backward/forward compatibility for Tools (cdx 1.5) #115

manifestori opened this issue Aug 13, 2023 · 11 comments · Fixed by #90
Labels

Comments

@manifestori
Copy link

Hi :)

I've been following the progress of cdx1.5 support in this library for quite some time now. I've noticed tools such as Trivy already used in production (trivy cli tool).

I wondered about your approach to maintaining compatibility (read/write) for Tools. It's the first time you've had to change the actual field type. Will you use an overriding mechanism, such as implementing UnmarshalJSON and convert old slices into the new model?

The decoding part is much more apparent as convertTool would probably be used for this purpose.

Can I work on this solution? I'd like to see a v1.5 compatible version of this library in the stable release.

@nscuro
Copy link
Member

nscuro commented Aug 14, 2023

Hey @manifestori, appreciate you reaching out!

The tools change has given me a bit of a headache if I'm being honest. Here are my thoughts:

  • The original Tools was not removed in v1.5, but only deprecated. So even users generating BOMs following v1.5 of the spec should still be able to use it. This means we cannot simply convert legacy tools to tool components etc. when encoding.
  • If we want to stay backward-compatible, the current Metadata.Tools field must stay as it is, which means adding support for the new tool types would require a new field. For lack of a better name, let's call it ToolsNew for now.

What I think should happen:

  1. When encoding for a spec version lower than v1.5, tools specified as components or services will need to be converted to legacy tools (implies potential loss of information).
  2. When encoding, a custom MarshalJSON and MarshalXML for Metadata must check both Tools and ToolsNew. If both are set, that's an error. If only one of them is set, encode its content as tools.
  3. When decoding, a custom UnmarshalJSON and UnmarshalXML for Metadata must inspect the tools node, and populate Tools or ToolsNew accordingly.

I'd love to avoid having to maintain custom marshalling logic for Metadata , so definitely open for other suggestions if you have any! Also, if you'd like to help here, that'd be much appreciated for sure.

@manifestori
Copy link
Author

We would never be able to approach this without custom logic for MarshalXXX and UnmarshalXXX as Tools cannot be a slice and not a slice at the same time :)

I liked you're approach for going with ToolsNew probably changing current tool to ToolsLegacy and making Tools as the existing spec format.

A different approach would be to change Tools to the new spec. converting older spec versions Tools into the new structure when encoding wouldn't be too hard; those "fake" components would have some fields missing (bom-ref, etc)
when decoding to older spec, we can just convert Tools.Components/Tools.Services back to the list of legacy Tools.

We just need to decide how to convert vendor. (author? publisher? group? supplier?)

@nscuro
Copy link
Member

nscuro commented Aug 22, 2023

Apologies for the late reply @manifestori.

We just need to decide how to convert vendor. (author? publisher? group? supplier?)

Doesn't seem like vendor can be mapped with 100% accuracy to any of those fields. I think supplier comes the closest, the other fields have a different meaning (selling something [vendor, supplier] doesn't imply authorship or publishing of the sold item).

@manifestori
Copy link
Author

Yeah I figured, but that's close enough.

So basically, adding ToolsLegacy for older specs, and use Tools for current spec.

I guess cdx spec won't change Tools again soon ;)

@nscuro
Copy link
Member

nscuro commented Aug 22, 2023

Yeah that sounds like a good way forward!

I guess cdx spec won't change Tools again soon ;)

Sure hope so...

@manifestori
Copy link
Author

Are you looking for community contributions or is it already on your roadmap @nscuro?

@northdpole
Copy link

hey @nscuro , +1 on the above, is this high on your priority list? otherwise happy to contribute the fix myself

@nscuro
Copy link
Member

nscuro commented Oct 19, 2023

Thanks @northdpole, appreciate the offer! It would indeed help immensely if we could get this contributed, as I am a bit burried with other stuff. However, if this is critical for you and potentially others, I may be able to re-prioritize.

@northdpole
Copy link

it's not critical at the moment as most tools I use support format definition, i'll try and get to this in the next 2 weeks

@northdpole
Copy link

sorry folks, I've ran out of time to do this, please don't wait for me

@nscuro
Copy link
Member

nscuro commented Dec 9, 2023

So the way I've settled on is this:

  • The type of Metadata.Tools and Vulnerability.Tools changes from *[]Tool to *ToolsChoice
  • ToolsChoice holds both "legacy" *[]Tool, as well as the new *[]Component and *[]Service fields
  • The Tool type, as well as the ToolsChoice.Tools field are marked as deprecated
  • During encoding and decoding, it is asserted that only one of both options can be present
  • When encoding to lower spec versions, Components and Services are converted to legacy Tools

nscuro added a commit that referenced this issue Dec 9, 2023
closes #115

Signed-off-by: nscuro <nscuro@protonmail.com>
@nscuro nscuro closed this as completed in #90 Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants