Fix incorrect auth secret_token key post 8.0 (#6769)

Also changes the e2e tests to check the case where a client tries to use an incorrect auth token to make a requests (which should be rejected). This acts as a test for correct configuration of the APM server with a auth token. Because we did not do this check we missed the misconfiguration since 8.0 in our e2e testing. Marking this as breaking because we shipped versions of ECK that misconfigured 8.0+ APM servers and therefore there might be user installations out there that work only because of this misconfiguration. If we now ship the fix and their clients are not actually setting an auth token then APM server will start rejecting their requests.
elastic · May 9, 2023 · 49e905e · 49e905e
1 parent 21b6b35
commit 49e905e
Show file tree

Hide file tree

Showing 7 changed files with 118 additions and 32 deletions.
diff --git a/pkg/controller/apmserver/config.go b/pkg/controller/apmserver/config.go
@@ -19,15 +19,17 @@ import (
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/certificates"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/reconciler"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/settings"
+	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/version"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/utils/k8s"
 )
 
 const (
 	// DefaultHTTPPort is the (default) port used by ApmServer
 	DefaultHTTPPort = 8200
 
-	APMServerHost        = "apm-server.host"
-	APMServerSecretToken = "apm-server.secret_token" //nolint:gosec
+	APMServerHost              = "apm-server.host"
+	APMServerLegacySecretToken = "apm-server.secret_token"      //nolint:gosec
+	APMServerSecretToken       = "apm-server.auth.secret_token" //nolint:gosec
 
 	APMServerSSLEnabled     = "apm-server.ssl.enabled"
 	APMServerSSLKey         = "apm-server.ssl.key"
@@ -40,11 +42,18 @@ func certificatesDir(associationType commonv1.AssociationType) string {
 	return fmt.Sprintf("config/%s-certs", associationType)
 }
 
+func apmServerSecretTokenKeyFor(v version.Version) string {
+	if v.GTE(version.MinFor(8, 0, 0)) {
+		return APMServerSecretToken
+	}
+	return APMServerLegacySecretToken
+}
+
 // reconcileApmServerConfig reconciles the configuration of the APM server: it first creates the configuration from the APM
 // specification and then reconcile the underlying secret.
-func reconcileApmServerConfig(ctx context.Context, client k8s.Client, as *apmv1.ApmServer) (corev1.Secret, error) {
+func reconcileApmServerConfig(ctx context.Context, client k8s.Client, as *apmv1.ApmServer, version version.Version) (corev1.Secret, error) {
 	// Create a new configuration from the APM object spec.
-	cfg, err := newConfigFromSpec(ctx, client, as)
+	cfg, err := newConfigFromSpec(ctx, client, as, version)
 	if err != nil {
 		return corev1.Secret{}, err
 	}
@@ -68,10 +77,10 @@ func reconcileApmServerConfig(ctx context.Context, client k8s.Client, as *apmv1.
 	return reconciler.ReconcileSecret(ctx, client, expectedConfigSecret, as)
 }
 
-func newConfigFromSpec(ctx context.Context, c k8s.Client, as *apmv1.ApmServer) (*settings.CanonicalConfig, error) {
+func newConfigFromSpec(ctx context.Context, c k8s.Client, as *apmv1.ApmServer, version version.Version) (*settings.CanonicalConfig, error) {
 	cfg := settings.MustCanonicalConfig(map[string]interface{}{
-		APMServerHost:        fmt.Sprintf(":%d", DefaultHTTPPort),
-		APMServerSecretToken: "${SECRET_TOKEN}",
+		APMServerHost:                       fmt.Sprintf(":%d", DefaultHTTPPort),
+		apmServerSecretTokenKeyFor(version): "${SECRET_TOKEN}",
 	})
 
 	esConfig, err := newElasticsearchConfigFromSpec(ctx, c, apmv1.ApmEsAssociation{ApmServer: as})

diff --git a/pkg/controller/apmserver/config_test.go b/pkg/controller/apmserver/config_test.go
@@ -16,6 +16,7 @@ import (
 	apmv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/apm/v1"
 	commonv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/common/v1"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/settings"
+	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/version"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/utils/k8s"
 )
 
@@ -25,11 +26,23 @@ func TestNewConfigFromSpec(t *testing.T) {
 		configOverrides map[string]interface{}
 		esAssocConf     *commonv1.AssociationConf
 		kbAssocConf     *commonv1.AssociationConf
+		version         version.Version
 		wantConf        map[string]interface{}
 		wantErr         bool
 	}{
 		{
-			name: "default config",
+			name:    "default config",
+			version: version.MinFor(8, 0, 0),
+			wantConf: map[string]interface{}{
+				"apm-server.auth.secret_token": "${SECRET_TOKEN}",
+			},
+		},
+		{
+			name:    "default config pre 8.0",
+			version: version.MinFor(7, 0, 0),
+			wantConf: map[string]interface{}{
+				"apm-server.secret_token": "${SECRET_TOKEN}",
+			},
 		},
 		{
 			name: "with overridden config",
@@ -39,6 +52,7 @@ func TestNewConfigFromSpec(t *testing.T) {
 			wantConf: map[string]interface{}{
 				"apm-server.secret_token": "MYSECRET",
 			},
+			version: version.MinFor(7, 0, 0),
 		},
 		{
 			name: "without Elasticsearch CA cert",
@@ -50,10 +64,13 @@ func TestNewConfigFromSpec(t *testing.T) {
 				URL:            "https://test-es-http.default.svc:9200",
 			},
 			wantConf: map[string]interface{}{
+				// version specific auth token
+				"apm-server.auth.secret_token":  "${SECRET_TOKEN}",
 				"output.elasticsearch.hosts":    []string{"https://test-es-http.default.svc:9200"},
 				"output.elasticsearch.username": "elastic",
 				"output.elasticsearch.password": "password",
 			},
+			version: version.MinFor(8, 0, 0),
 		},
 		{
 			name: "with Elasticsearch CA cert",
@@ -65,11 +82,13 @@ func TestNewConfigFromSpec(t *testing.T) {
 				URL:            "https://test-es-http.default.svc:9200",
 			},
 			wantConf: map[string]interface{}{
+				"apm-server.auth.secret_token":                     "${SECRET_TOKEN}",
 				"output.elasticsearch.hosts":                       []string{"https://test-es-http.default.svc:9200"},
 				"output.elasticsearch.username":                    "elastic",
 				"output.elasticsearch.password":                    "password",
 				"output.elasticsearch.ssl.certificate_authorities": []string{"config/elasticsearch-certs/ca.crt"},
 			},
+			version: version.MinFor(8, 0, 0),
 		},
 		{
 			name: "missing auth secret",
@@ -81,6 +100,7 @@ func TestNewConfigFromSpec(t *testing.T) {
 				URL:            "https://test-es-http.default.svc:9200",
 			},
 			wantErr: true,
+			version: version.MinFor(8, 0, 0),
 		},
 		{
 			name: "Kibana and Elasticsearch configuration",
@@ -110,7 +130,10 @@ func TestNewConfigFromSpec(t *testing.T) {
 				"apm-server.kibana.username":                    "apm-kb-user",
 				"apm-server.kibana.password":                    "password-kb-user",
 				"apm-server.kibana.ssl.certificate_authorities": []string{"config/kibana-certs/ca.crt"},
+				// version specific auth token
+				"apm-server.auth.secret_token": "${SECRET_TOKEN}",
 			},
+			version: version.MinFor(8, 0, 0),
 		},
 		{
 			name: "Elasticsearch fully configured and Kibana configuration without CA",
@@ -139,7 +162,10 @@ func TestNewConfigFromSpec(t *testing.T) {
 				"apm-server.kibana.host":     "https://test-kb-http.default.svc:9200",
 				"apm-server.kibana.username": "apm-kb-user",
 				"apm-server.kibana.password": "password-kb-user",
+				// version specific auth token
+				"apm-server.auth.secret_token": "${SECRET_TOKEN}",
 			},
+			version: version.MinFor(8, 0, 0),
 		},
 	}
 
@@ -158,7 +184,7 @@ func TestNewConfigFromSpec(t *testing.T) {
 			apmv1.NewApmEsAssociation(apmServer).SetAssociationConf(tc.esAssocConf)
 			apmv1.NewApmKibanaAssociation(apmServer).SetAssociationConf(tc.kbAssocConf)
 
-			gotConf, err := newConfigFromSpec(context.Background(), client, apmServer)
+			gotConf, err := newConfigFromSpec(context.Background(), client, apmServer, tc.version)
 			if tc.wantErr {
 				require.Error(t, err)
 				return
@@ -198,7 +224,6 @@ func mkConf(t *testing.T, overrides map[string]interface{}) *settings.CanonicalC
 	t.Helper()
 	cfg, err := settings.NewCanonicalConfigFrom(map[string]interface{}{
 		"apm-server.host":            ":8200",
-		"apm-server.secret_token":    "${SECRET_TOKEN}",
 		"apm-server.ssl.certificate": "/mnt/elastic-internal/http-certs/tls.crt",
 		"apm-server.ssl.enabled":     true,
 		"apm-server.ssl.key":         "/mnt/elastic-internal/http-certs/tls.key",

diff --git a/pkg/controller/apmserver/controller.go b/pkg/controller/apmserver/controller.go
@@ -262,7 +262,7 @@ func (r *ReconcileApmServer) doReconcile(ctx context.Context, as *apmv1.ApmServe
 		return results, state // will eventually retry
 	}
 
-	state, err = r.reconcileApmServerDeployment(ctx, state, as)
+	state, err = r.reconcileApmServerDeployment(ctx, state, as, asVersion)
 	if err != nil {
 		if apierrors.IsConflict(err) {
 			log.V(1).Info("Conflict while updating status")

diff --git a/pkg/controller/apmserver/deployment.go b/pkg/controller/apmserver/deployment.go
@@ -19,22 +19,19 @@ import (
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/deployment"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/keystore"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/tracing"
+	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/version"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/utils/k8s"
 )
 
-func (r *ReconcileApmServer) reconcileApmServerDeployment(
-	ctx context.Context,
-	state State,
-	as *apmv1.ApmServer,
-) (State, error) {
+func (r *ReconcileApmServer) reconcileApmServerDeployment(ctx context.Context, state State, as *apmv1.ApmServer, version version.Version) (State, error) {
 	span, ctx := apm.StartSpan(ctx, "reconcile_deployment", tracing.SpanTypeApp)
 	defer span.End()
 
 	tokenSecret, err := reconcileApmServerToken(ctx, r.Client, as)
 	if err != nil {
 		return state, err
 	}
-	reconciledConfigSecret, err := reconcileApmServerConfig(ctx, r.Client, as)
+	reconciledConfigSecret, err := reconcileApmServerConfig(ctx, r.Client, as, version)
 	if err != nil {
 		return state, err
 	}

diff --git a/pkg/controller/common/http/http_client.go b/pkg/controller/common/http/http_client.go
@@ -100,6 +100,16 @@ func IsNotFound(err error) bool {
 	return isHTTPError(err, http.StatusNotFound)
 }
 
+// IsUnauthorized checks whether the error was an HTTP 401 error.
+func IsUnauthorized(err error) bool {
+	return isHTTPError(err, http.StatusUnauthorized)
+}
+
+// IsForbidden checks whether the error was an HTTP 403 error.
+func IsForbidden(err error) bool {
+	return isHTTPError(err, http.StatusForbidden)
+}
+
 func isHTTPError(err error, statusCode int) bool {
 	apiErr := new(APIError)
 	if errors.As(err, &apiErr) {

diff --git a/test/e2e/test/apmserver/checks_apm.go b/test/e2e/test/apmserver/checks_apm.go
@@ -21,6 +21,7 @@ import (
 	apmv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/apm/v1"
 	esv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/elasticsearch/v1"
 	kbv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/kibana/v1"
+	commonhttp "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/http"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/version"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/elasticsearch/client"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/utils/k8s"
@@ -30,6 +31,10 @@ import (
 	"github.com/elastic/cloud-on-k8s/v2/test/e2e/test/kibana"
 )
 
+const sampleEventBody = `{"metadata": { "service": {"name": "1234_service-12a3", "language": {"name": "ecmascript"}, "agent": {"version": "3.14.0", "name": "elastic-node"}}}}
+{ "error": {"id": "abcdef0123456789", "timestamp": 1533827045999000,"log": {"level": "custom log level","message": "Cannot read property 'baz' of undefined"}}}
+{ "metricset": { "samples": { "go.memstats.heap.sys.bytes": { "value": 61235 } }, "timestamp": 1496170422281000 }}`
+
 type apmClusterChecks struct {
 	apmClient        *ApmClient
 	esClient         client.Client
@@ -43,6 +48,7 @@ func (b Builder) CheckStackTestSteps(k *test.K8sClient) test.StepList {
 		a.BuildApmServerClient(b.ApmServer, k),
 		a.CheckApmServerReachable(),
 		a.CheckApmServerVersion(b.ApmServer),
+		a.CheckAPMSecretTokenConfiguration(b.ApmServer, k),
 		a.CheckAPMEventCanBeIndexedInElasticsearch(b.ApmServer, k),
 		a.CheckRUMEventsAPI(b.RUMEnabled()),
 	}.WithSteps(a.CheckAgentConfiguration(b.ApmServer, k))
@@ -147,10 +153,37 @@ func (c *apmClusterChecks) CheckAPMEventCanBeIndexedInElasticsearch(apm apmv1.Ap
 	}
 }
 
+func (c *apmClusterChecks) CheckAPMSecretTokenConfiguration(apm apmv1.ApmServer, k *test.K8sClient) test.Step {
+	return test.Step{
+		Name: "APMServer should reject events with incorrect token setup",
+		Test: test.Eventually(func() error {
+			// All APM Server tests do not have an Elasticsearch reference.
+			if !apm.Spec.ElasticsearchRef.IsDefined() {
+				return nil
+			}
+
+			// as above for the functioning client: fetch the latest APM Server resource from the API because we need to
+			// get resources that are provided by the controller apm part of the status section
+			var updatedApmServer apmv1.ApmServer
+			if err := k.Client.Get(context.Background(), k8s.ExtractNamespacedName(&apm), &updatedApmServer); err != nil {
+				return err
+			}
+			client, err := NewAPMServerClientWithSecretToken(updatedApmServer, k, "not-a-valid-token")
+			if err != nil {
+				return err
+			}
+			ctx, cancel := context.WithTimeout(context.Background(), DefaultReqTimeout)
+			defer cancel()
+			_, err = client.IntakeV2Events(ctx, false, []byte(sampleEventBody))
+			if !commonhttp.IsUnauthorized(err) {
+				return fmt.Errorf("expected error 401 but was %w", err)
+			}
+			return nil
+		}),
+	}
+}
+
 func (c *apmClusterChecks) checkEventsAPI(apm apmv1.ApmServer) error {
-	sampleBody := `{"metadata": { "service": {"name": "1234_service-12a3", "language": {"name": "ecmascript"}, "agent": {"version": "3.14.0", "name": "elastic-node"}}}}
-{ "error": {"id": "abcdef0123456789", "timestamp": 1533827045999000,"log": {"level": "custom log level","message": "Cannot read property 'baz' of undefined"}}}
-{ "metricset": { "samples": { "go.memstats.heap.sys.bytes": { "value": 61235 } }, "timestamp": 1496170422281000 }}`
 	// before sending event, get the document count in the metric, and error index
 	// and save, as it is used to calculate how many docs should be in the index after
 	// the event is sent through APM Server.
@@ -176,7 +209,7 @@ func (c *apmClusterChecks) checkEventsAPI(apm apmv1.ApmServer) error {
 
 	ctx, cancel := context.WithTimeout(context.Background(), DefaultReqTimeout)
 	defer cancel()
-	eventsErrorResponse, err := c.apmClient.IntakeV2Events(ctx, false, []byte(sampleBody))
+	eventsErrorResponse, err := c.apmClient.IntakeV2Events(ctx, false, []byte(sampleEventBody))
 	if err != nil {
 		return err
 	}
@@ -189,15 +222,24 @@ func (c *apmClusterChecks) checkEventsAPI(apm apmv1.ApmServer) error {
 	return nil
 }
 
+func assertHTTP403(t assert.TestingT, err error, msgAndArgs ...interface{}) bool {
+	if !commonhttp.IsForbidden(err) {
+		return assert.Fail(t, fmt.Sprintf("expected HTTP 403 but was %+v", err), msgAndArgs)
+	}
+	return true
+}
+
 func (c *apmClusterChecks) CheckRUMEventsAPI(rumEnabled bool) test.Step {
 	sampleBody := `{"metadata":{"service":{"name":"apm-agent-js","version":"1.0.0","agent":{"name":"rum-js","version":"0.0.0"}}}}
 {"transaction":{"id":"611f4fa950f04631","type":"page-load","duration":643,"context":{"page":{"referer":"http://localhost:8000/test/e2e/","url":"http://localhost:8000/test/e2e/general-usecase/"}},"trace_id":"611f4fa950f04631aaaaaaaaaaaaaaaa","span_count":{"started":1}}}`
 
 	should := "forbidden"
-	assertError := assert.NotNil
+	assertApplicationError := assert.NotNil
+	assertRequestError := assertHTTP403
 	if rumEnabled {
 		should = "accepted"
-		assertError = assert.Nil
+		assertApplicationError = assert.Nil
+		assertRequestError = assert.NoError
 	}
 	//nolint:thelper
 	return test.Step{
@@ -206,9 +248,8 @@ func (c *apmClusterChecks) CheckRUMEventsAPI(rumEnabled bool) test.Step {
 			ctx, cancel := context.WithTimeout(context.Background(), DefaultReqTimeout)
 			defer cancel()
 			eventsErrorResponse, err := c.apmClient.IntakeV2Events(ctx, true, []byte(sampleBody))
-			require.NoError(t, err)
-
-			assertError(t, eventsErrorResponse)
+			assertRequestError(t, err)
+			assertApplicationError(t, eventsErrorResponse)
 		},
 	}
 }

diff --git a/test/e2e/test/apmserver/http_client.go b/test/e2e/test/apmserver/http_client.go
@@ -20,6 +20,7 @@ import (
 
 	apmv1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/apm/v1"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/controller/apmserver"
+	commonhttp "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/http"
 	"github.com/elastic/cloud-on-k8s/v2/pkg/utils/stringsutil"
 	"github.com/elastic/cloud-on-k8s/v2/test/e2e/test"
 )
@@ -45,7 +46,15 @@ func NewApmServerClient(as apmv1.ApmServer, k *test.K8sClient) (*ApmClient, erro
 	if err := k.Client.Get(context.Background(), secretTokenNamespacedName, &secretTokenSecret); err != nil {
 		return nil, err
 	}
+	secretToken, ok := secretTokenSecret.Data[apmserver.SecretTokenKey]
+	if !ok {
+		return nil, fmt.Errorf("secret token not found in secret: %s", as.Status.SecretTokenSecretName)
+	}
+
+	return NewAPMServerClientWithSecretToken(as, k, string(secretToken))
+}
 
+func NewAPMServerClientWithSecretToken(as apmv1.ApmServer, k *test.K8sClient, secretToken string) (*ApmClient, error) {
 	scheme := "http"
 	var caCerts []*x509.Certificate
 	if as.Spec.HTTP.TLS.Enabled() {
@@ -63,11 +72,6 @@ func NewApmServerClient(as apmv1.ApmServer, k *test.K8sClient) (*ApmClient, erro
 
 	client := test.NewHTTPClient(caCerts)
 
-	secretToken, ok := secretTokenSecret.Data[apmserver.SecretTokenKey]
-	if !ok {
-		return nil, fmt.Errorf("secret token not found in secret: %s", as.Status.SecretTokenSecretName)
-	}
-
 	return &ApmClient{
 		client:                   client,
 		endpoint:                 inClusterURL,
@@ -220,7 +224,7 @@ func (c *ApmClient) IntakeV2Events(ctx context.Context, rum bool, payload []byte
 		return nil, err
 	}
 
-	return &eventsErrorResponse, err
+	return &eventsErrorResponse, commonhttp.MaybeAPIError(resp)
 }
 
 // AgentConfig describes an agent configuration