Обсуждение: Auditing extension for PostgreSQL (Take 2)
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. 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. Auditing has been a concern everywhere I've used or introduced PostgreSQL. Over time I've developed a reasonably comprehensive (but complex) system of triggers, tables and functions to provide auditing for customers. While this has addressed most requirements, there are always questions of auditing aborted transactions, DDL changes, configurability, etc. which I have been unable to satisfy. There has been some discussion of whether or not this module needs to be in contrib. One reason customers trust contrib is that the modules are assumed to be held to a higher standard than code found on GitHub. This has already been pointed out. But I believe another important reason is that they know packages will be made available for a variety of platforms, and bugs are more likely to be experienced by many users and not just a few (or one). For this reason my policy is not to run custom-compiled code in production. This is especially true for something as mission critical as a relational database. 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. 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. 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. -- - David Steele david@pgmasters.net
Вложения
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
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
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
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
Вложения
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: https://www.postgresql.org/message-id/20150316234406.GH3636@alvh.no-ip.org -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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: > https://www.postgresql.org/message-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
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
> 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
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) <http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org> 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
On 4/3/15 3:59 AM, Sawada Masahiko wrote: > On Thu, Apr 2, 2015 at 2:46 AM, David Steele <david@pgmasters.net> wrote: >> 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) > <http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org> > > 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? When Alvaro posted his last patch set he recommended applying them to b3196e65: http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org Applying the 3/25 deparse patches onto b3196e65 (you'll see one trailing space error) then applying pg_audit-v6.patch works fine. -- - David Steele david@pgmasters.net
On Fri, Apr 3, 2015 at 10:01 PM, David Steele <david@pgmasters.net> wrote: > On 4/3/15 3:59 AM, Sawada Masahiko wrote: >> On Thu, Apr 2, 2015 at 2:46 AM, David Steele <david@pgmasters.net> wrote: >>> 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) >> <http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org> >> >> 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? > > When Alvaro posted his last patch set he recommended applying them to > b3196e65: > > http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org > > Applying the 3/25 deparse patches onto b3196e65 (you'll see one trailing > space error) then applying pg_audit-v6.patch works fine. > Thank you for your advice! I'm testing pg_audit, so I will send report to you as soon as possible. Regards, ------- Sawada Masahiko
On Fri, Apr 3, 2015 at 10:01 PM, David Steele <david@pgmasters.net> wrote: > On 4/3/15 3:59 AM, Sawada Masahiko wrote: >> On Thu, Apr 2, 2015 at 2:46 AM, David Steele <david@pgmasters.net> wrote: >>> 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) >> <http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org> >> >> 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? > > When Alvaro posted his last patch set he recommended applying them to > b3196e65: > > http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org > > Applying the 3/25 deparse patches onto b3196e65 (you'll see one trailing > space error) then applying pg_audit-v6.patch works fine. > I tested v6 patch, but I got SEGV when I executed test() function I mentioned up thread. Could you address this problem? Regards, ------- Sawada Masahiko
On 4/6/15 8:40 AM, Sawada Masahiko wrote: > On Fri, Apr 3, 2015 at 10:01 PM, David Steele <david@pgmasters.net> wrote: >> On 4/3/15 3:59 AM, Sawada Masahiko wrote: >>> On Thu, Apr 2, 2015 at 2:46 AM, David Steele <david@pgmasters.net> wrote: >>>> 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) >>> <http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org> >>> >>> 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? >> >> When Alvaro posted his last patch set he recommended applying them to >> b3196e65: >> >> http://www.postgresql.org/message-id/20150325175954.GL3636@alvh.no-ip.org >> >> Applying the 3/25 deparse patches onto b3196e65 (you'll see one trailing >> space error) then applying pg_audit-v6.patch works fine. >> > > I tested v6 patch, but I got SEGV when I executed test() function I > mentioned up thread. > Could you address this problem? Unfortunately I'm not able to reproduce the issue. Here's what I tried based on your original test: postgres=# create table hoge (id int); CREATE TABLE postgres=# create function test() returns int as $$ postgres$# declare postgres$# cur1 cursor for select * from hoge; postgres$# tmp int; postgres$# begin postgres$# open cur1; postgres$# fetch cur1 into tmp; postgres$# return tmp; postgres$# end$$ postgres-# language plpgsql ; CREATE FUNCTION postgres=# select test();test ------ (1 row) postgres=# insert into hoge values (1); INSERT 0 1 postgres=# select test();test ------ 1 (1 row) And the log output was: prefix LOG: statement: create table hoge (id int); prefix DEBUG: EventTriggerInvoke 16385 prefix LOG: AUDIT: SESSION,3,1,DDL,CREATE TABLE,TABLE,public.hoge,CREATE TABLE public.hoge (id pg_catalog.int4) WITH (oids=OFF) prefix LOG: statement: 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 ; prefix DEBUG: EventTriggerInvoke 16385 prefix LOG: AUDIT: SESSION,4,1,DDL,CREATE FUNCTION,FUNCTION,public.test(),"CREATE FUNCTION public.test() RETURNSpg_catalog.int4 LANGUAGE plpgsql VOLATILE CALLEDON NULL INPUT SECURITY INVOKER COST 100 AS 'declare cur1 cursor for select * from hoge; tmp int;begin open cur1; fetch cur1into tmp; return tmp;end'" prefix LOG: statement: select test(); prefix LOG: AUDIT: SESSION,5,1,READ,SELECT,,,select test(); prefix LOG: AUDIT: SESSION,5,2,FUNCTION,EXECUTE,FUNCTION,public.test,select test(); prefix LOG: AUDIT: SESSION,5,3,READ,SELECT,,,select * from hoge prefix CONTEXT: PL/pgSQL function test() line 6 at OPEN prefix LOG: statement: insert into hoge values (1); prefix LOG: AUDIT: SESSION,6,1,WRITE,INSERT,,,insert into hoge values (1); prefix LOG: statement: select test(); prefix LOG: AUDIT: SESSION,7,1,READ,SELECT,,,select test(); prefix LOG: AUDIT: SESSION,7,2,FUNCTION,EXECUTE,FUNCTION,public.test,select test(); prefix LOG: AUDIT: SESSION,7,3,READ,SELECT,,,select * from hoge prefix CONTEXT: PL/pgSQL function test() line 6 at OPEN Does the above look like what you did? Any other information about your environment would also be helpful. I'm thinking it might be some kind of build issue. -- - David Steele david@pgmasters.net
On 2/14/15 9:34 PM, David Steele wrote: > The patch I've attached satisfies the requirements that I've had from > customers in the past. What I'm missing is a more precise description/documentation of what those requirements might be. "Audit" is a "big word". It might imply regulatory or standards compliance on some level. We already have ways to log everything. If customers want "auditing" instead, they will hopefully have a precise requirements set, and we need a way to map that to a system configuration. I don't think "we need auditing" -> "oh there's this pg_audit thing, and it has a bunch of settings you can play with" is going to be enough of a workflow. For starters, I would consider the textual server log to be potentially lossy in many circumstances, so there would need to be additional information on how to configure that to be robust. (Also, more accurately, this is an "audit trail", not an "audit". An audit is an examination of a system, not a record of interactions with a system. An audit trail might be useful for an audit.) I see value in what you call object auditing, which is something you can't easily do at the moment. But what you call session auditing seems hardly distinct from statement logging. If we enhance log_statements a little bit, there will not be a need for an extra module to do almost the same thing.
On 6 April 2015 at 16:34, Peter Eisentraut <peter_e@gmx.net> wrote: > On 2/14/15 9:34 PM, David Steele wrote: >> The patch I've attached satisfies the requirements that I've had from >> customers in the past. > > What I'm missing is a more precise description/documentation of what > those requirements might be. > > "Audit" is a "big word". It might imply regulatory or standards > compliance on some level. We already have ways to log everything. If > customers want "auditing" instead, they will hopefully have a precise > requirements set, and we need a way to map that to a system > configuration. I don't think "we need auditing" -> "oh there's this > pg_audit thing, and it has a bunch of settings you can play with" is > going to be enough of a workflow. Yes, this needs better documentation, as does RLS. > For starters, I would consider the > textual server log to be potentially lossy in many circumstances, so > there would need to be additional information on how to configure that > to be robust. It was intended to be used with a log filter plugin, to allow it to be routed wherever is considered safe. > (Also, more accurately, this is an "audit trail", not an "audit". An > audit is an examination of a system, not a record of interactions with a > system. An audit trail might be useful for an audit.) No problem with calling it pg_audit_trail > I see value in what you call object auditing, which is something you > can't easily do at the moment. But what you call session auditing seems > hardly distinct from statement logging. If we enhance log_statements a > little bit, there will not be a need for an extra module to do almost > the same thing. Agreed: generating one line per statement isn't much different from log_statements. The earlier version of pg_audit generated different output. Specifically, it allowed you to generate output for each object tracked; one line per object. The present version can trigger an audit trail event for a statement, without tracking the object that was being audited. This prevents you from searching for "all SQL that touches table X", i.e. we know the statements were generated, but not which ones they were. IMHO that makes the resulting audit trail unusable for auditing purposes. I would like to see that functionality put back before it gets committed, if that occurs. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services
Simon Riggs wrote: > The present version can trigger an audit trail event for a statement, > without tracking the object that was being audited. This prevents you > from searching for "all SQL that touches table X", i.e. we know the > statements were generated, but not which ones they were. IMHO that > makes the resulting audit trail unusable for auditing purposes. I > would like to see that functionality put back before it gets > committed, if that occurs. Is there a consensus that the current version is the one that we should be reviewing, rather than the one Abhijit submitted? Last I checked, that wasn't at all clear. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
On 4/6/15 4:34 PM, Peter Eisentraut wrote:
> On 2/14/15 9:34 PM, David Steele wrote:
>> The patch I've attached satisfies the requirements that I've had
>> from customers in the past.
>
> What I'm missing is a more precise description/documentation of
> what those requirements might be.
Admittedly I'm not a financial or ISO certification auditor, but I've
been in the position of providing data to auditors on many of
occasions. The requests generally fall into three categories:
1) Data requests. Perhaps all the CDRs for a particular customer for
a particular month. Bulk data requests are not addressed by pg_audit.
2) DDL log. A list of all DDL changes made to the database. For
instance, the last time a function was updated and who did it. The
auditor would like to be sure that the function update timestamp
matches up with the last maintenance window and the person who is on
record as having done the updates.
3) DML log. This can be done with triggers, but requires quite a bit
of work and vigilance.
> "Audit" is a "big word". It might imply regulatory or standards
> compliance on some level. We already have ways to log everything.
> If customers want "auditing" instead, they will hopefully have a
> precise requirements set, and we need a way to map that to a
> system configuration. I don't think "we need auditing" -> "oh
> there's this pg_audit thing, and it has a bunch of settings you can
> play with" is going to be enough of a workflow. For starters, I
> would consider the textual server log to be potentially lossy in
> many circumstances, so there would need to be additional
> information on how to configure that to be robust.
Nothing is perfect, but there's a big difference between being able to
log everything and being able to use the data you logged to satisfy an
audit. Auditors tend to be reasonably tech savvy but there are
limits. An example of how pg_audit can provide better logging is at
the end of this email.
I agree that server logs are potentially lossy but that really
describes anywhere audit records might be written. Agreed that there
are better ways to do it (like writing back to the DB, or a remote
DB), but I thought those methods should be saved for a future version.
In my past experience having retention policies in place and being
able to show that they normally work are enough to satisfy an auditor.Accidents happen and that's understood - as long
asan explanation
for the failure is given. Such as, "We lost a backup tape, here's the
ticket for the incident and the name of the employee who handled the
case so you can follow up." Or, "On this date we had a disk failure
and lost the logs before the could be shipped, here's the ticket, etc."
> (Also, more accurately, this is an "audit trail", not an "audit".
> An audit is an examination of a system, not a record of
> interactions with a system. An audit trail might be useful for an
> audit.)
You are correct and I'd be happy to call it pg_audit_trail (as Simon
suggested) or pg_audit_log or something that's more descriptive.
> I see value in what you call object auditing, which is something
> you can't easily do at the moment. But what you call session
> auditing seems hardly distinct from statement logging. If we
> enhance log_statements a little bit, there will not be a need for
> an extra module to do almost the same thing.
Even with session auditing you can have multiple log entries per
backend call. This is particularly true for DO blocks and functions
calls.
Here's a relatively simple example, but it shows how complex this
could get. Let's say we have a DO block with dynamic SQL:
do $$
declare table_name text = 'do_table';
begin execute 'create table ' || table_name || ' ("weird name" int)'; execute 'drop table ' || table_name;
end; $$
Setting log_statement=all will certain work but you only get the DO
block logged:
LOG: statement: do $$
declare table_name text = 'do_table';
begin execute 'create table ' || table_name || ' ("weird name" int)'; execute 'drop table ' || table_name;
end; $$
With pg_audit you get (forgive the LFs that email added) much more:
LOG: AUDIT: SESSION,38,1,FUNCTION,DO,,,"do $$
declare table_name text = 'do_table';
begin execute 'create table ' || table_name || ' (""weird name"" int)'; execute 'drop table ' || table_name;
end; $$"
LOG: AUDIT: SESSION,38,2,DDL,CREATE
TABLE,TABLE,public.do_table,"CREATE TABLE public.do_table (""weird
name"" pg_catalog.int4 ) WITH (oids=OFF) "
LOG: AUDIT: SESSION,38,3,DDL,DROP TABLE,TABLE,public.do_table,drop
table do_table
Not only is the DO block logged but each sub statement is logged as
well. They are logically grouped by the statement ID (in this case
38) so it's clear they were run as a single command. The commands
(DO, DROP TABLE, CREATE TABLE) and fully-qualified object names are
provided and the statements are quoted and escaped when needed to
making parsing easier.
There's no question that this sort of thing could be done with
log_statements, but I would argue it's more than a little enhancement.
- --
- - David Steele
david@pgmasters.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBCAAGBQJVIyOyAAoJEIA2uIJQ5SFAOQEP/jcEsrAqxxGMn/Px6YSjzJCq
vKBGkilxNHbHn8GeAD617LPHl+4WjgmSWPA4OC2qbCa36tib0mBTRVpdaA1J9PTU
+Ml9kk9hHGdXTkoK2DlMFwVwJ4mZCvKXU4TOOYjdG6YkQaEoCdpEQ8n0Z0bxYogb
zBZ6GnxdkMzD8w33LByW9tf/ShWZsDKh47vqIhk1oGvQULlTGZ7CvAq793vWOCng
9+SBsct8BCUNRS0i1JcWjoLin9rJUNXLkyufIylKuAjbacBDIvQfRmKJJYTQA8lg
7K0Hy5gp7JNWTN+J6TQHM930FFFetVzXXaLRaJwZls9hqzPDSpXA2LleEQy9jzlf
CvSQgoAx/kkBEOjkKBAEL4PYcWXWhizysXkVAURwZ3huvm5wi8C2mVFilFz9oiZa
Z7L0FClFcBhX3ZuptDJOXF4WFdwE7TQDy3Go8aA/UY5gqe08Hqx/Atw881kBEC3j
uQIgdWY0WGVvT43igX44mUv3Q0aTNWHn/jTgaRURwPlpP+wViK3VIybIqiKtebq1
Iaqduge0pirDQMTDdFxt+F5C+ylK+R9TU9xPv8eQwrbq8o3ZIuoiLhtuzl8yQWMI
tiJJCfay4gkm+xZIjsFe9aj3Q0Xk4VjAt8MF9OunaNFTI4X5ZH3OSkZvmbbMjd1S
i6u19Khnj9ryje2nGNFS
=1jDh
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 4/6/15 4:47 PM, Simon Riggs wrote: > On 6 April 2015 at 16:34, Peter Eisentraut <peter_e@gmx.net> > wrote: >> "Audit" is a "big word". It might imply regulatory or standards >> compliance on some level. We already have ways to log >> everything. If customers want "auditing" instead, they will >> hopefully have a precise requirements set, and we need a way to >> map that to a system configuration. I don't think "we need >> auditing" -> "oh there's this pg_audit thing, and it has a bunch >> of settings you can play with" is going to be enough of a >> workflow. > > Yes, this needs better documentation, as does RLS. Discussions like these definitely help when it comes to knowing what to put in the documentation. The "what" is hard enough, the "why" gets into some scary territory. Still, audit requirements vary wildly and I'm not sure how much justice I could do to the topic in the contrib docs. I think more discussion of what's technically possible might be more fruitful. >> For starters, I would consider the textual server log to be >> potentially lossy in many circumstances, so there would need to >> be additional information on how to configure that to be robust. > > It was intended to be used with a log filter plugin, to allow it to > be routed wherever is considered safe. That would certainly work. >> (Also, more accurately, this is an "audit trail", not an "audit". >> An audit is an examination of a system, not a record of >> interactions with a system. An audit trail might be useful for >> an audit.) > > No problem with calling it pg_audit_trail Nor I. >> I see value in what you call object auditing, which is something >> you can't easily do at the moment. But what you call session >> auditing seems hardly distinct from statement logging. If we >> enhance log_statements a little bit, there will not be a need for >> an extra module to do almost the same thing. > > Agreed: generating one line per statement isn't much different > from log_statements. > > The earlier version of pg_audit generated different output. > Specifically, it allowed you to generate output for each object > tracked; one line per object. That is still doable, but is covered by object-level auditing. Even so, multiple log entries are possible (and even likely) with session auditing. See my response to Peter for details. > The present version can trigger an audit trail event for a > statement, without tracking the object that was being audited. This > prevents you from searching for "all SQL that touches table X", > i.e. we know the statements were generated, but not which ones they > were. IMHO that makes the resulting audit trail unusable for > auditing purposes. I would like to see that functionality put back > before it gets committed, if that occurs. Bringing this back would be easy (it actually requires removing, not adding code) but I'd prefer to make it configurable. - -- - - David Steele david@pgmasters.net -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVIybuAAoJEIA2uIJQ5SFAm/8P/2gS2oSvxF2VjP3WBJjH0d8p QHlni3SDBIlJPPE1ZnNYbtUANWKSw2Ublpk50223TeejEnNZfORtA7TZ9qic+3Ei 83yK4SzQcSMA1xqMvGTDS621l4/Nkw/uWKO8BDGePTHRjsEPpgMxJxsHVfNddd5Z MTP26vXPgyzj1H1GE4jPCi1kR6iiKx3GiagawmNJNgzdOXf25hQijpQ7mR0puw/T V75MeNr0WNi4CtsyDgNnx0oVKBN4olG6aId6+q3jt+yuxboJ53Nq59xbfvxYUR+3 uPKX9jfwInZxQc+70g2CcKj+EglB9cDn4oaMUkAxqYWKWyRW0O2gs0IIkbQqk8qK SlfBvAaZA1wfDelCztr8GHc8hLIh+hwb3mJq4zoclPg3+36hUgVyVIyRUjzW42sJ shvd2KWkxP4iwN1+tru9YK3qZ1GXkZfodtXdJ7iY14k5eXTKBuRgHFO8BRXxW9xp /KwIgkLD9gEjht6cncgP83lBoaxMFjrQE9N3hzX1wMM5ZYDAisbKK7JkGE2+yCsH L/aiCOxyHbxaMZATopATbCBhULDMLKl9oICKY+jv17yeyGG5F5D78AWv0tuvk1jW 5enydtXPhcBIXRWIvTZgCfDpFs5Hv5r/+V70tiMQbJIzg2qvxHmC0VLEubxky0XE TGfavKbTvK9dmw1dhzk5 =5KkP -----END PGP SIGNATURE-----
On 6 April 2015 at 20:38, David Steele <david@pgmasters.net> wrote: >> The earlier version of pg_audit generated different output. >> Specifically, it allowed you to generate output for each object >> tracked; one line per object. That discussion covers recursive SQL. That is important too, but not what I am saying. My point is what we log when an SQL statement covers multiple tables, e.g. join SELECTs, or inheritance cases, views. > That is still doable, but is covered by object-level auditing. Even > so, multiple log entries are possible (and even likely) with session > auditing. See my response to Peter for details. > >> The present version can trigger an audit trail event for a >> statement, without tracking the object that was being audited. This >> prevents you from searching for "all SQL that touches table X", >> i.e. we know the statements were generated, but not which ones they >> were. IMHO that makes the resulting audit trail unusable for >> auditing purposes. I would like to see that functionality put back >> before it gets committed, if that occurs. > > Bringing this back would be easy (it actually requires removing, not > adding code) but I'd prefer to make it configurable. That is my preference also. My concern was raised when it was *removed* without confirming others agreed. Typical questions: Who has written to table X? Who has read data from table Y yesterday between time1 and time2? Has anyone accessed a table directly, rather than through a security view? -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services
On 4/6/15 5:03 PM, Alvaro Herrera wrote: > Simon Riggs wrote: > >> The present version can trigger an audit trail event for a statement, >> without tracking the object that was being audited. This prevents you >> from searching for "all SQL that touches table X", i.e. we know the >> statements were generated, but not which ones they were. IMHO that >> makes the resulting audit trail unusable for auditing purposes. I >> would like to see that functionality put back before it gets >> committed, if that occurs. > > Is there a consensus that the current version is the one that we should > be reviewing, rather than the one Abhijit submitted? Last I checked, > that wasn't at all clear. Well, this one is the commitfest thread of record. At quick glance, my comments about "how does this map to specific customer requirements" apply to the other submission as well.
Attached is the v7 pg_audit patch. I've tried to address Peter's documentation concerns by cleaning up the terminology and adding a real-world case plus usage recommendations. The word "auditing" has been expunged from the docs in favor of the term "audit logging". Per Simon's request, there is now a pg_audit.log_relation setting that makes session audit logging exhaustively log all relations as it did before. The ROLE logging class is back as well. Simon also suggested a way that pg_audit could be tested with standard regression so I have converted all tests over and removed test.pl. Sawada, I'd certainly appreciate it if you'd try again and see if you are still getting a segfault with your test code (which you can find in the regression tests). Currently the patch will compile on master (I tested with b22a36a) or optionally with Alvaro's deparse patches applied (only 0001 & 0002 needed). I've supplied a different regression test out file (expected/pg_audit-deparse.out) which can be copied over the standard out file (expected/pg_audit.out) if you'd like to do regression on pg_audit with deparse. The small section of code that calls pg_event_trigger_ddl_commands() can be compiled by defining DEPARSE or removed the #ifdefs around that block. Please let me know if I've missed anything and I look forward to comments and questions. Thanks, -- - David Steele david@pgmasters.net
Вложения
This patch does not apply cleanly due to the moving of pgbench (patch to filelist.sgml failed). Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp > Attached is the v7 pg_audit patch. > > I've tried to address Peter's documentation concerns by cleaning up the > terminology and adding a real-world case plus usage recommendations. > The word "auditing" has been expunged from the docs in favor of the term > "audit logging". > > Per Simon's request, there is now a pg_audit.log_relation setting that > makes session audit logging exhaustively log all relations as it did > before. The ROLE logging class is back as well. > > Simon also suggested a way that pg_audit could be tested with standard > regression so I have converted all tests over and removed test.pl. > > Sawada, I'd certainly appreciate it if you'd try again and see if you > are still getting a segfault with your test code (which you can find in > the regression tests). > > Currently the patch will compile on master (I tested with b22a36a) or > optionally with Alvaro's deparse patches applied (only 0001 & 0002 > needed). I've supplied a different regression test out file > (expected/pg_audit-deparse.out) which can be copied over the standard > out file (expected/pg_audit.out) if you'd like to do regression on > pg_audit with deparse. The small section of code that calls > pg_event_trigger_ddl_commands() can be compiled by defining DEPARSE or > removed the #ifdefs around that block. > > Please let me know if I've missed anything and I look forward to > comments and questions. > > Thanks, > -- > - David Steele > david@pgmasters.net
On 4/14/15 7:13 PM, Tatsuo Ishii wrote: > This patch does not apply cleanly due to the moving of pgbench (patch > to filelist.sgml failed). Thank you for pointing that out! Ironic that it was the commit directly after the one I was testing with that broke the patch. It appears the end of the last CF is a very bad time to be behind HEAD. Fixed in attached v8 patch. -- - David Steele david@pgmasters.net
Вложения
> Thank you for pointing that out! > > Ironic that it was the commit directly after the one I was testing with > that broke the patch. It appears the end of the last CF is a very bad > time to be behind HEAD. > > Fixed in attached v8 patch. Thank you for your quick response. BTW, in my understanding pg_audit allows to track a table access even if it's used in a view. I think this is a nice feature and it would be better explicitly stated in the document and the test case is better included in the regression test. Here is a sample session: CREATE TABLE test2 (id INT); CREATE VIEW vtest2 AS SELECT * FROM test2; GRANT SELECT ON TABLE public.test2 TO auditor; GRANT SELECT ON TABLE public.vtest2 TO auditor; SELECT * FROM vtest2; NOTICE: AUDIT: SESSION,1,1,READ,SELECT,,,SELECT * FROM vtest2; NOTICE: AUDIT: OBJECT,1,1,READ,SELECT,VIEW,public.vtest2,SELECT * FROM vtest2; NOTICE: AUDIT: OBJECT,1,1,READ,SELECT,TABLE,public.test2,SELECT * FROM vtest2; Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
On Wed, Apr 15, 2015 at 8:57 AM, David Steele <david@pgmasters.net> wrote: > On 4/14/15 7:13 PM, Tatsuo Ishii wrote: >> This patch does not apply cleanly due to the moving of pgbench (patch >> to filelist.sgml failed). > > Thank you for pointing that out! > > Ironic that it was the commit directly after the one I was testing with > that broke the patch. It appears the end of the last CF is a very bad > time to be behind HEAD. > > Fixed in attached v8 patch. Thank you for updating the patch! I applied the patch and complied them successfully without WARNING. I tested v8 patch with CURSOR case I mentioned before, and got segmentation fault again. Here are log messages in my environment, =# select test();LOG: server process (PID 29730) was terminated by signal 11: Segmentation fault DETAIL: Failed process was running: select test(); LOG: terminating any other active server processes WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. FATAL: the database system is in recovery mode I hope that these messages helps you to address this problem. I will also try to address this. Regards, ------- Sawada Masahiko
On Wed, Apr 15, 2015 at 10:52 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> On Wed, Apr 15, 2015 at 8:57 AM, David Steele <david@pgmasters.net> wrote:
>> On 4/14/15 7:13 PM, Tatsuo Ishii wrote:
>>> This patch does not apply cleanly due to the moving of pgbench (patch
>>> to filelist.sgml failed).
>>
>> Thank you for pointing that out!
>>
>> Ironic that it was the commit directly after the one I was testing with
>> that broke the patch. It appears the end of the last CF is a very bad
>> time to be behind HEAD.
>>
>> Fixed in attached v8 patch.
>
> Thank you for updating the patch!
>
> I applied the patch and complied them successfully without WARNING.
>
> I tested v8 patch with CURSOR case I mentioned before, and got
> segmentation fault again.
> Here are log messages in my environment,
>
> =# select test();
> LOG: server process (PID 29730) was terminated by signal 11:
> Segmentation fault
> DETAIL: Failed process was running: select test();
> LOG: terminating any other active server processes
> WARNING: terminating connection because of crash of another server process
> DETAIL: The postmaster has commanded this server process to roll back
> the current transaction and exit, because another server process
> exited abnormally and possibly corrupted shared memory.
> HINT: In a moment you should be able to reconnect to the database and
> repeat your command.
> FATAL: the database system is in recovery mode
>
> I hope that these messages helps you to address this problem.
> I will also try to address this.
>
> Regards,
>
> -------
> Sawada Masahiko
> I will also try to address this.
I investigated this problem and inform you my result here.
When we execute test() function I mentioned, the three stack items in
total are stored into auditEventStack.
The two of them are freed by stack_pop() -> stack_free() (i.g,
stack_free() is called by stack_pop()).
One of them is freed by PortalDrop() -> .. ->
MemoryContextDeleteChildren() -> ... -> stack_free().
And it is freed at the same time as deleting pg_audit memory context,
and stack will be completely empty.
But after freeing all items, finish_xact_command() function could call
PortalDrop(), so ExecutorEnd() function could be called again.
pg_audit has ExecutorEnd_hook, so postgres tries to free that item.. SEGV.
In my environment, the following change fixes it.
--- pg_audit.c.org 2015-04-15 14:21:07.541866525 +0900
+++ pg_audit.c 2015-04-15 11:36:53.758877339 +0900
@@ -1291,7 +1291,7 @@ standard_ExecutorEnd(queryDesc);
/* Pop the audit event off the stack */
- if (!internalStatement)
+ if (!internalStatement && auditEventStack != NULL) { stack_pop(auditEventStack->stackId); }
It might be good idea to add Assert() at before calling stack_pop().
I'm not sure this change is exactly correct, but I hope this
information helps you.
Regards,
-------
Sawada Masahiko
On 4/15/15 11:30 AM, Sawada Masahiko wrote:
> On Wed, Apr 15, 2015 at 10:52 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>> I tested v8 patch with CURSOR case I mentioned before, and got
>> segmentation fault again.
>> Here are log messages in my environment,
>>
>> =# select test();
>> LOG: server process (PID 29730) was terminated by signal 11:
>> Segmentation fault
>> DETAIL: Failed process was running: select test();
>> LOG: terminating any other active server processes
>> WARNING: terminating connection because of crash of another server process
>> DETAIL: The postmaster has commanded this server process to roll back
>> the current transaction and exit, because another server process
>> exited abnormally and possibly corrupted shared memory.
>> HINT: In a moment you should be able to reconnect to the database and
>> repeat your command.
>> FATAL: the database system is in recovery mode
>
> I investigated this problem and inform you my result here.
>
> When we execute test() function I mentioned, the three stack items in
> total are stored into auditEventStack.
> The two of them are freed by stack_pop() -> stack_free() (i.g,
> stack_free() is called by stack_pop()).
> One of them is freed by PortalDrop() -> .. ->
> MemoryContextDeleteChildren() -> ... -> stack_free().
> And it is freed at the same time as deleting pg_audit memory context,
> and stack will be completely empty.
>
> But after freeing all items, finish_xact_command() function could call
> PortalDrop(), so ExecutorEnd() function could be called again.
> pg_audit has ExecutorEnd_hook, so postgres tries to free that item.. SEGV.
>
> In my environment, the following change fixes it.
>
> --- pg_audit.c.org 2015-04-15 14:21:07.541866525 +0900
> +++ pg_audit.c 2015-04-15 11:36:53.758877339 +0900
> @@ -1291,7 +1291,7 @@
> standard_ExecutorEnd(queryDesc);
>
> /* Pop the audit event off the stack */
> - if (!internalStatement)
> + if (!internalStatement && auditEventStack != NULL)
> {
> stack_pop(auditEventStack->stackId);
> }
>
> It might be good idea to add Assert() at before calling stack_pop().
>
> I'm not sure this change is exactly correct, but I hope this
> information helps you.
I appreciate you taking the time to look - this is the same conclusion I
came to. I also hardened another area that I thought might be vulnerable.
I've seen this problem with explicit cursors before (and fixed it in
another place a while ago). The memory context that is current in
ExecutorStart is freed before ExecutorEnd is called only in this case as
far as I can tell. I'm not sure this is very consistent behavior.
I have attached patch v9 which fixes this issue as you suggested, but
I'm not completely satisfied with it. It seems like there could be an
unintentional pop from the stack in a case of deeper nesting. This
might not be possible but it's hard to disprove.
I'll think about it some more, but meanwhile this patch addresses the
present issue.
--
- David Steele
david@pgmasters.net
Вложения
On 4/14/15 8:37 PM, Tatsuo Ishii wrote: > BTW, in my understanding pg_audit allows to track a table access even > if it's used in a view. I think this is a nice feature and it would be > better explicitly stated in the document and the test case is better > included in the regression test. > > Here is a sample session: > > CREATE TABLE test2 (id INT); > CREATE VIEW vtest2 AS SELECT * FROM test2; > GRANT SELECT ON TABLE public.test2 TO auditor; > GRANT SELECT ON TABLE public.vtest2 TO auditor; > SELECT * FROM vtest2; > NOTICE: AUDIT: SESSION,1,1,READ,SELECT,,,SELECT * FROM vtest2; > NOTICE: AUDIT: OBJECT,1,1,READ,SELECT,VIEW,public.vtest2,SELECT * FROM vtest2; > NOTICE: AUDIT: OBJECT,1,1,READ,SELECT,TABLE,public.test2,SELECT * FROM vtest2; That's the idea! In the documentation I throw around the word "relation" pretty liberally, but you are right that some clarification would be helpful. I have added a few parenthetical statements to the docs that should make them clearer. I also took your suggestion and added a view regression test. Both are in patch v9 which I attached to my previous email on this thread. Thank you for taking the time to have a look. -- - David Steele david@pgmasters.net
On Thu, Apr 16, 2015 at 2:34 AM, David Steele <david@pgmasters.net> wrote:
> On 4/15/15 11:30 AM, Sawada Masahiko wrote:
>> On Wed, Apr 15, 2015 at 10:52 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>>> I tested v8 patch with CURSOR case I mentioned before, and got
>>> segmentation fault again.
>>> Here are log messages in my environment,
>>>
>>> =# select test();
>>> LOG: server process (PID 29730) was terminated by signal 11:
>>> Segmentation fault
>>> DETAIL: Failed process was running: select test();
>>> LOG: terminating any other active server processes
>>> WARNING: terminating connection because of crash of another server process
>>> DETAIL: The postmaster has commanded this server process to roll back
>>> the current transaction and exit, because another server process
>>> exited abnormally and possibly corrupted shared memory.
>>> HINT: In a moment you should be able to reconnect to the database and
>>> repeat your command.
>>> FATAL: the database system is in recovery mode
>>
>> I investigated this problem and inform you my result here.
>>
>> When we execute test() function I mentioned, the three stack items in
>> total are stored into auditEventStack.
>> The two of them are freed by stack_pop() -> stack_free() (i.g,
>> stack_free() is called by stack_pop()).
>> One of them is freed by PortalDrop() -> .. ->
>> MemoryContextDeleteChildren() -> ... -> stack_free().
>> And it is freed at the same time as deleting pg_audit memory context,
>> and stack will be completely empty.
>>
>> But after freeing all items, finish_xact_command() function could call
>> PortalDrop(), so ExecutorEnd() function could be called again.
>> pg_audit has ExecutorEnd_hook, so postgres tries to free that item.. SEGV.
>>
>> In my environment, the following change fixes it.
>>
>> --- pg_audit.c.org 2015-04-15 14:21:07.541866525 +0900
>> +++ pg_audit.c 2015-04-15 11:36:53.758877339 +0900
>> @@ -1291,7 +1291,7 @@
>> standard_ExecutorEnd(queryDesc);
>>
>> /* Pop the audit event off the stack */
>> - if (!internalStatement)
>> + if (!internalStatement && auditEventStack != NULL)
>> {
>> stack_pop(auditEventStack->stackId);
>> }
>>
>> It might be good idea to add Assert() at before calling stack_pop().
>>
>> I'm not sure this change is exactly correct, but I hope this
>> information helps you.
>
> I appreciate you taking the time to look - this is the same conclusion I
> came to. I also hardened another area that I thought might be vulnerable.
>
> I've seen this problem with explicit cursors before (and fixed it in
> another place a while ago). The memory context that is current in
> ExecutorStart is freed before ExecutorEnd is called only in this case as
> far as I can tell. I'm not sure this is very consistent behavior.
>
> I have attached patch v9 which fixes this issue as you suggested, but
> I'm not completely satisfied with it. It seems like there could be an
> unintentional pop from the stack in a case of deeper nesting. This
> might not be possible but it's hard to disprove.
>
> I'll think about it some more, but meanwhile this patch addresses the
> present issue.
Thank you for updating the patch.
One question about regarding since v7 (or later) patch is;
What is the different between OBJECT logging and SESSION logging?
I used v9 patch with "pg_audit.log_relation = on", and got quite
similar but different log as follows.
=# select * from hoge, bar where hoge.col = bar.col;
NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.hoge,"select *
from hoge, bar where hoge.col = bar.col;"
NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.hoge,"select *
from hoge, bar where hoge.col = bar.col;"
NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.bar,"select * from
hoge, bar where hoge.col = bar.col;"
NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.bar,"select *
from hoge, bar where hoge.col = bar.col;"
The behaviour of SESSION log is similar to OBJECT log now, and SESSION
log per session (i.g, pg_audit.log_relation = off) is also similar to
'log_statement = all'. (enhancing log_statement might be enough)
So I couldn't understand needs of SESSION log.
Regards,
-------
Sawada Masahiko
On 4/20/15 4:40 AM, Sawada Masahiko wrote: > > Thank you for updating the patch. > > One question about regarding since v7 (or later) patch is; > What is the different between OBJECT logging and SESSION logging? In brief, session logging can log anything that happens in a user session while object logging only targets DML and SELECT on selected relations. The pg_audit.log_relation setting is intended to mimic the prior behavior of pg_audit before object logging was added. In general, I would not expect pg_audit.log = 'read, write' to be used with pg_audit.role. In other words, session logging of DML and SELECT should probably not be turned on at the same time as object logging is in use. Object logging is intended to be a fine-grained version of pg_audit.log = 'read, write' (one or both). > I used v9 patch with "pg_audit.log_relation = on", and got quite > similar but different log as follows. > > =# select * from hoge, bar where hoge.col = bar.col; > NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.hoge,"select * > from hoge, bar where hoge.col = bar.col;" > NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.hoge,"select * > from hoge, bar where hoge.col = bar.col;" > NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.bar,"select * from > hoge, bar where hoge.col = bar.col;" > NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.bar,"select * > from hoge, bar where hoge.col = bar.col;" > > The behaviour of SESSION log is similar to OBJECT log now, and SESSION > log per session (i.g, pg_audit.log_relation = off) is also similar to > 'log_statement = all'. (enhancing log_statement might be enough) > So I couldn't understand needs of SESSION log. Session logging is quite different from 'log_statement = all'. See: http://www.postgresql.org/message-id/552323B2.8060708@pgmasters.net and/or the "Why pg_audit?" section of the pg_audit documentation. I agree that it may make sense in the future to merge session logging into log_statements, but for now it does provide important additional functionality for creating audit logs. Regards, -- - David Steele david@pgmasters.net
On Mon, Apr 20, 2015 at 10:17 PM, David Steele <david@pgmasters.net> wrote: > On 4/20/15 4:40 AM, Sawada Masahiko wrote: >> >> Thank you for updating the patch. >> >> One question about regarding since v7 (or later) patch is; >> What is the different between OBJECT logging and SESSION logging? > > In brief, session logging can log anything that happens in a user > session while object logging only targets DML and SELECT on selected > relations. > > The pg_audit.log_relation setting is intended to mimic the prior > behavior of pg_audit before object logging was added. > > In general, I would not expect pg_audit.log = 'read, write' to be used > with pg_audit.role. In other words, session logging of DML and SELECT > should probably not be turned on at the same time as object logging is > in use. Object logging is intended to be a fine-grained version of > pg_audit.log = 'read, write' (one or both). Thank you for your explanation! I understood about how to use two logging style. >> I used v9 patch with "pg_audit.log_relation = on", and got quite >> similar but different log as follows. >> >> =# select * from hoge, bar where hoge.col = bar.col; >> NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.hoge,"select * >> from hoge, bar where hoge.col = bar.col;" >> NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.hoge,"select * >> from hoge, bar where hoge.col = bar.col;" >> NOTICE: AUDIT: OBJECT,8,1,READ,SELECT,TABLE,public.bar,"select * from >> hoge, bar where hoge.col = bar.col;" >> NOTICE: AUDIT: SESSION,8,1,READ,SELECT,TABLE,public.bar,"select * >> from hoge, bar where hoge.col = bar.col;" >> >> The behaviour of SESSION log is similar to OBJECT log now, and SESSION >> log per session (i.g, pg_audit.log_relation = off) is also similar to >> 'log_statement = all'. (enhancing log_statement might be enough) >> So I couldn't understand needs of SESSION log. > > Session logging is quite different from 'log_statement = all'. See: > > http://www.postgresql.org/message-id/552323B2.8060708@pgmasters.net > > and/or the "Why pg_audit?" section of the pg_audit documentation. > > I agree that it may make sense in the future to merge session logging > into log_statements, but for now it does provide important additional > functionality for creating audit logs. > I'm concerned that behaviour of pg_audit has been changed at a few times as far as I remember. Did we achieve consensus on this design? And one question; OBJECT logging of all tuple deletion (i.g. DELETE FROM hoge) seems like not work as follows. =# grant all on bar TO masahiko; (1) Delete all tuple =# insert into bar values(1); =# delete from bar ; NOTICE: AUDIT: SESSION,47,1,WRITE,DELETE,TABLE,public.bar,delete from bar ; DELETE 1 (2) Delete specified tuple (but same result as (1)) =# insert into bar values(1); =# delete from bar where col = 1; NOTICE: AUDIT: OBJECT,48,1,WRITE,DELETE,TABLE,public.bar,delete from bar where col = 1; NOTICE: AUDIT: SESSION,48,1,WRITE,DELETE,TABLE,public.bar,delete from bar where col = 1; DELETE 1 Regards, ------- Sawada Masahiko
On 4/23/15 5:49 AM, Sawada Masahiko wrote: > > I'm concerned that behaviour of pg_audit has been changed at a few > times as far as I remember. Did we achieve consensus on this design? The original author Abhijit expressed support for the SESSION/OBJECT concept before I started working on the code and so has Stephen Frost. As far as I know all outstanding comments from the community have been addressed. Overall behavior has not changed very much since being submitted to the CF in February - mostly just tweaks and additional options. > And one question; OBJECT logging of all tuple deletion (i.g. DELETE > FROM hoge) seems like not work as follows. > > > =# grant all on bar TO masahiko; > > (1) Delete all tuple > =# insert into bar values(1); > =# delete from bar ; > NOTICE: AUDIT: SESSION,47,1,WRITE,DELETE,TABLE,public.bar,delete from bar ; > DELETE 1 > > (2) Delete specified tuple (but same result as (1)) > =# insert into bar values(1); > =# delete from bar where col = 1; > NOTICE: AUDIT: OBJECT,48,1,WRITE,DELETE,TABLE,public.bar,delete from > bar where col = 1; > NOTICE: AUDIT: SESSION,48,1,WRITE,DELETE,TABLE,public.bar,delete from > bar where col = 1; > DELETE 1 Definitely a bug. Object logging works in the second case because the select privileges on the "col" column trigger logging. I have fixed this and added a regression test. I also found a way to get the stack memory context under the query memory context. Because of the order of execution it requires moving the memory context but I still think it's a much better solution. I was able to remove most of the stack pops (except function logging) and the output remained stable. I've also added some checking to make sure that if anything looks funny on the stack an error will be generated. Thanks for the feedback! -- - David Steele david@pgmasters.net
Вложения
On Fri, Apr 24, 2015 at 3:23 AM, David Steele <david@pgmasters.net> wrote: > On 4/23/15 5:49 AM, Sawada Masahiko wrote: >> >> I'm concerned that behaviour of pg_audit has been changed at a few >> times as far as I remember. Did we achieve consensus on this design? > > The original author Abhijit expressed support for the SESSION/OBJECT > concept before I started working on the code and so has Stephen Frost. > As far as I know all outstanding comments from the community have been > addressed. > > Overall behavior has not changed very much since being submitted to the > CF in February - mostly just tweaks and additional options. > >> And one question; OBJECT logging of all tuple deletion (i.g. DELETE >> FROM hoge) seems like not work as follows. >> >> >> =# grant all on bar TO masahiko; >> >> (1) Delete all tuple >> =# insert into bar values(1); >> =# delete from bar ; >> NOTICE: AUDIT: SESSION,47,1,WRITE,DELETE,TABLE,public.bar,delete from bar ; >> DELETE 1 >> >> (2) Delete specified tuple (but same result as (1)) >> =# insert into bar values(1); >> =# delete from bar where col = 1; >> NOTICE: AUDIT: OBJECT,48,1,WRITE,DELETE,TABLE,public.bar,delete from >> bar where col = 1; >> NOTICE: AUDIT: SESSION,48,1,WRITE,DELETE,TABLE,public.bar,delete from >> bar where col = 1; >> DELETE 1 > > Definitely a bug. Object logging works in the second case because the > select privileges on the "col" column trigger logging. I have fixed > this and added a regression test. > > I also found a way to get the stack memory context under the query > memory context. Because of the order of execution it requires moving > the memory context but I still think it's a much better solution. I was > able to remove most of the stack pops (except function logging) and the > output remained stable. > > I've also added some checking to make sure that if anything looks funny > on the stack an error will be generated. > > Thanks for the feedback! > Thank you for updating the patch! I ran the postgres regression test on database which is enabled pg_audit, it works fine. Looks good to me. If someone don't have review comment or bug report, I will mark this as "Ready for Committer". Regards, ------- Sawada Masahiko
On 4/28/15 2:14 AM, Sawada Masahiko wrote: > On Fri, Apr 24, 2015 at 3:23 AM, David Steele <david@pgmasters.net> wrote: >> I've also added some checking to make sure that if anything looks funny >> on the stack an error will be generated. >> >> Thanks for the feedback! >> > > Thank you for updating the patch! > I ran the postgres regression test on database which is enabled > pg_audit, it works fine. > Looks good to me. > > If someone don't have review comment or bug report, I will mark this > as "Ready for Committer". Thank you! I appreciate all your work reviewing this patch and of course everyone else who commented on, reviewed or tested the patch along the way. -- - David Steele david@pgmasters.net
On Wed, Apr 29, 2015 at 12:17 AM, David Steele <david@pgmasters.net> wrote: > On 4/28/15 2:14 AM, Sawada Masahiko wrote: >> On Fri, Apr 24, 2015 at 3:23 AM, David Steele <david@pgmasters.net> wrote: >>> I've also added some checking to make sure that if anything looks funny >>> on the stack an error will be generated. >>> >>> Thanks for the feedback! >>> >> >> Thank you for updating the patch! >> I ran the postgres regression test on database which is enabled >> pg_audit, it works fine. >> Looks good to me. >> >> If someone don't have review comment or bug report, I will mark this >> as "Ready for Committer". > > Thank you! I appreciate all your work reviewing this patch and of > course everyone else who commented on, reviewed or tested the patch > along the way. > I have changed the status this to "Ready for Committer". Regards, ------- Sawada Masahiko
On Thu, Apr 30, 2015 at 12:57 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Wed, Apr 29, 2015 at 12:17 AM, David Steele <david@pgmasters.net> wrote: >> On 4/28/15 2:14 AM, Sawada Masahiko wrote: >>> On Fri, Apr 24, 2015 at 3:23 AM, David Steele <david@pgmasters.net> wrote: >>>> I've also added some checking to make sure that if anything looks funny >>>> on the stack an error will be generated. >>>> >>>> Thanks for the feedback! >>>> >>> >>> Thank you for updating the patch! >>> I ran the postgres regression test on database which is enabled >>> pg_audit, it works fine. >>> Looks good to me. >>> >>> If someone don't have review comment or bug report, I will mark this >>> as "Ready for Committer". >> >> Thank you! I appreciate all your work reviewing this patch and of >> course everyone else who commented on, reviewed or tested the patch >> along the way. >> > > I have changed the status this to "Ready for Committer". The specification of "session audit logging" seems to be still unclear to me. For example, why doesn't "session audit logging" log queries accessing to a catalog like pg_class? Why doesn't it log any queries executed in aborted transaction state? Since there is no such information in the document, I'm afraid that users would easily get confused with it. Even if we document it, I'm not sure if the current behavior is good for the audit purpose. For example, some users may want to log even queries accessing to the catalogs. I have no idea about when the current CommitFest will end. But probably we don't have that much time left. So I'm thinking that maybe we should pick up small, self-contained and useful part from the patch and focus on that. If we try to commit every features that the patch provides, we might get nothing at least in 9.5, I'm afraid. Regards, -- Fujii Masao
On 4/30/15 6:05 AM, Fujii Masao wrote: > On Thu, Apr 30, 2015 at 12:57 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> >> I have changed the status this to "Ready for Committer". > > The specification of "session audit logging" seems to be still unclear to me. > For example, why doesn't "session audit logging" log queries accessing to The idea was that queries consisting of *only* catalog relations are essentially noise. This makes the log much quieter when tools like psql or PgAdmin are in use. Queries that contain a mix of catalog and user tables are logged. However, you make a good point, so in the spirit of cautious defaults I've changed the behavior to log in this case and allow admins to turn off the behavior if they choose with a new GUC, pg_audit.log_catalog. > a catalog like pg_class? Why doesn't it log any queries executed in aborted > transaction state? Since there is no such information in the document, The error that aborts a transaction can easily be logged via the standard logging facility. All prior statements in the transaction will be logged with pg_audit. This is acceptable from an audit logging perspective as long as it is documented behavior, which as you point out it currently is not. This has now been documented in the caveats sections which should make it clearer to users. > I'm afraid that users would easily get confused with it. Even if we document it, > I'm not sure if the current behavior is good for the audit purpose. For example, > some users may want to log even queries accessing to the catalogs. Agreed, and this is now the default. > I have no idea about when the current CommitFest will end. But probably > we don't have that much time left. So I'm thinking that maybe we should pick up > small, self-contained and useful part from the patch and focus on that. > If we try to commit every features that the patch provides, we might get > nothing at least in 9.5, I'm afraid. May 15th is the feature freeze, so that does give a little time. It's not clear to me what a "self-contained" part of the patch would be. If you have specific ideas on what could be broken out I'm interested to hear them. Patch v11 is attached with the changes discussed here plus some other improvements to the documentation suggested by Erik. -- - David Steele david@pgmasters.net
Вложения
On Fri, May 1, 2015 at 6:24 AM, David Steele <david@pgmasters.net> wrote: > On 4/30/15 6:05 AM, Fujii Masao wrote: >> On Thu, Apr 30, 2015 at 12:57 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> >>> I have changed the status this to "Ready for Committer". >> >> The specification of "session audit logging" seems to be still unclear to me. >> For example, why doesn't "session audit logging" log queries accessing to > > The idea was that queries consisting of *only* catalog relations are > essentially noise. This makes the log much quieter when tools like psql > or PgAdmin are in use. Queries that contain a mix of catalog and user > tables are logged. > > However, you make a good point, so in the spirit of cautious defaults > I've changed the behavior to log in this case and allow admins to turn > off the behavior if they choose with a new GUC, pg_audit.log_catalog. > >> a catalog like pg_class? Why doesn't it log any queries executed in aborted >> transaction state? Since there is no such information in the document, > > The error that aborts a transaction can easily be logged via the > standard logging facility. All prior statements in the transaction will > be logged with pg_audit. This is acceptable from an audit logging > perspective as long as it is documented behavior, which as you point out > it currently is not. > > This has now been documented in the caveats sections which should make > it clearer to users. > >> I'm afraid that users would easily get confused with it. Even if we document it, >> I'm not sure if the current behavior is good for the audit purpose. For example, >> some users may want to log even queries accessing to the catalogs. > > Agreed, and this is now the default. > >> I have no idea about when the current CommitFest will end. But probably >> we don't have that much time left. So I'm thinking that maybe we should pick up >> small, self-contained and useful part from the patch and focus on that. >> If we try to commit every features that the patch provides, we might get >> nothing at least in 9.5, I'm afraid. > > May 15th is the feature freeze, so that does give a little time. It's > not clear to me what a "self-contained" part of the patch would be. If > you have specific ideas on what could be broken out I'm interested to > hear them. > > Patch v11 is attached with the changes discussed here plus some other > improvements to the documentation suggested by Erik. > For now, since pg_audit patch has a pg_audit_ddl_command_end() function which uses part of un-committed "deparsing utility commands" patch API, pg_audit patch is completely depend on that patch. If API name, interface are changed, it would affect for pg_audit patch. I'm not sure about progress of "deparsing utility command" patch but you have any idea if that patch is not committed into 9.5 until May 15? Regards, ------- Sawada Masahiko
On 5/1/15 5:58 AM, Sawada Masahiko wrote: > On Fri, May 1, 2015 at 6:24 AM, David Steele <david@pgmasters.net> wrote: >> >> May 15th is the feature freeze, so that does give a little time. It's >> not clear to me what a "self-contained" part of the patch would be. If >> you have specific ideas on what could be broken out I'm interested to >> hear them. >> >> Patch v11 is attached with the changes discussed here plus some other >> improvements to the documentation suggested by Erik. >> > > For now, since pg_audit patch has a pg_audit_ddl_command_end() > function which uses part of un-committed "deparsing utility commands" > patch API, > pg_audit patch is completely depend on that patch. > If API name, interface are changed, it would affect for pg_audit patch. > I'm not sure about progress of "deparsing utility command" patch but > you have any idea if that patch is not committed into 9.5 until May > 15? Currently the deparse dependent code is ifdef'd so pg_audit compiles and operates just fine against master. Having the deparse code is nice because it allows less common object types to log with full-qualified names but it is in not a requirement for pg_audit. I'd like to see at least patch 0001 of deparse committed. Not only because it provides the functionality that deparse uses, but because it makes event triggers truly useful in pl/pgsql. -- - David Steele david@pgmasters.net
On 4/30/15 6:05 AM, Fujii Masao wrote: > The specification of "session audit logging" seems to be still unclear to me. As I had mentioned previously, I would prefer session audit logging to be integrated with the normal statement logging configuration.
* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 4/30/15 6:05 AM, Fujii Masao wrote:
> > The specification of "session audit logging" seems to be still unclear to me.
>
> As I had mentioned previously, I would prefer session audit logging to
> be integrated with the normal statement logging configuration.
I'd certainly love to see the logging in core be improved, but that's
also rather tangential to this extension and we could certainly have
both quite happily. Further, being able to configure and have
consistent information from the extension is valuable even if options
are added to the in-core logging to make it more flexible.
One particular advantage of having the extension is that having it
doesn't impact existing users of the in-core logging system. I don't
recall any specific proposals for improving the in-core logging system
to add the capabilities for session logging that the extension
provides, but it seems likely to require changes to at least the CSV
format, new log_line_prefix escape codes (which we're using quite a
few of already...), new GUCs, and potentially behavior changes to make
it work. As I say above, I'm happy to have those discussions (and
I've been party to them quite a few times in the past...), but it
seems unlikely to seriously reduce the usefulness of session logging
being in the extension and producing consistent output with the object
logging.
Thanks!
Stephen
On 5/4/15 3:00 PM, Stephen Frost wrote: > One particular advantage of having the extension is that having it > doesn't impact existing users of the in-core logging system. I don't > recall any specific proposals for improving the in-core logging system > to add the capabilities for session logging that the extension > provides, but it seems likely to require changes to at least the CSV > format, new log_line_prefix escape codes (which we're using quite a > few of already...), new GUCs, and potentially behavior changes to make > it work. As I say above, I'm happy to have those discussions (and > I've been party to them quite a few times in the past...), Well yeah, from my perspective, the reason not much has happened in the area of logging is that you and Magnus have repeatedly said you were planning some things. The reasons given above against changing logging just as easily apply to auditing. This implementation is only a starting point. We don't know whether it will fulfill anyone's requirements. The requirements for logging are "it feels good enough for an admin". The requirements for auditing are "it satisfies this checklist". We need to be prepared to aggressively evolve this as we gather more information about requirements. Otherwise this will become something like contrib/isn, where we know it doesn't satisfy real-world uses anymore, but we're afraid to touch it because someone might be using it and we don't have the domain knowledge to tell them to stop.
* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 5/4/15 3:00 PM, Stephen Frost wrote:
> > One particular advantage of having the extension is that having it
> > doesn't impact existing users of the in-core logging system. I don't
> > recall any specific proposals for improving the in-core logging system
> > to add the capabilities for session logging that the extension
> > provides, but it seems likely to require changes to at least the CSV
> > format, new log_line_prefix escape codes (which we're using quite a
> > few of already...), new GUCs, and potentially behavior changes to make
> > it work. As I say above, I'm happy to have those discussions (and
> > I've been party to them quite a few times in the past...),
>
> Well yeah, from my perspective, the reason not much has happened in the
> area of logging is that you and Magnus have repeatedly said you were
> planning some things.
If that was holding anyone back from working on it, then I'm truely
sorry. I would encourage anyone interesting in any particular feature
to speak up and ask what the status is and if others are working on
something, especially if they have time to spend advancing it. I was
certainly quite happy when Abhijit posted the initial version of this
nearly a year ago as it showed that there were individuals able to spend
substantial time on it, as well as a use-case which would be solved
through such an extension.
I don't wish to lay claim to any particular feature nor to make any
guarantees, but I will say that I'm happy to have moved into a position
in the past year where I can devote a great deal more in time and
resources towards PostgreSQL than I've ever been able to in the past.
> The reasons given above against changing logging just as easily apply to
> auditing.
I don't follow this logic. The concerns raised above are about changing
our in-core logging. We haven't got in-core auditing and so I don't see
how they apply to it.
> This implementation is only a starting point. We don't know
> whether it will fulfill anyone's requirements. The requirements for
> logging are "it feels good enough for an admin". The requirements for
> auditing are "it satisfies this checklist". We need to be prepared to
> aggressively evolve this as we gather more information about
> requirements. Otherwise this will become something like contrib/isn,
> where we know it doesn't satisfy real-world uses anymore, but we're
> afraid to touch it because someone might be using it and we don't have
> the domain knowledge to tell them to stop.
I agree that this is a starting point. From the discussions which I've
had with PostgreSQL users (both our clients and others), this does
fulfill a set of requirements for them. That isn't to say that it's a
complete and total solution, nor that we will stop here (we certainly
won't!), but I'm confident it *is* solving a real use-case for our
users. I don't mean to speak for them, but my understanding is that the
work which was done by Abhijit and 2ndQ was sponsored by an EU grant
which had a specific set of requirements which this is intended to
satisfy.
Further, we are absolutely prepared to aggressively evolve this as we
learn and understand how it's being used out in the field- but I don't
believe we're ever going to really understand that until we put
something out there.
Thanks!
Stephen
On 5/4/15 8:37 PM, Stephen Frost wrote: > I don't follow this logic. The concerns raised above are about changing > our in-core logging. We haven't got in-core auditing and so I don't see > how they apply to it. How is session "auditing" substantially different from statement logging? I think it is not, and we could tweak the logging facilities a bit to satisfy the audit trail case while making often-requested enhancement to the traditional logging use case as well at the same time. At least no one has disputed that yet. The only argument against has been that they don't want to touch the logging.
* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 5/4/15 8:37 PM, Stephen Frost wrote:
> > I don't follow this logic. The concerns raised above are about changing
> > our in-core logging. We haven't got in-core auditing and so I don't see
> > how they apply to it.
>
> How is session "auditing" substantially different from statement logging?
David had previously outlined the technical differences between the
statement logging we have today and what pgAudit does, but I gather
you're asking about it definitionally, though it ends up amounting to
much the same to me. Auditing is about "what happened" whereas
statement logging is "log whatever statement the user sent." pgAudit
bears this out by logging internal SQL statements and object
information, unlike what we do with statement logging today.
> I think it is not, and we could tweak the logging facilities a bit to
> satisfy the audit trail case while making often-requested enhancement to
> the traditional logging use case as well at the same time.
Changing our existing statement logging to be a "what happened" auditing
system is possible, but I don't see it as a "tweak."
> At least no one has disputed that yet. The only argument against has
> been that they don't want to touch the logging.
I'm afraid we've been talking past each other here- I'm fully on-board
with enhancing our in-core logging capabilities and even looking to the
future at having object auditing included in core. It's not my intent
to dispute that or to argue against it.
Perhaps I've misunderstood the thrust of this sub-thread, so let me
explain what I thought the discussion was. My understanding was that
you were concerned about having session auditing included in pgAudit
and, further, that you wanted to see our in-core statement logging be
improved. I agree that we want to improve the in-core statement logging
and, ideally, have an in-core auditing solution in the future. I was
attempting to address the concern about having session logging in
pgAudit by pointing out that it's valuable to have even if our in-core
statement logging is augmented, and further, having it in pgAudit does
not preclude or reduce our ability to improve the in-core statement
logging in the future; indeed, it's my hope that we'll get good feedback
from users of pgAudit which could guide our in-core implementation. As
for the concern that pgAudit may end up "rotting" in the tree as some
other contrib modules have, I can say with confidence that we will have
users of it just as soon as they're able to move to a version of PG
which includes it and therefore will be supporting it and addressing
issues as we discover them, as I suspect the others who have been
involved in this discussion will be also. Additionally, as discussed
last summer, we can provide a migration path (which does not need to be
automated or even feature compatible) from pgAudit to an in-core
solution and then sunset pgAudit.
Building an in-core solution, in my estimation at least, is going to
require at least a couple of release cycles and having the feedback from
users of pgAudit will be very valuable to building a good solution, but
I don't believe we'll get that feedback without including it.
Lastly, from the perspective of the session logging piece, the code
footprint for that in pgAudit isn't the bulk or even terribly
significant, most of the code would still be required for the object
auditing capability.
Thanks!
Stephen
On 5/7/15 8:26 AM, Stephen Frost wrote: > * Peter Eisentraut (peter_e@gmx.net) wrote: >> On 5/4/15 8:37 PM, Stephen Frost wrote: >>> I don't follow this logic. The concerns raised above are about changing >>> our in-core logging. We haven't got in-core auditing and so I don't see >>> how they apply to it. >> >> How is session "auditing" substantially different from statement logging? > > David had previously outlined the technical differences between the > statement logging we have today and what pgAudit does, but I gather > you're asking about it definitionally, though it ends up amounting to > much the same to me. Auditing is about "what happened" whereas > statement logging is "log whatever statement the user sent." pgAudit > bears this out by logging internal SQL statements and object > information, unlike what we do with statement logging today. That's a great way to describe it and I'll see if I can incorporate this idea into the docs to hopefully make the topic a bit clearer. >> I think it is not, and we could tweak the logging facilities a bit to >> satisfy the audit trail case while making often-requested enhancement to >> the traditional logging use case as well at the same time. > > Changing our existing statement logging to be a "what happened" auditing > system is possible, but I don't see it as a "tweak." Agreed - not the least of which would be figuring out a more detailed statement classification system for core which would probably be the first step. > Lastly, from the perspective of the session logging piece, the code > footprint for that in pgAudit isn't the bulk or even terribly > significant, most of the code would still be required for the object > auditing capability. Both have a decent footprint but also a lot in common so it's fair to say that removing session audit logging would not reduce the amount of code significantly. -- - David Steele david@pgmasters.net
On Thu, May 7, 2015 at 10:26:55AM -0400, Stephen Frost wrote: > * Peter Eisentraut (peter_e@gmx.net) wrote: > > On 5/4/15 8:37 PM, Stephen Frost wrote: > > > I don't follow this logic. The concerns raised above are about changing > > > our in-core logging. We haven't got in-core auditing and so I don't see > > > how they apply to it. > > > > How is session "auditing" substantially different from statement logging? > > David had previously outlined the technical differences between the > statement logging we have today and what pgAudit does, but I gather > you're asking about it definitionally, though it ends up amounting to > much the same to me. Auditing is about "what happened" whereas > statement logging is "log whatever statement the user sent." pgAudit > bears this out by logging internal SQL statements and object > information, unlike what we do with statement logging today. Well, what I was looking for is how auditing is _conceptually_ different from logging, e.g. I can clearly explain how authentication (prove who you are) and authorization (what you are allowed to do) are different. Your definition above seems to be more behavioral, e.g. what arrived vs. what happened. It is not clear to me why reporting such information is conceptually different and requires different infrastructure, i.e. we could not easily combine authentication and authorization into the same infrastructure, but logging and auditing seems similar. (Actually, pg_hba.conf contains authentication and some course-grained authorization only because it is a good place to do low-overhead authorization; there was a performance requirement to do it in pg_hba.conf to prevent DOS attacks.) > > At least no one has disputed that yet. The only argument against has > > been that they don't want to touch the logging. > > I'm afraid we've been talking past each other here- I'm fully on-board > with enhancing our in-core logging capabilities and even looking to the > future at having object auditing included in core. It's not my intent > to dispute that or to argue against it. > > Perhaps I've misunderstood the thrust of this sub-thread, so let me > explain what I thought the discussion was. My understanding was that > you were concerned about having session auditing included in pgAudit > and, further, that you wanted to see our in-core statement logging be > improved. I agree that we want to improve the in-core statement logging > and, ideally, have an in-core auditing solution in the future. I was > attempting to address the concern about having session logging in > pgAudit by pointing out that it's valuable to have even if our in-core > statement logging is augmented, and further, having it in pgAudit does > not preclude or reduce our ability to improve the in-core statement > logging in the future; indeed, it's my hope that we'll get good feedback > from users of pgAudit which could guide our in-core implementation. As What is our history of doing things in contrib because we are not sure what we want, then moving it into core? My general recollection is that there is usually something in the contrib version we don't want to add to core and people are locked into the contrib API, so we are left supporting it, e.g. xml2, though you could argue that auditing doesn't have application lock-in and xml2 was tied to an external library feature. > for the concern that pgAudit may end up "rotting" in the tree as some > other contrib modules have, I can say with confidence that we will have > users of it just as soon as they're able to move to a version of PG > which includes it and therefore will be supporting it and addressing > issues as we discover them, as I suspect the others who have been Uh, why are they not using the PGXN version of pg_audit, and if it is because it isn't shipped with Postgres, then these seem like unmotivated users who will complain for some reason if we ever move it out of contrib. I guess the over-arching question is whether we have to put this into contrib so we can get feedback and change the API, or whether using from PGXN or incrementally adding it to core is the right approach. > involved in this discussion will be also. Additionally, as discussed > last summer, we can provide a migration path (which does not need to be > automated or even feature compatible) from pgAudit to an in-core > solution and then sunset pgAudit. Uh, that usually ends badly too. > Building an in-core solution, in my estimation at least, is going to > require at least a couple of release cycles and having the feedback from > users of pgAudit will be very valuable to building a good solution, but > I don't believe we'll get that feedback without including it. See above --- is it jump through the user hoops and only then they will use it and give us feedback? How motivated can they be if they can't use the PGXN version? The bottom line is that for the _years_ we ship pg_audit in /contrib, we will have some logging stuff in postgresql.conf and some in contrib/pg_audit and that distinction is going to look quite odd. To the extent you incrementally add to core, you will have duplicate functionality in both places. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce,
* Bruce Momjian (bruce@momjian.us) wrote:
> On Thu, May 7, 2015 at 10:26:55AM -0400, Stephen Frost wrote:
> > * Peter Eisentraut (peter_e@gmx.net) wrote:
> > > On 5/4/15 8:37 PM, Stephen Frost wrote:
> > > > I don't follow this logic. The concerns raised above are about changing
> > > > our in-core logging. We haven't got in-core auditing and so I don't see
> > > > how they apply to it.
> > >
> > > How is session "auditing" substantially different from statement logging?
> >
> > David had previously outlined the technical differences between the
> > statement logging we have today and what pgAudit does, but I gather
> > you're asking about it definitionally, though it ends up amounting to
> > much the same to me. Auditing is about "what happened" whereas
> > statement logging is "log whatever statement the user sent." pgAudit
> > bears this out by logging internal SQL statements and object
> > information, unlike what we do with statement logging today.
>
> Well, what I was looking for is how auditing is _conceptually_ different
> from logging, e.g. I can clearly explain how authentication (prove who
> you are) and authorization (what you are allowed to do) are different.
I'd say that auditing is one category of logging, just as
statement/session logging is another category of logging. What we
currently have in core is well defined and used extensively- but it's a
specific kind of logging which is statement logging, and that's all we
provide currently. That isn't to say that there isn't commonality,
there certainly is, but pgAudit leverages that to a large extent already
by using the logging infrastructure in the backend for everything which
is logged.
> Your definition above seems to be more behavioral, e.g. what arrived vs.
> what happened. It is not clear to me why reporting such information is
> conceptually different and requires different infrastructure, i.e. we
> could not easily combine authentication and authorization into the same
> infrastructure, but logging and auditing seems similar.
Similar to logging, Authentication and Authorization also fall under a
more general category- access control.
> > > At least no one has disputed that yet. The only argument against has
> > > been that they don't want to touch the logging.
> >
> > I'm afraid we've been talking past each other here- I'm fully on-board
> > with enhancing our in-core logging capabilities and even looking to the
> > future at having object auditing included in core. It's not my intent
> > to dispute that or to argue against it.
> >
> > Perhaps I've misunderstood the thrust of this sub-thread, so let me
> > explain what I thought the discussion was. My understanding was that
> > you were concerned about having session auditing included in pgAudit
> > and, further, that you wanted to see our in-core statement logging be
> > improved. I agree that we want to improve the in-core statement logging
> > and, ideally, have an in-core auditing solution in the future. I was
> > attempting to address the concern about having session logging in
> > pgAudit by pointing out that it's valuable to have even if our in-core
> > statement logging is augmented, and further, having it in pgAudit does
> > not preclude or reduce our ability to improve the in-core statement
> > logging in the future; indeed, it's my hope that we'll get good feedback
> > from users of pgAudit which could guide our in-core implementation. As
>
> What is our history of doing things in contrib because we are not sure
> what we want, then moving it into core? My general recollection is that
> there is usually something in the contrib version we don't want to add
> to core and people are locked into the contrib API, so we are left
> supporting it, e.g. xml2, though you could argue that auditing doesn't
> have application lock-in and xml2 was tied to an external library
> feature.
That's exactly the argument that I'd make there. My recollection is
that we did move pieces of hstore and have moved pieces of other contrib
modules into core; perhaps we've not yet had a case where we've
completely pulled one in, but given the relatively low level of
dependency associated with pgAudit, I'm certainly hopeful that we'll be
able to here. Lack of history which could be pointed to that's exactly
what I'm suggesting here doesn't seem like a reason to not move forward
here though; the concept of having a capability initially in contrib and
then bringing it into core has certainly been discussed a number of
times on other threads and generally makes sense, at least to me,
especially when there's little API associated with the extension.
> > for the concern that pgAudit may end up "rotting" in the tree as some
> > other contrib modules have, I can say with confidence that we will have
> > users of it just as soon as they're able to move to a version of PG
> > which includes it and therefore will be supporting it and addressing
> > issues as we discover them, as I suspect the others who have been
>
> Uh, why are they not using the PGXN version of pg_audit, and if it is
> because it isn't shipped with Postgres, then these seem like unmotivated
> users who will complain for some reason if we ever move it out of
> contrib.
Put bluntly, but I believe accurately, there's very few organizations
who have auditing requirements who want anything to do with PGXN.
That's an entirely understandable and defensible position, as there's
very little control in that environment (intentionally so, which is what
makes it great for some users but not acceptable for others). I'd
hardly call those users "unmotivated" considering that they've put forth
substantial resources towards pgAudit in the form of the EU grant which
started it over a year ago and the further efforts being made to bring
it to PG.
> I guess the over-arching question is whether we have to put this into
> contrib so we can get feedback and change the API, or whether using from
> PGXN or incrementally adding it to core is the right approach.
I'm surprised to hear this question of if we "have to" do X, Y, or Z.
pgAudit brings a fantastic capability to PostgreSQL which users have
been asking to have for many years and is a feature we should be itching
to have included. That we can then take it and incrementally add it to
core, to leverage things which are only available in core (as discussed
last summer, including grammar and relation metadata), looks to me like
a great direction to go in and one which we could use over and over to
bring new features and capabilities to PG.
Lack of auditing is one of the capabilities that users coming from other
large RDBMS's see as preventing their ability to migrate to PostgreSQL.
Other databases (open and closed source) have it and have had it for
years and it's a serious shortcoming of ours that makes users either
stick with their existing vendor or look to other closed-source or even
open-source solutions.
> > involved in this discussion will be also. Additionally, as discussed
> > last summer, we can provide a migration path (which does not need to be
> > automated or even feature compatible) from pgAudit to an in-core
> > solution and then sunset pgAudit.
>
> Uh, that usually ends badly too.
I'm confused by this, as it was the result of our discussion and your
suggestion from last summer: 20140730192136.GM2791@momjian.us
I certainly hope that hasn't substantially changed as that entire
discussion is why we're even able to have this discussion about
including pgAudit now. I was very much on-board with trying to work on
an in-core solution until that thread convinced me that the upgrade
concerns which I was worried about wouldn't be an issue for inclusion of
an extension to provide the capability.
> > Building an in-core solution, in my estimation at least, is going to
> > require at least a couple of release cycles and having the feedback from
> > users of pgAudit will be very valuable to building a good solution, but
> > I don't believe we'll get that feedback without including it.
>
> See above --- is it jump through the user hoops and only then they will
> use it and give us feedback? How motivated can they be if they can't
> use the PGXN version?
Why wouldn't we want to include this capability in PG? I also addressed
the "why not PGXN" above. It it not a lack of motivation but the entire
intent and design of the PGXN system which precludes most large
organizations from using it, particularly for sensitive requirements
such as auditing.
> The bottom line is that for the _years_ we ship pg_audit in /contrib, we
> will have some logging stuff in postgresql.conf and some in
> contrib/pg_audit and that distinction is going to look quite odd. To
> the extent you incrementally add to core, you will have duplicate
> functionality in both places.
That's entirely correct, of course, but I'm not seeing it as an issue.
I'm certainly prepared to support shipping pgAudit in contrib, as are
others based on how this feature has been developed, for the years that
we'll have 9.5, 9.6 (or 10.0, etc) supported- and that's also another
reason why users will use it when they wouldn't use something on PGXN.
Further, I look forward to working incrementally to bring similar
capability into core, but I suspect those increments will largely be in
the infrastructure until we reach the point where we're able to provide
the user-facing bits, which is quite likely to go in all at once and
allow us a clear upgrade path from one to the other. Perhaps that's
optimistic, but we do tend to try and bring things in as whole
capabilities rather than bits and pieces and I don't expect us to need
to do it differently here.
Thanks!
Stephen
On 5/7/15 10:26 AM, Stephen Frost wrote: > Auditing is about "what happened" whereas > statement logging is "log whatever statement the user sent." pgAudit > bears this out by logging internal SQL statements and object > information, unlike what we do with statement logging today. I don't think this is quite correct. For example, log_min_duration_statement logs based on what happened. log_duration records what happened. log_checkpoints records what happened. log_statement also requires parsing before deciding whether to log. Generally, "logging" is "what happened". The stuff in syslog is what happened on the system.
Peter,
* Peter Eisentraut (peter_e@gmx.net) wrote:
> On 5/7/15 10:26 AM, Stephen Frost wrote:
> > Auditing is about "what happened" whereas
> > statement logging is "log whatever statement the user sent." pgAudit
> > bears this out by logging internal SQL statements and object
> > information, unlike what we do with statement logging today.
>
> I don't think this is quite correct. For example,
> log_min_duration_statement logs based on what happened. log_duration
> records what happened. log_checkpoints records what happened.
> log_statement also requires parsing before deciding whether to log.
I'm not sure I agree, but it seems a relatively minor point (please
correct me if you feel differently). You're certainly correct that
log_min_duration_statement allows filtering of the statement logging
based on what happened, but it's still statement logging. The other log
options are more in-line with "what happened" kind of logging, but they
also aren't user activity, so perhaps rephrasing my statement along the
lines of "what happened based on user activity" would make more sense.
On the other hand, log_checkpoints isn't "statement" or "session"
logging, which is what we had been discussing, I thought.
I agree that log_duration is more in-line with "what happened".
> Generally, "logging" is "what happened". The stuff in syslog is what
> happened on the system.
Agreed, but I had thought we were primairly focusing on session /
statement logging, which is the potential overlap in capability being
discussed as related to pgAudit (I don't expect pgAudit to ever include
checkpoint logging, for example). My email to Bruce, I believe,
clarifies how I've been thinking about statement/session logging and the
more general category of "logging" (which auditing certainly falls under
also, as "audit logging").
Thanks!
Stephen
On Thu, May 7, 2015 at 03:41:13PM -0400, Stephen Frost wrote: > Bruce, > > What is our history of doing things in contrib because we are not sure > > what we want, then moving it into core? My general recollection is that > > there is usually something in the contrib version we don't want to add > > to core and people are locked into the contrib API, so we are left > > supporting it, e.g. xml2, though you could argue that auditing doesn't > > have application lock-in and xml2 was tied to an external library > > feature. > > That's exactly the argument that I'd make there. My recollection is > that we did move pieces of hstore and have moved pieces of other contrib > modules into core; perhaps we've not yet had a case where we've > completely pulled one in, but given the relatively low level of > dependency associated with pgAudit, I'm certainly hopeful that we'll be > able to here. Lack of history which could be pointed to that's exactly > what I'm suggesting here doesn't seem like a reason to not move forward > here though; the concept of having a capability initially in contrib and > then bringing it into core has certainly been discussed a number of > times on other threads and generally makes sense, at least to me, > especially when there's little API associated with the extension. OK, I am just asking so we remember this can go badly, not that it will. > > I guess the over-arching question is whether we have to put this into > > contrib so we can get feedback and change the API, or whether using from > > PGXN or incrementally adding it to core is the right approach. > > I'm surprised to hear this question of if we "have to" do X, Y, or Z. > pgAudit brings a fantastic capability to PostgreSQL which users have > been asking to have for many years and is a feature we should be itching > to have included. That we can then take it and incrementally add it to > core, to leverage things which are only available in core (as discussed > last summer, including grammar and relation metadata), looks to me like > a great direction to go in and one which we could use over and over to > bring new features and capabilities to PG. > > Lack of auditing is one of the capabilities that users coming from other > large RDBMS's see as preventing their ability to migrate to PostgreSQL. > Other databases (open and closed source) have it and have had it for > years and it's a serious shortcoming of ours that makes users either > stick with their existing vendor or look to other closed-source or even > open-source solutions. Yes, more extensive auditing is definitely needed. > > > involved in this discussion will be also. Additionally, as discussed > > > last summer, we can provide a migration path (which does not need to be > > > automated or even feature compatible) from pgAudit to an in-core > > > solution and then sunset pgAudit. > > > > Uh, that usually ends badly too. > > I'm confused by this, as it was the result of our discussion and your > suggestion from last summer: 20140730192136.GM2791@momjian.us > > I certainly hope that hasn't substantially changed as that entire > discussion is why we're even able to have this discussion about > including pgAudit now. I was very much on-board with trying to work on > an in-core solution until that thread convinced me that the upgrade > concerns which I was worried about wouldn't be an issue for inclusion of > an extension to provide the capability. I had forgotten about that. Yes, pg_upgrade can easily do this. > > > Building an in-core solution, in my estimation at least, is going to > > > require at least a couple of release cycles and having the feedback from > > > users of pgAudit will be very valuable to building a good solution, but > > > I don't believe we'll get that feedback without including it. > > > > See above --- is it jump through the user hoops and only then they will > > use it and give us feedback? How motivated can they be if they can't > > use the PGXN version? > > Why wouldn't we want to include this capability in PG? I also addressed > the "why not PGXN" above. It it not a lack of motivation but the entire > intent and design of the PGXN system which precludes most large > organizations from using it, particularly for sensitive requirements > such as auditing. So they trust the Postgres, but not the PGXN authors --- I guess legally that makes sense. > > The bottom line is that for the _years_ we ship pg_audit in /contrib, we > > will have some logging stuff in postgresql.conf and some in > > contrib/pg_audit and that distinction is going to look quite odd. To > > the extent you incrementally add to core, you will have duplicate > > functionality in both places. > > That's entirely correct, of course, but I'm not seeing it as an issue. > I'm certainly prepared to support shipping pgAudit in contrib, as are > others based on how this feature has been developed, for the years that > we'll have 9.5, 9.6 (or 10.0, etc) supported- and that's also another > reason why users will use it when they wouldn't use something on PGXN. > > Further, I look forward to working incrementally to bring similar > capability into core, but I suspect those increments will largely be in > the infrastructure until we reach the point where we're able to provide > the user-facing bits, which is quite likely to go in all at once and > allow us a clear upgrade path from one to the other. Perhaps that's > optimistic, but we do tend to try and bring things in as whole > capabilities rather than bits and pieces and I don't expect us to need > to do it differently here. OK, I just felt I had to ask those questions so we know where the pitfalls are --- over-optimism always sets of alarms for me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 5/1/15 5:58 AM, Sawada Masahiko wrote: > For now, since pg_audit patch has a pg_audit_ddl_command_end() > function which uses part of un-committed "deparsing utility commands" > patch API, > pg_audit patch is completely depend on that patch. > If API name, interface are changed, it would affect for pg_audit patch. > I'm not sure about progress of "deparsing utility command" patch but > you have any idea if that patch is not committed into 9.5 until May > 15? The attached v12 patch removes the code that became redundant with Alvaro committing the event trigger/deparse work. I've updated the regression tests to reflect the changes, which were fairly minor and add additional information to the output. There are no longer any #ifdef'd code blocks. -- - David Steele david@pgmasters.net
Вложения
On Mon, May 11, 2015 at 9:07 PM, David Steele <david@pgmasters.net> wrote:
> On 5/1/15 5:58 AM, Sawada Masahiko wrote:
>> For now, since pg_audit patch has a pg_audit_ddl_command_end()
>> function which uses part of un-committed "deparsing utility commands"
>> patch API,
>> pg_audit patch is completely depend on that patch.
>> If API name, interface are changed, it would affect for pg_audit patch.
>> I'm not sure about progress of "deparsing utility command" patch but
>> you have any idea if that patch is not committed into 9.5 until May
>> 15?
>
> The attached v12 patch removes the code that became redundant with
> Alvaro committing the event trigger/deparse work. I've updated the
> regression tests to reflect the changes, which were fairly minor and add
> additional information to the output. There are no longer any #ifdef'd
> code blocks.
This is not a full review, but just a few thoughts...
What happens if the server crashes? Presumably, audit records emitted
just before the crash can be lost, possibly even if the transaction
went on to commit. That's no worse than what is already the case for
regular logging, of course, but it's maybe a bit more relevant here
because of the intended use of this information.
+ if (audit_on_relation(relOid, auditOid, auditPerms))
+ {
+ auditEventStack->auditEvent.granted = true;
+ }
Braces around single-statement blocks are not PostgreSQL style.
I wonder if driving the auditing system off of the required
permissions is really going to work well. That means that decisions
we make about permissions enforcement will have knock-on effects on
auditing. For example, the fact that we check permission on a view
rather than on the underlying tables will (I think) flow through to
how the auditing happens.
+ stackItem->auditEvent.commandTag == T_DoStmt &&
Use IsA(..., DoStmt) for this kind of test. There are many instances
of this pattern in the patch; they should al be fixed.
Using auditLogNotice to switch the log level between LOG and NOTICE is
weird. Switching from LOG to NOTICE is an increase in the logging
level relative to client_min_messages, but a decrease relative to
log_min_messages. With default settings, logging at LOG will go only
to the log (not the client) and logging at NOTICE will go only to the
client (not the log).
The documentation and comments in this patch are, by my estimation,
somewhat below our usual standard. For example:
+ DefineCustomBoolVariable("pg_audit.log_notice",
+ "Raise a
notice when logging",
Granted, there is a fuller explanation in the documentation, but that
doesn't make that explanation particularly good. (One might also
wonder why the log level isn't fully configurable instead of offering
only two choices.)
Here's another example:
+ DefineCustomBoolVariable("pg_audit.log_parameter",
+ "Enable
statement parameter logging",
It would be far better to say "lnclude the values of statement
parameters in auditing messages" or something like that. It is quite
clear that this GUC doesn't enable some sort of general statement
parameter logging facility; it changes the contents of auditing
messages that would have been emitted either way.
I don't want to focus too much on this particular example. The
comments and documentation really deserve a bit more attention
generally than they have gotten thus far. I am not saying they are
bad. I am just saying that, IMHO, they are not as good as we
typically try to make them.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert, all,
* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, May 11, 2015 at 9:07 PM, David Steele <david@pgmasters.net> wrote:
> > The attached v12 patch removes the code that became redundant with
> > Alvaro committing the event trigger/deparse work. I've updated the
> > regression tests to reflect the changes, which were fairly minor and add
> > additional information to the output. There are no longer any #ifdef'd
> > code blocks.
>
> This is not a full review, but just a few thoughts...
Thanks for that. David and I worked through your suggestions, a number
of my own, and some general cleanup and I've now pushed it.
> What happens if the server crashes? Presumably, audit records emitted
> just before the crash can be lost, possibly even if the transaction
> went on to commit. That's no worse than what is already the case for
> regular logging, of course, but it's maybe a bit more relevant here
> because of the intended use of this information.
Right, if the server crashes then we may lose information- but there
should be a log somewhere of the crash. I didn't do much in the way of
changes to the documentation, but this is definitely an area where we
should make it very clear what happens.
> Braces around single-statement blocks are not PostgreSQL style.
Fixed those and a number of other things, like not having entire
functions in if() blocks.
> I wonder if driving the auditing system off of the required
> permissions is really going to work well. That means that decisions
> we make about permissions enforcement will have knock-on effects on
> auditing. For example, the fact that we check permission on a view
> rather than on the underlying tables will (I think) flow through to
> how the auditing happens.
The checks against the permissions are independent and don't go through
our normal permission checking system, so I'm not too worried about this
aspect. I agree that we need to be vigilant and consider the impact of
changes to the permissions system, but there are also quite a few
regression tests in pg_audit and those should catch a lot of potential
issues.
> + stackItem->auditEvent.commandTag == T_DoStmt &&
>
> Use IsA(..., DoStmt) for this kind of test. There are many instances
> of this pattern in the patch; they should al be fixed.
Unfortunately, that's not actually a Node, so we can't just use IsA. We
considered making it one, but that would mean IsA() would return a
T_DoStmt or similar for something that isn't actually a T_DoStmt (it's
an auditEvent of a T_DoStmt). Still, I did go through and look at these
cases and made changes to improve them and clean things up to be neater.
> The documentation and comments in this patch are, by my estimation,
> somewhat below our usual standard. For example:
>
> + DefineCustomBoolVariable("pg_audit.log_notice",
> + "Raise a
> notice when logging",
This was improved, but I'm sure more can be done. Suggestions welcome.
> Granted, there is a fuller explanation in the documentation, but that
> doesn't make that explanation particularly good. (One might also
> wonder why the log level isn't fully configurable instead of offering
> only two choices.)
This was certainly a good point and we added support for choosing the
log level to log it at.
> I don't want to focus too much on this particular example. The
> comments and documentation really deserve a bit more attention
> generally than they have gotten thus far. I am not saying they are
> bad. I am just saying that, IMHO, they are not as good as we
> typically try to make them.
I've done quite a bit of rework of the comments and will be working on
improving the documentation also.
Thanks!
Stephen