[MDEV-30178] Explicit errors on required secured transport #2581

Chupsy · 2023-03-28T21:55:45Z

The Jira issue number for this PR is: MDEV-30178

Description

The error message for user connections using insecure transport when connection requires a secured transport.

To make the error message more relevant, introduce a new error 'ER_SECURE_TRANSPORT_REQUIRED', copy of MySQL error message with the error code 08004 (SQL-server rejected establishment SQL-connection).

Before: ER_ACCESS_DENIED "Access denied for user '%s'@'%s'"

Now: ER_ACCESS_SSL_REQUIRED "Connections using insecure transport are prohibited while --require_secure_transport=ON."

Also move the code of 'require_secure_transport' to be executed before
authentication verification, as it's not part of authentication but
rather verifying if connection should be allowed in the first place.

How can this PR be tested?

Modified the existing test for require_secure_transport to expect the new error.

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 PR adds a new error message, this should be fully backward compatible.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.

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-03-28T21:55:55Z

All committers have signed the CLA.

grooverdan

nice to see this bug being addressed. Thank you.

note the other test failures in BB (perfschema.hostcache_ipv6_ssl perfschema.hostcache_ipv4_ssl), check if they are returning the right error code and adjust accordingly.

@vuvova as the error code is the same can this be a 10.5 fix? or did you want the MySQL error code 3159? Did you want to do a final review?

sql/sql_acl.cc

grooverdan

thanks

Chupsy · 2023-03-30T16:34:26Z

Thank you for your review! I see that buildbot is failing at 2 places, is that "expected"? When checking, it does not seem to be due to my changes

grooverdan · 2023-03-31T04:09:53Z

Thank you for your review! I see that buildbot is failing at 2 places, is that "expected"? When checking, it does not seem to be due to my changes

Yes. Both seem unrelated to your change and have occurred before.

vuvova

I'm not sure this is a good approach in general. I'd think a user-independent check for opt_require_secure_transport should be done rather early, almost immediately after the connection is established. And it'll fail with ER_ACCESS_SSL_REQUIRED. It's not part of the authentication, it checks whether the connection itself can be allowed to exist.

While acl_check_ssl() checks user specific SSL requirements, is done after the password is verified and returns ER_ACCESS_DENIED, as it's part of the authentication.

Also, as it's the error message that MySQL has, can it have the same text and the same error number as in MySQL? For compatibility reasons.

vuvova · 2023-03-31T17:24:50Z

sql/share/errmsg-utf8.txt

@@ -10780,3 +10780,5 @@ ER_CM_OPTION_MISSING_REQUIREMENT
        eng "CHANGE MASTER TO option '%s=%s' is missing requirement %s"
 ER_SLAVE_STATEMENT_TIMEOUT 70100
        eng "Slave log event execution was interrupted (slave_max_statement_time exceeded)"
+ER_ACCESS_SSL_REQUIRED 28000


SQLSTATE 08004 is more appropriate here.

28000 is invalid authorization specification like ER_ACCESS_DENIED_ERROR
08004 is SQL-server rejected establishment SQL-connection like ER_NOT_SUPPORTED_AUTH_MODE

Chupsy · 2023-03-31T21:55:36Z

Thank you for your review @vuvova

I understand that we should follow errors from MySQL, here is the original error

Mysql error:SQLSTATE[HY000] [3159] Connections using insecure transport are prohibited while --require_secure_transport=ON

This is a bit different from what is proposed in this CR, the error would only concern this small part

    if (opt_require_secure_transport)
    {
      enum enum_vio_type type= vio_type(vio);

      bool has_secured_transport =
      #ifdef HAVE_OPENSSL
        (type == VIO_TYPE_SSL) ||
      #endif
      #ifdef _WIN32
        (type == VIO_TYPE_NAMEDPIPE);
      #else
        (type == VIO_TYPE_SOCKET);
      #endif

      if(!has_secured_transport){
        return ER_ACCESS_SSL_REQUIRED;
      }
    }

Other errors due to user specific SSL requirements will still return ER_ACCESS_DENIED

We can move this part to check it earlier in the function, before the check of the password?

Chupsy · 2023-04-03T23:36:02Z

Here are the changes I added:

reverted the changes that were no longer useful for custom errors in sql_acl
Moved the code to check require_secure_transport in sql_connect as part of the connection verification as discussed in comments earlier
Changed the error message and error id (ER_SECURE_TRANSPORT_REQUIRED) to match MySQL
Used 08004 as error code as proposed in previous comment

Chupsy · 2023-04-11T23:03:53Z

