ADR 44: describing support of SDPX SBOM format in konflux #213
Conversation
Signed-off-by: Jindrich Luza <jluza@redhat.com>
- Fixed title - Fixed typos and text corrections Signed-off-by: Jindrich Luza <jluza@redhat.com>
ADR/0039-spdx-support.md
Outdated
| - Package with the same package name and versionInfo set to None (or empty) is squashed with package with the same name and non-empty versionInfo | ||
| NOTE: packages cannot be merged together based on SPDXID attribute as there’s no specification in the spdx standard on how SPDXID should be calculated. Individual tools can calculate it differently while still passing condition to make it unique across the whole document. | ||
| ##### Relationships | ||
| SPDX relationships represent graph/tree structure of relations of elements in the document. The Root element is the SPDX document itself. Individual packages are in relationship CONTAINS with the root document. In the case of packages which were used to build the container, packages are in relationship BUILD_TOOL_OF with the root document. |
There was a problem hiding this comment.
Individual packages are in relationship CONTAINS with the root document.
Why is that important?
There was a problem hiding this comment.
I don't understand your question. It describes structure of the document so I guess that's why it's important.
There was a problem hiding this comment.
It might make this clearer to say The root element (the document itself) is named "SPDXRef-DOCUMENT".
The point, I think, is that an SPDX document can say "I describe this container image, which contains these things. The image was built using these other things, which aren't included". I don't know if CycloneDX 1.4 can express this.
There was a problem hiding this comment.
CycloneDX 1.4 probably can't, but we use 1.5
CycloneDX 1.5:
{
"metadata": {
"component": {
// main component (e.g. the output container image)
}
},
"components": [
// the dependencies included in the container image
],
"formulation": [
{
"components": [
// build-time-only dependencies of the container image
]
}
]
}Conversion to SPDX, in my mind:
SPDXRef-DOCUMENT DESCRIBES .metadata.component
.metadata.component CONTAINS .components[]
.formulation[].components[] BUILD-TOOL-OF .metadata.component
There was a problem hiding this comment.
In the current state, Konflux SBOMs don't have a useful .metadata.component (it's just the syft-specific nonsense component). Which IMO shouldn't stand in the way of this ADR, we should document the desired state and file a story to make it so
There was a problem hiding this comment.
I'm concerned about "base image" here: I think we're talking about builder images, not parent images.
Asi in, "parent image" = the last FROM instruction, "builder image" = any FROM instruction other than the last?
Currently, Konfux reports both of those in the .formulations[], distinguished by a custom property
There was a problem hiding this comment.
Asi in, "parent image" = the last FROM instruction, "builder image" = any FROM instruction other than the last?
Exactly.
There was a problem hiding this comment.
These make sense to me. The other BaseImages relations not so much - the SBOM doesn't describe the base images, those have their own SBOMs
So the you basically want:
SPDXRef-DOCUMENT -> DESCRIBES: SPDXRef-Image
SPDXRef-DOCUMENT -> CONTAINS: SPDXRef-Image
SPDXRef-Image -> CONTAINS: PackageA
SPDXRef-BuilderImage-Package-1-> BUILD_TOOL_OF: SPDXRef-Image
So for purl, I assume it will be something like:
pkg:docker/ubi9/ubi@sha256:abcdef?repository_url=registry.redhat.io
There was a problem hiding this comment.
That looks correct except for
SPDXRef-DOCUMENT -> CONTAINS: SPDXRef-Image
There was a problem hiding this comment.
And the purl will be pk:oci rather than pkg:docker, same as the builder/base images (but the exact format doesn't matter that much for this ADR). And FYI, Aleš is already working on the purl konflux-ci/build-tasks-dockerfiles#181
ADR/0039-spdx-support.md
Outdated
| - Package with the same package name and versionInfo set to None (or empty) is squashed with package with the same name and non-empty versionInfo | ||
| NOTE: packages cannot be merged together based on SPDXID attribute as there’s no specification in the spdx standard on how SPDXID should be calculated. Individual tools can calculate it differently while still passing condition to make it unique across the whole document. | ||
| ##### Relationships | ||
| SPDX relationships represent graph/tree structure of relations of elements in the document. The Root element is the SPDX document itself. Individual packages are in relationship CONTAINS with the root document. In the case of packages which were used to build the container, packages are in relationship BUILD_TOOL_OF with the root document. |
There was a problem hiding this comment.
It might make this clearer to say The root element (the document itself) is named "SPDXRef-DOCUMENT".
The point, I think, is that an SPDX document can say "I describe this container image, which contains these things. The image was built using these other things, which aren't included". I don't know if CycloneDX 1.4 can express this.
Signed-off-by: Jindrich Luza <jluza@redhat.com>
- Added section about syft specific behaviour
arewm
changed the title
|
It looks like this adds two identical ADRs with different numbers? |
- Fixed sbom lifecycle - fixed tables formating - added explanation of formulations conversion to relations - added more details in relationships merging Signed-off-by: Jindrich Luza <jluza@redhat.com>
I forgot to push mv changes. Now it should be in sync |
| | CycloneDX Attribute | SPDX Attribute | | ||
| |------------------------------|---------------------------------------------------------------| | ||
| | component.purl = `<PURL>` | package.externalRefs = [{referenceCategory:”PACKAGE-MANAGER”, | | ||
| | | referenceType:purl, | | ||
| | | referenceLocator: `<PURL>` | | ||
| | | }] | |
There was a problem hiding this comment.
Could you fix the formatting in this table and the tables below? It's not readable in the rendered doc https://github.com/midnightercz/konflux-ci-architecture/blob/spdx-support/ADR/0044-spdx-support.md#componentpurl
There was a problem hiding this comment.
I believe this was fixed. All tables look correct to me
There was a problem hiding this comment.
The loss of indentation hurts readability, especially for this one
How about changing them from Markdown tables to monospace blocks? Or just wrapping the tables in ``` so that they look the same as in the source code
| #### Merging SPDX | ||
| ##### Packages | ||
| Packages of two SPDX documents can be merged together as a concatenation of two lists. In cycloneDX component elements can have only a single purl attribute, therefore component elements representing packages with the same name and version but with different purl have to be stored as multiple elements. SPDX package elements can bear multiple purls. Therefore multiple cycloneDX components can be squashed together into single SPDX package element with purls concatenated into a single list. Following rules are applied to packages merging process: | ||
| - Packages with the same purl's package name and version and type are squashed into single package element |
There was a problem hiding this comment.
Similar concern to #213 (comment) - this shouldn't be the authoritative decision on how to merge packages. Not all tools should do it this way, cachi2 certainly shouldn't
There was a problem hiding this comment.
Perhaps we can just re-word this to make it clear that this is a sort of default suggestion for how to do the merging if you don't have more specific requirements
There was a problem hiding this comment.
yeah, good point
- Added root package into glossary - Added CDX metadata.component conversion section - Added example of merging relationship
Co-authored-by: Adam Cmiel <acmiel@redhat.com>
Co-authored-by: Adam Cmiel <acmiel@redhat.com>
- Remove consufing description from metadata.component
|
Oh and this thread still needs a resolution #213 (comment) |
description - Changed annotator format to reflex required format Signed-off-by: Jindrich Luza <jluza@redhat.com>
midnightercz
force-pushed
the
spdx-support
branch
from
570018d to
682e304
Compare
Co-authored-by: Adam Cmiel <acmiel@redhat.com>
Co-authored-by: Adam Cmiel <acmiel@redhat.com>
Signed-off-by: Jindrich Luza <jluza@redhat.com>
Signed-off-by: Jindrich Luza <jluza@redhat.com>
This commit builds on top of proposed SPDX support and addresses the following: * simplifies merge logic to align it with design in konflux-ci/architecture#213; * adjusts the model to make it work with Syft generated SPDX Sboms; * adds tests for some of existing and some new SPDX-CyDX handling and merging cases. This is not the final form and more work still has to be done: * several important test cases are pending; * CyDX SBOM has to be amended to better fit into the new structure; * Decisions need to be additionally documented within the code; * This commit and its parent must be split into several parts to simplify repository history. Co-authored-by: Jindrich Luza <midnightercz@gmail.com> Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
This commit builds on top of proposed SPDX support and addresses the following: * simplifies merge logic to align it with design in konflux-ci/architecture#213; * adjusts the model to make it work with Syft generated SPDX Sboms; * adds tests for some of existing and some new SPDX-CyDX handling and merging cases. This is not the final form and more work still has to be done: * several important test cases are pending; * CyDX SBOM has to be amended to better fit into the new structure; * Decisions need to be additionally documented within the code; * This commit and its parent must be split into several parts to simplify repository history. Co-authored-by: Jindrich Luza <midnightercz@gmail.com> Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
This commit builds on top of proposed SPDX support and addresses the following: * simplifies merge logic to align it with design in konflux-ci/architecture#213; * adjusts the model to make it work with Syft generated SPDX Sboms; * adds tests for some of existing and some new SPDX-CyDX handling and merging cases. This is not the final form and more work still has to be done: * several important test cases are pending; * CyDX SBOM has to be amended to better fit into the new structure; * Decisions need to be additionally documented within the code; * This commit and its parent must be split into several parts to simplify repository history. Co-authored-by: Jindrich Luza <midnightercz@gmail.com> Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>
No description provided.