buildpacks / libcnb

A non-opinionated language binding for the Cloud Native Buildpack Buildpack and Extension specifications
Apache License 2.0
31 stars 13 forks source link

Adds SBOM Formatter #140

Closed ForestEckhardt closed 2 months ago

ForestEckhardt commented 2 years ago

This PR adds an SBOM Formatter similar to the one found in the (packit/v2 implementation)[https://github.com/paketo-buildpacks/packit/blob/v2/sbom_formatter.go]. I believe that the current libcnb implementation is far too sparse leaving a lot of work up to either buildpack authors or library authors to decide the best way of handling the process of writing SBOMs. I believe that this interface is powerful because it allows for authors to still implement custom SBOMs however it abstracts away much of the boiler plate when it comes to having to actually manage and write the SBOM files. With this interface an author only needs to focus on writing a formatter that conforms to the io.Reader interface everything else will be handled for them by the library.

I would love this PR to be a conversation starter for a more unified SBOM experience as I see this as being one of the last barriers for packit being able to be built on top of libcnb.

dmikusa commented 2 years ago

A couple of notes:

  1. I think this needs to be rebased. There appear to be unrelated file changes happening from #139, which makes it a little hard to follow this PR.

  2. I don't really want to remove the existing SBOM functionality like the format consts and the methods for getting paths to the SBOM locations. Similarly, I still think there's value in the validation of SBOM types. I'm not opposed to adding new functionality, but I think we should build on what's there.

  3. Do you have any examples of how the SBOMFormat/SBOMFormatter is being used in actual buildpacks? I'm not sure I'm getting a good understanding from the tests. I'm curious how it can be used in a buildpack.

ForestEckhardt commented 2 years ago
  1. I think this needs to be rebased. There appear to be unrelated file changes happening from Removes usage of ioutil #139, which makes it a little hard to follow this PR.

Yeah that is my bad got ahead of myself. Rebased and hopefully should be easier to follow not.

  1. I don't really want to remove the existing SBOM functionality like the format consts and the methods for getting paths to the SBOM locations. Similarly, I still think there's value in the validation of SBOM types. I'm not opposed to adding new functionality, but I think we should build on what's there.

So would you just like to support both methods of setting the SBOM. Support a SBOM formatter but also allow authors to just fetch a SBOM path and then arbitrarily write a file there? I am a little worried that might muddy the API so I am curious what you feel the advantage of keeping the old API around.

I am willing to go in and add back SBOM type validation but I would like to hear the argument for keeping the path fetching method.

  1. Do you have any examples of how the SBOMFormat/SBOMFormatter is being used in actual buildpacks? I'm not sure I'm getting a good understanding from the tests. I'm curious how it can be used in a buildpack.

This is the formatter that is used by most packit/v2 buildpacks that have layer SBOM support. An example of where we use this is go-build in build.go. Here we use a custom packit SBOM formatter to scan the layer that contains all of our built binaries and then with that one scan we are able to generate multiple SBOM formats. We do this so that we only need to scan the app with Syft one and then using the data from that one scan we can generate all of the SBOMs that we need.

dmikusa commented 2 years ago

So would you just like to support both methods of setting the SBOM. Support a SBOM formatter but also allow authors to just fetch a SBOM path and then arbitrarily write a file there? I am a little worried that might muddy the API so I am curious what you feel the advantage of keeping the old API around.

To me, the reason I think we need to keep the existing API around is that it's not just for writing SBOMs. It's for anything where you might want to get the SBOM path. I definitely use it in testing. Ex: test cases to exercise conditional logic to write or not write the SBOM files. It may be less relevant with the introduction of the formatters, but the point is that having convenient ways to get the SBOM paths can be useful for other reasons as well. Since libcnb wraps the API, it seems natural to provide convenient functionality like this.

I'm pretty sure that we had originally discussed this style of API as well, where the actual writes are abstracted away. I'm trying to remember why we opted not to do that. I can't exactly remember though, sorry. Not sure if @samj1912 recalls. Definitely curious to hear his thoughts on this.

ForestEckhardt commented 2 years ago

@dmikusa-pivotal Alright everything should be merged to be more understandable!

samj1912 commented 2 years ago

@ForestEckhardt there are also some open questions about the way that packit implements SBOMs https://paketobuildpacks.slack.com/archives/C011S6EL49L/p1653411685386879

should we table this PR until at the very least the relevant parties can meet and discuss the API around the current packit implementation and its potential pros/cons?

ForestEckhardt commented 2 years ago

@ForestEckhardt there are also some open questions about the way that packit implements SBOMs https://paketobuildpacks.slack.com/archives/C011S6EL49L/p1653411685386879

should we table this PR until at the very least the relevant parties can meet and discuss the API around the current packit implementation and its potential pros/cons?

I am more than happy to table this for now. This PR was to hopefully have a good starting point for discussions and some practical code to look at during those discussions. I would appreciate any synchronous decisions to make their way here if at all possible.

ForestEckhardt commented 2 years ago

@samj1912 Is there any update on the discussions around this PR?

dmikusa commented 2 months ago

This PR is out of date, so I'm going to close it.

I started a conversation on Slack so we can discuss this more and what this might look like in the library.