Add SBOM Discovery with Rekor #1157
Conversation
mdeicas
force-pushed
the
rekor-cataloger
branch
2 times, most recently
from
eb08c20
to
95d5497
Compare
Signed-off-by: Marco Deicas <mdeicas@google.com>
Signed-off-by: Marco Deicas <mdeicas@google.com>
Signed-off-by: Marco Deicas <mdeicas@google.com>
Signed-off-by: Marco Deicas <mdeicas@google.com>
mdeicas
force-pushed
the
rekor-cataloger
branch
from
95d5497
to
582177f
Compare
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_getAndVerifySbomFromUuid(t *testing.T) { |
There was a problem hiding this comment.
@asraa Need some help with your expertise around rekor inclusion proofs.. would you be able to help review this test? Thanks!
| return errors.New("attestation has no sboms") | ||
| } | ||
| // take entry at index 0 because we currently do not handle multiple sboms within one attestation | ||
| expectedHash := att.Predicate.Sboms[0].Digest.Sha256 |
There was a problem hiding this comment.
Are digest and Sha256 guaranteed to not be nil in this case?
There was a problem hiding this comment.
Sha256's default value is the empty string, which would be caught in this function. Is this what you meant?
There was a problem hiding this comment.
Yep! I just want to be sure that the Digest field here never has a chance to throw a nil pointer exception.
There was a problem hiding this comment.
If Digest was ever nil then Digest.Sha256 would panic
| if len(att.Predicate.Sboms) > 1 { | ||
| log.Info("attestation found on Rekor with multiple SBOMS, which is not currently supported. Proceeding with the first SBOM.") | ||
| } | ||
| uri := att.Predicate.Sboms[0].URI |
There was a problem hiding this comment.
adding 2 cents, sorry for the length: I agree that there is no problem since validateAttestation is indeed called before getSbom, but this could be made more obvious. Ideally validations for code should be close (or really, as close as possible) to the code that needs this validation. Additionally, we should defend against future changes which might accidentally erode/reverse assumptions made here (in this case that validateAttestation is called in advance and has the proper checks relative to what's needed here for getting the SBOM).
One way to do this is to have the caller pass a more constrained data type:
att, err := parseAndValidateAttestation(logEntryAnon)
...
sbomBytes, err := getSbom(att.Predicate.Sboms[0] , client.httpClient)
...
This way getSbom is given the data that it operates on (not the full attestation) and the assumption that ...Sboms[0] will not panic is closer to the code that does this verification (still could be made closer, and the more closer the more ideal).
Though it's a little more clear, this code could still technically panic. We can adjust this to still not have redundant checks, but also make it safer:
func getFirstSbom(sboms []sbomEntry, client *http.Client) ([]byte, error) {
if len(att.Predicate.Sboms) > 1 {
// log something...
}
var bytes []byte
for _, s := range sboms {
resp, err client.Get(s.URI)
...
// get the sbom bytes, assign to 'bytes', then "break"
}
if len(bytes) == 0 {
return nil, fmt.Errorf("no sbom found")
}
return bytes, nil
}
And the caller:
att, err := parseAndValidateAttestation(logEntryAnon)
...
sbomBytes, err := getFirstSbom(att.Predicate.Sboms, client.httpClient)
...
Side note: renaming getSbom to getFirstSbom makes sense in this latter case, and with how it's written today.
| @@ -0,0 +1,2 @@ | |||
| # An image containing the example sample-golang-prov binary from https://github.com/lumjjb/sample-golang-prov | |||
| FROM docker.io/mdeicas/sample-golang-prov:latest | |||
There was a problem hiding this comment.
do we have a nice way to generate this without needing to rely on an external 3rd party docker image?
There was a problem hiding this comment.
One alternative is to put the sample-golang-prov binary in test-fixtures and have FROM scratch COPY sample-golang-prov /. Does this sound better?
If this change is made, then the same should probably be done for the image-rust-auditable docker file.
@wagoodman
|
@mdeicas low priority, but there are some linting errors showing up on the static analysis: I'm going through the rekor changes today and will add comments to that part of the codebase |
|
|
SGTM! |
|
I have changes for all of the comments in this thread ready in another branch (except for integration test w/o docker image and removing pointer references). It might be easier to merge this PR into Regardless, should I rebase and squash the commits in this PR to a single commit? The changes I have in this other branch include a fix for the static analysis check that failed and for the rekor cataloger integration test that failed. The integration test failed because catalogers that use external sources are now off by default. |
I think we can merge this PR into kubecon-draft. I can do that as a squash if you're ok with it @mdeicas. I know you took care to break the commits into logical units, but also see a value in squashing this into the draft branch as a baseline. We can then file a new PR against the draft branch with the changes you mentioned. |
Sounds good to me. Go ahead and squash. |
|
@lumjjb @wagoodman @mdeicas - changes have been merged into anchore:kubecon-draft feel free to file smaller PR against that branch with fixes mentioned in this thread or any additional information! |
Awesome thank you! Will do |
This PR adds the ability to discover build-time SBOMs from binaries with the Rekor transparency log. It does this by creating external document references for them in SPDX JSON. Explained in more detail in syft issue #1159 Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
This PR adds the ability to discover build-time SBOMs from binaries with the Rekor transparency log. It does this by creating external document references for them in SPDX JSON. Explained in more detail in syft issue #1159 Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
This PR adds the ability to discover build-time SBOMs from binaries with the Rekor transparency log. It does this by creating external document references for them in SPDX JSON. Explained in more detail in syft issue #1159 Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
This PR adds the ability to discover build-time SBOMs from binaries with the Rekor transparency log. It does this by creating external document references for them in SPDX JSON. Explained in more detail in syft issue #1159 Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
This PR adds the ability to discover build-time SBOMs of binaries with the Rekor transparency log and to create external document references for them in SPDX JSON. Explained in more detail in #1159