Auditing extension for PostgreSQL (Take 2)

David,

I've CC'd Abhijit, the original author of pgaudit, as it seems likely
he'd also be interested in this.

* David Steele (david@pgmasters.net) wrote:

I've posted a couple of messages over the last few weeks about the work
I've been doing on the pg_audit extension. The lack of response could
be due to either universal acclaim or complete apathy, but in any case I
think this is a very important topic so I want to give it another try.

Thanks! It's certainly an important topic to a lot of folks; I imagine
the lack of response is simply because people are busy.

I've extensively reworked the code that was originally submitted by
2ndQuandrant. This is not an indictment of their work, but rather an
attempt to redress concerns that were expressed by members of the
community. I've used core functions to determine how audit events
should be classified and simplified and tightened the code wherever
possible. I've removed deparse and event triggers and opted for methods
that rely only on existing hooks. In my last message I provided
numerous examples of configuration, usage, and output which I hoped
would alleviate concerns of complexity. I've also written a ton of unit
tests to make sure that the code works as expected.

The configuration approach you posted is definitely in-line with what I
was trying to get at previously- thanks for putting some actual code
behind it! While not a big fan of that other big RDBMS, I do like that
this approach ends up being so similar in syntax.

I realize this is not an ideal solution. Everybody (including me) wants
something that is in core with real policies and more options. It's
something that I am personally really eager to work on. But the reality
is that's not going to happen for 9.5 and probably not for 9.6 either.
Meanwhile, I believe the lack of some form of auditing is harming
adoption of PostgreSQL, especially in the financial and government sectors.

Agreed.

The patch I've attached satisfies the requirements that I've had from
customers in the past. I'm confident that if it gets out into the wild
it will bring all kinds of criticism and comments which will be valuable
in designing a robust in-core solution.

This is definitely something that makes sense to me, particularly for
such an important piece. I had argued previously that a contrib based
solution would make it difficult to build an in-core solution, but
others convinced me that it'd not only be possible but would probably be
preferrable as we'd gain experience with the contrib module and, as you
say, we'd be able to build a better in-core solution based on that
experience.

I'm submitting this patch to the Commitfest. I'll do everything I can
to address the concerns of the community and I'm happy to provide more
examples as needed. I'm hoping the sgml docs I've provided with the
patch will cover any questions, but of course feedback is always
appreciated.

Glad you submitted it to the CommitFest. Just glancing through the
code, it certainly looks a lot closer to being something which we could
move forward with. Using existing functions to work out the categories
(instead of massive switch statements) is certainly much cleaner and
removing those large #ifdef blocks has made the code a lot easier to
follow.

Lastly, I really like all the unit tests..

Additional comments in-line follow.

diff --git a/contrib/pg_audit/pg_audit.c b/contrib/pg_audit/pg_audit.c
new file mode 100644
index 0000000..b3914ac
--- /dev/null
+++ b/contrib/pg_audit/pg_audit.c
@@ -0,0 +1,1099 @@
+/*------------------------------------------------------------------------------
+ * pg_audit.c
+ *
+ * An auditing extension for PostgreSQL. Improves on standard statement logging
+ * by adding more logging classes, object level logging, and providing
+ * fully-qualified object names for all DML and many DDL statements.

It'd be good to quantify what 'many' means above.

+ * Copyright (c) 2014-2015, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		  contrib/pg_prewarm/pg_prewarm.c

Pretty sure this isn't pg_prewarm.c :)

+/*
+ * String contants for audit types - used when logging to distinguish session
+ * vs. object auditing.
+ */

"String constants"

+/*
+ * String contants for log classes - used when processing tokens in the
+ * pgaudit.log GUC.
+ */

Ditto.

+/* String contants for logging commands */

Ditto. :)

+/*
+ * This module collects AuditEvents from various sources (event triggers, and
+ * executor/utility hooks) and passes them to the log_audit_event() function.

This isn't using event triggers any more, right? Doesn't look like it.
I don't think that's a problem and it certainly seems to have simplified
things quite a bit, but the comment should be updated.

+/*
+ * Returns the oid of the role specified in pgaudit.role.
+ */
+static Oid
+audit_role_oid()

Couldn't you use get_role_oid() instead of having your own function..?

+	/*
+	 * Check privileges granted indirectly via role memberships. We do this in
+	 * a separate pass to minimize expensive indirect membership tests.  In
+	 * particular, it's worth testing whether a given ACL entry grants any
+	 * privileges still of interest before we perform the has_privs_of_role
+	 * test.
+	 */

I'm a bit on the fence about this.. Can you provide a use-case where
doing this makes sense..? Does this mean I could grant admin_role1 to
audit and then get auditing on everything that user1 has access to?
That seems like it might be useful for environments where such roles
already exist though it might end up covering more than is intended..

+ /* Make a copy of the column set */
+ tmpSet = bms_copy(attributeSet);

The comment should probably say why, eg:

/* bms_first_member is destructive, so make a copy before using it. */

+/*
+ * Define GUC variables and install hooks upon module load.
+ */
+void
+_PG_init(void)
+{
+	if (IsUnderPostmaster)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("pgaudit must be loaded via shared_preload_libraries")));
+
+	/*
+	 * pgaudit.role = "role1"

I'd make that example "audit", since that's what is generally expected,
right?

+################################################################################
+# test.pl - pgAudit Unit Tests
+################################################################################

This is definitiely nice..

diff --git a/doc/src/sgml/pgaudit.sgml b/doc/src/sgml/pgaudit.sgml
new file mode 100644
index 0000000..f3f4ab9
--- /dev/null
+++ b/doc/src/sgml/pgaudit.sgml
@@ -0,0 +1,316 @@
+<!-- doc/src/sgml/pgaudit.sgml -->
+
+<sect1 id="pgaudit" xreflabel="pgaudit">
+  <title>pg_audit</title>

There seems to be a number of places which are 'pgaudit' and a bunch
that are 'pg_audit'. I'm guessing you were thinking 'pg_audit', but
it'd be good to clean up and make them all consistent.

+  <indexterm zone="pgaudit">
+    <primary>pg_audit</primary>
+  </indexterm>
+
+  <para>
+    The <filename>pg_audit</filename> module provides session and object
+    auditing via the standard logging facility.  Session and object auditing are
+    completely independent and can be combined.
+  </para>
+
+  <sect2>
+    <title>Session Auditing</title>
+
+    <para>
+      Session auditing allows the logging of all commands that are executed by
+      a user in the backend.  Each command is logged with a single entry and
+      includes the audit type (e.g. <literal>SESSION</literal>), command type
+      (e.g. <literal>CREATE TABLE</literal>, <literal>SELECT</literal>) and
+      statement (e.g. <literal>"select * from test"</literal>).
+
+      Fully-qualified names and object types will be logged for
+      <literal>CREATE</literal>, <literal>UPDATE</literal>, and
+      <literal>DROP</literal> commands on <literal>TABLE</literal>,
+      <literal>MATVIEW</literal>, <literal>VIEW</literal>,
+      <literal>INDEX</literal>, <literal>FOREIGN TABLE</literal>,
+      <literal>COMPOSITE TYPE</literal>, <literal>INDEX</literal>, and
+      <literal>SEQUENCE</literal> objects as well as function calls.
+    </para>

Ah, you do have a list of what objects you get fully qualified names
for, nice. Are there obvious omissions from that list..? If so, we
might be able to change what happens with objectAccess to include them..

+      <para>
+        Enable session logging for all commands except miscellaneous:
+          <programlisting>
+pgaudit.log = 'all, -misc'
+          </programlisting>
+      </para>
+    </sect3>

Nice, I like that.

+    <sect3>
+      <title>Examples</title>
+
+      <para>
+        Set <literal>pgaudit.log = 'read, ddl'</literal> in
+        <literal>postgresql.conf</literal>.
+      </para>

Perhaps I missed it, but it'd be good to point out that GUCs can be set
at various levels. I know we probably say that somewhere else, but it's
particularly relevant for this.

That was just a quick read. Would be great if someone else who is
interested would take it for a spin and provide their own feedback.

Thanks again!

Stephen

On 15 February 2015 at 02:34, David Steele <david@pgmasters.net> wrote:

I've posted a couple of messages over the last few weeks about the work
I've been doing on the pg_audit extension. The lack of response could
be due to either universal acclaim or complete apathy, but in any case I
think this is a very important topic so I want to give it another try.

You mentioned you had been following the thread for some time and yet
had not contributed to it. Did that indicate your acclaim for the
earlier patch, or was that apathy? I think neither.

People have been working on this feature for >9 months now, so you
having to wait 9 days for a response is neither universal acclaim, nor
apathy. I've waited much longer than that for Stephen to give the
review he promised, but have not bad mouthed him for that wait, nor do
I do so now. In your first post you had removed the author's email
addresses, so they were likely unaware of your post. I certainly was.

I've extensively reworked the code that was originally submitted by
2ndQuandrant. This is not an indictment of their work, but rather an
attempt to redress concerns that were expressed by members of the
community. I've used core functions to determine how audit events
should be classified and simplified and tightened the code wherever
possible. I've removed deparse and event triggers and opted for methods
that rely only on existing hooks. In my last message I provided
numerous examples of configuration, usage, and output which I hoped
would alleviate concerns of complexity. I've also written a ton of unit
tests to make sure that the code works as expected.

Some people that have contributed ideas to this patch are from
2ndQuadrant, some are not. The main point is that we work together on
things, rather than writing a slightly altered version and then
claiming credit.

If you want to help, please do. We give credit where its due, not to
whoever touched the code last in some kind of bidding war. If we let
this happen, we'd generate a flood of confusing patch versions and
little would ever get committed.

Let's keep to one thread and work to include everybody's ideas then
we'll get something useful committed.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, RemoteDBA, 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

On 2/18/15 8:25 AM, Simon Riggs wrote:

On 15 February 2015 at 02:34, David Steele <david@pgmasters.net> wrote:

I've posted a couple of messages over the last few weeks about the work
I've been doing on the pg_audit extension. The lack of response could
be due to either universal acclaim or complete apathy, but in any case I
think this is a very important topic so I want to give it another try.

You mentioned you had been following the thread for some time and yet
had not contributed to it. Did that indicate your acclaim for the
earlier patch, or was that apathy? I think neither.

In my case it actually was acclaim. I was happy with the direction
things were going and had nothing in particular to add - and I didn't
think a +1 from me was going to carry any weight with the community.

I can see now that everyone's opinion matters here, so I'll be more
active about weighing in when I think something is valuable.

People have been working on this feature for >9 months now, so you
having to wait 9 days for a response is neither universal acclaim, nor
apathy. I've waited much longer than that for Stephen to give the
review he promised, but have not bad mouthed him for that wait, nor do
I do so now. In your first post you had removed the author's email
addresses, so they were likely unaware of your post. I certainly was.

I understand that, but with the CF closing I felt like I had to act.
Abhijit's last comment on the thread was that he was no longer going to
work on it in relation to 9.5. I felt that it was an important feature
(and one that I have a lot of interest in), so that's when I got involved.

I posted two messages, but I only addressed one of them directly to
Abhijit. As you said, I'm new here and I'm still getting used to the
way things are done.

I've extensively reworked the code that was originally submitted by
2ndQuandrant. This is not an indictment of their work, but rather an
attempt to redress concerns that were expressed by members of the
community. I've used core functions to determine how audit events
should be classified and simplified and tightened the code wherever
possible. I've removed deparse and event triggers and opted for methods
that rely only on existing hooks. In my last message I provided
numerous examples of configuration, usage, and output which I hoped
would alleviate concerns of complexity. I've also written a ton of unit
tests to make sure that the code works as expected.

Some people that have contributed ideas to this patch are from
2ndQuadrant, some are not. The main point is that we work together on
things, rather than writing a slightly altered version and then
claiming credit.

If you want to help, please do. We give credit where its due, not to
whoever touched the code last in some kind of bidding war. If we let
this happen, we'd generate a flood of confusing patch versions and
little would ever get committed.

Agreed, and I apologize if I came off that way. It certainly wasn't my
intention. I was hesitant because I had made so many changes and I
wasn't sure how the authors would feel about it. I wrote to them
privately to get their take on the situation.

Let's keep to one thread and work to include everybody's ideas then
we'll get something useful committed.

I'm a little confused about how to proceed here. I created a new thread
because the other patch had already been rejected. How should I handle
that?

--
- David Steele
david@pgmasters.net

Hi Stephen,

Thanks for your review. All fixed except for comments below:

On 2/17/15 10:34 AM, Stephen Frost wrote:

+	/*
+	 * Check privileges granted indirectly via role memberships. We do this in
+	 * a separate pass to minimize expensive indirect membership tests.  In
+	 * particular, it's worth testing whether a given ACL entry grants any
+	 * privileges still of interest before we perform the has_privs_of_role
+	 * test.
+	 */

