Import with Lightning fails when setting `security.enable-sem = true` for TiDB

[DanielZhangQD]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

Deploy TiDB cluster and set security.enable-sem = true for TiDB. Import data with Lightning with local backend

2. What did you expect to see? (Required)

Import succeeds

3. What did you see instead (Required)

[2021/09/01 07:30:05.741 +00:00] [INFO] [lightning.go:162] ["starting HTTP server"] [address="[::]:8289"]
[2021/09/01 07:30:05.741 +00:00] [INFO] [lightning.go:195] ["Lightning server is running, post to /tasks to start an import task"] [address="[::]:8289"]
[2021/09/01 07:30:07.086 +00:00] [WARN] [config.go:529] ["currently only per-task configuration can be applied, global configuration changes can only be made on startup"] ["global config changes"="[lightning.file,lightning.level,lightning.pprof-port,tidb.log-level]"]
[2021/09/01 07:30:07.086 +00:00] [WARN] [config.go:529] ["currently only per-task configuration can be applied, global configuration changes can only be made on startup"] ["global config changes"="[lightning.file,lightning.level,lightning.pprof-port,tidb.log-level]"]
[2021/09/01 07:30:07.087 +00:00] [INFO] [info.go:49] ["Welcome to TiDB-Lightning"] [release-version=v5.2.0] [git-hash=05d2210647d6a1503a8d772477e43b14a024f609] [git-branch=heads/refs/tags/v5.2.0] [go-version=go1.16.4] [utc-build-time="2021-08-27 05:56:11"] [race-enabled=false]
[2021/09/01 07:30:07.087 +00:00] [INFO] [lightning.go:217] [cfg] [cfg="{\"id\":1630481407086876039,\"lightning\":{\"table-concurrency\":8,\"index-concurrency\":4,\"region-concurrency\":4,\"io-concurrency\":5,\"check-requirements\":true,\"meta-schema-name\":\"lightning_metadata\"},\"tidb\":{\"host\":\"dbcluster-01-tidb\",\"port\":4000,\"user\":\"root\",\"status-port\":10080,\"pd-addr\":\"dbcluster-01-pd:2379\",\"sql-mode\":\"ONLY_FULL_GROUP_BY,NO_AUTO_CREATE_USER\",\"tls\":\"false\",\"security\":{\"ca-path\":\"/var/lib/lightning-tls/ca.crt\",\"cert-path\":\"/var/lib/lightning-tls/tls.crt\",\"key-path\":\"/var/lib/lightning-tls/tls.key\",\"redact-info-log\":false},\"max-allowed-packet\":67108864,\"distsql-scan-concurrency\":30,\"build-stats-concurrency\":20,\"index-serial-scan-concurrency\":20,\"checksum-table-concurrency\":2},\"checkpoint\":{\"schema\":\"tidb_lightning_checkpoint\",\"driver\":\"file\",\"enable\":true,\"keep-after-success\":false},\"mydumper\":{\"read-block-size\":65536,\"batch-size\":2147483648,\"batch-import-ratio\":0,\"data-source-dir\":\"s3://tidbcloud-samples/data-ingestion/?region=us-west-2\",\"character-set\":\"auto\",\"csv\":{\"separator\":\",\",\"delimiter\":\"\\\"\",\"terminator\":\"\",\"null\":\"NULL\",\"header\":false,\"trim-last-separator\":false,\"not-null\":false,\"backslash-escape\":true},\"max-region-size\":268435456,\"filter\":[\"*.*\",\"!mysql.*\",\"!sys.*\",\"!INFORMATION_SCHEMA.*\",\"!PERFORMANCE_SCHEMA.*\",\"!METRICS_SCHEMA.*\",\"!INSPECTION_SCHEMA.*\"],\"files\":null,\"no-schema\":false,\"case-sensitive\":false,\"strict-format\":true,\"default-file-rules\":true,\"ignore-data-columns\":null},\"tikv-importer\":{\"addr\":\"\",\"backend\":\"local\",\"on-duplicate\":\"replace\",\"max-kv-pairs\":4096,\"send-kv-pairs\":32768,\"region-split-size\":100663296,\"sorted-kv-dir\":\"/etc/endpoint\",\"disk-quota\":9223372036854775807,\"range-concurrency\":16,\"duplicate-detection\":false,\"engine-mem-cache-size\":536870912,\"local-writer-mem-cache-size\":134217728},\"post-restore\":{\"checksum\":\"required\",\"analyze\":\"required\",\"level-1-compact\":false,\"post-process-at-last\":true,\"compact\":false},\"cron\":{\"switch-mode\":\"5m0s\",\"log-progress\":\"5m0s\",\"check-disk-quota\":\"1m0s\"},\"routes\":null,\"security\":{\"ca-path\":\"/var/lib/lightning-tls/ca.crt\",\"cert-path\":\"/var/lib/lightning-tls/tls.crt\",\"key-path\":\"/var/lib/lightning-tls/tls.key\",\"redact-info-log\":false},\"black-white-list\":{\"do-tables\":null,\"do-dbs\":null,\"ignore-tables\":null,\"ignore-dbs\":null}}"]
[2021/09/01 07:30:07.096 +00:00] [ERROR] [lightning.go:208] ["tidb lightning encountered error"] [error="Error 1227: Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation"] [errorVerbose="Error 1227: Access denied; you need (at least one of) the RESTRICTED_VARIABLES_ADMIN privilege(s) for this operation\ngithub.com/pingcap/errors.AddStack\n\t/nfs/cache/mod/github.com/pingcap/errors@v0.11.5-0.20210425183316-da1aaba5fb63/errors.go:174\ngithub.com/pingcap/errors.Trace\n\t/nfs/cache/mod/github.com/pingcap/errors@v0.11.5-0.20210425183316-da1aaba5fb63/juju_adaptor.go:15\ngithub.com/pingcap/tidb/br/pkg/lightning/common.(*MySQLConnectParam).Connect\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/pkg/lightning/common/util.go:79\ngithub.com/pingcap/tidb/br/pkg/lightning/restore.DBFromConfig\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/pkg/lightning/restore/tidb.go:102\ngithub.com/pingcap/tidb/br/pkg/lightning.(*Lightning).run\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/pkg/lightning/lightning.go:264\ngithub.com/pingcap/tidb/br/pkg/lightning.(*Lightning).RunServer\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/pkg/lightning/lightning.go:205\nmain.main.func2\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/cmd/tidb-lightning/main.go:79\nmain.main\n\t/home/jenkins/agent/workspace/optimization-build-tidb-linux-amd/go/src/github.com/pingcap/br/br/cmd/tidb-lightning/main.go:86\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:225\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1371"]

