Skip to content

FB8-54, FB8-55, FB8-70, FB8-101: Expose more information to audit plugin #934

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

FB8-54, FB8-55, FB8-70, FB8-101: Expose more information to audit plugin #934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion include/mysql/plugin_audit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
#include "my_command.h"
#include "my_sqlcommand.h"

#define MYSQL_AUDIT_INTERFACE_VERSION 0x0401
#define MYSQL_AUDIT_INTERFACE_VERSION 0x0402

/**
@enum mysql_event_class_t
Expand Down Expand Up @@ -138,6 +138,12 @@ struct mysql_event_general {
MYSQL_LEX_CSTRING general_sql_command;
MYSQL_LEX_CSTRING general_external_user;
MYSQL_LEX_CSTRING general_ip;
/* Added in version 402 */
long long query_id;
MYSQL_LEX_CSTRING database;
long long affected_rows;
unsigned int port;
MYSQL_LEX_CSTRING connection_certificate;
};

/**
Expand Down
5 changes: 5 additions & 0 deletions include/mysql/plugin_audit.h.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,11 @@
MYSQL_LEX_CSTRING general_sql_command;
MYSQL_LEX_CSTRING general_external_user;
MYSQL_LEX_CSTRING general_ip;
long long query_id;
MYSQL_LEX_CSTRING database;
long long affected_rows;
unsigned int port;
MYSQL_LEX_CSTRING connection_certificate;
};
typedef enum {
MYSQL_AUDIT_CONNECTION_CONNECT = 1 << 0,
Expand Down
33 changes: 33 additions & 0 deletions mysql-test/suite/audit_null/r/event_params.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
INSTALL PLUGIN null_audit SONAME 'adt_null.so';
SET @@null_audit_extended_log = 1;
CREATE TABLE foo (v INT);
DROP TABLE foo;
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response database:test
CREATE TABLE foo (v INT);
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response affected_rows:0
INSERT INTO foo VALUES (1), (2);
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response affected_rows:2
SELECT * FROM foo;
v
1
2
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response affected_rows:-1
DELETE FROM foo;
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response affected_rows:2
DROP TABLE foo;
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response port:MYSQLD_PORT
UNINSTALL PLUGIN null_audit;
Warnings:
Warning 1620 Plugin is busy and will be uninstalled on shutdown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure the test works with --repeat=2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked, it works.

16 changes: 16 additions & 0 deletions mysql-test/suite/audit_null/r/event_params_cert.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
INSTALL PLUGIN null_audit SONAME 'adt_null.so';
CREATE USER cert_auth@localhost REQUIRE X509;
GRANT SELECT ON test.* TO cert_auth@localhost;
CREATE TABLE foo (i INT);
FLUSH PRIVILEGES;
SET @@null_audit_extended_log = 1;
SELECT * FROM foo;
i
SHOW STATUS LIKE "Audit_null_generic_event_response";
Variable_name Value
Audit_null_generic_event_response connection_certificate:-----BEGIN CERTIFICATE-----\nMIIDyDCCArCgAwIBAgIJAOG0pVw936YVMA0GCSqGSIb3DQEBCwUAMGMxCzAJBgNV\nBAYTAlNFMRIwEAYDVQQIDAlTdG9ja2hvbG0xEjAQBgNVBAcMCVN0b2NraG9sbTEP\nMA0GA1UECgwGT3JhY2xlMQ4wDAYDVQQLDAVNeVNRTDELMAkGA1UEAwwCQ0EwHhcN\nMTQxMjA1MDQ0OTIzWhcNMjkxMjAxMDQ0OTIzWjBnMQswCQYDVQQGEwJTRTESMBAG\nA1UECAwJU3RvY2tob2xtMRIwEAYDVQQHDAlTdG9ja2hvbG0xDzANBgNVBAoMBk9y\nYWNsZTEOMAwGA1UECwwFTXlTUUwxDzANBgNVBAMMBkNsaWVudDCCASIwDQYJKoZI\nhvcNAQEBBQADggEPADCCAQoCggEBAMjRof6kjPMbF3EbdDUR4A5sQAr7wPfw67vJ\nHaHH17CK9vHP+mvQeWTru2mlDYAG31IU0oUyz7/OKkcoW80LKKu7BzPVi9O0csSm\ntcw3uQOoeFYlWB8XMHzRCrvsPKMDkJeZkkmus1eWXBrp6AIjrsjJBVBj5XehmnMG\ndA5GUCjYyU/EHDe4UhgLrxkr1OVmdKTz8No
DROP USER cert_auth@localhost;
DROP TABLE foo;
UNINSTALL PLUGIN null_audit;
Warnings:
Warning 1620 Plugin is busy and will be uninstalled on shutdown
1 change: 1 addition & 0 deletions mysql-test/suite/audit_null/t/event_params-master.opt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
$AUDIT_NULL_OPT
44 changes: 44 additions & 0 deletions mysql-test/suite/audit_null/t/event_params.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--source include/have_null_audit_plugin.inc
--source include/count_sessions.inc
--source include/have_debug.inc

eval INSTALL PLUGIN null_audit SONAME '$AUDIT_NULL';

SET @@null_audit_extended_log = 1;

## database name in generic event
CREATE TABLE foo (v INT);
DROP TABLE foo;

--replace_regex /.*(database:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use something like

SET @param_name = 'database:';
SELECT SUBSTRING(REGEX_SUBSTR(variable_value, CONCAT('.*(', @param_name, '[^;]*).*' )), LENGTH(@param_name)) INTO @extracted_param
FROM performance_schema.global_status
WHERE variable_name='Audit_null_generic_event_response';

Then use --source include/assert.inc to check DATABASE() == @extracted_param

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that's better - with this approach, the query is easier to read, and the value is there in the result file. The test also won't stop at the first error, and in the end, it will display all issues, not only the first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason for this complexity is to have everything related to the assertion in one place (in the .test file) and not split between .test and .result. From my experience, .result files are never reviewed properly and developers very often do './mtr --record' without checking if the output is still expected.
This is just a suggestion - leaving this up to you.


## affected_rows in generic event
CREATE TABLE foo (v INT);
--replace_regex /.*(affected_rows:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise


INSERT INTO foo VALUES (1), (2);
--replace_regex /.*(affected_rows:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise


SELECT * FROM foo;
--replace_regex /.*(affected_rows:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise


DELETE FROM foo;
--replace_regex /.*(affected_rows:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise


DROP TABLE foo;

## port
let $MYSQLD_PORT= `SELECT @@port`;
--replace_result $MYSQLD_PORT MYSQLD_PORT
--replace_regex /.*(port:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise



UNINSTALL PLUGIN null_audit;

--source include/wait_until_count_sessions.inc
4 changes: 4 additions & 0 deletions mysql-test/suite/audit_null/t/event_params_cert-client.opt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
--ssl-mode=VERIFY_CA
--ssl-ca=$MYSQL_TEST_DIR/std_data/cacert.pem
--ssl-cert=$MYSQL_TEST_DIR/std_data/client-cert.pem
--ssl-key=$MYSQL_TEST_DIR/std_data/client-key.pem
1 change: 1 addition & 0 deletions mysql-test/suite/audit_null/t/event_params_cert-master.opt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
$AUDIT_NULL_OPT
27 changes: 27 additions & 0 deletions mysql-test/suite/audit_null/t/event_params_cert.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--source include/have_ssl.inc
--source include/count_sessions.inc
--source include/have_debug.inc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add --source include/count_sessions.inc as you are establishing/closing a new connection.

eval INSTALL PLUGIN null_audit SONAME '$AUDIT_NULL';

CREATE USER cert_auth@localhost REQUIRE X509;
GRANT SELECT ON test.* TO cert_auth@localhost;
CREATE TABLE foo (i INT);
FLUSH PRIVILEGES;
connect(con1,localhost,cert_auth,,,,,SSL);

SET @@null_audit_extended_log = 1;

SELECT * FROM foo;

--replace_regex /.*(connection_certificate:[^;]*).*/\1/
SHOW STATUS LIKE "Audit_null_generic_event_response";

