pgsql: Add pg_audit, an auditing extension

* Stephen Frost (sfrost@snowman.net) wrote:

Add pg_audit, an auditing extension

And... I busted the buildfarm. Will fix.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

And... I busted the buildfarm. Will fix.

 -- Load pg_audit module
  create extension pg_audit;
+ ERROR:  pg_audit must be loaded via shared_preload_libraries

This seems like a rather poorly thought-through error check.
It will break not only the buildfarm but any dump/restore scenario.
You really can't have extensions that refuse to let themselves
be created.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

And... I busted the buildfarm. Will fix.

-- Load pg_audit module
create extension pg_audit;
+ ERROR:  pg_audit must be loaded via shared_preload_libraries

This seems like a rather poorly thought-through error check.
It will break not only the buildfarm but any dump/restore scenario.
You really can't have extensions that refuse to let themselves
be created.

Yeah, the original idea behind it was to force the user to think about
if they really would want to load it later on down the line rather than
have it pre-loaded always.

I'll put something in the docs which recommends it and provides the
reasoning behind it.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

+ ERROR: pg_audit must be loaded via shared_preload_libraries

This seems like a rather poorly thought-through error check.
It will break not only the buildfarm but any dump/restore scenario.
You really can't have extensions that refuse to let themselves
be created.

Yeah, the original idea behind it was to force the user to think about
if they really would want to load it later on down the line rather than
have it pre-loaded always.

I'll put something in the docs which recommends it and provides the
reasoning behind it.

Could we apply a check at some later time, when the user actually does
something that is not sensible unless the library was preloaded? Even
then, just a WARNING might be better than ERROR.

(Still, it's not clear how you'd get buildfarm testing to pass, so
maybe this line of thought is just as fruitless.)

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

+ ERROR: pg_audit must be loaded via shared_preload_libraries

This seems like a rather poorly thought-through error check.
It will break not only the buildfarm but any dump/restore scenario.
You really can't have extensions that refuse to let themselves
be created.

Yeah, the original idea behind it was to force the user to think about
if they really would want to load it later on down the line rather than
have it pre-loaded always.

I'll put something in the docs which recommends it and provides the
reasoning behind it.

Could we apply a check at some later time, when the user actually does
something that is not sensible unless the library was preloaded? Even
then, just a WARNING might be better than ERROR.

Actually, we do.. The order in which the hooks are called matters and
if it's not loaded before anything real is called then it's going to
blow up.

(Still, it's not clear how you'd get buildfarm testing to pass, so
maybe this line of thought is just as fruitless.)

I've pushed a change which should clean it up by simply loading the
module after each reconnects is done, more-or-less simulating having it
be in shared_preload_libraries. It also wasn't using the correct
database for reconnecting.

I'll keep an eye on it.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

I've pushed a change which should clean it up by simply loading the
module after each reconnects is done, more-or-less simulating having it
be in shared_preload_libraries. It also wasn't using the correct
database for reconnecting.

I'll keep an eye on it.

Another thing that looks not amazingly well-thought-out about that
regression test is that it creates a superuser role with a known name
(and no password, not that adding a password would make it better).
This seems like it's just asking for trouble, especially in installcheck
scenarios where failure partway through would leave the superuser lying
around ready to be exploited. Do we *really* need that in the test?

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

I've pushed a change which should clean it up by simply loading the
module after each reconnects is done, more-or-less simulating having it
be in shared_preload_libraries. It also wasn't using the correct
database for reconnecting.

I'll keep an eye on it.

Another thing that looks not amazingly well-thought-out about that
regression test is that it creates a superuser role with a known name
(and no password, not that adding a password would make it better).

We create a lot of roles in other tests too; the foreign_data test is
the only one that create a superuser role. While working on the tests
for the DDL deparse thing, I had to create a script with a list of roles
that all the tests use, and it's pretty amazing. I remember thinking at
the time that it'd be better to initialize a number of standard roles in
an initial step, and have them be used consistently in the tests that
require them, rather than having create/drop everywhere.

