Skip to content

Commit

Permalink
Changes from code review.
Browse files Browse the repository at this point in the history 
* Move test_util.go into pkg/cosign/testing to make the intent clear.
* Create new file constants.go in pkg/cosign/util to prevent import cycle in test.
* Extract the end-to-end cosign tests into the new test/cosign_e2e_test.go file.
* goimports all changed files.
* Remove the signed indicator from the BuilderStatus object.
* Rename SignaturePath struct to CosignSignature, along with the fields.
* Remove capacity from the signature paths array.
  • Loading branch information
@stormqueen1990
stormqueen1990 committed Jul 3, 2023
1 parent 32d6e0f commit 588b5e7
Show file tree
Hide file tree
Showing 15 changed files with 760 additions and 748 deletions.
3 changes: 2 additions & 1 deletion cmd/completion/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ import (
"context"
"flag"
"fmt"
"github.com/sigstore/cosign/v2/pkg/oci/remote"
"io/ioutil"
"log"
"os"
"path/filepath"
"strings"

"github.com/sigstore/cosign/v2/pkg/oci/remote"

"github.com/BurntSushi/toml"
"github.com/buildpacks/lifecycle/platform"
"github.com/google/go-containerregistry/pkg/authn"
Expand Down
4 changes: 1 addition & 3 deletions pkg/apis/build/v1alpha2/builder_lifecycle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ type BuilderRecord struct {
ObservedStoreGeneration int64
ObservedStackGeneration int64
OS string
Signed bool
SignaturePaths []SignaturePath
SignaturePaths []CosignSignature
}

func (bs *BuilderStatus) BuilderRecord(record BuilderRecord) {
Expand All @@ -35,7 +34,6 @@ func (bs *BuilderStatus) BuilderRecord(record BuilderRecord) {
bs.ObservedStoreGeneration = record.ObservedStoreGeneration
bs.ObservedStackGeneration = record.ObservedStackGeneration
bs.OS = record.OS
bs.Signed = record.Signed
bs.SignaturePaths = record.SignaturePaths
}

Expand Down
9 changes: 4 additions & 5 deletions pkg/apis/build/v1alpha2/builder_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ type NamespacedBuilderSpec struct {
}

// +k8s:openapi-gen=true
type SignaturePath struct {
KeyName string `json:"keyName"`
Path string `json:"path"`
type CosignSignature struct {
SigningSecret string `json:"signingSecret"`
TargetDigest string `json:"targetDigest"`
}

// +k8s:openapi-gen=true
Expand All @@ -71,8 +71,7 @@ type BuilderStatus struct {
ObservedStackGeneration int64 `json:"observedStackGeneration,omitempty"`
ObservedStoreGeneration int64 `json:"observedStoreGeneration,omitempty"`
OS string `json:"os,omitempty"`
Signed bool `json:"signed"`
SignaturePaths []SignaturePath `json:"signaturePaths,omitempty"`
SignaturePaths []CosignSignature `json:"signaturePaths,omitempty"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
6 changes: 1 addition & 5 deletions pkg/cnb/create_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,14 @@ func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychai
}

var (
signaturePaths = make([]buildapi.SignaturePath, 0)
signed = false
signaturePaths = make([]buildapi.CosignSignature, 0)
)

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{
Expand All @@ -105,7 +102,6 @@ func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychai
ObservedStackGeneration: clusterStack.Status.ObservedGeneration,
ObservedStoreGeneration: fetcher.ClusterStoreObservedGeneration(),
OS: config.OS,
Signed: signed,
SignaturePaths: signaturePaths,
}

Expand Down
19 changes: 8 additions & 11 deletions pkg/cnb/create_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
KeychainFactory: keychainFactory,
LifecycleProvider: lifecycleProvider,
ImageSigner: &fakeBuilderSigner{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.SignaturePath, error) {
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
// no-op
return nil, nil
},
Expand Down Expand Up @@ -874,13 +874,11 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
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) {
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return nil, fmt.Errorf("failed to sign builder")
},
}
Expand All @@ -905,11 +903,11 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
}

subject.ImageSigner = &fakeBuilderSigner{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.SignaturePath, error) {
return []buildapi.SignaturePath{
signBuilder: func(ctx context.Context, s string, secrets []corev1.Secret, keychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return []buildapi.CosignSignature{
{
KeyName: fmt.Sprintf("k8s://%s/%s", fakeSecret.Namespace, fakeSecret.Name),
Path: "registry.local/test-image:signature-tag",
SigningSecret: fmt.Sprintf("k8s://%s/%s", fakeSecret.Namespace, fakeSecret.Name),
TargetDigest: "registry.local/test-image:signature-tag",
},
}, nil
},
Expand All @@ -918,17 +916,16 @@ func testCreateBuilderOs(os string, t *testing.T, when spec.G, it spec.S) {
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)
signBuilder func(context.Context, string, []corev1.Secret, authn.Keychain) ([]buildapi.CosignSignature, error)
}

func (s *fakeBuilderSigner) SignBuilder(ctx context.Context, imageReference string, signingSecrets []corev1.Secret, builderKeychain authn.Keychain) ([]buildapi.SignaturePath, error) {
func (s *fakeBuilderSigner) SignBuilder(ctx context.Context, imageReference string, signingSecrets []corev1.Secret, builderKeychain authn.Keychain) ([]buildapi.CosignSignature, error) {
return s.signBuilder(ctx, imageReference, signingSecrets, builderKeychain)
}

Expand Down
87 changes: 37 additions & 50 deletions pkg/cosign/image_signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@ import (
"os"
"time"

cosignutil "github.com/pivotal/kpack/pkg/cosign/util"

"github.com/pivotal/kpack/pkg/secret"

"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"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8sclient "k8s.io/client-go/kubernetes"

"github.com/buildpacks/lifecycle/platform"
Expand All @@ -27,7 +30,7 @@ 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)
SignBuilder(context.Context, string, []corev1.Secret, authn.Keychain) ([]v1alpha2.CosignSignature, error)
}

type ImageSigner struct {
Expand All @@ -36,17 +39,6 @@ type ImageSigner struct {
fetchSignatureFunc FetchSignatureFunc
}

const (
cosignRepositoryEnv = "COSIGN_REPOSITORY"
cosignDockerMediaTypesEnv = "COSIGN_DOCKER_MEDIA_TYPES"

SecretDataCosignKey = "cosign.key"
SecretDataCosignPassword = "cosign.password"
SecretDataCosignPublicKey = "cosign.pub"
DockerMediaTypesAnnotationPrefix = "kpack.io/cosign.docker-media-types"
RepositoryAnnotationPrefix = "kpack.io/cosign.repository"
)

func NewImageSigner(logger *log.Logger, signFunc SignFunc, fetchSignatureFunc FetchSignatureFunc) *ImageSigner {
return &ImageSigner{
Logger: logger,
Expand Down Expand Up @@ -95,17 +87,17 @@ func (s *ImageSigner) sign(ro *options.RootOptions, refImage, secretLocation, co
}}

if cosignRepository, ok := cosignRepositories[cosignSecret]; ok {
if err := os.Setenv(cosignRepositoryEnv, fmt.Sprintf("%s", cosignRepository)); err != nil {
return errors.Errorf("failed setting %s env variable: %v", cosignRepositoryEnv, err)
if err := os.Setenv(cosignutil.CosignRepositoryEnv, fmt.Sprintf("%s", cosignRepository)); err != nil {
return errors.Errorf("failed setting %s env variable: %v", cosignutil.CosignRepositoryEnv, err)
}
defer os.Unsetenv(cosignRepositoryEnv)
defer os.Unsetenv(cosignutil.CosignRepositoryEnv)
}

if cosignDockerMediaType, ok := cosignDockerMediaTypes[cosignSecret]; ok {
if err := os.Setenv(cosignDockerMediaTypesEnv, fmt.Sprintf("%s", cosignDockerMediaType)); err != nil {
if err := os.Setenv(cosignutil.CosignDockerMediaTypesEnv, fmt.Sprintf("%s", cosignDockerMediaType)); err != nil {
return errors.Errorf("failed setting COSIGN_DOCKER_MEDIA_TYPES env variable: %v", err)
}
defer os.Unsetenv(cosignDockerMediaTypesEnv)
defer os.Unsetenv(cosignutil.CosignDockerMediaTypesEnv)
}

var cosignAnnotations []string
Expand Down Expand Up @@ -139,33 +131,33 @@ func (s *ImageSigner) SignBuilder(
imageReference string,
cosignSecrets []corev1.Secret,
builderKeychain authn.Keychain,
) ([]v1alpha2.SignaturePath, error) {
const Timeout time.Duration = time.Second * 30
) ([]v1alpha2.CosignSignature, error) {
const Timeout = time.Second * 30

signaturePaths := make([]v1alpha2.SignaturePath, 0, 10)
signaturePaths := make([]v1alpha2.CosignSignature, 0)

for _, secret := range cosignSecrets {
keyRef := fmt.Sprintf("k8s://%s/%s", secret.Namespace, secret.Name)
keyOpts := options.KeyOpts{
KeyRef: keyRef,
PassFunc: func(bool) ([]byte, error) {
if password, ok := secret.Data[SecretDataCosignPassword]; ok {
if password, ok := secret.Data[cosignutil.SecretDataCosignPassword]; ok {
return password, nil
}

return []byte(""), nil
},
}

if cosignRepository, ok := secret.Annotations[RepositoryAnnotationPrefix]; ok {
if err := os.Setenv(cosignRepositoryEnv, cosignRepository); err != nil {
return nil, fmt.Errorf("failed setting %s env variable: %w", cosignRepositoryEnv, err)
if cosignRepository, ok := secret.Annotations[cosignutil.RepositoryAnnotationPrefix]; ok {
if err := os.Setenv(cosignutil.CosignRepositoryEnv, cosignRepository); err != nil {
return nil, fmt.Errorf("failed setting %s env variable: %w", cosignutil.CosignRepositoryEnv, err)
}
}

if cosignDockerMediaType, ok := secret.Annotations[DockerMediaTypesAnnotationPrefix]; ok {
if err := os.Setenv(cosignDockerMediaTypesEnv, cosignDockerMediaType); err != nil {
return nil, fmt.Errorf("failed setting %s env variable: %w", cosignDockerMediaTypesEnv, err)
if cosignDockerMediaType, ok := secret.Annotations[cosignutil.DockerMediaTypesAnnotationPrefix]; ok {
if err := os.Setenv(cosignutil.CosignDockerMediaTypesEnv, cosignDockerMediaType); err != nil {
return nil, fmt.Errorf("failed setting %s env variable: %w", cosignutil.CosignDockerMediaTypesEnv, err)
}
}

Expand All @@ -186,7 +178,7 @@ func (s *ImageSigner) SignBuilder(
keyOpts,
signOptions,
[]string{imageReference}); err != nil {
return nil, errors.Errorf("unable to sign image with specified key from secret %v in namespace %v: %v", secret.Name, secret.Namespace, err)
return nil, fmt.Errorf("unable to sign image with specified key from secret %s in namespace %s: %w", secret.Name, secret.Namespace, err)
}

reference, err := name.ParseReference(imageReference)
Expand Down Expand Up @@ -216,23 +208,23 @@ func (s *ImageSigner) SignBuilder(

signaturePaths = append(
signaturePaths,
v1alpha2.SignaturePath{
KeyName: keyRef, // TODO check if secret name is enough
Path: signatureTag.Digest(digest.String()).String(),
v1alpha2.CosignSignature{
SigningSecret: keyRef,
TargetDigest: signatureTag.Digest(digest.String()).String(),
},
)

if _, found := os.LookupEnv(cosignDockerMediaTypesEnv); found {
err = os.Unsetenv(cosignDockerMediaTypesEnv)
if _, found := os.LookupEnv(cosignutil.CosignDockerMediaTypesEnv); found {
err = os.Unsetenv(cosignutil.CosignDockerMediaTypesEnv)
if err != nil {
return nil, fmt.Errorf("failed to cleanup environment variable %s: %s", cosignDockerMediaTypesEnv, err)
return nil, fmt.Errorf("failed to cleanup environment variable %s: %w", cosignutil.CosignDockerMediaTypesEnv, err)
}
}

if _, found := os.LookupEnv(cosignRepositoryEnv); found {
err = os.Unsetenv(cosignRepositoryEnv)
if _, found := os.LookupEnv(cosignutil.CosignRepositoryEnv); found {
err = os.Unsetenv(cosignutil.CosignRepositoryEnv)
if err != nil {
return nil, fmt.Errorf("failed to cleanup environment variable %s: %s", cosignRepositoryEnv, err)
return nil, fmt.Errorf("failed to cleanup environment variable %s: %w", cosignutil.CosignRepositoryEnv, err)
}
}
}
Expand All @@ -241,25 +233,20 @@ func (s *ImageSigner) SignBuilder(
}

func FetchBuilderSigningSecrets(ctx context.Context, client k8sclient.Interface, namespace string, serviceAccountName string) ([]corev1.Secret, error) {
serviceAccount, err := client.CoreV1().ServiceAccounts(namespace).Get(ctx, serviceAccountName, metav1.GetOptions{})
secretFetcher := &secret.Fetcher{Client: client}
serviceAccountSecrets, err := secretFetcher.SecretsForServiceAccount(ctx, serviceAccountName, namespace)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to retrieve secrets for ServiceAccount %s/%s: %w", namespace, serviceAccountName, err)
}

cosignSecrets := make([]corev1.Secret, 0)

for _, secretName := range serviceAccount.Secrets {
secret, err := client.CoreV1().Secrets(namespace).Get(ctx, secretName.Name, metav1.GetOptions{})
if err != nil {
// failed to retrieve one secret, fails signing
return nil, err
}

_, passwordOk := secret.Data[SecretDataCosignPassword]
_, keyOk := secret.Data[SecretDataCosignKey]
for _, cosignSecret := range serviceAccountSecrets {
_, passwordOk := cosignSecret.Data[cosignutil.SecretDataCosignPassword]
_, keyOk := cosignSecret.Data[cosignutil.SecretDataCosignKey]

if passwordOk && keyOk {
cosignSecrets = append(cosignSecrets, *secret)
cosignSecrets = append(cosignSecrets, *cosignSecret)
}
}

Expand Down
Loading

0 comments on commit 588b5e7

Please sign in to comment.