4. What is your TiDB version? (Required)

v5.2.0

[kennytm]

Lightning must be able to read the global variable @@tidb_row_format_version. IMO SEM is all right to prevent writing to the variable, but should not restrict reading it.

For now to workaround this issue, the user running Lightning should be granted RESTRICTED_VARIABLES_ADMIN privilege.

[morgo]

@SunRunAway what do you think about relaxing the privilege requirement on tidb_row_format_version?

[SunRunAway]

I think the app developer rarely needs to know tidb_row_format_version. So IMO keeping tidb_row_format_version as protected and let Lightning get RESTRICTED_VARIABLES_ADMIN is fine when SEM is enabled.

[SunRunAway]

So I do not think it is a bug of TiDB. We just need to document the list of privileges that lightning requires when SEM is enabled in the documentation describing SEM. @kennytm @DanielZhangQD Are you ok with it?

[kennytm]

@SunRunAway It is OK for Lightning.

(The variable is used just for Local backend, and with Local backend the user needs to have direct access to PD and TiKV. Given the direct access a malicious user can just spawn their own TiDB instance with all restriction removed anyway.)

Though IMO most of the hidden variables have no security consequence by making them read-only. The only variables which absolutely must be hidden from the list is just @@tidb_config, and maybe @@tidb_slow_query_file too.

[SunRunAway]

Yes. We can either keep them hidden or read-only. I think both make sense. So I'd like to keep it hidden from the app developer unless some unrefusable reasons come.

[glorv]

@SunRunAway @kennytm Is there a conclution for this issue?

[SunRunAway]

The current design and code is ok. It is better to document the list of privileges that lightning requires when SEM is enabled.

[glorv]

@morgo Seems there is little document for SEM feature in tidb's document? So do we need to add extra note in tidb-lightning's privilege requirement or just leave it as it is. PTAL

[morgo]

@glorv SEM is mostly for our own internal usage, so it is something we can leave as is for now. But in future the plan is to list all of the privileges that TiDB supports and what they control. Once this is added, each statement reference page should also ideally refer back to the privileges it requires.

[glorv]

@glorv SEM is mostly for our own internal usage, so it is something we can leave as is for now. But in future the plan is to list all of the privileges that TiDB supports and what they control. Once this is added, each statement reference page should also ideally refer back to the privileges it requires.

Thanks for the reply. I will close this issue since there is nothing to do currently.

[github-actions[bot]]

Please check whether the issue should be labeled with 'affects-x.y' or 'fixes-x.y.z', and then remove 'needs-more-info' label.

[glorv]

This issue do not affect any version as it's expected behaviour.