From 5f8154319992ab40f3389ec7e6514a866563a40f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 18 May 2022 23:38:14 -0700 Subject: [PATCH 1/6] pkg/verify/store/configmap: Correct Signatures godocs to not return list This function calls the callback for each signature, it doesn't return a list. Fixes a godoc from the original implementation in c43c2cb208 (Create a resuable verify package for image release verification, 2020-07-16, #837). --- pkg/verify/store/configmap/configmap.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/verify/store/configmap/configmap.go b/pkg/verify/store/configmap/configmap.go index f1c374f461..ec404e48f1 100644 --- a/pkg/verify/store/configmap/configmap.go +++ b/pkg/verify/store/configmap/configmap.go @@ -84,8 +84,8 @@ func (s *Store) mostRecentConfigMaps() []corev1.ConfigMap { return s.last } -// Signatures returns a list of signatures that match the request -// digest out of config maps labelled with ReleaseLabelConfigMap in the +// Signatures fetches signatures for the provided digest +// out of config maps labelled with ReleaseLabelConfigMap in the // NamespaceLabelConfigMap namespace. func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { // avoid repeatedly reloading config maps From ead7c5a60819bf4bfa28859613fccba13c00955f Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 18 May 2022 23:43:15 -0700 Subject: [PATCH 2/6] pkg/verify: Timestamp store.Signatures errors Extending 1b9753d298 (pkg/verify: Expose underlying signature errors, 2022-04-21, #1358) with timestamps, so it's easy to see what's slow in situations like [1] where some portion of signature verification is surprisingly slow, and it's currently not clear what aspect is causing the slowdown. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2071998#c2 --- pkg/verify/verify.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/verify/verify.go b/pkg/verify/verify.go index a1f8d935a2..893996bf04 100644 --- a/pkg/verify/verify.go +++ b/pkg/verify/verify.go @@ -189,19 +189,19 @@ func (v *releaseVerifier) Verify(ctx context.Context, releaseDigest string) erro err := v.store.Signatures(ctx, "", releaseDigest, func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { if errIn != nil { klog.V(4).Infof("error retrieving signature for %s: %v", releaseDigest, errIn) - errs = append(errs, errIn) + errs = append(errs, fmt.Errorf("%s: %w", time.Now().Format(time.RFC3339), errIn)) return false, nil } for k, keyring := range remaining { content, _, err := verifySignatureWithKeyring(bytes.NewReader(signature), keyring) if err != nil { klog.V(4).Infof("keyring %q could not verify signature for %s: %v", k, releaseDigest, err) - errs = append(errs, err) + errs = append(errs, fmt.Errorf("%s: %w", time.Now().Format(time.RFC3339), err)) continue } if err := verifyAtomicContainerSignature(content, releaseDigest); err != nil { klog.V(4).Infof("signature for %s is not valid: %v", releaseDigest, err) - errs = append(errs, err) + errs = append(errs, fmt.Errorf("%s: %w", time.Now().Format(time.RFC3339), err)) continue } delete(remaining, k) From b594c76c85a80602e5ed62bf675f0ca9fd9937e1 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 19 May 2022 00:47:29 -0700 Subject: [PATCH 3/6] pkg/verify/store: Pass ErrNotFound to callback function Extending 1b9753d298 (pkg/verify: Expose underlying signature errors, 2022-04-21, #1358) with information about when store retrieval is exhausted, so it's easy to see what's slow in situations like [1] where some portion of signature verification is surprisingly slow, and it's currently not clear what aspect is causing the slowdown. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2071998#c2 --- pkg/verify/store/configmap/configmap.go | 6 ++++++ pkg/verify/store/memory/memory.go | 4 +++- pkg/verify/store/parallel/parallel.go | 8 ++++++- pkg/verify/store/parallel/parallel_test.go | 4 +++- pkg/verify/store/serial/serial.go | 3 ++- pkg/verify/store/serial/serial_test.go | 4 +++- pkg/verify/store/sigstore/sigstore.go | 25 +++++++++------------- pkg/verify/store/sigstore/sigstore_test.go | 7 +++++- pkg/verify/store/store.go | 17 +++++++++++++-- 9 files changed, 55 insertions(+), 23 deletions(-) diff --git a/pkg/verify/store/configmap/configmap.go b/pkg/verify/store/configmap/configmap.go index ec404e48f1..95706ae142 100644 --- a/pkg/verify/store/configmap/configmap.go +++ b/pkg/verify/store/configmap/configmap.go @@ -122,6 +122,12 @@ func (s *Store) Signatures(ctx context.Context, name string, digest string, fn s } } } + if done, err := fn(ctx, nil, fmt.Errorf("prefix %s in config map %s: %w", prefix, cm.ObjectMeta.Name, store.ErrNotFound)); err != nil || done { + return err + } + if err := ctx.Err(); err != nil { + return err + } } return nil } diff --git a/pkg/verify/store/memory/memory.go b/pkg/verify/store/memory/memory.go index a3256e9c0c..2642fbde6d 100644 --- a/pkg/verify/store/memory/memory.go +++ b/pkg/verify/store/memory/memory.go @@ -4,6 +4,7 @@ package memory import ( "context" + "fmt" "github.com/openshift/library-go/pkg/verify/store" ) @@ -26,7 +27,8 @@ func (s *Store) Signatures(ctx context.Context, name string, digest string, fn s } } - return nil + _, err := fn(ctx, nil, fmt.Errorf("%s %s: %w", s.String(), digest, store.ErrNotFound)) + return err } // String returns a description of where this store finds diff --git a/pkg/verify/store/parallel/parallel.go b/pkg/verify/store/parallel/parallel.go index 4d3161ad90..57d4a11e50 100644 --- a/pkg/verify/store/parallel/parallel.go +++ b/pkg/verify/store/parallel/parallel.go @@ -74,7 +74,13 @@ func (s *Store) Signatures(ctx context.Context, name string, digest string, fn s if loopError != nil { return loopError } - return ctx.Err() // because we discard context errors from the wrapped stores + + if err := ctx.Err(); err != nil { + return err // because we discard context errors from the wrapped stores + } + + _, err := fn(ctx, nil, fmt.Errorf("%s: %w", s.String(), store.ErrNotFound)) + return err } // String returns a description of where this store finds diff --git a/pkg/verify/store/parallel/parallel_test.go b/pkg/verify/store/parallel/parallel_test.go index 6a245a3249..5d0d095370 100644 --- a/pkg/verify/store/parallel/parallel_test.go +++ b/pkg/verify/store/parallel/parallel_test.go @@ -139,7 +139,9 @@ func TestStore(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { signatures := []string{} err := parallel.Signatures(ctx, "name", "sha256:123", func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { - if errIn != nil { + if errors.Is(errIn, store.ErrNotFound) { + return false, nil + } else if errIn != nil { return false, errIn } signatures = append(signatures, string(signature)) diff --git a/pkg/verify/store/serial/serial.go b/pkg/verify/store/serial/serial.go index 700c001a84..2c47b1ece3 100644 --- a/pkg/verify/store/serial/serial.go +++ b/pkg/verify/store/serial/serial.go @@ -38,7 +38,8 @@ func (s *Store) Signatures(ctx context.Context, name string, digest string, fn s } } - return nil + _, err := fn(ctx, nil, fmt.Errorf("%s: %w", s.String(), store.ErrNotFound)) + return err } // String returns a description of where this store finds diff --git a/pkg/verify/store/serial/serial_test.go b/pkg/verify/store/serial/serial_test.go index 6b9bd98730..8c14d90109 100644 --- a/pkg/verify/store/serial/serial_test.go +++ b/pkg/verify/store/serial/serial_test.go @@ -74,7 +74,9 @@ func TestStore(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { signatures := []string{} err := serial.Signatures(ctx, "name", "sha256:123", func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { - if errIn != nil { + if errors.Is(errIn, store.ErrNotFound) { + return false, nil + } else if errIn != nil { return false, errIn } signatures = append(signatures, string(signature)) diff --git a/pkg/verify/store/sigstore/sigstore.go b/pkg/verify/store/sigstore/sigstore.go index d61bb07f4e..32bbd0b418 100644 --- a/pkg/verify/store/sigstore/sigstore.go +++ b/pkg/verify/store/sigstore/sigstore.go @@ -32,8 +32,6 @@ import ( // an attacker was able to take ownership of the store to perform DoS on clusters). const maxSignatureSearch = 10 -var errNotFound = errors.New("no more signatures to check") - // Store provides access to signatures stored in memory. type Store struct { // URI is the base from which signature URIs are constructed. @@ -85,7 +83,7 @@ func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, ma req, err := http.NewRequest("GET", sigURL.String(), nil) if err != nil { - _, err = fn(ctx, nil, fmt.Errorf("could not build request to check signature: %v", err)) + _, err = fn(ctx, nil, fmt.Errorf("could not build request to check signature: %w", err)) return err // even if the callback ate the error, no sense in checking later indexes which will fail the same way } req = req.WithContext(ctx) @@ -110,25 +108,22 @@ func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, ma }() if resp.StatusCode == http.StatusNotFound { - return nil, errNotFound + return nil, store.ErrNotFound } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - if i == 1 { - klog.V(4).Infof("Could not find signature at store location %v", sigURL) - } - return nil, fmt.Errorf("unable to retrieve signature from %v: %d", sigURL, resp.StatusCode) + return nil, fmt.Errorf("response status %d", resp.StatusCode) } return ioutil.ReadAll(resp.Body) }() - if err == errNotFound { - break - } if err != nil { - klog.V(4).Info(err) - done, err := fn(ctx, nil, err) - if done || err != nil { - return err + err := fmt.Errorf("unable to retrieve signature from %s: %w", sigURL.String(), err) + done, err2 := fn(ctx, nil, err) + if done || err2 != nil { + return err2 + } + if errors.Is(err, store.ErrNotFound) { + break } continue } diff --git a/pkg/verify/store/sigstore/sigstore_test.go b/pkg/verify/store/sigstore/sigstore_test.go index cb33c13a1b..3428f693ac 100644 --- a/pkg/verify/store/sigstore/sigstore_test.go +++ b/pkg/verify/store/sigstore/sigstore_test.go @@ -3,12 +3,15 @@ package sigstore import ( "bytes" "context" + "errors" "io/ioutil" "net/http" "net/url" "reflect" "regexp" "testing" + + "github.com/openshift/library-go/pkg/verify/store" ) // RoundTripper implements http.RoundTripper in memory. @@ -113,7 +116,9 @@ func TestStore(t *testing.T) { var signatures []string err := sigstore.Signatures(ctx, "name", "sha256:123", func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { - if errIn != nil { + if errors.Is(errIn, store.ErrNotFound) { + return false, nil + } else if errIn != nil { return false, errIn } signatures = append(signatures, string(signature)) diff --git a/pkg/verify/store/store.go b/pkg/verify/store/store.go index 00a19d2be4..8c98cbdac7 100644 --- a/pkg/verify/store/store.go +++ b/pkg/verify/store/store.go @@ -3,6 +3,7 @@ package store import ( "context" + "errors" ) // Callback returns true if an acceptable signature has been found, or @@ -11,13 +12,25 @@ import ( // problem and the function can decide how to handle that error. type Callback func(ctx context.Context, signature []byte, errIn error) (done bool, err error) +// ErrNotFound is a base error for Callback, to be used when the store +// decides one signature-retrieval avenue is exhausted. +var ErrNotFound = errors.New("no more signatures to check") + // Store provides access to signatures by digest. type Store interface { // Signatures fetches signatures for the provided digest, feeding // them into the provided callback until an acceptable signature is - // found or an error occurs. Not finding any acceptable signatures - // is not an error; it is up to the caller to handle that case. + // found or an error occurs. + // + // Not finding additional signatures should result in a callback + // call with an error wrapping ErrNotFound, to allow the caller to + // figure out when and why the store was unable to find a signature. + // When a store has several lookup mechanisms, this may result in + // several callback calls with different ErrNotFound. Signatures + // itself should return nil in this case, because eventually running + // out of signatures is an expected part of any invocation where the + // callback calls never return done=true. Signatures(ctx context.Context, name string, digest string, fn Callback) error // String returns a description of where this store finds From 7df3fc0246fdc50298320eb5ecc629b4d52fee50 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 19 May 2022 01:36:12 -0700 Subject: [PATCH 4/6] pkg/verify: Include Signatures error in aggregated error We can hit this case if lookup fails (e.g. because it takes too long to retrieve over HTTP, and the Context times out). Instead of failing with just "context deadline exceeded", timestamp this error, roll it in with the others, and fall through to consider any remaining verifiers who have not yet been satisfied. --- pkg/verify/verify.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/verify/verify.go b/pkg/verify/verify.go index 893996bf04..73bbbd97cf 100644 --- a/pkg/verify/verify.go +++ b/pkg/verify/verify.go @@ -210,9 +210,8 @@ func (v *releaseVerifier) Verify(ctx context.Context, releaseDigest string) erro return len(remaining) == 0, nil }) if err != nil { - klog.V(4).Infof("Failed to retrieve signatures for %s (should never happen)", releaseDigest) - errs = append(errs, err) - return err + klog.V(4).Infof("Failed to retrieve signatures for %s: %v", releaseDigest, err) + errs = append(errs, fmt.Errorf("%s: %w", time.Now().Format(time.RFC3339), err)) } if len(remaining) > 0 { From 000ff45c91ba4a781115ee37464efcbde136d43e Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 19 May 2022 02:37:11 -0700 Subject: [PATCH 5/6] pkg/verify/store/parallel: Best-effort, non-blocking response write on context cancel From the spec [1]: If one or more of the communications can proceed, a single one that can proceed is chosen via a uniform pseudo-random selection. Otherwise, if there is a default case, that case is chosen. If there is no default case, the "select" statement blocks until at least one of the communications can proceed. So in this callback, we have been called by Signatures, and have a signature/errIn we'd like to pass back through responses. But maybe responses is full. In that case, we try to block on the write, in case we get some space to write later on. But we don't want to block forever. Previously, the parallel ctx.Done() would bail us out of a too-long wait (good), but in cases where it won the select random-choice we would discard the response data we'd been trying to send (bad). With this commit, the ctx.Done() case gets a single, non-blocking write attempt, so we can pass along the signature/errIn information if there happens to be space. If not, there will be plenty of earlier responses in that channel that we'll be able to process later. While I'm touching things here, pivot from 'true, nil' to 'false, ctx.Err()' to show that we're a bit grumpy about being canceled. The errorChannel collector is going to silently discard the Canceled error when it's aggregating errorChannel content, but the presence of the error will make it less likely that later logic pivots misinterpret this as "successful validation". [1]: https://go.dev/ref/spec#Select_statements --- pkg/verify/store/parallel/parallel.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/verify/store/parallel/parallel.go b/pkg/verify/store/parallel/parallel.go index 57d4a11e50..f5737a327e 100644 --- a/pkg/verify/store/parallel/parallel.go +++ b/pkg/verify/store/parallel/parallel.go @@ -35,7 +35,11 @@ func (s *Store) Signatures(ctx context.Context, name string, digest string, fn s errorChannel <- wrappedStore.Signatures(ctx, name, digest, func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { select { case <-ctx.Done(): - return true, nil + select { + case responses <- signatureResponse{signature: signature, errIn: errIn}: + default: + } + return false, ctx.Err() case responses <- signatureResponse{signature: signature, errIn: errIn}: } return false, nil From 4f76483c0e3c47a5d7c2257f45fb2d6561fc7b9b Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Thu, 19 May 2022 02:56:34 -0700 Subject: [PATCH 6/6] pkg/verify: Test error handling for canceled sigstore HTTP calls Ensure that we have access to this data, so it's easy to see what's slow in situations like [1]. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2071998#c2 --- pkg/verify/configmap_test.go | 75 ++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/pkg/verify/configmap_test.go b/pkg/verify/configmap_test.go index 034e1c0b01..1dfe35848a 100644 --- a/pkg/verify/configmap_test.go +++ b/pkg/verify/configmap_test.go @@ -1,9 +1,15 @@ package verify import ( + "bytes" + "context" + "errors" "io/ioutil" + "net/http" "path/filepath" + "regexp" "testing" + "time" "golang.org/x/crypto/openpgp" @@ -14,6 +20,38 @@ type VerifierAccessor interface { Verifiers() map[string]openpgp.EntityList } +// roundTripper implements http.RoundTripper in memory. +type roundTripper struct { + data map[string]string + delay time.Duration + requests []string +} + +// RoundTrip implements http.RoundTripper. +func (rt *roundTripper) RoundTrip(request *http.Request) (*http.Response, error) { + ctx := request.Context() + rt.requests = append(rt.requests, request.URL.String()) + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(rt.delay): + } + + data, ok := rt.data[request.URL.String()] + if !ok { + return &http.Response{ + StatusCode: http.StatusNotFound, + Body: ioutil.NopCloser(bytes.NewReader(nil)), + }, nil + } + + return &http.Response{ + StatusCode: http.StatusOK, + Body: ioutil.NopCloser(bytes.NewReader([]byte(data))), + }, nil +} + func Test_newFromConfigMapData(t *testing.T) { redhatData, err := ioutil.ReadFile(filepath.Join("testdata", "keyrings", "redhat.txt")) if err != nil { @@ -78,3 +116,40 @@ func Test_newFromConfigMapData(t *testing.T) { }) } } + +func Test_newFromConfigMapData_slow_sigstore(t *testing.T) { + redhatData, err := ioutil.ReadFile(filepath.Join("testdata", "keyrings", "redhat.txt")) + if err != nil { + t.Fatal(err) + } + + rt := &roundTripper{ + delay: time.Second, + } + + verifier, err := newFromConfigMapData("from_test", map[string]string{ + "store-example": "https://example.com/signatures", + "verifier-public-key-redhat": string(redhatData), + }, func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }) + + if err != nil { + t.Fatalf("newFromConfigMapData() error = %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + err = verifier.Verify(ctx, "sha256:123") + expected := regexp.MustCompile(`^unable to verify sha256:123 against keyrings: verifier-public-key-redhat$`) + if !expected.MatchString(err.Error()) { + t.Fatalf("expected %s, got %s", expected, err.Error()) + } + + wrapped := errors.Unwrap(err) + expected = regexp.MustCompile(`^[[][0-9TZ:-]*: Get "https://example.com/signatures/sha256=123/signature-1": context deadline exceeded, [0-9TZ:-]*: context deadline exceeded]$`) + if !expected.MatchString(wrapped.Error()) { + t.Fatalf("expected %s, got %s", expected, wrapped.Error()) + } +}