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.

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

feat: handle dssev001 tlog entry types #799

Merged
merged 8 commits into from
Aug 24, 2024

Conversation

ramonpetgrave64
Copy link
Contributor

re: slsa-framework/slsa-github-generator#3750

Rekor TLog entries can now be of the type dsse v0.0.1, as when what's returned when using sigstore-go's Bundle().

This is to support eventual Sigstore Bundles produced by slsa-github-generator's "generic" generator, which will likely use sigstore-go's Bundle to produce attestations

Tesing

Followup

Finish the work to produce bundles from the generic generators

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
len(env.Signatures),
len(dsseSchemaObj.Signatures))
}
// TODO(#487): verify the certs match.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from the original method. Not all envelopes will have certificates, so that implementation would be opportunistic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse

func extractCert(e *models.LogEntryAnon) (*x509.Certificate, error) {
to do this comparison?
It's best practice to compare the canonicalized entry for Rekor to what you have (e.g the artifact hash, certificate, and signature, or computing the leaf hash yourself and comparing), since there is a risk of someone swapping out an inclusion proof that is valid but for the wrong entry. Of course, the chance of two signatures being the same is very low, so there isn't really a threat here.

Copy link

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor things

verifiers/internal/gha/bundle.go Outdated Show resolved Hide resolved
verifiers/internal/gha/bundle.go Outdated Show resolved Hide resolved
verifiers/internal/gha/bundle.go Show resolved Hide resolved
verifiers/internal/gha/bundle.go Outdated Show resolved Hide resolved
ramonpetgrave64 and others added 5 commits August 15, 2024 20:55
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Copy link

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Dunno if you want a more "go-y" person to do a review too

@ramonpetgrave64
Copy link
Contributor Author

@cmurphy, please take a look

len(env.Signatures),
len(dsseSchemaObj.Signatures))
}
// TODO(#487): verify the certs match.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse

func extractCert(e *models.LogEntryAnon) (*x509.Certificate, error) {
to do this comparison?
It's best practice to compare the canonicalized entry for Rekor to what you have (e.g the artifact hash, certificate, and signature, or computing the leaf hash yourself and comparing), since there is a risk of someone swapping out an inclusion proof that is valid but for the wrong entry. Of course, the chance of two signatures being the same is very low, so there isn't really a threat here.

@ramonpetgrave64
Copy link
Contributor Author

@haydentherapper We can't yet because for the older attestations produce by slsa-github-generator, the certificate was not embedded within the envelope.

But that seems like another good reason for #487

@ramonpetgrave64 ramonpetgrave64 enabled auto-merge (squash) August 24, 2024 03:16
@ramonpetgrave64 ramonpetgrave64 merged commit 767ecf9 into main Aug 24, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants