Re: [HACKERS] Audit of logout

2014-09-13 Thread Tom Lane
Amit Kapila amit.kapil...@gmail.com writes:
 Yes, it was failing only on windows.  Today when I further tried to
 look into the issue, I found that if I rebuild plpgsql, it didn't occurred.
 So the conclusion is that it occurred due to build mismatch, however I
 am not sure why a rebuild of plpgsql is required for this patch.
 Sorry for the noise.

plpgsql contains references to PGC_SUSET and PGC_USERSET, whose values are
changed by this patch, so failure is unsurprising if you don't rebuild it.
(I saw failures in the regression tests even without Windows until I
updated plpgsql.)

Committed with minor cosmetic adjustments; mostly, rewriting some
comments.

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] Audit of logout

2014-09-06 Thread Amit Kapila
On Wed, Sep 3, 2014 at 8:09 PM, Fujii Masao masao.fu...@gmail.com wrote:

 On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila amit.kapil...@gmail.com
wrote:
  On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao masao.fu...@gmail.com
wrote:
 
  On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com
  wrote:
   On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao masao.fu...@gmail.com
   wrote:
   Changing PGC_SU_BACKEND parameter (log_connections) is
   visible even with a non-super user client due to above code.
   Shouldn't it be only visible for super-user logins?
  
   Simple steps to reproduce the problem:
   a. start Server (default configuration)
   b. connect with superuser
   c. change in log_connections to on in postgresql.conf
   d. perform select pg_reload_conf();
   e. connect with non-super-user
   f.  show log_connections;  --This step shows the value as on,
  --whereas I think it should
have
   been
   off
 
  In this case, log_connections is changed in postgresql.conf and it's
  reloaded, so ISTM that it's natural that even non-superuser sees the
  changed value. No? Maybe I'm missing something.
 
  Yeah, you are right.
 
  With the latest patch, I am getting one regression failure on windows.
  Attached is the regression diff.

 Thanks for testing the patch!

 That regression failure looks strange, I'm not sure yet why that
happened...
 Does it happen only on Windows?

Yes, it was failing only on windows.  Today when I further tried to
look into the issue, I found that if I rebuild plpgsql, it didn't occurred.
So the conclusion is that it occurred due to build mismatch, however I
am not sure why a rebuild of plpgsql is required for this patch.
Sorry for the noise.

There are no more comments from myside, so I will mark this as
Ready For Committer.

  Can we improve this line a bit?
  !  * BACKEND options are the same as SU_BACKEND ones, but they can
  BACKEND options can be set same as SU_BACKEND ones, ..

 Yep.

Okay.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] Audit of logout

2014-09-03 Thread Fujii Masao
On Thu, Aug 28, 2014 at 11:23 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao masao.fu...@gmail.com wrote:

 On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com
 wrote:
  On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao masao.fu...@gmail.com
  wrote:
  Changing PGC_SU_BACKEND parameter (log_connections) is
  visible even with a non-super user client due to above code.
  Shouldn't it be only visible for super-user logins?
 
  Simple steps to reproduce the problem:
  a. start Server (default configuration)
  b. connect with superuser
  c. change in log_connections to on in postgresql.conf
  d. perform select pg_reload_conf();
  e. connect with non-super-user
  f.  show log_connections;  --This step shows the value as on,
 --whereas I think it should have
  been
  off

 In this case, log_connections is changed in postgresql.conf and it's
 reloaded, so ISTM that it's natural that even non-superuser sees the
 changed value. No? Maybe I'm missing something.

 Yeah, you are right.

 With the latest patch, I am getting one regression failure on windows.
 Attached is the regression diff.

Thanks for testing the patch!

That regression failure looks strange, I'm not sure yet why that happened...
Does it happen only on Windows?

 Can we improve this line a bit?
 !  * BACKEND options are the same as SU_BACKEND ones, but they can
 BACKEND options can be set same as SU_BACKEND ones, ..

Yep.

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] Audit of logout

2014-08-28 Thread Amit Kapila
On Wed, Aug 27, 2014 at 5:19 PM, Fujii Masao masao.fu...@gmail.com wrote:

 On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com
wrote:
  On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao masao.fu...@gmail.com
wrote:
  Changing PGC_SU_BACKEND parameter (log_connections) is
  visible even with a non-super user client due to above code.
  Shouldn't it be only visible for super-user logins?
 
  Simple steps to reproduce the problem:
  a. start Server (default configuration)
  b. connect with superuser
  c. change in log_connections to on in postgresql.conf
  d. perform select pg_reload_conf();
  e. connect with non-super-user
  f.  show log_connections;  --This step shows the value as on,
 --whereas I think it should have
been
  off

 In this case, log_connections is changed in postgresql.conf and it's
 reloaded, so ISTM that it's natural that even non-superuser sees the
 changed value. No? Maybe I'm missing something.

Yeah, you are right.

With the latest patch, I am getting one regression failure on windows.
Attached is the regression diff.

Can we improve this line a bit?
!  * BACKEND options are the same as SU_BACKEND ones, but they can
BACKEND options can be set same as SU_BACKEND ones, ..


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


regression.diffs
Description: Binary data

-- 
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] Audit of logout

2014-08-27 Thread Fujii Masao
On Sat, Aug 23, 2014 at 3:44 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao masao.fu...@gmail.com wrote:

 Yep, the attached patch introduces PGC_SU_BACKEND and
 changes the contexts of log_connections and log_disconnections
 to PGC_SU_BACKEND. Review?


Thanks for reviewing the patch!

 1.
 ! else if (context != PGC_POSTMASTER  context != PGC_SU_BACKEND 
 ! context != PGC_SU_BACKEND  source != PGC_S_CLIENT)

 In the above check for PGC_SU_BACKEND is repeated, here
 one of the check should be PGC_SU_BACKEND  and other
 should be PGC_BACKEND.

Right. Fixed. Attached is the updated version of the patch.
BTW, I also added the following into the document of log_connections
and log_disconnections.

Only superusers can change this setting at session start.

 2.
 + case PGC_SU_BACKEND:
 + if (context == PGC_BACKEND)
 + {
 ..
 ..
 + return 0;
 + }
   case PGC_BACKEND:
   if (context == PGC_SIGHUP)

 Changing PGC_SU_BACKEND parameter (log_connections) is
 visible even with a non-super user client due to above code.
 Shouldn't it be only visible for super-user logins?

 Simple steps to reproduce the problem:
 a. start Server (default configuration)
 b. connect with superuser
 c. change in log_connections to on in postgresql.conf
 d. perform select pg_reload_conf();
 e. connect with non-super-user
 f.  show log_connections;  --This step shows the value as on,
--whereas I think it should have been
 off

In this case, log_connections is changed in postgresql.conf and it's
reloaded, so ISTM that it's natural that even non-superuser sees the
changed value. No? Maybe I'm missing something.

Regards,

-- 
Fujii Masao
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 4231,4236  local0.*/var/log/postgresql
--- 4231,4237 
 para
  Causes each attempted connection to the server to be logged,
  as well as successful completion of client authentication.
+ Only superusers can change this setting at session start.
  This parameter cannot be changed after session start.
  The default is off.
 /para
