From 2c9db6b68340be6db0083388fbecc6bd51ba83d1 Mon Sep 17 00:00:00 2001 From: catsby Date: Mon, 3 Feb 2020 14:33:21 -0600 Subject: [PATCH 1/3] database/influx: fix panic when trying to revoke user Guard against other nil responses --- .../database/influxdb/connection_producer.go | 2 +- plugins/database/influxdb/influxdb.go | 42 +++++++++------ plugins/database/influxdb/influxdb_test.go | 52 ++++++++++++++++++- 3 files changed, 77 insertions(+), 19 deletions(-) diff --git a/plugins/database/influxdb/connection_producer.go b/plugins/database/influxdb/connection_producer.go index 84f93bc81453..6c16c36b8e36 100644 --- a/plugins/database/influxdb/connection_producer.go +++ b/plugins/database/influxdb/connection_producer.go @@ -249,7 +249,7 @@ func isUserAdmin(cli influx.Client, user string) (bool, error) { if err != nil { return false, err } - if response.Error() != nil { + if response != nil && response.Error() != nil { return false, response.Error() } for _, res := range response.Results { diff --git a/plugins/database/influxdb/influxdb.go b/plugins/database/influxdb/influxdb.go index e77adf5f16db..60a2b9915735 100644 --- a/plugins/database/influxdb/influxdb.go +++ b/plugins/database/influxdb/influxdb.go @@ -129,21 +129,25 @@ func (i *Influxdb) CreateUser(ctx context.Context, statements dbplugin.Statement "password": password, }), "", "") response, err := cli.Query(q) - if err != nil && response.Error() != nil { - for _, stmt := range rollbackIFQL { - for _, query := range strutil.ParseArbitraryStringSlice(stmt, ";") { - query = strings.TrimSpace(query) - - if len(query) == 0 { - continue - } - q := influx.NewQuery(dbutil.QueryHelper(query, map[string]string{ - "username": username, - }), "", "") - - response, err := cli.Query(q) - if err != nil && response.Error() != nil { - return "", "", err + if err != nil { + if response != nil && response.Error() != nil { + for _, stmt := range rollbackIFQL { + for _, query := range strutil.ParseArbitraryStringSlice(stmt, ";") { + query = strings.TrimSpace(query) + + if len(query) == 0 { + continue + } + q := influx.NewQuery(dbutil.QueryHelper(query, map[string]string{ + "username": username, + }), "", "") + + response, err := cli.Query(q) + if err != nil { + if response != nil && response.Error() != nil { + return "", "", err + } + } } } } @@ -190,7 +194,9 @@ func (i *Influxdb) RevokeUser(ctx context.Context, statements dbplugin.Statement }), "", "") response, err := cli.Query(q) result = multierror.Append(result, err) - result = multierror.Append(result, response.Error()) + if response != nil { + result = multierror.Append(result, response.Error()) + } } } return result.ErrorOrNil() @@ -230,7 +236,9 @@ func (i *Influxdb) RotateRootCredentials(ctx context.Context, statements []strin }), "", "") response, err := cli.Query(q) result = multierror.Append(result, err) - result = multierror.Append(result, response.Error()) + if response != nil { + result = multierror.Append(result, response.Error()) + } } } diff --git a/plugins/database/influxdb/influxdb_test.go b/plugins/database/influxdb/influxdb_test.go index 0894b6da7611..8a552d66b6b1 100644 --- a/plugins/database/influxdb/influxdb_test.go +++ b/plugins/database/influxdb/influxdb_test.go @@ -193,6 +193,56 @@ func TestMyInfluxdb_RenewUser(t *testing.T) { } } +// TestInfluxdb_RevokeDeletedUser tests attempting to revoke a user that was +// deleted externally. Guards against a panic, see +// https://github.com/hashicorp/vault/issues/6734 +func TestInfluxdb_RevokeDeletedUser(t *testing.T) { + if os.Getenv("VAULT_ACC") == "" { + t.SkipNow() + } + cleanup, address, port := prepareInfluxdbTestContainer(t) + + connectionDetails := map[string]interface{}{ + "host": address, + "port": port, + "username": "influx-root", + "password": "influx-root", + } + + db := new() + _, err := db.Init(context.Background(), connectionDetails, true) + if err != nil { + t.Fatalf("err: %s", err) + } + + statements := dbplugin.Statements{ + Creation: []string{testInfluxRole}, + } + + usernameConfig := dbplugin.UsernameConfig{ + DisplayName: "test", + RoleName: "test", + } + + username, password, err := db.CreateUser(context.Background(), statements, usernameConfig, time.Now().Add(time.Minute)) + if err != nil { + t.Fatalf("err: %s", err) + } + + if err = testCredsExist(t, address, port, username, password); err != nil { + t.Fatalf("Could not connect with new credentials: %s", err) + } + + // call cleanup to remove database + cleanup() + + // attempt to revoke the user after database is gone + err = db.RevokeUser(context.Background(), statements, username) + if err == nil { + t.Fatalf("Expected err, got nil") + } +} + func TestInfluxdb_RevokeUser(t *testing.T) { if os.Getenv("VAULT_ACC") == "" { t.SkipNow() @@ -301,7 +351,7 @@ func testCredsExist(t testing.TB, address string, port int, username, password s if err != nil { return errwrap.Wrapf("error querying influxdb server: {{err}}", err) } - if response.Error() != nil { + if response != nil && response.Error() != nil { return errwrap.Wrapf("error using the correct influx database: {{err}}", response.Error()) } return nil From 622962afa342a5af17f539ca10afe12971370ef4 Mon Sep 17 00:00:00 2001 From: catsby Date: Tue, 4 Feb 2020 12:41:30 -0600 Subject: [PATCH 2/3] return an error if response is nil, which is unlikely but best safe than sorry --- plugins/database/influxdb/connection_producer.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugins/database/influxdb/connection_producer.go b/plugins/database/influxdb/connection_producer.go index 6c16c36b8e36..8a14325ac388 100644 --- a/plugins/database/influxdb/connection_producer.go +++ b/plugins/database/influxdb/connection_producer.go @@ -249,7 +249,10 @@ func isUserAdmin(cli influx.Client, user string) (bool, error) { if err != nil { return false, err } - if response != nil && response.Error() != nil { + if response == nil { + return false, fmt.Errorf("empty response") + } + if response.Error() != nil { return false, response.Error() } for _, res := range response.Results { From 37b0123384250105a06befa8abbcdf357ea56ff3 Mon Sep 17 00:00:00 2001 From: catsby Date: Wed, 5 Feb 2020 06:56:05 -0600 Subject: [PATCH 3/3] refactor a deeply nested statement into a function --- plugins/database/influxdb/influxdb.go | 45 ++++++++++++++++----------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/plugins/database/influxdb/influxdb.go b/plugins/database/influxdb/influxdb.go index 60a2b9915735..531798f8fe54 100644 --- a/plugins/database/influxdb/influxdb.go +++ b/plugins/database/influxdb/influxdb.go @@ -131,25 +131,7 @@ func (i *Influxdb) CreateUser(ctx context.Context, statements dbplugin.Statement response, err := cli.Query(q) if err != nil { if response != nil && response.Error() != nil { - for _, stmt := range rollbackIFQL { - for _, query := range strutil.ParseArbitraryStringSlice(stmt, ";") { - query = strings.TrimSpace(query) - - if len(query) == 0 { - continue - } - q := influx.NewQuery(dbutil.QueryHelper(query, map[string]string{ - "username": username, - }), "", "") - - response, err := cli.Query(q) - if err != nil { - if response != nil && response.Error() != nil { - return "", "", err - } - } - } - } + attemptRollback(cli, username, rollbackIFQL) } return "", "", err } @@ -158,6 +140,31 @@ func (i *Influxdb) CreateUser(ctx context.Context, statements dbplugin.Statement return username, password, nil } +// attemptRollback will attempt to roll back user creation if an error occurs in +// CreateUser +func attemptRollback(cli influx.Client, username string, rollbackStatements []string) error { + for _, stmt := range rollbackStatements { + for _, query := range strutil.ParseArbitraryStringSlice(stmt, ";") { + query = strings.TrimSpace(query) + + if len(query) == 0 { + continue + } + q := influx.NewQuery(dbutil.QueryHelper(query, map[string]string{ + "username": username, + }), "", "") + + response, err := cli.Query(q) + if err != nil { + if response != nil && response.Error() != nil { + return err + } + } + } + } + return nil +} + // RenewUser is not supported on Influxdb, so this is a no-op. func (i *Influxdb) RenewUser(ctx context.Context, statements dbplugin.Statements, username string, expiration time.Time) error { // NOOP