Re: [COMMITTERS] pgsql: Add pg_audit, an auditing extension - Mailing list pgsql-hackers

On Tue, May 26, 2015 at 08:10:04PM -0400, Stephen Frost wrote:
> * Noah Misch (noah@leadboat.com) wrote:

> > > +    /*
> > > +     * Don't audit substatements.  All the substatements we care about should
> > > +     * be covered by the event triggers.
> > > +     */
> > > +    if (context <= PROCESS_UTILITY_QUERY && !IsAbortedTransactionBlockState())
> > 
> > They aren't covered.  A GRANT inside CREATE SCHEMA escapes auditing:
> 
> Actually, they are covered.  David and I discussed this extensively,
> prior to your review, and concluded that this approach works because the
> items not under the EventTrigger charter are shared catalogs, updates to
> which wouldn't ever make sense to happen under a CREATE SCHEMA.  I do
> hope that we are able to improve on EventTriggers by supporting them for
> shared catalogs one day, but that is a discussion for another thread.
> 
> The issue which you discovered here is that GRANTs were categorized
> under CLASS_ROLE, but we have a check at the top of the DDL event
> trigger which does an early-exit if DDL isn't selected for inclusion.
> That clearly needs to be changed, so that the GRANT under CREATE SCHEMA
> is caught and logged properly.

The test case demonstrated a hole in GRANT auditing, and your diagnosis is
incorrect.  GRANT statements aren't subject to event triggers.  Selecting DDL
too, with pg_audit.log=all, does not audit the GRANT substatement.

> > Stephen, please revert this feature.  Most pg_audit bugs will create security
> > vulnerabilities, so incremental development in the PostgreSQL tree is the
> > wrong approach.  The feature needs substantial hacking, then an additional
> > committer's thorough review.  (The above sampling of defects and weak areas
> > was not a thorough review.)

pg_audit already has enough demonstrated bugs to be the most defective commit
I have ever studied.  Its defect level, routine for a mere Ready for Committer
patch, is unprecedented among committed work.  If that fails to astonish, look
at you continuing to _defend pg_audit's integrity_:

> I don't believe that this module is particularly more bug-ridden than
> other contrib modules or even parts of core.

Your negligence with respect to this commit calls into question every one of
your past commits and anything you might commit for years to come.

> I certainly welcome review from others and if there is not another
> committer-level formal review before we get close on 9.5 (say, end of
> August), then I'll revert it.  There is certainly no concern that doing
> so would be difficult to do, as it is entirely self-contained.

Not good enough.  I need those would-be reviewers' help reviewing ~100 other
sfrost commits before 9.5 final.

nm