disconnect con1;
connection default;
DROP USER cert_auth@localhost;
DROP TABLE foo;

UNINSTALL PLUGIN null_audit;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--source include/wait_until_count_sessions.inc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still no --source include/wait_until_count_sessions.inc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

--source include/wait_until_count_sessions.inc
2 changes: 2 additions & 0 deletions plugin/audit_null/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,7 @@
MYSQL_ADD_PLUGIN(audit_null audit_null.cc
MODULE_ONLY MODULE_OUTPUT_NAME "adt_null")

INCLUDE_DIRECTORIES(SYSTEM ${BOOST_PATCHES_DIR} ${BOOST_INCLUDE_DIR})

MYSQL_ADD_PLUGIN(test_security_context test_security_context.cc
TEST_ONLY MODULE_ONLY)
71 changes: 70 additions & 1 deletion plugin/audit_null/audit_null.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <mysqld_error.h>
#include <stdio.h>
#include <sys/types.h>
#include <sstream>
#include <boost/algorithm/string/replace.hpp>

#include "lex_string.h"
#include "m_ctype.h"
Expand Down Expand Up @@ -130,6 +132,13 @@ static char *g_record_buffer;

#undef AUDIT_NULL_VAR

#ifndef DBUG_OFF
static const constexpr size_t event_response_buffer_len = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is 1000 enough now when we have certificates also included in GENERAL STATUS event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now yes, as all other fields are short.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should event_response_buffer_len and generic_event_response definitions be also wrapped in #ifndef DBUG_OFF?

