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

Add run image SBOM #186

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Oct 26, 2021

@buildpack-bot
Copy link
Member

Maintainers,

As you review this RFC please queue up issues to be created using the following commands:

/queue-issue <repo> "<title>" [labels]...
/unqueue-issue <uid>

Issues

(none)

@aemengo aemengo force-pushed the add-run-image-sbom branch 7 times, most recently from 0c1fd35 to 0e8719d Compare October 28, 2021 02:58
@aemengo aemengo marked this pull request as ready for review October 28, 2021 02:59
@aemengo aemengo requested a review from a team as a code owner October 28, 2021 02:59
This RFC proposes the following -

- A run image can contain a layer holding an SBOM (in format of CycloneDX or SBOM) of installed packages at `/sbom/bom.<ext>.json`, where `<ext>` will be `cdx` for CycloneDX documents and `spdx` for SPDX documents. These will initially be the only 2 supported SBOM types.
- The digest of this layer is written as a `LABEL` on the corresponding run image with the key: `io.buildpacks.sbom`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that https://github.com/buildpacks/rfcs/blob/main/text/0095-sbom.md doesn't actually specify the label name for the layer containing the buildpack-provided SBOM. Maybe io.buildpacks.sbom.build and io.buildpacks.sbom.base?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or io.buildpacks.sbom.buildpack for buildpack-provided SBOM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original intention was for this to be a key inside the lifecycle metadata label, but we can move it to a separate label if that helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During our last discussion, we centered around:

io.buildpacks.app.sbom for buildpack SBoMs label
io.buildpacks.base.sbom for run-image SBoM label

https://buildpacks.slack.com/archives/C94UJCNV6/p1635433855037000?thread_ts=1635433432.033600&cid=C94UJCNV6


And a label with key of `io.buildpacks.sbom`, and value of the digest of this layer, stored as metadata on the run image.

When the lifecycle comes across a run image with this label and valid digest, it will attempt to merge this SBOM with any SBOMs delivered by buildpacks (during the build phase) and output them at `/layers/config/<type>/sbom/bom.<ext>`, per [#RFC 95: SBOM](https://github.com/buildpacks/rfcs/blob/main/text/0095-sbom.md). Initially this merging will only be supported for CycloneDX as it has a well-defined and efficient way of merging multiple `bom` files. A reference implementation can be found at the [`cyclonedx-cli`](https://github.com/CycloneDX/cyclonedx-cli) project. In the future we may add support for merging SPDX `bom` files as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this work for rebase? It might be helpful to be a bit more explicit about exactly how many SBOM layers there are, and what they hold. I am thinking that one layer for buildpack-provided SBOMs, another for run-image-provided SBOMs, and another containing the "merged" SBOMs (with a separate label containing the diff ID for each) would make this process more straightforward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the clarity around how merges would work around rebase. This was part of the reason I left this out of the original RFC since I didn't have a good enough idea on how to implement it while still keeping rebase fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During our last discussion, we decided to keep merging out of scope for this RFC. As of right now, 2 sBOM layers (and 2 labels) are proposed: 1 for run-image, and 1 buildpack. Downstream tooling can do the "merging", if desired.

This RFC proposes the following -

- A run image can contain a layer holding an SBOM (in format of CycloneDX or SBOM) of installed packages at `/sbom/bom.<ext>.json`, where `<ext>` will be `cdx` for CycloneDX documents and `spdx` for SPDX documents. These will initially be the only 2 supported SBOM types.
- The digest of this layer is written as a `LABEL` on the corresponding run image with the key: `io.buildpacks.sbom`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original intention was for this to be a key inside the lifecycle metadata label, but we can move it to a separate label if that helps.

text/0000-run-image-sbom.md Outdated Show resolved Hide resolved

And a label with key of `io.buildpacks.sbom`, and value of the digest of this layer, stored as metadata on the run image.

When the lifecycle comes across a run image with this label and valid digest, it will attempt to merge this SBOM with any SBOMs delivered by buildpacks (during the build phase) and output them at `/layers/config/<type>/sbom/bom.<ext>`, per [#RFC 95: SBOM](https://github.com/buildpacks/rfcs/blob/main/text/0095-sbom.md). Initially this merging will only be supported for CycloneDX as it has a well-defined and efficient way of merging multiple `bom` files. A reference implementation can be found at the [`cyclonedx-cli`](https://github.com/CycloneDX/cyclonedx-cli) project. In the future we may add support for merging SPDX `bom` files as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the clarity around how merges would work around rebase. This was part of the reason I left this out of the original RFC since I didn't have a good enough idea on how to implement it while still keeping rebase fast.


This RFC proposes the following -

- A run image can contain a layer holding an SBOM (in format of CycloneDX or SPDX or Syft) of installed packages at `/cnb/sbom/bom.<ext>.json`, where `<ext>` will be `cdx` for CycloneDX documents, `spdx` for SPDX documents, and `syft` for Syft documents. These will initially be the only 3 supported SBOM types.
Copy link
Member

@sclevine sclevine Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that we use /cnb/sbom/launch/bom.<ext>.json, so that both the build-time and runtime SBoM could be included in the future, but that would make it difficult to use the same base image as both a runtime and build-time base image.

Ideally, the build-time and runtime base image SBoM could be included in the final image without needing to modify either of their corresponding layers.

Instead, maybe we could use /cnb/sbom/<id>.<ext>.json, where <id> is the first 8 characters of the original base image digest. E.g., /cnb/sbom/ef3945a2.cdx.json.

Uniquely labeling the SBoM with a digest means that we could also include unextended SBoMs when the Dockerfiles RFC is implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put together a POC for attaching base image SBoMs in this format: https://github.com/sclevine/cnb-sbom
(You can also use it to read both base image and app layer SBoMs.)

Copy link
Contributor Author

@aemengo aemengo Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in Office Hours, on Nov 18, w/ @samj1912. There's disagreement about the complexity that this (the <id>, 8 characters of original base image digest) would introduce.

--
edit: Based on the following, I'm retracting my comment above and re-introducing the <id> format.

- A run image can contain a layer holding an SBOM (in format of CycloneDX or SPDX or Syft) of installed packages at `/cnb/sbom/bom.<ext>.json`, where `<ext>` will be `cdx` for CycloneDX documents, `spdx` for SPDX documents, and `syft` for Syft documents. These will initially be the only 3 supported SBOM types.
- The digest of this layer is written as a `LABEL` on the corresponding run image with the key: `io.buildpacks.base.sbom`.
- For the sake of parity, the digest of buildpack SBOM layers (created by lifecycle) is written as a `LABEL` on the corresponding run image with the key: `io.buildpacks.app.sbom`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, we may want to add a flag to lifecycle to include build-time SBoMs in the app image. We could use labels like io.buildpacks.build.base.sbom and io.buildpacks.build.app.sbom.

I don't think this means we should rename io.buildpacks.base.sbom to io.buildpacks.launch.base.sbom -- just thought it might be worth pointing this out.

Copy link
Contributor Author

@aemengo aemengo Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we may. I personally find it hard to base decisions purely on what may happen. I'd be curious if others explicitly have this use-case.

This RFC proposes the following -

- A run image can contain a layer holding an SBOM (in format of CycloneDX or SPDX or Syft) of installed packages at `/cnb/sbom/bom.<ext>.json`, where `<ext>` will be `cdx` for CycloneDX documents, `spdx` for SPDX documents, and `syft` for Syft documents. These will initially be the only 3 supported SBOM types.
- The digest of this layer is written as a `LABEL` on the corresponding run image with the key: `io.buildpacks.base.sbom`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use the diffID instead of the digest, to match io.buildpacks.app.sbom

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@sophiewigmore
Copy link

Hey @aemengo! What's the status of this RFC? I haven't been able to attend the CNB working group meeting. I ask because I am working on a Paketo RFC that relies on this RFC, and wondering if you plan to make more changes and what the timeline is if so? Thanks!!

@aemengo
Copy link
Contributor Author

aemengo commented Nov 17, 2021

Hey @aemengo! What's the status of this RFC? I haven't been able to attend the CNB working group meeting. I ask because I am working on a Paketo RFC that relies on this RFC, and wondering if you plan to make more changes and what the timeline is if so? Thanks!!

@sophiewigmore Thank you for asking. Unfortunately, I'm afraid that more changes will be required on this RFC. At the very least, more discussion needs to happen. During our last working group meeting, we had concerns about the workflow of attaching a BOM to an already-built base image, and thereby polluting the image. As of right now, the timeline is unknown. If your Paketo RFC is pending Paketo concerns, it might be more expedient to move at your own pace. Finally, I want remind (in a non-patronizing way) that buildpack RFCs are soon to close when all the core team members have voted and the "Final Comment Period" status is applied.

@hone
Copy link
Member

hone commented Dec 15, 2021

@jkutner @nebhale can you please review? ❤️

@aemengo aemengo force-pushed the add-run-image-sbom branch 2 times, most recently from 26d8797 to 4347716 Compare December 16, 2021 20:12
@samj1912
Copy link
Member

samj1912 commented Dec 16, 2021

@aemengo really sorry to be going back and forth on this, but @natalieparellano and I were discussing the genpkgs implementation (https://buildpacks.slack.com/archives/CCP60GJLS/p1639669644077300) and it might make the implementation easier if the output file name did not use the digest - especially for implementing it in pack where the digest for the output run image may not be available until the lifecycle actually goes ahead and exports it (and in this case the lifecycle might have to export the output run image to the registry to get the digest).

The other issue is that image digests are sometimes not preserved across registry transfers which makes it even more of an issue.

I think we should instead solve it this way - use the diffid in the label and have the lifecycle ensure that it loads the bom from that diffid only if it is the last layer in the output image.

Signed-off-by: Anthony Emengo <aemengo@vmware.com>
└── bom.<ext>.json
```

And a label with key of `io.buildpacks.base.sbom`, and value of the diffID of this layer, stored as metadata on the run image.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add the condition here that this must be the last layer on the image for the lifecycle to consider it a valid sbom? This is to prevent further extensions from re-using the same sbom.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify. How would other extensions use an SBOM depending on the layer order?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aemengo I meant something like the following -

let's say someone takes the above base run image with the SBOm and label+diffid and extends it as

FROM valid-run-image-with-sbom
USER root
RUN apt-get install my-package

This new image will still have the above specified SBOM and label with the appropriate diffid of the SBOM layer, but the SBOM will be incorrect/stale.

With my proposal, the above extension will cause other layers to be added after the SBOM layer which might be a good way to indicate to the lifecycle that the SBOM is potentially stale since the layer with the SBOM diffid won't be the last layer anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add the condition here that this must be the last layer on the image for the lifecycle to consider it a valid sbom?

@samj1912 If we go this route maybe we can loosen this a little to be SHOULD? I think a warning might be sufficient here.

Also, I am not sure what "must" would imply here. Would the build/rebase fail? Would the lifecycle remove the io.buildpacks.base.sbom` entirely.

@hone
Copy link
Member

hone commented Jan 12, 2022

@jkutner @ekcasey can you please take a look at this 🙏

@sclevine sclevine self-requested a review February 9, 2022 19:19
@jkutner jkutner self-requested a review February 10, 2022 15:07
@jabrown85
Copy link
Contributor

How will this work for Stack Extensions that replace the run image?


## Tooling

To facilitate adherence to this specification, the CNB project will create tooling that consumes a given run container image and a given SBOM json file, to create a new image with the SBOM information embedded at the correct location and diffID metadata attached. This allows for generation of SBOM files both within the image and outside it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this inteneded to be separate tooling or a part of pack. I would like to see the answer that question in this RFC.

Once we know which sub-team is responsible I think it's fine to punt the details of the interface to a sub-team specific RFC but it would be good to align generally about who this RFC is handing the baton to.


# Alternatives
[alternatives]: #alternatives

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Label is one alternative albeit an unappealing one b/c of the containerd label size issue.

An annotation is another alternative. The drawbacks to an annotation are consistency with buildpack-generated SBOM and the impossibility of doing something sensible in the daemon case.

@hone
Copy link
Member

hone commented Feb 16, 2022

@natalieparellano volunteering to take this over.

@ekcasey
Copy link
Member

ekcasey commented Feb 17, 2022

Summary of WG Discussion 02/17/22

There is lingering discomfort around storing this SBOM in the run image because:

  1. special tooling is required to create the image
  2. it may fall out of date
  3. there is widespread acknowledgment that the layer solution is temporary pending changes (removal of daemon support) that would allow us to store the SBOM in an annotation or attestation

Although we can solve problem 1 with custom tooling. Problem 2 cannot be completely solved. While the lifecycle can use the layer order as a hint about whether the SBOM is out of date, the lifecycle cannot guarantee accuracy. Efforts to solve for or mitigate problem 1 and mitigate problem 2 feel like a waste of effort given problem 3. It feels especially bad to bake new temporary interfaces into the spec, given that we would like to move towards spec stability.

We discussed moving the SBOM out of the run image, and making it a responsibility of the platform to supply an accurate run-image SBOM to the lifecycle for export, at which point it will be included in the sbom layer with the buildpacks generated SBOM. This free platforms up to be as strict or flexible as they want and to adapt to evolving specifications around sbom and attestation format/storage.

(cc @samj1912 @natalieparellano @jabrown85 @sclevine @jromero )

Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekcasey based on your latest comment do we have someone adopting this RFC to make the necessary changes or creating a superseding RFC? It sounds like we would still need an RFC for the SBOM contribution to the exporter.

@natalieparellano
Copy link
Member

@jromero please see #211

@hone hone marked this pull request as draft March 23, 2022 18:26
@loewenstein loewenstein mentioned this pull request May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet