From d55c1b68d8d88af1bd64522b106cfaa838e4e059 Mon Sep 17 00:00:00 2001 From: Max Lambrecht Date: Wed, 21 Jun 2023 14:10:33 -0500 Subject: [PATCH] Improvements and fixes in the X509CA disk (#233) Signed-off-by: Max Lambrecht --- conf/server/server_full.conf | 26 ++++--- doc/galadriel_server.md | 12 +-- pkg/common/cryptoutil/certs.go | 30 ++++++++ pkg/common/cryptoutil/certs_test.go | 84 ++++++++++++++++---- pkg/common/x509ca/disk/disk.go | 115 +++++++++++----------------- pkg/common/x509ca/disk/disk_test.go | 16 ++-- test/certtest/certs.go | 10 ++- 7 files changed, 180 insertions(+), 113 deletions(-) diff --git a/conf/server/server_full.conf b/conf/server/server_full.conf index 92bac4c6..10ecefcd 100644 --- a/conf/server/server_full.conf +++ b/conf/server/server_full.conf @@ -30,20 +30,26 @@ providers { # X509CA "disk": Utilizes a Certificate Authority (CA) certificate, loaded from disk, to issue X509 certificates. X509CA "disk" { - # key_file_path: The path to the PEM-encoded private key file of the CA. - # This path can be relative or absolute. + # key_file_path: Path to the CA key file. Key files must + # contain a single PEM encoded key. The supported key types are EC + # (ASN.1 or PKCS8 encoded) or RSA (PKCS1 or PKCS8 encoded). key_file_path = "./conf/server/dummy_root_ca.key" - # cert_file_path: The path to the PEM-encoded certificate file of the CA. - # This path can be relative or absolute. + # cert_file_path: If Galadriel is using a self-signed CA, cert_file_path + # should specify the path to a single PEM encoded certificate + # representing the CA certificate. If not self-signed, + # cert_file_path should specify the path to a file that must contain + # one or more certificates necessary to establish a valid certificate + # chain up the root certificates defined in bundle_file_path. cert_file_path = "./conf/server/dummy_root_ca.crt" - # bundle_file_path: Optional. Specifies the path to a file with one or more PEM-encoded certificates - # forming an upstream chain of trust. If Galadriel is using a self-signed CA, this can be omitted. - # However, if Galadriel uses an intermediate CA from a chain of trust that leads back to an external root CA - # to issue certificates, this parameter should point to a file containing one or more certificates that form - # this chain of trust. The path can be relative or absolute. - # Note: This bundle does not need to include the root CA itself. + # bundle_file_path: If Galadriel is using a self-signed CA, bundle_file_path + # can be left unset. If not self-signed, then bundle_file_path should + # be the path to a file that must contain one or more certificates + # representing the upstream root certificates and the file at + # cert_file_path contains one or more certificates necessary to chain up + # the root certificates in bundle_file_path (where the first + # certificate in cert_file_path is the CA certificate). # bundle_file_path = "" } diff --git a/doc/galadriel_server.md b/doc/galadriel_server.md index 86abeb9f..21016b50 100644 --- a/doc/galadriel_server.md +++ b/doc/galadriel_server.md @@ -70,12 +70,12 @@ providers { The X509CA section provides configuration details for X.509 CA providers: -| Option | Description | -|--------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `disk` | Uses a CA (either ROOT or INTERMEDIATE) and private key loaded from disk to issue X.509 certificates. | -| `key_file_path` | Path to the CA private key file in PEM format. This path can be relative or absolute. | -| `cert_file_path` | Path to the CA certificate file in PEM format. This path can be relative or absolute. | -| `bundle_file_path` | Required when the cert_file_path does not contain a self-signed CA certificate. This is the path to the file containing one or more certificates forming the chain of trust to a root CA. This file should contain any number of intermediate CA certificates that chain back to a root CA. This path can be relative or absolute. | +| Option | Description | +|--------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `disk` | Uses a CA (either ROOT or INTERMEDIATE) and private key loaded from disk to issue X.509 certificates. | +| `key_file_path` | Path to the CA private key file in PEM format. This path can be relative or absolute. | +| `cert_file_path` | Path to the CA certificate file in PEM format. If Galadriel is using a self-signed CA, cert_file_path should specify the path to a single PEM encoded certificate representing the CA certificate. If not self-signed, cert_file_path should specify the path to a file that must contain one or more certificates necessary to establish a valid certificate chain up the root certificates defined in bundle_file_path. This path can be relative or absolute. | +| `bundle_file_path` | Required when the cert_file_path does not contain a self-signed CA certificate. This is the path to the file containing one or more root CAs. This path can be relative or absolute. | #### Example: diff --git a/pkg/common/cryptoutil/certs.go b/pkg/common/cryptoutil/certs.go index a95c7daa..97a7a533 100644 --- a/pkg/common/cryptoutil/certs.go +++ b/pkg/common/cryptoutil/certs.go @@ -48,6 +48,36 @@ func CertificatesMatch(cert1, cert2 *x509.Certificate) bool { return bytes.Equal(cert1.Raw, cert2.Raw) } +// VerifyCertificateChain checks whether the provided certificate chain can be verified against the given intermediates and root CAs. +// The function will return an error if the verification fails. +func VerifyCertificateChain(certChain, intermediates, roots []*x509.Certificate, currentTime time.Time) error { + intermediatePool := x509.NewCertPool() + rootPool := x509.NewCertPool() + + for _, cert := range certChain[1:] { + intermediatePool.AddCert(cert) + } + for _, intermediate := range intermediates { + intermediatePool.AddCert(intermediate) + } + for _, root := range roots { + rootPool.AddCert(root) + } + + opts := x509.VerifyOptions{ + Roots: rootPool, + Intermediates: intermediatePool, + CurrentTime: currentTime, + } + + leaf := certChain[0] + if _, err := leaf.Verify(opts); err != nil { + return fmt.Errorf("unable to chain the certificate to a trusted CA: %w", err) + } + + return nil +} + // LoadCertificate loads a x509.Certificate from the given path. func LoadCertificate(path string) (*x509.Certificate, error) { certFile, err := os.ReadFile(path) diff --git a/pkg/common/cryptoutil/certs_test.go b/pkg/common/cryptoutil/certs_test.go index 9e603ae7..875c3734 100644 --- a/pkg/common/cryptoutil/certs_test.go +++ b/pkg/common/cryptoutil/certs_test.go @@ -14,6 +14,8 @@ import ( "github.com/stretchr/testify/require" ) +var clk = clock.NewFake() + func TestIsSelfSigned(t *testing.T) { cert, _ := createRootCA(t) assert.True(t, IsSelfSigned(cert)) @@ -30,6 +32,38 @@ func TestCertificatesMatch(t *testing.T) { assert.False(t, CertificatesMatch(cert, cert2)) } +func TestVerifyCertificateChain(t *testing.T) { + now := clk.Now() + + // Generate leaf, intermediate, and root certificates + leaf, intermediate, root := createCertChain(t, DefaultKeyType) + otherRoot, _ := createRootCA(t) + + // Test valid certificate chain + certChain := []*x509.Certificate{leaf, intermediate} + intermediates := []*x509.Certificate{intermediate} + roots := []*x509.Certificate{root, otherRoot} + + err := VerifyCertificateChain(certChain, intermediates, roots, now) + require.NoError(t, err) + + // Test certificate chain with missing root + missingRootChain := []*x509.Certificate{leaf} + missingRootIntermediates := []*x509.Certificate{intermediate} + missingRoots := []*x509.Certificate{} + + err = VerifyCertificateChain(missingRootChain, missingRootIntermediates, missingRoots, now) + require.Error(t, err) + + // Test certificate chain with wrong root + wrongRootChain := []*x509.Certificate{leaf, intermediate} + wrongRootIntermediates := []*x509.Certificate{intermediate} + wrongRootRoots := []*x509.Certificate{otherRoot} + + err = VerifyCertificateChain(wrongRootChain, wrongRootIntermediates, wrongRootRoots, now) + require.Error(t, err) +} + func TestLoadCertificate(t *testing.T) { // not a certificate _, err := LoadCertificate(rsaKeyPath) @@ -75,7 +109,6 @@ func TestEncodeCertificates(t *testing.T) { } func TestCreateX509Template(t *testing.T) { - clk := clock.NewFake() key, err := GenerateSigner(ECP384) require.NoError(t, err) uris := []*url.URL{{Scheme: "https", Host: "domain.test"}} @@ -98,7 +131,6 @@ func TestCreateX509Template(t *testing.T) { } func TestCreateCATemplate(t *testing.T) { - clk := clock.NewFake() key, err := GenerateSigner(ECP384) require.NoError(t, err) name := pkix.Name{CommonName: "test-cn"} @@ -116,7 +148,6 @@ func TestCreateCATemplate(t *testing.T) { } func TestCreateRootCATemplate(t *testing.T) { - clk := clock.NewFake() name := pkix.Name{CommonName: "test-cn"} twoHours := 2 * time.Hour @@ -131,7 +162,6 @@ func TestCreateRootCATemplate(t *testing.T) { } func TestSignX509(t *testing.T) { - clk := clock.NewFake() key, err := GenerateSigner(ECP384) require.NoError(t, err) uris := []*url.URL{{Scheme: "https", Host: "domain.test"}} @@ -155,7 +185,6 @@ func TestSignX509(t *testing.T) { } func TestSelfSignX509(t *testing.T) { - clk := clock.NewFake() name := pkix.Name{CommonName: "root"} tmpl, err := CreateRootCATemplate(clk, name, 5*time.Minute) require.NoError(t, err) @@ -172,7 +201,6 @@ func TestSelfSignX509(t *testing.T) { } func createRootCA(t *testing.T) (*x509.Certificate, crypto.PrivateKey) { - clk := clock.NewFake() name := pkix.Name{CommonName: "root-ca"} tmpl, err := CreateRootCATemplate(clk, name, 5*time.Minute) require.NoError(t, err) @@ -186,18 +214,10 @@ func createRootCA(t *testing.T) (*x509.Certificate, crypto.PrivateKey) { } func createCert(t *testing.T, keyType KeyType) (*x509.Certificate, crypto.PrivateKey) { - clk := clock.NewFake() - key, err := GenerateSigner(keyType) - require.NoError(t, err) - name := pkix.Name{CommonName: "test-cn"} - - tmpl, err := CreateX509Template(clk, key.Public(), name, nil, nil, 1*time.Hour) - require.NoError(t, err) - require.NotNil(t, tmpl) - // create parent certificate for signing parentCert, signingKey := createRootCA(t) + tmpl, key := createCertTemplate(t, keyType, pkix.Name{CommonName: "leaf-cert"}, false) cert, err := SignX509(tmpl, parentCert, signingKey) require.NoError(t, err) require.NotNil(t, cert) @@ -205,6 +225,40 @@ func createCert(t *testing.T, keyType KeyType) (*x509.Certificate, crypto.Privat return cert, key } +func createCertChain(t *testing.T, keyType KeyType) (leaf *x509.Certificate, intermediate *x509.Certificate, root *x509.Certificate) { + root, rootKey := createRootCA(t) + + intermediateTemplate, intermediateKey := createCertTemplate(t, keyType, pkix.Name{CommonName: "intermediate-cert"}, true) + intermediate, err := SignX509(intermediateTemplate, root, rootKey) + require.NoError(t, err) + require.NotNil(t, intermediate) + + certTemplate, _ := createCertTemplate(t, keyType, pkix.Name{CommonName: "leaf-cert"}, false) + leaf, _ = SignX509(certTemplate, intermediate, intermediateKey) + require.NoError(t, err) + require.NotNil(t, leaf) + + return +} + +func createCertTemplate(t *testing.T, keyType KeyType, name pkix.Name, isCa bool) (*x509.Certificate, crypto.PrivateKey) { + key, err := GenerateSigner(keyType) + require.NoError(t, err) + + var tmpl *x509.Certificate + if isCa { + tmpl, err = CreateCATemplate(clk, key.Public(), name, 1*time.Hour) + require.NoError(t, err) + require.NotNil(t, tmpl) + } else { + tmpl, err = CreateX509Template(clk, key.Public(), name, nil, nil, 1*time.Hour) + require.NoError(t, err) + require.NotNil(t, tmpl) + } + + return tmpl, key +} + func readFile(t *testing.T, path string) []byte { data, err := os.ReadFile(path) require.NoError(t, err) diff --git a/pkg/common/x509ca/disk/disk.go b/pkg/common/x509ca/disk/disk.go index a03cf3f3..e4594855 100644 --- a/pkg/common/x509ca/disk/disk.go +++ b/pkg/common/x509ca/disk/disk.go @@ -30,8 +30,9 @@ type X509CA struct { // certificate is the CA certificate for signing X509 certificates. certificate *x509.Certificate - // trustBundle is a collection of trusted certificates. - trustBundle []*x509.Certificate + // upstreamChain contains the intermediates certificates necessary to + // chain back to the upstream trust bundle. + upstreamChain []*x509.Certificate clock clock.Clock logger logrus.FieldLogger @@ -39,11 +40,11 @@ type X509CA struct { // Config is the configuration for a disk-based X509CA. type Config struct { - // The path to the file containing the X.509 CA certificate. + // The path to the file containing the self-signed X.509 CA certificate or a certificate chain of one or more intermediate CAs. CertFilePath string `hcl:"cert_file_path"` // The path to the file containing the X.509 CA private key. KeyFilePath string `hcl:"key_file_path"` - // The path to the file containing the X.509 trust bundle. + // The path to the file containing the X.509 trust bundle (root CAs). BundleFilePath string `hcl:"bundle_file_path"` Clock clock.Clock @@ -82,21 +83,27 @@ func (ca *X509CA) Configure(config *Config) error { return fmt.Errorf("unable to load private key: %v", err) } - cert, err := cryptoutil.LoadCertificate(config.CertFilePath) + certChain, err := cryptoutil.LoadCertificates(config.CertFilePath) if err != nil { return fmt.Errorf("unable to load certificate: %v", err) } + leafCert := certChain[0] - if err := cryptoutil.VerifyCertificatePrivateKey(cert, key); err != nil { + if err := cryptoutil.VerifyCertificatePrivateKey(leafCert, key); err != nil { return fmt.Errorf("certificate verification failed: %w", err) } - if err := ca.processTrustBundle(config, cert); err != nil { + if err := ca.processTrustBundle(config.BundleFilePath, certChain); err != nil { return err } - ca.certificate = cert - ca.signer = key.(crypto.Signer) + ca.certificate = leafCert + + signer, ok := key.(crypto.Signer) + if !ok { + return errors.New("failed to cast key to crypto.Signer") + } + ca.signer = signer return nil } @@ -104,6 +111,14 @@ func (ca *X509CA) Configure(config *Config) error { // IssueX509Certificate issues an X509 certificate using the disk-based private key and ROOT CA certificate. The certificate // is bound to the given public key and subject. func (ca *X509CA) IssueX509Certificate(ctx context.Context, params *x509ca.X509CertificateParams) ([]*x509.Certificate, error) { + // Check if the X509CA is correctly configured + if ca.certificate == nil { + return nil, errors.New("CA certificate is not configured") + } + if ca.signer == nil { + return nil, errors.New("CA signer is not configured") + } + if params.PublicKey == nil { return nil, errors.New(ErrPublicKeyRequired) } @@ -126,6 +141,11 @@ func (ca *X509CA) IssueX509Certificate(ctx context.Context, params *x509ca.X509C return nil, fmt.Errorf("failed to build certificate chain: %w", err) } + ca.logger.WithFields(logrus.Fields{ + "subject": cert.Subject, + "expiry": cert.NotAfter, + }).Info("Successfully issued new X.509 certificate") + return chain, nil } @@ -137,87 +157,38 @@ func (ca *X509CA) buildCertificateChain(leafCert *x509.Certificate) ([]*x509.Cer chain = append(chain, ca.certificate) } - // If the CA has a trust bundle, append the intermediate certificates in the trust bundle to the chain - if len(ca.trustBundle) > 0 { - chain = append(chain, ca.trustBundle...) + // If the CA has an upstream chain, append the intermediate certificates to the chain + if len(ca.upstreamChain) > 0 { + chain = append(chain, ca.upstreamChain...) } return chain, nil } -func (ca *X509CA) processTrustBundle(config *Config, cert *x509.Certificate) error { - if config.BundleFilePath == "" { - return ca.verifySelfSigned(cert) +func (ca *X509CA) processTrustBundle(bundlePath string, certChain []*x509.Certificate) error { + if bundlePath == "" { + return verifySelfSigned(certChain[0]) } - bundle, err := ca.loadTrustBundle(config) + bundle, err := cryptoutil.LoadCertificates(bundlePath) if err != nil { - return err + return fmt.Errorf("unable to load trust bundle: %v", err) } - err = ca.verifyTrustBundle(cert, bundle) - if err != nil { - return err - } - - ca.trustBundle = filterTrustBundle(bundle, cert) - - return nil -} - -func (ca *X509CA) loadTrustBundle(config *Config) ([]*x509.Certificate, error) { - bundle, err := cryptoutil.LoadCertificates(config.BundleFilePath) - if err != nil { - return nil, fmt.Errorf("unable to load trust bundle: %v", err) - } - - return bundle, nil -} - -func (ca *X509CA) verifyTrustBundle(cert *x509.Certificate, bundle []*x509.Certificate) error { - if err := ca.verifyCertificateCanChainToTrustBundle(cert, bundle); err != nil { + leafCert := certChain[0] + intermediates := certChain[1:] + if err := cryptoutil.VerifyCertificateChain([]*x509.Certificate{leafCert}, intermediates, bundle, ca.clock.Now()); err != nil { return fmt.Errorf("certificate chain verification failed: %w", err) } - ca.trustBundle = bundle + ca.upstreamChain = intermediates + return nil } -func (ca *X509CA) verifySelfSigned(cert *x509.Certificate) error { +func verifySelfSigned(cert *x509.Certificate) error { if !cryptoutil.IsSelfSigned(cert) { return errors.New(ErrTrustBundleRequired) } return nil } - -func (ca *X509CA) verifyCertificateCanChainToTrustBundle(cert *x509.Certificate, intermediates []*x509.Certificate) error { - intermediatePool := x509.NewCertPool() - rootPool := x509.NewCertPool() - for _, intermediate := range intermediates { - intermediatePool.AddCert(intermediate) - rootPool.AddCert(intermediate) - } - - opts := x509.VerifyOptions{ - Roots: rootPool, - Intermediates: intermediatePool, - CurrentTime: ca.clock.Now(), - } - - if _, err := cert.Verify(opts); err != nil { - return fmt.Errorf("unable to chain the certificate to a trusted CA: %w", err) - } - - return nil -} - -// filterTrustBundle removes self-signed and duplicate certificates from the provided bundle. -func filterTrustBundle(bundle []*x509.Certificate, cert *x509.Certificate) []*x509.Certificate { - var filteredBundle []*x509.Certificate - for _, certificate := range bundle { - if !cryptoutil.IsSelfSigned(certificate) && !cryptoutil.CertificatesMatch(certificate, cert) { - filteredBundle = append(filteredBundle, certificate) - } - } - return filteredBundle -} diff --git a/pkg/common/x509ca/disk/disk_test.go b/pkg/common/x509ca/disk/disk_test.go index 42cd4e5e..4050d721 100644 --- a/pkg/common/x509ca/disk/disk_test.go +++ b/pkg/common/x509ca/disk/disk_test.go @@ -60,20 +60,20 @@ func TestConfigure(t *testing.T) { expectedBundleLength: 0, }, { - name: "WithIntermediateCAAndTrustBundle", + name: "WithIntermediateChainAndTrustBundle", config: Config{ KeyFilePath: tempDir + "/intermediate-ca-2.key", - CertFilePath: tempDir + "/intermediate-ca-2.crt", + CertFilePath: tempDir + "/chain.crt", BundleFilePath: tempDir + "/bundle.crt", }, expectedBundleLength: 1, }, { - name: "WithIntermediateCADontChainBack", + name: "WithIntermediateCADontChainBackToRootCAInBundle", config: Config{ - KeyFilePath: tempDir + "/other-ca.key", - CertFilePath: tempDir + "/other-ca.crt", - BundleFilePath: tempDir + "/root-ca.crt", + KeyFilePath: tempDir + "/intermediate-ca-2.key", + CertFilePath: tempDir + "/intermediate-ca-2.crt", + BundleFilePath: tempDir + "/bundle.crt", }, err: "unable to chain the certificate to a trusted CA", }, @@ -122,7 +122,7 @@ func TestConfigure(t *testing.T) { assert.Contains(t, err.Error(), tc.err) } else { require.NoError(t, err) - assert.Equal(t, tc.expectedBundleLength, len(ca.trustBundle)) + assert.Equal(t, tc.expectedBundleLength, len(ca.upstreamChain)) } }) } @@ -157,7 +157,7 @@ func TestIssueX509CertificateWithTwoIntermediateCAs(t *testing.T) { tempDir := setupTest(t) config := Config{ KeyFilePath: tempDir + "/intermediate-ca-2.key", - CertFilePath: tempDir + "/intermediate-ca-2.crt", + CertFilePath: tempDir + "/chain.crt", BundleFilePath: tempDir + "/bundle.crt", Clock: clk, } diff --git a/test/certtest/certs.go b/test/certtest/certs.go index bd66ba39..75e1fd6c 100644 --- a/test/certtest/certs.go +++ b/test/certtest/certs.go @@ -107,11 +107,17 @@ func CreateTestCACertificates(t *testing.T, clk clock.Clock) string { err = os.WriteFile(tempDir+"/other-ca.key", cryptoutil.EncodeRSAPrivateKey(rsaKey), keyPerm) require.NoError(t, err) - // create a bundle of all the CA certificates - bundlePEM, err := cryptoutil.EncodeCertificates([]*x509.Certificate{rootCA, intermediateCA, intermediateCA2, otherCA}) + // create a bundle of all the ROOT CA certificates + bundlePEM, err := cryptoutil.EncodeCertificates([]*x509.Certificate{rootCA, otherCA}) require.NoError(t, err) err = os.WriteFile(tempDir+"/bundle.crt", bundlePEM, crtPerm) require.NoError(t, err) + // create a chain of all the intermediate certificates + chainPEM, err := cryptoutil.EncodeCertificates([]*x509.Certificate{intermediateCA2, intermediateCA}) + require.NoError(t, err) + err = os.WriteFile(tempDir+"/chain.crt", chainPEM, crtPerm) + require.NoError(t, err) + return tempDir }