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

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> 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.

GRANT statements are subject to event triggers in the most recent code-
see ExecGrantStmt_oids, src/backend/catalog/aclchk.c:592 and
EventTriggerCollectGrant(), src/backend/commands/event_trigger.c:1822.

Perhaps I've missed something in my analysis, but I don't believe you're
correct. I can certainly understand the confusion as GRANT statement
which are granting *roles* are not subject to event triggers, as they
are shared objects and therefore outside the scope of what event
triggers support, but they are also not supported by the CREATE SCHEMA
command.

> 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 truely hope you're able to review more patches. I certainly would
appreciate any further insight you can give me on issues beyond the
three which you found.

> > 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 stand by the commits that I've made. There have been ones I've
reverted, ones that I've realized were a mistake and have subsequently
apologized for and worked to address, and others. I am certainly not
above question, nor do I feel that I'm any more able to commit bugless
patches than the next committer. I look forward to reviews of my past
and future commits.

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

It was my understanding that we do not direct what committers or
reviewers work on. Still, I encourage you to do so and, further, to
review the work authored by myself and committed by others, along with
the work I've done for pginfra. All-in-all, this ~1800 line contrib
module strikes me as a relatively modest amount of work relative to the
role system and RLS. I'm already planning to put resources (beyond
myself) into review of what's been committed to 9.5 over the next few
months, including commits that I've made, but more eyes are certainly
welcome.

To be clear, I'm not saying that I will not revert this, but I am trying
to respond to the concerns raised in the best possible way that I can.
I had been hoping that there would be some attempt to explain to me why
you feel that the module is bug ridden and the worst commit you've ever
seen. I can certainly understand concern with the GRANT-based design,
or with ereport() being used for the audit log and how those aren't
ideal and I could respect an argument being made that it's not
acceptable for us to provide even a contrib module which uses that
approach, but this attack is certainly not what I was expecting nor do I
feel it's appropriate for this community.

Thanks!

Stephen