I'm a bit on the fence about this.. Can you provide a use-case where
doing this makes sense..? Does this mean I could grant admin_role1 to
audit and then get auditing on everything that user1 has access to?
That seems like it might be useful for environments where such roles
already exist though it might end up covering more than is intended..

The idea is that if there are already ready-made roles to be audited
then they don't need to be reconstituted for the audit role. You could
just do:

grant admin_role to audit;
grant user_role to audit;

Of course, we could list multiple roles in the pg_audit.role GUC, but I
thought this would be easier to use and maintain since there was some
worry about GUCs being fragile when they refer to database objects.

+################################################################################
+# test.pl - pgAudit Unit Tests
+################################################################################

This is definitiely nice..

diff --git a/doc/src/sgml/pgaudit.sgml b/doc/src/sgml/pgaudit.sgml
new file mode 100644
index 0000000..f3f4ab9
--- /dev/null
+++ b/doc/src/sgml/pgaudit.sgml
@@ -0,0 +1,316 @@
+<!-- doc/src/sgml/pgaudit.sgml -->
+
+<sect1 id="pgaudit" xreflabel="pgaudit">
+  <title>pg_audit</title>

There seems to be a number of places which are 'pgaudit' and a bunch
that are 'pg_audit'. I'm guessing you were thinking 'pg_audit', but
it'd be good to clean up and make them all consistent.

Fixed, though I still left the file name as pgaudit.sgml since all but
one module follow that convention.

+    <para>
+      Session auditing allows the logging of all commands that are executed by
+      a user in the backend.  Each command is logged with a single entry and
+      includes the audit type (e.g. <literal>SESSION</literal>), command type
+      (e.g. <literal>CREATE TABLE</literal>, <literal>SELECT</literal>) and
+      statement (e.g. <literal>"select * from test"</literal>).
+
+      Fully-qualified names and object types will be logged for
+      <literal>CREATE</literal>, <literal>UPDATE</literal>, and
+      <literal>DROP</literal> commands on <literal>TABLE</literal>,
+      <literal>MATVIEW</literal>, <literal>VIEW</literal>,
+      <literal>INDEX</literal>, <literal>FOREIGN TABLE</literal>,
+      <literal>COMPOSITE TYPE</literal>, <literal>INDEX</literal>, and
+      <literal>SEQUENCE</literal> objects as well as function calls.
+    </para>

Ah, you do have a list of what objects you get fully qualified names
for, nice. Are there obvious omissions from that list..? If so, we
might be able to change what happens with objectAccess to include them..

It seems like these are the objects where having a name really matters.
I'm more interested in using the deparse code to handle fully-qualified
names for additional objects rather than adding hooks.

+    <sect3>
+      <title>Examples</title>
+
+      <para>
+        Set <literal>pgaudit.log = 'read, ddl'</literal> in
+        <literal>postgresql.conf</literal>.
+      </para>

Perhaps I missed it, but it'd be good to point out that GUCs can be set
at various levels. I know we probably say that somewhere else, but it's
particularly relevant for this.

Yes, it's very relevant for this patch. Do you think it's enough to
call out the functionality, or should I provide examples? Maybe a
separate section just for this concept?

Patch v2 is attached for all changes except the doc change above.

--
- David Steele
david@pgmasters.net

On 2/23/15 10:59 AM, David Steele wrote:

On 2/17/15 10:34 AM, Stephen Frost wrote:

There seems to be a number of places which are 'pgaudit' and a bunch
that are 'pg_audit'. I'm guessing you were thinking 'pg_audit', but
it'd be good to clean up and make them all consistent.

Fixed, though I still left the file name as pgaudit.sgml since all but
one module follow that convention.

It turns out there are a few places where _ is not allowed. I've
reverted a few places to fix the doc build while maintaining the name as
pg_audit in the visible docs.

Perhaps I missed it, but it'd be good to point out that GUCs can be set
at various levels. I know we probably say that somewhere else, but it's
particularly relevant for this.

I added notes as suggested.

Patch v3 is attached.

--
- David Steele
david@pgmasters.net

At 2015-02-24 11:22:41 -0500, david@pgmasters.net wrote:

Patch v3 is attached.

[…]

+/* Log class enum used to represent bits in auditLogBitmap */
+enum LogClass
+{
+	LOG_NONE = 0,
+
+	/* SELECT */
+	LOG_READ = (1 << 0),
+
+	/* INSERT, UPDATE, DELETE, TRUNCATE */
+	LOG_WRITE = (1 << 1),
+
+	/* DDL: CREATE/DROP/ALTER */
+	LOG_DDL = (1 << 2),
+
+	/* Function execution */
+	LOG_FUNCTION = (1 << 4),
+
+	/* Function execution */
+	LOG_MISC = (1 << 5),
+
+	/* Absolutely everything */
+	LOG_ALL = ~(uint64)0
+};

The comment above LOG_MISC should be changed.

More fundamentally, this classification makes it easy to reuse LOGSTMT_*
(and a nice job you've done of that, with just a few additional special
cases), I don't think this level is quite enough for our needs. I think
it should at least be possible to specifically log commands that affect
privileges and roles.

I'm fond of finer categorisation for DDL as well, but I could live with
all DDL being lumped together.

I'm experimenting with a few approaches to do this without reintroducing
switch statements to test every command. That will require core changes,
but I think we can find an acceptable arrangement. I'll post a proof of
concept in a few days.

+ * Takes an AuditEvent and, if it log_check(), writes it to the audit
log.