***
*** 4258,4263  local0.*/var/log/postgresql
--- 4259,4265 
  varnamelog_connections/varname but at session termination,
  and includes the duration of the session.  This is off by
  default.
+ Only superusers can change this setting at session start.
  This parameter cannot be changed after session start.
 /para
/listitem
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***
*** 3258,3264  get_stats_option_name(const char *arg)
   * argv[0] is ignored in either case (it's assumed to be the program name).
   *
   * ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
!  * coming from the client, or PGC_SUSET for insecure options coming from
   * a superuser client.
   *
   * If a database name is present in the command line arguments, it's
--- 3258,3264 
   * argv[0] is ignored in either case (it's assumed to be the program name).
   *
   * ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
!  * coming from the client, or PGC_SU_BACKEND for insecure options coming from
   * a superuser client.
   *
   * If a database name is present in the command line arguments, it's
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
***
*** 957,963  process_startup_options(Port *port, bool am_superuser)
  	GucContext	gucctx;
  	ListCell   *gucopts;
  
! 	gucctx = am_superuser ? PGC_SUSET : PGC_BACKEND;
  
  	/*
  	 * First process any command-line switches that were included in the
--- 957,963 
  	GucContext	gucctx;
  	ListCell   *gucopts;
  
! 	gucctx = am_superuser ? PGC_SU_BACKEND : PGC_BACKEND;
  
  	/*
  	 * First process any command-line switches that were included in the
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 509,514  const char *const GucContext_Names[] =
--- 509,515 
  	 /* PGC_INTERNAL */ internal,
  	 /* PGC_POSTMASTER */ postmaster,
  	 /* PGC_SIGHUP */ sighup,
+ 	 /* PGC_SU_BACKEND */ superuser-backend,
  	 /* PGC_BACKEND */ backend,
  	 /* PGC_SUSET */ superuser,
  	 /* PGC_USERSET */ user
***
*** 907,913  static struct config_bool ConfigureNamesBool[] =
  		NULL, NULL, NULL
  	},
  	{
! 		{log_connections, PGC_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs each successful connection.),
  			NULL
  		},
--- 908,914 
  		NULL, NULL, NULL
  	},
  	{
! 		{log_connections, PGC_SU_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs each successful connection.),
  			NULL
  		},
***
*** 916,922  static struct config_bool ConfigureNamesBool[] =
  		NULL, NULL, NULL
  	},
  	{
! 		{log_disconnections, PGC_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs end of a 

Re: [HACKERS] Audit of logout

2014-08-23 Thread Amit Kapila
On Tue, Aug 5, 2014 at 8:04 PM, Fujii Masao masao.fu...@gmail.com wrote:

 Yep, the attached patch introduces PGC_SU_BACKEND and
 changes the contexts of log_connections and log_disconnections
 to PGC_SU_BACKEND. Review?


1.
! else if (context != PGC_POSTMASTER  context != PGC_SU_BACKEND 
!  context != PGC_SU_BACKEND  source != PGC_S_CLIENT)

In the above check for PGC_SU_BACKEND is repeated, here
one of the check should be PGC_SU_BACKEND  and other
should be PGC_BACKEND.

2.
+ case PGC_SU_BACKEND:
+ if (context == PGC_BACKEND)
+ {
..
..
+ return 0;
+ }
  case PGC_BACKEND:
  if (context == PGC_SIGHUP)

Changing PGC_SU_BACKEND parameter (log_connections) is
visible even with a non-super user client due to above code.
Shouldn't it be only visible for super-user logins?

Simple steps to reproduce the problem:
a. start Server (default configuration)
b. connect with superuser
c. change in log_connections to on in postgresql.conf
d. perform select pg_reload_conf();
e. connect with non-super-user
f.  show log_connections;  --This step shows the value as on,
   --whereas I think it should have
been off

This test has been performed on *Windows*.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] Audit of logout

2014-08-05 Thread Fujii Masao
On Tue, Jul 15, 2014 at 10:45 PM, Magnus Hagander mag...@hagander.net wrote:
 On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I wrote:
 In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
 wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
 PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
 are allowed to change it.  I don't have any objection to making these two
 settings only adjustable by superusers --- I just don't want to give up
 the existing timing restrictions for them.

 Another idea would be to get rid of PGC_SUSET as a separate category, and
 instead have a superuser-only bit in the GUC flags, which would apply to
 all categories.  This would be a bit more orthogonal, though likely a
 much more invasive change.

 That could become interesting in the futuren ow that we have ALTER
 SYSTEM SET. It could allow a non-superuser to make persistent
 configuration changes. Now, I'm not sure we actually *want* that
 though... But having it as a separate bit would make it possible for
 ALTER SYSTEM SET to say that for example regular users would be able
 to change work_mem persistently. But if we want to go down that route,
 we might need a more fine grained permissions model than just
 superuser vs non-superuser...

 I think going with the PGC_SU_BACKEND is the right choice at this
 time, until we have an actual usecase for the other :)

Yep, the attached patch introduces PGC_SU_BACKEND and
changes the contexts of log_connections and log_disconnections
to PGC_SU_BACKEND. Review?

Regards,

-- 
Fujii Masao
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***
*** 3258,3264  get_stats_option_name(const char *arg)
   * argv[0] is ignored in either case (it's assumed to be the program name).
   *
   * ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
!  * coming from the client, or PGC_SUSET for insecure options coming from
   * a superuser client.
   *
   * If a database name is present in the command line arguments, it's
--- 3258,3264 
   * argv[0] is ignored in either case (it's assumed to be the program name).
   *
   * ctx is PGC_POSTMASTER for secure options, PGC_BACKEND for insecure options
!  * coming from the client, or PGC_SU_BACKEND for insecure options coming from
   * a superuser client.
   *
   * If a database name is present in the command line arguments, it's
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
***
*** 957,963  process_startup_options(Port *port, bool am_superuser)
  	GucContext	gucctx;
  	ListCell   *gucopts;
  
! 	gucctx = am_superuser ? PGC_SUSET : PGC_BACKEND;
  
  	/*
  	 * First process any command-line switches that were included in the
--- 957,963 
  	GucContext	gucctx;
  	ListCell   *gucopts;
  
! 	gucctx = am_superuser ? PGC_SU_BACKEND : PGC_BACKEND;
  
  	/*
  	 * First process any command-line switches that were included in the
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 512,517  const char *const GucContext_Names[] =
--- 512,518 
  	 /* PGC_INTERNAL */ internal,
  	 /* PGC_POSTMASTER */ postmaster,
  	 /* PGC_SIGHUP */ sighup,
+ 	 /* PGC_SU_BACKEND */ superuser-backend,
  	 /* PGC_BACKEND */ backend,
  	 /* PGC_SUSET */ superuser,
  	 /* PGC_USERSET */ user
***
*** 910,916  static struct config_bool ConfigureNamesBool[] =
  		NULL, NULL, NULL
  	},
  	{
! 		{log_connections, PGC_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs each successful connection.),
  			NULL
  		},
--- 911,917 
  		NULL, NULL, NULL
  	},
  	{
! 		{log_connections, PGC_SU_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs each successful connection.),
  			NULL
  		},
***
*** 919,925  static struct config_bool ConfigureNamesBool[] =
  		NULL, NULL, NULL
  	},
  	{
! 		{log_disconnections, PGC_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs end of a session, including duration.),
  			NULL
  		},