For some reason, amd64-ubuntu-2204-msan is failing on tests unrelated to my changes...

    innodb.undo_truncate
    mariabackup.incremental_page_compressed
    innodb.innodb_page_compressed
    innodb.innodb_page_compressed
    innodb.innodb_page_compressed
    innodb.innodb_page_compressed
    innodb.innodb_page_compressed
    innodb_zip.create_options innodb_zip.recover innodb_zip.innodb-zip innodb_zip.bug56680

I'm not sure why these come up

Chupsy · 2023-04-29T01:12:37Z

@vuvova the CI is green, feel free to review at any time

Chupsy · 2023-05-01T19:20:32Z

The only CI failing is due to a test losing connection, I see that all PRs have the same issue, with a different test

dlenski · 2023-05-03T00:11:40Z

@LinuxJedi, @vuvova, could you please take another look at this?

(And perhaps re-start the amd64-ubuntu-2204-msan CI job which appears to be failing in a way that's unrelated to this PR.)

LinuxJedi · 2023-05-26T08:47:15Z

Agreed, this is likely unrelated. I'll look into it.

LinuxJedi

A few more style issues to fixup. Looking good though :)

sql/sql_connect.cc

sql/sql_acl.cc

Chupsy · 2023-06-05T23:51:53Z

I fixed the minor issues (and apparently the CI is working now)

vuvova · 2023-06-07T13:18:45Z

sql/share/errmsg-utf8.txt

@@ -10780,3 +10780,5 @@ ER_CM_OPTION_MISSING_REQUIREMENT
        eng "CHANGE MASTER TO option '%s=%s' is missing requirement %s"
 ER_SLAVE_STATEMENT_TIMEOUT 70100
        eng "Slave log event execution was interrupted (slave_max_statement_time exceeded)"
+ER_SECURE_TRANSPORT_REQUIRED 08004


let's try to have it the same number as in MySQL, if possible

MySQL 5.7: https://github.com/mysql/mysql-server/blob/5.7/sql/share/errmsg-utf8.txt#L7574-L7575

MySQL 8.0: https://github.com/mysql/mysql-server/blob/8.0/share/messages_to_clients.txt#L7650-L7651

switched to 3159 as it is the error number on MySQL

How did you do it? I don't see how it was done :(

Isn't it defined only through sql/share/errmsg-utf8.txt ? If so, I modified the error number to match the new number

vuvova · 2023-06-07T13:21:20Z

sql/sql_connect.cc

+      (type != VIO_TYPE_NAMEDPIPE);
+#else
+      (type != VIO_TYPE_SOCKET);
+#endif


do you need all these #ifdef's ? they make the code difficult to read. Values are defined unconditionally, so it seems that you can remove ifdefs and the code will still compile file.

Those #ifdefs predate this PR, as you can see from the diff of sql_acl.cc above. @Chupsy has simplified them considerably here.

Values are defined unconditionally, …

Right. https://github.com/MariaDB/server/blob/HEAD/include/violite.h#L38-L45

… so it seems that you can remove ifdefs and the code will still compile file.

There are a fairly enormous number of other places in the code which are wrapped in unnecessary #ifdef HAVE_OPENSSL, in that case 😅

I am not totally certain on how #ifdef work so I just optimized them with @dlenski, not sure what would happen if we removed them. Tell me if you want me to remove them completely from the code

Based on what @vuvova said and what I see in the code, it seems like you could simply change this section as follows…

return (type != VIO_TYPE_SSL) && (type != VIO_TYPE_NAMEDPIPE) && (type != VIO_TYPE_SOCKET);

… and it should still compile and work correctly on all platforms, even if MariaDB is (somehow) built without TLS support, and even though named pipes are only used on Windows builds, and Unix sockets are only used on POSIX builds.

sql/sql_connect.cc

dlenski · 2023-06-16T18:11:37Z

sql/sql_connect.cc

+      (type != VIO_TYPE_NAMEDPIPE);
+      (type != VIO_TYPE_SOCKET);


@Chupsy, these should be separated by ; rather than &&… probably should be caught by a compiler warning.

vuvova · 2023-06-18T17:47:26Z

sql/share/errmsg-utf8.txt

@@ -10780,3 +10780,5 @@ ER_CM_OPTION_MISSING_REQUIREMENT
        eng "CHANGE MASTER TO option '%s=%s' is missing requirement %s"
 ER_SLAVE_STATEMENT_TIMEOUT 70100
        eng "Slave log event execution was interrupted (slave_max_statement_time exceeded)"
+ER_SECURE_TRANSPORT_REQUIRED 3159


eh, no. you didn't change the error number, you've made sqlstate to be 3159, which isn't even a valid sqlstate value.

you can see error numbers in the include/mysqld_error.h file, they're generated, error messages are simply numbered sequentially.

you can see that the last MySQL error in errmsg-utf8.txt is currently ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS, number 3060. So if you want to add a new one with the number 3159, you need to add ~100 new error messages. Like this