static char generic_event_response[event_response_buffer_len + 1] = {
0,
};
#endif

/*
Plugin status variables for SHOW STATUS
*/
Expand All @@ -144,6 +153,11 @@ static SHOW_VAR simple_status[] = {

#undef AUDIT_NULL_VAR

#ifndef DBUG_OFF
{"Audit_null_generic_event_response", (char *)generic_event_response,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be wrapped in #ifndef DBUG_OFF?

SHOW_CHAR, SHOW_SCOPE_GLOBAL},
#endif

{0, 0, SHOW_UNDEF, SHOW_SCOPE_GLOBAL}};

/*
Expand Down Expand Up @@ -175,6 +189,12 @@ static MYSQL_THDVAR_INT(event_order_check_exact, PLUGIN_VAR_RQCMDARG,
"Plugin checks exact event order.", NULL, NULL, 1, 0, 1,
0);

#ifndef DBUG_OFF
static MYSQL_THDVAR_INT(extended_log, PLUGIN_VAR_RQCMDARG,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to declare this var as GLOBAL-only (MYSQL_SYSVAR_INT) since we are using a single global buffer for the output anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't that make thread level control useful? If it's a threadvar, you can turn it on for only one thread, and other threads won't accidentally modify the status variables, breaking tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such change is too visible for user to be discussed only here (unless FB chimes in)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be debug only since it's mainly used for testing? Are there security issues with non-privileged users getting access to see the last logged query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And is audit_null used outside MTR testing, in production? This is only present in that plugin, that's why I didn't see it as an issue. I can make it debug only, but that will also make all related tests debug only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in production, but if it's used as a template for other audit plugins, it might run the risk of it getting out. Let's make it debug-only and having the tests be debug-only would be fine.

"Provide extended debug information with audit_null.",
NULL, NULL, 1, 0, 1, 0);
#endif

static MYSQL_THDVAR_STR(event_record_def,
PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_MEMALLOC,
"Event recording definition", NULL, NULL, NULL);
Expand Down Expand Up @@ -411,6 +431,45 @@ static int process_command(MYSQL_THD thd, LEX_CSTRING event_command,
return 0;
}

#ifndef DBUG_OFF
/*
* Exposes a generic audit log event in a status variable
*/
static void log_event(const mysql_event_general *event) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function also be wrapped in #ifndef DBUG_OFF?

#define EVENT_PARAM(name) event_str << #name ":" << event->name << ";";
#define EVENT_PARAM_STR(name) \
{ \
std::string tmp(event->name.str, event->name.length); \
boost::replace_all(tmp, "\n", "\\n"); \
event_str << #name ":" << tmp << ";"; \
}
std::stringstream event_str;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also seeing an internal error compiling this:

../../../plugin/audit_null/audit_null.cc: In function ‘void log_event(const mysql_event_general*)’:
../../../plugin/audit_null/audit_null.cc:438:21: error: aggregate ‘std::stringstream event_str’ has incomplete type and cannot be defined
std::stringstream event_str;

EVENT_PARAM(event_subclass);
EVENT_PARAM(general_error_code);
// skipping general_thread_id
EVENT_PARAM_STR(general_user);
EVENT_PARAM_STR(general_command);
EVENT_PARAM_STR(general_query);
// EVENT_PARAM_STR(general_charset);
EVENT_PARAM(general_time);
EVENT_PARAM(general_rows);
EVENT_PARAM_STR(general_host);
EVENT_PARAM_STR(general_sql_command);
EVENT_PARAM_STR(general_external_user);
EVENT_PARAM_STR(general_ip);
EVENT_PARAM(query_id);
EVENT_PARAM_STR(database);
EVENT_PARAM(affected_rows);
EVENT_PARAM(port);
EVENT_PARAM_STR(connection_certificate);
#undef EVENT_PARAM
#undef EVENT_PARAM_STR

const std::string str = event_str.str();
strncpy(generic_event_response, str.c_str(), event_response_buffer_len);
}
#endif

