Re: Auditing extension for PostgreSQL (Take 2) - Mailing list pgsql-hackers

On Mon, May 11, 2015 at 9:07 PM, David Steele <david@pgmasters.net> wrote:
> On 5/1/15 5:58 AM, Sawada Masahiko wrote:
>> For now, since pg_audit patch has a pg_audit_ddl_command_end()
>> function which uses part of un-committed "deparsing utility commands"
>> patch API,
>> pg_audit patch is completely depend on that patch.
>> If API name, interface are changed, it would affect for pg_audit patch.
>> I'm not sure about progress of "deparsing utility command" patch but
>> you have any idea if that patch is not committed into 9.5 until May
>> 15?
>
> The attached v12 patch removes the code that became redundant with
> Alvaro committing the event trigger/deparse work.  I've updated the
> regression tests to reflect the changes, which were fairly minor and add
> additional information to the output.  There are no longer any #ifdef'd
> code blocks.

This is not a full review, but just a few thoughts...

What happens if the server crashes?  Presumably, audit records emitted
just before the crash can be lost, possibly even if the transaction
went on to commit.  That's no worse than what is already the case for
regular logging, of course, but it's maybe a bit more relevant here
because of the intended use of this information.

+            if (audit_on_relation(relOid, auditOid, auditPerms))
+            {
+                auditEventStack->auditEvent.granted = true;
+            }

Braces around single-statement blocks are not PostgreSQL style.

I wonder if driving the auditing system off of the required
permissions is really going to work well.  That means that decisions
we make about permissions enforcement will have knock-on effects on
auditing.  For example, the fact that we check permission on a view
rather than on the underlying tables will (I think) flow through to
how the auditing happens.

+            stackItem->auditEvent.commandTag == T_DoStmt &&

Use IsA(..., DoStmt) for this kind of test.  There are many instances
of this pattern in the patch; they should al be fixed.

Using auditLogNotice to switch the log level between LOG and NOTICE is
weird.  Switching from LOG to NOTICE is an increase in the logging
level relative to client_min_messages, but a decrease relative to
log_min_messages.   With default settings, logging at LOG will go only
to the log (not the client) and logging at NOTICE will go only to the
client (not the log).

The documentation and comments in this patch are, by my estimation,
somewhat below our usual standard.  For example:

+       DefineCustomBoolVariable("pg_audit.log_notice",
+                                                        "Raise a
notice when logging",

Granted, there is a fuller explanation in the documentation, but that
doesn't make that explanation particularly good.  (One might also
wonder why the log level isn't fully configurable instead of offering
only two choices.)

Here's another example:

+       DefineCustomBoolVariable("pg_audit.log_parameter",
+                                                        "Enable
statement parameter logging",

It would be far better to say "lnclude the values of statement
parameters in auditing messages" or something like that.  It is quite
clear that this GUC doesn't enable some sort of general statement
parameter logging facility; it changes the contents of auditing
messages that would have been emitted either way.

I don't want to focus too much on this particular example.  The
comments and documentation really deserve a bit more attention
generally than they have gotten thus far.  I am not saying they are
bad.  I am just saying that, IMHO, they are not as good as we
typically try to make them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company