eng "Do not support online operation on table with GIS index" spa "No soporta operación en línea en tabla con índice GIS" + +ER_MYSQL_3061 + eng "" +ER_MYSQL_3062 + eng "" ... and so on ... +ER_MYSQL_3157 + eng "" +ER_MYSQL_3158 + eng "" + +ER_SECURE_TRANSPORT_REQUIRED 08004 + eng "Connections using insecure transport are prohibited while --requi> +

let's try to have it the same number as in MySQL, if possible

I completely understand the goal here, but how is this going to be possible in the long run, without some explicit coordination?

If the wire protocol associates error codes with specific meanings purely based on their numeric codes, which are sequentially increasing numbers assigned by two diverging codebases (MySQL and MariaDB), then there's inevitably going to be a point where MySQL and MariaDB assign different meanings to the same code, right?

(Maybe it has happened already 🤷‍♂️)

So if you want to add a new one with the number 3159, you need to add ~100 new error messages.

Couldn't we simply skip to a new position using start-error-number, as done at several other points in the file? E.g.

ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_GIS chi "不要支持使用GIS索引的表中的在线操作" eng "Do not support online operation on table with GIS index" spa "No soporta operación en línea en tabla con índice GIS" + + start-error-number 3159 + ER_SECURE_TRANSPORT_REQUIRED 08004 + eng "Connections using insecure transport are prohibited while --require_secure_transport=ON."

Ranges are different. 1000-1999 is the historical range for MySQL/MariaDB common errors, 2000-2999 is for client-side errors from Connector/C, 3000-3999 is the new MySQL error range, 4000-4999 is the new MariaDB error range.

no we cannot, I even tried that before writing a previous comment. it is assumed that ranges are all 1000 messages wide, see how ER() macro is defined:

#define ERRORS_PER_RANGE 1000 #define ER_THD(thd,X) ((thd)->variables.errmsgs[((X)-ER_ERROR_FIRST) / ERRORS_PER_RANGE][(X) % ERRORS_PER_RANGE]) #define ER(X) ER_THD(current_thd, (X))

Chupsy · 2023-07-10T19:12:04Z

I fixed the error number and the error that @dlenski found, should be good to go now :)

vuvova

Looks good to me, thanks @Chupsy !

Chupsy · 2023-07-21T20:22:52Z

Thanks @vuvova !
@LinuxJedi anything else needed ? Can we merge the PR?

LinuxJedi · 2023-07-24T09:06:17Z

Thanks @vuvova ! @LinuxJedi anything else needed ? Can we merge the PR?

Can you please rebase against 11.2? Then I think we are good :)

The error message for user connections using insecure transport when secured transport is required is very uninformative and doesn't mention the requirement of secure transport at all. To make the error message more relevant, introduce a new error 'ER_SECURE_TRANSPORT_REQUIRED', copy of MySQL error message with the error code 08004 (SQL-server rejected establishment SQL-connection). Move the code of 'require_secure_transport' to be executed before authentication verification, as it's not part of authentication but rather verifying if connection should be allowed in the first place. 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.

grooverdan requested changes Mar 28, 2023

View reviewed changes

sql/sql_acl.cc Outdated Show resolved Hide resolved

sql/sql_acl.cc Outdated Show resolved Hide resolved

sql/sql_acl.cc Outdated Show resolved Hide resolved

sql/sql_acl.cc Outdated Show resolved Hide resolved

grooverdan added license-bsd-new Contributed under BSD-new license need feedback Can the contributor please address the questions asked. labels Mar 29, 2023

Chupsy force-pushed the 11.0 branch from 1b9702b to fd18126 Compare March 29, 2023 19:50

Chupsy requested a review from grooverdan March 29, 2023 20:59

grooverdan approved these changes Mar 29, 2023

View reviewed changes

grooverdan removed the need feedback Can the contributor please address the questions asked. label Mar 29, 2023

grooverdan assigned vuvova Mar 29, 2023

vuvova requested changes Mar 31, 2023

View reviewed changes

Chupsy requested a review from vuvova March 31, 2023 22:01

Chupsy force-pushed the 11.0 branch from fd18126 to 458be48 Compare April 3, 2023 23:30

Chupsy changed the title ~~[MDEV-30178] Explicit errors on ssl transport auth~~ [MDEV-30178] Explicit errors on required secured transport Apr 3, 2023

Chupsy force-pushed the 11.0 branch 6 times, most recently from ac813a5 to 3bc8def Compare April 5, 2023 20:46

Chupsy force-pushed the 11.0 branch from 3bc8def to 0a2e199 Compare April 11, 2023 00:08

Chupsy force-pushed the 11.0 branch from 0a2e199 to 09479db Compare April 28, 2023 23:04

Chupsy requested a review from grooverdan April 29, 2023 01:12

