Re: Auditing extension for PostgreSQL (Take 2)

At 2015-02-24 11:22:41 -0500, david(at)pgmasters(dot)net wrote:
>
> Patch v3 is attached.
>
> […]
>
> +/* Log class enum used to represent bits in auditLogBitmap */
> +enum LogClass
> +{
> + LOG_NONE = 0,
> +
> + /* SELECT */
> + LOG_READ = (1 << 0),
> +
> + /* INSERT, UPDATE, DELETE, TRUNCATE */
> + LOG_WRITE = (1 << 1),
> +
> + /* DDL: CREATE/DROP/ALTER */
> + LOG_DDL = (1 << 2),
> +
> + /* Function execution */
> + LOG_FUNCTION = (1 << 4),
> +
> + /* Function execution */
> + LOG_MISC = (1 << 5),
> +
> + /* Absolutely everything */
> + LOG_ALL = ~(uint64)0
> +};

The comment above LOG_MISC should be changed.

More fundamentally, this classification makes it easy to reuse LOGSTMT_*
(and a nice job you've done of that, with just a few additional special
cases), I don't think this level is quite enough for our needs. I think
it should at least be possible to specifically log commands that affect
privileges and roles.

I'm fond of finer categorisation for DDL as well, but I could live with
all DDL being lumped together.

I'm experimenting with a few approaches to do this without reintroducing
switch statements to test every command. That will require core changes,
but I think we can find an acceptable arrangement. I'll post a proof of
concept in a few days.

> + * Takes an AuditEvent and, if it log_check(), writes it to the audit
> log.

I don't think log_check is the most useful name, because this sentence
doesn't tell me what the function may do. Similarly, I would prefer to
have log_acl_check be renamed acl_grants_audit or similar. (These are
all static functions anyway, I don't think a log_ prefix is needed.)

> + /* Free the column set */
> + bms_free(tmpSet);

(An aside, really: there are lots of comments like this, which I don't
think add anything to understanding the code, and should be removed.)

> + /*
> + * We don't have access to the parsetree here, so we have to generate
> + * the node type, object type, and command tag by decoding
> + * rte->requiredPerms and rte->relkind.
> + */
> + auditEvent.logStmtLevel = LOGSTMT_MOD;

(I am also trying to find a way to avoid having to do this.)

> + /* Set object type based on relkind */
> + switch (class->relkind)
> + {
> + case RELKIND_RELATION:
> + utilityAuditEvent.objectType = OBJECT_TYPE_TABLE;
> + break;

This occurs elsewhere too. But I suppose new relkinds are added less
frequently than new commands.

Again on a larger level, I'm not sure how I feel about _avoiding_ the
use of event triggers for audit logging. Regardless of whether we use
the deparse code (which I personally think is a good idea; Álvaro has
been working on it, and it looks very nice) to log extra information,
using the object access hook inevitably means we have to reimplement
the identification/classification code here.

In "old" pgaudit, I think that extra effort is justified by the need to
be backwards compatible with pre-event trigger releases. In a 9.5-only
version, I am not at all convinced that this makes sense.

Thoughts?

-- Abhijit