Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 11/21/15 2:47 PM, Noah Misch wrote: > On Fri, Nov 20, 2015 at 12:11:00PM -0500, David Steele wrote: >> I fixed many of the issues that caused complaints at the end of the 9.5 >> cycle, but there are still two remaining items I would want to address >> before resubmitting: >> >> 1) There was concern about audit messages being sent to the clients. >> I'm looking at adding an option to the ereport macro to suppress sending >> messages to the clients. I'll submit that patch soon. > > I don't object to audit messages being client-visible; did anyone feel that > was a blocker? I did make a related point, namely, the documentation must not > assert that you can hide messages from clients if you in fact cannot. I don't think anyone felt it was a blocker but it seemed nobody thought it was ideal, either. I certainly don't. The error in the documentation was unfortunate but that's what the review process is all about. I'm appreciative of everyone who had a look at the patch. -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Fri, Nov 20, 2015 at 12:11:00PM -0500, David Steele wrote: > I fixed many of the issues that caused complaints at the end of the 9.5 > cycle, but there are still two remaining items I would want to address > before resubmitting: > > 1) There was concern about audit messages being sent to the clients. > I'm looking at adding an option to the ereport macro to suppress sending > messages to the clients. I'll submit that patch soon. I don't object to audit messages being client-visible; did anyone feel that was a blocker? I did make a related point, namely, the documentation must not assert that you can hide messages from clients if you in fact cannot. > 2) I would like make install-check to work with modules that require > shared_preload_libraries. My understanding is that Andrew may have already > fixed this, but if not I'll look into it. It would be nice to standardize a pattern for that, agreed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Hi Thom, On 11/18/15 8:54 AM, Thom Brown wrote: On 10 June 2015 at 14:41, Noah Misch wrote: On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote: I've certainly had quite the experience as a first-time contributor working on this patch. Perhaps I bit off more than I should have and I definitely managed to ruffle a few feathers along the way. I learned a lot about how the community works, both the good and the bad. Fear not, though, I'm not so easily discouraged and you'll definitely be hearing more from me. Glad to hear it. The stated purpose of contrib is: "include porting tools, analysis utilities, and plug-in features that are not part of the core PostgreSQL system, mainly because they address a limited audience or are too experimental to be part of the main source tree. This does not preclude their usefulness." Perhaps we should consider modifying that language, because from my perspective pg_audit fit the description perfectly. "What is contrib?" attracts enduring controversy; see recent thread "RFC: Remove contrib entirely" for the latest episode. However that discussion concludes, that documentation passage is not too helpful as a guide to predicting contrib patch reception. (Most recent contrib additions had an obvious analogy to an existing module, sidestepping the question.) Is pg_audit being resubmitted for 9.6 at all? A number of people have asked me about this over the last few months. I would certainly be interested in resubmitting this for 9.6. I fixed many of the issues that caused complaints at the end of the 9.5 cycle, but there are still two remaining items I would want to address before resubmitting: 1) There was concern about audit messages being sent to the clients. I'm looking at adding an option to the ereport macro to suppress sending messages to the clients. I'll submit that patch soon. 2) I would like make install-check to work with modules that require shared_preload_libraries. My understanding is that Andrew may have already fixed this, but if not I'll look into it. -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 10 June 2015 at 14:41, Noah Misch wrote: > On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote: >> I've certainly had quite the experience as a first-time contributor >> working on this patch. Perhaps I bit off more than I should have and I >> definitely managed to ruffle a few feathers along the way. I learned a >> lot about how the community works, both the good and the bad. Fear not, >> though, I'm not so easily discouraged and you'll definitely be hearing >> more from me. > > Glad to hear it. > >> The stated purpose of contrib is: "include porting tools, analysis >> utilities, and plug-in features that are not part of the core PostgreSQL >> system, mainly because they address a limited audience or are too >> experimental to be part of the main source tree. This does not preclude >> their usefulness." >> >> Perhaps we should consider modifying that language, because from my >> perspective pg_audit fit the description perfectly. > > "What is contrib?" attracts enduring controversy; see recent thread "RFC: > Remove contrib entirely" for the latest episode. However that discussion > concludes, that documentation passage is not too helpful as a guide to > predicting contrib patch reception. (Most recent contrib additions had an > obvious analogy to an existing module, sidestepping the question.) Is pg_audit being resubmitted for 9.6 at all? Thom -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Tue, Jun 09, 2015 at 03:54:59PM -0400, David Steele wrote: > I've certainly had quite the experience as a first-time contributor > working on this patch. Perhaps I bit off more than I should have and I > definitely managed to ruffle a few feathers along the way. I learned a > lot about how the community works, both the good and the bad. Fear not, > though, I'm not so easily discouraged and you'll definitely be hearing > more from me. Glad to hear it. > The stated purpose of contrib is: "include porting tools, analysis > utilities, and plug-in features that are not part of the core PostgreSQL > system, mainly because they address a limited audience or are too > experimental to be part of the main source tree. This does not preclude > their usefulness." > > Perhaps we should consider modifying that language, because from my > perspective pg_audit fit the description perfectly. "What is contrib?" attracts enduring controversy; see recent thread "RFC: Remove contrib entirely" for the latest episode. However that discussion concludes, that documentation passage is not too helpful as a guide to predicting contrib patch reception. (Most recent contrib additions had an obvious analogy to an existing module, sidestepping the question.) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
David Steele writes: > My honest, albeit novice, opinion is that it was a mistake to pull > pg_audit from contrib. I know more than anyone that it had flaws, > mostly owing to its implementation as an extension, but it also provided > capability that simply does not exist right now. Recent conversations > about PGXN demonstrate why that is not (currently) a good alternative > for distributing extensions. That means pg_audit will have a more > limited audience than it could have had. That's a shame, because people > are interested in pg_audit, warts and all. FWIW, I did not think that the consensus was that pg_audit could never be in contrib. As I understood it, people felt that (1) the API might not be sufficiently stable yet, and (2) there might be parts that should be in core eventually. (2) is problematic only because we do not have very good tools for migrating things from contrib to core without creating user-visible compatibility issues; which is not pg_audit's fault but it's still a constraint we have to keep in mind. So I'd encourage you to keep working on it and trying to address the issues that were brought up. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Hi Noah, On 6/8/15 10:13 AM, Noah Misch wrote: > My condemnation of the pg_audit commits probably hurt you as the feature's > authors. I am sorry for that. Your code was better than most "Ready for > Committer" code, and I hope you submit more patches in the future. I appreciate you saying this and especially for saying it publicly. I've certainly had quite the experience as a first-time contributor working on this patch. Perhaps I bit off more than I should have and I definitely managed to ruffle a few feathers along the way. I learned a lot about how the community works, both the good and the bad. Fear not, though, I'm not so easily discouraged and you'll definitely be hearing more from me. My honest, albeit novice, opinion is that it was a mistake to pull pg_audit from contrib. I know more than anyone that it had flaws, mostly owing to its implementation as an extension, but it also provided capability that simply does not exist right now. Recent conversations about PGXN demonstrate why that is not (currently) a good alternative for distributing extensions. That means pg_audit will have a more limited audience than it could have had. That's a shame, because people are interested in pg_audit, warts and all. The stated purpose of contrib is: "include porting tools, analysis utilities, and plug-in features that are not part of the core PostgreSQL system, mainly because they address a limited audience or are too experimental to be part of the main source tree. This does not preclude their usefulness." Perhaps we should consider modifying that language, because from my perspective pg_audit fit the description perfectly. Of course, I understand this is a community effort and I don't expect every contribution to be accepted and committed. Consider me disappointed yet determined. -- - David Steele da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Ian, Abhijit, and David, My condemnation of the pg_audit commits probably hurt you as the feature's authors. I am sorry for that. Your code was better than most "Ready for Committer" code, and I hope you submit more patches in the future. Regards, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Heikki, * Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 05/28/2015 11:14 AM, Stephen Frost wrote: > >* Heikki Linnakangas (hlinn...@iki.fi) wrote: > >>1. it's not flexible enough. How do you specify that all READs on > >>super_secret table must be logged, but on less_secret table, it's > >>enough to log only WRITEs? > > > >This is actually what that pg_audit.role setting was all about. To do > >the above, you would do: > > > >CREATE ROLE pgaudit; > >SET pg_audit.role = pgaudit; > >GRANT SELECT ON TABLE super_secret TO pgaudit; > >GRANT INSERT, UPDATE, DELETE ON TABLE less_secret TO pgaudit; > > > >With this, all commands executed which require SELECT rights on the > >table super_secret are logged, and all commands execute which require > >INSERT, UPDATE, or DELETE on the less_secret table are logged. > > Ah, I see. Wow, that's really Rube Goldbergian. It's certainly not ideal. It was discussed back in January, iirc. > >>2. GUCs can be changed easily on-the-fly, and configured flexibly. > >>But that's not really what you want for auditing. You want to have a > >>clear specification in one place. You'd want it to be more like > >>pg_hba.conf than postgresql.conf. Or more like Mandatory Access > >>Control, rather than Discretionary Access Control. > > > >I certainly appreciate the MAC vs. DAC discussion here, but I don't > >believe our AUDIT capability should explicitly require restarts of PG to > >be adjusted. > > Sure, I didn't mean we should require a restart. Requiring SIGHUP > seems reasonable though. I wouldn't have any issue with that. > >>A separate config file in $PGDATA would be a better way to configure > >>this. You would then have all the configuration in one place, and > >>that file could have a more flexible format, with per-schema rules > >>etc. (Wouldn't have to implement all that in the first version, but > >>that's the direction this should go to) > > > >The difficulty with a separate config file is that we don't have any way > >of properly attaching information to the existing tables in the > >database- table renames, dropped columns, etc, all become an issue then. > > True. I wouldn't be too worried about that though. If you rename a > table, that hopefully gets flagged in the audit log and someone will > ask "why did you rename that table?". If you're paranoid enough, you > could have a catch-all rule that audits everything that's not > explicitly listed, so a renamed table always gets audited. The general 'catch-all' approach was how we approached pg_audit in general, so, yes, that's the kind of auditing we expect people to want (or, at least, we would need to support it). You're right, in some environments that may be workable, but then it also has to be entirely managed outside of the database, which would make it extremely difficult to use in many environments, if not impossible.. > Of course, you could still support a labeling system, similar to the > pg_audit.role setting and GRANTs. For example, you could tag tables > with a label like "reads_need_auditing", and then in the config > file, you could specify that all READs on tables with that label > need to be audited. I think security labels would be a better system > to abuse for that than GRANT. You'd want to just label the objects, > and specify the READ/WRITE etc. attributes in the config file. Using SECURITY LABELs is certainly an interesting idea. GRANT was chosen because, with it, we also get information regarding the action that the user wants to audit (select/insert/update/delete, etc), without having to build all of that independently with some custom structure in the SECURITY LABEL system. > Who do you assume you can trust? Is it OK if the table owner can > disable/enable auditing for that table? In an ideal world, you would segregate the rights which the table owner has from both the permission system and the audit system. This has come up a number of times already and is, really, an independent piece of work. Having the permissions and auditing controlled by the same group is better than having all three pieces controlled by the ownership role, but having three distinct groups would be preferred. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 05/28/2015 11:14 AM, Stephen Frost wrote: * Heikki Linnakangas (hlinn...@iki.fi) wrote: 1. it's not flexible enough. How do you specify that all READs on super_secret table must be logged, but on less_secret table, it's enough to log only WRITEs? This is actually what that pg_audit.role setting was all about. To do the above, you would do: CREATE ROLE pgaudit; SET pg_audit.role = pgaudit; GRANT SELECT ON TABLE super_secret TO pgaudit; GRANT INSERT, UPDATE, DELETE ON TABLE less_secret TO pgaudit; With this, all commands executed which require SELECT rights on the table super_secret are logged, and all commands execute which require INSERT, UPDATE, or DELETE on the less_secret table are logged. Ah, I see. Wow, that's really Rube Goldbergian. 2. GUCs can be changed easily on-the-fly, and configured flexibly. But that's not really what you want for auditing. You want to have a clear specification in one place. You'd want it to be more like pg_hba.conf than postgresql.conf. Or more like Mandatory Access Control, rather than Discretionary Access Control. I certainly appreciate the MAC vs. DAC discussion here, but I don't believe our AUDIT capability should explicitly require restarts of PG to be adjusted. Sure, I didn't mean we should require a restart. Requiring SIGHUP seems reasonable though. A separate config file in $PGDATA would be a better way to configure this. You would then have all the configuration in one place, and that file could have a more flexible format, with per-schema rules etc. (Wouldn't have to implement all that in the first version, but that's the direction this should go to) The difficulty with a separate config file is that we don't have any way of properly attaching information to the existing tables in the database- table renames, dropped columns, etc, all become an issue then. True. I wouldn't be too worried about that though. If you rename a table, that hopefully gets flagged in the audit log and someone will ask "why did you rename that table?". If you're paranoid enough, you could have a catch-all rule that audits everything that's not explicitly listed, so a renamed table always gets audited. Of course, you could still support a labeling system, similar to the pg_audit.role setting and GRANTs. For example, you could tag tables with a label like "reads_need_auditing", and then in the config file, you could specify that all READs on tables with that label need to be audited. I think security labels would be a better system to abuse for that than GRANT. You'd want to just label the objects, and specify the READ/WRITE etc. attributes in the config file. Who do you assume you can trust? Is it OK if the table owner can disable/enable auditing for that table? I'm certainly all about adding new capabilities to pg_audit, but, as discussed elsewhere, I believe we really want many of those same capabilities in core (eg: being able to send logs to different places; my thinking is a rabbitMQ type of store rather than email, but perhaps we could support both..) and that's what I'm hoping to work towards in the near future. Sure, adding features like sending logs to different places in core is totally reasonable, independently of pg_audit. (Or not, but in any case, it's orthogonal :-) ) - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
All, Replying to Heikki's email as it's quite late here and I want to respond. Barring any further commentary, I'm planning to pull pg_audit out tomorrow (it wouldn't be intelligent for me to attempt to do so now). I really do appreciate all of the excellent feedback and comments, the excellent discussion, and the honest concerns raised about it. I truely love being a member of this community and to be among such highly respected and intelligent individuals, even if, on occation, I may be a bit brash. Thanks again, and I look forward to many interesting and positive discussions in Ottawa. Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Heikki, * Heikki Linnakangas (hlinn...@iki.fi) wrote: > On 05/28/2015 06:04 AM, Joshua D. Drake wrote: > >On 05/27/2015 07:02 PM, Stephen Frost wrote: > >>regardless of if they are included in the main git repo > >>or not. Being in the repo represents the support of the overall > >>community and PGDG, which is, understandably in my view, highly valuable > >>to the organizations which look to PostgreSQL as an open source > >>solution for their needs. > > > >I can't get behind this. Yes, there is a mysticism about the "core" > >support of modules but it is just that "mysticism". People are going to > >run what the experts tell them to run. If pg_audit is that good the > >experts will tell people to use it... > > Yeah, there are many popular tools and extensions that people use > together with PostgreSQL. PostGIS, pgbouncer, slony, pgbarman etc. > The criteria for what should be in contrib, in core, or a 3rd party > extension is a contentious topic. Thanks! I certainly agree. PostGIS is relatively easy to reason about- there is an entire community with a long history there also. pgbouncer and the others are not quite to that level. I agree it's a contentious topic and perhaps has been overly stressed. > The argument here is basically that if something is in core, it's > officially supported and endorsed by the PGDG. If that's the > argument for putting this in contrib, then you have to ask yourself > if the PGDG community is really willing to endorse this. I'm hearing > a lot of pushback from other committers, which points to "no", even > if all their technical arguments and worries turn out to be wrong. That's absolutely a fair point and one which I take seriously. You're right to point out that, even if one committer is behind a particular feature, that others may not be and therefore it's not really community supported. Clearly there is some general risk over time that contrib modules and features will be abandoned by the original authors and have to be taken up by others committers, but if there isn't support for the initial version then that's pretty unlikely and it could certainly deteriorate the reputation which PostgreSQL has earned. > I wrote the above without looking at the code or the documentation. > I haven't followed the discussion at all. I'm now looking at the > documentation, and have some comments based on just that: Understood, and thanks for reviewing the documentation. I have to admit that, just based on the above (I've not read all of the below quite yet), you make an excellent argument and one which I understand and agree with regarding the position of this particular module. > * I think it's a bad idea for the audit records to go to the same > log as other messages. Maybe that's useful as an option, but in > general there is no reason to believe that the log format used for > general logging is also a good format for audit logging. You > probably wouldn't care about process ID for audit logging, for > example. Also, how do you prevent spoofing audit records with > something like "SELECT \nAUDIT: SESSION, 2, ...". Maybe the log > formatting options can be used to prevent that, but just by looking > at the examples in the manual, I don't see how to do it. I entirely agree with you here- the existing logging structure was used as there is not a trivial way to use another and still support file-based logging. We could limit pg_audit to syslog only or to only support some other mechanism, but that tends to be even further limiting than the existing logging system. As I mentioned in my response to Tom, this is absolutely an area which needs improvement. > * I don't understand how the pg_audit.role setting and the audit > role system works. No problem, I'm happy to explain it. Essentially, the 'role' set there is the role checked for as a target of GRANT commands, which are used as a proxy for AUDIT commands, as we can't add metadata to tables from extensions today. > * Using GUCs for configuring it seems like a bad idea, because: I certainly agree with this, we need much more flexibility than what GUCs can provide but that's not easily done from an extension. This was always a compromise between based on what capabilities are provided for extensions from core and GUCs tend to be an "easy" answer, though certainly not always the correct one. > 1. it's not flexible enough. How do you specify that all READs on > super_secret table must be logged, but on less_secret table, it's > enough to log only WRITEs? This is actually what that pg_audit.role setting was all about. To do the above, you would do: CREATE ROLE pgaudit; SET pg_audit.role = pgaudit; GRANT SELECT ON TABLE super_secret TO pgaudit; GRANT INSERT, UPDATE, DELETE ON TABLE less_secret TO pgaudit; With this, all commands executed which require SELECT rights on the table super_secret are logged, and all commands execute which require INSERT, UPDATE, or DELETE on the less_secret table are logged. > 2. GUCs ca
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 05/28/2015 06:04 AM, Joshua D. Drake wrote: On 05/27/2015 07:02 PM, Stephen Frost wrote: regardless of if they are included in the main git repo or not. Being in the repo represents the support of the overall community and PGDG, which is, understandably in my view, highly valuable to the organizations which look to PostgreSQL as an open source solution for their needs. I can't get behind this. Yes, there is a mysticism about the "core" support of modules but it is just that "mysticism". People are going to run what the experts tell them to run. If pg_audit is that good the experts will tell people to use it... Yeah, there are many popular tools and extensions that people use together with PostgreSQL. PostGIS, pgbouncer, slony, pgbarman etc. The criteria for what should be in contrib, in core, or a 3rd party extension is a contentious topic. The argument here is basically that if something is in core, it's officially supported and endorsed by the PGDG. If that's the argument for putting this in contrib, then you have to ask yourself if the PGDG community is really willing to endorse this. I'm hearing a lot of pushback from other committers, which points to "no", even if all their technical arguments and worries turn out to be wrong. I wrote the above without looking at the code or the documentation. I haven't followed the discussion at all. I'm now looking at the documentation, and have some comments based on just that: * I think it's a bad idea for the audit records to go to the same log as other messages. Maybe that's useful as an option, but in general there is no reason to believe that the log format used for general logging is also a good format for audit logging. You probably wouldn't care about process ID for audit logging, for example. Also, how do you prevent spoofing audit records with something like "SELECT \nAUDIT: SESSION, 2, ...". Maybe the log formatting options can be used to prevent that, but just by looking at the examples in the manual, I don't see how to do it. * I don't understand how the pg_audit.role setting and the audit role system works. * Using GUCs for configuring it seems like a bad idea, because: 1. it's not flexible enough. How do you specify that all READs on super_secret table must be logged, but on less_secret table, it's enough to log only WRITEs? 2. GUCs can be changed easily on-the-fly, and configured flexibly. But that's not really what you want for auditing. You want to have a clear specification in one place. You'd want it to be more like pg_hba.conf than postgresql.conf. Or more like Mandatory Access Control, rather than Discretionary Access Control. A separate config file in $PGDATA would be a better way to configure this. You would then have all the configuration in one place, and that file could have a more flexible format, with per-schema rules etc. (Wouldn't have to implement all that in the first version, but that's the direction this should go to) I recommend making pg_audit an external extension, not a contrib module. As an extension, it can have its own life-cycle and not be tied to PostgreSQL releases. That would be a huge benefit for pg_audit. There is a lot of potential for new features to be added: more flexible configuration, more details to be logged, more ways to log, email alerts, etc. As an extension, all of those things could be developed independent of the PostgreSQL life-cycle. If there is need to fix vulnerabilities or other bugs, those can also be fixed independently of PostgreSQL minor releases. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Tom, Many thanks for your comments and for jumping into this discussion. * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Joshua D. Drake (j...@commandprompt.com) wrote: > >> It seems to me that perhaps the solution is then to pull pg_audit > >> into user space and instead work on a general solution (an API? > >> custom backend?) that provides what is needed. > > > I am planning on working on the necessary changes to core to include > > auditing capabilities. Hopefully, that will go more smoothly. I do not > > believe that auditing should or even can be an external module, indeed, > > pg_audit was exactly that attempt and, even with significant resources > > put into it over the past year, it falls far short of what any > > organization who is familiar with the capabilities in other RDBMSs would > > expect. That doesn't mean that I feel it's bad- it's a step in the > > right direction, but it isn't a complete solution. > > I'm fairly confused by your line of argument. If auditing can be done > by non-core code, then there is no great urgency to have it as a contrib > module. If it can't be done by non-core code, then creating a contrib > module is just a dead end that will soon leave nothing behind except > backwards-compatibility problems. Our experience with pulling contrib > modules into core has not been good in that respect. I consider it similar to dblink and FDWs. dblink is non-core code which allows querying other servers, but is ultimately a dead-end as, hopefully, we will eventually replace all of its capabilities with proper FDW support. Does that mean that it was a poor move to include it? No; it provided an extremely useful capability which a lot of users have been quite happy with. There are warts all over it and limitations to what it can do, issues with how it doesn't have any kind of analyze like information, no way for optimization to consider how best to integrate the query being run through dblink, etc, but would I use it? Absolutely. Is it a dead-end? Certainly, it'll eventually be completely replaced by FDWs. > As far as I can tell, pg_audit at this point is most charitably described > as "experimental", and I'm not sure that we want to put it into contrib > on that basis. Of late we've generally acted as though contrib modules > have the same kind of cross-version compatibility expectations that core > code does. It seems to me that that sort of promise is *way* premature > in this case; but I'm not seeing any large red warnings in pgaudit.sgml > that the described facilities are subject to change. You're correct- we should probably be putting something into the documentation which warns that the interfaces and auditing capabilities are likely to change in the future. On the other hand, I have been understandably chided regarding documentation changes which attempt to predict the future (see: changing include_realm). Further, as was discussed earlier this year, should we have a close enough match between pg_audit and in-core auditing support (something I'm certainly hopeful for- see the discussion about just how close the GRANT syntax used is to the AUDIT command in a certain very large RDBMS whose name starts with 'O'), we could provide a perl script or similar to facilitate the migration for users of the module to an in-core solution. > Quite aside from the question of whether we're ready to put a stake in the > ground about user-visible features of an audit facility, there seem to be > a lot of technical issues here: > > * Do we have adequate hooks in the backend with which auditing code can > detect events of interest (with acceptably low overhead)? I dunno, but > if we do not, having a contrib module doesn't fix it. I certainly agree with you here. The hooks in the Executor and ProcessUtility really aren't *ideal*, but it turns out that they actually are *sufficient* in a large number of cases. The addition of the event triggers/deparse code has helped that a great deal because it provides information which we weren't able to get previously. That was a rather late addition to the tree overall and I believe lead to the issue found regarding CREATE SCHEMA; it's not unreasonable for integration of such large changes like these late in the release cycle to least to an issue or two. There was quite a bit of consideration over the auditing information which was being collected, starting from the module as proposed last year around this time which covered one set of auditing, to the module as it is today which provides a somewhat different but largely overlapping and significantly more granular set of auditing information. Certainly, operating through the hooks and with the other capabilities offered by the extension system has been difficult and the module, as a whole, would be far simpler if in core, but I'm of the firm belief that the proposed extension will address many use-cases, not address many others, and provide extremely use
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 05/27/2015 07:02 PM, Stephen Frost wrote: JD, As it is currently an extension, it does not need to be in core. If this extension reaches a point where it needs to be in core to achieve some level of integration not currently provided then we can evaluate that problem. Currently, that is not the case. This simply doesn't make sense- all extensions can hook into the backend in the same way, That is exactly my point. regardless of if they are included in the main git repo or not. Being in the repo represents the support of the overall community and PGDG, which is, understandably in my view, highly valuable to the organizations which look to PostgreSQL as an open source solution for their needs. I can't get behind this. Yes, there is a mysticism about the "core" support of modules but it is just that "mysticism". People are going to run what the experts tell them to run. If pg_audit is that good the experts will tell people to use it... Just look at your own experience with pgBackrest. I am aware that pgBackrest is not an extension but it is a relatively new open source project that is gaining community based on its quality and contribution. It is arguably the best backup software available for Pg. It is not in core, it is not shipped with core. Your extension could be the best audit software available for Pg. It is not in core, because we don't need it to be. (slogan) JD Thanks! Stephen -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. Announcing "I'm offended" is basically telling the world you can't control your own emotions, so everyone else should do it for you. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Stephen Frost writes: > * Joshua D. Drake (j...@commandprompt.com) wrote: >> It seems to me that perhaps the solution is then to pull pg_audit >> into user space and instead work on a general solution (an API? >> custom backend?) that provides what is needed. > I am planning on working on the necessary changes to core to include > auditing capabilities. Hopefully, that will go more smoothly. I do not > believe that auditing should or even can be an external module, indeed, > pg_audit was exactly that attempt and, even with significant resources > put into it over the past year, it falls far short of what any > organization who is familiar with the capabilities in other RDBMSs would > expect. That doesn't mean that I feel it's bad- it's a step in the > right direction, but it isn't a complete solution. I'm fairly confused by your line of argument. If auditing can be done by non-core code, then there is no great urgency to have it as a contrib module. If it can't be done by non-core code, then creating a contrib module is just a dead end that will soon leave nothing behind except backwards-compatibility problems. Our experience with pulling contrib modules into core has not been good in that respect. As far as I can tell, pg_audit at this point is most charitably described as "experimental", and I'm not sure that we want to put it into contrib on that basis. Of late we've generally acted as though contrib modules have the same kind of cross-version compatibility expectations that core code does. It seems to me that that sort of promise is *way* premature in this case; but I'm not seeing any large red warnings in pgaudit.sgml that the described facilities are subject to change. Quite aside from the question of whether we're ready to put a stake in the ground about user-visible features of an audit facility, there seem to be a lot of technical issues here: * Do we have adequate hooks in the backend with which auditing code can detect events of interest (with acceptably low overhead)? I dunno, but if we do not, having a contrib module doesn't fix it. * Do we have an adequate design for specifying which out of the possible auditable events should be logged? I dunno about this either, but it seems like this is an area best kept out of core if at all possible. The design space seems pretty vast and I doubt that one size will fit all, or needs to fit all. * Do we have an appropriate mechanism for performing logging of events that we've decided to log? Here I think the answer is unquestionably "no"; basing audit logging on the existing elog mechanism with no changes is simply broken. elog is designed to be flexible about whether messages get reported and to where, which is exactly what you do *not* want for audit logs. This might not be too hard to fix, eg add another elevel with hard-wired rules about where it goes ... but the current patch didn't do that. A larger problem is that maybe the audit message stream shouldn't go to the postmaster log in the first place, but someplace else. * Do we have an appropriate mechanism for configuring the audit facility? I'm not totally sure, but again I think that the existing GUC mechanisms were not designed for this sort of thing and are probably too easy to subvert. (It might be hopeless to try to ensure that superusers can't escape auditing, but as it stands pg_audit doesn't even pretend to try. Even without the possibility of intentional subversion, there are too many ways to turn auditing off by accident.) On the whole, I think sticking this into contrib is premature. What it does today could be done just as well as a third-party extension. What it doesn't do today, we should work on, but I'm unconvinced that having this in contrib will make it easier to get there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
2015-05-28 3:30 GMT+02:00 Joshua D. Drake : > > On 05/27/2015 06:11 PM, Stephen Frost wrote: > > Thank you for your honest comments and your concern. >> >> I sincerely hope you're able to be involved in developing auditing for >> PostgreSQL in the future, as it is a key requirement in any secure >> environment. >> > > > Guys, > > I think we are overlooking the rather obvious elephant in the room. This > is an extension. There is no reason for it to be in core. Revert the patch, > gain independence, the ability to innovate mid-cycle and move on to bigger > fish. > any work in contrib has bigger progress. I wrote plpgsql_check - and the living out of contrib is neutral - I can do some changes freely, but it is used 1% users who can has benefit from this extension. 9.5 will be released after three months and lot of potential bugs can be fixed. It is contrib - it can be moved, removed, but better if can works. Regards Pavel > > Sincerely, > > JD > > > -- > Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 > PostgreSQL Centered full stack support, consulting and development. > Announcing "I'm offended" is basically telling the world you can't > control your own emotions, so everyone else should do it for you. > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
JD, * Joshua D. Drake (j...@commandprompt.com) wrote: > On 05/27/2015 06:38 PM, Stephen Frost wrote: > >While I certainly appreciate the support, I don't believe auditing will > >be able to work as an extension over the long term and if the community > >is unwilling or unable to accept steps in that direction through contrib > >modules or even changes to core to improve what we are able to provide > >in this area, > > It seems to me that perhaps the solution is then to pull pg_audit > into user space and instead work on a general solution (an API? > custom backend?) that provides what is needed. I am planning on working on the necessary changes to core to include auditing capabilities. Hopefully, that will go more smoothly. I do not believe that auditing should or even can be an external module, indeed, pg_audit was exactly that attempt and, even with significant resources put into it over the past year, it falls far short of what any organization who is familiar with the capabilities in other RDBMSs would expect. That doesn't mean that I feel it's bad- it's a step in the right direction, but it isn't a complete solution. > >I have very serious doubts about the willingness of > >organizations (particularly those in the financial and government space) > >to continue to seek out and support PostgreSQL as a viable open source > >alternative to the commerical RDBMS's which have had these capabilities > >for years. > > This may or may not be true considering and I am not sure it really > matters in the context of this argument. I'm confused by this comment. I've had discussions with both financial and government organizations who are interested in this capability and have been assured that they are interested in the PostgreSQL community building and supporting this, and are conversely utterly disinterested in either commerical or open-source products put out by individual organizations which can come and go. In my mind, this simply isn't a question. I wish that everyone could have the same experiences that I have had and to have been in the discussions that I've been in, but that's simply impractical and I would hope that my years in this community would count in my favor when I make these statements. > >I'm, again, not suggesting that a contrib module is going to be a > >workable long-term solution for all use-cases, but it would solve quite > >a few and would be known to be supported, and to have the support of the > >community, if released as part of PostgreSQL. > > If the demand for this module is there, it will receive the support > it needs regardless if it is in core. Please see above as I believe it addresses this point. > >These are extremely > >serious organizations who care about the reputation of PostgreSQL and > >the community for delivering quality software. I certainly have no > >intention to tarnish that in any way as it would be quite detrimental to > >myself and the community. If that means reverting a patch of my own, or > >one which I have supported, so be it. > > I can not speak to the quality of the patch. I can speak to what > others, people of repute in this community speak of this patch. The > leaning tower seems as if it is clearly in the, "we might want to > think about reverting this". I have been doing my best to follow all of the comments and concerns raised regarding this patch. I'm not sure that I share the optimism that you express in the quote above, but if two other committers feel that the feature needs to be reverted, then I will revert it. That is not a documented and formal policy in the community, as far as I'm aware, but it strikes me as reasonable, in the absence of any obvious collusion or hidden agendas. Should another committer speak up regarding the patch, perhaps things would change, but I have no disillusionment that such will happen in this case and have accepted it. > As it is currently an extension, it does not need to be in core. If > this extension reaches a point where it needs to be in core to > achieve some level of integration not currently provided then we can > evaluate that problem. Currently, that is not the case. This simply doesn't make sense- all extensions can hook into the backend in the same way, regardless of if they are included in the main git repo or not. Being in the repo represents the support of the overall community and PGDG, which is, understandably in my view, highly valuable to the organizations which look to PostgreSQL as an open source solution for their needs. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 05/27/2015 06:38 PM, Stephen Frost wrote: While I certainly appreciate the support, I don't believe auditing will be able to work as an extension over the long term and if the community is unwilling or unable to accept steps in that direction through contrib modules or even changes to core to improve what we are able to provide in this area, It seems to me that perhaps the solution is then to pull pg_audit into user space and instead work on a general solution (an API? custom backend?) that provides what is needed. I have very serious doubts about the willingness of organizations (particularly those in the financial and government space) to continue to seek out and support PostgreSQL as a viable open source alternative to the commerical RDBMS's which have had these capabilities for years. This may or may not be true considering and I am not sure it really matters in the context of this argument. I'm, again, not suggesting that a contrib module is going to be a workable long-term solution for all use-cases, but it would solve quite a few and would be known to be supported, and to have the support of the community, if released as part of PostgreSQL. If the demand for this module is there, it will receive the support it needs regardless if it is in core. These are extremely serious organizations who care about the reputation of PostgreSQL and the community for delivering quality software. I certainly have no intention to tarnish that in any way as it would be quite detrimental to myself and the community. If that means reverting a patch of my own, or one which I have supported, so be it. I can not speak to the quality of the patch. I can speak to what others, people of repute in this community speak of this patch. The leaning tower seems as if it is clearly in the, "we might want to think about reverting this". As it is currently an extension, it does not need to be in core. If this extension reaches a point where it needs to be in core to achieve some level of integration not currently provided then we can evaluate that problem. Currently, that is not the case. Sincerely, Joshua D. Drake Thanks! Stephen -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. Announcing "I'm offended" is basically telling the world you can't control your own emotions, so everyone else should do it for you. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
JD, * Joshua D. Drake (j...@commandprompt.com) wrote: > On 05/27/2015 06:11 PM, Stephen Frost wrote: > >Thank you for your honest comments and your concern. > > > >I sincerely hope you're able to be involved in developing auditing for > >PostgreSQL in the future, as it is a key requirement in any secure > >environment. > > I think we are overlooking the rather obvious elephant in the room. > This is an extension. There is no reason for it to be in core. > Revert the patch, gain independence, the ability to innovate > mid-cycle and move on to bigger fish. While I certainly appreciate the support, I don't believe auditing will be able to work as an extension over the long term and if the community is unwilling or unable to accept steps in that direction through contrib modules or even changes to core to improve what we are able to provide in this area, I have very serious doubts about the willingness of organizations (particularly those in the financial and government space) to continue to seek out and support PostgreSQL as a viable open source alternative to the commerical RDBMS's which have had these capabilities for years. I'm, again, not suggesting that a contrib module is going to be a workable long-term solution for all use-cases, but it would solve quite a few and would be known to be supported, and to have the support of the community, if released as part of PostgreSQL. These are extremely serious organizations who care about the reputation of PostgreSQL and the community for delivering quality software. I certainly have no intention to tarnish that in any way as it would be quite detrimental to myself and the community. If that means reverting a patch of my own, or one which I have supported, so be it. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On 05/27/2015 06:11 PM, Stephen Frost wrote: Thank you for your honest comments and your concern. I sincerely hope you're able to be involved in developing auditing for PostgreSQL in the future, as it is a key requirement in any secure environment. Guys, I think we are overlooking the rather obvious elephant in the room. This is an extension. There is no reason for it to be in core. Revert the patch, gain independence, the ability to innovate mid-cycle and move on to bigger fish. Sincerely, JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. Announcing "I'm offended" is basically telling the world you can't control your own emotions, so everyone else should do it for you. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Wed, May 27, 2015 at 04:36:53PM -0400, Stephen Frost wrote: > > This list includes all issues that I've > > identified from the commits done to master, comments made by the > > numerous individuals who have taken time to look at the patch and > > comment, and those found by the ongoing work of David and I (which has > > continued after the commit). If you both still feel that these are > > indicative of being just 'the tip of the iceberg', then I'll revert it. > > This list is just the tip of the iceberg. Thank you for your honest comments and your concern. I sincerely hope you're able to be involved in developing auditing for PostgreSQL in the future, as it is a key requirement in any secure environment. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Wed, May 27, 2015 at 04:36:53PM -0400, Stephen Frost wrote: > This list includes all issues that I've > identified from the commits done to master, comments made by the > numerous individuals who have taken time to look at the patch and > comment, and those found by the ongoing work of David and I (which has > continued after the commit). If you both still feel that these are > indicative of being just 'the tip of the iceberg', then I'll revert it. This list is just the tip of the iceberg. > I do not ask that you provide any further explanation as it's ultimately > a judgement call. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Robert, * Robert Haas (robertmh...@gmail.com) wrote: > I note that there are already 11 followup commits fixing bugs in > pg_audit, including 7 in the first 24 hours. It's usually not a good > thing when you can get the regression tests for a piece of > platform-independent code to pass in less than 8 tries. I suspect, > but do not know for certain, that this is just the tip of the iceberg. Having many commits immediately following a patch going in is certainly a reasonable reason to be concerned. What may not have been apparent is that all but one of those were specifically due to issues with how the regression tests were being run rather than any issues in the code. In an attempt to provide clarity and bring an objective view to the issues which have been identified in the pg_audit code since it was committed, I've developed the following list, which I kindly ask you and Noah to review objectivly. This list includes all issues that I've identified from the commits done to master, comments made by the numerous individuals who have taken time to look at the patch and comment, and those found by the ongoing work of David and I (which has continued after the commit). If you both still feel that these are indicative of being just 'the tip of the iceberg', then I'll revert it. I do not ask that you provide any further explanation as it's ultimately a judgement call. Note that I intentionally filtered out documentation and comment typo changes (either committed or planned), and the issues which were caused by how the regression tests were being run, as those are not germane to the code quality concerns. I would ask others to comment, but I can, understandably, appreciate that they would prefer to stay out of a conversion with such poor tone. I've attempted to order the list by severity, though that's clearly up for some debate, so take it for what you feel it is worth. SPI query qualify - Noah This is absolutely an issue, but we are kidding ourselves if we believe that this will never happen again. A holistic solution to this issue is needed. CREATE SCHEMA .. GRANT - Noah This is definitely an issue which needs to be addressed, but I don't believe this is an issue with the approach used (trusting event triggers to cover everything under CREATE SCHEMA). The event trigger is correctly collecting the GRANT command and pg_audit is outputting a record based on that (when DDL is enabled), but it's not detecting that the subcommand is a GRANT. That's certainly not great, but it's not an insurmountable problem either- correct the early exit logic, pull the command tag from pg_event_trigger_ddl_commands and use it. Portability issue wrt %ld - Tom Clearly an issue, but one which the buildfarm is specifically intended to catch and address. Note that this is the only code-level issue found by the buildfarm- all of the other commits associated with the buildfarm were due to how the regression tests were being run. T_AlterDefaultPrivilegesStmt classification fix - Noah This should clearly be ROLE instead of DDL. Use ereport() instead of elog() - Tom Clearly better, but I don't see how this could be exploited or necessairly rises to the level of 'bug', but I included it here for completeness. Reclassify REINDEX - Fujii This was done to match what is done in core regarding REINDEX. Note that it was intentionally set to DDL, and documented accordingly, as we felt the comment in the core code was correct, but, even so, we agreed with Fujii that we should probably match what core actually does here regardless. Suppress STATEMENT output - Fujii Makes sense but is just about reducing the amount of log traffic. Suppress CONTEXT output - David Similar to the STATEMENT case above, creates unnecessary noise in the logs. Further classification questions - Fujii These questions will undoubtably come up and there isn't one answer here but rather a case where we simply need to decide on something and document it accordingly. Remove ERROR, PANIC, and FATAL from log_level options - David This is a SUSET variable, so there isn't a particular security risk here, but it doesn't make sense to include these options. I'm hopeful, as I'm sure you understand, that the number of issues being reported is due principally to the amount of review and testing which has happened on this module since it went in and not because the quality of the code is particularly poor. I feel it's far more than happens for many commits, even ones which are backpatched, and I'm certainly hopeful that it makes for a better end result. I certainly do not intend to ask the community to accept a patch which is bugridden or full of holes, nor will I stand in the way of the community requesting that a feature be reverted. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Wed, May 27, 2015 at 9:24 AM, Stephen Frost wrote: > I agree that the documentation needs improvement. I don't agree with > your assessment that the code quality is well below the level we > normally expect, as committed. That's clearly a difficult thing to > judge, hence my compromise proposal to ensure that it either has a full > review or gets reverted and not included in a released version. That's not really acceptable in my view. I looked at it shortly before it was committed and said that it did not appear to me to be close to being ready. Noah took a look at it shortly after it was committed and said that it did not appear to him to be close to being ready. Apparently, that's not good enough for you. Your "compromise proposal" is that a third committer other than yourself should go look at it. But that's not the way our community works. It's hard to get one extra committer to look at most patches, let alone three. Committer bandwidth is too precious for that. Parallel sequential scan would be in this release but for the fact that Andres wasn't confident in certain portions of it, and I respected that by not committing even though not a single other person backed up his concerns. Basically, you're attempting to place the onus on other committers to find the bugs in your patch. If a third committer DOES come along and review it, you'll fix whatever they find and say "well, now it doesn't need to be reverted". That's just not right. As a committer, you are responsible for getting it right the first time, not committing stuff that is seriously broken and then cleaning it up as the issues are reported, or when you have time. If everyone adopted the approach you're taking here, we'd have an order of magnitude more serious bugs in a typical major release (and I would quit the project). I note that there are already 11 followup commits fixing bugs in pg_audit, including 7 in the first 24 hours. It's usually not a good thing when you can get the regression tests for a piece of platform-independent code to pass in less than 8 tries. I suspect, but do not know for certain, that this is just the tip of the iceberg. > I find it confusing that there is no appreciation here for the level of > interest and excitment which has happened around these features, or how > having these capabilities will grow our community, or how working with > David and others to develop new PostgreSQL developers who may some day > become committers and continue to carry PostgreSQL forward long past > when we are gone is a worthwhile goal. Those things are true of many patches. They don't excuse committing poor code or stream-rolling community process. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
* Noah Misch (n...@leadboat.com) wrote: > The test case demonstrated a hole in GRANT auditing, and your diagnosis is > incorrect. GRANT statements aren't subject to event triggers. Selecting DDL > too, with pg_audit.log=all, does not audit the GRANT substatement. GRANT statements are subject to event triggers in the most recent code- see ExecGrantStmt_oids, src/backend/catalog/aclchk.c:592 and EventTriggerCollectGrant(), src/backend/commands/event_trigger.c:1822. Perhaps I've missed something in my analysis, but I don't believe you're correct. I can certainly understand the confusion as GRANT statement which are granting *roles* are not subject to event triggers, as they are shared objects and therefore outside the scope of what event triggers support, but they are also not supported by the CREATE SCHEMA command. > pg_audit already has enough demonstrated bugs to be the most defective commit > I have ever studied. Its defect level, routine for a mere Ready for Committer > patch, is unprecedented among committed work. If that fails to astonish, look > at you continuing to _defend pg_audit's integrity_: I truely hope you're able to review more patches. I certainly would appreciate any further insight you can give me on issues beyond the three which you found. > > I don't believe that this module is particularly more bug-ridden than > > other contrib modules or even parts of core. > > Your negligence with respect to this commit calls into question every one of > your past commits and anything you might commit for years to come. I stand by the commits that I've made. There have been ones I've reverted, ones that I've realized were a mistake and have subsequently apologized for and worked to address, and others. I am certainly not above question, nor do I feel that I'm any more able to commit bugless patches than the next committer. I look forward to reviews of my past and future commits. > > I certainly welcome review from others and if there is not another > > committer-level formal review before we get close on 9.5 (say, end of > > August), then I'll revert it. There is certainly no concern that doing > > so would be difficult to do, as it is entirely self-contained. > > Not good enough. I need those would-be reviewers' help reviewing ~100 other > sfrost commits before 9.5 final. It was my understanding that we do not direct what committers or reviewers work on. Still, I encourage you to do so and, further, to review the work authored by myself and committed by others, along with the work I've done for pginfra. All-in-all, this ~1800 line contrib module strikes me as a relatively modest amount of work relative to the role system and RLS. I'm already planning to put resources (beyond myself) into review of what's been committed to 9.5 over the next few months, including commits that I've made, but more eyes are certainly welcome. To be clear, I'm not saying that I will not revert this, but I am trying to respond to the concerns raised in the best possible way that I can. I had been hoping that there would be some attempt to explain to me why you feel that the module is bug ridden and the worst commit you've ever seen. I can certainly understand concern with the GRANT-based design, or with ereport() being used for the audit log and how those aren't ideal and I could respect an argument being made that it's not acceptable for us to provide even a contrib module which uses that approach, but this attack is certainly not what I was expecting nor do I feel it's appropriate for this community. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Tue, May 26, 2015 at 08:10:04PM -0400, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: > > > + /* > > > + * Don't audit substatements. All the substatements we care about > > > should > > > + * be covered by the event triggers. > > > + */ > > > + if (context <= PROCESS_UTILITY_QUERY && > > > !IsAbortedTransactionBlockState()) > > > > They aren't covered. A GRANT inside CREATE SCHEMA escapes auditing: > > Actually, they are covered. David and I discussed this extensively, > prior to your review, and concluded that this approach works because the > items not under the EventTrigger charter are shared catalogs, updates to > which wouldn't ever make sense to happen under a CREATE SCHEMA. I do > hope that we are able to improve on EventTriggers by supporting them for > shared catalogs one day, but that is a discussion for another thread. > > The issue which you discovered here is that GRANTs were categorized > under CLASS_ROLE, but we have a check at the top of the DDL event > trigger which does an early-exit if DDL isn't selected for inclusion. > That clearly needs to be changed, so that the GRANT under CREATE SCHEMA > is caught and logged properly. The test case demonstrated a hole in GRANT auditing, and your diagnosis is incorrect. GRANT statements aren't subject to event triggers. Selecting DDL too, with pg_audit.log=all, does not audit the GRANT substatement. > > Stephen, please revert this feature. Most pg_audit bugs will create > > security > > vulnerabilities, so incremental development in the PostgreSQL tree is the > > wrong approach. The feature needs substantial hacking, then an additional > > committer's thorough review. (The above sampling of defects and weak areas > > was not a thorough review.) pg_audit already has enough demonstrated bugs to be the most defective commit I have ever studied. Its defect level, routine for a mere Ready for Committer patch, is unprecedented among committed work. If that fails to astonish, look at you continuing to _defend pg_audit's integrity_: > I don't believe that this module is particularly more bug-ridden than > other contrib modules or even parts of core. Your negligence with respect to this commit calls into question every one of your past commits and anything you might commit for years to come. > I certainly welcome review from others and if there is not another > committer-level formal review before we get close on 9.5 (say, end of > August), then I'll revert it. There is certainly no concern that doing > so would be difficult to do, as it is entirely self-contained. Not good enough. I need those would-be reviewers' help reviewing ~100 other sfrost commits before 9.5 final. nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Robert, Thank you for your response. * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, May 26, 2015 at 8:10 PM, Stephen Frost wrote: > > I certainly welcome review from others and if there is not another > > committer-level formal review before we get close on 9.5 (say, end of > > August), then I'll revert it. There is certainly no concern that doing > > so would be difficult to do, as it is entirely self-contained. > > Like Noah, I'm unhappy that this patch went in. I stated a few days > before it was committed that I felt the code quality and documentation > were well below the level we normally expect. Your response was to do > a little more work on the patch and then commit it without so much as > reposting it. Clearly, we have completely different ideas about what > constitutes an appropriate response to the sort of comments I sent. Just to be clear, I understand you're referring to this email: CA+TgmobtrK9ndfBeAhui1KDKpnUp4cNv07KukD73jOAfd=4...@mail.gmail.com David and I both spent a great deal of time reviewing that email and working to address your comments. I had David do as much of it as I felt that I could, as it is a learning experience for him as he works to become another dedicated PostgreSQL developer for the community. He then passed it on to me, and I took it further and he reviewed the results of my changes. To clarify, that email noted style issues (which I worked to fix, though I should have run pgindent, of course.. the discussed 'make indent' option would help a great deal with that), concerns about the design limitations (server crashes, using the permission system), a similar comment regarding where log messages go to the comment in the code which has had much discussion due to it, and issues with the documentation and comments. I did spend a good bit of time working to improve the comments, but did not focus on the documentation as that could be done post-feature freeze. Your comment at the bottom of that email and the subsequent lack of any other feedback from you or others regarding concern when I voiced my intention to work on pg_audit for 9.5 in response to Tom's summary thread lead me to believe that a full review by a qualified committer, with appropriate improvement to the comments and code, would satisfy the process requirements to move forward. There was no hidden agenda here or an attempt to hold off until the last minute, nor was it any secret that I was working on it. There was another, much larger, feature added to core, rather than simply a contrib module, which I have concerns may have issues too, but I don't believe that there was any problem with the process there either. The concerns were raised by one committer, a review and quite large rework was done by another, in conjunction with the author, and then the patch was committed. I agree that the documentation needs improvement. I don't agree with your assessment that the code quality is well below the level we normally expect, as committed. That's clearly a difficult thing to judge, hence my compromise proposal to ensure that it either has a full review or gets reverted and not included in a released version. > A near-record number of committers have objected to this patch in all > kinds of different ways. There have been concerns about process, > concerns about code quality, and concerns about whether this is really > something that we want. The normal way that works is that you either > (a) revise the patch or (b) try to convince the person raising the > concern that it's OK. The concern is addressed when the person who > raised it says it is, and not just because you thought about it and > decided it was OK. This can be taken too far in the other direction, > but we're not close to that in this instance. I worked to address all of those concerns, including those raised by the individuals you noted in our private discussion, all of whom have been silent on this since before it went in. There was clear agreement by a number of individuals on the need for the broader capability, but I don't believe that's going to spring forth and land in the tree in one fell swoop. This is a good step in that direction. The patch was revised numerous times and I did work to convince the individuals raising concerns that it was OK. I don't believe there is any doubt about either of those, but I would be happy to go through the archives and point out why I feel that my statements are accurate. > I am particularly troubled by the fact that what has happened with > this patch is very much like what happened with row-level security: a > patch that clearly wasn't finished and clearly had not had adequate > review got abruptly committed - by you - without any consensus so to > do. It seems likely to me that by now row-level security has had > enough review that it is solid enough to be in the tree - in > particular, I am encouraged by Dean Rasheed's work to improve the > patch. However, it is ab
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Tue, May 26, 2015 at 8:10 PM, Stephen Frost wrote: > I certainly welcome review from others and if there is not another > committer-level formal review before we get close on 9.5 (say, end of > August), then I'll revert it. There is certainly no concern that doing > so would be difficult to do, as it is entirely self-contained. Like Noah, I'm unhappy that this patch went in. I stated a few days before it was committed that I felt the code quality and documentation were well below the level we normally expect. Your response was to do a little more work on the patch and then commit it without so much as reposting it. Clearly, we have completely different ideas about what constitutes an appropriate response to the sort of comments I sent. A near-record number of committers have objected to this patch in all kinds of different ways. There have been concerns about process, concerns about code quality, and concerns about whether this is really something that we want. The normal way that works is that you either (a) revise the patch or (b) try to convince the person raising the concern that it's OK. The concern is addressed when the person who raised it says it is, and not just because you thought about it and decided it was OK. This can be taken too far in the other direction, but we're not close to that in this instance. I am particularly troubled by the fact that what has happened with this patch is very much like what happened with row-level security: a patch that clearly wasn't finished and clearly had not had adequate review got abruptly committed - by you - without any consensus so to do. It seems likely to me that by now row-level security has had enough review that it is solid enough to be in the tree - in particular, I am encouraged by Dean Rasheed's work to improve the patch. However, it is absolutely not the community process for that stuff to happen after the code is already in the tree. It is the community process for that stuff to happen before the code is in the tree. It will be impossible for our community to continue delivering quality software if you persist in taking this approach. Either the other committers will have to spend an inordinate effort fixing (or convincing you to fix) the stuff you break - instead of working on our own projects - and other people's patches - or we will have to ignore your commits, and the things that are broken in those commits will become bugs in our releases. Either way, the community loses. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Noah,
* Noah Misch (n...@leadboat.com) wrote:
> On Thu, May 14, 2015 at 01:38:49PM -0400, Tom Lane wrote:
> > * The comments in the code betray utter ignorance of how logging actually
> > works, in particular this:
> >
> > * Administrators can choose which log level the audit log is to be logged
> > * at. The default level is LOG, which goes into the server log but does
> > * not go to the client. Set to NOTICE in the regression tests.
> >
> > All the user has to do is change client_min_messages and he'll see all the
> > reports, which means if you think that letting the user see the audit
> > reports is a security problem then you have a hole a mile wide.
>
> That indicates the patch's general readiness:
While I agree that the comment was poorly worded and agreed with Tom
that it should be changed, I do not agree that it's indicative of the
patch's readiness, nor do I feel it's an issue that clients can see the
auditing information, provided that it's clearly documented.
I'm planning to commit a number of documentation updates (along with
other updates) to pg_audit to address that. Using ereport() for query
information isn't anything new- we do that in auto_explain, and we also
report the query in the CONTEXT lines of error messages. We should (and
I plan to) document that in the appropriate places and note that there
are risks associated with it (eg: with security definer functions).
> > + /* These are DDL, unless they are ROLE */
> > + case LOGSTMT_DDL:
> > + className = CLASS_DDL;
> > + class = LOG_DDL;
> > +
> > + /* Identify role statements */
> > + switch (stackItem->auditEvent.commandTag)
> > + {
> > + /* We know these are all role statements */
> > + case T_GrantStmt:
> > + case T_GrantRoleStmt:
> > + case T_CreateRoleStmt:
> > + case T_DropRoleStmt:
> > + case T_AlterRoleStmt:
> > + case T_AlterRoleSetStmt:
> > + className = CLASS_ROLE;
> > + class = LOG_ROLE;
> > + break;
>
> Not T_AlterDefaultPrivilegesStmt?
Agreed, that would be more sensible under ROLE than DDL.
> > +static void
> > +pg_audit_ProcessUtility_hook(Node *parsetree,
> > +const char
> > *queryString,
> > +ProcessUtilityContext
> > context,
> > +ParamListInfo params,
> > +DestReceiver *dest,
> > +char *completionTag)
> > +{
> > + AuditEventStackItem *stackItem = NULL;
> > + int64 stackId = 0;
> > +
> > + /*
> > +* Don't audit substatements. All the substatements we care about
> > should
> > +* be covered by the event triggers.
> > +*/
> > + if (context <= PROCESS_UTILITY_QUERY &&
> > !IsAbortedTransactionBlockState())
>
> They aren't covered. A GRANT inside CREATE SCHEMA escapes auditing:
Actually, they are covered. David and I discussed this extensively,
prior to your review, and concluded that this approach works because the
items not under the EventTrigger charter are shared catalogs, updates to
which wouldn't ever make sense to happen under a CREATE SCHEMA. I do
hope that we are able to improve on EventTriggers by supporting them for
shared catalogs one day, but that is a discussion for another thread.
The issue which you discovered here is that GRANTs were categorized
under CLASS_ROLE, but we have a check at the top of the DDL event
trigger which does an early-exit if DDL isn't selected for inclusion.
That clearly needs to be changed, so that the GRANT under CREATE SCHEMA
is caught and logged properly.
We put a great deal of thought into any and all places where we filter
data to do our best to prevent this from happening, taking steps beyond
what a simple module would do to capture information and make sure that
something is logged, even when we don't have all of the information
available. That isn't to say there aren't bugs- certainly issues have
been found through the various reviews and comments provided by multiple
individuals now, and I don't pretend that there are no others, but I
don't believe that this module is particularly more bug-ridden than
other contrib modules or even parts of core.
> I'm wary of the ease of forgetting to run CREATE EXTENSION. One gets much
> auditing from GUCs alone; for example, we audit "CREATE TABLE t ()" with or
> without the extension, but only with the extension do we audit the inner
> CREATE TABLE of "CREATE SCHEMA s CREATE TABLE t ()". A user that creates a
> database without creating the ext
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Thu, May 14, 2015 at 01:38:49PM -0400, Tom Lane wrote:
> * The comments in the code betray utter ignorance of how logging actually
> works, in particular this:
>
> * Administrators can choose which log level the audit log is to be logged
> * at. The default level is LOG, which goes into the server log but does
> * not go to the client. Set to NOTICE in the regression tests.
>
> All the user has to do is change client_min_messages and he'll see all the
> reports, which means if you think that letting the user see the audit
> reports is a security problem then you have a hole a mile wide.
That indicates the patch's general readiness:
> + /* These are DDL, unless they are ROLE */
> + case LOGSTMT_DDL:
> + className = CLASS_DDL;
> + class = LOG_DDL;
> +
> + /* Identify role statements */
> + switch (stackItem->auditEvent.commandTag)
> + {
> + /* We know these are all role statements */
> + case T_GrantStmt:
> + case T_GrantRoleStmt:
> + case T_CreateRoleStmt:
> + case T_DropRoleStmt:
> + case T_AlterRoleStmt:
> + case T_AlterRoleSetStmt:
> + className = CLASS_ROLE;
> + class = LOG_ROLE;
> + break;
Not T_AlterDefaultPrivilegesStmt?
> +static void
> +pg_audit_ProcessUtility_hook(Node *parsetree,
> + const char
> *queryString,
> + ProcessUtilityContext
> context,
> + ParamListInfo params,
> + DestReceiver *dest,
> + char *completionTag)
> +{
> + AuditEventStackItem *stackItem = NULL;
> + int64 stackId = 0;
> +
> + /*
> + * Don't audit substatements. All the substatements we care about
> should
> + * be covered by the event triggers.
> + */
> + if (context <= PROCESS_UTILITY_QUERY &&
> !IsAbortedTransactionBlockState())
They aren't covered. A GRANT inside CREATE SCHEMA escapes auditing:
create extension pg_audit;
SET pg_audit.log = 'role';
SET pg_audit.log_catalog = OFF;
SET pg_audit.log_level = 'warning';
SET pg_audit.log_parameter = on;
SET pg_audit.log_relation = on;
SET pg_audit.role = auditor;
SET pg_audit.log_statement_once = ON;
create table t ();
create role alice;
grant select on t to alice;
revoke select on t from alice;
\z t
create schema foo grant select on public.t to alice;
\z t
I'm wary of the ease of forgetting to run CREATE EXTENSION. One gets much
auditing from GUCs alone; for example, we audit "CREATE TABLE t ()" with or
without the extension, but only with the extension do we audit the inner
CREATE TABLE of "CREATE SCHEMA s CREATE TABLE t ()". A user that creates a
database without creating the extension might look at the audit messages and
mistakenly think the database is all set.
> + /* Return objects affected by the (non drop) DDL statement */
> + query = "SELECT UPPER(object_type), object_identity\n"
> + " FROM pg_event_trigger_ddl_commands()";
This SPI query neglects to schema-qualify its function calls.
> + DefineCustomStringVariable(
> + "pg_audit.log",
> +
> + "Specifies which classes of statements will be logged by
> session audit "
> + "logging. Multiple classes can be provided using a
> comma-separated "
> + "list and classes can be subtracted by prefacing the class with
> a "
> + "- sign.",
> +
> + NULL,
The short_desc is several lines long, while long_desc is NULL.
> --- /dev/null
> +++ b/contrib/pg_audit/sql/pg_audit.sql
I do applaud the breadth of test coverage.
> +-- Set pg_audit parameters for the current (super)user.
> +ALTER ROLE :current_user SET pg_audit.log = 'Role';
> +ALTER ROLE :current_user SET pg_audit.log_level = 'notice';
Do not ALTER the initial login role in a regression test. In the installcheck
case, the role belongs to the test operator; it is not ours to modify.
> +CREATE USER user1;
> +ALTER ROLE user1 SET pg_audit.log = 'ddl, ROLE';
> +ALTER ROLE user1 SET pg_audit.log_level = 'notice';
> +
> +--
> +-- Create, select, drop (select will not be audited)
> +\connect - user1
Adding new CREATE USER statements, as opposed to CREATE ROLE, to regression
tests is almost always wrong. During "make check" on Windows, such users
cannot connect. "REGRESS_OPTS = --create-role=user1" fixes that problem but
does not help installcheck to pass under MD5 authentication (which it did as
of commit c82725e). Skip past these c
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
On Sun, May 17, 2015 at 11:00 PM, Stephen Frost wrote: > Fujii, Michael, > > * Fujii Masao (masao.fu...@gmail.com) wrote: >> pg_audit uses 1.0.0 as its version number. But, is the third digit really >> required? Why? We usually uses the version number with two digits in >> contrib modules. > > [...] > >> In Makefile, PGFILEDESC should be added. >> >> +# pg_audit/Makefile >> >> should be "# contrib/pg_audit/Makefile" for the consistency. >> >> The categories of some SQL commands are different between log_statement and >> pg_audit. For example, REINDEX is treated as DDL in pg_audit, but not in >> log_statement. That's confusing. We should use the same "category-mapping" >> rule as much as possible. > > [...] > > * Michael Paquier (michael.paqu...@gmail.com) wrote: >> And on top of that the following things should be changed: >> - Removal of REGRESS_OPTS which is empty >> - Removal of MODULE, which overlaps with MODULE_big >> - $(WIN32RES) needs to be added to OBJS for Windows versioning > > I've pushed these changes. Thanks a lot! Here are other random comments on pg_audit. # Move to pgsql-hackers Isn't it better to suppress STATEMENT message in audit log? Otherwise, we always get two duplicate statements as follows, and which would increases the audit log volume very much, I'm afraid. LOG: AUDIT: SESSION,1,1,READ,SELECT,,,SELECT 1;, STATEMENT: SELECT 1; Also CONTEXT message containing the same statement can sometimes be logged at the same time. When I tried the object audit feature, I got the following log messages. The class and command type of this SELECT query is WRITE and UPDATE in the log message. Is this intentional? Why? Maybe pg_audit treats something like "SELECT FOR SHARE/UPDATE" as UPDATE command? But in session logging, the class and command type of this query was READ and SELECT. So confusing. LOG: AUDIT: OBJECT,2,1,WRITE,UPDATE,TABLE,public.aaa,"SELECT 1 FROM ONLY ""public"".""aaa"" x WHERE ""id"" OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x", CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."aaa" x WHERE "id" OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x" STATEMENT: INSERT INTO bbb VALUES (1); The queries to reproduce the above message is here. ALTER SYSTEM SET pg_audit.role TO 'foo'; SELECT pg_reload_conf(); CREATE TABLE aaa (id int primary key); CREATE TABLE bbb (id int references aaa(id)); INSERT INTO aaa VALUES(generate_series(1,100)); GRANT SELECT ON aaa TO foo; INSERT INTO bbb VALUES (1); Now the class of FETCH command is MISC, but isn't it better to classify that as READ? When a query contains a double quote or comma, it's quoted in the audit log as follows. Doesn't this make the analysis of the queries in the log files a bit difficult because other queries are not quoted? LOG: AUDIT: SESSION,2,1,READ,SELECT,,,"SELECT '""';", STATEMENT: SELECT '"'; When log_destination is csvlog, the above quoted query is quoted again. 2015-05-18 14:44:38.313 JST,"postgres","postgres",17725,"[local]",55597c45.453d,1,"SELECT",2015-05-18 14:44:37 JST,2/2,0,LOG,0,"AUDIT: SESSION,1,1,READ,SELECT,,,""SELECT '';"",",,"SELECT '""';",,,"psql" Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > Ah, ok. > > Pushed that, but some further notes: Thanks! Looking much better. > * The actual audit reports ought to be ereport() not elog(). I made them > so but did not insert an errcode(). ISTM that it would likely be a good > thing to assign a not-used-for-any-other-purpose errcode for this, but I'm > not terribly sure what category to put it in. Right, I had seen that too and had intended to change it, but somehow missed it in the other changes I was doing. I'll take a look at the categories and try to figure out what makes sense. > * The comments in the code betray utter ignorance of how logging actually > works, in particular this: > > * Administrators can choose which log level the audit log is to be logged > * at. The default level is LOG, which goes into the server log but does > * not go to the client. Set to NOTICE in the regression tests. I had rewored that last night and will reword it again to be more clear. > All the user has to do is change client_min_messages and he'll see all the > reports, which means if you think that letting the user see the audit > reports is a security problem then you have a hole a mile wide. There are certainly use-cases for this where that's not an issue and also ones where the user wouldn't be able to use pg_audit due to this. I'll update the docs to make the risk of this clear. At least for the use-cases we've been involved in, they've not been concerned about this. Still, any thoughts you have on this would certainly be welcome. I've been thinking about how we might re-route and tag messages in the backend for a number of years and I feel like this summer I'll have resources and time to spend working towards it. Providing a way to decide if a message should be sent only to the server log, or only to the client, or to an external system (syslog, pgQ, rabbitMQ, etc), or to some combination of those, is definitely one of the items on that list. > (I assume BTW that we're not considering it a security problem that > superusers can trivially escape auditing.) No, that's entirely understood and expected, which is one of the reasons it'd be good to reduce the number of superusers running around. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> At least on dromedary, this seems to be the problem: >> >> pg_audit.c: In function 'stack_pop': >> pg_audit.c:387: warning: format '%ld' expects type 'long int', but argument >> 3 has type 'int64' >> pg_audit.c: In function 'stack_valid': >> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument >> 3 has type 'int64' >> pg_audit.c:406: warning: format '%ld' expects type 'long int', but argument >> 4 has type 'int64' >> pg_audit.c: In function 'log_audit_event': >> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument >> 4 has type 'int64' >> pg_audit.c:676: warning: format '%ld' expects type 'long int', but argument >> 5 has type 'int64' >> >> Will push a fix shortly and we'll see what happens. > Ah, ok. Pushed that, but some further notes: * The actual audit reports ought to be ereport() not elog(). I made them so but did not insert an errcode(). ISTM that it would likely be a good thing to assign a not-used-for-any-other-purpose errcode for this, but I'm not terribly sure what category to put it in. * The comments in the code betray utter ignorance of how logging actually works, in particular this: * Administrators can choose which log level the audit log is to be logged * at. The default level is LOG, which goes into the server log but does * not go to the client. Set to NOTICE in the regression tests. All the user has to do is change client_min_messages and he'll see all the reports, which means if you think that letting the user see the audit reports is a security problem then you have a hole a mile wide. (I assume BTW that we're not considering it a security problem that superusers can trivially escape auditing.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Alvaro Herrera writes: > Tom Lane wrote: >> Another thing that looks not amazingly well-thought-out about that >> regression test is that it creates a superuser role with a known name >> (and no password, not that adding a password would make it better). > We create a lot of roles in other tests too; the foreign_data test is > the only one that create a superuser role. While working on the tests > for the DDL deparse thing, I had to create a script with a list of roles > that all the tests use, and it's pretty amazing. I remember thinking at > the time that it'd be better to initialize a number of standard roles in > an initial step, and have them be used consistently in the tests that > require them, rather than having create/drop everywhere. It would definitely be better if the names were less randomly chosen and hence less likely to conflict with existing role names in an installation. I'm not sure why we don't insist that they should all start with "regress" or similar, for instance. But what I'm on about at the moment is that I think creating new superusers is a bad idea from a security standpoint. It seems quite unlikely that we *have* to do that for testing purposes. Also, I notice that the pg_audit test fails to drop the roles it created, even if it reaches the end successfully. That's just bad. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add pg_audit, an auditing extension
Tom Lane wrote: > Stephen Frost writes: > > I've pushed a change which should clean it up by simply loading the > > module after each reconnects is done, more-or-less simulating having it > > be in shared_preload_libraries. It also wasn't using the correct > > database for reconnecting. > > > I'll keep an eye on it. > > Another thing that looks not amazingly well-thought-out about that > regression test is that it creates a superuser role with a known name > (and no password, not that adding a password would make it better). We create a lot of roles in other tests too; the foreign_data test is the only one that create a superuser role. While working on the tests for the DDL deparse thing, I had to create a script with a list of roles that all the tests use, and it's pretty amazing. I remember thinking at the time that it'd be better to initialize a number of standard roles in an initial step, and have them be used consistently in the tests that require them, rather than having create/drop everywhere. -- create roles used throughout the tests create role clstr_user; create role "current_user"; create role foreign_data_user; create role "Public"; create role regressgroup1; create role regressgroup2; create role regression_bob; create role regression_group; create role regression_user1; create role regression_user2; create role regression_user3; create role regression_user; create role regresslo; create role regress_rol_lock1; create role regress_test_indirect; create role regress_test_role; create role regress_test_role2; create role regress_test_role_super superuser; create role regressuser1; create role regressuser2; create role regressuser3; create role regressuser4; create role regressuser5; create role regtest_unpriv_user; create role regtest_addr_user; create role regtest_alter_user1; create role regtest_alter_user2; create role regtest_alter_user3; create role rls_regress_group1; create role rls_regress_group2; create role rls_regress_user0; create role rls_regress_user1; create role rls_regress_user2; create role rls_regress_exempt_user; create role schemauser2; create role seclabel_user1; create role seclabel_user2; create role selinto_user; create role testrol1; create role testrol2; create role testrolx; create role unprivileged_role; create role "user"; create role view_user2; -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
