From 3c9d354043f511f47caff1eacfc627e089c146fe Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 23 Aug 2018 16:02:20 +0800 Subject: [PATCH 1/6] executor: forbid user to drop important system table If a user drop some important tables by mistake, such as mysql.user, it's difficult to recover. In the worst case, the whole cluster fails to bootstrap, we should prevent that. --- executor/ddl.go | 40 ++++++++++++++++++++++++++++++++++++++++ executor/ddl_test.go | 6 ++++++ 2 files changed, 46 insertions(+) diff --git a/executor/ddl.go b/executor/ddl.go index ced59b52fb302..209017df38b55 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -24,6 +24,7 @@ import ( "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/model" "github.com/pingcap/tidb/mysql" + "github.com/pingcap/tidb/privilege/privileges" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/chunk" @@ -154,8 +155,19 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error { return errors.Trace(err) } +var errForbidDrop = errors.New("Hey, drop system database/table, are you sure?") + func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { dbName := model.NewCIStr(s.Name) + + // Protect important system table from been dropped by a mistake. + // I can hardly find a case that a user really need to do this. + if dbName.L == "mysql" { + if !privileges.SkipWithGrant { + return errors.Trace(errForbidDrop) + } + } + err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, dbName) if infoschema.ErrDatabaseNotExists.Equal(err) { if s.IfExists { @@ -179,6 +191,26 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return errors.Trace(err) } +var systemTables = map[string]struct{}{ + "user": struct{}{}, + "db": struct{}{}, + "tables_priv": struct{}{}, + "columns_priv": struct{}{}, + "gc_delete_range": struct{}{}, + "gc_delete_range_done": struct{}{}, +} + +func isSystemTable(schema, table string) bool { + if schema != "mysql" { + return false + } + + if _, ok := systemTables[table]; ok { + return true + } + return false +} + func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { var notExistTables []string for _, tn := range s.Tables { @@ -198,6 +230,14 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { return errors.Trace(err) } + // Protect important system table from been dropped by a mistake. + // I can hardly find a case that a user really need to do this. + if isSystemTable(tn.Schema.L, tn.Name.L) { + if !privileges.SkipWithGrant { + return errors.Trace(errForbidDrop) + } + } + if config.CheckTableBeforeDrop { log.Warnf("admin check table `%s`.`%s` before drop.", fullti.Schema.O, fullti.Name.O) sql := fmt.Sprintf("admin check table `%s`.`%s`", fullti.Schema.O, fullti.Name.O) diff --git a/executor/ddl_test.go b/executor/ddl_test.go index 87c69c5b95ac2..b1bc55bd370ed 100644 --- a/executor/ddl_test.go +++ b/executor/ddl_test.go @@ -128,6 +128,9 @@ func (s *testSuite) TestCreateDropDatabase(c *C) { c.Assert(err.Error(), Equals, plan.ErrNoDB.Error()) _, err = tk.Exec("select * from t;") c.Assert(err.Error(), Equals, plan.ErrNoDB.Error()) + + _, err = tk.Exec("drop database mysql") + c.Assert(err, NotNil) } func (s *testSuite) TestCreateDropTable(c *C) { @@ -137,6 +140,9 @@ func (s *testSuite) TestCreateDropTable(c *C) { tk.MustExec("drop table if exists drop_test") tk.MustExec("create table drop_test (a int)") tk.MustExec("drop table drop_test") + + _, err := tk.Exec("drop table mysql.user") + c.Assert(err, NotNil) } func (s *testSuite) TestCreateDropIndex(c *C) { From a7fd95cba629160b4f4697336f458d628b70fc23 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 23 Aug 2018 16:28:49 +0800 Subject: [PATCH 2/6] fix CI --- executor/ddl.go | 12 ++++++------ privilege/privileges/cache_test.go | 6 ++++++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/executor/ddl.go b/executor/ddl.go index 209017df38b55..be3135341e99d 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -192,12 +192,12 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { } var systemTables = map[string]struct{}{ - "user": struct{}{}, - "db": struct{}{}, - "tables_priv": struct{}{}, - "columns_priv": struct{}{}, - "gc_delete_range": struct{}{}, - "gc_delete_range_done": struct{}{}, + "user": {}, + "db": {}, + "tables_priv": {}, + "columns_priv": {}, + "gc_delete_range": {}, + "gc_delete_range_done": {}, } func isSystemTable(schema, table string) bool { diff --git a/privilege/privileges/cache_test.go b/privilege/privileges/cache_test.go index 723373e80ffa3..809e54709f456 100644 --- a/privilege/privileges/cache_test.go +++ b/privilege/privileges/cache_test.go @@ -200,6 +200,12 @@ func (s *testCacheSuite) TestAbnormalMySQLTable(c *C) { c.Assert(err, IsNil) defer se.Close() + saveSkipWithGrant := privileges.SkipWithGrant + defer func() { + privileges.SkipWithGrant = saveSkipWithGrant + }() + privileges.SkipWithGrant = true + // Simulate the case mysql.user is synchronized from MySQL. mustExec(c, se, "DROP TABLE mysql.user;") mustExec(c, se, "USE mysql;") From 8bbeac76777beae7e3298677fef1b174cebc6a24 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Thu, 23 Aug 2018 19:56:16 +0800 Subject: [PATCH 3/6] address comment --- executor/ddl.go | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/executor/ddl.go b/executor/ddl.go index be3135341e99d..26dae946e5a7d 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -155,7 +155,7 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error { return errors.Trace(err) } -var errForbidDrop = errors.New("Hey, drop system database/table, are you sure?") +var errForbidDrop = errors.New("Drop mysql tables is forbidden") func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { dbName := model.NewCIStr(s.Name) @@ -191,26 +191,6 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return errors.Trace(err) } -var systemTables = map[string]struct{}{ - "user": {}, - "db": {}, - "tables_priv": {}, - "columns_priv": {}, - "gc_delete_range": {}, - "gc_delete_range_done": {}, -} - -func isSystemTable(schema, table string) bool { - if schema != "mysql" { - return false - } - - if _, ok := systemTables[table]; ok { - return true - } - return false -} - func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { var notExistTables []string for _, tn := range s.Tables { @@ -232,7 +212,7 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { // Protect important system table from been dropped by a mistake. // I can hardly find a case that a user really need to do this. - if isSystemTable(tn.Schema.L, tn.Name.L) { + if tn.Schema.L == "mysql" { if !privileges.SkipWithGrant { return errors.Trace(errForbidDrop) } From a9f82e66e2313d9dcb15ba5ad6307a7d34ada5b1 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Mon, 27 Aug 2018 10:52:25 +0800 Subject: [PATCH 4/6] address comment --- executor/ddl.go | 29 ++++++++++++++++++++--------- executor/ddl_test.go | 2 +- privilege/privileges/cache_test.go | 6 ------ 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/executor/ddl.go b/executor/ddl.go index 26dae946e5a7d..acece01077ad5 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -24,7 +24,6 @@ import ( "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/model" "github.com/pingcap/tidb/mysql" - "github.com/pingcap/tidb/privilege/privileges" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/chunk" @@ -155,7 +154,7 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error { return errors.Trace(err) } -var errForbidDrop = errors.New("Drop mysql tables is forbidden") +var errForbidDrop = errors.New("Drop tidb system tables is forbidden") func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { dbName := model.NewCIStr(s.Name) @@ -163,9 +162,7 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { // Protect important system table from been dropped by a mistake. // I can hardly find a case that a user really need to do this. if dbName.L == "mysql" { - if !privileges.SkipWithGrant { - return errors.Trace(errForbidDrop) - } + return errors.Trace(errForbidDrop) } err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, dbName) @@ -191,6 +188,22 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return errors.Trace(err) } +var systemTables = map[string]struct{}{ + "tidb": {}, + "gc_delete_range": {}, + "gc_delete_range_done": {}, +} + +func isSystemTable(schema, table string) bool { + if schema != "mysql" { + return false + } + if _, ok := systemTables[table]; ok { + return true + } + return false +} + func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { var notExistTables []string for _, tn := range s.Tables { @@ -212,10 +225,8 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { // Protect important system table from been dropped by a mistake. // I can hardly find a case that a user really need to do this. - if tn.Schema.L == "mysql" { - if !privileges.SkipWithGrant { - return errors.Trace(errForbidDrop) - } + if isSystemTable(tn.Schema.L, tn.Name.L) { + return errors.Trace(errForbidDrop) } if config.CheckTableBeforeDrop { diff --git a/executor/ddl_test.go b/executor/ddl_test.go index b1bc55bd370ed..4d102ffa0cc20 100644 --- a/executor/ddl_test.go +++ b/executor/ddl_test.go @@ -141,7 +141,7 @@ func (s *testSuite) TestCreateDropTable(c *C) { tk.MustExec("create table drop_test (a int)") tk.MustExec("drop table drop_test") - _, err := tk.Exec("drop table mysql.user") + _, err := tk.Exec("drop table mysql.gc_delete_range") c.Assert(err, NotNil) } diff --git a/privilege/privileges/cache_test.go b/privilege/privileges/cache_test.go index 809e54709f456..723373e80ffa3 100644 --- a/privilege/privileges/cache_test.go +++ b/privilege/privileges/cache_test.go @@ -200,12 +200,6 @@ func (s *testCacheSuite) TestAbnormalMySQLTable(c *C) { c.Assert(err, IsNil) defer se.Close() - saveSkipWithGrant := privileges.SkipWithGrant - defer func() { - privileges.SkipWithGrant = saveSkipWithGrant - }() - privileges.SkipWithGrant = true - // Simulate the case mysql.user is synchronized from MySQL. mustExec(c, se, "DROP TABLE mysql.user;") mustExec(c, se, "USE mysql;") From a0a844ea2f067d8820eb1c9738591fa5aceadb7e Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Wed, 29 Aug 2018 22:41:00 +0800 Subject: [PATCH 5/6] address comment --- executor/ddl.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/executor/ddl.go b/executor/ddl.go index acece01077ad5..6e573854c0163 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -154,15 +154,13 @@ func (e *DDLExec) executeCreateIndex(s *ast.CreateIndexStmt) error { return errors.Trace(err) } -var errForbidDrop = errors.New("Drop tidb system tables is forbidden") - func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { dbName := model.NewCIStr(s.Name) // Protect important system table from been dropped by a mistake. // I can hardly find a case that a user really need to do this. if dbName.L == "mysql" { - return errors.Trace(errForbidDrop) + return errors.New("Drop 'mysql' database is forbidden") } err := domain.GetDomain(e.ctx).DDL().DropSchema(e.ctx, dbName) @@ -226,7 +224,7 @@ func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { // Protect important system table from been dropped by a mistake. // I can hardly find a case that a user really need to do this. if isSystemTable(tn.Schema.L, tn.Name.L) { - return errors.Trace(errForbidDrop) + return errors.Errorf("Drop tidb system table '%s.%s' is forbidden", tn.Schema.L, tn.Name.L) } if config.CheckTableBeforeDrop { From 27b89fd187e56b28b1cc7c245c1b5b35abd42451 Mon Sep 17 00:00:00 2001 From: tiancaiamao Date: Wed, 29 Aug 2018 22:48:53 +0800 Subject: [PATCH 6/6] update comment --- executor/ddl.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/executor/ddl.go b/executor/ddl.go index 6e573854c0163..0f19c49d1a98b 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -186,6 +186,8 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return errors.Trace(err) } +// If one drop those tables by mistake, it's difficult to recover. +// In the worst case, the whole TiDB cluster fails to bootstrap, so we prevent user from dropping them. var systemTables = map[string]struct{}{ "tidb": {}, "gc_delete_range": {},