--- 920,926 
  		NULL, NULL, NULL
  	},
  	{
! 		{log_disconnections, PGC_SU_BACKEND, LOGGING_WHAT,
  			gettext_noop(Logs end of a session, including duration.),
  			NULL
  		},
***
*** 5681,5696  set_config_option(const char *name, const char *value,
  			 * signals to individual backends only.
  			 */
  			break;
  		case PGC_BACKEND:
  			if (context == PGC_SIGHUP)
  			{
  /*
!  * If a PGC_BACKEND parameter is changed in the config file,
!  * we want to accept the new value in the postmaster (whence
!  * it will propagate to subsequently-started backends), but
!  * ignore it in existing backends.  This is a tad klugy, but
!  * necessary because we don't re-read the config file during
!  * backend start.
   *
   * In EXEC_BACKEND builds, this works differently: we load all
   * nondefault settings from the CONFIG_EXEC_PARAMS file during
--- 5682,5706 
  			 * 

Re: [HACKERS] Audit of logout

2014-07-28 Thread Fujii Masao
On Mon, Jul 28, 2014 at 12:22 PM, Amit Kapila amit.kapil...@gmail.com wrote:
 On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway m...@joeconway.com wrote:

 No. If we change it to PGC_SIGHUP, SHOW command does display
 the changed value after a reload. It's the same behavior as other
 PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
 You can test that by using the patch.

 As this patch is marked as Needs Review, so I went ahead and
 picked up for review, however after reading mail chain, it seems to
 me that there is a general inclination to have a new category in
 GucContext for this feature.  I don't see the patch implementing the
 same in this thread, so I think it is better to move it to next CF
 (2014-08).

Yep, agreed. I just moved this to next CF.

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] Audit of logout

2014-07-27 Thread Amit Kapila
On Thu, Jul 3, 2014 at 1:13 AM, Fujii Masao masao.fu...@gmail.com wrote:
 On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway m...@joeconway.com wrote:

 No. If we change it to PGC_SIGHUP, SHOW command does display
 the changed value after a reload. It's the same behavior as other
 PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
 You can test that by using the patch.

As this patch is marked as Needs Review, so I went ahead and
picked up for review, however after reading mail chain, it seems to
me that there is a general inclination to have a new category in
GucContext for this feature.  I don't see the patch implementing the
same in this thread, so I think it is better to move it to next CF
(2014-08).

Whats your opinion?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] Audit of logout

2014-07-15 Thread Magnus Hagander
On Wed, Jul 2, 2014 at 10:52 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 I wrote:
 In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
 wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
 PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
 are allowed to change it.  I don't have any objection to making these two
 settings only adjustable by superusers --- I just don't want to give up
 the existing timing restrictions for them.

 Another idea would be to get rid of PGC_SUSET as a separate category, and
 instead have a superuser-only bit in the GUC flags, which would apply to
 all categories.  This would be a bit more orthogonal, though likely a
 much more invasive change.

That could become interesting in the futuren ow that we have ALTER
SYSTEM SET. It could allow a non-superuser to make persistent
configuration changes. Now, I'm not sure we actually *want* that
though... But having it as a separate bit would make it possible for
ALTER SYSTEM SET to say that for example regular users would be able
to change work_mem persistently. But if we want to go down that route,
we might need a more fine grained permissions model than just
superuser vs non-superuser...

I think going with the PGC_SU_BACKEND is the right choice at this
time, until we have an actual usecase for the other :)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/


-- 
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] Audit of logout

2014-07-02 Thread Fujii Masao
On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway m...@joeconway.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 06/13/2014 07:29 AM, Tom Lane wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
 masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to
 audit all logouts. But since log_disconnections is defined with
 PGC_BACKEND, it can be changed at connection start. This means
 that any client (even nonsuperuser) can freely disable
 log_disconnections not to log his or her logout even when the
 system admin enables it in postgresql.conf. Isn't this
 problematic for audit?

 That's harmful for audit purpose. I think that we should make
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
 forbid non-superusers from changing its setting. Attached patch
 does this.

 This whole argument seems wrong unless I'm missing something:

 test=# set log_connections = on;
 ERROR:  parameter log_connections cannot be set after connection start
 test=# set log_disconnections = off;
 ERROR:  parameter log_disconnections cannot be set after connection
 start

Hmm... I found that you had marked this proposal as Returned with Feedback.
But I don't think that we reached the consensus to do that. I think that it's
still worth discussing this topic in this CF. So I marked this as Needs Review
again.

If you strongly think that this proposal should be marked as
Returned with Feedback, could you let me know why you think so?

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] Audit of logout

2014-07-02 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/01/2014 11:55 PM, Fujii Masao wrote:
 Hmm... I found that you had marked this proposal as Returned with
 Feedback. But I don't think that we reached the consensus to do
 that. I think that it's still worth discussing this topic in this
 CF. So I marked this as Needs Review again.
 
 If you strongly think that this proposal should be marked as 
 Returned with Feedback, could you let me know why you think so?

Returned with Feedback means, well exactly that ;-) -- the patch as
linked to the CF page is wrong and cannot be applied the way it is
currently. It is therefore returned to you to be fixed. It does not
mean Rejected which is what you seem to infer.

As mentioned on the CF the documentation change is flat wrong, and you
yourself have said that you think PGC_SUSET is wrong now and
PGC_SIGHUP should be used instead.

Furthermore I doubt you have tested the change to PGC_SIGHUP because I
did a quick test, and for some reason I haven't yet tracked down SHOW
does not see the change to log_disconnections as you said it would
after a reload, unlike other PGC_SIGHUP vars. So there is more
thought, testing, and possibly coding needed to make this a viable change.

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting,  24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTtBm6AAoJEDfy90M199hlEzQQAJOCZrUb1fQw5ArGCBRm3T2n
gB1SobGGbmSweY3M8D/U55/IpjWEp3Emma3869tTVG51d6r1vRchwax1Ol2IUuek
Z7YwdgysoUOY1RNbqsa/WUVS23djKplgA7azrDu+MXFJpupBQjCH7lumtJ/L1dbC
1ua3NKZg914HkDTNHkD2AKL6o4LupfRM0hcSmBUZRG0fWLgSBiHza+idzFR1TfK2
6SpK0T6u3H6R7eU/7YluapY6LC/nA0bx+zM7sBEsWE2Wb1NQ/DER/fkuSO0V0N3X
/ljRUP7sds2KOlIJ95081JVbN5lTM2rQfd6ZLu5CsTKEDKR+PL8rOGgCX2mMoTS5
gc8vDLtyArqzcZpmRTufyBUvQAAS7CyIG7JxJNyWkEDwnn/B8HqBuGdLIdS8VdV5
oEdhbcKuN5cMTodMAv1h/QPcpSE72O/zZ6XxJGD5Wcpury7BTmBkLNJnZOsY4GU0
Nlxcc3tTvMsZYpvrJYBVfypQ6J7PCCwx1qra2GLtA7fkSeH+tXuqQ0IKVXi2bYEr
TDC2Cx/37cn56Z9PObAUJlYmoolix8MD27m6cEW0PvkJrDe7tEPWpEf38MUeZ0ae
kinXrB0MtxrrqICsWBjtxz0HGdsbQUreYs3JadnmkAR8cvhunDuHFShZv6GGzaZL
PzhOgGxxHemWGF7er2YO
=tiPs
-END PGP SIGNATURE-


-- 
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] Audit of logout

2014-07-02 Thread Fujii Masao
On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway m...@joeconway.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 07/01/2014 11:55 PM, Fujii Masao wrote:
 Hmm... I found that you had marked this proposal as Returned with
 Feedback. But I don't think that we reached the consensus to do
 that. I think that it's still worth discussing this topic in this
 CF. So I marked this as Needs Review again.

 If you strongly think that this proposal should be marked as
 Returned with Feedback, could you let me know why you think so?

 Returned with Feedback means, well exactly that ;-) -- the patch as
 linked to the CF page is wrong and cannot be applied the way it is
 currently. It is therefore returned to you to be fixed. It does not
 mean Rejected which is what you seem to infer.

I think that we should use Waiting on Author in that case.

Returned with Feedback is not Rejected. That's right. But it basically
means that the bugfixed or revised version of the patch will NOT be
reviewed in this CF. IOW, the author has to wait for the patch review
until next CF.

 As mentioned on the CF the documentation change is flat wrong, and you
 yourself have said that you think PGC_SUSET is wrong now and
 PGC_SIGHUP should be used instead.

 Furthermore I doubt you have tested the change to PGC_SIGHUP because I
 did a quick test, and for some reason I haven't yet tracked down SHOW
 does not see the change to log_disconnections as you said it would
 after a reload, unlike other PGC_SIGHUP vars.

No. If we change it to PGC_SIGHUP, SHOW command does display
the changed value after a reload. It's the same behavior as other
PGC_SIGHUP parameters do. Attached patch just changes it to PGC_SIGHUP.
You can test that by using the patch.

OTOH, the current behavior, i.e., log_disconnections with PGC_BACKEND,
is that SHOW command doesn't display the changed value after a reload.

 So there is more
 thought, testing, and possibly coding needed to make this a viable change.

+1

Regards,

-- 
Fujii Masao
From 098f11cfbf8f9370829e24e9f8d704c050c98875 Mon Sep 17 00:00:00 2001
From: MasaoFujii masao.fu...@gmail.com
Date: Fri, 13 Jun 2014 22:09:39 +0900
Subject: [PATCH] Make log_disconnections PGC_SUSET rather than PGC_SIGHUP.

So far even non-superusers could disable log_disconnections in order to
prevent their session logout from being logged because the parameter was
defined with PGC_BACKEND. This was harmful in the systems which need to
audit all session logouts by using log_disconnections. For this problem,
this commit changes the GUC context of log_disconnections to PGC_SIGHUP
and forbids any client from changing its setting.
---
 doc/src/sgml/config.sgml | 3 ++-
 src/backend/utils/misc/guc.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 37d78b3..f5f0324 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4257,7 +4257,8 @@ local0.*/var/log/postgresql
 varnamelog_connections/varname but at session termination,
 and includes the duration of the session.  This is off by
 default.
-This parameter cannot be changed after session start.
+This parameter can only be set in the filenamepostgresql.conf/
+file or on the server command line.
/para
   /listitem
  /varlistentry
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 3a31a75..19e521b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -919,7 +919,7 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		{log_disconnections, PGC_BACKEND, LOGGING_WHAT,
+		{log_disconnections, PGC_SIGHUP, LOGGING_WHAT,
 			gettext_noop(Logs end of a session, including duration.),
 			NULL
 		},
-- 
1.7.12.4 (Apple Git-37)


-- 
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] Audit of logout

2014-07-02 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/02/2014 12:43 PM, Fujii Masao wrote:
 I think that we should use Waiting on Author in that case.
 
 Returned with Feedback is not Rejected. That's right. But it
 basically means that the bugfixed or revised version of the patch
 will NOT be reviewed in this CF. IOW, the author has to wait for
 the patch review until next CF.

Doesn't mean that to me but feel free to change it to Waiting on
Author if you prefer :-)

Is there any official explanation as to what those states mean
documented anywhere?


 No. If we change it to PGC_SIGHUP, SHOW command does display the
 changed value after a reload. It's the same behavior as other 
 PGC_SIGHUP parameters do. Attached patch just changes it to
 PGC_SIGHUP. You can test that by using the patch.

I tried it already myself and it did not work the same for me. Perhaps
I messed something up but I tried two or three times and the value of
log_disconnections did not change in the open session when I did SHOW
after reload, but in new backends it did. On the other hand I also
tried a different SIGHUP var (log_checkpoints) and saw that effect
immediately in the open session. Was too tired to follow up last night
though. Have you verified for yourself that it behaves the way you say?

Joe



- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting,  24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTtGMTAAoJEDfy90M199hlZs0P/A4ptorZOFkvW3DJRDfKoE2f
v9wiWHYQJtN31sdDdljRD+3WvfiGJhMVCVukzZbaZejVGsEOModEOSLJZgkPhqa/
n/TSD77KdfHWkiAENkBmsBJLfqccysRxpWsJ5rPSiXqmyz+hxHJ6u8R4RGf/gPXn
15Rq3ZYNtGP25pTlHUr869y1D9dqnEzYdhf69ql4iIQPgsGow+bppZGVEEFd6MsF
2rpUTgdczhn4yl/kMTW25CFm19dHjj+m/v+Yv9uTg0JWX8xBtQ5hT9Z8Pgc/xU2U
WDsdfQ46ORinBOfPd/RPoJoIjxpMGMtuB5dX0mHsVzp7+kqSdKI+5amSz8YxqBE8
ii01AOfH2fOws236QQtVywWRHJMs7xWj5k/YNe1ABNoh5wFQRaaFkVC5r/i+Url2
3lDHUN/MjTeXieBQMUDNmqQbrRShYQqBnWl05n8dn/6V5U42eundJCr0tO+awW9V
3gTVTpwymsv7AkJfk3LjUybTYYDee3YM1A2YhdufYrhmQJttjh1kaeai9G05y5SZ
vU6DL+FJkVukxq5n6MTDzcxKrL0SYcRUVW0FZmng/Wn5SrsBC8AifkuR3Z1uFY5s
TjgCMDr/RzTGlXjITJQOl9smc6P/0yI8JKvdkAGnjeKY9zYsVn35fs/6HGMPmusZ
DWEp8jAdDohoWgiZCz4x
=0qGo
-END PGP SIGNATURE-


-- 
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] Audit of logout

2014-07-02 Thread Tom Lane
Fujii Masao masao.fu...@gmail.com writes:
 On Wed, Jul 2, 2014 at 11:39 PM, Joe Conway m...@joeconway.com wrote:
 Returned with Feedback means, well exactly that ;-) -- the patch as
 linked to the CF page is wrong and cannot be applied the way it is
 currently. It is therefore returned to you to be fixed. It does not
 mean Rejected which is what you seem to infer.

 I think that we should use Waiting on Author in that case.

