Fix bugs in googleca and update flag description (#897)

* Fix bug in googleca and update flag description Signed-off-by: Priya Wadhwa <[email protected]> * Remove requirement for specifying certificateAuthority Signed-off-by: Priya Wadhwa <[email protected]> * Code review comments Signed-off-by: Priya Wadhwa <[email protected]> * Fix nit Signed-off-by: Priya Wadhwa <[email protected]> * Update variable names Signed-off-by: Priya Wadhwa <[email protected]> Signed-off-by: Priya Wadhwa <[email protected]>
sigstore · Nov 29, 2022 · 865c05a · 865c05a
1 parent 4da60f7
commit 865c05a
Show file tree

Hide file tree

Showing 3 changed files with 148 additions and 14 deletions.
diff --git a/cmd/app/serve.go b/cmd/app/serve.go
@@ -60,7 +60,8 @@ func newServeCmd() *cobra.Command {
 	cmd.Flags().String("log_type", "dev", "logger type to use (dev/prod)")
 	cmd.Flags().String("ca", "", "googleca | tinkca | pkcs11ca | fileca | kmsca | ephemeralca (for testing)")
 	cmd.Flags().String("aws-hsm-root-ca-path", "", "Path to root CA on disk (only used with AWS HSM)")
-	cmd.Flags().String("gcp_private_ca_parent", "", "private ca parent: /projects/<project>/locations/<location>/<name> (only used with --ca googleca)")
+	cmd.Flags().String("gcp_private_ca_parent", "", "private ca parent: projects/<project>/locations/<location>/caPools/<caPool> (only used with --ca googleca)"+
+		"Optionally specify /certificateAuthorities/<caID>, which will bypass CA pool load balancing.")
 	cmd.Flags().String("hsm-caroot-id", "", "HSM ID for Root CA (only used with --ca pkcs11ca)")
 	cmd.Flags().String("ct-log-url", "http://localhost:6962/test", "host and path (with log prefix at the end) to the ct log")
 	cmd.Flags().String("ct-log-public-key-path", "", "Path to a PEM-encoded public key of the CT log, used to verify SCTs")

diff --git a/pkg/ca/googleca/v1/googleca.go b/pkg/ca/googleca/v1/googleca.go
@@ -39,8 +39,10 @@ import (
 )
 
 type CertAuthorityService struct {
-	parent string
-	client *privateca.CertificateAuthorityClient
+	certAuthorityID       string
+	certAuthorityResource string
+	caPoolResource        string
+	client                *privateca.CertificateAuthorityClient
 
 	// protected by once
 	cachedRoots     [][]*x509.Certificate
@@ -52,10 +54,26 @@ func NewCertAuthorityService(ctx context.Context, parent string, opts ...option.
 	if err != nil {
 		return nil, err
 	}
-	return &CertAuthorityService{
-		parent: parent,
+	c := CertAuthorityService{
 		client: client,
-	}, nil
+	}
+
+	if !strings.Contains(parent, "certificateAuthorities") {
+		c.caPoolResource = parent
+		return &c, nil
+	}
+	// parent should be in the form projects/*/locations/*/caPools/*/certificateAuthorities/*
+	// to create a cert, we only want projects/*/locations/*/caPools/*
+	caPoolParent := strings.Split(parent, "/certificateAuthorities")
+	c.caPoolResource = caPoolParent[0]
+
+	s := strings.SplitAfter(parent, "certificateAuthorities/")
+	if len(s) != 2 {
+		return nil, fmt.Errorf("certificate authority should be specified in the format projects/*/locations/*/caPools/*/certificateAuthorities/*")
+	}
+	c.certAuthorityID = s[1]
+	c.certAuthorityResource = parent
+	return &c, nil
 }
 
 // getPubKeyFormat Returns the PublicKey KeyFormat required by gcp privateca.
@@ -82,7 +100,7 @@ func convertID(id asn1.ObjectIdentifier) []int32 {
 	return nid
 }
 
-func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.CreateCertificateRequest, error) {
+func Req(parent, certAuthority string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.CreateCertificateRequest, error) {
 	pubkeyFormat, err := getPubKeyFormat(pemBytes)
 	if err != nil {
 		return nil, err
@@ -109,7 +127,7 @@ func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.C
 		})
 	}
 
-	return &privatecapb.CreateCertificateRequest{
+	req := &privatecapb.CreateCertificateRequest{
 		Parent: parent,
 		Certificate: &privatecapb.Certificate{
 			Lifetime: durationpb.New(time.Until(cert.NotAfter)),
@@ -134,7 +152,13 @@ func Req(parent string, pemBytes []byte, cert *x509.Certificate) (*privatecapb.C
 				},
 			},
 		},
-	}, nil
+	}
+
+	if certAuthority != "" {
+		req.IssuingCertificateAuthorityId = certAuthority
+	}
+
+	return req, nil
 }
 
 func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
@@ -143,10 +167,44 @@ func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certi
 		return c.cachedRoots, nil
 	}
 
