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