[MDEV-33479] Extend Unix socket authentication to support authentication_string #2671

HugoWenTD · 2023-06-16T20:48:43Z

Description

Before this change the unix socket auth plugin returned true only when the OS socket user id matches the MariaDB user name.
The authentication string was ignored.

Now if an authentication string is defined with in unix_socket authentication rule, then the authentication string will be used to compare with the socket's user name, and the plugin will return a positive if matching.

Make the plugin to fill in the @@external_user variable.

This change is similar to MySQL commit of mysql/mysql-server@6ddbc58e.
However there's one difference with above commit:

For MySQL, both OS user matches DB user name and OS user matches the authentication string will be allowed to connect.
For MariaDB, we only allows the OS user matches the authentication string to connect, if the authentication string is defined.
This is because allowing both OS user names has risks and couldn't handle the case that a customer only wants to allow one single OS user to connect which doesn't matches the DB user name.

If DB user is created with multiple unix_socket options for example:
create user A identified via unix_socket as 'B' or unix_socket as 'C';
Then both Unix user of B and C are accepted.

How can this PR be tested?

Existing MTR test of plugins.unix_socket is not impacted.
Also add a new MTR test to verify authentication with authentication string. See the MTR test cases for supported/unsupported cases.

Basing the PR against the correct MariaDB version

This is a new feature and the PR is based against the latest MariaDB development branch

Backward compatibility

This is a new feature to make use of authentication string and should not affect backwards compatibility.

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

CLAassistant · 2023-06-16T20:48:48Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

HugoWenTD · 2023-06-16T21:05:04Z

While the PR is to port exact same behavior and change from MySQL, we do have a concern about the expected behavior and possible security risk.

With this change, both OS user that matches the DB user name and OS user that matches the authentication string will be allowed to use socket connection.

For example if the DB user is created with create user 'admin' identified via unix_socket as 'root';, both admin and root OS user will be allowed:
- sudo -u admin mysql -uadmin
- sudo -u root mysql -uadmin
However if an admin user does not originally exist in the OS.
Someone who managed to create an admin OS user and grant themselves privilege to use admin OS user will be able to connect to the database using sudo -u admin mysql -uadmin.

To solve above issue, we could consider only allowing sudo -u authentication_string mysql -uadmin if authentication_string is defined.

However that would cause inconsistent behavior with MySQL server.
In addition, it will cause behavior changing if any existing MariaDB user is using unix_socket with authentication_string defined on their database now..

Please let me know about your opinion and if you prefer to keep the same behavior as MySQL or introduce above limitation.

HugoWenTD · 2023-06-16T21:13:04Z

Two builds were failing but it doesn't seem to be related to my commit:

buildbot/amd64-debian-10-debug-embedded — Build done.
buildbot/amd64-debian-11-debug-ps-embedded — Build done.

main.group_min_max 'innodb'              w4 [ fail ]
        Test ended at 2023-06-16 21:01:57
CURRENT_TEST: main.group_min_max
Warning: /home/buildbot/amd64-debian-10-debug-embedded/build/libmysqld/examples/mysqltest_embedded: unknown variable 'loose-ssl-ca=/home/buildbot/amd64-debian-10-debug-embedded/build/mysql-test/std_data/cacert.pem'
Warning: /home/buildbot/amd64-debian-10-debug-embedded/build/libmysqld/examples/mysqltest_embedded: unknown variable 'loose-ssl-cert=/home/buildbot/amd64-debian-10-debug-embedded/build/mysql-test/std_data/client-cert.pem'
Warning: /home/buildbot/amd64-debian-10-debug-embedded/build/libmysqld/examples/mysqltest_embedded: unknown variable 'loose-ssl-key=/home/buildbot/amd64-debian-10-debug-embedded/build/mysql-test/std_data/client-key.pem'
Warning: /home/buildbot/amd64-debian-10-debug-embedded/build/libmysqld/examples/mysqltest_embedded: unknown option '--loose-skip-ssl'
--- /home/buildbot/amd64-debian-10-debug-embedded/build/mysql-test/main/group_min_max.result	2023-06-16 20:55:57.000000000 +0000
+++ /home/buildbot/amd64-debian-10-debug-embedded/build/mysql-test/main/group_min_max.reject	2023-06-16 21:01:57.130122890 +0000
@@ -4225,7 +4225,9 @@
 from information_schema.optimizer_trace;
 CAST(json_value(json_extract(trace, '$**.chosen_access_method.cost'), '$[0]')
 as DOUBLE) < 1.0e100
