Remove cmd.LoadCert in favor of core.LoadCert (#5165)

Having both of these very similar methods sitting around
only serves to increase confusion. This removes the last
few places which use `cmd.LoadCert` and replaces them
with `core.LoadCert`, and deletes the method itself.

Fixes #5163

cmd/boulder-wfe/main.go

@@ -16,6 +16,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	noncepb "github.com/letsencrypt/boulder/nonce/proto"
 	rapb "github.com/letsencrypt/boulder/ra/proto"
@@ -157,8 +158,9 @@ func main() {
 	wfe.DirectoryCAAIdentity = c.WFE.DirectoryCAAIdentity
 	wfe.DirectoryWebsite = c.WFE.DirectoryWebsite
 
-	wfe.IssuerCert, err = cmd.LoadCert(c.Common.IssuerCert)
+	issuerCert, err := core.LoadCert(c.Common.IssuerCert)
-	cmd.FailOnError(err, fmt.Sprintf("Couldn't read issuer cert [%s]", c.Common.IssuerCert))
+	cmd.FailOnError(err, fmt.Sprintf("Couldn't load issuer cert [%s]", c.Common.IssuerCert))
+	wfe.IssuerCert = &issuance.Certificate{Certificate: issuerCert}
 
 	logger.Infof("WFE using key policy: %#v", kp)
 

cmd/boulder-wfe2/main.go

@@ -19,6 +19,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	noncepb "github.com/letsencrypt/boulder/nonce/proto"
 	rapb "github.com/letsencrypt/boulder/ra/proto"
@@ -346,8 +347,9 @@ func main() {
 	wfe.DirectoryWebsite = c.WFE.DirectoryWebsite
 	wfe.LegacyKeyIDPrefix = c.WFE.LegacyKeyIDPrefix
 
-	wfe.IssuerCert, err = cmd.LoadCert(c.Common.IssuerCert)
+	issuerCert, err := core.LoadCert(c.Common.IssuerCert)
-	cmd.FailOnError(err, fmt.Sprintf("Couldn't read issuer cert [%s]", c.Common.IssuerCert))
+	cmd.FailOnError(err, fmt.Sprintf("Couldn't load issuer cert [%s]", c.Common.IssuerCert))
+	wfe.IssuerCert = &issuance.Certificate{Certificate: issuerCert}
 
 	logger.Infof("WFE using key policy: %#v", kp)
 

cmd/shell.go

@@ -3,8 +3,6 @@ package cmd
 
 import (
 	"encoding/json"
-	"encoding/pem"
-	"errors"
 	"expvar"
 	"fmt"
 	"io/ioutil"
@@ -252,28 +250,6 @@ func FailOnError(err error, msg string) {
 	}
 }
 
-// LoadCert loads a PEM-formatted certificate from the provided path, returning
-// it as a byte array, or an error if it couldn't be decoded.
-func LoadCert(path string) (cert []byte, err error) {
-	if path == "" {
-		err = errors.New("Issuer certificate was not provided in config.")
-		return
-	}
-	pemBytes, err := ioutil.ReadFile(path)
-	if err != nil {
-		return
-	}
-
-	block, _ := pem.Decode(pemBytes)
-	if block == nil || block.Type != "CERTIFICATE" {
-		err = errors.New("Invalid certificate value returned")
-		return
-	}
-
-	cert = block.Bytes
-	return
-}
-
 // ReadConfigFile takes a file path as an argument and attempts to
 // unmarshal the content of the file into a struct containing a
 // configuration of a boulder component.

cmd/shell_test.go

@@ -148,36 +148,6 @@ func TestVersionString(t *testing.T) {
 	test.AssertEquals(t, versionStr, expected)
 }
 
-func TestLoadCert(t *testing.T) {
-	testCases := []struct {
-		path        string
-		expectedErr string
-	}{
-		{
-			"",
-			"Issuer certificate was not provided in config.",
-		},
-		{
-			"../does/not/exist",
-			"open ../does/not/exist: no such file or directory",
-		},
-		{
-			"./testdata/key.pem",
-			"Invalid certificate value returned",
-		},
-	}
-
-	for _, tc := range testCases {
-		_, err := LoadCert(tc.path)
-		test.AssertError(t, err, fmt.Sprintf("LoadCert(%q) did not error", tc.path))
-		test.AssertEquals(t, err.Error(), tc.expectedErr)
-	}
-
-	bytes, err := LoadCert("./testdata/cert.pem")
-	test.AssertNotError(t, err, "LoadCert(\"./testdata/cert.pem\") errored")
-	test.AssertNotEquals(t, len(bytes), 0)
-}
-
 func TestReadConfigFile(t *testing.T) {
 	err := ReadConfigFile("", nil)
 	test.AssertError(t, err, "ReadConfigFile('') did not error")

core/util.go

@@ -279,17 +279,20 @@ func LoadCertBundle(filename string) ([]*x509.Certificate, error) {
 }
 
 // LoadCert loads a PEM certificate specified by filename or returns an error
-func LoadCert(filename string) (cert *x509.Certificate, err error) {
+func LoadCert(filename string) (*x509.Certificate, error) {
 	certPEM, err := ioutil.ReadFile(filename)
 	if err != nil {
-		return
+		return nil, err
 	}
 	block, _ := pem.Decode(certPEM)
 	if block == nil {
 		return nil, fmt.Errorf("No data in cert PEM file %s", filename)
 	}
-	cert, err = x509.ParseCertificate(block.Bytes)
+	cert, err := x509.ParseCertificate(block.Bytes)
-	return
+	if err != nil {
+		return nil, err
+	}
+	return cert, nil
 }
 
 // retryJitter is used to prevent bunched retried queries from falling into lockstep

core/util_test.go

@@ -1,10 +1,12 @@
 package core
 
 import (
+	"encoding/asn1"
 	"encoding/json"
 	"fmt"
 	"math"
 	"math/big"
+	"os"
 	"sort"
 	"strings"
 	"testing"
@@ -153,6 +155,30 @@ func TestValidSerial(t *testing.T) {
 	test.AssertEquals(t, isValidSerial, true)
 }
 
+func TestLoadCert(t *testing.T) {
+	var osPathErr *os.PathError
+	_, err := LoadCert("")
+	test.AssertError(t, err, "Loading empty path did not error")
+	test.AssertErrorWraps(t, err, &osPathErr)
+
+	_, err = LoadCert("totally/fake/path")
+	test.AssertError(t, err, "Loading nonexistent path did not error")
+	test.AssertErrorWraps(t, err, &osPathErr)
+
+	_, err = LoadCert("../test/test-ca.der")
+	test.AssertError(t, err, "Loading non-PEM file did not error")
+	test.AssertEquals(t, err.Error(), "No data in cert PEM file ../test/test-ca.der")
+
+	var asnStructuralErr asn1.StructuralError
+	_, err = LoadCert("../test/test-ca.key")
+	test.AssertError(t, err, "Loading non-cert file did not error")
+	test.AssertErrorWraps(t, err, &asnStructuralErr)
+
+	cert, err := LoadCert("../test/test-ca.pem")
+	test.AssertNotError(t, err, "Failed to load cert file")
+	test.AssertEquals(t, cert.Subject.CommonName, "happy hacker fake CA")
+}
+
 func TestRetryBackoff(t *testing.T) {
 	assertBetween := func(a, b, c float64) {
 		t.Helper()

wfe/wfe.go

@@ -26,6 +26,7 @@ import (
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/identifier"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics/measured_http"
 	"github.com/letsencrypt/boulder/nonce"
@@ -74,8 +75,8 @@ type WebFrontEndImpl struct {
 	// URL configuration parameters
 	BaseURL string
 
-	// Issuer certificate (DER) for /acme/issuer-cert
+	// Issuer certificate for /acme/issuer-cert
-	IssuerCert []byte
+	IssuerCert *issuance.Certificate
 
 	// URL to the current subscriber agreement (should contain some version identifier)
 	SubscriberAgreementURL string
@@ -1499,7 +1500,7 @@ func (wfe *WebFrontEndImpl) Issuer(ctx context.Context, logEvent *web.RequestEve
 	// TODO Content negotiation
 	response.Header().Set("Content-Type", "application/pkix-cert")
 	response.WriteHeader(http.StatusOK)
-	if _, err := response.Write(wfe.IssuerCert); err != nil {
+	if _, err := response.Write(wfe.IssuerCert.Raw); err != nil {
 		wfe.log.Warningf("Could not write response: %s", err)
 	}
 }

wfe/wfe_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/identifier"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/mocks"
@@ -2131,7 +2132,8 @@ func TestTermsRedirect(t *testing.T) {
 
 func TestIssuer(t *testing.T) {
 	wfe, _ := setupWFE(t)
-	wfe.IssuerCert = []byte{0, 0, 1}
+	wfe.IssuerCert = &issuance.Certificate{Certificate: &x509.Certificate{}}
+	wfe.IssuerCert.Raw = []byte{0, 0, 1}
 
 	responseWriter := httptest.NewRecorder()
 
@@ -2139,7 +2141,7 @@ func TestIssuer(t *testing.T) {
 		Method: "GET",
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
-	test.Assert(t, bytes.Compare(responseWriter.Body.Bytes(), wfe.IssuerCert) == 0, "Incorrect bytes returned")
+	test.Assert(t, bytes.Compare(responseWriter.Body.Bytes(), wfe.IssuerCert.Raw) == 0, "Incorrect bytes returned")
 }
 
 func TestGetCertificate(t *testing.T) {

wfe2/wfe.go

@@ -23,6 +23,7 @@ import (
 	"github.com/letsencrypt/boulder/goodkey"
 	bgrpc "github.com/letsencrypt/boulder/grpc"
 	"github.com/letsencrypt/boulder/identifier"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics/measured_http"
 	"github.com/letsencrypt/boulder/nonce"
@@ -76,8 +77,8 @@ type WebFrontEndImpl struct {
 	clk   clock.Clock
 	stats wfe2Stats
 
-	// Issuer certificate (DER) for /acme/issuer-cert
+	// Issuer certificate for /acme/issuer-cert
-	IssuerCert []byte
+	IssuerCert *issuance.Certificate
 
 	// certificateChains maps AIA issuer URLs to a slice of []byte containing a leading
 	// newline and one or more PEM encoded certificates separated by a newline,
@@ -1705,7 +1706,7 @@ func (wfe *WebFrontEndImpl) Issuer(ctx context.Context, logEvent *web.RequestEve
 	// TODO Content negotiation
 	response.Header().Set("Content-Type", "application/pkix-cert")
 	response.WriteHeader(http.StatusOK)
-	if _, err := response.Write(wfe.IssuerCert); err != nil {
+	if _, err := response.Write(wfe.IssuerCert.Raw); err != nil {
 		wfe.log.Warningf("Could not write response: %s", err)
 	}
 }

wfe2/wfe_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/identifier"
+	"github.com/letsencrypt/boulder/issuance"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/mocks"
@@ -911,6 +912,7 @@ func TestNonceEndpoint(t *testing.T) {
 
 func TestHTTPMethods(t *testing.T) {
 	wfe, _ := setupWFE(t)
+	wfe.IssuerCert = &issuance.Certificate{Certificate: &x509.Certificate{}}
 	mux := wfe.Handler(metrics.NoopRegisterer)
 
 	// NOTE: Boulder's muxer treats HEAD as implicitly allowed if GET is specified
@@ -1744,7 +1746,8 @@ func TestAccount(t *testing.T) {
 
 func TestIssuer(t *testing.T) {
 	wfe, _ := setupWFE(t)
-	wfe.IssuerCert = []byte{0, 0, 1}
+	wfe.IssuerCert = &issuance.Certificate{Certificate: &x509.Certificate{}}
+	wfe.IssuerCert.Raw = []byte{0, 0, 1}
 
 	responseWriter := httptest.NewRecorder()
 
@@ -1752,7 +1755,7 @@ func TestIssuer(t *testing.T) {
 		Method: "GET",
 	})
 	test.AssertEquals(t, responseWriter.Code, http.StatusOK)
-	test.Assert(t, bytes.Compare(responseWriter.Body.Bytes(), wfe.IssuerCert) == 0, "Incorrect bytes returned")
+	test.Assert(t, bytes.Compare(responseWriter.Body.Bytes(), wfe.IssuerCert.Raw) == 0, "Incorrect bytes returned")
 }
 
 func TestGetCertificate(t *testing.T) {