/**
@brief Plugin function handler.

Expand Down Expand Up @@ -454,9 +513,16 @@ static int audit_null_notify(MYSQL_THD thd, mysql_event_class_t event_class,
case MYSQL_AUDIT_GENERAL_RESULT:
number_of_calls_general_result++;
break;
case MYSQL_AUDIT_GENERAL_STATUS:
case MYSQL_AUDIT_GENERAL_STATUS: {
#ifndef DBUG_OFF
const int extended_info = static_cast<int>(THDVAR(thd, extended_log));
if (extended_info != 0) {
log_event(static_cast<const mysql_event_general *>(event));
}
#endif
number_of_calls_general_status++;
break;
}
default:
break;
}
Expand Down Expand Up @@ -767,6 +833,9 @@ static SYS_VAR *system_variables[] = {
MYSQL_SYSVAR(event_order_check_consume_ignore_count),
MYSQL_SYSVAR(event_order_started),
MYSQL_SYSVAR(event_order_check_exact),
#ifndef DBUG_OFF
MYSQL_SYSVAR(extended_log),
#endif

MYSQL_SYSVAR(event_record_def),
MYSQL_SYSVAR(event_record),
Expand Down
15 changes: 9 additions & 6 deletions sql/auth/sql_authentication.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,9 +1382,9 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, const char *data,

end = my_stpnmov(end, server_version, SERVER_VERSION_LENGTH);
end = my_stpcpy(end, " ");
end = my_stpnmov(end,
MYSQL_COMPILATION_COMMENT,
SERVER_VERSION_LENGTH - (end - buff - 1)) + 1;
end = my_stpnmov(end, MYSQL_COMPILATION_COMMENT,
SERVER_VERSION_LENGTH - (end - buff - 1)) +
1;

DBUG_ASSERT(sizeof(my_thread_id) == 4);
int4store((uchar *)end, mpvio->thread_id);
Expand Down Expand Up @@ -1844,11 +1844,14 @@ static bool read_client_connect_attrs(char **ptr, size_t *max_bytes_available,
return false;
}

typedef std::string Sql_string_t;
static Sql_string_t x509_cert_write(X509 *cert);

static bool acl_check_ssl(THD *thd, const ACL_USER *acl_user) {
#if defined(HAVE_OPENSSL)
Vio *vio = thd->get_protocol_classic()->get_vio();
SSL *ssl = (SSL *)vio->ssl_arg;
X509 *cert;
X509 *cert = nullptr;
#endif /* HAVE_OPENSSL */

/*
Expand All @@ -1875,6 +1878,7 @@ static bool acl_check_ssl(THD *thd, const ACL_USER *acl_user) {
if (vio_type(vio) == VIO_TYPE_SSL &&
SSL_get_verify_result(ssl) == X509_V_OK &&
(cert = SSL_get_peer_certificate(ssl))) {
thd->set_connection_certificate(x509_cert_write(cert));
X509_free(cert);
return 0;
}
Expand Down Expand Up @@ -1924,6 +1928,7 @@ static bool acl_check_ssl(THD *thd, const ACL_USER *acl_user) {
}
OPENSSL_free(ptr);
}
thd->set_connection_certificate(x509_cert_write(cert));
X509_free(cert);
return 0;
#else /* HAVE_OPENSSL */
Expand Down Expand Up @@ -4129,8 +4134,6 @@ static SYS_VAR *sha256_password_sysvars[] = {
MYSQL_SYSVAR(private_key_path), MYSQL_SYSVAR(public_key_path),
MYSQL_SYSVAR(auto_generate_rsa_keys), 0};

typedef std::string Sql_string_t;

/**
Exception free resize

Expand Down
7 changes: 7 additions & 0 deletions sql/sql_audit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,10 +371,17 @@ int mysql_audit_notify(THD *thd, mysql_event_general_subclass_t subclass,
event.general_user.str = user_buff;
event.general_user.length = make_user_name(sctx, user_buff);
event.general_ip = sctx->ip();
event.database.str = thd->db().str;
event.database.length = thd->db().length;
event.query_id = thd->query_id;
event.general_host = sctx->host();
event.general_external_user = sctx->external_user();
event.general_rows = thd->get_stmt_da()->current_row_for_condition();
event.general_sql_command = sql_statement_names[thd->lex->sql_command];
event.affected_rows = thd->get_row_count_func();
event.port = mysqld_port;
event.connection_certificate.str = thd->connection_certificate().c_str();
event.connection_certificate.length = thd->connection_certificate().size();

thd_get_audit_query(thd, &event.general_query,
(const CHARSET_INFO **)&event.general_charset);
Expand Down
Loading