I don't think there's a huge distinction between Waiting on Author and
Returned with Feedback.  The former means we think the author will
probably submit a new version shortly, the latter means we think it'll
take awhile, but in any particular case those predictions could turn
out wrong.  I don't have a problem with moving a patch from Returned with
Feedback back to Needs Review if the author is able to turn it around
while the CF is still going on.

As far as the particular case goes, it strikes me that the real issue
here is that we're confusing privilege level with time-duration of the
setting's effect.  The point of marking log_connections as PGC_BACKEND is
that changing it within a session after a given session starts is useless,
and it's probably better to freeze it so it can tell you what was actually
done by the current session.  The point of marking log_disconnections as
PGC_BACKEND is so that you can freeze the determination of whether a given
session will log its disconnection at the same time that its determination
of whether to log its connection got frozen, and thus ensure that each
connection log entry should eventually have a disconnection log entry,
assuming you have both enabled.  These considerations are not invalidated
by questions of which users should be allowed to adjust the value.

In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
are allowed to change it.  I don't have any objection to making these two
settings only adjustable by superusers --- I just don't want to give up
the existing timing restrictions for them.

(If we did this, there are probably some other settings that should become
PGC_SU_BACKEND, eg session_preload_libraries ...)

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] Audit of logout

2014-07-02 Thread Abhijit Menon-Sen
At 2014-07-02 12:52:51 -0700, m...@joeconway.com wrote:

 Doesn't mean that to me but feel free to change it to Waiting on
 Author if you prefer :-)
 
 Is there any official explanation as to what those states mean
 documented anywhere?