I don't think log_check is the most useful name, because this sentence
doesn't tell me what the function may do. Similarly, I would prefer to
have log_acl_check be renamed acl_grants_audit or similar. (These are
all static functions anyway, I don't think a log_ prefix is needed.)

+ /* Free the column set */
+ bms_free(tmpSet);

(An aside, really: there are lots of comments like this, which I don't
think add anything to understanding the code, and should be removed.)

+		/*
+		 * We don't have access to the parsetree here, so we have to generate
+		 * the node type, object type, and command tag by decoding
+		 * rte->requiredPerms and rte->relkind.
+		 */
+		auditEvent.logStmtLevel = LOGSTMT_MOD;

(I am also trying to find a way to avoid having to do this.)

+		/* Set object type based on relkind */
+		switch (class->relkind)
+		{
+			case RELKIND_RELATION:
+				utilityAuditEvent.objectType = OBJECT_TYPE_TABLE;
+				break;

This occurs elsewhere too. But I suppose new relkinds are added less
frequently than new commands.

Again on a larger level, I'm not sure how I feel about _avoiding_ the
use of event triggers for audit logging. Regardless of whether we use
the deparse code (which I personally think is a good idea; Álvaro has
been working on it, and it looks very nice) to log extra information,
using the object access hook inevitably means we have to reimplement
the identification/classification code here.

In "old" pgaudit, I think that extra effort is justified by the need to
be backwards compatible with pre-event trigger releases. In a 9.5-only
version, I am not at all convinced that this makes sense.

Thoughts?

-- Abhijit

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

Thanks for the review, Abhijit.

On 3/23/15 1:31 AM, Abhijit Menon-Sen wrote:

At 2015-02-24 11:22:41 -0500, david@pgmasters.net wrote:

Patch v3 is attached.
+
+	/* Function execution */
+	LOG_MISC = (1 << 5),

The comment above LOG_MISC should be changed.

Fixed.

More fundamentally, this classification makes it easy to reuse LOGSTMT_*
(and a nice job you've done of that, with just a few additional special
cases), I don't think this level is quite enough for our needs. I think
it should at least be possible to specifically log commands that affect
privileges and roles.

I agree, but this turns out to be easier said than done. In the prior
code for instance, CREATE ROLE was classified as USER, while ALTER ROLE
.. RENAME was classified as DDL. This is because any rename gets the
command tag T_RenameStmt. CreateCommandTag does return "ALTER ROLE",
but now we're in the realm of string-matching again which is not my
favorite thing. Let me see if there is a clean way to get this
accomplished. I've also felt this is the one thing I'd like to see
broken out.

I'm fond of finer categorisation for DDL as well, but I could live with
all DDL being lumped together.

I'm experimenting with a few approaches to do this without reintroducing
switch statements to test every command. That will require core changes,
but I think we can find an acceptable arrangement. I'll post a proof of
concept in a few days.

I also think finer-grained categorization would be best accomplished
with some core changes. It seemed too late to get those in for 9.5 so I
decided to proceed with what I knew could be done reliably with the idea
to improve it with core changes going forward.

I look forward to your proof-of-concept.

+ * Takes an AuditEvent and, if it log_check(), writes it to the audit
log.

I don't think log_check is the most useful name, because this sentence
doesn't tell me what the function may do. Similarly, I would prefer to
have log_acl_check be renamed acl_grants_audit or similar. (These are
all static functions anyway, I don't think a log_ prefix is needed.)

log_check() has become somewhat vestigial at this point since it is only
called from one place - I've been considering removing it and merging
into log_audit_event(). For the moment I've improved the comments.

I like acl_grants_audit() and agree that it's a clearer name. I'll
incorporate that into the next version and apply the same scheme to the
other ACL functionsas well as do a general review of naming.

+ /* Free the column set */
+ bms_free(tmpSet);

(An aside, really: there are lots of comments like this, which I don't
think add anything to understanding the code, and should be removed.)

I generally feel like you can't have too many comments. I think even
the less interesting/helpful comments help break the code into
functional sections for readability.

+		/*
+		 * We don't have access to the parsetree here, so we have to generate
+		 * the node type, object type, and command tag by decoding
+		 * rte->requiredPerms and rte->relkind.
+		 */
+		auditEvent.logStmtLevel = LOGSTMT_MOD;

(I am also trying to find a way to avoid having to do this.)

That would be excellent.

+		/* Set object type based on relkind */
+		switch (class->relkind)
+		{
+			case RELKIND_RELATION:
+				utilityAuditEvent.objectType = OBJECT_TYPE_TABLE;
+				break;

This occurs elsewhere too. But I suppose new relkinds are added less
frequently than new commands.

Well, that's the hope at least. I should mention that ALL statements
will be logged no matter what additional classification happens. The
amount of information returned may not be ideal, but nothing is ever
excluded from logging (depending on the classes selected, of course).

Again on a larger level, I'm not sure how I feel about _avoiding_ the
use of event triggers for audit logging. Regardless of whether we use
the deparse code (which I personally think is a good idea; Álvaro has
been working on it, and it looks very nice) to log extra information,
using the object access hook inevitably means we have to reimplement
the identification/classification code here.

In "old" pgaudit, I think that extra effort is justified by the need to
be backwards compatible with pre-event trigger releases. In a 9.5-only
version, I am not at all convinced that this makes sense.

Thoughts?

I was nervous about basing pg_audit on code that I wasn't sure would be
committed (at the time). Since pg_event_trigger_get_creation_commands()
is tied up with deparse, I honestly didn't feel like the triggers were
bringing much to the table.

That being said, I agree that the deparse code is very useful and now
looks certain to be committed for 9.5.

I have prepared a patch that brings event triggers and deparse back to
pg_audit based on the Alvaro's dev/deparse branch at
git://git.postgresql.org/git/2ndquadrant_bdr.git (commit 0447fc5). I've
updated the unit tests accordingly.

I left in the OAT code for now. It does add detail to one event that
the event triggers do not handle (creating PK indexes) and I feel that
it lends an element of safety in case something happens to the event
triggers. OAT is also required for function calls so the code path
cannot be eliminated entirely. I'm not committed to keeping the
redundant OAT code, but I'd rather not remove it until deparse is
committed to master.

I've been hesitant to post this patch as it will not work in master
(though it will compile), but I don't want to hold on to it any longer
since the end of the CF is nominally just weeks away. If you want to
run the patch in master, you'll need to disable the
pg_audit_ddl_command_end trigger.

I've also addressed Fujii's concerns about logging parameters - this is
now an option. The event stack has been formalized and
MemoryContextRegisterResetCallback() is used to cleanup the stack on errors.

Let me know what you think.

--
- David Steele
david@pgmasters.net

On Tue, Mar 24, 2015 at 1:40 AM, David Steele <david@pgmasters.net> wrote:

Thanks for the review, Abhijit.

On 3/23/15 1:31 AM, Abhijit Menon-Sen wrote:

At 2015-02-24 11:22:41 -0500, david@pgmasters.net wrote:

Patch v3 is attached.
+
+    /* Function execution */
+    LOG_MISC = (1 << 5),

The comment above LOG_MISC should be changed.

Fixed.

More fundamentally, this classification makes it easy to reuse LOGSTMT_*
(and a nice job you've done of that, with just a few additional special
cases), I don't think this level is quite enough for our needs. I think
it should at least be possible to specifically log commands that affect
privileges and roles.

I agree, but this turns out to be easier said than done. In the prior
code for instance, CREATE ROLE was classified as USER, while ALTER ROLE
.. RENAME was classified as DDL. This is because any rename gets the
command tag T_RenameStmt. CreateCommandTag does return "ALTER ROLE",
but now we're in the realm of string-matching again which is not my
favorite thing. Let me see if there is a clean way to get this
accomplished. I've also felt this is the one thing I'd like to see
broken out.

I'm fond of finer categorisation for DDL as well, but I could live with
all DDL being lumped together.

I'm experimenting with a few approaches to do this without reintroducing
switch statements to test every command. That will require core changes,
but I think we can find an acceptable arrangement. I'll post a proof of
concept in a few days.

I also think finer-grained categorization would be best accomplished
with some core changes. It seemed too late to get those in for 9.5 so I
decided to proceed with what I knew could be done reliably with the idea
to improve it with core changes going forward.

I look forward to your proof-of-concept.

+ * Takes an AuditEvent and, if it log_check(), writes it to the audit
log.

I don't think log_check is the most useful name, because this sentence
doesn't tell me what the function may do. Similarly, I would prefer to
have log_acl_check be renamed acl_grants_audit or similar. (These are
all static functions anyway, I don't think a log_ prefix is needed.)

log_check() has become somewhat vestigial at this point since it is only
called from one place - I've been considering removing it and merging
into log_audit_event(). For the moment I've improved the comments.

I like acl_grants_audit() and agree that it's a clearer name. I'll
incorporate that into the next version and apply the same scheme to the
other ACL functionsas well as do a general review of naming.

+ /* Free the column set */
+ bms_free(tmpSet);

(An aside, really: there are lots of comments like this, which I don't
think add anything to understanding the code, and should be removed.)

I generally feel like you can't have too many comments. I think even
the less interesting/helpful comments help break the code into
functional sections for readability.

+            /*
+             * We don't have access to the parsetree here, so we have to generate
+             * the node type, object type, and command tag by decoding
+             * rte->requiredPerms and rte->relkind.
+             */
+            auditEvent.logStmtLevel = LOGSTMT_MOD;

(I am also trying to find a way to avoid having to do this.)

That would be excellent.

+            /* Set object type based on relkind */
+            switch (class->relkind)
+            {
+                    case RELKIND_RELATION:
+                            utilityAuditEvent.objectType = OBJECT_TYPE_TABLE;
+                            break;

This occurs elsewhere too. But I suppose new relkinds are added less
frequently than new commands.

Well, that's the hope at least. I should mention that ALL statements
will be logged no matter what additional classification happens. The
amount of information returned may not be ideal, but nothing is ever
excluded from logging (depending on the classes selected, of course).

Again on a larger level, I'm not sure how I feel about _avoiding_ the
use of event triggers for audit logging. Regardless of whether we use
the deparse code (which I personally think is a good idea; Álvaro has
been working on it, and it looks very nice) to log extra information,
using the object access hook inevitably means we have to reimplement
the identification/classification code here.

In "old" pgaudit, I think that extra effort is justified by the need to
be backwards compatible with pre-event trigger releases. In a 9.5-only
version, I am not at all convinced that this makes sense.

Thoughts?

I was nervous about basing pg_audit on code that I wasn't sure would be
committed (at the time). Since pg_event_trigger_get_creation_commands()
is tied up with deparse, I honestly didn't feel like the triggers were
bringing much to the table.

That being said, I agree that the deparse code is very useful and now
looks certain to be committed for 9.5.

I have prepared a patch that brings event triggers and deparse back to
pg_audit based on the Alvaro's dev/deparse branch at
git://git.postgresql.org/git/2ndquadrant_bdr.git (commit 0447fc5). I've
updated the unit tests accordingly.

I left in the OAT code for now. It does add detail to one event that
the event triggers do not handle (creating PK indexes) and I feel that
it lends an element of safety in case something happens to the event
triggers. OAT is also required for function calls so the code path
cannot be eliminated entirely. I'm not committed to keeping the
redundant OAT code, but I'd rather not remove it until deparse is
committed to master.

I've been hesitant to post this patch as it will not work in master
(though it will compile), but I don't want to hold on to it any longer
since the end of the CF is nominally just weeks away. If you want to
run the patch in master, you'll need to disable the
pg_audit_ddl_command_end trigger.

I've also addressed Fujii's concerns about logging parameters - this is
now an option. The event stack has been formalized and
MemoryContextRegisterResetCallback() is used to cleanup the stack on errors.

Let me know what you think.

Hi,

I tied to look into latest patch, but got following error.

masahiko [pg_audit] $ LANG=C make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o
pg_audit.o pg_audit.c
pg_audit.c: In function 'log_audit_event':
pg_audit.c:456: warning: ISO C90 forbids mixed declarations and code
pg_audit.c: In function 'pg_audit_ddl_command_end':
pg_audit.c:1436: error: 'pg_event_trigger_expand_command' undeclared
(first use in this function)
pg_audit.c:1436: error: (Each undeclared identifier is reported only once
pg_audit.c:1436: error: for each function it appears in.)
make: *** [pg_audit.o] Error 1

Am I missing something?

Regards,

-------
Sawada Masahiko

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

Sawada Masahiko wrote:

I tied to look into latest patch, but got following error.

masahiko [pg_audit] $ LANG=C make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o
pg_audit.o pg_audit.c
pg_audit.c: In function 'log_audit_event':
pg_audit.c:456: warning: ISO C90 forbids mixed declarations and code
pg_audit.c: In function 'pg_audit_ddl_command_end':
pg_audit.c:1436: error: 'pg_event_trigger_expand_command' undeclared
(first use in this function)

You need to apply my deparsing patch first, last version of which I
posted here:
/messages/by-id/20150316234406.GH3636@alvh.no-ip.org

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

On 3/23/15 1:39 PM, Sawada Masahiko wrote:

On Tue, Mar 24, 2015 at 1:40 AM, David Steele <david@pgmasters.net> wrote:

I have prepared a patch that brings event triggers and deparse back to
pg_audit based on the Alvaro's dev/deparse branch at
git://git.postgresql.org/git/2ndquadrant_bdr.git (commit 0447fc5). I've
updated the unit tests accordingly.

I've been hesitant to post this patch as it will not work in master
(though it will compile), but I don't want to hold on to it any longer
since the end of the CF is nominally just weeks away. If you want to
run the patch in master, you'll need to disable the
pg_audit_ddl_command_end trigger.

Hi,

I tied to look into latest patch, but got following error.

masahiko [pg_audit] $ LANG=C make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o
pg_audit.o pg_audit.c
pg_audit.c: In function 'log_audit_event':
pg_audit.c:456: warning: ISO C90 forbids mixed declarations and code
pg_audit.c: In function 'pg_audit_ddl_command_end':
pg_audit.c:1436: error: 'pg_event_trigger_expand_command' undeclared
(first use in this function)
pg_audit.c:1436: error: (Each undeclared identifier is reported only once
pg_audit.c:1436: error: for each function it appears in.)
make: *** [pg_audit.o] Error 1

Am I missing something?

It's my mistake. I indicated that this would compile under master - but
that turns out not to be true because of this function. It will only
compile cleanly in Alvaro's branch mentioned above.

My apologies - this is why I have been hesitant to post this patch
before. You are welcome to try with Alvaro's deparse branch or wait
until it has been committed to master.

I've attached patch v5 only to cleanup the warnings you saw.

--
- David Steele
david@pgmasters.net

On Tue, Mar 24, 2015 at 3:17 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Sawada Masahiko wrote:

I tied to look into latest patch, but got following error.

masahiko [pg_audit] $ LANG=C make
gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o
pg_audit.o pg_audit.c
pg_audit.c: In function 'log_audit_event':
pg_audit.c:456: warning: ISO C90 forbids mixed declarations and code
pg_audit.c: In function 'pg_audit_ddl_command_end':
pg_audit.c:1436: error: 'pg_event_trigger_expand_command' undeclared
(first use in this function)

You need to apply my deparsing patch first, last version of which I
posted here:
/messages/by-id/20150316234406.GH3636@alvh.no-ip.org

Thank you for the info.
I've applied these patchese successfully.

I looked into this module, and had a few comments as follows.
1. pg_audit audits only one role currently.
In currently code, we can not multiple user as auditing user. Why?
(Sorry if this topic already has been discussed.)

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

3. pg_audit logged OBJECT log even EXPLAIN command.
EXPLAIN command does not touch the table actually, but pg_audit writes
audit OBJECT log.
I'm not sure we need to log it. Is it intentional?

Regards,

-------
Sawada Masahiko

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

Hi Sawada,

Thank you for taking the time to look at the patch.

On 3/24/15 10:28 AM, Sawada Masahiko wrote:

I've applied these patchese successfully.

I looked into this module, and had a few comments as follows.
1. pg_audit audits only one role currently.
In currently code, we can not multiple user as auditing user. Why?
(Sorry if this topic already has been discussed.)

There is only one master audit role in a bid for simplicity. However,
there are two ways you can practically have multiple audit roles (both
are mentioned in the docs):

1) The audit role honors inheritance so you can grant all your audit
roles to the "master" role set in pg_audit.role and all the roles will
be audited.

2) Since pg_audit.role is a GUC, you can set a different audit role per
database by using ALTER DATABASE ... SET. You can set the GUC per logon
role as well though that would probably make things very complicated.
The GUC is SUSET so normal users cannot tamper with it.

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

Yes, object auditing does not work until some grants have been made to
the audit role.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

Looks right to me. If you don't want the session logging then disable
pg_audit.log.

Session and object logging are completely independent from each other:
one or the other, or both, or neither can be enabled at any time.

3. pg_audit logged OBJECT log even EXPLAIN command.
EXPLAIN command does not touch the table actually, but pg_audit writes
audit OBJECT log.
I'm not sure we need to log it. Is it intentional?

This is intentional. They are treated as queries since in production
they should be relatively rare (that is, not part of a normal function
or process) and it's good information to have because EXPLAIN can be
used to determine the number of rows in a table, and could also be used
to figure out when data is added or removed from a table. In essence,
it is a query even if it does not return row data.

If that sounds paranoid, well, auditing is all about paranoia!

--
- David Steele
david@pgmasters.net

Hi David,

Thank you for your answer!

On Wed, Mar 25, 2015 at 12:38 AM, David Steele <david@pgmasters.net> wrote:

Hi Sawada,

Thank you for taking the time to look at the patch.

On 3/24/15 10:28 AM, Sawada Masahiko wrote:

I've applied these patchese successfully.

I looked into this module, and had a few comments as follows.
1. pg_audit audits only one role currently.
In currently code, we can not multiple user as auditing user. Why?
(Sorry if this topic already has been discussed.)

There is only one master audit role in a bid for simplicity. However,
there are two ways you can practically have multiple audit roles (both
are mentioned in the docs):

1) The audit role honors inheritance so you can grant all your audit
roles to the "master" role set in pg_audit.role and all the roles will
be audited.

2) Since pg_audit.role is a GUC, you can set a different audit role per
database by using ALTER DATABASE ... SET. You can set the GUC per logon
role as well though that would probably make things very complicated.
The GUC is SUSET so normal users cannot tamper with it.

I understood.

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

Yes, object auditing does not work until some grants have been made to
the audit role.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

Looks right to me. If you don't want the session logging then disable
pg_audit.log.

Session and object logging are completely independent from each other:
one or the other, or both, or neither can be enabled at any time.

It means that OBJECT log is not logged just after creating table, even
if that table is touched by its owner.
To write OBJECT log, we need to grant privilege to role at least. right?

3. pg_audit logged OBJECT log even EXPLAIN command.
EXPLAIN command does not touch the table actually, but pg_audit writes
audit OBJECT log.
I'm not sure we need to log it. Is it intentional?

This is intentional. They are treated as queries since in production
they should be relatively rare (that is, not part of a normal function
or process) and it's good information to have because EXPLAIN can be
used to determine the number of rows in a table, and could also be used
to figure out when data is added or removed from a table. In essence,
it is a query even if it does not return row data.

Okay, I understood.

Regards,

-------
Sawada Masahiko

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

On Wed, Mar 25, 2015 at 12:38 AM, David Steele <david@pgmasters.net> wrote:

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

Yes, object auditing does not work until some grants have been made to
the audit role.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

Looks right to me. If you don't want the session logging then disable
pg_audit.log.

Session and object logging are completely independent from each other:
one or the other, or both, or neither can be enabled at any time.

It means that OBJECT log is not logged just after creating table, even
if that table is touched by its owner.
To write OBJECT log, we need to grant privilege to role at least. right?

Exactly. Privileges must be granted to the audit role in order for
object auditing to work.

--
- David Steele
david@pgmasters.net

On Wed, Mar 25, 2015 at 12:23 PM, David Steele <david@pgmasters.net> wrote:

On Wed, Mar 25, 2015 at 12:38 AM, David Steele <david@pgmasters.net> wrote:

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

Yes, object auditing does not work until some grants have been made to
the audit role.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

Looks right to me. If you don't want the session logging then disable
pg_audit.log.

Session and object logging are completely independent from each other:
one or the other, or both, or neither can be enabled at any time.

It means that OBJECT log is not logged just after creating table, even
if that table is touched by its owner.
To write OBJECT log, we need to grant privilege to role at least. right?

Exactly. Privileges must be granted to the audit role in order for
object auditing to work.

The table owner always can touch its table.
So does it lead that table owner can get its table information while
hiding OBJECT logging?

Also I looked into latest patch again.
Here are two review comment.

1.

typedef struct
{
int64 statementId;
int64 substatementId;

Both statementId and substatementId could be negative number.
I think these should be uint64 instead.

2.
I got ERROR when executing function uses cursor.

1) create empty table (hoge table)
2) create test function as follows.

create function test() returns int as $$
declare
cur1 cursor for select * from hoge;
tmp int;
begin
open cur1;
fetch cur1 into tmp;
return tmp;
end$$
language plpgsql ;

3) execute test function (got ERROR)
=# select test();
LOG: AUDIT: SESSION,6,1,READ,SELECT,,,selecT test();
LOG: AUDIT: SESSION,6,2,FUNCTION,EXECUTE,FUNCTION,public.test,selecT test();
LOG: AUDIT: SESSION,6,3,READ,SELECT,,,select * from hoge
CONTEXT: PL/pgSQL function test() line 6 at OPEN
ERROR: pg_audit stack is already empty
STATEMENT: selecT test();

