From 4f59553a7ffb8e3e919cfbdf22fb4b759ee6a140 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 2 Jul 2020 11:53:51 +0800 Subject: [PATCH 1/2] tlscommon: require cert in ServerConfig.Validate It does not make sense to configure server-side TLS without specifying a certificate and key pair. Check that both a certificate and key are configured. We were previously checking that both or neither were specified. --- .../common/transport/tlscommon/server_config.go | 8 ++++++++ libbeat/common/transport/tlscommon/tls.go | 15 +++++---------- libbeat/common/transport/tlscommon/tls_test.go | 9 ++++++++- libbeat/common/transport/tlscommon/types.go | 8 ++++---- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/libbeat/common/transport/tlscommon/server_config.go b/libbeat/common/transport/tlscommon/server_config.go index 3cea793eaab..866d6e3c28c 100644 --- a/libbeat/common/transport/tlscommon/server_config.go +++ b/libbeat/common/transport/tlscommon/server_config.go @@ -113,6 +113,14 @@ func (c *ServerConfig) Unpack(cfg common.Config) error { // Validate values the TLSConfig struct making sure certificate sure we have both a certificate and // a key. func (c *ServerConfig) Validate() error { + if c.IsEnabled() { + // c.Certificate.Validate() ensures that both a certificate and key + // are specified, or neither are specified. For server-side TLS we + // require both to be specified. + if c.Certificate.Certificate == "" { + return ErrCertificateUnspecified + } + } return c.Certificate.Validate() } diff --git a/libbeat/common/transport/tlscommon/tls.go b/libbeat/common/transport/tlscommon/tls.go index 67661c89bd4..5dea417637e 100644 --- a/libbeat/common/transport/tlscommon/tls.go +++ b/libbeat/common/transport/tlscommon/tls.go @@ -33,18 +33,13 @@ const logSelector = "tls" // LoadCertificate will load a certificate from disk and return a tls.Certificate or error func LoadCertificate(config *CertificateConfig) (*tls.Certificate, error) { + if err := config.Validate(); err != nil { + return nil, err + } + certificate := config.Certificate key := config.Key - - hasCertificate := certificate != "" - hasKey := key != "" - - switch { - case hasCertificate && !hasKey: - return nil, ErrCertificateNoKey - case !hasCertificate && hasKey: - return nil, ErrKeyNoCertificate - case !hasCertificate && !hasKey: + if certificate == "" { return nil, nil } diff --git a/libbeat/common/transport/tlscommon/tls_test.go b/libbeat/common/transport/tlscommon/tls_test.go index 94e74a49b92..b89308494e0 100644 --- a/libbeat/common/transport/tlscommon/tls_test.go +++ b/libbeat/common/transport/tlscommon/tls_test.go @@ -172,9 +172,13 @@ func TestApplyWithConfig(t *testing.T) { func TestServerConfigDefaults(t *testing.T) { t.Run("when CA is not explicitly set", func(t *testing.T) { var c ServerConfig - config := common.MustNewConfigFrom([]byte(``)) + config := common.MustNewConfigFrom(` +certificate: mycert.pem +key: mykey.pem +`) err := config.Unpack(&c) require.NoError(t, err) + c.Certificate = CertificateConfig{} // prevent reading non-existent files tmp, err := LoadTLSServerConfig(&c) require.NoError(t, err) @@ -196,10 +200,13 @@ func TestServerConfigDefaults(t *testing.T) { yamlStr := ` certificate_authorities: [ca_test.pem] + certificate: mycert.pem + key: mykey.pem ` var c ServerConfig config, err := common.NewConfigWithYAML([]byte(yamlStr), "") err = config.Unpack(&c) + c.Certificate = CertificateConfig{} // prevent reading non-existent files require.NoError(t, err) tmp, err := LoadTLSServerConfig(&c) require.NoError(t, err) diff --git a/libbeat/common/transport/tlscommon/types.go b/libbeat/common/transport/tlscommon/types.go index 93cdf95464e..3c14f1f1ca9 100644 --- a/libbeat/common/transport/tlscommon/types.go +++ b/libbeat/common/transport/tlscommon/types.go @@ -29,10 +29,10 @@ var ( ErrNotACertificate = errors.New("file is not a certificate") // ErrCertificateNoKey indicate a configuration error with missing key file - ErrCertificateNoKey = errors.New("key file not configured") + ErrKeyUnspecified = errors.New("key file not configured") // ErrKeyNoCertificate indicate a configuration error with missing certificate file - ErrKeyNoCertificate = errors.New("certificate file not configured") + ErrCertificateUnspecified = errors.New("certificate file not configured") ) var tlsCipherSuites = map[string]tlsCipherSuite{ @@ -261,9 +261,9 @@ func (c *CertificateConfig) Validate() error { switch { case hasCertificate && !hasKey: - return ErrCertificateNoKey + return ErrKeyUnspecified case !hasCertificate && hasKey: - return ErrKeyNoCertificate + return ErrCertificateUnspecified } return nil } From 46c518c10d07d92c0e30695a1eefd2b5a9a8e35f Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Thu, 2 Jul 2020 12:28:01 +0800 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.next.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 331f348f992..70ee3f151b3 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -138,6 +138,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix redis key setting not allowing upper case characters. {pull}18854[18854] {issue}18640[18640] - Fix config reload metrics (`libbeat.config.module.start/stops/running`). {pull}19168[19168] - Fix metrics hints builder to avoid wrong container metadata usage when port is not exposed {pull}18979[18979] +- Server-side TLS config now validates certificate and key are both specified {pull}19584[19584] *Auditbeat*