From 9c27b5b5cba68dabc5bbac926a8953de91b18179 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Wed, 3 Jul 2024 10:26:48 +0200 Subject: [PATCH 1/3] factor out keyless verification helper function Signed-off-by: Dmitry S --- cmd/cosign/cli/verify/verify.go | 116 +++++++++++++++++++------------- 1 file changed, 68 insertions(+), 48 deletions(-) diff --git a/cmd/cosign/cli/verify/verify.go b/cmd/cosign/cli/verify/verify.go index a4e657601e4..0dc0cf91ffc 100644 --- a/cmd/cosign/cli/verify/verify.go +++ b/cmd/cosign/cli/verify/verify.go @@ -175,54 +175,8 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) { } } if keylessVerification(c.KeyRef, c.Sk) { - switch { - case c.CertChain != "": - chain, err := loadCertChainFromFileOrURL(c.CertChain) - if err != nil { - return err - } - co.RootCerts = x509.NewCertPool() - co.RootCerts.AddCert(chain[len(chain)-1]) - if len(chain) > 1 { - co.IntermediateCerts = x509.NewCertPool() - for _, cert := range chain[:len(chain)-1] { - co.IntermediateCerts.AddCert(cert) - } - } - case c.CARoots != "": - caRoots, err := loadCertChainFromFileOrURL(c.CARoots) - if err != nil { - return err - } - co.RootCerts = x509.NewCertPool() - if len(caRoots) > 0 { - for _, cert := range caRoots { - co.RootCerts.AddCert(cert) - } - } - if c.CAIntermediates != "" { - caIntermediates, err := loadCertChainFromFileOrURL(c.CAIntermediates) - if err != nil { - return err - } - if len(caIntermediates) > 0 { - co.IntermediateCerts = x509.NewCertPool() - for _, cert := range caIntermediates { - co.IntermediateCerts.AddCert(cert) - } - } - } - default: - // This performs an online fetch of the Fulcio roots from a TUF repository. - // This is needed for verifying keyless certificates (both online and offline). - co.RootCerts, err = fulcio.GetRoots() - if err != nil { - return fmt.Errorf("getting Fulcio roots: %w", err) - } - co.IntermediateCerts, err = fulcio.GetIntermediates() - if err != nil { - return fmt.Errorf("getting Fulcio intermediates: %w", err) - } + if err := handleKeylessVerification(c.CertChain, c.CARoots, c.CAIntermediates, co); err != nil { + return err } } keyRef := c.KeyRef @@ -556,3 +510,69 @@ func shouldVerifySCT(ignoreSCT bool, keyRef string, sk bool) bool { } return true } + +// handleKeylessVerification handles the verification of keyless signatures +// for the verify command. +// +// TODO(dmitris) - mention additionally verify-attestation, verify-blob, verify-blob-attestation +// commands when they are extended to call this function. +// +// The co *cosign.CheckOpts is both input and output parameter - it gets updated +// with the root and intermediate certificates needed for verification. +// If both certChain and caRootsFile are empty strings, the Fulcio roots are loaded. +func handleKeylessVerification(certChainFile string, + caRootsFile string, + caIntermediatesFile string, + co *cosign.CheckOpts) error { + var err error + switch { + case certChainFile != "": + chain, err := loadCertChainFromFileOrURL(certChainFile) + if err != nil { + return err + } + co.RootCerts = x509.NewCertPool() + co.RootCerts.AddCert(chain[len(chain)-1]) + if len(chain) > 1 { + co.IntermediateCerts = x509.NewCertPool() + for _, cert := range chain[:len(chain)-1] { + co.IntermediateCerts.AddCert(cert) + } + } + case caRootsFile != "": + caRoots, err := loadCertChainFromFileOrURL(caRootsFile) + if err != nil { + return err + } + co.RootCerts = x509.NewCertPool() + if len(caRoots) > 0 { + for _, cert := range caRoots { + co.RootCerts.AddCert(cert) + } + } + if caIntermediatesFile != "" { + caIntermediates, err := loadCertChainFromFileOrURL(caIntermediatesFile) + if err != nil { + return err + } + if len(caIntermediates) > 0 { + co.IntermediateCerts = x509.NewCertPool() + for _, cert := range caIntermediates { + co.IntermediateCerts.AddCert(cert) + } + } + } + default: + // This performs an online fetch of the Fulcio roots from a TUF repository. + // This is needed for verifying keyless certificates (both online and offline). + co.RootCerts, err = fulcio.GetRoots() + if err != nil { + return fmt.Errorf("getting Fulcio roots: %w", err) + } + co.IntermediateCerts, err = fulcio.GetIntermediates() + if err != nil { + return fmt.Errorf("getting Fulcio intermediates: %w", err) + } + } + return nil +} From 32949de56d4329ebea5a64dc084b391e1612b4a8 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Wed, 3 Jul 2024 11:43:46 +0200 Subject: [PATCH 2/3] unit test for loadCertsKeylessVerification helper Signed-off-by: Dmitry S --- cmd/cosign/cli/verify/verify.go | 6 +- cmd/cosign/cli/verify/verify_test.go | 182 ++++++++++++++++++++++++--- test/helpers_test.go | 1 - 3 files changed, 168 insertions(+), 21 deletions(-) diff --git a/cmd/cosign/cli/verify/verify.go b/cmd/cosign/cli/verify/verify.go index 0dc0cf91ffc..e78312a5c46 100644 --- a/cmd/cosign/cli/verify/verify.go +++ b/cmd/cosign/cli/verify/verify.go @@ -175,7 +175,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) { } } if keylessVerification(c.KeyRef, c.Sk) { - if err := handleKeylessVerification(c.CertChain, c.CARoots, c.CAIntermediates, co); err != nil { + if err := loadCertsKeylessVerification(c.CertChain, c.CARoots, c.CAIntermediates, co); err != nil { return err } } @@ -511,7 +511,7 @@ func shouldVerifySCT(ignoreSCT bool, keyRef string, sk bool) bool { return true } -// handleKeylessVerification handles the verification of keyless signatures +// loadCertsKeylessVerification loads certificates for the verification of keyless signatures // for the verify command. // // TODO(dmitris) - mention additionally verify-attestation, verify-blob, verify-blob-attestation @@ -520,7 +520,7 @@ func shouldVerifySCT(ignoreSCT bool, keyRef string, sk bool) bool { // The co *cosign.CheckOpts is both input and output parameter - it gets updated // with the root and intermediate certificates needed for verification. // If both certChain and caRootsFile are empty strings, the Fulcio roots are loaded. -func handleKeylessVerification(certChainFile string, +func loadCertsKeylessVerification(certChainFile string, caRootsFile string, caIntermediatesFile string, co *cosign.CheckOpts) error { diff --git a/cmd/cosign/cli/verify/verify_test.go b/cmd/cosign/cli/verify/verify_test.go index 6eca32e5537..620c841b6f9 100644 --- a/cmd/cosign/cli/verify/verify_test.go +++ b/cmd/cosign/cli/verify/verify_test.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "crypto" + "crypto/ecdsa" "crypto/rand" "crypto/sha256" "crypto/x509" @@ -35,7 +36,9 @@ import ( "github.com/google/go-containerregistry/pkg/name" "github.com/sigstore/cosign/v2/cmd/cosign/cli/options" + "github.com/sigstore/cosign/v2/internal/pkg/cosign/fulcio/fulcioroots" "github.com/sigstore/cosign/v2/internal/ui" + "github.com/sigstore/cosign/v2/pkg/cosign" "github.com/sigstore/cosign/v2/pkg/oci" "github.com/sigstore/cosign/v2/pkg/oci/static" "github.com/sigstore/cosign/v2/test" @@ -43,6 +46,81 @@ import ( "github.com/stretchr/testify/assert" ) +type certData struct { + RootCert *x509.Certificate + RootKey *ecdsa.PrivateKey + SubCert *x509.Certificate + SubKey *ecdsa.PrivateKey + LeafCert *x509.Certificate + PrivKey *ecdsa.PrivateKey + RootCertPEM []byte + SubCertPEM []byte + LeafCertPEM []byte +} + +func getTestCerts(t *testing.T) *certData { + t.Helper() + eexts := []pkix.Extension{ + {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 2}, Value: []byte("myWorkflowTrigger")}, + {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 3}, Value: []byte("myWorkflowSha")}, + {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 4}, Value: []byte("myWorkflowName")}, + {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 5}, Value: []byte("myWorkflowRepository")}, + {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 6}, Value: []byte("myWorkflowRef")}, + } + cd := &certData{} + var err error + if cd.RootCert, cd.RootKey, err = test.GenerateRootCa(); err != nil { + t.Fatal(err) + } + if cd.SubCert, cd.SubKey, err = test.GenerateSubordinateCa(cd.RootCert, cd.RootKey); err != nil { + t.Fatal(err) + } + if cd.LeafCert, cd.PrivKey, err = test.GenerateLeafCert("subject", "oidc-issuer", cd.SubCert, cd.SubKey, eexts...); err != nil { + t.Fatal(err) + } + cd.RootCertPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cd.RootCert.Raw}) + cd.SubCertPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cd.SubCert.Raw}) + cd.LeafCertPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cd.LeafCert.Raw}) + return cd +} + +func makeCertChainFile(t *testing.T, rootCert, subCert, leafCert []byte) string { + t.Helper() + f, err := os.CreateTemp("", "certchain") + if err != nil { + t.Fatal(err) + } + defer f.Close() + _, err = f.Write(append(append(rootCert, subCert...), leafCert...)) + if err != nil { + t.Fatal(err) + } + return f.Name() +} + +func makeRootsIntermediatesFiles(t *testing.T, roots, intermediates []byte) (string, string) { + t.Helper() + rootF, err := os.CreateTemp("", "roots") + if err != nil { + t.Fatal(err) + } + defer rootF.Close() + _, err = rootF.Write(roots) + if err != nil { + t.Fatal(err) + } + intermediateF, err := os.CreateTemp("", "intermediates") + if err != nil { + t.Fatal(err) + } + defer intermediateF.Close() + _, err = intermediateF.Write(intermediates) + if err != nil { + t.Fatal(err) + } + return rootF.Name(), intermediateF.Name() +} + func TestPrintVerification(t *testing.T) { // while we are adding a more human-readable output for cert extensions, on the other hand // we want as backward compatible as possible, so we are keeping the old OIDs field names as well. @@ -76,22 +154,9 @@ func TestPrintVerification(t *testing.T) { } ] ` - eexts := []pkix.Extension{ - {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 2}, Value: []byte("myWorkflowTrigger")}, - {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 3}, Value: []byte("myWorkflowSha")}, - {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 4}, Value: []byte("myWorkflowName")}, - {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 5}, Value: []byte("myWorkflowRepository")}, - {Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 57264, 1, 6}, Value: []byte("myWorkflowRef")}, - } - rootCert, rootKey, _ := test.GenerateRootCa() - subCert, subKey, _ := test.GenerateSubordinateCa(rootCert, rootKey) - leafCert, privKey, _ := test.GenerateLeafCert("subject", "oidc-issuer", subCert, subKey, eexts...) - pemRoot := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: rootCert.Raw}) - pemSub := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: subCert.Raw}) - pemLeaf := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: leafCert.Raw}) - + certs := getTestCerts(t) rootPool := x509.NewCertPool() - rootPool.AddCert(rootCert) + rootPool.AddCert(certs.RootCert) // Generate the payload for the image, and check the digest. b := bytes.Buffer{} @@ -107,11 +172,11 @@ func TestPrintVerification(t *testing.T) { p := b.Bytes() h := sha256.Sum256(p) - signature, _ := privKey.Sign(rand.Reader, h[:], crypto.SHA256) + signature, _ := certs.PrivKey.Sign(rand.Reader, h[:], crypto.SHA256) ociSig, _ := static.NewSignature(p, base64.StdEncoding.EncodeToString(signature), - static.WithCertChain(pemLeaf, appendSlices([][]byte{pemSub, pemRoot}))) + static.WithCertChain(certs.LeafCertPEM, appendSlices([][]byte{certs.SubCertPEM, certs.RootCertPEM}))) captureOutput := func(f func()) string { reader, writer, err := os.Pipe() @@ -198,3 +263,86 @@ func TestVerifyCertMissingIssuer(t *testing.T) { t.Fatal("verify expected 'need --certificate-oidc-issuer'") } } + +func TestLoadCertsKeylessVerification(t *testing.T) { + certs := getTestCerts(t) + certChainFile := makeCertChainFile(t, certs.RootCertPEM, certs.SubCertPEM, certs.LeafCertPEM) + rootsFile, intermediatesFile := makeRootsIntermediatesFiles(t, certs.RootCertPEM, certs.SubCertPEM) + tests := []struct { + name string + certChain string + caRoots string + caIntermediates string + co *cosign.CheckOpts + sigstoreRootFile string + wantErr bool + }{ + { + name: "default fulcio", + wantErr: false, + }, + { + name: "non-existent SIGSTORE_ROOT_FILE", + sigstoreRootFile: "tesdata/nosuch-asdfjkl.pem", + wantErr: true, + }, + { + name: "good certchain", + certChain: certChainFile, + wantErr: false, + }, + { + name: "bad certchain", + certChain: "testdata/nosuch-certchain-file.pem", + wantErr: true, + }, + { + name: "roots", + caRoots: rootsFile, + wantErr: false, + }, + { + name: "bad roots", + caRoots: "testdata/nosuch-roots-file.pem", + wantErr: true, + }, + { + name: "roots and intermediate", + caRoots: rootsFile, + caIntermediates: intermediatesFile, + wantErr: false, + }, + { + name: "bad roots good intermediate", + caRoots: "testdata/nosuch-roots-file.pem", + caIntermediates: intermediatesFile, + wantErr: true, + }, + { + name: "good roots bad intermediate", + caRoots: rootsFile, + caIntermediates: "testdata/nosuch-intermediates-file.pem", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.sigstoreRootFile != "" { + os.Setenv("SIGSTORE_ROOT_FILE", tt.sigstoreRootFile) + } else { + t.Setenv("SIGSTORE_ROOT_FILE", "") + } + fulcioroots.ReInit() + if tt.co == nil { + tt.co = &cosign.CheckOpts{} + } + + err := loadCertsKeylessVerification(tt.certChain, tt.caRoots, tt.caIntermediates, tt.co) + if err == nil && tt.wantErr { + t.Fatalf("expected error but got none") + } else if err != nil && !tt.wantErr { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/test/helpers_test.go b/test/helpers_test.go index 7c4743428a7..3ee522e8f43 100644 --- a/test/helpers_test.go +++ b/test/helpers_test.go @@ -86,7 +86,6 @@ func verifyCertificateChain(t *testing.T, certChainFile string) { } // Check if the file contents are a PEM-encoded TLS certificate - t.Logf("DMDEBUG 76 before isPEMEncodedCertChain") if !isPEMEncodedCertChain(data) { t.Fatalf("file %s doesn't contain a valid PEM-encoded TLS certificate chain", certChainFile) } From a8a73411e903d9360f179384b3b2b9a40d78220a Mon Sep 17 00:00:00 2001 From: Dmitry Savintsev Date: Tue, 9 Jul 2024 11:23:53 +0200 Subject: [PATCH 3/3] remove username from TODOs Signed-off-by: Dmitry Savintsev --- cmd/cosign/cli/verify/verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/cosign/cli/verify/verify.go b/cmd/cosign/cli/verify/verify.go index e78312a5c46..80649b92927 100644 --- a/cmd/cosign/cli/verify/verify.go +++ b/cmd/cosign/cli/verify/verify.go @@ -514,7 +514,7 @@ func shouldVerifySCT(ignoreSCT bool, keyRef string, sk bool) bool { // loadCertsKeylessVerification loads certificates for the verification of keyless signatures // for the verify command. // -// TODO(dmitris) - mention additionally verify-attestation, verify-blob, verify-blob-attestation +// TODO - mention additionally verify-attestation, verify-blob, verify-blob-attestation // commands when they are extended to call this function. // // The co *cosign.CheckOpts is both input and output parameter - it gets updated