It seems like that the item in stack is already freed by deleting
pg_audit memory context (in MemoryContextDelete()),
before calling stack_pop in dropping of top-level Portal.

Regards,

-------
Sawada Masahiko

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

On 3/25/15 7:46 AM, Sawada Masahiko wrote:

On Wed, Mar 25, 2015 at 12:23 PM, David Steele <david@pgmasters.net> wrote:

On Wed, Mar 25, 2015 at 12:38 AM, David Steele <david@pgmasters.net> wrote:

2. OBJECT auditing does not work before adding acl info to pg_class.rel_acl.
In following situation, pg_audit can not audit OBJECT log.
$ cat postgresql.conf | grep audit
shared_preload_libraries = 'pg_audit'
pg_audit.role = 'hoge_user'
pg_audit.log = 'read, write'
$ psql -d postgres -U hoge_user
=# create table hoge(col int);
=# select * from hoge;
LOG: AUDIT: SESSION,3,1,READ,SELECT,,,select * from hoge;

OBJECT audit log is not logged here since pg_class.rel_acl is empty
yet. (Only logged SESSION log)
So after creating another unconcerned role and grant any privilege to that user,
OBJECT audit is logged successfully.

Yes, object auditing does not work until some grants have been made to
the audit role.

=# create role bar_user;
=# grant select on hoge to bar_user;
=# select * from hoge;
LOG: AUDIT: SESSION,4,1,READ,SELECT,,,select * from hoge;
LOG: AUDIT: OBJECT,4,1,READ,SELECT,TABLE,public.hoge,select * from hoge;