I don't know if there's an official definition, but my understanding of
returned with feedback was always pretty much in agreement with what
Fujii wrote. If the author is expected to post a revised patch soon, it
gets marked waiting on author, and returned with feedback means it
will take longer, probably in the next CF.

But I also treat these labels as a matter of convenience, and definitely
not some mark of shame where the author should feel upset that the patch
was put in one state or the other. As far as I'm concerned, patches can
be switched from returned with feedback to needs review to ready
for committer at the drop of a hat if updates are made in time.

-- Abhijit


-- 
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] Audit of logout

2014-07-02 Thread Tom Lane
I wrote:
 In short, maybe we ought to invent a new category PGC_SU_BACKEND (not
 wedded to this spelling), which is to PGC_BACKEND as PGC_SUSET is to
 PGC_USERSET, ie same when-it-can-be-changed behavior but only superusers
 are allowed to change it.  I don't have any objection to making these two
 settings only adjustable by superusers --- I just don't want to give up
 the existing timing restrictions for them.

Another idea would be to get rid of PGC_SUSET as a separate category, and
instead have a superuser-only bit in the GUC flags, which would apply to
all categories.  This would be a bit more orthogonal, though likely a
much more invasive change.

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] Audit of logout

2014-07-02 Thread Alvaro Herrera
Abhijit Menon-Sen wrote:
 At 2014-07-02 12:52:51 -0700, m...@joeconway.com wrote:
 
  Doesn't mean that to me but feel free to change it to Waiting on
  Author if you prefer :-)
  
  Is there any official explanation as to what those states mean
  documented anywhere?
 
 I don't know if there's an official definition, but my understanding of
 returned with feedback was always pretty much in agreement with what
 Fujii wrote. If the author is expected to post a revised patch soon, it
 gets marked waiting on author, and returned with feedback means it
 will take longer, probably in the next CF.

I think the main thing with returned with feedback is the patch is not
supposed to be handled any further in the current commitfest.  Normally
if a new version is submitted, it's opened as a new entry in a future
commitfest.  So it's one of the three final states for a patch, the
other two being committed and rejected.  The other status values,
needs review, waiting on author, and ready for committer are
transient and can change to any other state.

So I disagree with you here:

 But I also treat these labels as a matter of convenience, and definitely
 not some mark of shame where the author should feel upset that the patch
 was put in one state or the other. As far as I'm concerned, patches can
 be switched from returned with feedback to needs review to ready
 for committer at the drop of a hat if updates are made in time.

A patch that is Returned with Feedback is *not* supposed to go back to
needs review or any of the other states.  If we expect that the author
is going to update the patch, the right state to use is Waiting on
author.

In any case, no state is a mark of shame on the author.  We are not
supposed to judge people here.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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


Re: [HACKERS] Audit of logout

2014-07-02 Thread Abhijit Menon-Sen
At 2014-07-02 16:47:16 -0400, alvhe...@2ndquadrant.com wrote:
 
 If we expect that the author is going to update the patch, the right
 state to use is Waiting on author.

Quite so. That's how I understand it, and what I've been doing. But what
if we really *don't* expect the author to update the patch, but they do
it anyway? That's the only case I was referring to.

If the right thing to do is to open an entry in the next CF (as you said
earlier in your mail), that's all right with me.

-- Abhijit


-- 
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] Audit of logout

2014-07-02 Thread Alvaro Herrera
Abhijit Menon-Sen wrote:
 At 2014-07-02 16:47:16 -0400, alvhe...@2ndquadrant.com wrote:
  
  If we expect that the author is going to update the patch, the right
  state to use is Waiting on author.
 
 Quite so. That's how I understand it, and what I've been doing. But what
 if we really *don't* expect the author to update the patch, but they do
 it anyway? That's the only case I was referring to.
 
 If the right thing to do is to open an entry in the next CF (as you said
 earlier in your mail), that's all right with me.

As Tom says I think we should be open to the possibility that we made a
mistake and that it should return to needs review, when reasonable.
For example if the author takes long to update and we're in the final
steps of closing the commitfest, I don't think we need to feel forced to
re-examine the patch in the same commitfest.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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


Re: [HACKERS] Audit of logout

2014-07-02 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/02/2014 02:15 PM, Abhijit Menon-Sen wrote:
 At 2014-07-02 16:47:16 -0400, alvhe...@2ndquadrant.com wrote:
 
 If we expect that the author is going to update the patch, the
 right state to use is Waiting on author.
 
 Quite so. That's how I understand it, and what I've been doing. But
 what if we really *don't* expect the author to update the patch,
 but they do it anyway? That's the only case I was referring to.
 
 If the right thing to do is to open an entry in the next CF (as you
 said earlier in your mail), that's all right with me.

In any case I think this side discussion has proven there is not
universal understanding on the particulars and in any case we ought to
have clear definitions (probably with examples) documented somewhere
if anyone is expected to take them quite so specifically.

Also, given Tom's remarks on the other part of this thread, I would
not be surprised if this turned out to be a much larger change than
Fujii-san originally hoped for, and thus may in fact get delayed to
next CF.

Joe

- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting,  24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTtHhTAAoJEDfy90M199hlPmYP/09Cfq8zktVIEePqp/IN9bTo
RW2xL8C/+TsDlHcHNmwB46PpP4xOYnOP6my7EOS4xRKJrF6fwybZUu2nJ2T+ifkP
VHNTRHbW2ndpOR1CkmiJvbNGdr4uWHdzt1RsMrbPP+qIQl5tBnb/vBfI33B8/qzC
tkYCSbhcVaYACajCJaobelWfUbREgBf255x0VdprdyBjmSRq/d1trm3sh2IshKCz
KV2YjLWN6lVENAtp8LLFbpZLVOQPQ0direY1bwcSM+/SC9XqhQeaEWWp7WMgeGtl
2VNObiFJIGDWjQFc2qW/VIyKVHGhvkxpuQ3KSw4gj64EQ561OTGYIW0S+QMHXwt8
krW50yVHw/MJ6efmx2rlBbBqugMB/rw3qe6YpuEcF5s8ezQt9b5ejQC9hZxAl8ln
d8BFISjlT5nroBRCvPYeJO8k7mFB9JFfaOi/GWru+dg/J4Wz/V6Rjr+n4yybjg2V
vNRbJZgAJBIn6iTlKHrx3/QtU7tt4kUIr7gUCVEhYLLAFikPIItARzNYcJZ77jVK
lX30v95gw8IJm1MmhEEkQFKmXzoTYNhh8sEn6t3a8zxRRcIIy+l0jD5lEDxSEirj
lRIgvHtU+LyNkCY6d+V3jPuyg1FlJcNhotUr7KSgwUuij+MrVMxook5IiiJsu/3s
VayKFPq0FLkpkJXSIaha
=afxT
-END PGP SIGNATURE-


-- 
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] Audit of logout

2014-07-01 Thread Fujii Masao
On Mon, Jun 23, 2014 at 5:42 PM, Fujii Masao masao.fu...@gmail.com wrote:
 On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway m...@joeconway.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 06/13/2014 07:29 AM, Tom Lane wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
 masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to
 audit all logouts. But since log_disconnections is defined with
 PGC_BACKEND, it can be changed at connection start. This means
 that any client (even nonsuperuser) can freely disable
 log_disconnections not to log his or her logout even when the
 system admin enables it in postgresql.conf. Isn't this
 problematic for audit?

 That's harmful for audit purpose. I think that we should make
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
 forbid non-superusers from changing its setting. Attached patch
 does this.

 This whole argument seems wrong unless I'm missing something:

 test=# set log_connections = on;
 ERROR:  parameter log_connections cannot be set after connection start
 test=# set log_disconnections = off;
 ERROR:  parameter log_disconnections cannot be set after connection
 start

 You can change log_connections/disconnections via connection option as follows

 $ grep log_disconnections $PGDATA/postgresql.conf
 log_disconnections = on

 $ psql -U hoge -d options='-c log_disconnections=off'
 = show log_disconnections ;
  log_disconnections
 
  off
 (1 row)

 = \du
  List of roles
  Role name |   Attributes   | Member of
 ---++---
  hoge  || {}
  postgres  | Superuser, Create role, Create DB, Replication | {}


 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when
 log_connections is set.


 That might be a good idea though.

 David pointed the merit of keeping those two parameters separate upthread
 and I understand his thought.
 http://www.postgresql.org/message-id/1402675662004-5807224.p...@n5.nabble.com

Let me explain the problem which I'd like to address here, again.

The problem is that any client (even non-superuser) can change the setting of
log_disconnections when it connects to the server. For example, you can do by
using options connection parameter as follows.

$ psql -U hoge -d options='-c log_disconnections=off'
= show log_disconnections ;
 log_disconnections

 off
(1 row)

This means that, even when DBA enables log_disconnections in postgresql.conf
in order to audit all the logouts, a client can freely avoid logging
of his or her
logout for some reasons. I think this situation problematic especially for audit
purpose.

You may think that logging logins is enough for audit purpose and
logging logouts
is not so important. But imagine the case where all logins are logged but some
corresponding logouts are not logged. This would make it difficult to check and
verify the validities of audit logs. It's not easy to handle such
users that only
login is logged.

Any client can enable/disable log_disconnections because it's defined with
PGC_BACKEND context. By definition, GUC parameters with PGC_BACKEND can
be changed at connection startup. But it cannot be changed after connection is
established.

BTW, log_connections is also defined with PGC_BACKEND, so any client can
change its setting at connection startup. But *fortunately* it's used
by the server
before the changed value is given from a client at connection startup. So, the
server always uses the setting value in postgresql.conf, whether a
client tries to
change log_connections or not. Therefore log_connections doesn't cause the
problem which I described the above at all. That's good for audit purpose. OTOH,
ISTM that defining log_connections with PGC_BACKEND is a bit strange since
a client cannot change it at all.

To address the problem, I'm thinking to change the GUC context of
log_disconnections from PGC_BACKEND to PGC_SIGHUP. This prevents a client
from changing the setting. In the top of the thread, I posted the patch which
changed the context to PGC_SUSET, but now I'm thinking PGC_SIGHUP is better.
Since log_disconnections is used (if it's enabled, the callback
function which logs
lotout is registered) just after connection is established, using PGC_SUSET and
allowing superuser to change the setting by SET command is useless.

Changing the GUC context of log_disconnections cause two behavior changes.

(1) As I explained the above, it prevents a client from changing the setting.
 Yeah, this is what I want.

(2) It allows DBA to change log_disconnections of running backends by
 reloading the configuration file. But such changed value has no effect on
 whether logout is logged or not 

Re: [HACKERS] Audit of logout

2014-06-23 Thread Fujii Masao
On Sat, Jun 21, 2014 at 12:59 PM, Joe Conway m...@joeconway.com wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 06/13/2014 07:29 AM, Tom Lane wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
 masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to
 audit all logouts. But since log_disconnections is defined with
 PGC_BACKEND, it can be changed at connection start. This means
 that any client (even nonsuperuser) can freely disable
 log_disconnections not to log his or her logout even when the
 system admin enables it in postgresql.conf. Isn't this
 problematic for audit?

 That's harmful for audit purpose. I think that we should make
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
 forbid non-superusers from changing its setting. Attached patch
 does this.

 This whole argument seems wrong unless I'm missing something:

 test=# set log_connections = on;
 ERROR:  parameter log_connections cannot be set after connection start
 test=# set log_disconnections = off;
 ERROR:  parameter log_disconnections cannot be set after connection
 start

You can change log_connections/disconnections via connection option as follows

$ grep log_disconnections $PGDATA/postgresql.conf
log_disconnections = on

$ psql -U hoge -d options='-c log_disconnections=off'
= show log_disconnections ;
 log_disconnections

 off
(1 row)

= \du
 List of roles
 Role name |   Attributes   | Member of
---++---
 hoge  || {}
 postgres  | Superuser, Create role, Create DB, Replication | {}


 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when
 log_connections is set.


 That might be a good idea though.

David pointed the merit of keeping those two parameters separate upthread
and I understand his thought.
http://www.postgresql.org/message-id/1402675662004-5807224.p...@n5.nabble.com

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] Audit of logout

2014-06-20 Thread Fujii Masao
On Fri, Jun 13, 2014 at 11:29 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to audit all 
 logouts.
 But since log_disconnections is defined with PGC_BACKEND, it can be changed
 at connection start. This means that any client (even nonsuperuser) can 
 freely
 disable log_disconnections not to log his or her logout even when the
 system admin
 enables it in postgresql.conf. Isn't this problematic for audit?

 That's harmful for audit purpose. I think that we should make
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order
 to forbid non-superusers from changing its setting. Attached
 patch does this.

 TBH, this complaint seems entirely artificial.  If somebody needs to audit
 disconnections, and they see a connection record with no disconnection
 record but the session's no longer there, they certainly know that a
 disconnection occurred.  And if there wasn't a server crash to explain it,
 they go slap the wrist of whoever violated corporate policy by turning off
 log_disconnections.  Why do we need system-level enforcement of this?

 Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
 setting, which was to try to prevent disconnections from being logged
 or not logged when the corresponding connection was not logged or logged
 respectively.  If an auditor wants the system to enforce that there are
 matched pairs of those records, he's not exactly going to be satisfied by
 being told that only superusers can cause them to not match.

 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when log_connections
 is set.

 Another answer is to make both variables PGC_SIGHUP, on the grounds
 that it doesn't make much sense for them not to be applied system-wide;
 except that I think there was some idea that logging might be enabled
 per-user or per-database using ALTER ROLE/DATABASE.