-- create roles used throughout the tests
create role clstr_user;
create role "current_user";
create role foreign_data_user;
create role "Public";
create role regressgroup1;
create role regressgroup2;
create role regression_bob;
create role regression_group;
create role regression_user1;
create role regression_user2;
create role regression_user3;
create role regression_user;
create role regresslo;
create role regress_rol_lock1;
create role regress_test_indirect;
create role regress_test_role;
create role regress_test_role2;
create role regress_test_role_super superuser;
create role regressuser1;
create role regressuser2;
create role regressuser3;
create role regressuser4;
create role regressuser5;
create role regtest_unpriv_user;
create role regtest_addr_user;
create role regtest_alter_user1;
create role regtest_alter_user2;
create role regtest_alter_user3;
create role rls_regress_group1;
create role rls_regress_group2;
create role rls_regress_user0;
create role rls_regress_user1;
create role rls_regress_user2;
create role rls_regress_exempt_user;
create role schemauser2;
create role seclabel_user1;
create role seclabel_user2;
create role selinto_user;
create role testrol1;
create role testrol2;
create role testrolx;
create role unprivileged_role;
create role "user";
create role view_user2;

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

I've pushed a change which should clean it up by simply loading the
module after each reconnects is done, more-or-less simulating having it
be in shared_preload_libraries. It also wasn't using the correct
database for reconnecting.

I'll keep an eye on it.

Another thing that looks not amazingly well-thought-out about that
regression test is that it creates a superuser role with a known name
(and no password, not that adding a password would make it better).
This seems like it's just asking for trouble, especially in installcheck
scenarios where failure partway through would leave the superuser lying
around ready to be exploited. Do we *really* need that in the test?

I'm not sure that we need it.. It's certainly darn handy to have since
we're reconnecting to get the role attributes updated when the GUCs are
being changed and we need to be doing that as a superuser. We certainly
create superuser login roles throughout the regression tests today,
including those in core (CREATE ROLE foreign_data_user LOGIN SUPERUSER;
for example). I had thought the issues with having a superuser role in
the regression tests had been addressed by not allowing the socket to be
in a public spot and the other changes..

We don't currently drop the role though at the end though.

If we had the name of the role to reconnect as then I don't think we'd
need the "super" role, but we'd need to be able to reconnect as that
role too, to be able to flip back and forth between roles which are not
superusers and ones which are.

Will investigate that, suggestions certainly welcome. Looks like
there's a more interesting issue that termite ran into.

Thanks!

Stephen

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

Another thing that looks not amazingly well-thought-out about that
regression test is that it creates a superuser role with a known name
(and no password, not that adding a password would make it better).

We create a lot of roles in other tests too; the foreign_data test is
the only one that create a superuser role. While working on the tests
for the DDL deparse thing, I had to create a script with a list of roles
that all the tests use, and it's pretty amazing. I remember thinking at
the time that it'd be better to initialize a number of standard roles in
an initial step, and have them be used consistently in the tests that
require them, rather than having create/drop everywhere.

It would definitely be better if the names were less randomly chosen and
hence less likely to conflict with existing role names in an installation.
I'm not sure why we don't insist that they should all start with "regress"
or similar, for instance.

But what I'm on about at the moment is that I think creating new
superusers is a bad idea from a security standpoint. It seems quite
unlikely that we *have* to do that for testing purposes.

Also, I notice that the pg_audit test fails to drop the roles it
created, even if it reaches the end successfully. That's just bad.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Stephen Frost <sfrost@snowman.net> writes:

We don't currently drop the role though at the end though.

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails. Please fix.

Will investigate that, suggestions certainly welcome. Looks like
there's a more interesting issue that termite ran into.

Yeah, lapwing too. If it reproduces on any of my critters, I'll
take a look.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

We don't currently drop the role though at the end though.

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails. Please fix.

Right, will do, though one kind of requires the other (we can't drop the
only user we know how to connect as which is a superuser...). I'll
figure out a way to make it work though.

Will investigate that, suggestions certainly welcome. Looks like
there's a more interesting issue that termite ran into.

Yeah, lapwing too. If it reproduces on any of my critters, I'll
take a look.

That'd be great, I'm trying to reproduce it here but I'm not seeing it.

I'm at least somewhat suspicious it has to do with loading the library,
since we (pretty clearly..) didn't test that much as it's not really
intended.

Thanks!

Stephen

On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:

Add pg_audit, an auditing extension

This extension provides detailed logging classes, ability to control
logging at a per-object level, and includes fully-qualified object
names for logged statements (DML and DDL) in independent fields of the
log output.

