(8.0) PXC-4709: User is wrongly replicated to other nodes causing inconsistencies

[kamil-holubicki]

https://perconadev.atlassian.net/browse/PXC-4709

Problem:
Node is evicted from the cluster by inconsistency voting protocol. It happens when CREATE/ALTER USER is executed by the user without AUTHENTICATION_POLICY_ADMIN privilege.

Cause:
TOI is replicated before actual execution. On source node it fails, but on replicated node, it is executed by wsrep_applier thread working in root user context, so it doesn't fail remotely. It kicks-in inconsistency voting protocol which evicts the node from the cluster.

Solution:
On the Source side validate CREATE/ALTER USER against authentication policy before replicating. However, continue the normal flow, but skip TOI if validation fails.
This is to keep original errors/warnings generated (and used for inconsistency voting). Eg. In case of policy violation and nonexistent plugin happening on the same time, we want ER_PLUGIN_IS_NOT_LOADED to be reported as the original flow does.

[dlenev]

Hello Kamil!

After taking an initial look at your patch I have a few questions/proposals.
Please see below.

Note that I plan to continue looking at it, so more questions/proposals
might pop-up.

[dlenev]

Hello Kamil!

I have a few more questions/comments about your patch.
Please see below.

[dlenev]

Could you please elaborate in what scenario we modify acl_cache in the code below so WRITE_MODE lock becomes necessary? Conceptually the fact that correctness check has a side effect on ACL cache bothers me. This is not how ACL cache normally works AFAIU.

[kamil-holubicki]

This is the callstack:

insert_entry_in_db_cache  <--- assert for write lock here
acl_get
check_access
is_privileged_user_for_credential_change
wsrep_check_for_auth_policy
mysql_alter_user
mysql_execute_command
dispatch_sql_command
dispatch_command
do_command
handle_connection
pfs_spawn_thread

It is well visible in pxc_alter_user tests. In simple
as root:
CREATE USER User1@localhost IDENTIFIED BY 'old_pass';
ALTER USER User1@localhost IDENTIFIED BY 'new_pass';
login as User1:
ALTER USER User1@localhost IDENTIFIED BY 'absolutely_new_pass' REPLACE 'new_pass';

[dlenev]

OK. Thanks for the information!
How about rephrasing the above comment to something like:
// Accessing ACL cache requires lock. We get WRITE_MODE lock since the call to
// is_privileged_user_for_credential_change() below, might have to update part of ACL
// cache with DB-level privileges while it it checks if user has UPDATE privilege on 'mysql'
// database.
?

[kamil-holubicki]

Done

[dlenev]

Hello Kamil!

I think it is acceptable to push the patch after considering my comment about adjusting the comment about WRITE_MODE lock.

I am using word "acceptable" rather than LGTM/OK since I am not too happy about the whole approach with doing double checking. But I realize that trying to solve it in nicer way will probably result in too big diff against Upstream.

[satya-bodapati]

one minor comment on defualt parameter location.

[kamil-holubicki]

Hello Kamil!

I think it is acceptable to push the patch after considering my comment about adjusting the comment about WRITE_MODE lock.

I am using word "acceptable" rather than LGTM/OK since I am not too happy about the whole approach with doing double checking. But I realize that trying to solve it in nicer way will probably result in too big diff against Upstream.

Hi Dmitry,
Yes, it is not a perfect solution and has its own flaws. So far this approach works for several other similar cases, but I'm open to discussion. Maybe we can do it better in the future. For now unfortunately, this is the only thing we can do without diverging too much from upstream (and we definitely don't want to do so)