Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

privilege checker doesn't support wildcard database permissions #7645

Closed
dveeden opened this issue Nov 18, 2022 · 4 comments · Fixed by #7739
Closed

privilege checker doesn't support wildcard database permissions #7645

dveeden opened this issue Nov 18, 2022 · 4 comments · Fixed by #7739
Assignees
Labels
affects-6.1 This bug affects the 6.1.x(LTS) versions. area/dm Issues or PRs related to DM. severity/moderate type/bug The issue is confirmed as a bug.

Comments

@dveeden
Copy link
Contributor

dveeden commented Nov 18, 2022

What did you do?

Task:

name: demo-mysql8-tidb
task-mode: "all"
mysql-instances:
  - source-id: "mysql8"
    block-allow-list: "mysql8-tidb-test"

target-database:
  host: "127.0.0.1"
  port: 4000
  user: "root"
  password: ""

block-allow-list:
  mysql8-tidb-test:
    do-dbs: ["demo_foobar"] 

Source DB privileges:

mysql-8.0.31> SHOW GRANTS FOR 'dmuser'@'%';
+--------------------------------------------------------------------+
| Grants for dmuser@%                                                |
+--------------------------------------------------------------------+
| GRANT REPLICATION SLAVE, REPLICATION CLIENT ON *.* TO `dmuser`@`%` |
| GRANT SELECT ON `demo_%`.* TO `dmuser`@`%`                         |
+--------------------------------------------------------------------+
2 rows in set (0.00 sec)

check-task output:

{
    "result": false,
    "msg": "[code=26005:class=dm-master:scope=internal:level=medium], Message: fail to check synchronization configuration with type: check was failed, please see detail
    	detail: {
		"results": [
			{
				"id": 3,
				"name": "source db dump privilege checker",
				"desc": "check dump privileges of source DB",
				"state": "fail",
				"errors": [
					{
						"severity": "fail",
						"short_error": "lack of Select privilege: {`demo_foobar`.`t1`}; "
					}
				],
				"instruction": "You need grant related privileges.",
				"extra": "address of db instance - 127.0.0.1:8031"
			},
			{
				"id": 2,
				"name": "mysql_version",
				"desc": "check whether mysql version is satisfied",
				"state": "warn",
				"errors": [
					{
						"severity": "warn",
						"short_error": "version suggested less than 8.0.0 but got 8.0.31"
					}
				],
				"extra": "address of db instance - 127.0.0.1:8031"
			}
		],
		"summary": {
			"passed": false,
			"total": 12,
			"successful": 10,
			"failed": 1,
			"warning": 1
		}
	}"
}

What did you expect to see?

Permission check to succeed.

What did you see instead?

lack of Select privilege: {`demo_foobar`.`t1`};

Versions of the cluster

DM version (run dmctl -V or dm-worker -V or dm-master -V):

v6.5.0 master at 9d60d064ef5e681867a60c95cdb2b99c10c0c67c

Upstream MySQL/MariaDB server version:

8.0.31

Downstream TiDB cluster version (execute SELECT tidb_version(); in a MySQL client):

v6.1.2

How did you deploy DM: tiup or manually?

local binary

current status of DM cluster (execute query-status <task-name> in dmctl)

(paste current status of DM cluster here)
@dveeden dveeden added area/dm Issues or PRs related to DM. type/bug The issue is confirmed as a bug. labels Nov 18, 2022
@dveeden
Copy link
Contributor Author

dveeden commented Nov 18, 2022

The problem is that dbName (e.g. demo_%) is checked against the list of dbs in privs.dbs without the % being treated as a wild card.

A naive, insecure and incomplete fix would be the patch below. But this should give some information about where the problem is. This replaces the % with .* (so demo_% becomes demo_.*) and a regexp is build out of that.

diff --git a/dm/pkg/checker/privilege.go b/dm/pkg/checker/privilege.go
index ea97d3d84..04e5e1766 100644
--- a/dm/pkg/checker/privilege.go
+++ b/dm/pkg/checker/privilege.go
@@ -17,6 +17,7 @@ import (
        "context"
        "database/sql"
        "fmt"
+       "regexp"
        "strings"
 
        "github.com/pingcap/errors"
@@ -305,9 +306,15 @@ func VerifyPrivileges(
                                if !ok || privs.needGlobal {
                                        continue
                                }
-                               if _, ok := privs.dbs[dbName]; !ok {
+                               dbNameRegex, err := regexp.Compile(strings.ReplaceAll(dbName, "%", ".*"))
+                               if err != nil {
                                        continue
                                }
+                               for db := range privs.dbs {
+                                       if dbNameRegex.MatchString(db) {
+                                               delete(privs.dbs, db)
+                                       }
+                               }
                                // dumpling could report error if an allow-list table is lack of privilege.
                                // we only check that SELECT is granted on all columns, otherwise we can't SHOW CREATE TABLE
                                if privElem.Priv == mysql.SelectPriv && len(privElem.Cols) != 0 {

@lance6716
Copy link
Contributor

Can you paste the link that how TiDB handles % in privilege?

@ti-chi-bot
Copy link
Member

@lance6716: GitHub didn't allow me to assign the following users: liumengya94.

Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

I found this https://github.com/pingcap/tidb/blob/d3a63e93ea8872ae9ead22ba2c3cce82e6c3bbb2/util/stringutil/string_util.go#L244

/assign @liumengya94

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-6.1 This bug affects the 6.1.x(LTS) versions. area/dm Issues or PRs related to DM. severity/moderate type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants