Re: [COMMITTERS] pgsql: Add pg_audit, an auditing extension

Tom,

Many thanks for your comments and for jumping into this discussion.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Joshua D. Drake (jd@commandprompt.com) wrote:
> >> It seems to me that perhaps the solution is then to pull pg_audit
> >> into user space and instead work on a general solution (an API?
> >> custom backend?) that provides what is needed.
>
> > I am planning on working on the necessary changes to core to include
> > auditing capabilities.  Hopefully, that will go more smoothly.  I do not
> > believe that auditing should or even can be an external module, indeed,
> > pg_audit was exactly that attempt and, even with significant resources
> > put into it over the past year, it falls far short of what any
> > organization who is familiar with the capabilities in other RDBMSs would
> > expect.  That doesn't mean that I feel it's bad- it's a step in the
> > right direction, but it isn't a complete solution.
>
> I'm fairly confused by your line of argument.  If auditing can be done
> by non-core code, then there is no great urgency to have it as a contrib
> module.  If it can't be done by non-core code, then creating a contrib
> module is just a dead end that will soon leave nothing behind except
> backwards-compatibility problems.  Our experience with pulling contrib
> modules into core has not been good in that respect.

I consider it similar to dblink and FDWs.  dblink is non-core code which
allows querying other servers, but is ultimately a dead-end as,
hopefully, we will eventually replace all of its capabilities with
proper FDW support.  Does that mean that it was a poor move to include
it?  No; it provided an extremely useful capability which a lot of users
have been quite happy with.  There are warts all over it and limitations
to what it can do, issues with how it doesn't have any kind of analyze
like information, no way for optimization to consider how best to
integrate the query being run through dblink, etc, but would I use it?
Absolutely.  Is it a dead-end?  Certainly, it'll eventually be
completely replaced by FDWs.

> As far as I can tell, pg_audit at this point is most charitably described
> as "experimental", and I'm not sure that we want to put it into contrib
> on that basis.  Of late we've generally acted as though contrib modules
> have the same kind of cross-version compatibility expectations that core
> code does.  It seems to me that that sort of promise is *way* premature
> in this case; but I'm not seeing any large red warnings in pgaudit.sgml
> that the described facilities are subject to change.

You're correct- we should probably be putting something into the
documentation which warns that the interfaces and auditing capabilities
are likely to change in the future.  On the other hand, I have been
understandably chided regarding documentation changes which attempt to
predict the future (see: changing include_realm).  Further, as was
discussed earlier this year, should we have a close enough match between
pg_audit and in-core auditing support (something I'm certainly hopeful
for- see the discussion about just how close the GRANT syntax used is to
the AUDIT command in a certain very large RDBMS whose name starts with
'O'), we could provide a perl script or similar to facilitate the
migration for users of the module to an in-core solution.

> Quite aside from the question of whether we're ready to put a stake in the
> ground about user-visible features of an audit facility, there seem to be
> a lot of technical issues here:
>
> * Do we have adequate hooks in the backend with which auditing code can
> detect events of interest (with acceptably low overhead)?  I dunno, but
> if we do not, having a contrib module doesn't fix it.

I certainly agree with you here.  The hooks in the Executor and
ProcessUtility really aren't *ideal*, but it turns out that they
actually are *sufficient* in a large number of cases.  The addition of
the event triggers/deparse code has helped that a great deal because it
provides information which we weren't able to get previously.  That was
a rather late addition to the tree overall and I believe lead to the
issue found regarding CREATE SCHEMA; it's not unreasonable for
integration of such large changes like these late in the release cycle
to least to an issue or two.

There was quite a bit of consideration over the auditing information
which was being collected, starting from the module as proposed last
year around this time which covered one set of auditing, to the module
as it is today which provides a somewhat different but largely
overlapping and significantly more granular set of auditing information.
Certainly, operating through the hooks and with the other capabilities
offered by the extension system has been difficult and the module, as a
whole, would be far simpler if in core, but I'm of the firm belief that
the proposed extension will address many use-cases, not address many
others, and provide extremely useful feedback for us as we work to
develop a better, in-core, solution.

> * Do we have an adequate design for specifying which out of the possible
> auditable events should be logged?  I dunno about this either, but it
> seems like this is an area best kept out of core if at all possible.
> The design space seems pretty vast and I doubt that one size will fit all,
> or needs to fit all.

This is an excellent point for consideration.  One of the big concerns
which has been raised regarding this feature has been how auditable
events are defined and tracked, because this module leverages the GRANT
system for something it was not designed for.  As it turns out, however,
the GRANT system has a great deal of similarity with regard to the
specificity of what users would like to be logged- again, I would draw
the line between the GRANT command and the AUDIT command implemented in
other systems.  AUDIT INSERT ON mytable; isn't far from GRANT INSERT ON
mytable to joe;.  That's not to say that this is complete; as noted
previously, this comes very close to what a certain v10 (or v11?  it's
late and I forget) AUDIT capability that another RDBMS has, but the v12
version includes more.  Still- is this a useful capability, even given
the limitations?  Absolutely, based on the discussions I've had.
Indeed, the entire GRANT-based approach was suggested by an individual
in the finance community who saw how well it matched up to their needs.

> * Do we have an appropriate mechanism for performing logging of events
> that we've decided to log?  Here I think the answer is unquestionably
> "no"; basing audit logging on the existing elog mechanism with no changes
> is simply broken.  elog is designed to be flexible about whether messages
> get reported and to where, which is exactly what you do *not* want for
> audit logs.  This might not be too hard to fix, eg add another elevel with
> hard-wired rules about where it goes ... but the current patch didn't do
> that.  A larger problem is that maybe the audit message stream shouldn't
> go to the postmaster log in the first place, but someplace else.

pg_audit tried quite hard to avoid changing core.  Changing where
logging goes for different kinds of events has been on my personal to-do
list for years and, as Peter pointed out very plainly recently, it's not
been done yet.  I certainly agree with you regarding this point, but
would such a hard-wired rule have been acceptable?  I sincerely doubt
it; I know that I wouldn't have been happy with it and it would have
been a stop-gap measure rather than a proper design.  I feel bad about
attempting to predict the future here, given the discussion, but I will
go out on a limb and say that this is one of the specific pieces which
I was planning to address next as it is extremely important and will
require serious consideration to ensure that we don't break error
logging as we change it.  This is the kind of change which will require
an entire release cycle to handle, isn't directly related to auditing
but at the same time required by it, and which is beyond what I would
consider my personal level of capability to handle by myself.  I'm
certainly hopeful that others will be willing to step up and help with
it, but I have little doubt that, had I tried to change the logging
system along with adding a contrib module which simply uses it, I would
have broken things badly.

That is not to say that I feel that using the elog system makes the
module useless on its face.  It certainly reduces the number of use
cases for which it can be used, and that's unfortunate, but it's
certainly not a death knell.  Many environments which I've worked in
have very strict control over who can access the production systems and
have absolutely zero free-form user access other than by superusers.  As
was discussed in NY earlier this year, organizations have developed
entire proxy systems for managing access to PG through controlled
systems which also provide certain amounts of auditing, to address the
lack of any support in this area.  That's beyond what I've personally
done but is certainly another point to consider.

> * Do we have an appropriate mechanism for configuring the audit facility?
> I'm not totally sure, but again I think that the existing GUC mechanisms
> were not designed for this sort of thing and are probably too easy to
> subvert.  (It might be hopeless to try to ensure that superusers can't
> escape auditing, but as it stands pg_audit doesn't even pretend to try.
> Even without the possibility of intentional subversion, there are too
> many ways to turn auditing off by accident.)

I find it a bit bizarre that we would hope to "try" when it comes to
preventing superusers from escaping auditing, particularly in a contrib
module.  Indeed, wouldn't that simply open up an avenue of never ending
attacks regarding such an attempt?  I can think of any number of ways
which auditing could be subverted, broken, disabled, or otherwise
escaped by a superuser, and I don't consider myself anywhere near the
most intelligent or creative individual in the room.  Even an in-core
solution, in my view at least, is going to have to accept that
superusers won't be contained or guaranteed to be audited, or at least,
not with outside help (eg: SELinux or similar capability).

> On the whole, I think sticking this into contrib is premature.  What it
> does today could be done just as well as a third-party extension.  What
> it doesn't do today, we should work on, but I'm unconvinced that having
> this in contrib will make it easier to get there.

You are certainly correct that evertyhing pg_audit does today could be
done just as well as a third-party extension- just like everything which
exists inside of contrib.  Having it in contrib will *greatly* increase
the visibility of the capability and the amount of feedback we will get
regarding it.  For evidence of this, I would submit this:

http://www.depesz.com/2015/05/22/waiting-for-9-5-add-pg_audit-an-auditing-extension/

To be clear- I am not suggesting that we have to consider this when
deciding if the capability should be reverted or not; I've not brought
it up in the earlier discussions at all, but I felt it germane to this
discussion as to if having something in contrib changes anything
regarding how a particular module is viewed by the outside world.  It
is, absolutely, a game changer, and for a feature which we hope to
eventually fold into core, having the community see how it's used, what
issues come up, what concerns are raised, and how the limitations impact
its uptake, is quite useful to guide a proper in-core solution and feed
into the design of the capability.

Tom, as this is your first mail on the subject, I'd ask that you
consider my comments, but I have no problem if you feel they are not
sufficient to sway your feeling that the addition of pg_audit is
premature.  I'm very glad to hear that, at a high level, you are in
support of having such a capability eventually added.  I don't mean to
draw this out, but I did want to answer your questions, as best I could,
as to why I felt this was progress.  Long story short, I'm happy to pull
it out if my responses don't sway you.
Thanks!
    Stephen