The both OBJCET and SESSION log are logged.

Looks right to me. If you don't want the session logging then disable
pg_audit.log.

Session and object logging are completely independent from each other:
one or the other, or both, or neither can be enabled at any time.

It means that OBJECT log is not logged just after creating table, even
if that table is touched by its owner.
To write OBJECT log, we need to grant privilege to role at least. right?

Exactly. Privileges must be granted to the audit role in order for
object auditing to work.

The table owner always can touch its table.
So does it lead that table owner can get its table information while
hiding OBJECT logging?

Yes, the table owner would be able to access the table without object
logging if grants to that table were not made to the audit role. That
would also be true for any other user that had grants on the table.

The purpose of object auditing is to allow more fine-grained control and
is intended to be used in situations where you only want to audit some
things, rather than all things. Logging everything is better done with
the session logging.

However, object logging does yield more information since it lists every
table that was touched by the statement, so there may be cases where
you'd like to object log everything. In that case I'd recommend writing
a bit of plpgsql code to create the grants.

Also I looked into latest patch again.
Here are two review comment.

1.

typedef struct
{
int64 statementId;
int64 substatementId;

Both statementId and substatementId could be negative number.
I think these should be uint64 instead.

True. I did this because printf formatting for uint64 seems to be vary
across platforms. int64 formatting is more standard and still gives
more than enough IDs.

I could change it back to uint64 if you have a portable way to modify
the sprintf at line 507.

2.
I got ERROR when executing function uses cursor.

1) create empty table (hoge table)
2) create test function as follows.

