From c42897bd2ee2232008439dcd10f8ef5eaadd05b5 Mon Sep 17 00:00:00 2001 From: Pier-Hugues Pellerin Date: Tue, 18 Dec 2018 21:27:50 -0500 Subject: [PATCH] Cherry-pick #9624 to 6.x: Be more strict when handling the access token (#9643) Cherry-pick of PR #9624 to 6.x branch. Original message: 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 40abc593edb9..b9102e4ca1fa 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -51,6 +51,7 @@ https://github.com/elastic/beats/compare/v6.5.0...6.x[Check the HEAD diff] - 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] - Ignore non index fields in default_field for Elasticsearch. {pull}9549[9549] - Update Golang to 1.10.6. {pull}9563[9563] +- 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) + }) + } +}