From 8dd740f58975bac4937dd448269a80aff5cb75de Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Tue, 18 Dec 2018 15:29:16 -0500 Subject: [PATCH] Be more strict when handling the access token (#9624) Add validations for the Kibana response with the access token, and enforce that the access token is not empty when unpacking the configuration. Since the access token in the keystore could be edited. Fixes: #9621 --- CHANGELOG.asciidoc | 1 + x-pack/libbeat/management/api/enroll.go | 20 ++++++-- x-pack/libbeat/management/api/enroll_test.go | 52 +++++++++++++++----- x-pack/libbeat/management/config.go | 11 +++++ x-pack/libbeat/management/config_test.go | 41 +++++++++++++++ 5 files changed, 110 insertions(+), 15 deletions(-) create mode 100644 x-pack/libbeat/management/config_test.go diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 44c0d379a13e..696b0f2debca 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -56,6 +56,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha1...master[Check the HEAD d - When collecting swap metrics for beats telemetry or system metricbeat module handle cases of free swap being bigger than total swap by assuming no swap is being used. {issue}6271[6271] {pull}9383[9383] - Adding logging traces at debug level when the pipeline client receives the following events: onFilteredOut, onDroppedOnPublish. {pull}9016[9016] - Ignore non index fields in default_field for Elasticsearch. {pull}9549[9549] +- Enforce validation for the Central Management access token. {issue}9621[9621] *Auditbeat* diff --git a/x-pack/libbeat/management/api/enroll.go b/x-pack/libbeat/management/api/enroll.go index c03cb0795ce2..e5331bcf910a 100644 --- a/x-pack/libbeat/management/api/enroll.go +++ b/x-pack/libbeat/management/api/enroll.go @@ -8,10 +8,22 @@ import ( "net/http" "github.com/gofrs/uuid" + "github.com/pkg/errors" "github.com/elastic/beats/libbeat/common" ) +type enrollResponse struct { + AccessToken string `json:"access_token"` +} + +func (e *enrollResponse) Validate() error { + if len(e.AccessToken) == 0 { + return errors.New("empty access_token") + } + return nil +} + // Enroll a beat in central management, this call returns a valid access token to retrieve configurations func (c *Client) Enroll(beatType, beatName, beatVersion, hostname string, beatUUID uuid.UUID, enrollmentToken string) (string, error) { params := common.MapStr{ @@ -21,9 +33,7 @@ func (c *Client) Enroll(beatType, beatName, beatVersion, hostname string, beatUU "host_name": hostname, } - resp := struct { - AccessToken string `json:"access_token"` - }{} + resp := enrollResponse{} headers := http.Header{} headers.Set("kbn-beats-enrollment-token", enrollmentToken) @@ -33,5 +43,9 @@ func (c *Client) Enroll(beatType, beatName, beatVersion, hostname string, beatUU return "", err } + if err := resp.Validate(); err != nil { + return "", err + } + return resp.AccessToken, err } diff --git a/x-pack/libbeat/management/api/enroll_test.go b/x-pack/libbeat/management/api/enroll_test.go index 7f98075af0a2..58fb60df9123 100644 --- a/x-pack/libbeat/management/api/enroll_test.go +++ b/x-pack/libbeat/management/api/enroll_test.go @@ -62,18 +62,46 @@ func TestEnrollValid(t *testing.T) { } func TestEnrollError(t *testing.T) { - beatUUID, err := uuid.NewV4() - if err != nil { - t.Fatal(err) + tests := map[string]struct { + body string + responseCode int + }{ + "invalid enrollment token": { + body: `{"message": "Invalid enrollment token"}`, + responseCode: 400, + }, + "invalid token response": { + body: `{"access_token": ""}`, + responseCode: 200, + }, } - server, client := newServerClientPair(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, `{"message": "Invalid enrollment token"}`, 400) - })) - defer server.Close() - - accessToken, err := client.Enroll("metricbeat", "beatname", "6.3.0", "myhostname.lan", beatUUID, "thisismyenrollmenttoken") - - assert.NotNil(t, err) - assert.Equal(t, "", accessToken) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + beatUUID, err := uuid.NewV4() + if err != nil { + t.Fatal(err) + } + + server, client := newServerClientPair(t, http.HandlerFunc(func( + w http.ResponseWriter, + r *http.Request, + ) { + http.Error(w, test.body, test.responseCode) + })) + defer server.Close() + + accessToken, err := client.Enroll( + "metricbeat", + "beatname", + "6.3.0", + "myhostname.lan", + beatUUID, + "thisismyenrollmenttoken", + ) + + assert.NotNil(t, err) + assert.Equal(t, "", accessToken) + }) + } } diff --git a/x-pack/libbeat/management/config.go b/x-pack/libbeat/management/config.go index 3781938f59cf..e58409321123 100644 --- a/x-pack/libbeat/management/config.go +++ b/x-pack/libbeat/management/config.go @@ -5,6 +5,7 @@ package management import ( + "errors" "io" "text/template" "time" @@ -66,6 +67,8 @@ const ManagedConfigTemplate = ` #xpack.monitoring.elasticsearch: ` +var errEmptyAccessToken = errors.New("access_token is empty, you must reenroll your Beat") + // Config for central management type Config struct { // true when enrolled @@ -81,6 +84,14 @@ type Config struct { Blacklist ConfigBlacklistSettings `config:"blacklist" yaml:"blacklist"` } +// Validate validates the fields in the config. +func (c *Config) Validate() error { + if len(c.AccessToken) == 0 { + return errEmptyAccessToken + } + return nil +} + func defaultConfig() *Config { return &Config{ Period: 60 * time.Second, diff --git a/x-pack/libbeat/management/config_test.go b/x-pack/libbeat/management/config_test.go new file mode 100644 index 000000000000..655f1fe33e0d --- /dev/null +++ b/x-pack/libbeat/management/config_test.go @@ -0,0 +1,41 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +package management + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/elastic/beats/libbeat/common" +) + +func TestConfigValidate(t *testing.T) { + tests := map[string]struct { + config *common.Config + err bool + }{ + "missing access_token": { + config: common.MustNewConfigFrom(map[string]interface{}{}), + err: true, + }, + "access_token is present": { + config: common.MustNewConfigFrom(map[string]interface{}{"access_token": "abc1234"}), + err: false, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + c := defaultConfig() + err := test.config.Unpack(c) + if test.err { + assert.Error(t, err) + return + } + assert.NoError(t, err) + }) + } +}