Add SBOM Discovery with Rekor

[mdeicas]

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

[lumjjb]

@asraa Need some help with your expertise around rekor inclusion proofs.. would you be able to help review this test? Thanks!

[spiffcs]

Are digest and Sha256 guaranteed to not be nil in this case?

[mdeicas]

Sha256's default value is the empty string, which would be caught in this function. Is this what you meant?

[spiffcs]

Yep! I just want to be sure that the Digest field here never has a chance to throw a nil pointer exception.

[spiffcs]

If Digest was ever nil then Digest.Sha256 would panic

[wagoodman]

very nice work! I added a few suggestions, mostly nit. From a high level there are a lot of uses of pointers that appear unnecessary --not a blocking comment, but it would be good to do another pass at trying to remove some pointer references.

[wagoodman]

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.

[wagoodman]

do we have a nice way to generate this without needing to rely on an external 3rd party docker image?

[mdeicas]

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

[spiffcs]

@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

[spiffcs]

Also @mdeicas and @lumjjb - I created the kubecon-draft branch so we can update this to be handed off and merged here before we take the plunge on main.

If you have a better name that you're using for this project I can absolutely update that branch name!

[mdeicas]

kubecon-draft is good with me; I changed the PR target to it. I can file another PR with changes from these comments when it is merged.

[lumjjb]

I created the kubecon-draft branch so we can update this to be handed off and merged here before we take the plunge on main.

SGTM!

[mdeicas]

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 kubecon-draft and then review these changes as another PR. Alternatively, I can just push the changes to this PR. Which approach should we take?

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.

[spiffcs]

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 kubecon-draft and then review these changes as another PR. Alternatively, I can just push the changes to this PR. Which approach should we take?

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.

[mdeicas]

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.

[spiffcs]

@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!

[mdeicas]

@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