Authors: Ian Barwick, Abhijit Menon-Sen, David Steele
Reviews by: Robert Haas, Tatsuo Ishii, Sawada Masahiko, Fujii Masao,
Simon Riggs

Discussion with: Josh Berkus, Jaime Casanova, Peter Eisentraut,
David Fetter, Yeb Havinga, Alvaro Herrera, Petr Jelinek, Tom Lane,
MauMau, Bruce Momjian, Jim Nasby, Michael Paquier,
Fabrízio de Royes Mello, Neil Tiffin

A quick glance shows a parameter called pg_audit.log_statement_once,
which isn't mentioned in the docs. Also, pg_audit.log_level isn't in
the logs, but seems to offer a superset of functionality to
pg_audit.log_notice, which doesn't appear as a valid config parameter.

Is this expected? Are the doc updates supposed to upcoming anyway?

--
Thom

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Thom,

* Thom Brown (thom@linux.com) wrote:

A quick glance shows a parameter called pg_audit.log_statement_once,
which isn't mentioned in the docs. Also, pg_audit.log_level isn't in
the logs, but seems to offer a superset of functionality to
pg_audit.log_notice, which doesn't appear as a valid config parameter.

Is this expected? Are the doc updates supposed to upcoming anyway?

Thanks! Yes, doc updates are coming and I'll make sure to include
those.

Thanks again,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails. Please fix.

Right, will do, though one kind of requires the other (we can't drop the
only user we know how to connect as which is a superuser...). I'll
figure out a way to make it work though.

Instead of physically reconnecting, could you do SET ROLE or SET SESSION
AUTHORIZATION? I think that's what we do in the core tests.

I'm at least somewhat suspicious it has to do with loading the library,
since we (pretty clearly..) didn't test that much as it's not really
intended.

There are several more crashes in the BF now. They're not at initial
library load AFAICS. Hard to tell if it's platform-specific or just
randomly fails sometimes. Have you tried valgrind to see if there's
uninitialized-memory touches?

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails. Please fix.

Right, will do, though one kind of requires the other (we can't drop the
only user we know how to connect as which is a superuser...). I'll
figure out a way to make it work though.

Instead of physically reconnecting, could you do SET ROLE or SET SESSION
AUTHORIZATION? I think that's what we do in the core tests.

Unfortunately, neither of those reset the role GUCs which are set with
ALTER ROLE.

I'm at least somewhat suspicious it has to do with loading the library,
since we (pretty clearly..) didn't test that much as it's not really
intended.

There are several more crashes in the BF now. They're not at initial
library load AFAICS. Hard to tell if it's platform-specific or just
randomly fails sometimes. Have you tried valgrind to see if there's
uninitialized-memory touches?

Will give that a shot and see what it says.

Thanks!

Stephen

Thom,

* Thom Brown (thom@linux.com) wrote:

On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:

Add pg_audit, an auditing extension

A quick glance shows a parameter called pg_audit.log_statement_once,
which isn't mentioned in the docs. Also, pg_audit.log_level isn't in
the logs, but seems to offer a superset of functionality to
pg_audit.log_notice, which doesn't appear as a valid config parameter.

Is this expected? Are the doc updates supposed to upcoming anyway?

Actually, I see them both in pgaudit.sgml in master..?

Is there somewhere else they're missing from?

Thanks!

Stephen

On 14 May 2015 at 17:45, Stephen Frost <sfrost@snowman.net> wrote:

Thom,

* Thom Brown (thom@linux.com) wrote:

On 14 May 2015 at 15:36, Stephen Frost <sfrost@snowman.net> wrote:

Add pg_audit, an auditing extension

A quick glance shows a parameter called pg_audit.log_statement_once,
which isn't mentioned in the docs. Also, pg_audit.log_level isn't in
the logs, but seems to offer a superset of functionality to
pg_audit.log_notice, which doesn't appear as a valid config parameter.

Is this expected? Are the doc updates supposed to upcoming anyway?

Actually, I see them both in pgaudit.sgml in master..?

Is there somewhere else they're missing from?

Ah, my apologies. I can see from the file date that pgaudit.html is
dated 2 weeks ago, so I'm looking at an old copy. Not sure what
happened there as all other files were newer. In any case, I've
rebuilt and see neither of those issues I reported exist anymore.
Sorry for the noise.

