-
Notifications
You must be signed in to change notification settings - Fork 49
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.
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
fix: fix rekor verification #277
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
This change will need backporting to all major releases! TODO tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -470,15 +470,6 @@ func Test_runVerifyGHAArtifactPath(t *testing.T) { | |||
err: serrors.ErrorMismatchWorkflowInputs, | |||
noversion: true, | |||
}, | |||
// Regression test of sharded UUID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to remove this or only ommit the v0.0.2?
RootCerts: roots, | ||
CertOidcIssuer: certOidcIssuer, | ||
} | ||
verifier, err := cosign.ValidateAndUnpackCert(certs[0], co) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe create cert : cert[0]
variable.
// GetRekorEntriesWithCert finds all entry UUIDs with the full intoto attestation. | ||
// The attestation generated by the slsa-github-generator libraries contain a signing certificate. | ||
func GetRekorEntriesWithCert(rClient *client.Rekor, provenance []byte) (*dsselib.Envelope, *x509.Certificate, error) { | ||
func GetRekorEntriesWithCert(rClient *client.Rekor, provenance []byte) ( | ||
*models.LogEntryAnon, error) { | ||
// Use intoto attestation to find rekor entry UUIDs. | ||
params := entries.NewSearchLogQueryParams() | ||
searchLogQuery := models.SearchLogQuery{} | ||
certPem, err := envelope.GetCertFromEnvelope(provenance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We. already extract the cert in the caller certPem, err := envelope.GetCertFromEnvelope(provenance)
, shall we pass it as argument to this function?
} | ||
|
||
if len(resp.GetPayload()) != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to change != 1
to ==0
?
if err != nil { | ||
fmt.Fprintf(os.Stderr, "Error computing leaf hash for tlog entry at index: %d\n", *entry.LogIndex) | ||
continue | ||
if err := rverify.VerifyLogEntry(context.Background(), &e, verifier); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a comment what this function verifies. This verifies the SET only? Do we need to verify inclusion proof + root signature to verify the entry is part of the current tree... is there even a way to verify it's the "current" top of the tree...?
Thank you! I realized in doing the backported fixes that I can make a much less invasive change because that helped me pinpoint the exact issue; I will make the full fixes here (relying on rekor libraries) after the backports just FYI |
Signed-off-by: Asra Ali asraa@google.com
This fixes rekor verification by:
VerifyProvenanceSignature
: verifies the provenance in a step-by-step manner: covering signature validation, rekor lookup, and signature time validation (against entry upload time)