Chupsy requested a review from LinuxJedi May 23, 2023 19:18

LinuxJedi requested changes Jun 1, 2023

View reviewed changes

sql/sql_connect.cc Outdated Show resolved Hide resolved

sql/sql_connect.cc Outdated Show resolved Hide resolved

sql/sql_acl.cc Outdated Show resolved Hide resolved

Chupsy force-pushed the 11.0 branch 2 times, most recently from 6b56768 to 669fc7c Compare June 5, 2023 21:36

Chupsy requested review from LinuxJedi and dlenski June 5, 2023 23:51

dlenski approved these changes Jun 6, 2023

View reviewed changes

vuvova reviewed Jun 7, 2023

View reviewed changes

Chupsy force-pushed the 11.0 branch 2 times, most recently from c9581e1 to 98e97d4 Compare June 12, 2023 16:47

Chupsy requested a review from vuvova June 12, 2023 16:50

Chupsy force-pushed the 11.0 branch 2 times, most recently from b04861c to afc3bea Compare June 16, 2023 14:57

dlenski reviewed Jun 16, 2023

View reviewed changes

vuvova reviewed Jun 18, 2023

View reviewed changes

Chupsy force-pushed the 11.0 branch from f94d634 to f653564 Compare July 10, 2023 17:57

Chupsy requested review from dlenski and vuvova July 10, 2023 17:58

dlenski approved these changes Jul 10, 2023

View reviewed changes

vuvova approved these changes Jul 21, 2023

View reviewed changes

grooverdan changed the base branch from 11.0 to 11.2 July 25, 2023 08:03

grooverdan force-pushed the 11.0 branch from daf05bf to 84aa30b Compare July 25, 2023 08:05

Merge branch '11.2' into 11.0

62732be

LinuxJedi merged commit 742f960 into MariaDB:11.2 Jul 25, 2023
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEV-30178] Explicit errors on required secured transport #2581

[MDEV-30178] Explicit errors on required secured transport #2581

Chupsy commented Mar 28, 2023 •

edited

CLAassistant commented Mar 28, 2023 •

edited

grooverdan left a comment

grooverdan left a comment

Chupsy commented Mar 30, 2023

grooverdan commented Mar 31, 2023

vuvova left a comment

vuvova Mar 31, 2023

Chupsy commented Mar 31, 2023 •

edited

Chupsy commented Apr 3, 2023

Chupsy commented Apr 11, 2023

Chupsy commented Apr 29, 2023

Chupsy commented May 1, 2023

dlenski commented May 3, 2023 •

edited

LinuxJedi commented May 26, 2023

LinuxJedi left a comment

Chupsy commented Jun 5, 2023

vuvova Jun 7, 2023

dlenski Jun 7, 2023

Chupsy Jun 12, 2023

vuvova Jun 12, 2023

Chupsy Jun 13, 2023

vuvova Jun 7, 2023

dlenski Jun 7, 2023

Chupsy Jun 12, 2023

dlenski Jun 12, 2023

dlenski Jun 16, 2023

vuvova Jun 18, 2023

dlenski Jun 18, 2023 •

edited

vuvova Jun 18, 2023

Chupsy commented Jul 10, 2023

vuvova left a comment

Chupsy commented Jul 21, 2023

LinuxJedi commented Jul 24, 2023

[MDEV-30178] Explicit errors on required secured transport #2581

[MDEV-30178] Explicit errors on required secured transport #2581

Conversation

Chupsy commented Mar 28, 2023 • edited

Description

How can this PR be tested?

Basing the PR against the correct MariaDB version

Backward compatibility

PR quality check

Copyright

CLAassistant commented Mar 28, 2023 • edited

grooverdan left a comment

Choose a reason for hiding this comment

grooverdan left a comment

Choose a reason for hiding this comment

Chupsy commented Mar 30, 2023

grooverdan commented Mar 31, 2023

vuvova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chupsy commented Mar 31, 2023 • edited

Chupsy commented Apr 3, 2023

Chupsy commented Apr 11, 2023

Chupsy commented Apr 29, 2023

Chupsy commented May 1, 2023

dlenski commented May 3, 2023 • edited

LinuxJedi commented May 26, 2023

LinuxJedi left a comment

Choose a reason for hiding this comment

Chupsy commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlenski Jun 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chupsy commented Jul 10, 2023

vuvova left a comment

Choose a reason for hiding this comment

Chupsy commented Jul 21, 2023

LinuxJedi commented Jul 24, 2023

Chupsy commented Mar 28, 2023 •

edited

CLAassistant commented Mar 28, 2023 •

edited

Chupsy commented Mar 31, 2023 •

edited

dlenski commented May 3, 2023 •

edited

dlenski Jun 18, 2023 •

edited