But, IIUC, since PGC_BACKEND parameters cannot be set per-role and per-database,
such idea seems impractical. No? ISTM that there is no big
user-visible problematic
change of the behavior even if we redefine log_connections and
log_disconnections
as PGC_SIGHUP.

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] Audit of logout

2014-06-20 Thread Joe Conway
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 06/13/2014 07:29 AM, Tom Lane wrote:
 Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao
 masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to
 audit all logouts. But since log_disconnections is defined with
 PGC_BACKEND, it can be changed at connection start. This means
 that any client (even nonsuperuser) can freely disable
 log_disconnections not to log his or her logout even when the 
 system admin enables it in postgresql.conf. Isn't this
 problematic for audit?
 
 That's harmful for audit purpose. I think that we should make 
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order to
 forbid non-superusers from changing its setting. Attached patch
 does this.

This whole argument seems wrong unless I'm missing something:

test=# set log_connections = on;
ERROR:  parameter log_connections cannot be set after connection start
test=# set log_disconnections = off;
ERROR:  parameter log_disconnections cannot be set after connection
start


 I wonder whether we should just get rid of log_disconnections as a 
 separate variable, instead logging disconnections when
 log_connections is set.


That might be a good idea though.


 Another answer is to make both variables PGC_SIGHUP, on the
 grounds that it doesn't make much sense for them not to be applied
 system-wide; except that I think there was some idea that logging
 might be enabled per-user or per-database using ALTER
 ROLE/DATABASE.


I don't think this is a good idea because of the reason you mention.

Joe


- -- 
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting,  24x7 Support
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTpQMcAAoJEDfy90M199hltHAP/2hEnKymoEq6zryaSpHZ2j0O
mj/8bEzCgYR/S4KUW8uqCzYK0g3HD5ncXJZkqpnaYvySV5YnopeUjuHaXxZOmuxx
GSbtmxo0wE5cYfEartVsX+ve0j7uSUwXBYZWD3em9FXNwFMnfVt3E/izwmHEnC7u
pIFHz6wKn6/QKaU9u/XRln4SZOAzeh4aYaHZy+5mhmGoU8fIJtZvdjEJSuAxxgzm
LMKGM/hgF23itpjjutDxQNoTUP+JGh0WzwqeW1t4+Y6T7HqXeTeT4IWsw3AH5sPg
e/NM+x4oeX9In6Gn4MLwT4R5Qai/JnaKGpzUv0jXlWPPvB23ilsb87eJ0BdbKDu1
LyxH16bH23DYL9LW+GAULRoMP78PLMKh4Mx2pe9KSL9tEBENvYpf+ew3IOfRmTlD
MAQRvhzspjPWp1AMQ9eNjX+63mpAeTBfHOBlVKUznhljHdDN7rcwpOzL82ecowDi
nM9bC+Me1jabaxRdu2cxt+p28BB5Ez3CX2wOz2JpM0ObruneoFhYCKXM9fUaD1d2
zJXiNtD7VgsUUtz+DGrNB32PyvzguhK0MXpX6/kRl5L1Xkpa4L+AV1nXWCkJYD6D
+btVgDscfnlWo1lQimq7B0KVET4zXnyI97vE7Xx0U7mvo8FZ8SQQHhbA7iy4P2SI
HUlqaKVcx2PLgoRAEEfL
=vQd8
-END PGP SIGNATURE-


-- 
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] Audit of logout

2014-06-17 Thread Robert Haas
On Mon, Jun 16, 2014 at 4:14 PM, Stephen Frost sfr...@snowman.net wrote:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Fujii Masao masao.fu...@gmail.com writes:
  That's harmful for audit purpose. I think that we should make
  log_disconnections PGC_SUSET rather than PGC_BACKEND in order
  to forbid non-superusers from changing its setting. Attached
  patch does this.

 I tend to agree with this- those are things which regular users really
 shouldn't be able to modify.  Policy enforcement can be done when there
 isn't system enforcement, but you really don't want to fall back to
 policy enforcement when you don't need to.

+1.

 TBH, this complaint seems entirely artificial.  If somebody needs to audit
 disconnections, and they see a connection record with no disconnection
 record but the session's no longer there, they certainly know that a
 disconnection occurred.  And if there wasn't a server crash to explain it,
 they go slap the wrist of whoever violated corporate policy by turning off
 log_disconnections.  Why do we need system-level enforcement of this?

 Going through every log file, and writing something to address log file
 changes, to hunt for those cases is no small amount of effort which
 you're asking every administrator with an audit requirement to write
 code to do to provide something which we make it appear as if we're
 doing for them.  It's certainly a violation of POLA that users can
 decide on their own that their disconnections don't get logged.

+1.

 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when log_connections
 is set.

 Another answer is to make both variables PGC_SIGHUP, on the grounds
 that it doesn't make much sense for them not to be applied system-wide;
 except that I think there was some idea that logging might be enabled
 per-user or per-database using ALTER ROLE/DATABASE.

 Both of these options look pretty reasonable to me as a way to improve
 the current situation.  I'm not really sure that I see the use-case for
 only logging connections/disconnections on a per-user or per-database
 basis; in my experience it's not a lot of traffic to log it all and I
 don't recall ever seeing anyone set those anything other than
 system-wide.

 I like the idea of flexibility in what's logged, I just don't see this
 specific case as really needing it.

I don't really like either of these ideas much, and I don't really see
the problem with Fujii Masao's original proposal.  It's true that some
installations may find it valuable to preserve the property that a
disconnect is logged whenever they connect is logged, but if that's
really what's at issue, then presumably the superuser is not going to
be flipping these settings on and off on the fly *anyway*.

-- 
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] Audit of logout

2014-06-16 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Fujii Masao masao.fu...@gmail.com writes:
  That's harmful for audit purpose. I think that we should make
  log_disconnections PGC_SUSET rather than PGC_BACKEND in order
  to forbid non-superusers from changing its setting. Attached
  patch does this.

I tend to agree with this- those are things which regular users really
shouldn't be able to modify.  Policy enforcement can be done when there
isn't system enforcement, but you really don't want to fall back to
policy enforcement when you don't need to.

 TBH, this complaint seems entirely artificial.  If somebody needs to audit
 disconnections, and they see a connection record with no disconnection
 record but the session's no longer there, they certainly know that a
 disconnection occurred.  And if there wasn't a server crash to explain it,
 they go slap the wrist of whoever violated corporate policy by turning off
 log_disconnections.  Why do we need system-level enforcement of this?

