Make client_auth optional by default. (#2284)

Author: simitt

_meta/beat.yml

@@ -75,11 +75,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.

apm-server.docker.yml

@@ -75,11 +75,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.

apm-server.yml

@@ -75,11 +75,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.

beater/beater_test.go

@@ -32,8 +32,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/elastic/beats/libbeat/common/transport/tlscommon"
-
 	"github.com/gofrs/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -43,6 +41,7 @@ import (
 	"github.com/elastic/apm-server/tests/loader"
 	"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/outputs"
 	pubs "github.com/elastic/beats/libbeat/publisher"
 	"github.com/elastic/beats/libbeat/publisher/pipeline"
@@ -78,9 +77,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",
 				},
 				"concurrent_requests": 15,
 				"expvar": map[string]interface{}{
@@ -130,7 +130,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,
@@ -238,7 +238,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,

beater/config.go

@@ -143,6 +143,19 @@ type InstrumentationConfig struct {
 
 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")
 	}

changelogs/6.8.asciidoc

@@ -23,10 +23,6 @@ https://github.com/elastic/apm-server/compare/v6.7.2\...v6.8.0[View commits]
 
 https://github.com/elastic/apm-server/compare/v6.8.0\...v6.8.1[View commits]
 
-[float]
-==== Bug fixes
-- Require client authentication by default when `apm-server.ssl.enabled` is true {pull}2224[2224].
-
 [float]
 ==== Added
-- 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].

tests/system/test_access.py

@@ -193,8 +193,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)
@@ -212,7 +211,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):
@@ -230,9 +230,6 @@ def test_https_auth_cert_ok(self):
 
 
 class TestSSLDefaultSupportedProcotolsTest(TestSecureServerBaseTest):
-    def ssl_overrides(self):
-        return {"ssl_client_authentication": "none"}
-
     def test_tls_v1_0(self):
         self.ssl_connect(protocol=ssl.PROTOCOL_TLSv1)
 
@@ -245,8 +242,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):