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

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> At least on dromedary, this seems to be the problem:
>> 
>> pg_audit.c: In function 'stack_pop':
>> pg_audit.c:387: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
>> pg_audit.c: In function 'stack_valid':
>> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 3 has type 'int64'
>> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
>> pg_audit.c: In function 'log_audit_event':
>> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 4 has type 'int64'
>> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument 5 has type 'int64'
>> 
>> Will push a fix shortly and we'll see what happens.

> Ah, ok.

Pushed that, but some further notes:

* The actual audit reports ought to be ereport() not elog().  I made them
so but did not insert an errcode().  ISTM that it would likely be a good
thing to assign a not-used-for-any-other-purpose errcode for this, but I'm
not terribly sure what category to put it in.

* The comments in the code betray utter ignorance of how logging actually
works, in particular this:
* Administrators can choose which log level the audit log is to be logged* at.  The default level is LOG, which goes
intothe server log but does* not go to the client.  Set to NOTICE in the regression tests.
 

All the user has to do is change client_min_messages and he'll see all the
reports, which means if you think that letting the user see the audit
reports is a security problem then you have a hole a mile wide.

(I assume BTW that we're not considering it a security problem that
superusers can trivially escape auditing.)
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Ah, ok.
>
> Pushed that, but some further notes:

Thanks!  Looking much better.

> * The actual audit reports ought to be ereport() not elog().  I made them
> so but did not insert an errcode().  ISTM that it would likely be a good
> thing to assign a not-used-for-any-other-purpose errcode for this, but I'm
> not terribly sure what category to put it in.

Right, I had seen that too and had intended to change it, but somehow
missed it in the other changes I was doing.  I'll take a look at the
categories and try to figure out what makes sense.

> * The comments in the code betray utter ignorance of how logging actually
> works, in particular this:
>
>  * Administrators can choose which log level the audit log is to be logged
>  * at.  The default level is LOG, which goes into the server log but does
>  * not go to the client.  Set to NOTICE in the regression tests.

I had rewored that last night and will reword it again to be more clear.

> All the user has to do is change client_min_messages and he'll see all the
> reports, which means if you think that letting the user see the audit
> reports is a security problem then you have a hole a mile wide.

There are certainly use-cases for this where that's not an issue and
also ones where the user wouldn't be able to use pg_audit due to this.

I'll update the docs to make the risk of this clear.  At least for the
use-cases we've been involved in, they've not been concerned about this.
Still, any thoughts you have on this would certainly be welcome.  I've
been thinking about how we might re-route and tag messages in the
backend for a number of years and I feel like this summer I'll have
resources and time to spend working towards it.  Providing a way to
decide if a message should be sent only to the server log, or only to
the client, or to an external system (syslog, pgQ, rabbitMQ, etc), or to
some combination of those, is definitely one of the items on that list.

> (I assume BTW that we're not considering it a security problem that
> superusers can trivially escape auditing.)

No, that's entirely understood and expected, which is one of the reasons
it'd be good to reduce the number of superusers running around.
Thanks!
    Stephen

On Thu, May 14, 2015 at 01:38:49PM -0400, Tom Lane wrote:
> * The comments in the code betray utter ignorance of how logging actually
> works, in particular this:
> 
>  * Administrators can choose which log level the audit log is to be logged
>  * at.  The default level is LOG, which goes into the server log but does
>  * not go to the client.  Set to NOTICE in the regression tests.
> 
> All the user has to do is change client_min_messages and he'll see all the
> reports, which means if you think that letting the user see the audit
> reports is a security problem then you have a hole a mile wide.

That indicates the patch's general readiness:

