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

The users defined in the VIEW definition are not using the upstream users' definitions, but are using the CDC delivery users. #11837

Open
pepezzzz opened this issue Dec 5, 2024 · 4 comments
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. area/ticdc Issues or PRs related to TiCDC. type/bug The issue is confirmed as a bug.

Comments

@pepezzzz
Copy link

pepezzzz commented Dec 5, 2024

What did you do?

A regular user creates a VIEW, and the user in the VIEW definition is the same as the regular user who created it.

What did you expect to see?

The user in the VIEW definition of the downstream cluster is also a regular user created upstream.

What did you see instead?

The user in the VIEW definition of the downstream cluster is the CDC delivery user.

Versions of the cluster

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

6.5.4

Upstream TiKV version (execute tikv-server --version):

6.5.4

TiCDC version (execute cdc version):

6.5.8
@pepezzzz pepezzzz added area/ticdc Issues or PRs related to TiCDC. type/bug The issue is confirmed as a bug. labels Dec 5, 2024
@CharlesCheung96
Copy link
Contributor

CharlesCheung96 commented Dec 6, 2024

Reproduce

  1. Create a view in upstream TiDB with tidb user
use test
CREATE TABLE `t` (
  `id` bigint NOT NULL PRIMARY KEY,
  `c` char(30) NOT NULL DEFAULT '')
CREATE VIEW v_t AS SELECT `id`  FROM `t`;
  1. Query the view
  • In upstream
CREATE ALGORITHM=UNDEFINED DEFINER=`tidb`@`%` SQL SECURITY DEFINER VIEW `v_t` (`id`) AS SELECT `id` AS
`id` FROM `test`.`t`
  • In downstream
CREATE ALGORITHM=UNDEFINED DEFINER=`cdc`@`%` SQL SECURITY DEFINER VIEW `v_t` (`id`) AS SELECT `id` AS
`id` FROM `test`.`t`

The DEFINER is incorrectly set to cdc user.

Root Cause Analysis:

CREATE VIEW v_t AS SELECT `id` FROM `t`
CREATE ALGORITHM = UNDEFINED DEFINER = CURRENT_USER SQL SECURITY DEFINER VIEW `v_t` AS SELECT `id` FROM `t`;

Solutions

The Definer fields of CreateViewStmt need to be set correctly before restore the ddl query from parser.

Note even if TiCDC generates the correct sql, the following issues must be addressed to properly replicate such event:

  • The user (configured in TiCDC sinkuri) needs super permission to perform such operation.
  • The target definer exists in downstream.

@CharlesCheung96 CharlesCheung96 added affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. labels Dec 8, 2024
@kennytm
Copy link
Contributor

kennytm commented Dec 11, 2024

For reference MySQL encodes the CURRENT_USER into its binlog as metadata (I think we can try it with MySQL -> DM -> TiDB to see what happened):

  • When binary logging is enabled and CURRENT_USER() or CURRENT_USER is used as the definer in any of these statements, MySQL Server ensures that the statement is applied to the same user on both the source and the replica when the statement is replicated. In some cases, such as statements that change passwords, the function reference is expanded before it is written to the binary log, so that the statement includes the user name. For all other cases, the name of the current user on the source is replicated to the replica as metadata, and the replica applies the statement to the current user named in the metadata, rather than to the current user on the replica.

@flowbehappy
Copy link
Collaborator

flowbehappy commented Dec 11, 2024

@benmeadowcroft I think we need to decide

  1. Whether we should support this feature.
  2. How to support it correctly. We can follow how MySQL binlog does.

@benmeadowcroft
Copy link

@flowbehappy my concerns align with the issues raised by @CharlesCheung96 above:

  • The user (configured in TiCDC sinkuri) needs super permission to perform such operation.

The requirement to adopt super permission to support this capability means that it would be difficult for many users to adopt TiCDC while aligning with their desire to adopt a least privilege approach for the TiCDC account. With a richer user model we could work around this, but I do not believe we have that capability today.

  • The target definer exists in downstream.

Given that TiCDC does not currently support replication of system tables or DCL keeping the sets of users consistent across clusters may be challenging.

Given this, I do not think this would be a high priority right now, given the security and operational challenges it would introduce. If those issues were to be addressed, than I think this would be something to consider taking on.

Thoughts on the above reasoning?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. area/ticdc Issues or PRs related to TiCDC. type/bug The issue is confirmed as a bug.
Projects
None yet
Development

No branches or pull requests

5 participants