Fix BYO-root with intermediate to fetch intermediates from annotation

This fixes a regression where intermediates, when not present in the intermediate certificate pool, would be fetched from the OCI chain annotation. Ref #1554, which includes more details Signed-off-by: Hayden Blauzvern <[email protected]>
sigstore · Sep 13, 2022 · 5e42630 · 5e42630
1 parent 01492c6
commit 5e42630
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 5 deletions.
diff --git a/internal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go b/internal/pkg/cosign/fulcio/fulcioroots/fulcioroots.go
@@ -59,7 +59,8 @@ func GetIntermediates() (*x509.CertPool, error) {
 
 func initRoots() (*x509.CertPool, *x509.CertPool, error) {
 	rootPool := x509.NewCertPool()
-	intermediatePool := x509.NewCertPool()
+	// intermediatePool should be nil if no intermediates are found
+	var intermediatePool *x509.CertPool
 
 	rootEnv := os.Getenv(altRoot)
 	if rootEnv != "" {
@@ -76,6 +77,9 @@ func initRoots() (*x509.CertPool, *x509.CertPool, error) {
 			if bytes.Equal(cert.RawSubject, cert.RawIssuer) {
 				rootPool.AddCert(cert)
 			} else {
+				if intermediatePool == nil {
+					intermediatePool = x509.NewCertPool()
+				}
 				intermediatePool.AddCert(cert)
 			}
 		}

diff --git a/internal/pkg/cosign/fulcio/fulcioroots/fulcioroots_test.go b/internal/pkg/cosign/fulcio/fulcioroots/fulcioroots_test.go
@@ -16,13 +16,19 @@ package fulcioroots
 
 import (
 	"os"
+	"sync"
 	"testing"
 
 	"github.com/sigstore/cosign/test"
 	"github.com/sigstore/sigstore/pkg/cryptoutils"
 )
 
+func resetState() {
+	rootsOnce = sync.Once{}
+}
+
 func TestGetFulcioRoots(t *testing.T) {
+	t.Cleanup(resetState)
 	rootCert, rootPriv, _ := test.GenerateRootCa()
 	rootPemCert, _ := cryptoutils.MarshalCertificateToPEM(rootCert)
 	subCert, _, _ := test.GenerateSubordinateCa(rootCert, rootPriv)
@@ -42,17 +48,47 @@ func TestGetFulcioRoots(t *testing.T) {
 	}
 	t.Setenv("SIGSTORE_ROOT_FILE", tmpCertFile.Name())
 
-	if rootCertPool, re := Get(); err != nil {
-		t.Fatalf("failed to get roots: %v", re)
+	if rootCertPool, err := Get(); err != nil {
+		t.Fatalf("failed to get roots: %v", err)
 	} else if len(rootCertPool.Subjects()) != 1 { // nolint:staticcheck
 		// ignore deprecation error because certificates do not contain from SystemCertPool
 		t.Errorf("expected 1 root certificate, got 0")
 	}
 
-	if subCertPool, ie := GetIntermediates(); err != nil {
-		t.Fatalf("failed to get intermediates: %v", ie)
+	if subCertPool, err := GetIntermediates(); err != nil {
+		t.Fatalf("failed to get intermediates: %v", err)
 	} else if len(subCertPool.Subjects()) != 1 { // nolint:staticcheck
 		// ignore deprecation error because certificates do not contain from SystemCertPool
 		t.Errorf("expected 1 intermediate certificate, got 0")
 	}
+
+}
+
+func TestGetFulcioRootsWithoutIntermediate(t *testing.T) {
+	t.Cleanup(resetState)
+	rootCert, _, _ := test.GenerateRootCa()
+	rootPemCert, _ := cryptoutils.MarshalCertificateToPEM(rootCert)
+
+	tmpCertFile, err := os.CreateTemp(t.TempDir(), "cosign_fulcio_root_*.cert")
+	if err != nil {
+		t.Fatalf("failed to create temp cert file: %v", err)
+	}
+	defer tmpCertFile.Close()
+	if _, err := tmpCertFile.Write(rootPemCert); err != nil {
+		t.Fatalf("failed to write cert file: %v", err)
+	}
+	t.Setenv("SIGSTORE_ROOT_FILE", tmpCertFile.Name())
+
+	if rootCertPool, err := Get(); err != nil {
+		t.Fatalf("failed to get roots: %v", err)
+	} else if len(rootCertPool.Subjects()) != 1 { // nolint:staticcheck
+		// ignore deprecation error because certificates do not contain from SystemCertPool
+		t.Errorf("expected 1 root certificate, got 0")
+	}
+
+	if subCertPool, err := GetIntermediates(); err != nil {
+		t.Fatalf("failed to get intermediates: %v", err)
+	} else if subCertPool != nil {
+		t.Errorf("expected no intermediate cert pool")
+	}
 }