DO NOT MERGE - Review comments, will be squashed

Author: smarterclayton

pkg/cvo/cvo.go

@@ -305,7 +305,7 @@ func loadConfigMapVerifierDataFromUpdate(update *payload.Update, clientBuilder v
 		if err != nil {
 			return nil, nil, errors.Wrapf(err, "%s is not valid: %v", src, err)
 		}
-		verifier, err := verify.NewFromConfigMap(src, data, clientBuilder)
+		verifier, err := verify.NewFromConfigMapData(src, data, clientBuilder)
 		if err != nil {
 			return nil, nil, err
 		}

pkg/verify/configmap.go

@@ -14,10 +14,10 @@ import (
 // ReleaseAnnotationConfigMapVerifier is an annotation set on a config map in the
 // release payload to indicate that this config map controls signing for the payload.
 // Only the first config map within the payload should be used, regardless of whether
-// it has data. See NewFromConfigMap for more.
+// it has data. See NewFromConfigMapData for more.
 const ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-config-map"
 
-// NewFromConfigMap expects to receive the data field of the first config map in the release
+// NewFromConfigMapData expects to receive the data field of the first config map in the release
 // image payload with the annotation "release.openshift.io/verification-config-map". Only the
 // first payload item in lexographic order will be considered - all others are ignored. The
 // verifier returned by this method
@@ -46,7 +46,7 @@ const ReleaseAnnotationConfigMapVerifier = "release.openshift.io/verification-co
 // The returned verifier will require that any new release image will only be considered verified
 // if each provided public key has signed the release image digest. The signature may be in any
 // store and the lookup order is internally defined.
-func NewFromConfigMap(src string, data map[string]string, clientBuilder ClientBuilder) (*ReleaseVerifier, error) {
+func NewFromConfigMapData(src string, data map[string]string, clientBuilder ClientBuilder) (*ReleaseVerifier, error) {
 	verifiers := make(map[string]openpgp.EntityList)
 	var stores []*url.URL
 	for k, v := range data {

pkg/verify/configmap_test.go

@@ -56,7 +56,7 @@ func Test_loadReleaseVerifierFromConfigMap(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := NewFromConfigMap("from_test", tt.data, DefaultClient)
+			got, err := NewFromConfigMapData("from_test", tt.data, DefaultClient)
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("loadReleaseVerifierFromPayload() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/verify/verify.go

@@ -27,6 +27,8 @@ import (
 // in this package uses the container signature format defined at https://github.com/containers/image
 // to authenticate that a given release image digest has been signed by a trusted party.
 type Interface interface {
+	// Verify should return nil if the provided release digest has suffient signatures to be considered
+	// valid. It should return an error in all other cases.
 	Verify(ctx context.Context, releaseDigest string) error
 }
 
@@ -50,8 +52,30 @@ func (rejectVerifier) Verify(ctx context.Context, releaseDigest string) error {
 // Reject fails always fails verification.
 var Reject Interface = rejectVerifier{}
 
+// ClientBuilder provides a method for generating an HTTP Client configured
+// with cluster proxy settings, if they exist.
+type ClientBuilder interface {
+	// HTTPClient returns a client suitable for retrieving signatures. It is not
+	// required to be unique per call, but may be called concurrently.
+	HTTPClient() (*http.Client, error)
+}
+
+// DefaultClient uses the default http.Client for accessing signatures.
+var DefaultClient = simpleClientBuilder{}
+
+// simpleClientBuilder implements the ClientBuilder interface and may be used for testing.
+type simpleClientBuilder struct{}
+
+// HTTPClient from simpleClientBuilder creates an http.Client with no configuration.
+func (s simpleClientBuilder) HTTPClient() (*http.Client, error) {
+	return &http.Client{}, nil
+}
+
+// maxSignatureSearch prevents unbounded recursion on malicious signature stores (if
+// an attacker was able to take ownership of the store to perform DoS on clusters).
 const maxSignatureSearch = 10
 
+// validReleaseDigest is a verification rule to filter clearly invalid digests.
 var validReleaseDigest = regexp.MustCompile(`^[a-zA-Z0-9:]+$`)
 
 // ReleaseVerifier implements a signature intersection operation on a provided release
@@ -89,26 +113,9 @@ func (v *ReleaseVerifier) WithStores(stores ...SignatureStore) *ReleaseVerifier
 	}
 }
 
-// ClientBuilder provides a method for generating an HTTP Client configured
-// with cluster proxy settings, if they exist.
-type ClientBuilder interface {
-	HTTPClient() (*http.Client, error)
-}
-
-// DefaultClient uses the default http.Client for accessing signatures.
-var DefaultClient = simpleClientBuilder{}
-
-// simpleClientBuilder implements the ClientBuilder interface and may be used for testing.
-type simpleClientBuilder struct{}
-
-// HTTPClient from simpleClientBuilder creates an httpClient with no configuration.
-func (s simpleClientBuilder) HTTPClient() (*http.Client, error) {
-	return &http.Client{}, nil
-}
-
 // Verifiers returns a copy of the verifiers in this payload.
 func (v *ReleaseVerifier) Verifiers() map[string]openpgp.EntityList {
-	out := make(map[string]openpgp.EntityList)
+	out := make(map[string]openpgp.EntityList, len(v.verifiers))
 	for k, v := range v.verifiers {
 		out[k] = v
 	}

pkg/verify/verify_test.go

@@ -348,7 +348,7 @@ func Test_ReleaseVerifier_Signatures(t *testing.T) {
 	if err := verifier.Verify(context.Background(), signedDigest); err != nil {
 		t.Fatal(err)
 	}
-	if sigs := verifier.Signatures(); len(sigs) != 64 || !reflect.DeepEqual(sigs[signedDigest], [][]byte{expectedSignature}) {
+	if sigs := verifier.Signatures(); len(sigs) != maxSignatureCacheSize || !reflect.DeepEqual(sigs[signedDigest], [][]byte{expectedSignature}) {
 		t.Fatalf("%d %#v", len(sigs), sigs)
 	}
 }