From ae2e077725de086c890895c9558f7db969ee56b9 Mon Sep 17 00:00:00 2001 From: Aaron Blum Date: Sun, 28 Feb 2021 11:39:37 -0500 Subject: [PATCH] server: reduced autogenerated cert key size to 2048 to speed up testing server: aligned generated certificate lifespans with cli package This resolves a break with our `certs` command when attempting to interact with generated certificates. Fixing this will require a larger refactor to come in a later commit. Release justification: low, normalizing existing functionality Release note: None --- pkg/security/auto_tls_init.go | 4 +- pkg/server/auto_tls_init.go | 46 ++++++------ pkg/server/auto_tls_init_test.go | 118 +++++++++++++++++++++---------- 3 files changed, 108 insertions(+), 60 deletions(-) diff --git a/pkg/security/auto_tls_init.go b/pkg/security/auto_tls_init.go index 57b06b0735b0..68a9ca6588e5 100644 --- a/pkg/security/auto_tls_init.go +++ b/pkg/security/auto_tls_init.go @@ -26,8 +26,8 @@ import ( ) // TODO(aaron-crl): This shared a name and purpose with the value in -// pkg/security and should be consolidated. -const defaultKeySize = 4096 +// pkg/cli/cert.go and should be consolidated. +const defaultKeySize = 2048 // notBeforeMargin provides a window to compensate for potential clock skew. const notBeforeMargin = time.Second * 30 diff --git a/pkg/server/auto_tls_init.go b/pkg/server/auto_tls_init.go index c4e781def4f6..748a359d4f43 100644 --- a/pkg/server/auto_tls_init.go +++ b/pkg/server/auto_tls_init.go @@ -28,13 +28,12 @@ import ( "github.com/cockroachdb/errors/oserror" ) -// TODO(aaron-crl): Pluck this from Config. -// Define default CA certificate lifespan of 366 days. -const caCertLifespan = time.Hour * 24 * 366 - -// TODO(aaron-crl): Pluck this from Config. -// Define default service certificate lifespan of 30 days. -const serviceCertLifespan = time.Hour * 24 * 30 +// TODO(aaron-crl): This is an exact copy from `pkg/cli/cert.go` and should +// be refactored to share consts. +// We use 366 days on certificate lifetimes to at least match X years, +// otherwise leap years risk putting us just under. +const defaultCALifetime = 10 * 366 * 24 * time.Hour // ten years +const defaultCertLifetime = 5 * 366 * 24 * time.Hour // five years // Service Name Strings for autogenerated certificates. const serviceNameInterNode = "InterNode Service" @@ -138,7 +137,8 @@ func (sb *ServiceCertificateBundle) loadOrCreateServiceCertificates( serviceKeyPath string, caCertPath string, caKeyPath string, - initLifespan time.Duration, + serviceCertLifespan time.Duration, + caCertLifespan time.Duration, serviceName string, hostnames []string, ) error { @@ -176,7 +176,7 @@ func (sb *ServiceCertificateBundle) loadOrCreateServiceCertificates( } } else if oserror.IsNotExist(err) { // CA cert does not yet exist, create it and its key. - err = sb.createServiceCA(caCertPath, caKeyPath, initLifespan, serviceName) + err = sb.createServiceCA(caCertPath, caKeyPath, caCertLifespan, serviceName) if err != nil { return errors.Wrap( err, "failed to create Service CA", @@ -187,7 +187,7 @@ func (sb *ServiceCertificateBundle) loadOrCreateServiceCertificates( // CA cert and key should now be loaded, create service cert and key. //var hostCert, hostKey []byte sb.HostCertificate, sb.HostKey, err = security.CreateServiceCertAndKey( - initLifespan, + serviceCertLifespan, serviceName, hostnames, sb.CACertificate, @@ -313,7 +313,8 @@ func (b *CertificateBundle) InitializeFromConfig(c base.Config) error { cl.NodeKeyPath(), cl.CACertPath(), cl.CAKeyPath(), - serviceCertLifespan, + defaultCertLifetime, + defaultCALifetime, serviceNameInterNode, []string{c.Addr, c.AdvertiseAddr}, ) @@ -323,12 +324,10 @@ func (b *CertificateBundle) InitializeFromConfig(c base.Config) error { } // Initialize User auth certificates. - // TODO(aaron-crl): Double check that we want to do this. It seems - // like this is covered by the interface certificates? err = b.UserAuth.loadOrCreateUserAuthCACertAndKey( cl.ClientCACertPath(), cl.ClientCAKeyPath(), - caCertLifespan, + defaultCALifetime, serviceNameUserAuth, ) if err != nil { @@ -342,7 +341,8 @@ func (b *CertificateBundle) InitializeFromConfig(c base.Config) error { cl.SQLServiceKeyPath(), cl.SQLServiceCACertPath(), cl.SQLServiceCAKeyPath(), - serviceCertLifespan, + defaultCertLifetime, + defaultCALifetime, serviceNameSQL, // TODO(aaron-crl): Add RPC variable to config or SplitSQLAddr. []string{c.SQLAddr, c.SQLAdvertiseAddr}, @@ -358,7 +358,8 @@ func (b *CertificateBundle) InitializeFromConfig(c base.Config) error { cl.RPCServiceKeyPath(), cl.RPCServiceCACertPath(), cl.RPCServiceCAKeyPath(), - serviceCertLifespan, + defaultCertLifetime, + defaultCALifetime, serviceNameRPC, // TODO(aaron-crl): Add RPC variable to config. []string{c.SQLAddr, c.SQLAdvertiseAddr}, @@ -374,7 +375,8 @@ func (b *CertificateBundle) InitializeFromConfig(c base.Config) error { cl.UIKeyPath(), cl.UICACertPath(), cl.UICAKeyPath(), - serviceCertLifespan, + defaultCertLifetime, + defaultCALifetime, serviceNameUI, []string{c.HTTPAddr, c.HTTPAdvertiseAddr}, ) @@ -473,7 +475,7 @@ func (sb *ServiceCertificateBundle) loadCACertAndKeyIfExists( certPath string, keyPath string, ) error { // TODO(aaron-crl): Possibly add a warning to the log that a CA was not - // found + // found. err := sb.loadCACertAndKey(certPath, keyPath) if oserror.IsNotExist(err) { return nil @@ -549,7 +551,7 @@ func rotateGeneratedCerts(c base.Config) error { err = b.InterNode.rotateServiceCert( cl.NodeCertPath(), cl.NodeKeyPath(), - serviceCertLifespan, + defaultCertLifetime, serviceNameInterNode, []string{c.HTTPAddr, c.HTTPAdvertiseAddr}, ) @@ -565,7 +567,7 @@ func rotateGeneratedCerts(c base.Config) error { err = b.SQLService.rotateServiceCert( cl.SQLServiceCertPath(), cl.SQLServiceKeyPath(), - serviceCertLifespan, + defaultCertLifetime, serviceNameSQL, []string{c.HTTPAddr, c.HTTPAdvertiseAddr}, ) @@ -579,7 +581,7 @@ func rotateGeneratedCerts(c base.Config) error { err = b.RPCService.rotateServiceCert( cl.RPCServiceCertPath(), cl.RPCServiceKeyPath(), - serviceCertLifespan, + defaultCertLifetime, serviceNameRPC, []string{c.HTTPAddr, c.HTTPAdvertiseAddr}, ) @@ -593,7 +595,7 @@ func rotateGeneratedCerts(c base.Config) error { err = b.AdminUIService.rotateServiceCert( cl.UICertPath(), cl.UIKeyPath(), - serviceCertLifespan, + defaultCertLifetime, serviceNameUI, []string{c.HTTPAddr, c.HTTPAdvertiseAddr}, ) diff --git a/pkg/server/auto_tls_init_test.go b/pkg/server/auto_tls_init_test.go index c493be4414ec..8643bcab7bfa 100644 --- a/pkg/server/auto_tls_init_test.go +++ b/pkg/server/auto_tls_init_test.go @@ -67,22 +67,38 @@ func loadAllCertsFromDisk(cfg base.Config) (CertificateBundle, error) { if err != nil { return bundleFromDisk, err } - bundleFromDisk.InterNode.loadOrCreateServiceCertificates( - cl.NodeCertPath(), cl.NodeKeyPath(), "", "", 0, "", []string{}, + + err = bundleFromDisk.InterNode.loadOrCreateServiceCertificates( + cl.NodeCertPath(), cl.NodeKeyPath(), "", "", 0, 0, "", []string{}, ) + if err != nil { + return bundleFromDisk, err + } + // TODO(aaron-crl): Figure out how to handle client auth case. //bundleFromDisk.UserAuth.loadOrCreateServiceCertificates( // cl.ClientCertPath(), cl.ClientKeyPath(), "", "", 0, "", []string{}, //) - bundleFromDisk.SQLService.loadOrCreateServiceCertificates( - cl.SQLServiceCertPath(), cl.SQLServiceKeyPath(), "", "", 0, "", []string{}, + err = bundleFromDisk.SQLService.loadOrCreateServiceCertificates( + cl.SQLServiceCertPath(), cl.SQLServiceKeyPath(), "", "", 0, 0, "", []string{}, ) - bundleFromDisk.RPCService.loadOrCreateServiceCertificates( - cl.RPCServiceCertPath(), cl.RPCServiceKeyPath(), "", "", 0, "", []string{}, + if err != nil { + return bundleFromDisk, err + } + + err = bundleFromDisk.RPCService.loadOrCreateServiceCertificates( + cl.RPCServiceCertPath(), cl.RPCServiceKeyPath(), "", "", 0, 0, "", []string{}, ) - bundleFromDisk.AdminUIService.loadOrCreateServiceCertificates( - cl.UICertPath(), cl.UIKeyPath(), "", "", 0, "", []string{}, + if err != nil { + return bundleFromDisk, err + } + + err = bundleFromDisk.AdminUIService.loadOrCreateServiceCertificates( + cl.UICertPath(), cl.UIKeyPath(), "", "", 0, 0, "", []string{}, ) + if err != nil { + return bundleFromDisk, err + } return bundleFromDisk, nil } @@ -90,13 +106,13 @@ func loadAllCertsFromDisk(cfg base.Config) (CertificateBundle, error) { func certCompareHelper(t *testing.T, desireEqual bool) func([]byte, []byte, string) { if desireEqual { return func(b1 []byte, b2 []byte, certName string) { - if bytes.Compare(b1, b2) != 0 { + if !bytes.Equal(b1, b2) { t.Fatalf("bytes for %s not equal", certName) } } } return func(b1 []byte, b2 []byte, certName string) { - if bytes.Compare(b1, b2) == 0 && b1 != nil { + if bytes.Equal(b1, b2) && b1 != nil { t.Fatalf("bytes for %s were equal", certName) } } @@ -109,42 +125,42 @@ func compareBundleCaCerts( // Compare InterNode CA cert and key. cmp( cb1.InterNode.CACertificate, - cb2.InterNode.CACertificate, "InterNodeCA cert") + cb2.InterNode.CACertificate, serviceNameInterNode+" CA cert") cmp( cb1.InterNode.CAKey, - cb2.InterNode.CAKey, "InterNodeCA key") + cb2.InterNode.CAKey, serviceNameInterNode+" CA key") // Compare UserAuth CA cert and key. cmp( cb1.UserAuth.CACertificate, - cb2.UserAuth.CACertificate, "UserAuth CA cert") + cb2.UserAuth.CACertificate, serviceNameUserAuth+" CA cert") cmp( cb1.UserAuth.CAKey, - cb2.UserAuth.CAKey, "UserAuth CA key") + cb2.UserAuth.CAKey, serviceNameUserAuth+" CA key") // Compare SQL CA cert and key. cmp( cb1.SQLService.CACertificate, - cb2.SQLService.CACertificate, "SQLService CA cert") + cb2.SQLService.CACertificate, serviceNameSQL+" CA cert") cmp( cb1.SQLService.CAKey, - cb2.SQLService.CAKey, "SQLService CA key") + cb2.SQLService.CAKey, serviceNameSQL+" CA key") // Compare RPC CA cert and key. cmp( cb1.RPCService.CACertificate, - cb2.RPCService.CACertificate, "RPCService CA cert") + cb2.RPCService.CACertificate, serviceNameRPC+" CA cert") cmp( cb1.RPCService.CAKey, - cb2.RPCService.CAKey, "RPCService CA key") + cb2.RPCService.CAKey, serviceNameRPC+" CA key") // Compare UI CA cert and key. cmp( cb1.AdminUIService.CACertificate, - cb2.AdminUIService.CACertificate, "AdminUIService CA cert") + cb2.AdminUIService.CACertificate, serviceNameUI+" CA cert") cmp( cb1.AdminUIService.CAKey, - cb2.AdminUIService.CAKey, "AdminUIService CA key") + cb2.AdminUIService.CAKey, serviceNameUI+" CA key") } @@ -155,31 +171,31 @@ func compareBundleServiceCerts( cmp( cb1.InterNode.HostCertificate, - cb2.InterNode.HostCertificate, "InterNode Host cert") + cb2.InterNode.HostCertificate, serviceNameInterNode+" Host cert") cmp( cb1.InterNode.HostKey, - cb2.InterNode.HostKey, "InterNode Host key") + cb2.InterNode.HostKey, serviceNameInterNode+" Host key") cmp( cb1.SQLService.HostCertificate, - cb2.SQLService.HostCertificate, "SQLService Host cert") + cb2.SQLService.HostCertificate, serviceNameSQL+" Host cert") cmp( cb1.SQLService.HostKey, - cb2.SQLService.HostKey, "SQLService Host key") + cb2.SQLService.HostKey, serviceNameSQL+" Host key") cmp( cb1.RPCService.HostCertificate, - cb2.RPCService.HostCertificate, "RPCService Host cert") + cb2.RPCService.HostCertificate, serviceNameRPC+" Host cert") cmp( cb1.RPCService.HostKey, - cb2.RPCService.HostKey, "RPCService Host key") + cb2.RPCService.HostKey, serviceNameRPC+" Host key") cmp( cb1.AdminUIService.HostCertificate, - cb2.AdminUIService.HostCertificate, "AdminUIService Host cert") + cb2.AdminUIService.HostCertificate, serviceNameUI+" Host cert") cmp( cb1.AdminUIService.HostKey, - cb2.AdminUIService.HostKey, "AdminUIService Host key") + cb2.AdminUIService.HostKey, serviceNameUI+" Host key") } // TestDummyInitializeNodeFromBundle is a placeholder for actual testing functions. @@ -192,7 +208,11 @@ func TestDummyInitializeNodeFromBundle(t *testing.T) { if err != nil { t.Fatalf("failed to create test temp dir: %s", err) } - defer os.RemoveAll(tempDir) + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Fatal(err) + } + }() certBundle := CertificateBundle{} cfg := base.Config{ @@ -218,14 +238,19 @@ func TestDummyCertLoader(t *testing.T) { // TestNodeCertRotation tests that the rotation function will overwrite the // expected certificates and fail if they are not there. -// TODO(aaron-crl): correct this func TestRotationOnUnintializedNode(t *testing.T) { + defer leaktest.AfterTest(t)() + // Create a temp dir for all certificate tests. tempDir, err := ioutil.TempDir("", "auto_tls_init_test") if err != nil { t.Fatalf("failed to create test temp dir: %s", err) } - defer os.RemoveAll(tempDir) + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Fatal(err) + } + }() cfg := base.Config{ SSLCertsDir: tempDir, @@ -235,14 +260,17 @@ func TestRotationOnUnintializedNode(t *testing.T) { // Check to see that the only file in dir is the EOF. dir, err := os.Open(cfg.SSLCertsDir) if err != nil { - t.Fatalf("failed to open cfg.SSLCertsDir: %q", cfg.SSLCertsDir) + t.Fatalf( + "failed to open cfg.SSLCertsDir: %q with err: %v", + cfg.SSLCertsDir, + err) } + defer dir.Close() _, err = dir.Readdir(1) if err != io.EOF { // Directory is not empty to start with, this is an error. t.Fatal("files added to cfg.SSLCertsDir when they shouldn't have been") } - dir.Close() err = rotateGeneratedCerts(cfg) if err != nil { @@ -252,12 +280,18 @@ func TestRotationOnUnintializedNode(t *testing.T) { } func TestRotationOnIntializedNode(t *testing.T) { + defer leaktest.AfterTest(t)() + // Create a temp dir for all certificate tests. tempDir, err := ioutil.TempDir("", "auto_tls_init_test") if err != nil { t.Fatalf("failed to create test temp dir: %s", err) } - defer os.RemoveAll(tempDir) + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Fatal(err) + } + }() cfg := base.Config{ SSLCertsDir: tempDir, @@ -284,12 +318,18 @@ func TestRotationOnIntializedNode(t *testing.T) { } func TestRotationOnPartialIntializedNode(t *testing.T) { + defer leaktest.AfterTest(t)() + // Create a temp dir for all certificate tests. tempDir, err := ioutil.TempDir("", "auto_tls_init_test") if err != nil { t.Fatalf("failed to create test temp dir: %s", err) } - defer os.RemoveAll(tempDir) + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Fatal(err) + } + }() cfg := base.Config{ SSLCertsDir: tempDir, @@ -343,12 +383,18 @@ func TestRotationOnPartialIntializedNode(t *testing.T) { // TestRotationOnBrokenIntializedNode in the partially provisioned case (remove the Client and UI CAs). func TestRotationOnBrokenIntializedNode(t *testing.T) { + defer leaktest.AfterTest(t)() + // Create a temp dir for all certificate tests. tempDir, err := ioutil.TempDir("", "auto_tls_init_test") if err != nil { t.Fatalf("failed to create test temp dir: %s", err) } - defer os.RemoveAll(tempDir) + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Fatal(err) + } + }() cfg := base.Config{ SSLCertsDir: tempDir,