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.

Sign up for GitHub

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

Jump to bottom

Verify sigs and attestations in parallel #3066

Merged
merged 2 commits into from
Jun 20, 2023
Merged

Verify sigs and attestations in parallel #3066

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfe0dd3
Verify sigs and attestations in parallel
priyawadhwa Jun 20, 2023
41db33e
Fix race condition with bundleVerified bool
priyawadhwa Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 68 additions & 30 deletions pkg/cosign/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"os"
"regexp"
"strings"
"sync"
"time"

"github.com/pkg/errors"
Expand Down Expand Up @@ -597,24 +598,42 @@ func verifySignatures(ctx context.Context, sigs oci.Signatures, h v1.Hash, co *C
}
}

validationErrs := []string{}
validationErrs := make([]string, len(sl))
signatures := make([]oci.Signature, len(sl))
bundlesVerified := make([]bool, len(sl))

for _, sig := range sl {
sig, err := static.Copy(sig)
if err != nil {
validationErrs = append(validationErrs, err.Error())
continue
}
verified, err := VerifyImageSignature(ctx, sig, h, co)
bundleVerified = bundleVerified || verified
if err != nil {
validationErrs = append(validationErrs, err.Error())
continue
var wg sync.WaitGroup

for i, sig := range sl {
wg.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here as below.

go func(sig oci.Signature, index int) {
defer wg.Done()
sig, err := static.Copy(sig)
if err != nil {
validationErrs[index] = err.Error()
return
}
verified, err := VerifyImageSignature(ctx, sig, h, co)
bundlesVerified[index] = verified
if err != nil {
validationErrs[index] = err.Error()
return
}
signatures[index] = sig
}(sig, i)
}
wg.Wait()

for _, s := range signatures {
if s != nil {
checkedSignatures = append(checkedSignatures, s)
}
}

// Phew, we made it.
checkedSignatures = append(checkedSignatures, sig)
for _, verified := range bundlesVerified {
bundleVerified = bundleVerified || verified
}

if len(checkedSignatures) == 0 {
// TODO: ErrNoMatchingSignatures.Unwrap should return []error,
// or we should replace "...%s" strings.Join with "...%w", errors.Join.
Expand Down Expand Up @@ -934,25 +953,44 @@ func verifyImageAttestations(ctx context.Context, atts oci.Signatures, h v1.Hash
return nil, false, err
}

validationErrs := []string{}
for _, att := range sl {
att, err := static.Copy(att)
if err != nil {
validationErrs = append(validationErrs, err.Error())
continue
}
if err := func(att oci.Signature) error {
verified, err := verifyInternal(ctx, att, h, verifyOCIAttestation, co)
bundleVerified = bundleVerified || verified
return err
}(att); err != nil {
validationErrs = append(validationErrs, err.Error())
continue
validationErrs := make([]string, len(sl))
attestations := make([]oci.Signature, len(sl))
bundlesVerified := make([]bool, len(sl))

var wg sync.WaitGroup

for i, att := range sl {
wg.Add(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to recall (maybe I'm forgetting some details and have something wrong), but for loops where you're then launching go funcs, you needed to make a local copy, so something like this:
for i, att := range sl {
i := i
att := att
wg.Add(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yah i've seen that before! i think that passing in the values to the go function, like go func(att oci.Signature, index int), copies the values as well and accomplishes the same thing

I looked into it just to make sure i hadn't mixed up different things and this post says you can do it either way :)

Copy link
Contributor

Choose a reason for hiding this comment

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

lol, yeah, thanks for looking. I couldn't recall all the details, but remember there be dragons!!

go func(att oci.Signature, index int) {
defer wg.Done()
att, err := static.Copy(att)
if err != nil {
validationErrs[index] = err.Error()
return
}
if err := func(att oci.Signature) error {
verified, err := verifyInternal(ctx, att, h, verifyOCIAttestation, co)
bundlesVerified[index] = verified
return err
}(att); err != nil {
validationErrs[index] = err.Error()
return
}
attestations[index] = att
}(att, i)
}
wg.Wait()

for _, a := range attestations {
if a != nil {
checkedAttestations = append(checkedAttestations, a)
}
}

// Phew, we made it.
checkedAttestations = append(checkedAttestations, att)
for _, verified := range bundlesVerified {
bundleVerified = bundleVerified || verified
}

if len(checkedAttestations) == 0 {
return nil, false, &ErrNoMatchingAttestations{
fmt.Errorf("no matching attestations: %s", strings.Join(validationErrs, "\n ")),
Expand Down