diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 31632920f0..ae632da076 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -46,7 +46,9 @@ import ( "github.com/openshift/cluster-version-operator/pkg/payload/precondition" preconditioncv "github.com/openshift/cluster-version-operator/pkg/payload/precondition/clusterversion" "github.com/openshift/cluster-version-operator/pkg/verify" - "github.com/openshift/cluster-version-operator/pkg/verify/verifyconfigmap" + "github.com/openshift/cluster-version-operator/pkg/verify/store" + "github.com/openshift/cluster-version-operator/pkg/verify/store/configmap" + "github.com/openshift/cluster-version-operator/pkg/verify/store/serial" ) const ( @@ -306,8 +308,8 @@ func loadConfigMapVerifierDataFromUpdate(update *payload.Update, clientBuilder v // allow the verifier to consult the cluster for signature data, and also configure // a process that writes signatures back to that store - signatureStore := verifyconfigmap.NewStore(configMapClient, nil) - verifier = verifier.WithStores(signatureStore) + signatureStore := configmap.NewStore(configMapClient, nil) + verifier.Store = &serial.Store{Stores: []store.Store{signatureStore, verifier.Store}} persister := verify.NewSignatureStorePersister(signatureStore, verifier) return verifier, persister, nil } diff --git a/pkg/verify/configmap.go b/pkg/verify/configmap.go index d770101945..48d45b4b36 100644 --- a/pkg/verify/configmap.go +++ b/pkg/verify/configmap.go @@ -9,6 +9,9 @@ import ( "github.com/pkg/errors" "golang.org/x/crypto/openpgp" "k8s.io/klog" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" + "github.com/openshift/cluster-version-operator/pkg/verify/store/parallel" ) // ReleaseAnnotationConfigMapVerifier is an annotation set on a config map in the @@ -48,7 +51,7 @@ const ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-co // store and the lookup order is internally defined. func NewFromConfigMapData(src string, data map[string]string, clientBuilder ClientBuilder) (*ReleaseVerifier, error) { verifiers := make(map[string]openpgp.EntityList) - var stores []*url.URL + var stores []store.Store for k, v := range data { switch { case strings.HasPrefix(k, "verifier-public-key-"): @@ -63,7 +66,16 @@ func NewFromConfigMapData(src string, data map[string]string, clientBuilder Clie if err != nil || (u.Scheme != "http" && u.Scheme != "https" && u.Scheme != "file") { return nil, fmt.Errorf("%s has an invalid key %q: must be a valid URL with scheme file://, http://, or https://", src, k) } - stores = append(stores, u) + if u.Scheme == "file" { + stores = append(stores, &fileStore{ + directory: u.Path, + }) + } else { + stores = append(stores, &httpStore{ + uri: u, + httpClient: clientBuilder.HTTPClient, + }) + } default: klog.Warningf("An unexpected key was found in %s and will be ignored (expected store-* or verifier-public-key-*): %s", src, k) } @@ -75,7 +87,7 @@ func NewFromConfigMapData(src string, data map[string]string, clientBuilder Clie return nil, fmt.Errorf("%s did not provide any GPG public keys to verify signatures from and cannot be used", src) } - return NewReleaseVerifier(verifiers, stores, clientBuilder), nil + return NewReleaseVerifier(verifiers, ¶llel.Store{Stores: stores}), nil } func loadArmoredOrUnarmoredGPGKeyRing(data []byte) (openpgp.EntityList, error) { diff --git a/pkg/verify/verifyconfigmap/store.go b/pkg/verify/store/configmap/configmap.go similarity index 91% rename from pkg/verify/verifyconfigmap/store.go rename to pkg/verify/store/configmap/configmap.go index f14cba694a..947029f072 100644 --- a/pkg/verify/verifyconfigmap/store.go +++ b/pkg/verify/store/configmap/configmap.go @@ -1,4 +1,4 @@ -package verifyconfigmap +package configmap import ( "context" @@ -16,6 +16,8 @@ import ( corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/client-go/util/retry" "k8s.io/klog" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" ) // ReleaseLabelConfigMap is a label applied to a configmap inside the @@ -87,10 +89,10 @@ func digestToKeyPrefix(digest string) (string, error) { return fmt.Sprintf("%s-%s", algo, hash), nil } -// DigestSignatures returns a list of signatures that match the request +// Signatures returns a list of signatures that match the request // digest out of config maps labelled with ReleaseLabelConfigMap in the // openshift-config-managed namespace. -func (s *Store) DigestSignatures(ctx context.Context, digest string) ([][]byte, error) { +func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { // avoid repeatedly reloading config maps items := s.mostRecentConfigMaps() r := s.limiter.Reserve() @@ -100,7 +102,7 @@ func (s *Store) DigestSignatures(ctx context.Context, digest string) ([][]byte, }) if err != nil { s.rememberMostRecentConfigMaps([]corev1.ConfigMap{}) - return nil, err + return err } items = configMaps.Items s.rememberMostRecentConfigMaps(configMaps.Items) @@ -108,23 +110,28 @@ func (s *Store) DigestSignatures(ctx context.Context, digest string) ([][]byte, prefix, err := digestToKeyPrefix(digest) if err != nil { - return nil, err + return err } - var signatures [][]byte for _, cm := range items { klog.V(4).Infof("searching for %s in signature config map %s", prefix, cm.ObjectMeta.Name) for k, v := range cm.BinaryData { if strings.HasPrefix(k, prefix) { klog.V(4).Infof("key %s from signature config map %s matches %s", k, cm.ObjectMeta.Name, digest) - signatures = append(signatures, v) + done, err := fn(ctx, v, nil) + if err != nil || done { + return err + } + if err := ctx.Err(); err != nil { + return err + } } } } - return signatures, nil + return nil } -// Store attempts to persist the provided signatures into a form DigestSignatures will +// Store attempts to persist the provided signatures into a form Signatures will // retrieve. func (s *Store) Store(ctx context.Context, signaturesByDigest map[string][][]byte) error { cm := &corev1.ConfigMap{ diff --git a/pkg/verify/store/memory/memory.go b/pkg/verify/store/memory/memory.go new file mode 100644 index 0000000000..84b866e0f7 --- /dev/null +++ b/pkg/verify/store/memory/memory.go @@ -0,0 +1,36 @@ +// Package memory implements an in-memory signature store. This is +// mostly useful for testing. +package memory + +import ( + "context" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" +) + +// Store provides access to signatures stored in memory. +type Store struct { + // Data maps digests to slices of signatures. + Data map[string][][]byte +} + +// Signatures fetches signatures for the provided digest. +func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + for _, signature := range s.Data[digest] { + done, err := fn(ctx, signature, nil) + if err != nil || done { + return err + } + if err := ctx.Err(); err != nil { + return err + } + } + + return nil +} + +// String returns a description of where this store finds +// signatures. +func (s *Store) String() string { + return "in-memory signature store" +} diff --git a/pkg/verify/store/parallel/parallel.go b/pkg/verify/store/parallel/parallel.go new file mode 100644 index 0000000000..5a94f29826 --- /dev/null +++ b/pkg/verify/store/parallel/parallel.go @@ -0,0 +1,92 @@ +// Package parallel combines several signature stores in a single store. +// Signatures are searched in each substore simultaneously until a +// match is found. +package parallel + +import ( + "context" + "fmt" + "strings" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" +) + +type signatureResponse struct { + signature []byte + errIn error +} + +// Store provides access to signatures stored in sub-stores. +type Store struct { + Stores []store.Store +} + +// Signatures fetches signatures for the provided digest. +func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + nestedCtx, cancel := context.WithCancel(ctx) + defer cancel() + responses := make(chan signatureResponse, len(s.Stores)) + errorChannelCount := 0 + errorChannel := make(chan error, 1) + + for i := range s.Stores { + errorChannelCount++ + go func(ctx context.Context, wrappedStore store.Store, name string, digest string, responses chan signatureResponse, errorChannel chan error) { + 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 + case responses <- signatureResponse{signature: signature, errIn: errIn}: + } + return false, nil + }) + }(nestedCtx, s.Stores[i], name, digest, responses, errorChannel) + } + + allDone := false + var loopError error + for errorChannelCount > 0 { + if allDone { + err := <-errorChannel + errorChannelCount-- + if loopError == nil && err != nil && err != context.Canceled && err != context.DeadlineExceeded { + loopError = err + } + } else { + select { + case response := <-responses: + done, err := fn(ctx, response.signature, response.errIn) + if done || err != nil { + allDone = true + loopError = err + cancel() + } + case err := <-errorChannel: + errorChannelCount-- + if loopError == nil && err != nil && err != context.Canceled && err != context.DeadlineExceeded { + loopError = err + } + } + } + } + close(responses) + close(errorChannel) + if loopError != nil { + return loopError + } + return ctx.Err() // because we discard context errors from the wrapped stores +} + +// String returns a description of where this store finds +// signatures. +func (s *Store) String() string { + wrapped := "no stores" + if len(s.Stores) > 0 { + names := make([]string, 0, len(s.Stores)) + for _, store := range s.Stores { + names = append(names, store.String()) + } + wrapped = strings.Join(names, ", ") + } + return fmt.Sprintf("parallel signature store wrapping %s", wrapped) +} diff --git a/pkg/verify/store/parallel/parallel_test.go b/pkg/verify/store/parallel/parallel_test.go new file mode 100644 index 0000000000..e149f78908 --- /dev/null +++ b/pkg/verify/store/parallel/parallel_test.go @@ -0,0 +1,168 @@ +package parallel + +import ( + "context" + "errors" + "fmt" + "log" + "reflect" + "regexp" + "testing" + "time" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" + "github.com/openshift/cluster-version-operator/pkg/verify/store/memory" +) + +// delay wraps a store and introduces a delay before each callback. +type delay struct { + store store.Store + delay time.Duration +} + +// Signatures fetches signatures for the provided digest. +func (s *delay) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + nestedCtx, cancel := context.WithCancel(ctx) + defer cancel() + responses := make(chan *signatureResponse, 1) + go func(ctx context.Context, name string, digest string, responses chan *signatureResponse) { + err := s.store.Signatures(ctx, name, digest, func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { + select { + case <-ctx.Done(): + return true, nil + case responses <- &signatureResponse{signature: signature, errIn: errIn}: + log.Printf("queued response: %s", string(signature)) + } + return false, nil + }) + if err != nil && err != context.Canceled && err != context.DeadlineExceeded { + log.Fatal(err) + } + select { + case <-ctx.Done(): + case responses <- nil: + } + close(responses) + return + }(nestedCtx, name, digest, responses) + + for { + time.Sleep(s.delay) + select { + case <-ctx.Done(): + return ctx.Err() + case response := <-responses: + if response == nil { + return nil + } + log.Printf("sent response: %s", string(response.signature)) + done, err := fn(ctx, response.signature, response.errIn) + if done || err != nil { + return err + } + } + } + + return nil +} + +// String returns a description of where this store finds +// signatures. +func (s *delay) String() string { + return fmt.Sprintf("delay signature store wrapping %s with a delay of %s", s.store, s.delay) +} + +func TestStore(t *testing.T) { + ctx := context.Background() + parallel := &Store{ + Stores: []store.Store{ + &delay{ + store: &memory.Store{ + Data: map[string][][]byte{ + "sha256:123": { + []byte("store-1-sig-1"), + []byte("store-1-sig-2"), + }, + }, + }, + delay: 200 * time.Millisecond, + }, + &delay{ + store: &memory.Store{ + Data: map[string][][]byte{ + "sha256:123": { + []byte("store-2-sig-1"), + []byte("store-2-sig-2"), + }, + }, + }, + delay: 300 * time.Millisecond, + }, + }, + } + + for _, testCase := range []struct { + name string + doneSignature string + doneError error + expectedSignatures []string + expectedError *regexp.Regexp + }{ + { + name: "all", + expectedSignatures: []string{ + "store-1-sig-1", // 200 ms + "store-2-sig-1", // 300 ms + "store-1-sig-2", // 400 ms + "store-2-sig-2", // 600 ms + }, + }, + { + name: "done early", + doneSignature: "store-1-sig-2", + expectedSignatures: []string{ + "store-1-sig-1", + "store-2-sig-1", + "store-1-sig-2", + }, + }, + { + name: "error early", + doneSignature: "store-1-sig-2", + doneError: errors.New("test error"), + expectedSignatures: []string{ + "store-1-sig-1", + "store-2-sig-1", + "store-1-sig-2", + }, + expectedError: regexp.MustCompile("^test error$"), + }, + } { + 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 { + return false, errIn + } + signatures = append(signatures, string(signature)) + if string(signature) == testCase.doneSignature { + return true, testCase.doneError + } + return false, nil + }) + if err == nil { + if testCase.expectedError != nil { + t.Fatalf("signatures succeeded when we expected %s", testCase.expectedError) + } + } else if testCase.expectedError == nil { + t.Fatalf("signatures failed when we expected success: %v", err) + } else if !testCase.expectedError.MatchString(err.Error()) { + t.Fatalf("signatures failed with %v (expected %s)", err, testCase.expectedError) + } + + if !reflect.DeepEqual(signatures, testCase.expectedSignatures) { + t.Fatalf("signatures gathered %v when we expected %v", signatures, testCase.expectedSignatures) + } + }) + } +} diff --git a/pkg/verify/store/serial/serial.go b/pkg/verify/store/serial/serial.go new file mode 100644 index 0000000000..8c2107a087 --- /dev/null +++ b/pkg/verify/store/serial/serial.go @@ -0,0 +1,56 @@ +// Package serial combines several signature stores in a single store. +// Signatures are searched in each substore in turn until a match is +// found. +package serial + +import ( + "context" + "fmt" + "strings" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" +) + +// Store provides access to signatures stored in sub-stores. +type Store struct { + Stores []store.Store +} + +// Signatures fetches signatures for the provided digest. +func (s *Store) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + allDone := false + + wrapper := func(ctx context.Context, signature []byte, errIn error) (done bool, err error) { + done, err = fn(ctx, signature, errIn) + if done { + allDone = true + } + return done, err + } + + for _, store := range s.Stores { + err := store.Signatures(ctx, name, digest, wrapper) + if err != nil || allDone { + return err + } + if err := ctx.Err(); err != nil { + return err + } + } + + return nil +} + +// String returns a description of where this store finds +// signatures. +func (s *Store) String() string { + wrapped := "no stores" + if len(s.Stores) > 0 { + names := make([]string, 0, len(s.Stores)) + for _, store := range s.Stores { + names = append(names, store.String()) + } + wrapped = strings.Join(names, ", ") + } + return fmt.Sprintf("serial signature store wrapping %s", wrapped) +} diff --git a/pkg/verify/store/serial/serial_test.go b/pkg/verify/store/serial/serial_test.go new file mode 100644 index 0000000000..7830fad8c6 --- /dev/null +++ b/pkg/verify/store/serial/serial_test.go @@ -0,0 +1,101 @@ +package serial + +import ( + "context" + "errors" + "reflect" + "regexp" + "testing" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" + "github.com/openshift/cluster-version-operator/pkg/verify/store/memory" +) + +func TestStore(t *testing.T) { + ctx := context.Background() + serial := &Store{ + Stores: []store.Store{ + &memory.Store{ + Data: map[string][][]byte{ + "sha256:123": { + []byte("store-1-sig-1"), + []byte("store-1-sig-2"), + }, + }, + }, + &memory.Store{ + Data: map[string][][]byte{ + "sha256:123": { + []byte("store-2-sig-1"), + []byte("store-2-sig-2"), + }, + }, + }, + }, + } + + for _, testCase := range []struct { + name string + doneSignature string + doneError error + expectedSignatures []string + expectedError *regexp.Regexp + }{ + { + name: "all", + expectedSignatures: []string{ + "store-1-sig-1", + "store-1-sig-2", + "store-2-sig-1", + "store-2-sig-2", + }, + }, + { + name: "done early", + doneSignature: "store-2-sig-1", + expectedSignatures: []string{ + "store-1-sig-1", + "store-1-sig-2", + "store-2-sig-1", + }, + }, + { + name: "error early", + doneSignature: "store-2-sig-1", + doneError: errors.New("test error"), + expectedSignatures: []string{ + "store-1-sig-1", + "store-1-sig-2", + "store-2-sig-1", + }, + expectedError: regexp.MustCompile("^test error$"), + }, + } { + 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 { + return false, errIn + } + signatures = append(signatures, string(signature)) + if string(signature) == testCase.doneSignature { + return true, testCase.doneError + } + return false, nil + }) + if err == nil { + if testCase.expectedError != nil { + t.Fatalf("signatures succeeded when we expected %s", testCase.expectedError) + } + } else if testCase.expectedError == nil { + t.Fatalf("signatures failed when we expected success: %v", err) + } else if !testCase.expectedError.MatchString(err.Error()) { + t.Fatalf("signatures failed with %v (expected %s)", err, testCase.expectedError) + } + + if !reflect.DeepEqual(signatures, testCase.expectedSignatures) { + t.Fatalf("signatures gathered %v when we expected %v", signatures, testCase.expectedSignatures) + } + }) + } +} diff --git a/pkg/verify/store/store.go b/pkg/verify/store/store.go new file mode 100644 index 0000000000..00a19d2be4 --- /dev/null +++ b/pkg/verify/store/store.go @@ -0,0 +1,27 @@ +// Package store defines generic interfaces for signature stores. +package store + +import ( + "context" +) + +// Callback returns true if an acceptable signature has been found, or +// an error if the loop should be aborted. If there was a problem +// retrieving the signature, the incoming error will describe the +// 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) + +// 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. + Signatures(ctx context.Context, name string, digest string, fn Callback) error + + // String returns a description of where this store finds + // signatures. The string is a short clause intended for display in + // a description of the verifier. + String() string +} diff --git a/pkg/verify/verify.go b/pkg/verify/verify.go index 22348ce79a..60e17ba706 100644 --- a/pkg/verify/verify.go +++ b/pkg/verify/verify.go @@ -21,6 +21,8 @@ import ( "golang.org/x/crypto/openpgp" "k8s.io/klog" + + "github.com/openshift/cluster-version-operator/pkg/verify/store" ) // Interface performs verification of the provided content. The default implementation @@ -32,17 +34,6 @@ type Interface interface { Verify(ctx context.Context, releaseDigest string) error } -// SignatureStore retrieves signatures for a digest or returns an error. It requires String() -// for describing the store. -type SignatureStore interface { - // DigestSignatures returns zero or more signatures for the provided digest, or returns an - // error if no signatures could be retrieved. - DigestSignatures(ctx context.Context, digest string) ([][]byte, error) - // String should return a description of where this store finds signatures in a short - // clause intended for display in a description of the verifier. - String() string -} - type rejectVerifier struct{} func (rejectVerifier) Verify(ctx context.Context, releaseDigest string) error { @@ -52,6 +43,10 @@ func (rejectVerifier) Verify(ctx context.Context, releaseDigest string) error { // Reject fails always fails verification. var Reject Interface = rejectVerifier{} +// HTTPClient returns a client suitable for retrieving signatures. It is not +// required to be unique per call, but may be called concurrently. +type HTTPClient func() (*http.Client, error) + // ClientBuilder provides a method for generating an HTTP Client configured // with cluster proxy settings, if they exist. type ClientBuilder interface { @@ -84,37 +79,23 @@ var validReleaseDigest = regexp.MustCompile(`^[a-zA-Z0-9:]+$`) type ReleaseVerifier struct { verifiers map[string]openpgp.EntityList - locations []*url.URL - clientBuilder ClientBuilder - stores []SignatureStore + // Store is the store from which release signatures are retrieved. + Store store.Store lock sync.Mutex signatureCache map[string][][]byte } // NewReleaseVerifier creates a release verifier for the provided inputs. -func NewReleaseVerifier(verifiers map[string]openpgp.EntityList, locations []*url.URL, clientBuilder ClientBuilder) *ReleaseVerifier { +func NewReleaseVerifier(verifiers map[string]openpgp.EntityList, store store.Store) *ReleaseVerifier { return &ReleaseVerifier{ - verifiers: verifiers, - locations: locations, - clientBuilder: clientBuilder, + verifiers: verifiers, + Store: store, signatureCache: make(map[string][][]byte), } } -// WithStores copies the provided verifier and adds any provided stores to the list. -func (v *ReleaseVerifier) WithStores(stores ...SignatureStore) *ReleaseVerifier { - return &ReleaseVerifier{ - verifiers: v.verifiers, - locations: v.locations, - clientBuilder: v.clientBuilder, - - stores: append(append(make([]SignatureStore, 0, len(v.stores)+len(stores)), v.stores...), stores...), - signatureCache: v.Signatures(), - } -} - // Verifiers returns a copy of the verifiers in this payload. func (v *ReleaseVerifier) Verifiers() map[string]openpgp.EntityList { out := make(map[string]openpgp.EntityList, len(v.verifiers)) @@ -160,35 +141,11 @@ func (v *ReleaseVerifier) String() string { } fmt.Fprint(&builder, ")") } - - hasLocations := len(v.locations) > 0 - hasStores := len(v.stores) > 0 - if hasLocations || hasStores { - fmt.Fprintf(&builder, " - will check for signatures in containers/image format") - if hasLocations { - fmt.Fprintf(&builder, " at") - for i, location := range v.locations { - if i != 0 { - fmt.Fprint(&builder, ",") - } - fmt.Fprintf(&builder, " %s", location.String()) - } - } - if hasStores { - if hasLocations { - fmt.Fprintf(&builder, " and from") - } else { - fmt.Fprintf(&builder, " from") - } - for i, store := range v.stores { - if i != 0 { - fmt.Fprint(&builder, ",") - } - fmt.Fprintf(&builder, " %s", store.String()) - } - } + fmt.Fprintf(&builder, " - will check for signatures in containers/image format at") + if v.Store == nil { + fmt.Fprint(&builder, " ") } else { - fmt.Fprintf(&builder, " - ") + fmt.Fprintf(&builder, " %s", v.Store) } return builder.String() } @@ -197,7 +154,7 @@ func (v *ReleaseVerifier) String() string { // matching release digest in any of the provided locations for all verifiers, or returns // an error. func (v *ReleaseVerifier) Verify(ctx context.Context, releaseDigest string) error { - if len(v.verifiers) == 0 || (len(v.locations) == 0 && len(v.stores) == 0) { + if len(v.verifiers) == 0 || v.Store == nil { return fmt.Errorf("the release verifier is incorrectly configured, unable to verify digests") } if len(releaseDigest) == 0 { @@ -211,85 +168,35 @@ func (v *ReleaseVerifier) Verify(ctx context.Context, releaseDigest string) erro return nil } - parts := strings.SplitN(releaseDigest, ":", 3) - if len(parts) != 2 || len(parts[0]) == 0 || len(parts[1]) == 0 { - return fmt.Errorf("the provided release image digest must be of the form ALGO:HASH") - } - algo, hash := parts[0], parts[1] - name := fmt.Sprintf("%s=%s", algo, hash) - remaining := make(map[string]openpgp.EntityList, len(v.verifiers)) for k, v := range v.verifiers { remaining[k] = v } var signedWith [][]byte - verifier := func(path string, signature []byte) (bool, error) { + 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) + 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: %v", k, err) + klog.V(4).Infof("keyring %q could not verify signature for %s: %v", k, releaseDigest, err) continue } if err := verifyAtomicContainerSignature(content, releaseDigest); err != nil { - klog.V(4).Infof("signature %q is not valid: %v", path, err) + klog.V(4).Infof("signature for %s is not valid: %v", releaseDigest, err) continue } delete(remaining, k) signedWith = append(signedWith, signature) } - return len(remaining) > 0, nil - } - - // check the stores to see if they match any signatures - for i, store := range v.stores { - if len(remaining) == 0 { - break - } - signatures, err := store.DigestSignatures(ctx, releaseDigest) - if err != nil { - klog.V(4).Infof("store %s could not load signatures: %v", store, err) - continue - } - for _, signature := range signatures { - hasUnsigned, err := verifier(fmt.Sprintf("store %d", i), signature) - if err != nil { - return err - } - if !hasUnsigned { - break - } - } - } - - var client *http.Client - for _, location := range v.locations { - if len(remaining) == 0 { - break - } - switch location.Scheme { - case "file": - dir := filepath.Join(location.Path, name) - if err := checkFileSignatures(ctx, dir, maxSignatureSearch, verifier); err != nil { - return err - } - case "http", "https": - if client == nil { - var err error - client, err = v.clientBuilder.HTTPClient() - if err != nil { - return err - } - } - - copied := *location - copied.Path = path.Join(location.Path, name) - if err := checkHTTPSignatures(ctx, client, copied, maxSignatureSearch, verifier); err != nil { - return err - } - default: - return fmt.Errorf("internal error: the store %s type is unrecognized, cannot verify signatures", location) - } + return len(remaining) == 0, nil + }) + if err != nil { + klog.V(4).Infof("Failed to retrieve signatures for %s (should never happen)", releaseDigest) + return err } if len(remaining) > 0 { @@ -348,11 +255,20 @@ func (v *ReleaseVerifier) cacheVerification(releaseDigest string, signedWith [][ v.signatureCache[releaseDigest] = signedWith } -// checkFileSignatures reads signatures as "signature-1", "signature-2", etc out of a directory until -// either the provided fn returns an error, false, or no such file exists. No more than maxSignaturesToCheck -// will be read. -func checkFileSignatures(ctx context.Context, dir string, maxSignaturesToCheck int, fn func(path string, signature []byte) (bool, error)) error { - base := filepath.Join(dir, "signature-") +type fileStore struct { + directory string +} + +// Signatures reads signatures as "signature-1", "signature-2", etc. out of a digest-based subdirectory. +func (s *fileStore) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + parts := strings.SplitN(digest, ":", 3) + if len(parts) != 2 || len(parts[0]) == 0 || len(parts[1]) == 0 { + return fmt.Errorf("the provided release image digest must be of the form ALGO:HASH") + } + algo, hash := parts[0], parts[1] + digestPathSegment := fmt.Sprintf("%s=%s", algo, hash) + + base := filepath.Join(s.directory, digestPathSegment, "signature-") for i := 1; i < maxSignatureSearch; i++ { if err := ctx.Err(); err != nil { return err @@ -365,26 +281,67 @@ func checkFileSignatures(ctx context.Context, dir string, maxSignaturesToCheck i } if err != nil { klog.V(4).Infof("unable to load signature: %v", err) + done, err := fn(ctx, nil, err) + if done || err != nil { + return err + } continue } - ok, err := fn(path, data) - if err != nil { + done, err := fn(ctx, data, nil) + if done || err != nil { return err } - if !ok { - break - } } return nil } +func (s *fileStore) String() string { + return fmt.Sprintf("file://%s", s.directory) +} + var errNotFound = fmt.Errorf("no more signatures to check") -// checkHTTPSignatures reads signatures as "signature-1", "signature-2", etc as children of the provided URL +type httpStore struct { + uri *url.URL + httpClient HTTPClient +} + +// Signatures reads signatures as "signature-1", "signature-2", etc. as children of a digest URI // over HTTP or HTTPS until either the provided fn returns an error, false, or the server returns 404. No // more than maxSignaturesToCheck will be read. If the provided context is cancelled search will be terminated. -func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, maxSignaturesToCheck int, fn func(path string, signature []byte) (bool, error)) error { - base := filepath.Join(u.Path, "signature-") +func (s *httpStore) Signatures(ctx context.Context, name string, digest string, fn store.Callback) error { + parts := strings.SplitN(digest, ":", 3) + if len(parts) != 2 || len(parts[0]) == 0 || len(parts[1]) == 0 { + return fmt.Errorf("the provided release image digest must be of the form ALGO:HASH") + } + algo, hash := parts[0], parts[1] + digestPathSegment := fmt.Sprintf("%s=%s", algo, hash) + + switch s.uri.Scheme { + case "http", "https": + client, err := s.httpClient() + if err != nil { + _, err = fn(ctx, nil, err) + return err + } + + copied := *s.uri + copied.Path = path.Join(copied.Path, digestPathSegment) + if err := checkHTTPSignatures(ctx, client, copied, maxSignatureSearch, fn); err != nil { + return err + } + default: + return fmt.Errorf("the store %s scheme is unrecognized", s.uri) + } + + return nil +} + +// checkHTTPSignatures reads signatures as "signature-1", "signature-2", etc. as children of the provided URL +// over HTTP or HTTPS. No more than maxSignatureSearch will be read. If the provided context is cancelled +// search will be terminated. +func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, maxSignaturesToCheck int, fn store.Callback) error { + base := path.Join(u.Path, "signature-") sigURL := u for i := 1; i < maxSignatureSearch; i++ { if err := ctx.Err(); err != nil { @@ -395,13 +352,18 @@ func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, ma req, err := http.NewRequest("GET", sigURL.String(), nil) if err != nil { - return fmt.Errorf("could not build request to check signature: %v", err) + _, err = fn(ctx, nil, fmt.Errorf("could not build request to check signature: %v", 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) // load the body, being careful not to allow unbounded reads resp, err := client.Do(req) if err != nil { klog.V(4).Infof("unable to load signature: %v", err) + done, err := fn(ctx, nil, err) + if done || err != nil { + return err + } continue } data, err := func() ([]byte, error) { @@ -414,7 +376,7 @@ func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, ma body.Close() }() - if resp.StatusCode == 404 { + if resp.StatusCode == http.StatusNotFound { return nil, errNotFound } if resp.StatusCode < 200 || resp.StatusCode >= 300 { @@ -431,23 +393,30 @@ func checkHTTPSignatures(ctx context.Context, client *http.Client, u url.URL, ma } if err != nil { klog.V(4).Info(err) + done, err := fn(ctx, nil, err) + if done || err != nil { + return err + } continue } if len(data) == 0 { continue } - ok, err := fn(sigURL.String(), data) - if err != nil { + done, err := fn(ctx, data, nil) + if done || err != nil { return err } - if !ok { - break - } } return nil } +// String returns a description of where this store finds +// signatures. +func (s *httpStore) String() string { + return s.uri.String() +} + // verifySignatureWithKeyring performs a containers/image verification of the provided signature // message, checking for the integrity and authenticity of the provided message in r. It will return // the identity of the signer if successful along with the message contents. diff --git a/pkg/verify/verify_test.go b/pkg/verify/verify_test.go index 78bad7a6bd..71302e3ecd 100644 --- a/pkg/verify/verify_test.go +++ b/pkg/verify/verify_test.go @@ -10,27 +10,15 @@ import ( "net/url" "path/filepath" "reflect" + "strings" "testing" "golang.org/x/crypto/openpgp" -) - -type fakeSigStore struct { - digest string - signatures [][]byte - err error -} -func (s *fakeSigStore) DigestSignatures(ctx context.Context, digest string) ([][]byte, error) { - if len(s.digest) > 0 && s.digest != digest { - panic("unexpected digest") - } - return s.signatures, s.err -} - -func (s *fakeSigStore) String() string { - return "test sig store" -} + "github.com/openshift/cluster-version-operator/pkg/verify/store" + "github.com/openshift/cluster-version-operator/pkg/verify/store/memory" + "github.com/openshift/cluster-version-operator/pkg/verify/store/serial" +) func Test_ReleaseVerifier_Verify(t *testing.T) { data, err := ioutil.ReadFile(filepath.Join("testdata", "keyrings", "redhat.txt")) @@ -73,12 +61,18 @@ func Test_ReleaseVerifier_Verify(t *testing.T) { emptyServerURL, _ := url.Parse(emptyServer.URL) validSignatureData, err := ioutil.ReadFile(filepath.Join("testdata", "signatures", "sha256=e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", "signature-1")) + validSignatureStore := &memory.Store{ + Data: map[string][][]byte{ + "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7": { + validSignatureData, + }, + }, + } tests := []struct { name string verifiers map[string]openpgp.EntityList - locations []*url.URL - stores []SignatureStore + store store.Store releaseDigest string wantErr bool }{ @@ -88,127 +82,93 @@ func Test_ReleaseVerifier_Verify(t *testing.T) { { name: "valid signature for sha over file", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - locations: []*url.URL{ - {Scheme: "file", Path: "testdata/signatures"}, - }, - stores: []SignatureStore{ - &fakeSigStore{err: fmt.Errorf("logged only")}, + store: &serial.Store{ + Stores: []store.Store{ + &fileStore{directory: "testdata/signatures"}, + }, }, verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, }, { name: "valid signature for sha over http", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - locations: []*url.URL{ - sigServerURL, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + store: &httpStore{uri: sigServerURL, httpClient: DefaultClient.HTTPClient}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, }, { name: "valid signature for sha from store", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - stores: []SignatureStore{ - &fakeSigStore{}, - &fakeSigStore{err: fmt.Errorf("logged only")}, - &fakeSigStore{digest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", signatures: [][]byte{validSignatureData}}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + store: validSignatureStore, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, }, { name: "valid signature for sha over http with custom gpg key", releaseDigest: "sha256:edd9824f0404f1a139688017e7001370e2f3fbc088b94da84506653b473fe140", - locations: []*url.URL{ - sigServerURL, - }, - verifiers: map[string]openpgp.EntityList{"simple": simple}, + store: &httpStore{uri: sigServerURL, httpClient: DefaultClient.HTTPClient}, + verifiers: map[string]openpgp.EntityList{"simple": simple}, }, { name: "valid signature for sha over http with multi-key keyring", releaseDigest: "sha256:edd9824f0404f1a139688017e7001370e2f3fbc088b94da84506653b473fe140", - locations: []*url.URL{ - sigServerURL, - }, - verifiers: map[string]openpgp.EntityList{"combined": combined}, + store: &httpStore{uri: sigServerURL, httpClient: DefaultClient.HTTPClient}, + verifiers: map[string]openpgp.EntityList{"combined": combined}, }, { name: "store rejects if no store found", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - stores: []SignatureStore{ - &fakeSigStore{}, - &fakeSigStore{}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &memory.Store{}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "file location rejects if digest is not found", releaseDigest: "sha256:0000000000000000000000000000000000000000000000000000000000000000", - locations: []*url.URL{ - {Scheme: "file", Path: "testdata/signatures"}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &fileStore{directory: "testdata/signatures"}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "http location rejects if digest is not found", releaseDigest: "sha256:0000000000000000000000000000000000000000000000000000000000000000", - locations: []*url.URL{ - sigServerURL, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &httpStore{uri: sigServerURL, httpClient: DefaultClient.HTTPClient}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "sha contains invalid characters", releaseDigest: "!sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - locations: []*url.URL{ - {Scheme: "file", Path: "testdata/signatures"}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &fileStore{directory: "testdata/signatures"}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "sha contains too many separators", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7:", - locations: []*url.URL{ - {Scheme: "file", Path: "testdata/signatures"}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &fileStore{directory: "testdata/signatures"}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "could not find signature in file location", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - locations: []*url.URL{ - {Scheme: "file", Path: "testdata/signatures-2"}, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &fileStore{directory: "testdata/signatures-2"}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, { name: "could not find signature in http location", releaseDigest: "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", - locations: []*url.URL{ - emptyServerURL, - }, - verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, - wantErr: true, + store: &httpStore{uri: emptyServerURL, httpClient: DefaultClient.HTTPClient}, + verifiers: map[string]openpgp.EntityList{"redhat": redhatPublic}, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err != nil { - t.Fatal(err) - } - v := &ReleaseVerifier{ - verifiers: tt.verifiers, - stores: tt.stores, - locations: tt.locations, - clientBuilder: DefaultClient, - } + v := NewReleaseVerifier(tt.verifiers, tt.store) if err := v.Verify(context.Background(), tt.releaseDigest); (err != nil) != tt.wantErr { t.Errorf("ReleaseVerifier.Verify() error = %v, wantErr %v", err, tt.wantErr) } @@ -229,53 +189,34 @@ func Test_ReleaseVerifier_String(t *testing.T) { tests := []struct { name string verifiers map[string]openpgp.EntityList - locations []*url.URL - stores []SignatureStore + store store.Store want string }{ { - want: `All release image digests must have GPG signatures from - `, + name: "none", + want: "All release image digests must have GPG signatures from - will check for signatures in containers/image format at ", }, { - locations: []*url.URL{ - {Scheme: "http", Host: "localhost", Path: "test"}, - {Scheme: "file", Path: "/absolute/url"}, - }, - want: `All release image digests must have GPG signatures from - will check for signatures in containers/image format at http://localhost/test, file:///absolute/url`, - }, - { - verifiers: map[string]openpgp.EntityList{ - "redhat": redhatPublic, - }, - locations: []*url.URL{{Scheme: "http", Host: "localhost", Path: "test"}}, - want: `All release image digests must have GPG signatures from redhat (567E347AD0044ADE55BA8A5F199E2F91FD431D51: Red Hat, Inc. (release key 2) ) - will check for signatures in containers/image format at http://localhost/test`, + name: "HTTP store", + store: &httpStore{uri: &url.URL{Scheme: "http", Host: "localhost", Path: "test"}}, + want: `All release image digests must have GPG signatures from - will check for signatures in containers/image format at http://localhost/test`, }, { - verifiers: map[string]openpgp.EntityList{ - "redhat": redhatPublic, - }, - stores: []SignatureStore{&fakeSigStore{}, &fakeSigStore{}}, - want: `All release image digests must have GPG signatures from redhat (567E347AD0044ADE55BA8A5F199E2F91FD431D51: Red Hat, Inc. (release key 2) ) - will check for signatures in containers/image format from test sig store, test sig store`, + name: "file store", + store: &fileStore{directory: "absolute/path"}, + want: "All release image digests must have GPG signatures from - will check for signatures in containers/image format at file://absolute/path", }, { + name: "Red Hat verifier", verifiers: map[string]openpgp.EntityList{ "redhat": redhatPublic, }, - locations: []*url.URL{ - {Scheme: "http", Host: "localhost", Path: "test"}, - {Scheme: "file", Path: "/absolute/url"}, - }, - stores: []SignatureStore{&fakeSigStore{}, &fakeSigStore{}}, - want: `All release image digests must have GPG signatures from redhat (567E347AD0044ADE55BA8A5F199E2F91FD431D51: Red Hat, Inc. (release key 2) ) - will check for signatures in containers/image format at http://localhost/test, file:///absolute/url and from test sig store, test sig store`, + want: "All release image digests must have GPG signatures from redhat (567E347AD0044ADE55BA8A5F199E2F91FD431D51: Red Hat, Inc. (release key 2) ) - will check for signatures in containers/image format at ", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - v := &ReleaseVerifier{ - verifiers: tt.verifiers, - locations: tt.locations, - stores: tt.stores, - } + v := NewReleaseVerifier(tt.verifiers, tt.store) if got := v.String(); got != tt.want { t.Errorf("ReleaseVerifier.String() = %v, want %v", got, tt.want) } @@ -295,11 +236,17 @@ func Test_ReleaseVerifier_Signatures(t *testing.T) { const signedDigest = "sha256:e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7" + expectedSignature, err := ioutil.ReadFile(filepath.Join("testdata", "signatures", strings.Replace(signedDigest, ":", "=", 1), "signature-1")) + if err != nil { + t.Fatal(err) + } + goodStore := &memory.Store{Data: map[string][][]byte{}} + goodStore.Data[signedDigest] = [][]byte{expectedSignature} + // verify we don't cache a negative result verifier := NewReleaseVerifier( map[string]openpgp.EntityList{"redhat": redhatPublic}, - []*url.URL{{Scheme: "file", Path: "testdata/signatures-wrong"}}, - DefaultClient, + &memory.Store{}, ) if err := verifier.Verify(context.Background(), signedDigest); err == nil || err.Error() != "unable to locate a valid signature for one or more sources" { t.Fatal(err) @@ -311,8 +258,7 @@ func Test_ReleaseVerifier_Signatures(t *testing.T) { // verify we cache a valid request verifier = NewReleaseVerifier( map[string]openpgp.EntityList{"redhat": redhatPublic}, - []*url.URL{{Scheme: "file", Path: "testdata/signatures"}}, - DefaultClient, + goodStore, ) if err := verifier.Verify(context.Background(), signedDigest); err != nil { t.Fatal(err) @@ -321,8 +267,8 @@ func Test_ReleaseVerifier_Signatures(t *testing.T) { t.Fatalf("%#v", sigs) } - // verify we hit the cache instead of verifying, even after changing the locations directory - verifier.locations = []*url.URL{{Scheme: "file", Path: "testdata/signatures-wrong"}} + // verify we hit the cache instead of verifying, even after changing the store + verifier.Store = &memory.Store{} if err := verifier.Verify(context.Background(), signedDigest); err != nil { t.Fatal(err) } @@ -331,15 +277,9 @@ func Test_ReleaseVerifier_Signatures(t *testing.T) { } // verify we maintain a maximum number of cache entries a valid request - expectedSignature, err := ioutil.ReadFile(filepath.Join("testdata", "signatures", "sha256=e3f12513a4b22a2d7c0e7c9207f52128113758d9d68c7d06b11a0ac7672966f7", "signature-1")) - if err != nil { - t.Fatal(err) - } - verifier = NewReleaseVerifier( map[string]openpgp.EntityList{"redhat": redhatPublic}, - []*url.URL{{Scheme: "file", Path: "testdata/signatures"}}, - DefaultClient, + goodStore, ) for i := 0; i < maxSignatureCacheSize*2; i++ { verifier.signatureCache[fmt.Sprintf("test-%d", i)] = [][]byte{[]byte("blah")}