Instead of continuing when Attestation data is not present, this shou…

[trmiller]

…ld be failing with a clear error. Otherwise it will incorrectly return success

[trmiller]

@sudo-bmitch PTAL - I think the previous change inadvertently marked the request successful, where as this should fail, just not crash like it was.

[pxp928]

LGTM

[sudo-bmitch]

Thinking through this, the for loop on 35 feels like the issue. We continue if the uuid's don't match. It's a success if the for loop is empty or only has bad uuid entries. It fails if any one of the entries is bad. And I'm guessing we want logic that succeeds if any entry matches, and fails for all other scenarios.

[sudo-bmitch]

Here's a diff of the option I'm thinking of:

$ git diff
diff --git a/cmd/cli/rekor/rekoruuid_validator.go b/cmd/cli/rekor/rekoruuid_validator.go
index bea88ab..0bc4ba0 100644
--- a/cmd/cli/rekor/rekoruuid_validator.go
+++ b/cmd/cli/rekor/rekoruuid_validator.go
@@ -69,9 +69,11 @@ func Validate(vendorFile config.VendorFile, downloadedFile string) (err error) {
                        }
                        return err
                }
+               // valid entry found
+               return nil
        }
 
-       return nil
+       return fmt.Errorf("no valid rekor entries found, payload contained %v", resp.Payload)
 }
 
 type ImageValidationError struct {

However, that does trigger an error with the validation we're trying to run. Looking at the log entry, Attestation is allowed to be nil and is not marked as a required field, so I suspect we want to validate based on the Body which is required:
https://pkg.go.dev/github.com/sigstore/[email protected]/pkg/generated/models#LogEntryAnon

Looking at that body field in at least one failing command, it appears to be base64 encoded data, but since it's an empty interface, we probably can't assume it will always be the same format:

...
Error: no valid rekor entries found, payload contained map[de02942bd2a6ebca8c094b7e69d31ccbc38d528d37f1b18d2f008e3710779f10:{<nil> eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaW50b3RvIiwic3BlYyI6eyJjb250ZW50Ijp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiIwMmYyY2VkMDU1ODVjNjY2NTVmODg5Mzc5NTc0M2Y5Njg3MWVhMjIyYTg4N2Y1YzY2ZGY1ZTJmZDJhMWRmZjAxIn19LCJwdWJsaWNLZXkiOiJMUzB0TFMxQ1JVZEpUaUJRVlVKTVNVTWdTMFZaTFMwdExTMEtUVVpyZDBWM1dVaExiMXBKZW1vd1EwRlJXVWxMYjFwSmVtb3dSRUZSWTBSUlowRkZia3hPZHpOU1dYZzVlRkZxV0dKVlJYYzRkbTl1V0ROVk5DdDBRZ3ByVUc1S2NTdDZkRE00TmxORGIwY3daWGRKU0RWTlFqZ3JSMnBKUkVkQmNsVlZURk5FWm1wbVRUTXhSV0ZsTHpjeGEyRjJRVlZKTUU5QlBUMEtMUzB0TFMxRlRrUWdVRlZDVEVsRElFdEZXUzB0TFMwdENnPT0ifX0= 0xc000025b90 0xc00052a730 0xc000025b98 0xc000020440}]

$ base64 -d
eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaW50b3RvIiwic3BlYyI6eyJjb250ZW50Ijp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiIwMmYyY2VkMDU1ODVjNjY2NTVmODg5Mzc5NTc0M2Y5Njg3MWVhMjIyYTg4N2Y1YzY2ZGY1ZTJmZDJhMWRmZjAxIn19LCJwdWJsaWNLZXkiOiJMUzB0TFMxQ1JVZEpUaUJRVlVKTVNVTWdTMFZaTFMwdExTMEtUVVpyZDBWM1dVaExiMXBKZW1vd1EwRlJXVWxMYjFwSmVtb3dSRUZSWTBSUlowRkZia3hPZHpOU1dYZzVlRkZxV0dKVlJYYzRkbTl1V0ROVk5DdDBRZ3ByVUc1S2NTdDZkRE00TmxORGIwY3daWGRKU0RWTlFqZ3JSMnBKUkVkQmNsVlZURk5FWm1wbVRUTXhSV0ZsTHpjeGEyRjJRVlZKTUU5QlBUMEtMUzB0TFMxRlRrUWdVRlZDVEVsRElFdEZXUzB0TFMwdENnPT0ifX0=
{"apiVersion":"0.0.1","kind":"intoto","spec":{"content":{"hash":{"algorithm":"sha256","value":"02f2ced05585c66655f8893795743f96871ea222a887f5c66df5e2fd2a1dff01"}},"publicKey":"LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUZrd0V3WUhLb1pJemowQ0FRWUlLb1pJemowREFRY0RRZ0FFbkxOdzNSWXg5eFFqWGJVRXc4dm9uWDNVNCt0QgprUG5KcSt6dDM4NlNDb0cwZXdJSDVNQjgrR2pJREdBclVVTFNEZmpmTTMxRWFlLzcxa2F2QVVJME9BPT0KLS0tLS1FTkQgUFVCTElDIEtFWS0tLS0tCg=="}}

TL;DR we can prevent the false positives, but I think there's still work to validate when Attestation is nil to prevent false negatives.