FB8-53: Expose user certificate details to command line #959

percona-ysorokin · 2019-02-11T19:39:54Z

Jira ticket: https://jira.percona.com/browse/FB8-53

Reference Patch: f29eb03
Reference Patch: c631017

'THD' class extended with the following convenience methods:

'has_net_vio()'
'get_net_vio()'
'has_net_vio_ssl_arg()'
'get_net_vio_ssl_arg()'
'get_peer_certificate_info()'

Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl'
which populates 'INFORMATION_SCHEMA.AUTHINFO' table.

This is a change to help the implementation of SSL based authentication
efforts. Currently the certificate details are unavailable to the sys admin.
The v3 extensions usually carry important information for identifying the
user.

This change adds a new table to the information schema to expose auth
details. It is organized by process ID like processlist table.

Test Plan: Run newly added test case

Originally Reviewed By: santoshb

sql/sql_class.h

sql/sql_show.cc

sql/sql_class.cc

percona-ysorokin · 2019-02-13T17:18:47Z

Rebased on a newer trunk with audit extensions, addressed review comments. Ready for another round of review.

dutow

Looking at the changes/tests, the commit message is also a bit strange - it isn't a new command line option or anything like that, but an information schema extension.

dutow · 2019-02-15T08:08:46Z

sql/sql_class.cc

+    print_result = PEM_write_bio_X509(bio.get(), cert.get());
+  if (print_result != 1) return {};
+
+  // decouple buffer and close bio object


Are you sure this comment is true?
The bio object is closed when the unique_ptr is destructed, at the end of the function. And if it decouples it, and it doesn't free the memory, we are leaking memory - but based on the documentation, I don't think BIO_get_mem_ptr does that.

Oh, this is just an outdated comment. There is no decoupling here at all. Removing.

dutow · 2019-02-15T08:13:49Z

sql/sql_class.cc