--
Thom

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Thom,

* Thom Brown (thom@linux.com) wrote:

On 14 May 2015 at 17:45, Stephen Frost <sfrost@snowman.net> wrote:

Is there somewhere else they're missing from?

Ah, my apologies. I can see from the file date that pgaudit.html is
dated 2 weeks ago, so I'm looking at an old copy. Not sure what
happened there as all other files were newer. In any case, I've
rebuilt and see neither of those issues I reported exist anymore.
Sorry for the noise.

No problem at all, please let me know if you come across any issues.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

There are several more crashes in the BF now. They're not at initial
library load AFAICS. Hard to tell if it's platform-specific or just
randomly fails sometimes. Have you tried valgrind to see if there's
uninitialized-memory touches?

Will give that a shot and see what it says.

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.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

There are several more crashes in the BF now. They're not at initial
library load AFAICS. Hard to tell if it's platform-specific or just
randomly fails sometimes. Have you tried valgrind to see if there's
uninitialized-memory touches?

Will give that a shot and see what it says.

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.

Thanks!

Stephen

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

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

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Quite aside from any security risks, that means that running "make
installcheck" twice in a row fails. Please fix.

Right, will do, though one kind of requires the other (we can't drop the
only user we know how to connect as which is a superuser...). I'll
figure out a way to make it work though.

Instead of physically reconnecting, could you do SET ROLE or SET SESSION
AUTHORIZATION? I think that's what we do in the core tests.

Alright, I believe this has been fixed now, using the brand-new \gset
option.

Two installcheck's in a row still breaks though.. I'm not quite sure
what to do about that but I'm certainly open to thoughts. I can reset
the role attributes later, but those get logged with the username used
too in the ALTER statement, which changes.

I'll continue to think about it though, perhaps there's a way I can
disable logging as the superuser without it logging the role involved.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

I'll continue to think about it though, perhaps there's a way I can
disable logging as the superuser without it logging the role involved.

Of course, it occured to me how to address this immediately after, even
though it hadn't in the hour or so prior. I can just bump
client_min_messages up to warning and then reset the role attributes...

That appears to be working. Will push an update to fix this shortly.

Thanks!

Stephen

Hi

I am testing it, and output is strange

2015-05-15 11:49:25.046 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT: drop table foo;
2015-05-15 11:49:28.291 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
logged>
2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT: CREATE TABLE foo(a
int, b int);
2015-05-15 11:49:31.486 CEST pavel postgres: LOG: AUDIT:
SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT: INSERT INTO foo
VALUES(10,20);
2015-05-15 11:49:33.446 CEST pavel postgres: LOG: AUDIT:
SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT: SELECT * FROM foo
WHERE a = 10;

I am missing object name, unexpected string <not logged>

configuration:
pg_audit.log = 'read, write, ddl'

2015-05-14 21:30 GMT+02:00 Stephen Frost <sfrost@snowman.net>:

On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am testing it, and output is strange

2015-05-15 11:49:25.046 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT: drop table foo;
2015-05-15 11:49:28.291 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
logged>
2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT: CREATE TABLE foo(a
int, b int);
2015-05-15 11:49:31.486 CEST pavel postgres: LOG: AUDIT:
SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT: INSERT INTO foo
VALUES(10,20);
2015-05-15 11:49:33.446 CEST pavel postgres: LOG: AUDIT:
SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT: SELECT * FROM foo
WHERE a = 10;

I am missing object name, unexpected string <not logged>

configuration:
pg_audit.log = 'read, write, ddl'

From what I can tell, that last value should be for parameters. You'd
need to set pg_audit.log_parameter to on, and then prepare and execute
a statement with a parameter.

--
Thom

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

2015-05-15 12:14 GMT+02:00 Thom Brown <thom@linux.com>:

On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am testing it, and output is strange

2015-05-15 11:49:25.046 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT: drop table foo;
2015-05-15 11:49:28.291 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
logged>
2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT: CREATE TABLE

foo(a

int, b int);
2015-05-15 11:49:31.486 CEST pavel postgres: LOG: AUDIT:
SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT: INSERT INTO foo
VALUES(10,20);
2015-05-15 11:49:33.446 CEST pavel postgres: LOG: AUDIT:
SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT: SELECT * FROM

