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

Noah,

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