> +        /* These are DDL, unless they are ROLE */
> +        case LOGSTMT_DDL:
> +            className = CLASS_DDL;
> +            class = LOG_DDL;
> +
> +            /* Identify role statements */
> +            switch (stackItem->auditEvent.commandTag)
> +            {
> +                /* We know these are all role statements */
> +                case T_GrantStmt:
> +                case T_GrantRoleStmt:
> +                case T_CreateRoleStmt:
> +                case T_DropRoleStmt:
> +                case T_AlterRoleStmt:
> +                case T_AlterRoleSetStmt:
> +                    className = CLASS_ROLE;
> +                    class = LOG_ROLE;
> +                    break;

Not T_AlterDefaultPrivilegesStmt?

> +static void
> +pg_audit_ProcessUtility_hook(Node *parsetree,
> +                             const char *queryString,
> +                             ProcessUtilityContext context,
> +                             ParamListInfo params,
> +                             DestReceiver *dest,
> +                             char *completionTag)
> +{
> +    AuditEventStackItem *stackItem = NULL;
> +    int64 stackId = 0;
> +
> +    /*
> +     * 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:

create extension pg_audit;
SET pg_audit.log = 'role';
SET pg_audit.log_catalog = OFF;
SET pg_audit.log_level = 'warning';
SET pg_audit.log_parameter = on;
SET pg_audit.log_relation = on;
SET pg_audit.role = auditor;
SET pg_audit.log_statement_once = ON;
create table t ();
create role alice;
grant select on t to alice;
revoke select on t from alice;
\z t
create schema foo grant select on public.t to alice;
\z t

I'm wary of the ease of forgetting to run CREATE EXTENSION.  One gets much
auditing from GUCs alone; for example, we audit "CREATE TABLE t ()" with or
without the extension, but only with the extension do we audit the inner
CREATE TABLE of "CREATE SCHEMA s CREATE TABLE t ()".  A user that creates a
database without creating the extension might look at the audit messages and
mistakenly think the database is all set.

> +    /* Return objects affected by the (non drop) DDL statement */
> +    query = "SELECT UPPER(object_type), object_identity\n"
> +            "  FROM pg_event_trigger_ddl_commands()";

This SPI query neglects to schema-qualify its function calls.

> +    DefineCustomStringVariable(
> +        "pg_audit.log",
> +
> +        "Specifies which classes of statements will be logged by session audit "
> +        "logging. Multiple classes can be provided using a comma-separated "
> +        "list and classes can be subtracted by prefacing the class with a "
> +        "- sign.",
> +
> +        NULL,

The short_desc is several lines long, while long_desc is NULL.

> --- /dev/null
> +++ b/contrib/pg_audit/sql/pg_audit.sql

I do applaud the breadth of test coverage.

> +-- Set pg_audit parameters for the current (super)user.
> +ALTER ROLE :current_user SET pg_audit.log = 'Role';
> +ALTER ROLE :current_user SET pg_audit.log_level = 'notice';

Do not ALTER the initial login role in a regression test.  In the installcheck
case, the role belongs to the test operator; it is not ours to modify.

> +CREATE USER user1;
> +ALTER ROLE user1 SET pg_audit.log = 'ddl, ROLE';
> +ALTER ROLE user1 SET pg_audit.log_level = 'notice';
> +
> +--
> +-- Create, select, drop (select will not be audited)
> +\connect - user1

Adding new CREATE USER statements, as opposed to CREATE ROLE, to regression
tests is almost always wrong.  During "make check" on Windows, such users
cannot connect.  "REGRESS_OPTS = --create-role=user1" fixes that problem but
does not help installcheck to pass under MD5 authentication (which it did as
of commit c82725e).  Skip past these challenges by using SET statements
instead of ALTER ROLE + \connect.

> +    <para>
> +      Settings can be specified globally (in
> +      <filename>postgresql.conf</filename> or using
> +      <literal>ALTER SYSTEM ... SET</>), at the database level (using
> +      <literal>ALTER DATABASE ... SET</literal>), or at the role level (using
> +      <literal>ALTER ROLE ... SET</literal>).  Note that settings are not
> +      inherited through normal role inheritance and <literal>SET ROLE</> will
> +      not alter a user's <literal>pg_audit</> settings.  This is a limitation
> +      of the roles system and not inherent to <literal>pg_audit</>.
> +    </para>

This paragraph applies equally to every SUSET GUC in PostgreSQL.

> +    <para>
> +      The <literal>pg_audit</> extension must be loaded in
> +      <xref linkend="guc-shared-preload-libraries">.  Otherwise, an error
> +      will be raised at load time and no audit logging will occur.
> +    </para>

The test suite is a counterexample to that.

> +    <para>
> +      Object audit logging logs statements that affect a particular relation.
> +      Only <literal>SELECT</>, <literal>INSERT</>, <literal>UPDATE</> and
> +      <literal>DELETE</> commands are supported.  <literal>TRUNCATE</> is not
> +      included because there is no specific privilege for it.
> +    </para>

PostgreSQL has a TRUNCATE privilege:

GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }   [, ...] | ALL [ PRIVILEGES ] }   ON {
[TABLE ] table_name [, ...]        | ALL TABLES IN SCHEMA schema_name [, ...] }   TO role_specification [, ...] [ WITH
GRANTOPTION ]
 

> +      <para>
> +        Object-level audit logging is implemented via the roles system.  The
> +        <xref linkend="guc-pgaudit-role"> setting defines the role that
> +        will be used for audit logging.  A relation (<literal>TABLE</>,
> +        <literal>VIEW</>, etc.) will be audit logged when the audit role has
> +        permissions for the command executed or inherits the permissions from
> +        another role.  This allows you to effectively have multiple audit roles
> +        even though there is a single master role in any context.
> +      </para>

This is clever, but it means that any user can add to the list of audited
objects by granting individual object permissions, or the role itself, to the
audit role.  Crowding the audit log is one thing, but object owners can also
use REVOKE to stop auditing an object.  Perhaps the answer is to put all
important objects under trusted owners, but that's a heavy restriction.  Do
other RDBMS auditing features behave this way?


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

nm

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Thu, May 14, 2015 at 01:38:49PM -0400, Tom Lane wrote:
> > * The comments in the code betray utter ignorance of how logging actually
> > works, in particular this:
> >
> >  * Administrators can choose which log level the audit log is to be logged
> >  * at.  The default level is LOG, which goes into the server log but does
> >  * not go to the client.  Set to NOTICE in the regression tests.
> >
> > All the user has to do is change client_min_messages and he'll see all the
> > reports, which means if you think that letting the user see the audit
> > reports is a security problem then you have a hole a mile wide.
>
> That indicates the patch's general readiness:

While I agree that the comment was poorly worded and agreed with Tom
that it should be changed, I do not agree that it's indicative of the
patch's readiness, nor do I feel it's an issue that clients can see the
auditing information, provided that it's clearly documented.

I'm planning to commit a number of documentation updates (along with
other updates) to pg_audit to address that.  Using ereport() for query
information isn't anything new- we do that in auto_explain, and we also
report the query in the CONTEXT lines of error messages.  We should (and
I plan to) document that in the appropriate places and note that there
are risks associated with it (eg: with security definer functions).

> > +        /* These are DDL, unless they are ROLE */
> > +        case LOGSTMT_DDL:
> > +            className = CLASS_DDL;
> > +            class = LOG_DDL;
> > +
> > +            /* Identify role statements */
> > +            switch (stackItem->auditEvent.commandTag)
> > +            {
> > +                /* We know these are all role statements */
> > +                case T_GrantStmt:
> > +                case T_GrantRoleStmt:
> > +                case T_CreateRoleStmt:
> > +                case T_DropRoleStmt:
> > +                case T_AlterRoleStmt:
> > +                case T_AlterRoleSetStmt:
> > +                    className = CLASS_ROLE;
> > +                    class = LOG_ROLE;
> > +                    break;
>
> Not T_AlterDefaultPrivilegesStmt?

Agreed, that would be more sensible under ROLE than DDL.

> > +static void
> > +pg_audit_ProcessUtility_hook(Node *parsetree,
> > +                             const char *queryString,
> > +                             ProcessUtilityContext context,
> > +                             ParamListInfo params,
> > +                             DestReceiver *dest,
> > +                             char *completionTag)
> > +{
> > +    AuditEventStackItem *stackItem = NULL;
> > +    int64 stackId = 0;
> > +
> > +    /*
> > +     * 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.

We put a great deal of thought into any and all places where we filter
data to do our best to prevent this from happening, taking steps beyond
what a simple module would do to capture information and make sure that
something is logged, even when we don't have all of the information
available.  That isn't to say there aren't bugs- certainly issues have
been found through the various reviews and comments provided by multiple
individuals now, and I don't pretend that there are no others, but I
don't believe that this module is particularly more bug-ridden than
other contrib modules or even parts of core.

> I'm wary of the ease of forgetting to run CREATE EXTENSION.  One gets much
> auditing from GUCs alone; for example, we audit "CREATE TABLE t ()" with or
> without the extension, but only with the extension do we audit the inner
> CREATE TABLE of "CREATE SCHEMA s CREATE TABLE t ()".  A user that creates a
> database without creating the extension might look at the audit messages and
> mistakenly think the database is all set.

I agree with this concern, but I don't believe there's a lot that we can
do about it except to document it.  I had asked, during the discussion
of deparse and related pieces, that it be possible for us to directly
call the necessary functions to get at the information we needed without
having to go through the event trigger system.  That didn't happen and I
accepted that we would need to use CREATE EXTENSION.  Users are
certainly able to monitor their databases or include the extension in
template1 to make sure it is pulled in for any new databases.

> > +    /* Return objects affected by the (non drop) DDL statement */
> > +    query = "SELECT UPPER(object_type), object_identity\n"
> > +            "  FROM pg_event_trigger_ddl_commands()";
>
> This SPI query neglects to schema-qualify its function calls.

Agreed, will fix.

> > +    DefineCustomStringVariable(
> > +        "pg_audit.log",
> > +
> > +        "Specifies which classes of statements will be logged by session audit "
> > +        "logging. Multiple classes can be provided using a comma-separated "
> > +        "list and classes can be subtracted by prefacing the class with a "
> > +        "- sign.",
> > +
> > +        NULL,
>
> The short_desc is several lines long, while long_desc is NULL.

Good point, will address.

> > --- /dev/null
> > +++ b/contrib/pg_audit/sql/pg_audit.sql
>
> I do applaud the breadth of test coverage.

Thanks!  We'll be adding more based on your feedback and that of Fujii
and others.

> > +-- Set pg_audit parameters for the current (super)user.
> > +ALTER ROLE :current_user SET pg_audit.log = 'Role';
> > +ALTER ROLE :current_user SET pg_audit.log_level = 'notice';
>
> Do not ALTER the initial login role in a regression test.  In the installcheck
> case, the role belongs to the test operator; it is not ours to modify.

Alright, we could address that, but I imagine the right answer is to
take the approach discussed below, instead of modifying the roles.

> > +CREATE USER user1;
> > +ALTER ROLE user1 SET pg_audit.log = 'ddl, ROLE';
> > +ALTER ROLE user1 SET pg_audit.log_level = 'notice';
> > +
> > +--
> > +-- Create, select, drop (select will not be audited)
> > +\connect - user1
>
> Adding new CREATE USER statements, as opposed to CREATE ROLE, to regression
> tests is almost always wrong.  During "make check" on Windows, such users
> cannot connect.  "REGRESS_OPTS = --create-role=user1" fixes that problem but
> does not help installcheck to pass under MD5 authentication (which it did as
> of commit c82725e).  Skip past these challenges by using SET statements
> instead of ALTER ROLE + \connect.

Unfortunately, it's not possible to avoid the CREATE USER statements, if
we wish to exercise the GUCs set via ALTER ROLE.  GUCs set that way are
only set on brand new connections as that specific role, and given that
these are all SUSET GUCs, non-superusers wouldn't be able to change
them.

That said, the GUC system isn't really what we're trying to test here,
and, I agree, we can simply SET/RESET from the initial superuser role
throughout the regression tests and use SET ROLE instead.  It's not
exactly the same as testing with a real user who has GUCs set on their
role, but it should work for the testing requirements of this module.

> > +    <para>
> > +      Settings can be specified globally (in
> > +      <filename>postgresql.conf</filename> or using
> > +      <literal>ALTER SYSTEM ... SET</>), at the database level (using
> > +      <literal>ALTER DATABASE ... SET</literal>), or at the role level (using
> > +      <literal>ALTER ROLE ... SET</literal>).  Note that settings are not
> > +      inherited through normal role inheritance and <literal>SET ROLE</> will
> > +      not alter a user's <literal>pg_audit</> settings.  This is a limitation
> > +      of the roles system and not inherent to <literal>pg_audit</>.
> > +    </para>
>
> This paragraph applies equally to every SUSET GUC in PostgreSQL.

Agreed, we should make sure this is adequately documented in the main
docs and reference that from here instead.

> > +    <para>
> > +      The <literal>pg_audit</> extension must be loaded in
> > +      <xref linkend="guc-shared-preload-libraries">.  Otherwise, an error
> > +      will be raised at load time and no audit logging will occur.
> > +    </para>
>
> The test suite is a counterexample to that.

This was a relatively late change to appease the buildfarm.  It was not,
previously, the case, so the docs do need updating here.

> > +    <para>
> > +      Object audit logging logs statements that affect a particular relation.
> > +      Only <literal>SELECT</>, <literal>INSERT</>, <literal>UPDATE</> and
> > +      <literal>DELETE</> commands are supported.  <literal>TRUNCATE</> is not
> > +      included because there is no specific privilege for it.
> > +    </para>
>
> PostgreSQL has a TRUNCATE privilege:
>
> GRANT { { SELECT | INSERT | UPDATE | DELETE | TRUNCATE | REFERENCES | TRIGGER }
>     [, ...] | ALL [ PRIVILEGES ] }
>     ON { [ TABLE ] table_name [, ...]
>          | ALL TABLES IN SCHEMA schema_name [, ...] }
>     TO role_specification [, ...] [ WITH GRANT OPTION ]

Excellent point, we should be able to fix that.  As I expect many of
us remember, it wasn't always the case.

> > +      <para>
> > +        Object-level audit logging is implemented via the roles system.  The
> > +        <xref linkend="guc-pgaudit-role"> setting defines the role that
> > +        will be used for audit logging.  A relation (<literal>TABLE</>,
> > +        <literal>VIEW</>, etc.) will be audit logged when the audit role has
> > +        permissions for the command executed or inherits the permissions from
> > +        another role.  This allows you to effectively have multiple audit roles
> > +        even though there is a single master role in any context.
> > +      </para>
>
> This is clever, but it means that any user can add to the list of audited
> objects by granting individual object permissions, or the role itself, to the
> audit role.  Crowding the audit log is one thing, but object owners can also
> use REVOKE to stop auditing an object.  Perhaps the answer is to put all
> important objects under trusted owners, but that's a heavy restriction.  Do
> other RDBMS auditing features behave this way?

Indeed, that's correct.  It is certainly a limitation and one which I
hope to remove eventually, but until then, we need to document it.  I
don't believe it's a reason to not have this capability.  Object owners
can also DROP objects or columns, or add columns which may not be
included.  There has been very little discussion about segregating
permissions (or auditing) from object ownership, but we will hopefully,
eventually, get there.  However, even with such a heavy restriction, I
have confidence that there will be significant users; certainly I have
worked in a number of environments where this would have been a very
welcome capability, even with its limitations.  Please understand that
these limitations were understood at the time of the design of the
feature, they are not surprises, nor is there a misunderstanding
regarding them.

Further, I would be mildly surprised if our first pass at supporting
auditing in-core doesn't have that same limitation of being subject to
the object owner.  It's a fall-back we've used for a long time and
changing that will be quite difficult.

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

I certainly understand the concern about this code potentially creating
more security vulnerabilities than other areas, but I don't agree that
we should avoid including the capability due to that fear.  The prior
discussion about incremental development was about adding the necessary
pieces to core to eventually provide better auditing than what this
provides; this module is very clearly intended to be a complete feature,
with limitations which users need to be aware of prior to deploying it.

I have a reasonably high degree of confidence that users who are
interested in this capability will properly research the limitations and
be able to decide for themselves if they are able to deploy it in their
environments.

The feature which is "auditing", as a whole, needs a great deal more
than even simply removing the limitations associated with the design of
this module.  This was never intended to be the one true auditing
solution, nor was it ever portrayed as such, but rather it's a good
start with specific limitations that users need to be aware of.  This is
a step along the route to proper in-core auditing support, much the
same as the various changes which have been made for logical replication
and BDR, and as dblink was a step on the road towards FDWs.  Just as we
did not have the ultimate solution land in core in one release cycle
for any of those, I don't expect this initial capability to be without
limitations.

I do not take this position lightly and it has been top-of-mind for me
over these past days.  I've further discussed it with community members,
users of PG (those who are not our clients, who I've discussed it
extensively with already), and read some of the discussion about how
this capability may be used in non-auditing situations.  Perhaps it
should have been given a different name, one which would not come across
as trying to address such a large, difficult, and contentious problem,
but that is a two-edged sword, as users who this module would work for
may not realize the capability that this module provides in that case.

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.
Thanks!
    Stephen

On Tue, May 26, 2015 at 8:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
> 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.

Like Noah, I'm unhappy that this patch went in.  I stated a few days
before it was committed that I felt the code quality and documentation
were well below the level we normally expect.  Your response was to do
a little more work on the patch and then commit it without so much as
reposting it.  Clearly, we have completely different ideas about what
constitutes an appropriate response to the sort of comments I sent.

A near-record number of committers have objected to this patch in all
kinds of different ways.  There have been concerns about process,
concerns about code quality, and concerns about whether this is really
something that we want.  The normal way that works is that you either
(a) revise the patch or (b) try to convince the person raising the
concern that it's OK.  The concern is addressed when the person who
raised it says it is, and not just because you thought about it and
decided it was OK.  This can be taken too far in the other direction,
but we're not close to that in this instance.

I am particularly troubled by the fact that what has happened with
this patch is very much like what happened with row-level security: a
patch that clearly wasn't finished and clearly had not had adequate
review got abruptly committed - by you - without any consensus so to
do.  It seems likely to me that by now row-level security has had
enough review that it is solid enough to be in the tree - in
particular, I am encouraged by Dean Rasheed's work to improve the
patch.  However, it is absolutely not the community process for that
stuff to happen after the code is already in the tree.  It is the
community process for that stuff to happen before the code is in the
tree.

It will be impossible for our community to continue delivering quality
software if you persist in taking this approach.  Either the other
committers will have to spend an inordinate effort fixing (or
convincing you to fix) the stuff you break - instead of working on our
own projects - and other people's patches - or we will have to ignore
your commits, and the things that are broken in those commits will
become bugs in our releases.  Either way, the community loses.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

Thank you for your response.

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, May 26, 2015 at 8:10 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > 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.
>
> Like Noah, I'm unhappy that this patch went in.  I stated a few days
> before it was committed that I felt the code quality and documentation
> were well below the level we normally expect.  Your response was to do
> a little more work on the patch and then commit it without so much as
> reposting it.  Clearly, we have completely different ideas about what
> constitutes an appropriate response to the sort of comments I sent.

Just to be clear, I understand you're referring to this email:

CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4dUg@mail.gmail.com

David and I both spent a great deal of time reviewing that email and
working to address your comments.  I had David do as much of it as I
felt that I could, as it is a learning experience for him as he works to
become another dedicated PostgreSQL developer for the community.  He
then passed it on to me, and I took it further and he reviewed the
results of my changes.

To clarify, that email noted style issues (which I worked to fix,
though I should have run pgindent, of course..  the discussed
'make indent' option would help a great deal with that), concerns about
the design limitations (server crashes, using the permission system),
a similar comment regarding where log messages go to the comment in the
code which has had much discussion due to it, and issues with the
documentation and comments.  I did spend a good bit of time working to
improve the comments, but did not focus on the documentation as that
could be done post-feature freeze.

Your comment at the bottom of that email and the subsequent lack of any
other feedback from you or others regarding concern when I voiced my
intention to work on pg_audit for 9.5 in response to Tom's summary
thread lead me to believe that a full review by a qualified committer,
with appropriate improvement to the comments and code, would satisfy the
process requirements to move forward.  There was no hidden agenda here
or an attempt to hold off until the last minute, nor was it any secret
that I was working on it.  There was another, much larger, feature added
to core, rather than simply a contrib module, which I have concerns may
have issues too, but I don't believe that there was any problem with the
process there either.  The concerns were raised by one committer, a
review and quite large rework was done by another, in conjunction with
the author, and then the patch was committed.

I agree that the documentation needs improvement.  I don't agree with
your assessment that the code quality is well below the level we
normally expect, as committed.  That's clearly a difficult thing to
judge, hence my compromise proposal to ensure that it either has a full
review or gets reverted and not included in a released version.

> A near-record number of committers have objected to this patch in all
> kinds of different ways.  There have been concerns about process,
> concerns about code quality, and concerns about whether this is really
> something that we want.  The normal way that works is that you either
> (a) revise the patch or (b) try to convince the person raising the
> concern that it's OK.  The concern is addressed when the person who
> raised it says it is, and not just because you thought about it and
> decided it was OK.  This can be taken too far in the other direction,
> but we're not close to that in this instance.

I worked to address all of those concerns, including those raised by the
individuals you noted in our private discussion, all of whom have been
silent on this since before it went in.  There was clear agreement by
a number of individuals on the need for the broader capability, but I
don't believe that's going to spring forth and land in the tree in one
fell swoop.  This is a good step in that direction.  The patch was
revised numerous times and I did work to convince the individuals
raising concerns that it was OK.  I don't believe there is any doubt
about either of those, but I would be happy to go through the archives
and point out why I feel that my statements are accurate.

> I am particularly troubled by the fact that what has happened with
> this patch is very much like what happened with row-level security: a
> patch that clearly wasn't finished and clearly had not had adequate
> review got abruptly committed - by you - without any consensus so to
> do.  It seems likely to me that by now row-level security has had
> enough review that it is solid enough to be in the tree - in
> particular, I am encouraged by Dean Rasheed's work to improve the
> patch.  However, it is absolutely not the community process for that
> stuff to happen after the code is already in the tree.  It is the
> community process for that stuff to happen before the code is in the
> tree.

I agreed with you regarding row level security and we had a great
discussion about how it's a large feature which needed more review than
most and, further, that it's important us to all realize that we are
custodians of the whole tree and the patches which we commit in
particular and that being a committer is about being trusted to maintain
and address bugs and issues which arise from what we have committed,
across the years.  I have been in this community for a long time, nearly
all of that time in a purely volunteer fashion, working as best I could
in my spare time.  Should things change for me in the future, though I
truely hope they don't, I will continue to fulfill my obligations to the
community in my role as a committer to the best of my abilities.

> It will be impossible for our community to continue delivering quality
> software if you persist in taking this approach.  Either the other
> committers will have to spend an inordinate effort fixing (or
> convincing you to fix) the stuff you break - instead of working on our
> own projects - and other people's patches - or we will have to ignore
> your commits, and the things that are broken in those commits will
> become bugs in our releases.  Either way, the community loses.

I find it confusing that there is no appreciation here for the level of
interest and excitment which has happened around these features, or how
having these capabilities will grow our community, or how working with
David and others to develop new PostgreSQL developers who may some day
become committers and continue to carry PostgreSQL forward long past
when we are gone is a worthwhile goal.

I do believe that this is a quality piece of software and that it will
be a benefit to the community, or I wouldn't have committed it in the
first place.  There are certainly other patches which I've bounced back
while doing reviews, others that I've rewritten nearly from scratch, or
reviewed, tweaked, and committed.  I've also reverted my share of
patches when issues have been pointed out with them, and held off on
moving forward when there has been clear opposition to the patch being
committed at the time.  Further, as I said above, I will continue to
maintain these features and any others which I commit (which I truely
hope to be numerous) over the years to come.  I hope that's clear from
the history which I have with the community.  I'm sure bugs will be
found in our releases in many areas, but I do not believe that this
feature will have substantially more than others, even so, I'm willing
to set that aside and work with another committer to review the feature,
to make sure that doesn't happen with this patch, and have agreed to
revert it if I'm unable to do so because there is no one willing to.  I
am not asking anyone to do this against their will or to take time away
from their own projects, nor do I wish for the community to lose, as
that would mean I'd be losing too.
Thanks!
    Stephen

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

* Noah Misch (noah@leadboat.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

On Wed, May 27, 2015 at 9:24 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I agree that the documentation needs improvement.  I don't agree with
> your assessment that the code quality is well below the level we
> normally expect, as committed.  That's clearly a difficult thing to
> judge, hence my compromise proposal to ensure that it either has a full
> review or gets reverted and not included in a released version.

That's not really acceptable in my view.  I looked at it shortly
before it was committed and said that it did not appear to me to be
close to being ready.  Noah took a look at it shortly after it was
committed and said that it did not appear to him to be close to being
ready.  Apparently, that's not good enough for you.  Your "compromise
proposal" is that a third committer other than yourself should go look
at it.

But that's not the way our community works.  It's hard to get one
extra committer to look at most patches, let alone three.  Committer
bandwidth is too precious for that.  Parallel sequential scan would be
in this release but for the fact that Andres wasn't confident in
certain portions of it, and I respected that by not committing even
though not a single other person backed up his concerns.

Basically, you're attempting to place the onus on other committers to
find the bugs in your patch.  If a third committer DOES come along and
review it, you'll fix whatever they find and say "well, now it doesn't
need to be reverted".  That's just not right.  As a committer, you are
responsible for getting it right the first time, not committing stuff
that is seriously broken and then cleaning it up as the issues are
reported, or when you have time.  If everyone adopted the approach
you're taking here, we'd have an order of magnitude more serious bugs
in a typical major release (and I would quit the project).

I note that there are already 11 followup commits fixing bugs in
pg_audit, including 7 in the first 24 hours.  It's usually not a good
thing when you can get the regression tests for a piece of
platform-independent code to pass in less than 8 tries.  I suspect,
but do not know for certain, that this is just the tip of the iceberg.

> I find it confusing that there is no appreciation here for the level of
> interest and excitment which has happened around these features, or how
> having these capabilities will grow our community, or how working with
> David and others to develop new PostgreSQL developers who may some day
> become committers and continue to carry PostgreSQL forward long past
> when we are gone is a worthwhile goal.

Those things are true of many patches.  They don't excuse committing
poor code or stream-rolling community process.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> I note that there are already 11 followup commits fixing bugs in
> pg_audit, including 7 in the first 24 hours.  It's usually not a good
> thing when you can get the regression tests for a piece of
> platform-independent code to pass in less than 8 tries.  I suspect,
> but do not know for certain, that this is just the tip of the iceberg.

Having many commits immediately following a patch going in is certainly
a reasonable reason to be concerned.  What may not have been apparent is
that all but one of those were specifically due to issues with how the
regression tests were being run rather than any issues in the code.

In an attempt to provide clarity and bring an objective view to the
issues which have been identified in the pg_audit code since it was
committed, I've developed the following list, which I kindly ask you
and Noah to review objectivly.  This list includes all issues that I've
identified from the commits done to master, comments made by the
numerous individuals who have taken time to look at the patch and
comment, and those found by the ongoing work of David and I (which has
continued after the commit).  If you both still feel that these are
indicative of being just 'the tip of the iceberg', then I'll revert it.
I do not ask that you provide any further explanation as it's ultimately
a judgement call.

Note that I intentionally filtered out documentation and comment typo
changes (either committed or planned), and the issues which were caused
by how the regression tests were being run, as those are not germane to
the code quality concerns.

I would ask others to comment, but I can, understandably, appreciate
that they would prefer to stay out of a conversion with such poor tone.

I've attempted to order the list by severity, though that's clearly up
for some debate, so take it for what you feel it is worth.

SPI query qualify - Noah
 This is absolutely an issue, but we are kidding ourselves if we believe that this will never happen again.  A holistic
solutionto this issue is needed.
 

CREATE SCHEMA .. GRANT - Noah
 This is definitely an issue which needs to be addressed, but I don't believe this is an issue with the approach used
(trustingevent triggers to cover everything under CREATE SCHEMA).  The event trigger is correctly collecting the GRANT
commandand pg_audit is outputting a record based on that (when DDL is enabled), but it's not detecting that the
subcommandis a GRANT.  That's certainly not great, but it's not an insurmountable problem either- correct the early
exitlogic, pull the command tag from pg_event_trigger_ddl_commands and use it.
 

Portability issue wrt %ld - Tom
 Clearly an issue, but one which the buildfarm is specifically intended to catch and address.  Note that this is the
onlycode-level issue found by the buildfarm- all of the other commits associated with the buildfarm were due to how the
regressiontests were being run.
 

T_AlterDefaultPrivilegesStmt classification fix - Noah
 This should clearly be ROLE instead of DDL.

Use ereport() instead of elog() - Tom
 Clearly better, but I don't see how this could be exploited or necessairly rises to the level of 'bug', but I included
ithere for completeness.
 

Reclassify REINDEX - Fujii
 This was done to match what is done in core regarding REINDEX.  Note that it was intentionally set to DDL, and
documentedaccordingly, as we felt the comment in the core code was correct, but, even so, we agreed with Fujii that we
shouldprobably match what core actually does here regardless.
 

Suppress STATEMENT output - Fujii
 Makes sense but is just about reducing the amount of log traffic.

Suppress CONTEXT output - David
 Similar to the STATEMENT case above, creates unnecessary noise in the logs.

Further classification questions - Fujii
 These questions will undoubtably come up and there isn't one answer here but rather a case where we simply need to
decideon something and document it accordingly.
 

Remove ERROR, PANIC, and FATAL from log_level options - David
 This is a SUSET variable, so there isn't a particular security risk here, but it doesn't make sense to include these
options.

I'm hopeful, as I'm sure you understand, that the number of issues being
reported is due principally to the amount of review and testing which
has happened on this module since it went in and not because the quality
of the code is particularly poor.  I feel it's far more than happens for
many commits, even ones which are backpatched, and I'm certainly hopeful
that it makes for a better end result.  I certainly do not intend to ask
the community to accept a patch which is bugridden or full of holes, nor
will I stand in the way of the community requesting that a feature be
reverted.
Thanks!
    Stephen

On Wed, May 27, 2015 at 04:36:53PM -0400, Stephen Frost wrote:
> This list includes all issues that I've
> identified from the commits done to master, comments made by the
> numerous individuals who have taken time to look at the patch and
> comment, and those found by the ongoing work of David and I (which has
> continued after the commit).  If you both still feel that these are
> indicative of being just 'the tip of the iceberg', then I'll revert it.

This list is just the tip of the iceberg.

> I do not ask that you provide any further explanation as it's ultimately
> a judgement call.

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Wed, May 27, 2015 at 04:36:53PM -0400, Stephen Frost wrote:
> > This list includes all issues that I've
> > identified from the commits done to master, comments made by the
> > numerous individuals who have taken time to look at the patch and
> > comment, and those found by the ongoing work of David and I (which has
> > continued after the commit).  If you both still feel that these are
> > indicative of being just 'the tip of the iceberg', then I'll revert it.
>
> This list is just the tip of the iceberg.

Thank you for your honest comments and your concern.

I sincerely hope you're able to be involved in developing auditing for
PostgreSQL in the future, as it is a key requirement in any secure
environment.
Thanks again!
    Stephen

On 05/27/2015 06:11 PM, Stephen Frost wrote:

> Thank you for your honest comments and your concern.
>
> I sincerely hope you're able to be involved in developing auditing for
> PostgreSQL in the future, as it is a key requirement in any secure
> environment.


Guys,

I think we are overlooking the rather obvious elephant in the room. This 
is an extension. There is no reason for it to be in core. Revert the 
patch, gain independence, the ability to innovate mid-cycle and move on 
to bigger fish.

Sincerely,

JD


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.

JD,

* Joshua D. Drake (jd@commandprompt.com) wrote:
> On 05/27/2015 06:11 PM, Stephen Frost wrote:
> >Thank you for your honest comments and your concern.
> >
> >I sincerely hope you're able to be involved in developing auditing for
> >PostgreSQL in the future, as it is a key requirement in any secure
> >environment.
>
> I think we are overlooking the rather obvious elephant in the room.
> This is an extension. There is no reason for it to be in core.
> Revert the patch, gain independence, the ability to innovate
> mid-cycle and move on to bigger fish.

While I certainly appreciate the support, I don't believe auditing will
be able to work as an extension over the long term and if the community
is unwilling or unable to accept steps in that direction through contrib
modules or even changes to core to improve what we are able to provide
in this area, I have very serious doubts about the willingness of
organizations (particularly those in the financial and government space)
to continue to seek out and support PostgreSQL as a viable open source
alternative to the commerical RDBMS's which have had these capabilities
for years.

I'm, again, not suggesting that a contrib module is going to be a
workable long-term solution for all use-cases, but it would solve quite
a few and would be known to be supported, and to have the support of the
community, if released as part of PostgreSQL.  These are extremely
serious organizations who care about the reputation of PostgreSQL and
the community for delivering quality software.  I certainly have no
intention to tarnish that in any way as it would be quite detrimental to
myself and the community.  If that means reverting a patch of my own, or
one which I have supported, so be it.
Thanks!
    Stephen

On 05/27/2015 06:38 PM, Stephen Frost wrote:

> While I certainly appreciate the support, I don't believe auditing will
> be able to work as an extension over the long term and if the community
> is unwilling or unable to accept steps in that direction through contrib
> modules or even changes to core to improve what we are able to provide
> in this area,

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 have very serious doubts about the willingness of
> organizations (particularly those in the financial and government space)
> to continue to seek out and support PostgreSQL as a viable open source
> alternative to the commerical RDBMS's which have had these capabilities
> for years.

This may or may not be true considering and I am not sure it really 
matters in the context of this argument.

>
> I'm, again, not suggesting that a contrib module is going to be a
> workable long-term solution for all use-cases, but it would solve quite
> a few and would be known to be supported, and to have the support of the
> community, if released as part of PostgreSQL.

If the demand for this module is there, it will receive the support it 
needs regardless if it is in core.

> These are extremely
> serious organizations who care about the reputation of PostgreSQL and
> the community for delivering quality software.  I certainly have no
> intention to tarnish that in any way as it would be quite detrimental to
> myself and the community.  If that means reverting a patch of my own, or
> one which I have supported, so be it.

I can not speak to the quality of the patch. I can speak to what others, 
people of repute in this community speak of this patch. The leaning 
tower seems as if it is clearly in the, "we might want to think about 
reverting this".

As it is currently an extension, it does not need to be in core. If this 
extension reaches a point where it needs to be in core to achieve some 
level of integration not currently provided then we can evaluate that 
problem. Currently, that is not the case.

Sincerely,

Joshua D. Drake


>
>     Thanks!
>
>         Stephen
>


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.

JD,

* Joshua D. Drake (jd@commandprompt.com) wrote:
> On 05/27/2015 06:38 PM, Stephen Frost wrote:
> >While I certainly appreciate the support, I don't believe auditing will
> >be able to work as an extension over the long term and if the community
> >is unwilling or unable to accept steps in that direction through contrib
> >modules or even changes to core to improve what we are able to provide
> >in this area,
>
> 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 have very serious doubts about the willingness of
> >organizations (particularly those in the financial and government space)
> >to continue to seek out and support PostgreSQL as a viable open source
> >alternative to the commerical RDBMS's which have had these capabilities
> >for years.
>
> This may or may not be true considering and I am not sure it really
> matters in the context of this argument.

I'm confused by this comment.  I've had discussions with both financial
and government organizations who are interested in this capability and
have been assured that they are interested in the PostgreSQL community
building and supporting this, and are conversely utterly disinterested
in either commerical or open-source products put out by individual
organizations which can come and go.  In my mind, this simply isn't a
question.  I wish that everyone could have the same experiences that I
have had and to have been in the discussions that I've been in, but
that's simply impractical and I would hope that my years in this
community would count in my favor when I make these statements.

> >I'm, again, not suggesting that a contrib module is going to be a
> >workable long-term solution for all use-cases, but it would solve quite
> >a few and would be known to be supported, and to have the support of the
> >community, if released as part of PostgreSQL.
>
> If the demand for this module is there, it will receive the support
> it needs regardless if it is in core.

Please see above as I believe it addresses this point.

> >These are extremely
> >serious organizations who care about the reputation of PostgreSQL and
> >the community for delivering quality software.  I certainly have no
> >intention to tarnish that in any way as it would be quite detrimental to
> >myself and the community.  If that means reverting a patch of my own, or
> >one which I have supported, so be it.
>
> I can not speak to the quality of the patch. I can speak to what
> others, people of repute in this community speak of this patch. The
> leaning tower seems as if it is clearly in the, "we might want to
> think about reverting this".

I have been doing my best to follow all of the comments and concerns
raised regarding this patch.  I'm not sure that I share the optimism
that you express in the quote above, but if two other committers feel
that the feature needs to be reverted, then I will revert it.  That is
not a documented and formal policy in the community, as far as I'm
aware, but it strikes me as reasonable, in the absence of any obvious
collusion or hidden agendas.  Should another committer speak up
regarding the patch, perhaps things would change, but I have no
disillusionment that such will happen in this case and have accepted it.

> As it is currently an extension, it does not need to be in core. If
> this extension reaches a point where it needs to be in core to
> achieve some level of integration not currently provided then we can
> evaluate that problem. Currently, that is not the case.

This simply doesn't make sense- all extensions can hook into the backend
in the same way, 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.
Thanks!
    Stephen

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.

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.

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.

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

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

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

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.
        regards, tom lane

On 05/27/2015 07:02 PM, Stephen Frost wrote:
> JD,
>
>> As it is currently an extension, it does not need to be in core. If
>> this extension reaches a point where it needs to be in core to
>> achieve some level of integration not currently provided then we can
>> evaluate that problem. Currently, that is not the case.
>
> This simply doesn't make sense- all extensions can hook into the backend
> in the same way,

That is exactly my point.


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

Just look at your own experience with pgBackrest. I am aware that 
pgBackrest is not an extension but it is a relatively new open source 
project that is gaining community based on its quality and contribution. 
It is arguably the best backup software available for Pg. It is not in 
core, it is not shipped with core.

Your extension could be the best audit software available for Pg. It is 
not in core, because we don't need it to be. (slogan)

JD

>
>     Thanks!
>
>         Stephen
>


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Announcing "I'm offended" is basically telling the world you can't
control your own emotions, so everyone else should do it for you.

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

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.

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.

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:

* 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 don't understand how the pg_audit.role setting and the audit role 
system works.

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

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?

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.

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)


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.

- Heikki

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

All,

Replying to Heikki's email as it's quite late here and I want to
respond.  Barring any further commentary, I'm planning to pull pg_audit
out tomorrow (it wouldn't be intelligent for me to attempt to do so now).
I really do appreciate all of the excellent feedback and comments, the
excellent discussion, and the honest concerns raised about it.  I truely
love being a member of this community and to be among such highly
respected and intelligent individuals, even if, on occation, I may be a
bit brash.

Thanks again, and I look forward to many interesting and positive
discussions in Ottawa.
Stephen

On 05/28/2015 11:14 AM, Stephen Frost wrote:
> * Heikki Linnakangas (hlinnaka@iki.fi) wrote:
>> 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.

Ah, I see. Wow, that's really Rube Goldbergian.

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

Sure, I didn't mean we should require a restart. Requiring SIGHUP seems 
reasonable though.

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

True. I wouldn't be too worried about that though. If you rename a 
table, that hopefully gets flagged in the audit log and someone will ask 
"why did you rename that table?". If you're paranoid enough, you could 
have a catch-all rule that audits everything that's not explicitly 
listed, so a renamed table always gets audited.

Of course, you could still support a labeling system, similar to the 
pg_audit.role setting and GRANTs. For example, you could tag tables with 
a label like "reads_need_auditing", and then in the config file, you 
could specify that all READs on tables with that label need to be 
audited. I think security labels would be a better system to abuse for 
that than GRANT. You'd want to just label the objects, and specify the 
READ/WRITE etc. attributes in the config file.

Who do you assume you can trust? Is it OK if the table owner can 
disable/enable auditing for that table?

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

Sure, adding features like sending logs to different places in core is 
totally reasonable, independently of pg_audit. (Or not, but in any case, 
it's orthogonal :-) )

- Heikki

Heikki,

* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> On 05/28/2015 11:14 AM, Stephen Frost wrote:
> >* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
> >>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.
>
> Ah, I see. Wow, that's really Rube Goldbergian.

It's certainly not ideal.  It was discussed back in January, iirc.

> >>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.
>
> Sure, I didn't mean we should require a restart. Requiring SIGHUP
> seems reasonable though.

I wouldn't have any issue with that.

> >>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.
>
> True. I wouldn't be too worried about that though. If you rename a
> table, that hopefully gets flagged in the audit log and someone will
> ask "why did you rename that table?". If you're paranoid enough, you
> could have a catch-all rule that audits everything that's not
> explicitly listed, so a renamed table always gets audited.

The general 'catch-all' approach was how we approached pg_audit in
general, so, yes, that's the kind of auditing we expect people to want
(or, at least, we would need to support it).  You're right, in some
environments that may be workable, but then it also has to be entirely
managed outside of the database, which would make it extremely difficult
to use in many environments, if not impossible..

> Of course, you could still support a labeling system, similar to the
> pg_audit.role setting and GRANTs. For example, you could tag tables
> with a label like "reads_need_auditing", and then in the config
> file, you could specify that all READs on tables with that label
> need to be audited. I think security labels would be a better system
> to abuse for that than GRANT. You'd want to just label the objects,
> and specify the READ/WRITE etc. attributes in the config file.

Using SECURITY LABELs is certainly an interesting idea.  GRANT was
chosen because, with it, we also get information regarding the action
that the user wants to audit (select/insert/update/delete, etc), without
having to build all of that independently with some custom structure in
the SECURITY LABEL system.

> Who do you assume you can trust? Is it OK if the table owner can
> disable/enable auditing for that table?

In an ideal world, you would segregate the rights which the table owner
has from both the permission system and the audit system.  This has come
up a number of times already and is, really, an independent piece of
work.  Having the permissions and auditing controlled by the same group
is better than having all three pieces controlled by the ownership role,
but having three distinct groups would be preferred.
Thanks!
    Stephen

Ian, Abhijit, and David,

My condemnation of the pg_audit commits probably hurt you as the feature's
authors.  I am sorry for that.  Your code was better than most "Ready for
Committer" code, and I hope you submit more patches in the future.

Regards,
nm

Hi Noah,

On 6/8/15 10:13 AM, Noah Misch wrote:
> My condemnation of the pg_audit commits probably hurt you as the feature's
> authors.  I am sorry for that.  Your code was better than most "Ready for
> Committer" code, and I hope you submit more patches in the future.

I appreciate you saying this and especially for saying it publicly.

I've certainly had quite the experience as a first-time contributor
working on this patch.  Perhaps I bit off more than I should have and I
definitely managed to ruffle a few feathers along the way.  I learned a
lot about how the community works, both the good and the bad.  Fear not,
though, I'm not so easily discouraged and you'll definitely be hearing
more from me.

My honest, albeit novice, opinion is that it was a mistake to pull
pg_audit from contrib.  I know more than anyone that it had flaws,
mostly owing to its implementation as an extension, but it also provided
capability that simply does not exist right now.  Recent conversations
about PGXN demonstrate why that is not (currently) a good alternative
for distributing extensions.  That means pg_audit will have a more
limited audience than it could have had.  That's a shame, because people
are interested in pg_audit, warts and all.

The stated purpose of contrib is: "include porting tools, analysis
utilities, and plug-in features that are not part of the core PostgreSQL
system, mainly because they address a limited audience or are too
experimental to be part of the main source tree. This does not preclude
their usefulness."

Perhaps we should consider modifying that language, because from my
perspective pg_audit fit the description perfectly.

Of course, I understand this is a community effort and I don't expect
every contribution to be accepted and committed.  Consider me
disappointed yet determined.

--
- David Steele
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
> My honest, albeit novice, opinion is that it was a mistake to pull
> pg_audit from contrib.  I know more than anyone that it had flaws,
> mostly owing to its implementation as an extension, but it also provided
> capability that simply does not exist right now.  Recent conversations
> about PGXN demonstrate why that is not (currently) a good alternative
> for distributing extensions.  That means pg_audit will have a more
> limited audience than it could have had.  That's a shame, because people
> are interested in pg_audit, warts and all.

FWIW, I did not think that the consensus was that pg_audit could never
be in contrib.  As I understood it, people felt that (1) the API might
not be sufficiently stable yet, and (2) there might be parts that should
be in core eventually.  (2) is problematic only because we do not have
very good tools for migrating things from contrib to core without creating
user-visible compatibility issues; which is not pg_audit's fault but it's
still a constraint we have to keep in mind.  So I'd encourage you to keep
working on it and trying to address the issues that were brought up.
        regards, tom lane

On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote:
> I've certainly had quite the experience as a first-time contributor
> working on this patch.  Perhaps I bit off more than I should have and I
> definitely managed to ruffle a few feathers along the way.  I learned a
> lot about how the community works, both the good and the bad.  Fear not,
> though, I'm not so easily discouraged and you'll definitely be hearing
> more from me.

Glad to hear it.

> The stated purpose of contrib is: "include porting tools, analysis
> utilities, and plug-in features that are not part of the core PostgreSQL
> system, mainly because they address a limited audience or are too
> experimental to be part of the main source tree. This does not preclude
> their usefulness."
> 
> Perhaps we should consider modifying that language, because from my
> perspective pg_audit fit the description perfectly.

"What is contrib?" attracts enduring controversy; see recent thread "RFC:
Remove contrib entirely" for the latest episode.  However that discussion
concludes, that documentation passage is not too helpful as a guide to
predicting contrib patch reception.  (Most recent contrib additions had an
obvious analogy to an existing module, sidestepping the question.)

On 10 June 2015 at 14:41, Noah Misch <noah@leadboat.com> wrote:
> On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote:
>> I've certainly had quite the experience as a first-time contributor
>> working on this patch.  Perhaps I bit off more than I should have and I
>> definitely managed to ruffle a few feathers along the way.  I learned a
>> lot about how the community works, both the good and the bad.  Fear not,
>> though, I'm not so easily discouraged and you'll definitely be hearing
>> more from me.
>
> Glad to hear it.
>
>> The stated purpose of contrib is: "include porting tools, analysis
>> utilities, and plug-in features that are not part of the core PostgreSQL
>> system, mainly because they address a limited audience or are too
>> experimental to be part of the main source tree. This does not preclude
>> their usefulness."
>>
>> Perhaps we should consider modifying that language, because from my
>> perspective pg_audit fit the description perfectly.
>
> "What is contrib?" attracts enduring controversy; see recent thread "RFC:
> Remove contrib entirely" for the latest episode.  However that discussion
> concludes, that documentation passage is not too helpful as a guide to
> predicting contrib patch reception.  (Most recent contrib additions had an
> obvious analogy to an existing module, sidestepping the question.)

Is pg_audit being resubmitted for 9.6 at all?

Thom

Hi Thom,

On 11/18/15 8:54 AM, Thom Brown wrote:
> On 10 June 2015 at 14:41, Noah Misch <noah@leadboat.com> wrote:
>> On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote:
>>> I've certainly had quite the experience as a first-time contributor
>>> working on this patch.  Perhaps I bit off more than I should have and I
>>> definitely managed to ruffle a few feathers along the way.  I learned a
>>> lot about how the community works, both the good and the bad.  Fear not,
>>> though, I'm not so easily discouraged and you'll definitely be hearing
>>> more from me.
>>
>> Glad to hear it.
>>
>>> The stated purpose of contrib is: "include porting tools, analysis
>>> utilities, and plug-in features that are not part of the core PostgreSQL
>>> system, mainly because they address a limited audience or are too
>>> experimental to be part of the main source tree. This does not preclude
>>> their usefulness."
>>>
>>> Perhaps we should consider modifying that language, because from my
>>> perspective pg_audit fit the description perfectly.
>>
>> "What is contrib?" attracts enduring controversy; see recent thread "RFC:
>> Remove contrib entirely" for the latest episode.  However that discussion
>> concludes, that documentation passage is not too helpful as a guide to
>> predicting contrib patch reception.  (Most recent contrib additions had an
>> obvious analogy to an existing module, sidestepping the question.)
>
> Is pg_audit being resubmitted for 9.6 at all?

A number of people have asked me about this over the last few months.  I 
would certainly be interested in resubmitting this for 9.6.

I fixed many of the issues that caused complaints at the end of the 9.5 
cycle, but there are still two remaining items I would want to address 
before resubmitting:

1) There was concern about audit messages being sent to the clients.
I'm looking at adding an option to the ereport macro to suppress sending
messages to the clients.  I'll submit that patch soon.

2) I would like make install-check to work with modules that require 
shared_preload_libraries.  My understanding is that Andrew may have 
already fixed this, but if not I'll look into it.

-- 
-David
david@pgmasters.net

On Fri, Nov 20, 2015 at 12:11:00PM -0500, David Steele wrote:
> I fixed many of the issues that caused complaints at the end of the 9.5
> cycle, but there are still two remaining items I would want to address
> before resubmitting:
> 
> 1) There was concern about audit messages being sent to the clients.
> I'm looking at adding an option to the ereport macro to suppress sending
> messages to the clients.  I'll submit that patch soon.

I don't object to audit messages being client-visible; did anyone feel that
was a blocker?  I did make a related point, namely, the documentation must not
assert that you can hide messages from clients if you in fact cannot.

> 2) I would like make install-check to work with modules that require
> shared_preload_libraries.  My understanding is that Andrew may have already
> fixed this, but if not I'll look into it.

It would be nice to standardize a pattern for that, agreed.

On 11/21/15 2:47 PM, Noah Misch wrote:
> On Fri, Nov 20, 2015 at 12:11:00PM -0500, David Steele wrote:
>> I fixed many of the issues that caused complaints at the end of the 9.5
>> cycle, but there are still two remaining items I would want to address
>> before resubmitting:
>>
>> 1) There was concern about audit messages being sent to the clients.
>> I'm looking at adding an option to the ereport macro to suppress sending
>> messages to the clients.  I'll submit that patch soon.
>
> I don't object to audit messages being client-visible; did anyone feel that
> was a blocker?  I did make a related point, namely, the documentation must not
> assert that you can hide messages from clients if you in fact cannot.

I don't think anyone felt it was a blocker but it seemed nobody thought
it was ideal, either.  I certainly don't.

The error in the documentation was unfortunate but that's what the
review process is all about.  I'm appreciative of everyone who had a
look at the patch.

--
-David
david@pgmasters.net