foo

WHERE a = 10;

I am missing object name, unexpected string <not logged>

configuration:
pg_audit.log = 'read, write, ddl'

From what I can tell, that last value should be for parameters. You'd
need to set pg_audit.log_parameter to on, and then prepare and execute
a statement with a parameter.

yes

2015-05-15 12:18:39.545 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,READ,PREPARE,,,prepare x(int) as select * from foo where a =
$1;,<none>
2015-05-15 12:18:39.545 CEST pavel postgres: STATEMENT: prepare x(int) as
select * from foo where a = $1;
2015-05-15 12:18:48.065 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,READ,SELECT,,,prepare x(int) as select * from foo where a =
$1;,10
2015-05-15 12:18:48.065 CEST pavel postgres: STATEMENT: execute x(10);

but when pg_audit.log_parameter is off, then this value should be empty

Regards

Pavel

On 15 May 2015 at 11:20, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-15 12:14 GMT+02:00 Thom Brown <thom@linux.com>:

On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am testing it, and output is strange

2015-05-15 11:49:25.046 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT: drop table foo;
2015-05-15 11:49:28.291 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
logged>
2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT: CREATE TABLE
foo(a
int, b int);
2015-05-15 11:49:31.486 CEST pavel postgres: LOG: AUDIT:
SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not logged>
2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT: INSERT INTO foo
VALUES(10,20);
2015-05-15 11:49:33.446 CEST pavel postgres: LOG: AUDIT:
SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT: SELECT * FROM
foo
WHERE a = 10;

I am missing object name, unexpected string <not logged>

configuration:
pg_audit.log = 'read, write, ddl'

From what I can tell, that last value should be for parameters. You'd
need to set pg_audit.log_parameter to on, and then prepare and execute
a statement with a parameter.

yes

2015-05-15 12:18:39.545 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,READ,PREPARE,,,prepare x(int) as select * from foo where a =
$1;,<none>
2015-05-15 12:18:39.545 CEST pavel postgres: STATEMENT: prepare x(int) as
select * from foo where a = $1;
2015-05-15 12:18:48.065 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,READ,SELECT,,,prepare x(int) as select * from foo where a =
$1;,10
2015-05-15 12:18:48.065 CEST pavel postgres: STATEMENT: execute x(10);

but when pg_audit.log_parameter is off, then this value should be empty

If it were empty, it would then be indistinguishable from a statement
executed with a single parameter passed as an empty string.

Speaking of which, I notice that nulls show up as nothing too. How
does one distinguish between an empty string and a null in the logs?

--
Thom

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

2015-05-15 12:37 GMT+02:00 Thom Brown <thom@linux.com>:

On 15 May 2015 at 11:20, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2015-05-15 12:14 GMT+02:00 Thom Brown <thom@linux.com>:

On 15 May 2015 at 10:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

I am testing it, and output is strange

2015-05-15 11:49:25.046 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,DDL,DROP TABLE,,,drop table foo;,<not logged>
2015-05-15 11:49:25.046 CEST pavel postgres: STATEMENT: drop table

foo;

2015-05-15 11:49:28.291 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,DDL,CREATE TABLE,,,"CREATE TABLE foo(a int, b int);",<not
logged>
2015-05-15 11:49:28.291 CEST pavel postgres: STATEMENT: CREATE TABLE
foo(a
int, b int);
2015-05-15 11:49:31.486 CEST pavel postgres: LOG: AUDIT:
SESSION,3,1,WRITE,INSERT,,,"INSERT INTO foo VALUES(10,20);",<not

logged>

2015-05-15 11:49:31.486 CEST pavel postgres: STATEMENT: INSERT INTO

foo

VALUES(10,20);
2015-05-15 11:49:33.446 CEST pavel postgres: LOG: AUDIT:
SESSION,4,1,READ,SELECT,,,SELECT * FROM foo WHERE a = 10;,<not logged>
2015-05-15 11:49:33.446 CEST pavel postgres: STATEMENT: SELECT * FROM
foo
WHERE a = 10;

I am missing object name, unexpected string <not logged>

configuration:
pg_audit.log = 'read, write, ddl'

