Date: Mon, 23 Mar 2015 11:01:46 +0530
From: Abhijit Menon-Sen <ams@2ndQuadrant.com>
To: David Steele <david@pgmasters.net>
Cc: Stephen Frost <sfrost@snowman.net>,
 =?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@2ndquadrant.com>,
 pgsql-hackers@postgresql.org
Subject: Re: Auditing extension for PostgreSQL (Take 2)
Message-ID: <20150323053146.GA1919@toroid.org>
References: <54E005CC.1060605@pgmasters.net>
 <20150217153402.GQ6717@tamriel.snowman.net>
 <54EB4E56.7050102@pgmasters.net> <54ECA551.8060306@pgmasters.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <54ECA551.8060306@pgmasters.net>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

At 2015-02-24 11:22:41 -0500, david@pgmasters.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers