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.

Sign up for GitHub

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

Jump to bottom

imagetools inspect: handle provenance and sboms #1444

Merged
merged 2 commits into from
Jan 7, 2023
Merged

imagetools inspect: handle provenance and sboms #1444

merged 2 commits into from
Jan 7, 2023

Conversation

crazy-max
Copy link
Member

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

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
docs/reference/buildx_imagetools_inspect.md Outdated Show resolved Hide resolved
util/imagetools/loader.go Show resolved Hide resolved
util/imagetools/loader.go Show resolved Hide resolved
util/imagetools/loader.go Outdated Show resolved Hide resolved
util/imagetools/loader.go Outdated Show resolved Hide resolved
util/imagetools/printers.go Show resolved Hide resolved
@jedevc jedevc mentioned this pull request Dec 8, 2022
Merged
crazy-max
crazy-max commented Dec 13, 2022
View reviewed changes
util/imagetools/loader.go
@@ -0,0 +1,415 @@
package imagetools

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

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
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
Collaborator

jedevc commented Dec 15, 2022

Aside from the above, LGTM 🎉

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

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

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
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
Member

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
Member

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.

@crazy-max
imagetools inspect: missing annotations key
2fb9db9 
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@tonistiigi
Copy link
Member

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
Member Author

crazy-max commented Jan 5, 2023 •
edited

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
Collaborator

jedevc commented Jan 5, 2023 •
edited

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 <crazy-max@users.noreply.github.com>
@tonistiigi
Copy link
Member

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
Member

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.

None yet
3 participants
@crazy-max @jedevc @tonistiigi