[Maria-developers] MDEV-4472 (audit plugin)
Hi, Sergei. Here is the plugin code for you to check out. It constists of a single new file, so i send you the link to it. http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c What's missing in it as i'm not sure how it can be done: - No server host/ip in the log. How get that information? - SYSLOG logging. The SysLog i knew was declared in the syslog.h. That's the interface talking to the local syslogd instance. If it's what we need to use, then i have no idea how to specify the Syslog server IP for that daemon (which is required by the spec). I see nothing in the API about it and i thought it should be configured with the daemon's cfg file or something like that. Best regards. HF
Hi, Alexey! On Jun 21, Alexey Botchkov wrote:
Hi, Sergei.
Here is the plugin code for you to check out. It constists of a single new file, so i send you the link to it. http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c
See my review attached. Comments are marked by // at the start of the line.
What's missing in it as i'm not sure how it can be done: - No server host/ip in the log. How get that information?
What server host/ip?
- SYSLOG logging. The SysLog i knew was declared in the syslog.h. That's the interface talking to the local syslogd instance. If it's what we need to use, then i have no idea how to specify the Syslog server IP for that daemon (which is required by the spec). I see nothing in the API about it and i thought it should be configured with the daemon's cfg file or something like that.
Right. So don't implement that, just use openlog/syslog/closelog. Regards, Sergei
Hi, Sergei. Modified version of the file is in the same place:
What's missing in it as i'm not sure how it can be done: - No server host/ip in the log. How get that information?
What server host/ip?
I mean i don't know about the server's host/ip. The clien't one can be found in the event as i understand.
Right. So don't implement that, just use openlog/syslog/closelog.
done. From the file: line 169:
typedef struct server_audit_user_hash { unsigned char table[0x10000]; struct server_audit_user_rec userlist[USERLIST_SIZE_LIMIT]; int n_users; } SERVER_AUDIT_USER_HASH; // why? why did you invent your own hash, while we have a perfectly // usable hash in the server already?
I thought you'd going to like it :) Technically i didn't invent anything - just implemented the common algorithm locally. Works faster than the mysys/HASH invocation and not that much more lines of code. But ok - redone with the server's HASH. line 248:
static int write_log(const char *message, size_t len) { if (output_type == OUTPUT_FILE) return logger_write(logfile, message, len); return 0; } // better extend the logger service to support syslog
I don't think so. The logger service is naturally oriented on files with all these rotations and filesizes. And the SYSLOG interface is also pretty simple and natural for it's purpose. So i really don't like mixing these in one service. line 490:
servhost_len= strlen(servhost); error_header(); fprintf(stderr, "STARTED\n"); // really? I thought you need to log it in the log // preferrably with the plugin version, and may be the server version too
Spec states that we should report that events to the main server log. And it seems right to me. The logging in the plugin isn't necessarily enabled here, and we know for sure that the plugin is enabled if we see records from it in it's log. Best regards. HF On 06/22/2013 07:06 PM, Sergei Golubchik wrote:
Hi, Alexey!
On Jun 21, Alexey Botchkov wrote:
Hi, Sergei.
Here is the plugin code for you to check out. It constists of a single new file, so i send you the link to it. http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c
See my review attached. Comments are marked by // at the start of the line.
What's missing in it as i'm not sure how it can be done: - No server host/ip in the log. How get that information?
What server host/ip?
- SYSLOG logging. The SysLog i knew was declared in the syslog.h. That's the interface talking to the local syslogd instance. If it's what we need to use, then i have no idea how to specify the Syslog server IP for that daemon (which is required by the spec). I see nothing in the API about it and i thought it should be configured with the daemon's cfg file or something like that.
Right. So don't implement that, just use openlog/syslog/closelog.
Regards, Sergei
Hi, Alexey! On Jun 25, Alexey Botchkov wrote:
Modified version of the file is in the same place:
Ok
What's missing in it as i'm not sure how it can be done: - No server host/ip in the log. How get that information?
What server host/ip?
I mean i don't know about the server's host/ip. The clien't one can be found in the event as i understand.
Yes. But surely, you can find your own host/ip, cannot you? See gethostname(), getaddrinfo(), etc.
Works faster than the mysys/HASH invocation and not that much more lines of code. But ok - redone with the server's HASH.
Thanks!
line 248:
static int write_log(const char *message, size_t len) { if (output_type == OUTPUT_FILE) return logger_write(logfile, message, len); return 0; } // better extend the logger service to support syslog
I don't think so. The logger service is naturally oriented on files with all these rotations and filesizes. And the SYSLOG interface is also pretty simple and natural for it's purpose. So i really don't like mixing these in one service.
My thought was that to log whatever is needed, a plugin opens and configures a logger, and that's pretty much all it needs to do. Having a purely file-oriented logger, every plugin that wants syslog, would need to copy what you've done here, in this plugin. While having one logger that understands many destinations, would automatically make all plugins syslog-enabled. On the other hand, logging to syslog often means using a different logging format, which means plugins won't necessarily be automatically "syslog enabled". Ok, let's keep syslog separate for now. When we'll have more logs that go to syslog, we'll see if it makes sense to factor the common code out.
fprintf(stderr, "STARTED\n"); // really? I thought you need to log it in the log // preferrably with the plugin version, and may be the server version too
Spec states that we should report that events to the main server log. And it seems right to me. The logging in the plugin isn't necessarily enabled here, and we know for sure that the plugin is enabled if we see records from it in it's log.
Fine. Regards, Sergei
Hi, Serg. Here is the last version of the plugin for your review: http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c <http://myoffice.izhnet.ru/%7Ealex/server_audit/server_audit.c> I think I addressed there everything we discussed. Best regards. HF 25.06.2013 15:15, Sergei Golubchik wrote:
Hi, Alexey!
On Jun 25, Alexey Botchkov wrote:
Modified version of the file is in the same place: Ok
Hi, Alexey! On Jul 01, Alexey Botchkov wrote:
Hi, Serg.
Here is the last version of the plugin for your review: http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c <http://myoffice.izhnet.ru/%7Ealex/server_audit/server_audit.c>
I think I addressed there everything we discussed.
Could you send a complete patch instead, please? Looks ok, a couple of comments: #ifndef MARIADB_ONLY #undef MYSQL_SERVICE_LOGGER_INCLUDED #undef MYSQL_DYNAMIC_PLUGIN ^^^ why undefs? I wouldn't need to ask this question, if I'd have a complete patch, that includes file_logger.c ... if (gethostname(servhost, sizeof(servhost))) strcpy(servhost, "NA"); ^^^ It's usually written as "N/A" and means "not applicable". But the host name here is most certainly applicable. Better use "???" or "unknown" or even "localhost". And two more comments: 1. I presume the complete patch includes test cases for the new plugin. 2. Could you please move HASH into a service? In a separate changeset, of course. Moving an existing function into a service does not change the source code of plugins, so you can do it after you've pushed this audit plugin, if you'd like. Regards, Sergei
Sergei, The complete patch is here: http://lists.askmonty.org/pipermail/commits/2013-July/004880.html
#ifndef MARIADB_ONLY #undef MYSQL_SERVICE_LOGGER_INCLUDED #undef MYSQL_DYNAMIC_PLUGIN
^^^ why undefs?
I want the service_logger.h to be included in no-mysql-dynamic-plugin mode. But the rest of the services should be included normally, as i belive. So in the code i first do #define MYSQL_SERVICE_LOGGER_INCLUDED, so the #include <services.h> didn't actually included the service_logger.h. Then I do the #undef and #include <service_logger.h> separately.
Could you please move HASH into a service?
Ok, will do right after this task (so it won't be forgotten). But yes, I think it's better to do it separately. Best regards. HF 02.07.2013 14:34, Sergei Golubchik wrote:
Hi, Alexey!
On Jul 01, Alexey Botchkov wrote:
Hi, Serg.
Here is the last version of the plugin for your review: http://myoffice.izhnet.ru/~alex/server_audit/server_audit.c <http://myoffice.izhnet.ru/%7Ealex/server_audit/server_audit.c>
I think I addressed there everything we discussed. Could you send a complete patch instead, please? Looks ok, a couple of comments:
#ifndef MARIADB_ONLY #undef MYSQL_SERVICE_LOGGER_INCLUDED #undef MYSQL_DYNAMIC_PLUGIN
^^^ why undefs? I wouldn't need to ask this question, if I'd have a complete patch, that includes file_logger.c
... if (gethostname(servhost, sizeof(servhost))) strcpy(servhost, "NA"); ^^^ It's usually written as "N/A" and means "not applicable". But the host name here is most certainly applicable. Better use "???" or "unknown" or even "localhost".
And two more comments: 1. I presume the complete patch includes test cases for the new plugin. 2. Could you please move HASH into a service? In a separate changeset, of course. Moving an existing function into a service does not change the source code of plugins, so you can do it after you've pushed this audit plugin, if you'd like.
Regards, Sergei
participants (2)
-
Alexey Botchkov -
Sergei Golubchik