Re: pgaudit - an auditing extension for PostgreSQL

Abhijit,

* Abhijit Menon-Sen (ams@2ndQuadrant.com) wrote:
> 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".

I'm confused by this statement..  Having the role listed in
pgaudit.roles would still mean that everything that role does is logged.
Adding other roles to be audited would simply be 'GRANT audit TO
role1;'.

Is there a specific action which you don't think would be audited
through this?

> > > 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?

You can grant roles to other roles, which is how the role-level
auditing would be handled.  Hopefully that clarifies the idea here.

> > 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.

Right, because it's not flexible.  You can't say that you want r1 to
have X actions logged, with r2 having Y actions logged.

> 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.

I agree that it's valuable to support simple use-cases with simple
configurations.

> > 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.

One of the issues that I see is just how much of the code is under the
#ifdef's.  If this is going into contrib, we really shouldn't have
references and large code blocks that are #ifdef'd out implementations
based on out-of-core patches.  Further, the pieces which are under the
#ifdef's for DEPARSE would likely end up having to be under #if
PG_VERSION_NUM, as the deparse tree goes into 9.5.  Really, pgaudit
pre-deparse and post-deparse are quite different and having them all in
one pgaudit.c ends up being pretty grotty.  Have you considered
splitting pgaudit.c into multiple files, perhaps along the pre/post
deparse lines?

> > 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.

Ok, good.

> > 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?

One thought might be to provide the intersection between LOGSTMT and
ObjectTypes.  We can learn what commands are DDL and what are
modification commands from GetCommandLogLevel.  The other distinctions
are mostly about different object types and we might be able to use
ObjectPropertyType and ObjectTypeMap for that, perhaps adding another
'kind' to ObjectProperty for the object categorization.

There are still further distinctions and I agree that it's very useful
to be able to identify which commands are privilege-related
(T_GrantStmt, T_GrantRoleStmt, T_CreatePolicyStmt, etc) vs. things like
Vacuum.

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

That thought was based on looking at ObjectTypeMap- we might be able to
provide a way for administrators to configure which objects they want to
be audited by naming them using the names provided by ObjectTypeMap.

> > > /*
> > >  * 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 agree that log_audit_event() gets called multiple times, but there's
no 'collection' of AuditEvents, ever, is there?  An AuditEvent is
created and then passed to log_audit_event which then issues an ereport-
and that's it.  "collects" above implies queueing is happening, at least
to me.  Perhaps if you said "This module collects audit events" then
it'd be a bit clearer but as it stands, it's referencing a specific
structure.

> > 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.

I understand what you're getting at, but I'm not sure that 3 places
really rises to 'around the whole code'.  I'm also hopeful that it might
be possible to reduce that down to 2 places- commands which go through
the main Executor and therefore call under ExecutorCheckPerms and those
which go through ProcessUtility and fall under the ProcessUtility hook.

> > >         /*
> > >          * 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.)

Well, there's a number of other Executor hooks (Start/Run/Finish/End)
which are all passed QueryDesc, as an example.  I'm not sure that you'd
actually want to deal with using those specific hooks, but another hook
which is just run (instead of an instead-of type hook) and gets the
QueryDesc would certainly give you access to a lot more information
directly without having to back into it from the range table.

> > >         /*
> > >          * 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, ExecCheckRTPerms is
>     called and there's a hook there which the module can use to say "ok,
>     does the audit role have any permissions 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?

The key part above is "any".  We're using the grant system as a proxy
for saying "I want to audit column X".  That's different from "I want to
audit commands which would be allowed if I *only* had access to column
X".  If I want to audit access to column X, then:

select A, B, X from mytable;

Should be audited, even if the audit role doesn't have access to columns
A or B.

> > 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.

Ok, hopefully the above helps. :)

> > > /*
> > >  * 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.

Yeah- but are we always going to have to deal with a partial event
trigger set?  Shouldn't it be possible to cover the object access hook
cases with event triggers?
Thanks!
    Stephen