-1
+NULL
+Warnings:
+Warning	4037	Unexpected end of JSON text in argument 1 to function 'json_extract'
 set optimizer_use_condition_selectivity = @tmp, optimizer_trace=@tmp2;
 drop table t1;
 #
mysqltest: Result length mismatch

vuvova · 2023-06-17T14:15:36Z

I would say that it's illogical that after

create user 'admin' identified via unix_socket as 'root';

both admin and root can login using this account. I'd intuitively expect that only root should be allowed to login. If one wants to allow both the correct syntax in MariaDB would be

create user 'admin' identified via unix_socket as 'root' or unix_socket as 'admin';

where the last as 'admin' can be omitted, but I kept it in this example for clarity

HugoWenTD · 2023-06-19T15:56:48Z

I'd intuitively expect that only root should be allowed to login. If one wants to allow both the correct syntax is MariaDB would be
create user 'admin' identified via unix_socket as 'root' or unix_socket as 'admin';
where the last as 'admin' can be omitted, but I kept it in this example for clarity

@vuvova Thank you Sergei for your quick response! That makes sense! I'll update the PR correspondingly.

vuvova · 2023-06-19T17:26:00Z

please, don't forget to write tests for that. Normally this OR thing works by sending a "change plugin" packet to the client, but unix_socket doesn't send anything, so retrying several unix_socket's may hit some bug in the implementation

HugoWenTD · 2023-06-19T21:39:08Z

@vuvova Updated the commit and PR description. Please see the different cases in the new MTR test.

plugin/auth_socket/auth_socket.c

LinuxJedi

Looks good to me, added a minor comment. I will defer to @vuvova for final review as he commented on this previously (or someone else can review). I'll also create a Jira for this now.

LinuxJedi · 2024-02-16T15:26:21Z

plugin/auth_socket/auth_socket.c

@@ -146,7 +152,7 @@ maria_declare_plugin(auth_socket)
  PLUGIN_LICENSE_GPL,
  NULL,
  NULL,
-  0x0100,
+  0x0101,
  NULL,
  NULL,
  "1.0",


Really minor thing, but with the hex version bump, this should probably become "1.01" too.

And the string version should match too

Thank you for reviewing and pointing out the version string mismatch! I overlooked it as MySQL does not have that string. The version string seems can be auto-generated by the hex 0x0101 which is parsed as version 1.1? (I'm not sure why we keep two types of version though)
I saw the version string is eventually populated into PLUGIN_AUTH_VERSION in INFORMATION_SCHEMA.PLUGINS, and the hex is affecting PLUGIN_VERSION column.

PLUGIN_NAME PLUGIN_VERSION PLUGIN_AUTHOR PLUGIN_MATURITY PLUGIN_AUTH_VERSION unix_socket 1.1 Sergei Golubchik Stable 1.1

I've updated them to match in lasted commit and rebased on 11.5 branch. Thank you!

LinuxJedi · 2024-02-16T15:31:12Z

Created MDEV-33479 to track this.

Before this change the unix socket auth plugin returned true only when the OS socket user id matches the MariaDB user name. The authentication string was ignored. Now if an authentication string is defined with in `unix_socket` authentication rule, then the authentication string will be used to compare with the socket's user name, and the plugin will return a positive if matching. Make the plugin to fill in the @@external_user variable. This change is similar to MySQL commit of mysql/mysql-server@6ddbc58e. However there's one difference with above commit: - For MySQL, both Unix user matches DB user name and Unix user matches the authentication string will be allowed to connect. - For MariaDB, we only allows the Unix user matches the authentication string to connect, if the authentication string is defined. This is because allowing both Unix user names has risks and couldn't handle the case that a customer only wants to allow one single Unix user to connect which doesn't matches the DB user name. If DB user is created with multiple unix_socket options for example: `create user A identified via unix_socket as 'B' or unix_socket as 'C';` Then both Unix user of B and C are accepted. Existing MTR test of `plugins.unix_socket` is not impacted. Also add a new MTR test to verify authentication with authentication string. See the MTR test cases for supported/unsupported cases. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

HugoWenTD force-pushed the github-wenhug-10.11-socketauth branch from f9d1660 to ff211d4 Compare June 19, 2023 21:34

HugoWenTD force-pushed the github-wenhug-10.11-socketauth branch 2 times, most recently from 604ad1d to 31b1c96 Compare June 19, 2023 22:02

dlenski reviewed Jun 28, 2023

View reviewed changes

plugin/auth_socket/auth_socket.c Outdated Show resolved Hide resolved

HugoWenTD force-pushed the github-wenhug-10.11-socketauth branch 3 times, most recently from 7818d7a to b82932d Compare July 3, 2023 20:24

LinuxJedi approved these changes Feb 16, 2024

View reviewed changes

LinuxJedi changed the title ~~Extend Unix socket authentication to support authentication_string~~ [MDEV-33479] Extend Unix socket authentication to support authentication_string Feb 16, 2024

LinuxJedi requested a review from vuvova February 16, 2024 15:31

HugoWenTD force-pushed the github-wenhug-10.11-socketauth branch from b82932d to ef8e8d1 Compare February 20, 2024 19:13

HugoWenTD changed the base branch from 10.11 to 11.5 February 20, 2024 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEV-33479] Extend Unix socket authentication to support authentication_string #2671

[MDEV-33479] Extend Unix socket authentication to support authentication_string #2671

HugoWenTD commented Jun 16, 2023 •

edited

CLAassistant commented Jun 16, 2023

HugoWenTD commented Jun 16, 2023

HugoWenTD commented Jun 16, 2023

vuvova commented Jun 17, 2023 •

edited

HugoWenTD commented Jun 19, 2023

vuvova commented Jun 19, 2023

HugoWenTD commented Jun 19, 2023

LinuxJedi left a comment

LinuxJedi Feb 16, 2024

grooverdan Feb 20, 2024

HugoWenTD Feb 20, 2024

LinuxJedi commented Feb 16, 2024

[MDEV-33479] Extend Unix socket authentication to support authentication_string #2671

Are you sure you want to change the base?

[MDEV-33479] Extend Unix socket authentication to support authentication_string #2671

Conversation

HugoWenTD commented Jun 16, 2023 • edited

Description

How can this PR be tested?

Basing the PR against the correct MariaDB version

Backward compatibility

Copyright

CLAassistant commented Jun 16, 2023

HugoWenTD commented Jun 16, 2023

HugoWenTD commented Jun 16, 2023

vuvova commented Jun 17, 2023 • edited

HugoWenTD commented Jun 19, 2023

vuvova commented Jun 19, 2023

HugoWenTD commented Jun 19, 2023

LinuxJedi left a comment

Choose a reason for hiding this comment

LinuxJedi Feb 16, 2024

Choose a reason for hiding this comment

grooverdan Feb 20, 2024

Choose a reason for hiding this comment

HugoWenTD Feb 20, 2024

Choose a reason for hiding this comment

LinuxJedi commented Feb 16, 2024

HugoWenTD commented Jun 16, 2023 •

edited

vuvova commented Jun 17, 2023 •

edited