Re: pgaudit - an auditing extension for PostgreSQL

Hello Stephen.

Thanks very much for having a look at the revised pgaudit.

At 2015-01-18 22:28:37 -0500, sfrost@snowman.net wrote:
>
> > 2. Set pgaudit.roles = 'r1, r2, …', which logs everything done by r1,
> >    r2, and any of their descendants.
>
> If these roles were the 'audit' roles as considered under #1 then
> couldn't administrators control what pgaudit.roles provide today using
> role memberships granted to the roles listed?

Yes. But then there would be no convenient way to say "just log
everything this particular role does".

> > 3. Set pgaudit.log = 'read, write, …', which logs any events in any
> >    of the listed classes.
> 
> Approach #1 addresses this too, doesn't it?

Approach #1 applies only to things you can grant permissions for. What
if you want to audit other commands?

> As currently implemented, role-level auditing is required to have DDL
> be logged, but you may very well want to log all DDL and no DML, for
> example.

Well, as formerly implemented, you could do this by adding r1 to .roles
and then setting .log appropriately. But you know that already and, if
I'm not mistaken, have said you don't like it.

I'm all for making it possible to configure fine-grained auditing, but
I don't think that should come at the expense of being able to whack
things with a simpler, heavier^Wmore inclusive hammer if you want to.

> My feeling is that we should strip down what goes into contrib to be
> 9.5 based.

I do not think I can express a constructive opinion about this. I will
go along with whatever people agree is best.

> I'm also wondering if pieces of that are now out-of-date with where
> core is.

Yes, they are. I'll clean that up.

> I don't particularly like how pgaudit will need to be kept in sync
> with what's in ProcessUtility (normal and slow).

Sure, it's a pain.

> I'd really like to see this additional information regarding what kind
> a command is codified in core.  Ideally, we'd have a single place
> which tracks the kind of command and possibly other information which
> can then be addressed all at once when new commands are added.

What would this look like, and is it a realistic goal for 9.5?

> Also, we could allow more granularity by using the actual classes
> which we already have for objects.

Explain?

> > /*
> >  * This module collects AuditEvents from various sources (event
> >  * triggers, and executor/utility hooks) and passes them to the
> >  * log_audit_event() function.
> >  *
> >  * An AuditEvent represents an operation that potentially affects a
> >  * single object. If an underlying command affects multiple objects,
> >  * multiple AuditEvents must be created to represent it.
> >  */
> 
> The above doesn't appear to be accurate (perhaps it once was?) as
> log_audit_event() only takes a single AuditEvent structure at a time
> and it's not a list.  I'm not sure that needs to change, just pointing
> out that the comment appears to be inaccurate.

If multiple AuditEvents are created (as the ExecutorCheckPerms_hook may
do), log_audit_event() will be called multiple times. The comment is
correct, but I'll try to make it more clear.

> I'm inclined to suggest that the decision be made earlier on about if
> a given action should be logged, as the earlier we do that the less
> work we have to do and we could avoid having to pass in things like
> 'granted' to the log_audit_event function at all.

I considered that, but it makes it much harder to review all of the
places where such decisions are made. It's partly because I wrote the
code in this way that I was able to quickly change it to work the way
you suggested (at least once I understood what you suggested).

I prefer the one-line function and a few «if (pgaudit_configured())»
checks (to avoid creating AuditEvents if they won't be needed at all)
and centralising the decision-making rather than spreading the latter
around the whole code.

> > /*
> >  * Takes a role OID and returns true if the role is mentioned in
> >  * pgaudit.roles or if it inherits from a role mentioned therein;
> >  * returns false otherwise.
> >  */
> > 
> > static bool
> > role_is_audited(Oid roleid)
> > {
> > …
> 
> The results here should be cached, as was discussed earlier, iirc.

I'll look into that, but it's a peripheral matter.

> Whatever is considered 'noise' should at least be explicitly listed,
> so we know we're covering everything.

OK.

> > /*
> >  * Takes an AuditEvent and, if it should_be_logged(), writes it to the
> >  * audit log. The AuditEvent is assumed to be completely filled in by
> >  * the caller (unknown values must be set to "" so that they can be
> >  * logged without error checking).
> >  */
> > 
> > static void
> > log_audit_event(AuditEvent *e)
> > {
> 
> So, clearly, this is an area which could use some improvement.
> Specifically, being able to write to an independent file instead of just
> using ereport(), supporting other output formats (JSON, maybe even a
> table..).  Also, have you considered using a dynamic shared memory block
> to queue the logging messages to and then a background worker to write
> them out in-order to a file?  It'd clearly be similar to our existing
> ereport() mechanism, but importantly you'd be able to send it to another
> file.  Being able to log directly to syslog or other message queues
> would be great also.
> 
> These are things which could be added later, of course.

I have considered all of those things and, as I have said before, taken
a conscious decision to not do any of them. I strongly feel they should
be added only later, after we have agreed on the basic approach.

> >         /*
> >          * 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.
> >          */
> 
> This seems like it could be improved- have you considered suggesting a
> hook which is, perhaps, in a better place and could pass in this
> information instead of having to attempt to regenerate it?

No. (I don't see how that could work.)

> >         /*
> >          * If a role named "audit" exists, we check if it has been
> >          * granted permission to perform the operation identified above.
> >          * If so, we must log the event regardless of the static pgaudit
> >          * settings.
> >          */
> 
> This isn't quite what was intended.  This isn't about what the audit
> role could do but rather auditing specifically what the role has
> access to.

This is what you said earlier:
   «The magic "audit" role has SELECT rights on a given table.  When   any user does a SELECT against that table,
ExecCheckRTPermsis   called and there's a hook there which the module can use to say "ok,   does the audit role have
anypermissions here?" and, if the result   is yes, then the command is audited.»
 

As far as I can tell, that's exactly what the code does. In what way
have I misunderstood your suggestion now?

> As an example, if an UPDATE with a WHERE clause is executed and the
> audit user has SELECT rights on the table, then the UPDATE should be
> logged due to the WHERE clause.

Sorry, that makes my head hurt. I will think about it and respond later.

> > /*
> >  * Create AuditEvents for certain kinds of CREATE and ALTER statements,
> >  * as detected by log_object_access() in lieu of event trigger support
> >  * for them.
> >  */
> 
> Just to comment on this, the object access framework feels like it
> should, perhaps, be reworked to be based on event triggers..

The point of that code in the object access hook is to stand in when
event triggers are not available.

> > log_function_execution(Oid objectId)
> > {
> > …
> 
> This doesn't address overloaded functions, does it?  It probably
> should include the arguments and types in 'name'.

OK. I'll look into that.

-- Abhijit