Going through every log file, and writing something to address log file
changes, to hunt for those cases is no small amount of effort which
you're asking every administrator with an audit requirement to write
code to do to provide something which we make it appear as if we're
doing for them.  It's certainly a violation of POLA that users can
decide on their own that their disconnections don't get logged.

 Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
 setting, which was to try to prevent disconnections from being logged
 or not logged when the corresponding connection was not logged or logged
 respectively.  If an auditor wants the system to enforce that there are
 matched pairs of those records, he's not exactly going to be satisfied by
 being told that only superusers can cause them to not match.

This is only accurate when a superuser exists in the system and even so,
superuser access can be much more easily reviewed as it's going to be a
lot less traffic and there may be other auditing mechanisms in place for
that access.

 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when log_connections
 is set.
 
 Another answer is to make both variables PGC_SIGHUP, on the grounds
 that it doesn't make much sense for them not to be applied system-wide;
 except that I think there was some idea that logging might be enabled
 per-user or per-database using ALTER ROLE/DATABASE.

Both of these options look pretty reasonable to me as a way to improve
the current situation.  I'm not really sure that I see the use-case for
only logging connections/disconnections on a per-user or per-database
basis; in my experience it's not a lot of traffic to log it all and I
don't recall ever seeing anyone set those anything other than
system-wide.

I like the idea of flexibility in what's logged, I just don't see this
specific case as really needing it. 

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Audit of logout

2014-06-16 Thread Abhijit Menon-Sen
At 2014-06-13 10:29:24 -0400, t...@sss.pgh.pa.us wrote:

 I wonder whether we should just get rid of log_disconnections as a
 separate variable, instead logging disconnections when log_connections
 is set.

I like that idea.

-- Abhijit


-- 
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] Audit of logout

2014-06-13 Thread Fujii Masao
On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Hi,

 Some users enable log_disconnections in postgresql.conf to audit all logouts.
 But since log_disconnections is defined with PGC_BACKEND, it can be changed
 at connection start. This means that any client (even nonsuperuser) can freely
 disable log_disconnections not to log his or her logout even when the
 system admin
 enables it in postgresql.conf. Isn't this problematic for audit?

That's harmful for audit purpose. I think that we should make
log_disconnections PGC_SUSET rather than PGC_BACKEND in order
to forbid non-superusers from changing its setting. Attached
patch does this.

Also defining log_disconnections with PGC_BACKEND itself seems strange.
Since it's used only at connection termination, there seems to be
no need to fix its setting value at connection startup. No? OTOH,
for example, log_connections and post_auth_delay are defined with
PGC_BACKEND and their settings can be changed only at connection startup.
This seems intuitive because they are used only at connection
startup and it's useless to change their settings after that. But
the situation of log_disconnections seems different from them.
Am I missing something?

One concern is; the patch may break the existing application if it
relies on the current behavior of log_disconnections. But I'm
wondering if such applications really exist.

Thought?

Regards,

-- 
Fujii Masao
From d3e6db1516a8cbb557e38a56b26c34ed7e51d9e1 Mon Sep 17 00:00:00 2001
From: MasaoFujii masao.fu...@gmail.com
Date: Fri, 13 Jun 2014 22:09:39 +0900
Subject: [PATCH] Make log_disconnections PGC_SUSET rather than PGC_BACKEND.

So far even non-superusers could disable log_disconnections in order to
prevent their session logout from being logged because the parameter was
defined with PGC_BACKEND. This was harmful in the systems which need to
audit all session logouts by using log_disconnections. For this problem,
this commit changes the GUC context of log_disconnections to PGC_SUSET
and forbids non-superuser from changing its setting.
---
 doc/src/sgml/config.sgml |2 +-
 src/backend/utils/misc/guc.c |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 697cf99..184d864 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4234,7 +4234,7 @@ local0.*/var/log/postgresql
 varnamelog_connections/varname but at session termination,
 and includes the duration of the session.  This is off by
 default.
-This parameter cannot be changed after session start.
+Only superusers can change this setting.
/para
   /listitem
  /varlistentry
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1d094f0..7c84c9f 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -922,7 +922,7 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 	{
-		{log_disconnections, PGC_BACKEND, LOGGING_WHAT,
+		{log_disconnections, PGC_SUSET, LOGGING_WHAT,
 			gettext_noop(Logs end of a session, including duration.),
 			NULL
 		},
-- 
1.7.1


-- 
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] Audit of logout

2014-06-13 Thread Tom Lane
Fujii Masao masao.fu...@gmail.com writes:
 On Thu, Jun 12, 2014 at 8:51 PM, Fujii Masao masao.fu...@gmail.com wrote:
 Some users enable log_disconnections in postgresql.conf to audit all logouts.
 But since log_disconnections is defined with PGC_BACKEND, it can be changed
 at connection start. This means that any client (even nonsuperuser) can 
 freely
 disable log_disconnections not to log his or her logout even when the
 system admin
 enables it in postgresql.conf. Isn't this problematic for audit?

 That's harmful for audit purpose. I think that we should make
 log_disconnections PGC_SUSET rather than PGC_BACKEND in order
 to forbid non-superusers from changing its setting. Attached
 patch does this.

TBH, this complaint seems entirely artificial.  If somebody needs to audit
disconnections, and they see a connection record with no disconnection
record but the session's no longer there, they certainly know that a
disconnection occurred.  And if there wasn't a server crash to explain it,
they go slap the wrist of whoever violated corporate policy by turning off
log_disconnections.  Why do we need system-level enforcement of this?

Moreover, your proposed fix breaks the entire point of the PGC_BACKEND
setting, which was to try to prevent disconnections from being logged
or not logged when the corresponding connection was not logged or logged
respectively.  If an auditor wants the system to enforce that there are
matched pairs of those records, he's not exactly going to be satisfied by
being told that only superusers can cause them to not match.

I wonder whether we should just get rid of log_disconnections as a
separate variable, instead logging disconnections when log_connections
is set.

Another answer is to make both variables PGC_SIGHUP, on the grounds
that it doesn't make much sense for them not to be applied system-wide;
except that I think there was some idea that logging might be enabled
per-user or per-database using ALTER ROLE/DATABASE.

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] Audit of logout

2014-06-13 Thread David G Johnston
Tom Lane-2 wrote
 Another answer is to make both variables PGC_SIGHUP, on the grounds
 that it doesn't make much sense for them not to be applied system-wide;
 except that I think there was some idea that logging might be enabled
 per-user or per-database using ALTER ROLE/DATABASE.

From a trouble-shooting standpoint if I know that client software in
question is focused on particular users/databases being able to only enable
connection logging for those would make sense.  Whether that is a true
production concern is another matter but the possibility does exist.

I personally do not get how a logoff is a risk auditing event.  Logons and
actual queries I can understand.

For the same reason keeping them separate has merit since for imaginable
circumstances the logoffs are noise but the logons are important - and
forcing them to be on/off in tandem disallows the option to remove the noise
from the logs.

David J.






--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/Audit-of-logout-tp5806985p5807224.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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


[HACKERS] Audit of logout

2014-06-12 Thread Fujii Masao
Hi,

Some users enable log_disconnections in postgresql.conf to audit all logouts.
But since log_disconnections is defined with PGC_BACKEND, it can be changed
at connection start. This means that any client (even nonsuperuser) can freely
disable log_disconnections not to log his or her logout even when the
system admin
enables it in postgresql.conf. Isn't this problematic for audit?

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