Make client_auth optional by default. (elastic#2283)

bmorelli25 · Jun 13, 2019 · ae9cc67 · ae9cc67
1 parent 89c0cb5
commit ae9cc67
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 37 deletions.
diff --git a/_meta/beat.yml b/_meta/beat.yml
@@ -65,11 +65,10 @@ apm-server:
     # Configure curve types for ECDHE based cipher suites
     #curve_types: []
 
-    # Configure which types of client authentication are supported.
-    # Options are `none`, `optional`, and `required`.
-    # It is strongly recommended to always set to `required`
-    # Default is `required`.
-    #client_authentication: "required"
+    # Configure which type of client authentication is supported.
+    # Options are `none`, `optional`, and `required`. Default is `optional`.
+    #client_authentication: "optional"
+
 
   #rum:
     # To enable real user monitoring (RUM) support set this to true.

diff --git a/apm-server.docker.yml b/apm-server.docker.yml
@@ -65,11 +65,10 @@ apm-server:
     # Configure curve types for ECDHE based cipher suites
     #curve_types: []
 
-    # Configure which types of client authentication are supported.
-    # Options are `none`, `optional`, and `required`.
-    # It is strongly recommended to always set to `required`
-    # Default is `required`.
-    #client_authentication: "required"
+    # Configure which type of client authentication is supported.
+    # Options are `none`, `optional`, and `required`. Default is `optional`.
+    #client_authentication: "optional"
+
 
   #rum:
     # To enable real user monitoring (RUM) support set this to true.

diff --git a/apm-server.yml b/apm-server.yml
@@ -65,11 +65,10 @@ apm-server:
     # Configure curve types for ECDHE based cipher suites
     #curve_types: []
 
-    # Configure which types of client authentication are supported.
-    # Options are `none`, `optional`, and `required`.
-    # It is strongly recommended to always set to `required`
-    # Default is `required`.
-    #client_authentication: "required"
+    # Configure which type of client authentication is supported.
+    # Options are `none`, `optional`, and `required`. Default is `optional`.
+    #client_authentication: "optional"
+
 
   #rum:
     # To enable real user monitoring (RUM) support set this to true.

diff --git a/beater/beater_test.go b/beater/beater_test.go
@@ -28,12 +28,11 @@ import (
 	"testing"
 	"time"
 
-	"github.com/elastic/beats/libbeat/common/transport/tlscommon"
-
 	"github.com/stretchr/testify/assert"
 
 	"github.com/elastic/beats/libbeat/beat"
 	"github.com/elastic/beats/libbeat/common"
+	"github.com/elastic/beats/libbeat/common/transport/tlscommon"
 	"github.com/elastic/beats/libbeat/logp"
 	"github.com/elastic/beats/libbeat/outputs"
 	pubs "github.com/elastic/beats/libbeat/publisher"
@@ -67,9 +66,10 @@ func TestBeatConfig(t *testing.T) {
 				"capture_personal_data": true,
 				"secret_token":          "1234random",
 				"ssl": map[string]interface{}{
-					"enabled":     true,
-					"key":         "1234key",
-					"certificate": "1234cert",
+					"enabled":               true,
+					"key":                   "1234key",
+					"certificate":           "1234cert",
+					"client_authentication": "none",
 				},
 				"expvar": map[string]interface{}{
 					"enabled": true,
@@ -113,7 +113,7 @@ func TestBeatConfig(t *testing.T) {
 				TLS: &tlscommon.ServerConfig{
 					Enabled:     &truthy,
 					Certificate: outputs.CertificateConfig{Certificate: "1234cert", Key: "1234key"},
-					ClientAuth:  4},
+					ClientAuth:  0},
 				AugmentEnabled: true,
 				Expvar: &ExpvarConfig{
 					Enabled: &truthy,
@@ -185,7 +185,7 @@ func TestBeatConfig(t *testing.T) {
 				TLS: &tlscommon.ServerConfig{
 					Enabled:     &truthy,
 					Certificate: outputs.CertificateConfig{Certificate: "", Key: ""},
-					ClientAuth:  4},
+					ClientAuth:  3},
 				AugmentEnabled: true,
 				Expvar: &ExpvarConfig{
 					Enabled: &truthy,

diff --git a/beater/config.go b/beater/config.go
@@ -127,6 +127,19 @@ func (m *Mode) Unpack(s string) error {
 
 func newConfig(version string, ucfg *common.Config) (*Config, error) {
 	c := defaultConfig(version)
+
+	if ucfg.HasField("ssl") {
+		ssl, err := ucfg.Child("ssl", -1)
+		if err != nil {
+			return nil, err
+		}
+		if !ssl.HasField("client_authentication") {
+			if err := ucfg.SetString("ssl.client_authentication", -1, "optional"); err != nil {
+				return nil, err
+			}
+		}
+	}
+
 	if err := ucfg.Unpack(c); err != nil {
 		return nil, errors.Wrap(err, "Error processing configuration")
 	}

diff --git a/changelogs/7.2.asciidoc b/changelogs/7.2.asciidoc
@@ -10,10 +10,6 @@ https://github.com/elastic/apm-server/compare/7.1\...7.2[View commits]
 
 https://github.com/elastic/apm-server/compare/v7.1.0\...v7.2.0[View commits]
 
-[float]
-==== Bug fixes
-- Require client authentication by default when `apm-server.ssl.enabled` is true {pull}2224[2224].
-
 [float]
 ==== Added
 - Make stacktrace lineno optional {pull}2105[2105].
@@ -24,4 +20,4 @@ https://github.com/elastic/apm-server/compare/v7.1.0\...v7.2.0[View commits]
 - Add ephemeral_id attribute in the metadata {pull}2179[2179].
 - Add ILM support for APM Server, using fixed policies {pull}2099[2099],{pull}2209[2209].
 - Add `setup --index-management` cmd, deprecate `setup --template` cmd {pull}2180[2180],{pull}2099[2099].
-- Support more SSL config options for agent/server communication {pull}2224[2224].
+- Support more SSL config options for agent/server communication {pull}2224[2224],{pull}2281[2281].
diff --git a/tests/system/test_access.py b/tests/system/test_access.py
@@ -154,9 +154,9 @@ def test_https_no_cert_ok(self):
         r = self.send_http_request(verify=self.ca_cert)
         assert r.status_code == 202, r.status_code
 
-    @raises(ChunkedEncodingError)
     def test_http_fails(self):
-        self.send_http_request(protocol='http')
+        with self.assertRaises(Exception):
+            self.send_http_request(protocol='http')
 
     @raises(SSLError)
     def test_https_server_validation_fails(self):
@@ -165,8 +165,7 @@ def test_https_server_validation_fails(self):
 
 
 class TestSSLEnabledOptionalClientVerificationTest(TestSecureServerBaseTest):
-    def ssl_overrides(self):
-        return {"ssl_client_authentication": "optional"}
+    # no ssl_overrides necessary as `optional` is default
 
     def test_https_no_certificate_ok(self):
         r = self.send_http_request(verify=self.ca_cert)
@@ -184,7 +183,8 @@ def test_https_auth_cert_ok(self):
 
 
 class TestSSLEnabledRequiredClientVerificationTest(TestSecureServerBaseTest):
-    # no ssl_overrides necessary as `required` is default
+    def ssl_overrides(self):
+        return {"ssl_client_authentication": "required"}
 
     @raises(SSLError)
     def test_https_no_cert_fails(self):
@@ -202,9 +202,6 @@ def test_https_auth_cert_ok(self):
 
 
 class TestSSLDefaultSupportedProcotolsTest(TestSecureServerBaseTest):
-    def ssl_overrides(self):
-        return {"ssl_client_authentication": "none"}
-
     @raises(ssl.SSLError)
     def test_tls_v1_0(self):
         self.ssl_connect(protocol=ssl.PROTOCOL_TLSv1)
@@ -218,8 +215,7 @@ def test_tls_v1_2(self):
 
 class TestSSLSupportedProcotolsTest(TestSecureServerBaseTest):
     def ssl_overrides(self):
-        return {"ssl_client_authentication": "none",
-                "ssl_supported_protocols": ["TLSv1.2"]}
+        return {"ssl_supported_protocols": ["TLSv1.2"]}
 
     @raises(ssl.SSLError)
     def test_tls_v1_1(self):