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@pgmasters.net