From 4bcac5e133660d5c379d51a6e2f7e2ad8455b6c8 Mon Sep 17 00:00:00 2001 From: Mauren Berti Date: Thu, 25 May 2023 15:09:41 -0400 Subject: [PATCH] Add test coverage for new builder signing feature. --- cmd/controller/main.go | 5 + pkg/cnb/create_builder.go | 27 ++-- pkg/cnb/create_builder_test.go | 67 ++++++++++ pkg/cosign/image_signer.go | 17 ++- pkg/cosign/image_signer_test.go | 228 ++++++++++++++++++++++++++++---- 5 files changed, 302 insertions(+), 42 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e340688ed..7a3c1f42b 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -10,6 +10,10 @@ import ( "strconv" "time" + "github.com/pivotal/kpack/pkg/cosign" + "github.com/sigstore/cosign/v2/cmd/cosign/cli/sign" + ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote" + "github.com/Masterminds/semver/v3" "go.uber.org/zap" "golang.org/x/sync/errgroup" @@ -200,6 +204,7 @@ func main() { KpackVersion: cmd.Identifer, LifecycleProvider: lifecycleProvider, KeychainFactory: keychainFactory, + ImageSigner: cosign.NewImageSigner(log.New(os.Stdout, "", 0), sign.SignCmd, ociremote.SignatureTag), } buildController := build.NewController(ctx, options, k8sClient, buildInformer, podInformer, metadataRetriever, buildpodGenerator, keychainFactory, *injectedSidecarSupport) diff --git a/pkg/cnb/create_builder.go b/pkg/cnb/create_builder.go index 1ff6ee2b6..5ed1c67b4 100644 --- a/pkg/cnb/create_builder.go +++ b/pkg/cnb/create_builder.go @@ -2,15 +2,11 @@ package cnb import ( "context" - "github.com/pivotal/kpack/pkg/cosign" - "github.com/sigstore/cosign/v2/cmd/cosign/cli/sign" - ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote" - corev1 "k8s.io/api/core/v1" - "log" - "os" "github.com/google/go-containerregistry/pkg/authn" ggcrv1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/pivotal/kpack/pkg/cosign" + corev1 "k8s.io/api/core/v1" buildapi "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1" @@ -31,11 +27,11 @@ type RemoteBuilderCreator struct { LifecycleProvider LifecycleProvider KpackVersion string KeychainFactory registry.KeychainFactory + ImageSigner cosign.BuilderSigner } func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychain authn.Keychain, stackKeychain authn.Keychain, fetcher RemoteBuildpackFetcher, clusterStack *buildapi.ClusterStack, spec buildapi.BuilderSpec, signingSecrets []corev1.Secret) (buildapi.BuilderRecord, error) { buildImage, _, err := r.RegistryClient.Fetch(stackKeychain, clusterStack.Status.BuildImage.LatestImage) - cosignSigner := cosign.NewImageSigner(log.New(os.Stdout, "", 0), sign.SignCmd, ociremote.SignatureTag) if err != nil { return buildapi.BuilderRecord{}, err @@ -84,9 +80,18 @@ func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychai return buildapi.BuilderRecord{}, err } - signaturePaths, err := cosignSigner.SignBuilder(ctx, identifier, signingSecrets, builderKeychain) - if err != nil { - return buildapi.BuilderRecord{}, err + var ( + signaturePaths = make([]buildapi.SignaturePath, 0) + signed = false + ) + + if len(signingSecrets) > 0 { + signaturePaths, err = r.ImageSigner.SignBuilder(ctx, identifier, signingSecrets, builderKeychain) + if err != nil { + return buildapi.BuilderRecord{}, err + } + + signed = true } builder := buildapi.BuilderRecord{ @@ -100,7 +105,7 @@ func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychai ObservedStackGeneration: clusterStack.Status.ObservedGeneration, ObservedStoreGeneration: fetcher.ClusterStoreObservedGeneration(), OS: config.OS, - Signed: err == nil, + Signed: signed, SignaturePaths: signaturePaths, } diff --git a/pkg/cnb/create_builder_test.go b/pkg/cnb/create_builder_test.go index 25aa4baef..1c7d7b4ef 100644 --- a/pkg/cnb/create_builder_test.go +++ b/pkg/cnb/create_builder_test.go @@ -177,6 +177,12 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) { KpackVersion: "v1.2.3 (git sha: abcdefg123456)", KeychainFactory: keychainFactory, LifecycleProvider: lifecycleProvider, + ImageSigner: &fakeBuilderSigner{ + signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.SignaturePath, error) { + // no-op + return nil, nil + }, + }, } addBuildpack = func(t *testing.T, id, version, homepage, api string, stacks []corev1alpha1.BuildpackStack) { @@ -862,9 +868,70 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) { require.EqualError(t, err, "unsupported platform apis in kpack lifecycle: 0.1, 0.2, 0.999, expecting one of: 0.3, 0.4, 0.5, 0.6, 0.7, 0.8") }) }) + + when("signing a builder image", func() { + it("does not return the signed flag set when no secrets were present", func() { + builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{}) + require.NoError(t, err) + require.NotNil(t, builderRecord) + + require.False(t, builderRecord.Signed) + }) + + it("returns an error if signing fails", func() { + subject.ImageSigner = &fakeBuilderSigner{ + signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.SignaturePath, error) { + return nil, fmt.Errorf("failed to sign builder") + }, + } + + fakeSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-creds", + Namespace: "test-namespace", + }, + } + + _, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{fakeSecret}) + require.Error(t, err) + }) + + it("sets the signed flag when signing succeeds", func() { + fakeSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-creds", + Namespace: "test-namespace", + }, + } + + subject.ImageSigner = &fakeBuilderSigner{ + signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.SignaturePath, error) { + return []buildapi.SignaturePath{ + { + KeyName: fmt.Sprintf("k8s://%s/%s", fakeSecret.Namespace, fakeSecret.Name), + Path: "registry.local/test-image:signature-tag", + }, + }, nil + }, + } + + builderRecord, err := subject.CreateBuilder(ctx, builderKeychain, stackKeychain, fetcher, stack, clusterBuilderSpec, []corev1.Secret{fakeSecret}) + require.NoError(t, err) + require.NotNil(t, builderRecord) + require.True(t, builderRecord.Signed) + }) + }) }) } +type fakeBuilderSigner struct { + signBuilder func(context.Context, string, []corev1.Secret, authn.Keychain) ([]buildapi.SignaturePath, error) +} + +func (s *fakeBuilderSigner) SignBuilder(ctx context.Context, imageReference string, signingSecrets []corev1.Secret, builderKeychain authn.Keychain) ([]buildapi.SignaturePath, error) { + return s.signBuilder(ctx, imageReference, signingSecrets, builderKeychain) +} + type fakeLifecycleProvider struct { metadata LifecycleMetadata layers map[string]v1.Layer diff --git a/pkg/cosign/image_signer.go b/pkg/cosign/image_signer.go index 61f47a437..91a740190 100644 --- a/pkg/cosign/image_signer.go +++ b/pkg/cosign/image_signer.go @@ -3,18 +3,19 @@ package cosign import ( "context" "fmt" + "io/ioutil" + "log" + "os" + "time" + "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" cosignremote "github.com/sigstore/cosign/v2/pkg/oci/remote" - "io/ioutil" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sclient "k8s.io/client-go/kubernetes" - "log" - "os" - "time" "github.com/buildpacks/lifecycle/platform" "github.com/pkg/errors" @@ -25,6 +26,10 @@ type SignFunc func(*options.RootOptions, options.KeyOpts, options.SignOptions, [ type FetchSignatureFunc func(name.Reference, ...cosignremote.Option) (name.Tag, error) +type BuilderSigner interface { + SignBuilder(context.Context, string, []corev1.Secret, authn.Keychain) ([]v1alpha2.SignaturePath, error) +} + type ImageSigner struct { Logger *log.Logger signFunc SignFunc @@ -153,13 +158,13 @@ func (s *ImageSigner) SignBuilder( } if cosignRepository, ok := secret.Annotations[RepositoryAnnotationPrefix]; ok { - if err := os.Setenv(cosignRepositoryEnv, fmt.Sprintf("%s", cosignRepository)); err != nil { + if err := os.Setenv(cosignRepositoryEnv, cosignRepository); err != nil { return nil, fmt.Errorf("failed setting %s env variable: %w", cosignRepositoryEnv, err) } } if cosignDockerMediaType, ok := secret.Annotations[DockerMediaTypesAnnotationPrefix]; ok { - if err := os.Setenv(cosignDockerMediaTypesEnv, fmt.Sprintf("%s", cosignDockerMediaType)); err != nil { + if err := os.Setenv(cosignDockerMediaTypesEnv, cosignDockerMediaType); err != nil { return nil, fmt.Errorf("failed setting %s env variable: %w", cosignDockerMediaTypesEnv, err) } } diff --git a/pkg/cosign/image_signer_test.go b/pkg/cosign/image_signer_test.go index 9fc3c3720..2f7b17918 100644 --- a/pkg/cosign/image_signer_test.go +++ b/pkg/cosign/image_signer_test.go @@ -4,8 +4,8 @@ import ( "bufio" "context" "crypto" + "encoding/base64" "fmt" - "github.com/google/go-containerregistry/pkg/v1/remote" "io/ioutil" "log" "net/http/httptest" @@ -21,7 +21,11 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/registry" + v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/random" + "github.com/google/go-containerregistry/pkg/v1/remote" + registry2 "github.com/pivotal/kpack/pkg/registry" + "github.com/pivotal/kpack/pkg/registry/registryfakes" "github.com/sclevine/spec" "github.com/sigstore/cosign/v2/cmd/cosign/cli/download" "github.com/sigstore/cosign/v2/cmd/cosign/cli/options" @@ -32,6 +36,8 @@ import ( "github.com/sigstore/cosign/v2/pkg/signature" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var fetchSignatureFunc = func(_ name.Reference, options ...ociremote.Option) (name.Tag, error) { @@ -49,19 +55,19 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) { report platform.ExportReport reader *os.File writer *os.File - expectedImageName string stopRegistry func() imageCleanup func() repo string + expectedImageName string ) it.Before(func() { _, reader, writer = mockLogger(t) - repo, stopRegistry = reg(t) + repo, stopRegistry = fakeRegistry(t) expectedImageName = path.Join(repo, "test-cosign-image") - imageCleanup = pushRandomImage(t, expectedImageName) + _, imageCleanup = pushRandomImage(t, expectedImageName) }) it.After(func() { @@ -85,11 +91,11 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) { // Override secretLocation for test secretLocation = createCosignKeyFiles(t) - secretKey1 = path.Join(secretLocation, "secret-name-1", "cosign.key") - publicKey1 = path.Join(secretLocation, "secret-name-1", "cosign.pub") - publicKey2 = path.Join(secretLocation, "secret-name-2", "cosign.pub") - passwordFile1 = path.Join(secretLocation, "secret-name-1", "cosign.password") - passwordFile2 = path.Join(secretLocation, "secret-name-2", "cosign.password") + secretKey1 = path.Join(secretLocation, "secret-name-1", SecretDataCosignKey) + publicKey1 = path.Join(secretLocation, "secret-name-1", SecretDataCosignPublicKey) + publicKey2 = path.Join(secretLocation, "secret-name-2", SecretDataCosignPublicKey) + passwordFile1 = path.Join(secretLocation, "secret-name-1", SecretDataCosignPassword) + passwordFile2 = path.Join(secretLocation, "secret-name-2", SecretDataCosignPassword) report = createReportToml(t, expectedImageName) @@ -261,7 +267,7 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) { }) it("sets COSIGN_REPOSITORY environment variable", func() { - altRepo, altStopRegistry := reg(t) + altRepo, altStopRegistry := fakeRegistry(t) defer altStopRegistry() altImageName := path.Join(altRepo, "test-cosign-image-alt") @@ -442,18 +448,18 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) { it("signs an image", func() { secretLocation := t.TempDir() - repo, stop := reg(t) + repo, stop := fakeRegistry(t) defer stop() imgName := path.Join(repo, "cosign-e2e") - cleanup := pushRandomImage(t, imgName) + _, cleanup := pushRandomImage(t, imgName) defer cleanup() password := "" keypair(t, secretLocation, "secret-name-1", password) - privKeyPath := path.Join(secretLocation, "secret-name-1", "cosign.key") - pubKeyPath := path.Join(secretLocation, "secret-name-1", "cosign.pub") + privKeyPath := path.Join(secretLocation, "secret-name-1", SecretDataCosignKey) + pubKeyPath := path.Join(secretLocation, "secret-name-1", SecretDataCosignPublicKey) ctx := context.Background() // Verify+download should fail at first @@ -490,10 +496,182 @@ func testImageSigner(t *testing.T, when spec.G, it spec.S) { }) when("#SignBuilder", func() { + it("resolves the digest of a signature correctly", func() { + var ( + signCallCount = 0 + fetchSignatureCallCount = 0 + ) + + fakeImageSignatureTag := fmt.Sprintf("%s:%s", expectedImageName, "test.sig") + digest, cleanup := pushRandomImage(t, fakeImageSignatureTag) + defer cleanup() + + fakeImageSigner := &ImageSigner{ + signFunc: func(rootOptions *options.RootOptions, opts options.KeyOpts, signOptions options.SignOptions, i []string) error { + t.Helper() + signCallCount++ + return nil + }, + fetchSignatureFunc: func(reference name.Reference, option ...ociremote.Option) (name.Tag, error) { + t.Helper() + + fetchSignatureCallCount++ + return name.NewTag(fakeImageSignatureTag) + }, + } + + fakeSecret := generateFakeKeyPair(t, "", nil) + cosignCreds := []corev1.Secret{fakeSecret} + cosignSA := corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-sa", + Namespace: "test-namespace", + }, + Secrets: []corev1.ObjectReference{ + { + Name: fakeSecret.Name, + }, + }, + } + + secretRef := registry2.SecretRef{ + ServiceAccount: cosignSA.Name, + Namespace: cosignSA.Namespace, + } + + keychainFactory := ®istryfakes.FakeKeychainFactory{} + fakeKeychain := ®istryfakes.FakeKeychain{} + keychainFactory.AddKeychainForSecretRef(t, secretRef, fakeKeychain) + + signaturePaths, err := fakeImageSigner.SignBuilder(context.Background(), expectedImageName, cosignCreds, fakeKeychain) + require.NoError(t, err) + require.NotEmpty(t, signaturePaths) + require.NotNil(t, signaturePaths[0]) + + assert.Contains(t, signaturePaths[0].Path, digest.String()) + assert.Contains(t, signaturePaths[0].KeyName, fakeSecret.Namespace) + assert.Contains(t, signaturePaths[0].KeyName, fakeSecret.Name) + + require.Equal(t, 1, signCallCount) + require.Equal(t, 1, fetchSignatureCallCount) + }) + + it("sets environment variables when needed", func() { + var ( + signCallCount = 0 + fetchSignatureCallCount = 0 + signaturesPath = path.Join(repo, "signatures") + dockerMediaTypesValue = "1" + ) + + fakeImageSignatureTag := fmt.Sprintf("%s:%s", signaturesPath, "test.sig") + digest, cleanup := pushRandomImage(t, fakeImageSignatureTag) + defer cleanup() + + fakeImageSigner := &ImageSigner{ + signFunc: func(rootOptions *options.RootOptions, opts options.KeyOpts, signOptions options.SignOptions, i []string) error { + t.Helper() + + value, found := os.LookupEnv(cosignRepositoryEnv) + require.True(t, found) + require.NotNil(t, value) + assert.Equal(t, signaturesPath, value) + + value, found = os.LookupEnv(cosignDockerMediaTypesEnv) + require.True(t, found) + require.NotNil(t, value) + assert.Equal(t, dockerMediaTypesValue, value) + + signCallCount++ + return nil + }, + fetchSignatureFunc: func(reference name.Reference, option ...ociremote.Option) (name.Tag, error) { + t.Helper() + + fetchSignatureCallCount++ + return name.NewTag(fakeImageSignatureTag) + }, + } + + annotations := map[string]string{ + "kpack.io/cosign.repository": signaturesPath, + "kpack.io/cosign.docker-media-types": dockerMediaTypesValue, + } + + fakeSecret := generateFakeKeyPair(t, "", annotations) + cosignCreds := []corev1.Secret{fakeSecret} + cosignSA := corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-sa", + Namespace: "test-namespace", + }, + Secrets: []corev1.ObjectReference{ + { + Name: fakeSecret.Name, + }, + }, + } + + secretRef := registry2.SecretRef{ + ServiceAccount: cosignSA.Name, + Namespace: cosignSA.Namespace, + } + + keychainFactory := ®istryfakes.FakeKeychainFactory{} + fakeKeychain := ®istryfakes.FakeKeychain{} + keychainFactory.AddKeychainForSecretRef(t, secretRef, fakeKeychain) + + signaturePaths, err := fakeImageSigner.SignBuilder(context.Background(), expectedImageName, cosignCreds, fakeKeychain) + require.NoError(t, err) + require.NotEmpty(t, signaturePaths) + require.NotNil(t, signaturePaths[0]) + + assert.Contains(t, signaturePaths[0].Path, digest.String()) + assert.Contains(t, signaturePaths[0].KeyName, fakeSecret.Namespace) + assert.Contains(t, signaturePaths[0].KeyName, fakeSecret.Name) + + require.Equal(t, 1, signCallCount) + require.Equal(t, 1, fetchSignatureCallCount) + }) }) } +func generateFakeKeyPair(t *testing.T, password string, annotations map[string]string) corev1.Secret { + passFunc := func(_ bool) ([]byte, error) { + return []byte(password), nil + } + + keys, err := sigstoreCosign.GenerateKeyPair(passFunc) + require.NoError(t, err) + + encodedPublicKey := make([]byte, base64.StdEncoding.EncodedLen(len(keys.PublicBytes))) + base64.StdEncoding.Encode(encodedPublicKey, keys.PublicBytes) + + encodedPrivateKey := make([]byte, base64.StdEncoding.EncodedLen(len(keys.PrivateBytes))) + base64.StdEncoding.Encode(encodedPrivateKey, keys.PrivateBytes) + + encodedPassword := make([]byte, base64.StdEncoding.EncodedLen(len([]byte(password)))) + base64.StdEncoding.Encode(encodedPassword, []byte(password)) + + data := map[string][]byte{ + SecretDataCosignPublicKey: encodedPublicKey, + SecretDataCosignKey: encodedPrivateKey, + SecretDataCosignPassword: encodedPassword, + } + + secret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cosign-creds", + Namespace: "test-namespace", + Annotations: annotations, + }, + Data: data, + } + + return secret +} + func mockLogger(t *testing.T) (*bufio.Scanner, *os.File, *os.File) { reader, writer, err := os.Pipe() if err != nil { @@ -545,7 +723,7 @@ func assertUnset(t *testing.T, envName string, msg ...string) { assert.Equal(t, "", value) } -func reg(t *testing.T) (string, func()) { +func fakeRegistry(t *testing.T) (string, func()) { r := httptest.NewServer(registry.New()) u, err := url.Parse(r.URL) assert.Nil(t, err) @@ -553,20 +731,20 @@ func reg(t *testing.T) (string, func()) { return u.Host, r.Close } -func pushRandomImage(t *testing.T, imageRef string) func() { +func pushRandomImage(t *testing.T, imageRef string) (v1.Hash, func()) { ref, err := name.ParseReference(imageRef, name.WeakValidation) - assert.Nil(t, err) + require.NoError(t, err) img, err := random.Image(512, 5) - assert.Nil(t, err) + require.NoError(t, err) regClientOpts := registryClientOpts(context.Background()) err = remote.Write(ref, img, regClientOpts...) - assert.Nil(t, err) + require.NoError(t, err) - _, err = remote.Get(ref, regClientOpts...) - assert.Nil(t, err) + resp, err := remote.Get(ref, regClientOpts...) + require.NoError(t, err) cleanup := func() { _ = remote.Delete(ref, regClientOpts...) @@ -574,7 +752,7 @@ func pushRandomImage(t *testing.T, imageRef string) func() { _ = remote.Delete(ref, regClientOpts...) } - return cleanup + return resp.Digest, cleanup } func registryClientOpts(ctx context.Context) []remote.Option { @@ -595,15 +773,15 @@ func keypair(t *testing.T, dirPath, secretName, password string) { err = os.Mkdir(filepath.Join(dirPath, secretName), 0700) assert.Nil(t, err) - privKeyPath := filepath.Join(dirPath, secretName, "cosign.key") + privKeyPath := filepath.Join(dirPath, secretName, SecretDataCosignKey) err = ioutil.WriteFile(privKeyPath, keys.PrivateBytes, 0600) assert.Nil(t, err) - pubKeyPath := filepath.Join(dirPath, secretName, "cosign.pub") + pubKeyPath := filepath.Join(dirPath, secretName, SecretDataCosignPublicKey) err = ioutil.WriteFile(pubKeyPath, keys.PublicBytes, 0600) assert.Nil(t, err) - passwordPath := filepath.Join(dirPath, secretName, "cosign.password") + passwordPath := filepath.Join(dirPath, secretName, SecretDataCosignPassword) passwordBytes, _ := passFunc(true) err = ioutil.WriteFile(passwordPath, passwordBytes, 0600) assert.Nil(t, err)