From what I can tell, that last value should be for parameters. You'd
need to set pg_audit.log_parameter to on, and then prepare and execute
a statement with a parameter.

yes

2015-05-15 12:18:39.545 CEST pavel postgres: LOG: AUDIT:
SESSION,1,1,READ,PREPARE,,,prepare x(int) as select * from foo where a =
$1;,<none>
2015-05-15 12:18:39.545 CEST pavel postgres: STATEMENT: prepare x(int)

as

select * from foo where a = $1;
2015-05-15 12:18:48.065 CEST pavel postgres: LOG: AUDIT:
SESSION,2,1,READ,SELECT,,,prepare x(int) as select * from foo where a =
$1;,10
2015-05-15 12:18:48.065 CEST pavel postgres: STATEMENT: execute x(10);

but when pg_audit.log_parameter is off, then this value should be empty

If it were empty, it would then be indistinguishable from a statement
executed with a single parameter passed as an empty string.

I am thinking about situation when pg_audit.log_parameter is disabled

Speaking of which, I notice that nulls show up as nothing too. How
does one distinguish between an empty string and a null in the logs?

it can use a COPY notation

* Thom Brown (thom@linux.com) wrote:

If it were empty, it would then be indistinguishable from a statement
executed with a single parameter passed as an empty string.

NULL, but yes.

Speaking of which, I notice that nulls show up as nothing too. How
does one distinguish between an empty string and a null in the logs?

It should be "", to match how the COPY protocol handles it. Will take a
look.

Thanks!

Stephen

On Thu, May 14, 2015 at 11:36 PM, Stephen Frost <sfrost@snowman.net> wrote:

Add pg_audit, an auditing extension

This extension provides detailed logging classes, ability to control
logging at a per-object level, and includes fully-qualified object
names for logged statements (DML and DDL) in independent fields of the
log output.

Authors: Ian Barwick, Abhijit Menon-Sen, David Steele
Reviews by: Robert Haas, Tatsuo Ishii, Sawada Masahiko, Fujii Masao,
Simon Riggs

Discussion with: Josh Berkus, Jaime Casanova, Peter Eisentraut,
David Fetter, Yeb Havinga, Alvaro Herrera, Petr Jelinek, Tom Lane,
MauMau, Bruce Momjian, Jim Nasby, Michael Paquier,
Fabrízio de Royes Mello, Neil Tiffin

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/ac52bb0442f79076b14acd8ad5b696946c1053b8

Modified Files
--------------
contrib/Makefile | 1 +
contrib/pg_audit/.gitignore | 5 +
contrib/pg_audit/Makefile | 21 +
contrib/pg_audit/expected/pg_audit.out | 964 ++++++++++++++++
contrib/pg_audit/pg_audit--1.0.0.sql | 22 +
contrib/pg_audit/pg_audit.c | 1870 ++++++++++++++++++++++++++++++++
contrib/pg_audit/pg_audit.conf | 1 +
contrib/pg_audit/pg_audit.control | 5 +
contrib/pg_audit/sql/pg_audit.sql | 617 +++++++++++
doc/src/sgml/contrib.sgml | 1 +
doc/src/sgml/filelist.sgml | 1 +
doc/src/sgml/pgaudit.sgml | 678 ++++++++++++
12 files changed, 4186 insertions(+)

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR: pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT: SQL statement "SELECT UPPER(object_type), object_identity
FROM pg_event_trigger_ddl_commands()"

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

Regards,

--
Fujii Masao

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

I have to admit, I didn't look closely at how we handled versions in
contrib modules and that has been the same since the patch was first
posted, as I recall. No problem changing it to 1.0 and I'll take care
of that soon.

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR: pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT: SQL statement "SELECT UPPER(object_type), object_identity
FROM pg_event_trigger_ddl_commands()"

Interesting. I'm very curious about this error and if it impacts other
extensions which use event triggers. I'll look into it.

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

Good points, will address.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

David, Simon and I have all considered this at different times and my
recollection is that we all felt that it made sense as DDL because
CLUSTER is DDL (which is actually noted in the comments). However, you
bring up a good point that classifying it as DDL makes it different from
what the core system does and it'd probably be good to be consistent.

The question here really is- is that the right classification for
REINDEX to have in core? If so, shouldn't CLUSTER have the same?
Probably a discussion to have on -hackers rather than here. I'll go
ahead and change it to match what core does. Then, if core changes to
make it DDL, pg_audit will automatically pick that up and also treat it
as DDL.

