From a0be9ccc7c1c6dcba1ac92ff81785231fa0afd41 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 16:20:09 -0400 Subject: [PATCH 01/10] Enforce presence of Certificate Authorities, Certificate file and Key when using LoadTLSServerConfig --- CHANGELOG.next.asciidoc | 4 ++ .../transport/tlscommon/server_config.go | 4 +- .../common/transport/tlscommon/tls_test.go | 38 +++++++++++++++++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index e20aa14bbc28..7ea3920eab43 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -107,6 +107,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix goroutine leak on non-explicit finalization of log input. {pull}12164[12164] - Skipping unparsable log entries from docker json reader {pull}12268[12268] - Require client_auth by default when ssl is enabled for tcp input {pull}12333[12333] +- Require client_auth by default when ssl is enabled for TCP input {pull}12333[12333] +- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}xxx[xxx] *Heartbeat* @@ -136,6 +138,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849] - In the kibana/stats metricset, only log error (don't also index it) if xpack is enabled. {pull}12265[12265] - Require client_auth by default when ssl is enabled for module http metricset server{pull}12333[12333] +- Require client_auth by default when SSL is enabled for module http metricset server{pull}12333[12333] +- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}xxx[xxx] *Packetbeat* diff --git a/libbeat/common/transport/tlscommon/server_config.go b/libbeat/common/transport/tlscommon/server_config.go index 79d4722049f4..1786faea50b3 100644 --- a/libbeat/common/transport/tlscommon/server_config.go +++ b/libbeat/common/transport/tlscommon/server_config.go @@ -31,8 +31,8 @@ type ServerConfig struct { VerificationMode TLSVerificationMode `config:"verification_mode"` // one of 'none', 'full' Versions []TLSVersion `config:"supported_protocols"` CipherSuites []tlsCipherSuite `config:"cipher_suites"` - CAs []string `config:"certificate_authorities"` - Certificate CertificateConfig `config:",inline"` + CAs []string `config:"certificate_authorities" validate:"required"` + Certificate CertificateConfig `config:",inline" validate:"required"` CurveTypes []tlsCurveType `config:"curve_types"` ClientAuth tlsClientAuth `config:"client_authentication"` //`none`, `optional` or `required` } diff --git a/libbeat/common/transport/tlscommon/tls_test.go b/libbeat/common/transport/tlscommon/tls_test.go index b4b1f1dd093a..3d14fb2545ef 100644 --- a/libbeat/common/transport/tlscommon/tls_test.go +++ b/libbeat/common/transport/tlscommon/tls_test.go @@ -167,9 +167,15 @@ func TestApplyWithConfig(t *testing.T) { } func TestServerConfigDefaults(t *testing.T) { + yamlStr := ` + certificate: ca_test.pem + key: ca_test.key + certificate_authorities: [ca_test.pem] + ` var c ServerConfig - config := common.MustNewConfigFrom([]byte(``)) - err := config.Unpack(&c) + config, err := common.NewConfigWithYAML([]byte(yamlStr), "") + require.NoError(t, err) + err = config.Unpack(&c) require.NoError(t, err) tmp, err := LoadTLSServerConfig(&c) require.NoError(t, err) @@ -178,8 +184,8 @@ func TestServerConfigDefaults(t *testing.T) { assert.NotNil(t, cfg) // values not set by default - assert.Len(t, cfg.Certificates, 0) - assert.Nil(t, cfg.ClientCAs) + assert.Len(t, cfg.Certificates, 1) + assert.NotNil(t, cfg.ClientCAs) assert.Len(t, cfg.CipherSuites, 0) assert.Len(t, cfg.CurvePreferences, 0) // values set by default @@ -189,6 +195,30 @@ func TestServerConfigDefaults(t *testing.T) { assert.Equal(t, tls.RequireAndVerifyClientCert, cfg.ClientAuth) } +func TestServerConfigEnsureCA(t *testing.T) { + yamlStr := ` + certificate: ca_test.pem + key: ca_test.key + ` + var c ServerConfig + config, err := common.NewConfigWithYAML([]byte(yamlStr), "") + require.NoError(t, err) + err = config.Unpack(&c) + require.Error(t, err) +} + +func TestServerConfigCertificate(t *testing.T) { + yamlStr := ` + certificate: ca_test.pem + certificate_authorities: [ca_test.pem] + ` + var c ServerConfig + config, err := common.NewConfigWithYAML([]byte(yamlStr), "") + require.NoError(t, err) + err = config.Unpack(&c) + require.Error(t, err) +} + func TestApplyWithServerConfig(t *testing.T) { yamlStr := ` certificate: ca_test.pem From bcc9c419e0cff6f6bb26d46c162bec524e1e6d3c Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 16:51:46 -0400 Subject: [PATCH 02/10] fix changelog --- CHANGELOG.next.asciidoc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 7ea3920eab43..c6fec0e2da9a 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -107,8 +107,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix goroutine leak on non-explicit finalization of log input. {pull}12164[12164] - Skipping unparsable log entries from docker json reader {pull}12268[12268] - Require client_auth by default when ssl is enabled for tcp input {pull}12333[12333] -- Require client_auth by default when ssl is enabled for TCP input {pull}12333[12333] -- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}xxx[xxx] +- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}12355[12355] *Heartbeat* @@ -138,8 +137,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849] - In the kibana/stats metricset, only log error (don't also index it) if xpack is enabled. {pull}12265[12265] - Require client_auth by default when ssl is enabled for module http metricset server{pull}12333[12333] -- Require client_auth by default when SSL is enabled for module http metricset server{pull}12333[12333] -- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}xxx[xxx] +- Require Certificate authorities, Certificate file and key when SSL is enabled for module http server. {pull}12355[12355] *Packetbeat* From 7dc143363813fd705b1776941d7e6d18738f34d4 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 16:53:37 -0400 Subject: [PATCH 03/10] typo --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index c6fec0e2da9a..107bb6386f23 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -137,7 +137,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849] - In the kibana/stats metricset, only log error (don't also index it) if xpack is enabled. {pull}12265[12265] - Require client_auth by default when ssl is enabled for module http metricset server{pull}12333[12333] -- Require Certificate authorities, Certificate file and key when SSL is enabled for module http server. {pull}12355[12355] +- Require Certificate authorities, Certificate file and key when SSL is enabled for module http metricset server. {pull}12355[12355] *Packetbeat* From e476ba2a6c3e1a1137b875405055e8bf59d887c0 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 17:18:44 -0400 Subject: [PATCH 04/10] Update CHANGELOG.next.asciidoc Co-Authored-By: Andrew Kroh --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 107bb6386f23..f645d6e5a179 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -107,7 +107,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Fix goroutine leak on non-explicit finalization of log input. {pull}12164[12164] - Skipping unparsable log entries from docker json reader {pull}12268[12268] - Require client_auth by default when ssl is enabled for tcp input {pull}12333[12333] -- Require Certificate authorities, Certificate file and key when SSL is enabled for the TCP input. {pull}12355[12355] +- Require certificate authorities, certificate file, and key when SSL is enabled for the TCP input. {pull}12355[12355] *Heartbeat* From 4aba3c87e4ebe96fe932490e55b92f0682fd3f55 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:15:05 -0400 Subject: [PATCH 05/10] review --- .../common/transport/tlscommon/server_config.go | 13 +++++++++++-- libbeat/common/transport/tlscommon/tls_test.go | 14 +++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/libbeat/common/transport/tlscommon/server_config.go b/libbeat/common/transport/tlscommon/server_config.go index 1786faea50b3..ce1fb36f3a7d 100644 --- a/libbeat/common/transport/tlscommon/server_config.go +++ b/libbeat/common/transport/tlscommon/server_config.go @@ -19,6 +19,7 @@ package tlscommon import ( "crypto/tls" + "errors" "github.com/joeshaw/multierror" @@ -31,8 +32,8 @@ type ServerConfig struct { VerificationMode TLSVerificationMode `config:"verification_mode"` // one of 'none', 'full' Versions []TLSVersion `config:"supported_protocols"` CipherSuites []tlsCipherSuite `config:"cipher_suites"` - CAs []string `config:"certificate_authorities" validate:"required"` - Certificate CertificateConfig `config:",inline" validate:"required"` + CAs []string `config:"certificate_authorities"` + Certificate CertificateConfig `config:",inline"` CurveTypes []tlsCurveType `config:"curve_types"` ClientAuth tlsClientAuth `config:"client_authentication"` //`none`, `optional` or `required` } @@ -91,6 +92,7 @@ func LoadTLSServerConfig(config *ServerConfig) (*TLSConfig, error) { }, nil } +// Unpack unpacks the TLS Server configuration. func (c *ServerConfig) Unpack(cfg common.Config) error { clientAuthKey := "client_authentication" if !cfg.HasField(clientAuthKey) { @@ -101,6 +103,13 @@ func (c *ServerConfig) Unpack(cfg common.Config) error { if err := cfg.Unpack(&sCfg); err != nil { return err } + + if sCfg.VerificationMode != VerifyNone { + if len(sCfg.CAs) == 0 { + return errors.New("certificate authorities not configured") + } + } + *c = ServerConfig(sCfg) return nil } diff --git a/libbeat/common/transport/tlscommon/tls_test.go b/libbeat/common/transport/tlscommon/tls_test.go index 3d14fb2545ef..74a522edbaf3 100644 --- a/libbeat/common/transport/tlscommon/tls_test.go +++ b/libbeat/common/transport/tlscommon/tls_test.go @@ -207,7 +207,7 @@ func TestServerConfigEnsureCA(t *testing.T) { require.Error(t, err) } -func TestServerConfigCertificate(t *testing.T) { +func TestServerConfigCertificateKey(t *testing.T) { yamlStr := ` certificate: ca_test.pem certificate_authorities: [ca_test.pem] @@ -219,6 +219,18 @@ func TestServerConfigCertificate(t *testing.T) { require.Error(t, err) } +func TestServerConfigCertificate(t *testing.T) { + yamlStr := ` + key: ca_test.key + certificate_authorities: [ca_test.pem] + ` + var c ServerConfig + config, err := common.NewConfigWithYAML([]byte(yamlStr), "") + require.NoError(t, err) + err = config.Unpack(&c) + require.Error(t, err) +} + func TestApplyWithServerConfig(t *testing.T) { yamlStr := ` certificate: ca_test.pem From db72945cd253b6191820ea46cd177901eb9821e5 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:18:39 -0400 Subject: [PATCH 06/10] add explicit test for none case.. --- libbeat/common/transport/tlscommon/tls_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/libbeat/common/transport/tlscommon/tls_test.go b/libbeat/common/transport/tlscommon/tls_test.go index 74a522edbaf3..608607641814 100644 --- a/libbeat/common/transport/tlscommon/tls_test.go +++ b/libbeat/common/transport/tlscommon/tls_test.go @@ -195,6 +195,17 @@ func TestServerConfigDefaults(t *testing.T) { assert.Equal(t, tls.RequireAndVerifyClientCert, cfg.ClientAuth) } +func TestServerConfigSkipCACertificateAndKeyWhenVerifyNone(t *testing.T) { + yamlStr := ` + verification_mode: none + ` + var c ServerConfig + config, err := common.NewConfigWithYAML([]byte(yamlStr), "") + require.NoError(t, err) + err = config.Unpack(&c) + require.NoError(t, err) +} + func TestServerConfigEnsureCA(t *testing.T) { yamlStr := ` certificate: ca_test.pem From 5e4cdf57637d79b05f111bad06350363fc35bea1 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:19:54 -0400 Subject: [PATCH 07/10] one if --- libbeat/common/transport/tlscommon/server_config.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libbeat/common/transport/tlscommon/server_config.go b/libbeat/common/transport/tlscommon/server_config.go index ce1fb36f3a7d..76b621795769 100644 --- a/libbeat/common/transport/tlscommon/server_config.go +++ b/libbeat/common/transport/tlscommon/server_config.go @@ -104,10 +104,8 @@ func (c *ServerConfig) Unpack(cfg common.Config) error { return err } - if sCfg.VerificationMode != VerifyNone { - if len(sCfg.CAs) == 0 { - return errors.New("certificate authorities not configured") - } + if sCfg.VerificationMode != VerifyNone && len(sCfg.CAs) == 0 { + return errors.New("certificate authorities not configured") } *c = ServerConfig(sCfg) From 28b62fe5a29897799652e91a5b246026536b368e Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:25:04 -0400 Subject: [PATCH 08/10] Update CHANGELOG.next.asciidoc --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index f645d6e5a179..a603197f09d9 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -137,7 +137,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849] - In the kibana/stats metricset, only log error (don't also index it) if xpack is enabled. {pull}12265[12265] - Require client_auth by default when ssl is enabled for module http metricset server{pull}12333[12333] -- Require Certificate authorities, Certificate file and key when SSL is enabled for module http metricset server. {pull}12355[12355] +- Require certificate authorities, certificate file and key when SSL is enabled for module http metricset server. {pull}12355[12355] *Packetbeat* From a66d4cf0fae3c1231358bdac12a72e12d9780e6b Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:25:14 -0400 Subject: [PATCH 09/10] Update libbeat/common/transport/tlscommon/server_config.go Co-Authored-By: Andrew Kroh --- libbeat/common/transport/tlscommon/server_config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libbeat/common/transport/tlscommon/server_config.go b/libbeat/common/transport/tlscommon/server_config.go index 76b621795769..2228d12a819a 100644 --- a/libbeat/common/transport/tlscommon/server_config.go +++ b/libbeat/common/transport/tlscommon/server_config.go @@ -105,7 +105,7 @@ func (c *ServerConfig) Unpack(cfg common.Config) error { } if sCfg.VerificationMode != VerifyNone && len(sCfg.CAs) == 0 { - return errors.New("certificate authorities not configured") + return errors.New("certificate_authorities not configured") } *c = ServerConfig(sCfg) From da4120209d42338f59637f18a10dc3f959c7bdaa Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Wed, 29 May 2019 18:27:04 -0400 Subject: [PATCH 10/10] typo --- CHANGELOG.next.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index a603197f09d9..dd33afdefdf3 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -137,7 +137,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Ignore prometheus metrics when their values are NaN or Inf. {pull}12084[12084] {issue}10849[10849] - In the kibana/stats metricset, only log error (don't also index it) if xpack is enabled. {pull}12265[12265] - Require client_auth by default when ssl is enabled for module http metricset server{pull}12333[12333] -- Require certificate authorities, certificate file and key when SSL is enabled for module http metricset server. {pull}12355[12355] +- Require certificate authorities, certificate file, and key when SSL is enabled for module http metricset server. {pull}12355[12355] *Packetbeat*