From 315b8bf594220fd15732fe59c7ef135cd71d7ed9 Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Tue, 24 Apr 2018 16:31:42 -0700
Subject: [PATCH] Simplify the CAProvider.Sign method

---
 agent/connect/ca_provider.go        |  4 +---
 agent/consul/connect_ca_endpoint.go | 28 ++++++++++++++++++++----
 agent/consul/connect_ca_provider.go | 33 ++++++++++++-----------------
 3 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/agent/connect/ca_provider.go b/agent/connect/ca_provider.go
index 1eb5bde11f..bec0288510 100644
--- a/agent/connect/ca_provider.go
+++ b/agent/connect/ca_provider.go
@@ -2,8 +2,6 @@ package connect
 
 import (
 	"crypto/x509"
-
-	"github.com/hashicorp/consul/agent/structs"
 )
 
 // CAProvider is the interface for Consul to interact with
@@ -24,7 +22,7 @@ type CAProvider interface {
 	GenerateIntermediate() (string, error)
 
 	// Sign signs a leaf certificate used by Connect proxies from a CSR.
-	Sign(*x509.CertificateRequest) (*structs.IssuedCert, error)
+	Sign(*x509.CertificateRequest) (string, error)
 
 	// CrossSignCA must accept a CA certificate signed by another CA's key
 	// and cross sign it exactly as it is such that it forms a chain back the the
diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go
index b0041423ed..b952c5f877 100644
--- a/agent/consul/connect_ca_endpoint.go
+++ b/agent/consul/connect_ca_endpoint.go
@@ -254,17 +254,37 @@ func (s *ConnectCA) Sign(
 		return err
 	}
 
-	// todo(kyhavlov): more validation on the CSR before signing
-
 	provider := s.srv.getCAProvider()
 
-	cert, err := provider.Sign(csr)
+	// todo(kyhavlov): more validation on the CSR before signing
+	pem, err := provider.Sign(csr)
+	if err != nil {
+		return err
+	}
+
+	// Parse the SPIFFE ID
+	spiffeId, err := connect.ParseCertURI(csr.URIs[0])
+	if err != nil {
+		return err
+	}
+	serviceId, ok := spiffeId.(*connect.SpiffeIDService)
+	if !ok {
+		return fmt.Errorf("SPIFFE ID in CSR must be a service ID")
+	}
+	cert, err := connect.ParseCert(pem)
 	if err != nil {
 		return err
 	}
 
 	// Set the response
-	*reply = *cert
+	reply = &structs.IssuedCert{
+		SerialNumber: connect.HexString(cert.SerialNumber.Bytes()),
+		CertPEM:      pem,
+		Service:      serviceId.Service,
+		ServiceURI:   cert.URIs[0].String(),
+		ValidAfter:   cert.NotBefore,
+		ValidBefore:  cert.NotAfter,
+	}
 
 	return nil
 }
diff --git a/agent/consul/connect_ca_provider.go b/agent/consul/connect_ca_provider.go
index 2f32a3d67a..d7d76f88ee 100644
--- a/agent/consul/connect_ca_provider.go
+++ b/agent/consul/connect_ca_provider.go
@@ -184,7 +184,7 @@ func (c *ConsulCAProvider) Cleanup() error {
 
 // Sign returns a new certificate valid for the given SpiffeIDService
 // using the current CA.
-func (c *ConsulCAProvider) Sign(csr *x509.CertificateRequest) (*structs.IssuedCert, error) {
+func (c *ConsulCAProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 	// Lock during the signing so we don't use the same index twice
 	// for different cert serial numbers.
 	c.Lock()
@@ -194,36 +194,36 @@ func (c *ConsulCAProvider) Sign(csr *x509.CertificateRequest) (*structs.IssuedCe
 	state := c.srv.fsm.State()
 	_, providerState, err := state.CAProviderState(c.id)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
 	// Create the keyId for the cert from the signing private key.
 	signer, err := connect.ParseSigner(providerState.PrivateKey)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 	if signer == nil {
-		return nil, fmt.Errorf("error signing cert: Consul CA not initialized yet")
+		return "", fmt.Errorf("error signing cert: Consul CA not initialized yet")
 	}
 	keyId, err := connect.KeyId(signer.Public())
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 
 	// Parse the SPIFFE ID
 	spiffeId, err := connect.ParseCertURI(csr.URIs[0])
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 	serviceId, ok := spiffeId.(*connect.SpiffeIDService)
 	if !ok {
-		return nil, fmt.Errorf("SPIFFE ID in CSR must be a service ID")
+		return "", fmt.Errorf("SPIFFE ID in CSR must be a service ID")
 	}
 
 	// Parse the CA cert
 	caCert, err := connect.ParseCert(providerState.RootCert)
 	if err != nil {
-		return nil, fmt.Errorf("error parsing CA cert: %s", err)
+		return "", fmt.Errorf("error parsing CA cert: %s", err)
 	}
 
 	// Cert template for generation
@@ -257,11 +257,11 @@ func (c *ConsulCAProvider) Sign(csr *x509.CertificateRequest) (*structs.IssuedCe
 	bs, err := x509.CreateCertificate(
 		rand.Reader, &template, caCert, signer.Public(), signer)
 	if err != nil {
-		return nil, fmt.Errorf("error generating certificate: %s", err)
+		return "", fmt.Errorf("error generating certificate: %s", err)
 	}
 	err = pem.Encode(&buf, &pem.Block{Type: "CERTIFICATE", Bytes: bs})
 	if err != nil {
-		return nil, fmt.Errorf("error encoding private key: %s", err)
+		return "", fmt.Errorf("error encoding private key: %s", err)
 	}
 
 	// Increment the leaf cert index
@@ -273,21 +273,14 @@ func (c *ConsulCAProvider) Sign(csr *x509.CertificateRequest) (*structs.IssuedCe
 	}
 	resp, err := c.srv.raftApply(structs.ConnectCARequestType, args)
 	if err != nil {
-		return nil, err
+		return "", err
 	}
 	if respErr, ok := resp.(error); ok {
-		return nil, respErr
+		return "", respErr
 	}
 
 	// Set the response
-	return &structs.IssuedCert{
-		SerialNumber: connect.HexString(template.SerialNumber.Bytes()),
-		CertPEM:      buf.String(),
-		Service:      serviceId.Service,
-		ServiceURI:   template.URIs[0].String(),
-		ValidAfter:   template.NotBefore,
-		ValidBefore:  template.NotAfter,
-	}, nil
+	return buf.String(), nil
 }
 
 // CrossSignCA returns the given intermediate CA cert signed by the current active root.