Thanks!

Stephen

Stephen Frost wrote:

* Fujii Masao (masao.fujii@gmail.com) wrote:

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR: pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT: SQL statement "SELECT UPPER(object_type), object_identity
FROM pg_event_trigger_ddl_commands()"

Interesting. I'm very curious about this error and if it impacts other
extensions which use event triggers. I'll look into it.

Can you explain what's going on here? It seems a bit odd to me.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

David, Simon and I have all considered this at different times and my
recollection is that we all felt that it made sense as DDL because
CLUSTER is DDL (which is actually noted in the comments). However, you
bring up a good point that classifying it as DDL makes it different from
what the core system does and it'd probably be good to be consistent.

The question here really is- is that the right classification for
REINDEX to have in core? If so, shouldn't CLUSTER have the same?

Thiking a bit about this, it seems that REINDEX and CLUSTER are not
really DDL. They are more "maintenance commands"; they don't define
anything that wasn't defined before.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 5/15/15 3:44 PM, Stephen Frost wrote:

* Fujii Masao (masao.fujii@gmail.com) wrote:

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR: pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT: SQL statement "SELECT UPPER(object_type), object_identity
FROM pg_event_trigger_ddl_commands()"

Interesting. I'm very curious about this error and if it impacts other
extensions which use event triggers. I'll look into it.

I'm looking into this. There's definitely something strange going on
here, but not sure it's in pg_audit.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

David, Simon and I have all considered this at different times and my
recollection is that we all felt that it made sense as DDL because
CLUSTER is DDL (which is actually noted in the comments). However, you
bring up a good point that classifying it as DDL makes it different from
what the core system does and it'd probably be good to be consistent.

The question here really is- is that the right classification for
REINDEX to have in core? If so, shouldn't CLUSTER have the same?
Probably a discussion to have on -hackers rather than here. I'll go
ahead and change it to match what core does. Then, if core changes to
make it DDL, pg_audit will automatically pick that up and also treat it
as DDL.

I admit I was influenced by this comment in core:

case T_ReindexStmt:
lev = LOGSTMT_ALL; /* should this be DDL? */
break;

and the fact that CLUSTER is DDL. I'm not married to REINDEX as DDL,
though. I'm happy to make it consistent with core. If it changes in
core, it would also change in pg_audit.

--
- David Steele
david@pgmasters.net

On Sat, May 16, 2015 at 4:44 AM, Stephen Frost <sfrost@snowman.net> wrote:

Fujii,

* Fujii Masao (masao.fujii@gmail.com) wrote:

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

I have to admit, I didn't look closely at how we handled versions in
contrib modules and that has been the same since the patch was first
posted, as I recall. No problem changing it to 1.0 and I'll take care
of that soon.

CREATE EXTENSION pg_audit failed with the following error message
when shared_preload_libraries and pg_audit.log are set to pg_audit and
ddl, respectively.

=# create extension pg_audit ;
ERROR: pg_event_trigger_ddl_commands() can only be called in an event
trigger function
CONTEXT: SQL statement "SELECT UPPER(object_type), object_identity
FROM pg_event_trigger_ddl_commands()"

Interesting. I'm very curious about this error and if it impacts other
extensions which use event triggers. I'll look into it.

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

Good points, will address.

And on top of that the following things should be changed:
- Removal of REGRESS_OPTS which is empty
- Removal of MODULE, which overlaps with MODULE_big
- $(WIN32RES) needs to be added to OBJS for Windows versioning
Please find in the patch attached the fixes needed.
--
Michael

Fujii, Michael,

* Fujii Masao (masao.fujii@gmail.com) wrote:

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

[...]

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

[...]

* Michael Paquier (michael.paquier@gmail.com) wrote:

And on top of that the following things should be changed:
- Removal of REGRESS_OPTS which is empty
- Removal of MODULE, which overlaps with MODULE_big
- $(WIN32RES) needs to be added to OBJS for Windows versioning

I've pushed these changes.

Thanks!

Stephen

On Sun, May 17, 2015 at 11:00 PM, Stephen Frost <sfrost@snowman.net> wrote:

Fujii, Michael,

