Re: Auditing extension for PostgreSQL (Take 2)

Hi Stephen,

Thanks for your review. All fixed except for comments below:

On 2/17/15 10:34 AM, Stephen Frost wrote:
>> + /*
>> + * Check privileges granted indirectly via role memberships. We do this in
>> + * a separate pass to minimize expensive indirect membership tests. In
>> + * particular, it's worth testing whether a given ACL entry grants any
>> + * privileges still of interest before we perform the has_privs_of_role
>> + * test.
>> + */
>
> I'm a bit on the fence about this.. Can you provide a use-case where
> doing this makes sense..? Does this mean I could grant admin_role1 to
> audit and then get auditing on everything that user1 has access to?
> That seems like it might be useful for environments where such roles
> already exist though it might end up covering more than is intended..

The idea is that if there are already ready-made roles to be audited
then they don't need to be reconstituted for the audit role. You could
just do:

grant admin_role to audit;
grant user_role to audit;

Of course, we could list multiple roles in the pg_audit.role GUC, but I
thought this would be easier to use and maintain since there was some
worry about GUCs being fragile when they refer to database objects.

>> +################################################################################
>> +# test.pl - pgAudit Unit Tests
>> +################################################################################
>
> This is definitiely nice..
>
>> diff --git a/doc/src/sgml/pgaudit.sgml b/doc/src/sgml/pgaudit.sgml
>> new file mode 100644
>> index 0000000..f3f4ab9
>> --- /dev/null
>> +++ b/doc/src/sgml/pgaudit.sgml
>> @@ -0,0 +1,316 @@
>> +<!-- doc/src/sgml/pgaudit.sgml -->
>> +
>> +<sect1 id="pgaudit" xreflabel="pgaudit">
>> + <title>pg_audit</title>
>
> There seems to be a number of places which are 'pgaudit' and a bunch
> that are 'pg_audit'. I'm guessing you were thinking 'pg_audit', but
> it'd be good to clean up and make them all consistent.

Fixed, though I still left the file name as pgaudit.sgml since all but
one module follow that convention.

>> + <para>
>> + Session auditing allows the logging of all commands that are executed by
>> + a user in the backend. Each command is logged with a single entry and
>> + includes the audit type (e.g. <literal>SESSION</literal>), command type
>> + (e.g. <literal>CREATE TABLE</literal>, <literal>SELECT</literal>) and
>> + statement (e.g. <literal>"select * from test"</literal>).
>> +
>> + Fully-qualified names and object types will be logged for
>> + <literal>CREATE</literal>, <literal>UPDATE</literal>, and
>> + <literal>DROP</literal> commands on <literal>TABLE</literal>,
>> + <literal>MATVIEW</literal>, <literal>VIEW</literal>,
>> + <literal>INDEX</literal>, <literal>FOREIGN TABLE</literal>,
>> + <literal>COMPOSITE TYPE</literal>, <literal>INDEX</literal>, and
>> + <literal>SEQUENCE</literal> objects as well as function calls.
>> + </para>
>
> Ah, you do have a list of what objects you get fully qualified names
> for, nice. Are there obvious omissions from that list..? If so, we
> might be able to change what happens with objectAccess to include them..

It seems like these are the objects where having a name really matters.
I'm more interested in using the deparse code to handle fully-qualified
names for additional objects rather than adding hooks.

>> + <sect3>
>> + <title>Examples</title>
>> +
>> + <para>
>> + Set <literal>pgaudit.log = 'read, ddl'</literal> in
>> + <literal>postgresql.conf</literal>.
>> + </para>
>
> Perhaps I missed it, but it'd be good to point out that GUCs can be set
> at various levels. I know we probably say that somewhere else, but it's
> particularly relevant for this.

Yes, it's very relevant for this patch. Do you think it's enough to
call out the functionality, or should I provide examples? Maybe a
separate section just for this concept?

Patch v2 is attached for all changes except the doc change above.

--
- David Steele
david(at)pgmasters(dot)net