-	// fetch the latest values for the specified CA
+	// if a specific certificate authority was specified, use that one
+	if c.certAuthorityResource != "" {
+		return c.getCertificateAuthorityTrustBundle(ctx)
+	}
+	// otherwise, get certs from all of the CAs in the pool
+	return c.listCertificateAuthorityTrustBundle(ctx)
+}
+
+func (c *CertAuthorityService) getCertificateAuthorityTrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
+	var roots [][]*x509.Certificate
+	ca, err := c.client.GetCertificateAuthority(ctx, &privatecapb.GetCertificateAuthorityRequest{
+		Name: c.certAuthorityResource,
+	})
+	if err != nil {
+		return nil, err
+	}
+	// if we fail to parse the PEM content, return an error
+	caCerts, err := cryptoutils.LoadCertificatesFromPEM(strings.NewReader(strings.Join(ca.PemCaCertificates, "")))
+	if err != nil {
+		return [][]*x509.Certificate{}, fmt.Errorf("failed parsing PemCACertificates response: %w", err)
+	}
+	if len(caCerts) == 0 {
+		return [][]*x509.Certificate{}, fmt.Errorf("error fetching root certificates")
+	}
+	roots = append(roots, caCerts)
+
+	c.cachedRootsOnce.Do(func() {
+		c.cachedRoots = roots
+	})
+
+	return c.cachedRoots, nil
+}
+
+func (c *CertAuthorityService) listCertificateAuthorityTrustBundle(ctx context.Context) ([][]*x509.Certificate, error) {
+	// fetch the latest values for the specified CA pool
 	var roots [][]*x509.Certificate
 	cas := c.client.ListCertificateAuthorities(ctx, &privatecapb.ListCertificateAuthoritiesRequest{
-		Parent: c.parent,
+		Parent: c.caPoolResource,
 	})
 	for {
 		ca, done := cas.Next()
@@ -161,7 +219,7 @@ func (c *CertAuthorityService) TrustBundle(ctx context.Context) ([][]*x509.Certi
 		if err != nil {
 			return [][]*x509.Certificate{}, fmt.Errorf("failed parsing PemCACertificates response: %w", err)
 		}
-		if len(roots) == 0 {
+		if len(caCerts) == 0 {
 			return [][]*x509.Certificate{}, fmt.Errorf("error fetching root certificates")
 		}
 		roots = append(roots, caCerts)
@@ -184,7 +242,7 @@ func (c *CertAuthorityService) CreateCertificate(ctx context.Context, principal
 		return nil, ca.ValidationError(err)
 	}
 
-	req, err := Req(c.parent, pubKeyBytes, cert)
+	req, err := Req(c.caPoolResource, c.certAuthorityID, pubKeyBytes, cert)
 	if err != nil {
 		return nil, ca.ValidationError(err)
 	}

diff --git a/pkg/ca/googleca/v1/googleca_test.go b/pkg/ca/googleca/v1/googleca_test.go
@@ -149,7 +149,82 @@ func TestReq(t *testing.T) {
 		},
 	}
 
-	req, err := Req(parent, pubKeyBytes, cert)
+	req, err := Req(parent, "", pubKeyBytes, cert)
+	// We must copy over this field because we don't inject a clock, so
+	// lifetime will always be different.
+	expectedReq.Certificate.Lifetime = req.Certificate.Lifetime
+
+	if err != nil {
+		t.Fatalf("unexpected error, got: %v", err)
+	}
+	if !proto.Equal(req, expectedReq) {
+		t.Fatalf("proto equality failed, expected: %v, got: %v", req, expectedReq)
+	}
+}
+
+func TestReqCertAuthority(t *testing.T) {
+	parent := "parent-ca"
+	priv, err := rsa.GenerateKey(rand.Reader, 2048)
+	failErr(t, err)
+
+	uri := "sigstore.dev"
+	parsedURI, err := url.Parse(uri)
+	failErr(t, err)
+
+	emailAddress := "[email protected]"
+	notAfter := time.Now().Add(time.Minute * 10)
+	pubKeyBytes, err := cryptoutils.MarshalPublicKeyToPEM(priv.Public())
+	failErr(t, err)
+	ext := pkix.Extension{Id: asn1.ObjectIdentifier{1, 2, 3}, Value: []byte{1, 2, 3}}
+
+	cert := &x509.Certificate{
+		NotAfter:        notAfter,
+		EmailAddresses:  []string{emailAddress},
+		URIs:            []*url.URL{parsedURI},
+		ExtraExtensions: []pkix.Extension{ext},
+	}
+
+	expectedReq := &privatecapb.CreateCertificateRequest{
+		Parent:                        parent,
+		IssuingCertificateAuthorityId: "cert-authority",
+		Certificate: &privatecapb.Certificate{
+			CertificateConfig: &privatecapb.Certificate_Config{
+				Config: &privatecapb.CertificateConfig{
+					PublicKey: &privatecapb.PublicKey{
+						Format: privatecapb.PublicKey_PEM,
+						Key:    pubKeyBytes,
+					},
+					X509Config: &privatecapb.X509Parameters{
+						KeyUsage: &privatecapb.KeyUsage{
+							BaseKeyUsage: &privatecapb.KeyUsage_KeyUsageOptions{
+								DigitalSignature: true,
+							},
+							ExtendedKeyUsage: &privatecapb.KeyUsage_ExtendedKeyUsageOptions{
+								CodeSigning: true,
+							},
+						},
+						AdditionalExtensions: []*privatecapb.X509Extension{
+							{
+								ObjectId: &privatecapb.ObjectId{
+									ObjectIdPath: convertID(ext.Id),
+								},
+								Value: ext.Value,
+							},
+						},
+					},
+					SubjectConfig: &privatecapb.CertificateConfig_SubjectConfig{
+						Subject: &privatecapb.Subject{},
+						SubjectAltName: &privatecapb.SubjectAltNames{
+							EmailAddresses: []string{emailAddress},
+							Uris:           []string{uri},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	req, err := Req(parent, "cert-authority", pubKeyBytes, cert)
 	// We must copy over this field because we don't inject a clock, so
 	// lifetime will always be different.
 	expectedReq.Certificate.Lifetime = req.Certificate.Lifetime