create function test() returns int as $$
declare
cur1 cursor for select * from hoge;
tmp int;
begin
open cur1;
fetch cur1 into tmp;
return tmp;
end$$
language plpgsql ;

3) execute test function (got ERROR)
=# select test();
LOG: AUDIT: SESSION,6,1,READ,SELECT,,,selecT test();
LOG: AUDIT: SESSION,6,2,FUNCTION,EXECUTE,FUNCTION,public.test,selecT test();
LOG: AUDIT: SESSION,6,3,READ,SELECT,,,select * from hoge
CONTEXT: PL/pgSQL function test() line 6 at OPEN
ERROR: pg_audit stack is already empty
STATEMENT: selecT test();

It seems like that the item in stack is already freed by deleting
pg_audit memory context (in MemoryContextDelete()),
before calling stack_pop in dropping of top-level Portal.

Good catch, I'll add this to my test cases and work on a fix. I think I
see a good way to approach it.

--
- David Steele
david@pgmasters.net

Hi Sawada,

On 3/25/15 9:24 AM, David Steele wrote:

On 3/25/15 7:46 AM, Sawada Masahiko wrote:

2.
I got ERROR when executing function uses cursor.

1) create empty table (hoge table)
2) create test function as follows.

create function test() returns int as $$
declare
cur1 cursor for select * from hoge;
tmp int;
begin
open cur1;
fetch cur1 into tmp;
return tmp;
end$$
language plpgsql ;