* Fujii Masao (masao.fujii@gmail.com) wrote:

pg_audit uses 1.0.0 as its version number. But, is the third digit really
required? Why? We usually uses the version number with two digits in
contrib modules.

[...]

In Makefile, PGFILEDESC should be added.

+# pg_audit/Makefile

should be "# contrib/pg_audit/Makefile" for the consistency.

The categories of some SQL commands are different between log_statement and
pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in
log_statement. That's confusing. We should use the same "category-mapping"
rule as much as possible.

[...]

* Michael Paquier (michael.paquier@gmail.com) wrote:

And on top of that the following things should be changed:
- Removal of REGRESS_OPTS which is empty
- Removal of MODULE, which overlaps with MODULE_big
- $(WIN32RES) needs to be added to OBJS for Windows versioning

I've pushed these changes.

Thanks a lot!

Here are other random comments on pg_audit.
# Move to pgsql-hackers

Isn't it better to suppress STATEMENT message in audit log?
Otherwise, we always get two duplicate statements as follows, and which would
increases the audit log volume very much, I'm afraid.

LOG: AUDIT: SESSION,1,1,READ,SELECT,,,SELECT 1;,<not logged>
STATEMENT: SELECT 1;

Also CONTEXT message containing the same statement can sometimes be logged
at the same time.

When I tried the object audit feature, I got the following log messages.
The class and command type of this SELECT query is WRITE and UPDATE in the log
message. Is this intentional? Why? Maybe pg_audit treats something like
"SELECT FOR SHARE/UPDATE" as UPDATE command? But in session logging,
the class and command type of this query was READ and SELECT. So confusing.

LOG: AUDIT: OBJECT,2,1,WRITE,UPDATE,TABLE,public.aaa,"SELECT 1
FROM ONLY ""public"".""aaa"" x WHERE ""id"" OPERATOR(pg_catalog.=) $1
FOR KEY SHARE OF x",<not logged>
CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."aaa" x WHERE
"id" OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x"
STATEMENT: INSERT INTO bbb VALUES (1);

The queries to reproduce the above message is here.

ALTER SYSTEM SET pg_audit.role TO 'foo';
SELECT pg_reload_conf();
CREATE TABLE aaa (id int primary key);
CREATE TABLE bbb (id int references aaa(id));
INSERT INTO aaa VALUES(generate_series(1,100));
GRANT SELECT ON aaa TO foo;
INSERT INTO bbb VALUES (1);

Now the class of FETCH command is MISC, but isn't it better to classify that as
READ?

When a query contains a double quote or comma, it's quoted in the audit log
as follows. Doesn't this make the analysis of the queries in the log files a bit
difficult because other queries are not quoted?

LOG: AUDIT: SESSION,2,1,READ,SELECT,,,"SELECT '""';",<not logged>
STATEMENT: SELECT '"';

When log_destination is csvlog, the above quoted query is quoted again.

2015-05-18 14:44:38.313
JST,"postgres","postgres",17725,"[local]",55597c45.453d,1,"SELECT",2015-05-18
14:44:37 JST,2/2,0,LOG,00000,"AUDIT:
SESSION,1,1,READ,SELECT,,,""SELECT '""""';"",<not
logged>",,,,,,"SELECT '""';",,,"psql"

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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 solution to
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 (trusting event
triggers to cover everything under CREATE SCHEMA). The event trigger
is correctly collecting the GRANT command and pg_audit is outputting a
record based on that (when DDL is enabled), but it's not detecting
that the subcommand is a GRANT. That's certainly not great, but it's
not an insurmountable problem either- correct the early exit logic,
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 only code-level issue
found by the buildfarm- all of the other commits associated with the
buildfarm were due to how the regression tests 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 it here 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 documented accordingly, as
we felt the comment in the core code was correct, but, even so, we
agreed with Fujii that we should probably 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 decide on 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

2015-05-28 3:30 GMT+02:00 Joshua D. Drake <jd@commandprompt.com>:

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.

any work in contrib has bigger progress.

I wrote plpgsql_check - and the living out of contrib is neutral - I can
do some changes freely, but it is used 1% users who can has benefit from
this extension. 9.5 will be released after three months and lot of
potential bugs can be fixed. It is contrib - it can be moved, removed, but
better if can works.

Regards

Pavel

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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