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

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 GRANT OPTION ]

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