Skip to content

imagetools inspect: handle provenance and sboms#1444

Merged
tonistiigi merged 2 commits intodocker:masterfrom
crazy-max:inspect-attest
Jan 7, 2023
Merged

imagetools inspect: handle provenance and sboms#1444
tonistiigi merged 2 commits intodocker:masterfrom
crazy-max:inspect-attest

Conversation

@crazy-max
Copy link
Copy Markdown
Member

@crazy-max crazy-max commented Dec 2, 2022 •
edited
Loading

Supports provenance and sbom for imagetools inspect command.

$ docker buildx imagetools inspect crazymax/buildkit:attest --format "{{json .}}"

https://gist.github.com/crazy-max/5bbc2e60c58263a0162bbf3b920d3d09

@crazy-max crazy-max added the kind/enhancement New feature or request label Dec 3, 2022
jedevc
jedevc reviewed Dec 6, 2022
View reviewed changes
Comment thread docs/reference/buildx_imagetools_inspect.md Outdated
Comment thread util/imagetools/loader.go
Comment thread util/imagetools/loader.go
Comment thread util/imagetools/loader.go Outdated
Comment thread util/imagetools/loader.go Outdated
Comment thread util/imagetools/printers.go
@jedevc jedevc mentioned this pull request Dec 8, 2022
Merged
crazy-max
crazy-max commented Dec 13, 2022
View reviewed changes
Comment thread util/imagetools/loader.go
@@ -0,0 +1,415 @@
package imagetools

// TODO: replace with go-imageinspect library when public
Copy link
Copy Markdown
Member Author

@crazy-max crazy-max Dec 13, 2022

Choose a reason for hiding this comment

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

@jedevc Yes we should use in follow-up the go-imageinspect library.

@crazy-max crazy-max force-pushed the inspect-attest branch 2 times, most recently from b14ba80 to 6dc71a9 Compare December 13, 2022 17:00
@crazy-max crazy-max marked this pull request as ready for review December 14, 2022 18:54
@crazy-max crazy-max added this to the v0.10.0 milestone Dec 15, 2022
@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Dec 15, 2022

Is there a way we can support multiple SBOMs? e.g. in the case of a dockerfile where multiple stages have been scanned, we should have a way to get multiple.

Maybe .SBOM for the "first" SBOM, and .SBOMS[idx] for others?

Side note: the issue with ordering here is that the "most relevant" SBOM is currently not guaranteed to be first (though I think I have a solution for fixing that buildkit-side).

@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Dec 15, 2022

Aside from the above, LGTM 🎉

tonistiigi
tonistiigi reviewed Dec 15, 2022
View reviewed changes
Comment thread util/imagetools/loader.go Outdated
return nil
}

type provenance struct { // TODO: this is only a stub, to be refactored later
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi Dec 15, 2022

Choose a reason for hiding this comment

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

Could we just show the raw SLSA in here. Just json.RawMessage.

tonistiigi
tonistiigi reviewed Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

The SBOM part seems ok but I don't think we should invent another temporary provenance type atm.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Dec 15, 2022

If we possibly want to add our own typed structs for SBOM/Provenance in the future then maybe it is better if we leave the names of the raw structs as SPDX and SLSA atm.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Dec 16, 2022

Did some testing with https://github.com/docker/buildx/compare/master...tonistiigi:buildx:inspect-provenance?expand=1 branch. Mostly seems to work with raw values but commands are really slow(when blobs are being pulled I believe). Maybe this can be optimized or should show debug logs. Especially because some of the SBOM values can be really big they should only be pulled when user really asks for the data.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jan 5, 2023

If we possibly want to add our own typed structs for SBOM/Provenance in the future then maybe it is better if we leave the names of the raw structs as SPDX and SLSA atm.

wdyt @crazy-max @jedevc

@crazy-max
Copy link
Copy Markdown
Member Author

crazy-max commented Jan 5, 2023 •
edited
Loading

If we possibly want to add our own typed structs for SBOM/Provenance in the future then maybe it is better if we leave the names of the raw structs as SPDX and SLSA atm.

Not sure about the SLSA terminology here. It can be VSA or Provenance atm so I think Provenance still makes sense. Maybe SLSAProvenance? And for SBOM we could have SBOMSPDX?

@jedevc
Copy link
Copy Markdown
Collaborator

jedevc commented Jan 5, 2023 •
edited
Loading

The merged name combination seems odd to me - maybe we could go with {{ .SBOM.SPDX }} and {{ .Provenance.SLSA }}? Those can be the raw data structures, leaving us open in the future to additional formats such as {{ .SBOM.CycloneDX }}, as well as utility fields for our own typed structs in the future, e.g. {{ .SBOM.Packages[0].Name }}.

@crazy-max
imagetools inspect: handle provenance and sbom
e6b9aba 
use stub structs for SLSA/SBOM while waiting for
go-imageinspect library to be public.

Signed-off-by: CrazyMax <[email protected]>
@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jan 5, 2023

maybe we could go with {{ .SBOM.SPDX }} and {{ .Provenance.SLSA }}?

I'm not sure if it is possible to have {{ .SBOM.SPDX }} be the spdx and {{ .SBOM }} itself a clear package list later without all the spdx garbage. If this is possible then lgtm. Otherwise SBOMSPDX seems fine to me as well. Although that would also mess up {{ json . }}. I think what we really need is just a custom format handling for these special keys (eg like atm there some commands have --format table). Content is only loaded when the format is exactly the key. In {{ json . }} there would be no SBOM fields.

@tonistiigi tonistiigi merged commit 332dfb4 into docker:master Jan 7, 2023
@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Jan 7, 2023

I merged this in but if we still want to make changes to naming we still can before the GA.

@crazy-max crazy-max deleted the inspect-attest branch January 7, 2023 08:46
@jedevc jedevc mentioned this pull request Jan 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jedevc jedevc jedevc left review comments

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees

No one assigned

Labels

kind/enhancement New feature or request

Projects

None yet

Milestone

v0.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@crazy-max @jedevc @tonistiigi