3) execute test function (got ERROR)
=# select test();
LOG: AUDIT: SESSION,6,1,READ,SELECT,,,selecT test();
LOG: AUDIT: SESSION,6,2,FUNCTION,EXECUTE,FUNCTION,public.test,selecT test();
LOG: AUDIT: SESSION,6,3,READ,SELECT,,,select * from hoge
CONTEXT: PL/pgSQL function test() line 6 at OPEN
ERROR: pg_audit stack is already empty
STATEMENT: selecT test();

It seems like that the item in stack is already freed by deleting
pg_audit memory context (in MemoryContextDelete()),
before calling stack_pop in dropping of top-level Portal.

This has been fixed and I have attached a new patch. I've seen this
with cursors before where the parent MemoryContext is freed before
control is returned to ProcessUtility. I think that's strange behavior
but there's not a lot I can do about it.

The code I put in to deal with this situation was not quite robust
enough so I had to harden it a bit more.

Let me know if you see any other issues.

Thanks,
--
- David Steele
david@pgmasters.net

On 3/23/15 12:40 PM, David Steele wrote:

On 3/23/15 1:31 AM, Abhijit Menon-Sen wrote:

I'm experimenting with a few approaches to do this without reintroducing
switch statements to test every command. That will require core changes,
but I think we can find an acceptable arrangement. I'll post a proof of
concept in a few days.

Any progress on the POC? I'm interested in trying to get the ROLE class
back in before the Commitfest winds up, but not very happy with my
current string-matching options.

+ * Takes an AuditEvent and, if it log_check(), writes it to the audit
log.

I don't think log_check is the most useful name, because this sentence
doesn't tell me what the function may do. Similarly, I would prefer to
have log_acl_check be renamed acl_grants_audit or similar. (These are
all static functions anyway, I don't think a log_ prefix is needed.)

log_check() has become somewhat vestigial at this point since it is only
called from one place - I've been considering removing it and merging
into log_audit_event(). For the moment I've improved the comments.

log_check() got rolled into log_audit_event().

I like acl_grants_audit() and agree that it's a clearer name. I'll
incorporate that into the next version and apply the same scheme to the
other ACL functionsas well as do a general review of naming.

I ended up going with audit_on_acl, audit_on_relation, etc. and reworked
some of the other function names.

I attached the v6 patch to my previous email, or you can find it on the
CF page.

--
- David Steele
david@pgmasters.net

On Thu, Apr 2, 2015 at 2:46 AM, David Steele <david@pgmasters.net> wrote:

Hi Sawada,

On 3/25/15 9:24 AM, David Steele wrote:

On 3/25/15 7:46 AM, Sawada Masahiko wrote:

2.
I got ERROR when executing function uses cursor.

1) create empty table (hoge table)
2) create test function as follows.

create function test() returns int as $$
declare
cur1 cursor for select * from hoge;
tmp int;
begin
open cur1;
fetch cur1 into tmp;
return tmp;
end$$
language plpgsql ;

3) execute test function (got ERROR)
=# select test();
LOG: AUDIT: SESSION,6,1,READ,SELECT,,,selecT test();
LOG: AUDIT: SESSION,6,2,FUNCTION,EXECUTE,FUNCTION,public.test,selecT test();
LOG: AUDIT: SESSION,6,3,READ,SELECT,,,select * from hoge
CONTEXT: PL/pgSQL function test() line 6 at OPEN
ERROR: pg_audit stack is already empty
STATEMENT: selecT test();

It seems like that the item in stack is already freed by deleting
pg_audit memory context (in MemoryContextDelete()),
before calling stack_pop in dropping of top-level Portal.

This has been fixed and I have attached a new patch. I've seen this
with cursors before where the parent MemoryContext is freed before
control is returned to ProcessUtility. I think that's strange behavior
but there's not a lot I can do about it.

Thank you for updating the patch!

The code I put in to deal with this situation was not quite robust
enough so I had to harden it a bit more.

Let me know if you see any other issues.

I pulled HEAD, and then tried to compile source code after applied
following "deparsing utility command patch" without #0001 and #0002.
(because these two patches are already pushed)
</messages/by-id/20150325175954.GL3636@alvh.no-ip.org&gt;

But I could not use new pg_audit patch due to compile error (that
deparsing utility command patch might have).
I think I'm missing something, but I'm not sure.
Could you tell me which patch should I apply before compiling pg_audit?

Regards,

-------
Sawada Masahiko

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