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

Heikki,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 05/28/2015 06:04 AM, Joshua D. Drake wrote:
> >On 05/27/2015 07:02 PM, Stephen Frost wrote:
> >>regardless of if they are included in the main git repo
> >>or not.  Being in the repo represents the support of the overall
> >>community and PGDG, which is, understandably in my view, highly valuable
> >>to the organizations which look to PostgreSQL as an open source
> >>solution for their needs.
> >
> >I can't get behind this. Yes, there is a mysticism about the "core"
> >support of modules but it is just that "mysticism". People are going to
> >run what the experts tell them to run. If pg_audit is that good the
> >experts will tell people to use it...
>
> Yeah, there are many popular tools and extensions that people use
> together with PostgreSQL. PostGIS, pgbouncer, slony, pgbarman etc.
> The criteria for what should be in contrib, in core, or a 3rd party
> extension is a contentious topic.

Thanks!  I certainly agree.  PostGIS is relatively easy to reason about-
there is an entire community with a long history there also.  pgbouncer
and the others are not quite to that level.  I agree it's a contentious
topic and perhaps has been overly stressed.

> The argument here is basically that if something is in core, it's
> officially supported and endorsed by the PGDG. If that's the
> argument for putting this in contrib, then you have to ask yourself
> if the PGDG community is really willing to endorse this. I'm hearing
> a lot of pushback from other committers, which points to "no", even
> if all their technical arguments and worries turn out to be wrong.

That's absolutely a fair point and one which I take seriously.  You're
right to point out that, even if one committer is behind a particular
feature, that others may not be and therefore it's not really community
supported.  Clearly there is some general risk over time that contrib
modules and features will be abandoned by the original authors and have
to be taken up by others committers, but if there isn't support for the
initial version then that's pretty unlikely and it could certainly
deteriorate the reputation which PostgreSQL has earned.

> I wrote the above without looking at the code or the documentation.
> I haven't followed the discussion at all. I'm now looking at the
> documentation, and have some comments based on just that:

Understood, and thanks for reviewing the documentation.  I have to admit
that, just based on the above (I've not read all of the below quite
yet), you make an excellent argument and one which I understand and
agree with regarding the position of this particular module.

> * I think it's a bad idea for the audit records to go to the same
> log as other messages. Maybe that's useful as an option, but in
> general there is no reason to believe that the log format used for
> general logging is also a good format for audit logging. You
> probably wouldn't care about process ID for audit logging, for
> example. Also, how do you prevent spoofing audit records with
> something like "SELECT \nAUDIT: SESSION, 2, ...". Maybe the log
> formatting options can be used to prevent that, but just by looking
> at the examples in the manual, I don't see how to do it.

I entirely agree with you here- the existing logging structure was used
as there is not a trivial way to use another and still support
file-based logging.  We could limit pg_audit to syslog only or to only
support some other mechanism, but that tends to be even further limiting
than the existing logging system.  As I mentioned in my response to Tom,
this is absolutely an area which needs improvement.

> * I don't understand how the pg_audit.role setting and the audit
> role system works.

No problem, I'm happy to explain it.  Essentially, the 'role' set there
is the role checked for as a target of GRANT commands, which are used as
a proxy for AUDIT commands, as we can't add metadata to tables from
extensions today.

> * Using GUCs for configuring it seems like a bad idea, because:

I certainly agree with this, we need much more flexibility than what
GUCs can provide but that's not easily done from an extension.  This was
always a compromise between based on what capabilities are provided for
extensions from core and GUCs tend to be an "easy" answer, though
certainly not always the correct one.

> 1. it's not flexible enough. How do you specify that all READs on
> super_secret table must be logged, but on less_secret table, it's
> enough to log only WRITEs?

This is actually what that pg_audit.role setting was all about.  To do
the above, you would do:

CREATE ROLE pgaudit;
SET pg_audit.role = pgaudit;
GRANT SELECT ON TABLE super_secret TO pgaudit;
GRANT INSERT, UPDATE, DELETE ON TABLE less_secret TO pgaudit;

With this, all commands executed which require SELECT rights on the
table super_secret are logged, and all commands execute which require
INSERT, UPDATE, or DELETE on the less_secret table are logged.

> 2. GUCs can be changed easily on-the-fly, and configured flexibly.
> But that's not really what you want for auditing. You want to have a
> clear specification in one place. You'd want it to be more like
> pg_hba.conf than postgresql.conf. Or more like Mandatory Access
> Control, rather than Discretionary Access Control.

I certainly appreciate the MAC vs. DAC discussion here, but I don't
believe our AUDIT capability should explicitly require restarts of PG to
be adjusted.  We could certainly require that of the GUCs, but it's
unclear to me how that would really be beneficial.  What matters is
controlling how those parameters are changed, I believe.

> A separate config file in $PGDATA would be a better way to configure
> this. You would then have all the configuration in one place, and
> that file could have a more flexible format, with per-schema rules
> etc. (Wouldn't have to implement all that in the first version, but
> that's the direction this should go to)

The difficulty with a separate config file is that we don't have any way
of properly attaching information to the existing tables in the
database- table renames, dropped columns, etc, all become an issue then.

> I recommend making pg_audit an external extension, not a contrib
> module. As an extension, it can have its own life-cycle and not be
> tied to PostgreSQL releases. That would be a huge benefit for
> pg_audit. There is a lot of potential for new features to be added:
> more flexible configuration, more details to be logged, more ways to
> log, email alerts, etc. As an extension, all of those things could
> be developed independent of the PostgreSQL life-cycle. If there is
> need to fix vulnerabilities or other bugs, those can also be fixed
> independently of PostgreSQL minor releases.

I'm certainly all about adding new capabilities to pg_audit, but, as
discussed elsewhere, I believe we really want many of those same
capabilities in core (eg: being able to send logs to different places;
my thinking is a rabbitMQ type of store rather than email, but perhaps
we could support both..) and that's what I'm hoping to work towards in
the near future.

Still I do understand that there is little support for including this as
an extension among the general PostgreSQL community and, in particular,
the top committers.  I really do appreciate your feedback on this.
Thanks again!
    Stephen