+std::string THD::extract_peer_certificate_info(const THD *thd, bool printable) {
+  if (!thd->has_net_vio_ssl_arg()) return {};
+
+  auto ssl = static_cast<SSL *>(thd->get_net_vio_ssl_arg());


This could be const SSL*, and the get_net_vio getters could also return const pointers.

Good point. Reworked.

Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: facebook@f29eb03 Reference Patch: facebook@c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()'; - 'get_net_vio()'; - 'has_net_vio_ssl_arg()'; - 'get_net_vio_ssl_arg()'; - 'extract_peer_certificate_info()'. Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. The following changes were implemented on the top of the "FB8-54, FB8-55, FB8-70, FB8-101: Expose more information to audit plugin" (facebook@3092225). * Fixed problem with empty connection certificates for users who are connecting via SSL to the server but were not created with 'SSL | X509' option in 'CREATE USER' statement. * The following methods are now used as a substitution for their duplicates: - 'update_connection_certificate()'; - 'reset_connection_certificate()'; - 'get_connection_certificate()'. - Increased stability of the 'audit_null.event_params_cert' MTR test case. ---------- facebook@f29eb03 ---------- Expose user certificate details to command line. Summary: This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Test Plan: Run newly added test case Reviewers: pengt, tianx, jtolmer, santoshb Originally Reviewed By: santoshb ---------- facebook@c631017 ---------- Fix leak in get_peer_cert_info Summary: Valgrind shows that we are not freeing the X509 object that is returned when we call SSL_get_peer_certificate. Fix this by free'ing it at various exit points. Squash with: f29eb03 Expose user certificate details to command line. Test Plan: mysqltest.sh --valgrind main.information_schema_authinfo Reviewers: kradhakrishnan Originally Reviewed By: kradhakrishnan Subscribers: jkedgar, webscalesql-eng

facebook-github-bot

@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hermanlee · 2019-02-22T23:04:26Z

sql/auth/sql_authentication.cc

@@ -3375,6 +3370,8 @@ int acl_authenticate(THD *thd, enum_server_command command) {
      DBUG_RETURN(1);
    }

+    thd->update_connection_certificate();


Should this check for failure? If acl_check_ssl() returned 0, but then update_connection_certificate() hits a problem allocating memory during Bio_new, the certificate stored for the connection would be "". Would it be safer to fail the authentication?

@hermanlee My reasoning is that this certificate string stored in THD is used only for information purposes and in my opinion in case of OOM it is better to provide inaccurate information but let users log in rather than completely deny their access. In any case, leaving this up to you to decide. It's not a big deal to rework this.

hermanlee

Chances of the failure in update_connection_certificate is low, and returning "" is probably just fine.

Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: f29eb03 Reference Patch: c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: #959 Reviewed By: lth Differential Revision: D14137362 Pulled By: lth fbshipit-source-id: b42aeba

Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: f29eb03 Reference Patch: c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: #959 Test Plan: Run newly added test case Originally Reviewed By: santoshb Reviewed By: lth Differential Revision: D14137362 Pulled By: lth fbshipit-source-id: d391cad

Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: facebook@f29eb03 Reference Patch: facebook@c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: facebook#959 Test Plan: Run newly added test case Originally Reviewed By: santoshb Reviewed By: lth Differential Revision: D14137362 Pulled By: lth fbshipit-source-id: d391cad

facebook#959) Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: facebook@f29eb03 Reference Patch: facebook@c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: facebook#959 Reviewed By: lloyd Differential Revision: D14137362 Pulled By: lth

…percona#959) Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: facebook/mysql-5.6@f29eb03 Reference Patch: facebook/mysql-5.6@c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: facebook/mysql-5.6#959 Reviewed By: lloyd Differential Revision: D14137362 Pulled By: lth

facebook#959) Summary: Jira ticket: https://jira.percona.com/browse/FB8-53 Reference Patch: facebook@f29eb03 Reference Patch: facebook@c631017 'THD' class extended with the following convenience methods: - 'has_net_vio()' - 'get_net_vio()' - 'has_net_vio_ssl_arg()' - 'get_net_vio_ssl_arg()' - 'get_peer_certificate_info()' Implemented new 'Fill_authinfo_list' class derived from 'Do_THD_Impl' which populates 'INFORMATION_SCHEMA.AUTHINFO' table. This is a change to help the implementation of SSL based authentication efforts. Currently the certificate details are unavailable to the sys admin. The v3 extensions usually carry important information for identifying the user. This change adds a new table to the information schema to expose auth details. It is organized by process ID like processlist table. Pull Request resolved: facebook#959 Reviewed By: lloyd Differential Revision: D14137362 Pulled By: lth

facebook-github-bot added the CLA Signed label Feb 11, 2019

laurynas-biveinis suggested changes Feb 12, 2019

View reviewed changes

sql/sql_class.h Outdated Show resolved Hide resolved

sql/sql_show.cc Outdated Show resolved Hide resolved

dutow reviewed Feb 12, 2019

View reviewed changes

sql/sql_class.cc Outdated Show resolved Hide resolved

percona-ysorokin force-pushed the fbms-8.0-bug_fb8_53-schema_authinfo branch from de4a7cc to b96a591 Compare February 13, 2019 17:13

dutow reviewed Feb 15, 2019

View reviewed changes

percona-ysorokin force-pushed the fbms-8.0-bug_fb8_53-schema_authinfo branch from b96a591 to 8a82959 Compare February 15, 2019 13:44

dutow approved these changes Feb 15, 2019

View reviewed changes

percona-ysorokin changed the title ~~WIP FB8-53: Expose user certificate details to command line~~ FB8-53: Expose user certificate details to command line Feb 15, 2019

facebook-github-bot reviewed Feb 19, 2019

View reviewed changes

hermanlee requested changes Feb 22, 2019

View reviewed changes

hermanlee approved these changes Mar 6, 2019

View reviewed changes

hermanlee closed this Mar 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FB8-53: Expose user certificate details to command line #959

FB8-53: Expose user certificate details to command line #959

percona-ysorokin commented Feb 11, 2019 •

edited by hermanlee

Loading

percona-ysorokin commented Feb 13, 2019

dutow left a comment

dutow Feb 15, 2019

percona-ysorokin Feb 15, 2019 •

edited

Loading

dutow Feb 15, 2019

percona-ysorokin Feb 15, 2019

facebook-github-bot left a comment

hermanlee Feb 22, 2019

percona-ysorokin Mar 7, 2019

hermanlee left a comment

FB8-53: Expose user certificate details to command line #959

FB8-53: Expose user certificate details to command line #959

Conversation

percona-ysorokin commented Feb 11, 2019 • edited by hermanlee Loading

percona-ysorokin commented Feb 13, 2019

dutow left a comment

Choose a reason for hiding this comment

dutow Feb 15, 2019

Choose a reason for hiding this comment

percona-ysorokin Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

dutow Feb 15, 2019

Choose a reason for hiding this comment

percona-ysorokin Feb 15, 2019

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

hermanlee Feb 22, 2019

Choose a reason for hiding this comment

percona-ysorokin Mar 7, 2019

Choose a reason for hiding this comment

hermanlee left a comment

Choose a reason for hiding this comment

percona-ysorokin commented Feb 11, 2019 •

edited by hermanlee

Loading

percona-ysorokin Feb 15, 2019 •

edited

Loading