From de7e716a8274c43295e7aa2ce6a43c4a3f392aa6 Mon Sep 17 00:00:00 2001
From: Jan Dittberner <jandd@cacert.org>
Date: Wed, 30 Nov 2022 20:12:26 +0100
Subject: [PATCH] Improve signer setup code

- allow multiple attempts to setup certificates
- use CAB forum BR compliant CRLDistributionPoint for Subordinate CA
  certificates by referencing their own CRL instead of their parent CA's CRL
- store certificates in DER encoded form
---
 internal/config/config.go      |  4 ++--
 internal/config/config_test.go |  6 ++---
 internal/hsm/hsm.go            | 40 ++++++++++------------------------
 3 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/internal/config/config.go b/internal/config/config.go
index 273e5c5..ab2d354 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -270,8 +270,8 @@ func (c *SignerConfig) BuildOCSPURL(cert *CaCertificateEntry) string {
 	return c.global.URLPatterns.Ocsp
 }
 
-func (c *SignerConfig) BuildCRLUrl(cert *CaCertificateEntry) string {
-	return fmt.Sprintf(c.global.URLPatterns.CRL, cert.Parent)
+func (c *SignerConfig) BuildCRLUrl(label string) string {
+	return fmt.Sprintf(c.global.URLPatterns.CRL, label)
 }
 
 func (c *SignerConfig) GetParentCA(label string) (*CaCertificateEntry, error) {
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 6548b3c..ae3ba17 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -633,11 +633,11 @@ func TestSignerConfig_CertificateFileName(t *testing.T) {
 func TestSignerConfig_BuildCRLUrl(t *testing.T) {
 	sc := loadSignerConfig(t)
 
-	ca, err := sc.GetCADefinition("sub1")
+	_, err := sc.GetCADefinition("sub1")
 	require.NoError(t, err)
 
-	url := sc.BuildCRLUrl(ca)
-	assert.Equal(t, "http://crl.example.org/root.crl", url)
+	url := sc.BuildCRLUrl("sub1")
+	assert.Equal(t, "http://crl.example.org/sub1.crl", url)
 }
 
 func TestSignerConfig_BuildIssuerUrl(t *testing.T) {
diff --git a/internal/hsm/hsm.go b/internal/hsm/hsm.go
index 17de7dc..f3bc308 100644
--- a/internal/hsm/hsm.go
+++ b/internal/hsm/hsm.go
@@ -25,7 +25,6 @@ import (
 	"crypto/rsa"
 	"crypto/x509"
 	"encoding/asn1"
-	"encoding/pem"
 	"errors"
 	"fmt"
 	"math/big"
@@ -175,16 +174,7 @@ func (c *caFile) loadCertificate(caDirectory string) (*x509.Certificate, error)
 		return nil, fmt.Errorf("could not read %s: %w", certFile, err)
 	}
 
-	pemData, _ := pem.Decode(certData)
-	if pemData == nil {
-		return nil, fmt.Errorf("no PEM data in %s", certFile)
-	}
-
-	if pemData.Type != "CERTIFICATE" {
-		return nil, fmt.Errorf("no certificate found in %s", certFile)
-	}
-
-	certificate, err := x509.ParseCertificate(pemData.Bytes)
+	certificate, err := x509.ParseCertificate(certData)
 	if err != nil {
 		return nil, fmt.Errorf("could not parse certificate from %s: %w", certFile, err)
 	}
@@ -233,9 +223,11 @@ func (a *Access) GetRootCACertificate(label string) (*x509.Certificate, error) {
 		return certificate, nil
 	}
 
-	keyPair, err = a.getKeyPair(label, caCert.KeyInfo)
-	if err != nil {
-		return nil, err
+	if a.IsSetupMode() {
+		keyPair, err = a.getKeyPair(label, caCert.KeyInfo)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	if certificate != nil {
@@ -244,6 +236,8 @@ func (a *Access) GetRootCACertificate(label string) (*x509.Certificate, error) {
 			return nil, err
 		}
 
+		caCert.Certificate, caCert.KeyPair = certificate, keyPair
+
 		return certificate, nil
 	}
 
@@ -355,7 +349,7 @@ func (a *Access) GetSubordinateCACertificate(certLabel string) (*x509.Certificat
 			ExtKeyUsage:           caCert.ExtKeyUsage,
 			IssuingCertificateURL: []string{sc.BuildIssuerURL(caCert)},
 			OCSPServer:            []string{sc.BuildOCSPURL(caCert)},
-			CRLDistributionPoints: []string{sc.BuildCRLUrl(caCert)},
+			CRLDistributionPoints: []string{sc.BuildCRLUrl(certLabel)},
 			PolicyIdentifiers: []asn1.ObjectIdentifier{
 				// use policy identifiers from http://wiki.cacert.org/OidAllocation
 				oidCAcertClass3PolicyV1,
@@ -414,15 +408,10 @@ func (a *Access) generateSubordinateCACertificate(
 		parent.KeyPair,
 	)
 	if err != nil {
-		return nil, fmt.Errorf("could not create subordinate CA certificate: %w", err)
+		return nil, fmt.Errorf("could not create subordinate CA certificate %s: %w", certLabel, err)
 	}
 
-	certBlock := &pem.Block{
-		Type:  "CERTIFICATE",
-		Bytes: certBytes,
-	}
-
-	err = certFile.storeCertificate(a.caDirectory, pem.EncodeToMemory(certBlock))
+	err = certFile.storeCertificate(a.caDirectory, certBytes)
 	if err != nil {
 		return nil, err
 	}
@@ -590,12 +579,7 @@ func (a *Access) generateRootCACertificate(
 		return nil, fmt.Errorf("could not create root certificate: %w", err)
 	}
 
-	certBlock := &pem.Block{
-		Type:  "CERTIFICATE",
-		Bytes: certBytes,
-	}
-
-	if err = certFile.storeCertificate(a.caDirectory, pem.EncodeToMemory(certBlock)); err != nil {
+	if err = certFile.storeCertificate(a.caDirectory, certBytes); err != nil {
 		return nil, err
 	}