Re: [HACKERS] log files and permissions

[Martin Pihlak]

Tom Lane wrote:
 Itagaki Takahiro itagaki.takah...@gmail.com writes:
 ...
 We should also check the value not to be something like 0699.
 How about checking it with (file_mode  ~0666) != 0 ?
 ...
 I want show_log_file_mode to print the setting value in octal format.
 
 It seems like a whole lot of lily-gilding is going on here.  Just make
 it work like unix_socket_permissions already does.  That's been there
 for years and nobody has complained about it.
 

Thanks, somehow I missed that we can already specify octal integers
as GUC-s. I now converted the log_file_mode to integer and dropped
the assign_log_file_mode function.

regards,
Martin
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2789,2794  local0.*/var/log/postgresql
--- 2789,2817 
/listitem
   /varlistentry
  
+  varlistentry id=guc-log-file-mode xreflabel=log_file_mode
+   termvarnamelog_file_mode/varname (typeinteger/type)/term
+   indexterm
+primaryvarnamelog_file_mode/ configuration parameter/primary
+   /indexterm
+   listitem
+para
+ On Unix systems this parameter sets the permissions for log files
+ when varnamelogging_collector/varname is enabled. On Microsoft
+ Windows the file mode will be ignored.
+ The parameter value is expected to be a numeric mode
+ specified in the format accepted by the
+ functionchmod/function and functionumask/function
+ system calls.  (To use the customary octal format the number
+ must start with a literal0/literal (zero).)
+/para
+para
+ This parameter can only be set in the filenamepostgresql.conf/
+ file or on the server command line.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-rotation-age xreflabel=log_rotation_age
termvarnamelog_rotation_age/varname (typeinteger/type)/term
indexterm
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***
*** 73,78  int			Log_RotationSize = 10 * 1024;
--- 73,79 
  char	   *Log_directory = NULL;
  char	   *Log_filename = NULL;
  bool		Log_truncate_on_rotation = false;
+ int			Log_file_mode = 0600;
  
  /*
   * Globally visible state (used by elog.c)
***
*** 135,140  static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool am_rotating);
  
  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***
*** 516,530  SysLogger_Start(void)
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = fopen(filename, a);
! 
! 	if (!syslogFile)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(syslogFile, NULL, LBF_MODE, 0);
  
  	pfree(filename);
  
--- 518,524 
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = logfile_open(filename, a, false);
  
  	pfree(filename);
  
***
*** 1004,1027  open_csvlogfile(void)
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = fopen(filename, a);
  
! 	if (!fh)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
  
! 	setvbuf(fh, NULL, LBF_MODE, 0);
  
! #ifdef WIN32
! 	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
! #endif
  
! 	csvlogFile = fh;
  
! 	pfree(filename);
  
  }
  
  /*
--- 998,1050 
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = logfile_open(filename, a, false);
  
! 	csvlogFile = fh;
  
! 	pfree(filename);
  
! }
  
! /*
!  * Open the logfile with proper permissions and buffering options.
!  *
!  * We ignore any errors if the logfile is opened as part of log rotation. In
!  * other cases the errors are treated as fatal.
!  */
! static FILE *
! logfile_open(const char *filename, const char *mode, bool am_rotating)
! {
! 	mode_t	oumask;
! 	FILE   *fh;
  
! 	oumask = umask((mode_t) (~Log_file_mode  0777));
! 	fh = fopen(filename, mode);
! 	umask(oumask);
! 
! 	if (fh)
! 	{
! 		setvbuf(fh, NULL, LBF_MODE, 0);
! 
! #ifdef WIN32
! 		_setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
! #endif
! 	}
! 	else
! 	{
! 		if (am_rotating)
! 			ereport(LOG,
! 	(errcode_for_file_access(),
! 	 errmsg(could not create new log file \%s\: %m,
! 			filename)));
! 		else
! 			ereport(FATAL,
! 	(errcode_for_file_access(),
! 	 errmsg(could not create log file \%s\: %m,
! 			filename)));
! 	}
  
+ 	return fh;
  }
  
  /*
***
*** 1070,1088  logfile_rotate(bool time_based_rotation, int size_rotation_for)
  		if 

Re: [HACKERS] log files and permissions

[Tom Lane]

Martin Pihlak martin.pih...@gmail.com writes:
 Thanks, somehow I missed that we can already specify octal integers
 as GUC-s. I now converted the log_file_mode to integer and dropped
 the assign_log_file_mode function.

Applied with a few corrections.  The noncosmetic changes were:

* prevent Log_file_mode from disabling S_IWUSR permissions --- we had
better be able to write the files no matter what.

* save and restore errno across ereport() call; needed since some
callers look at errno after a failure.

* make unix_socket_permissions print its value in octal, for consistency
  with log_file_mode.

BTW, I'm not 100% convinced that having the octal show-functions is
a good idea, mainly because they aren't consistent with the other
columns in pg_settings:

regression=# select * from pg_settings where name = 'log_file_mode';
 name  | setting | unit |   category   |
short_desc| 
  extra_desc
| co
ntext | vartype | source  | min_val | max_val | enumvals | boot_val | reset_val 
| sourcefile | sourceline 
---+-+--+--+
--+-

+---
--+-+-+-+-+--+--+---
++
 log_file_mode | 0600|  | Reporting and Logging / Where to Log | Sets th
e file permissions for log files. | The parameter value is expected to be a nume
ric mode specification in the form accepted by the chmod and umask system calls.
 (To use the customary octal format the number must start with a 0 (zero).) | si
ghup  | integer | default | 0   | 511 |  | 384  | 384   
||   
(1 row)

I guess this is not strictly incorrect, as long as you understand what
the leading '0' means per C conventions, but it looks a bit weird.
However, we're not going to be able to improve on this without a lot more
hackery than I think it's worth.

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] log files and permissions

[Martin Pihlak]

Itagaki Takahiro wrote:
 I think the patch is almost ready for committer except the following
 three issues:
 
 2010/7/13 Fujii Masao masao.fu...@gmail.com:
 + if (!*value || *endptr || file_mode  0 || file_mode  0777)
 The sticky bit cannot be set via log_file_mode. Is this intentional? 

Yes -- I don't think there is a use case for sticky or setuid bits on log
files, even allowing execute is questionable.

 We should also check the value not to be something like 0699.
 How about checking it with (file_mode  ~0666) != 0 ?

Aha, that would ensure that the execute bit is not specified. Works for me.
The 0699 and other invalid octal values are caught by strtol()

 +#ifndef WIN32
 + chmod(filename, Log_file_mode);
 +#endif
 Don't we need to check the return value of chmod()?
 
 I prefer umask() rather than chmod() here.
 

Converted to umask()

 +const char *show_log_file_mode(void);
 You forgot to write the show_log_file_mode()? I was not able to find that
 in the patch.
 
 I want show_log_file_mode to print the setting value in octal format.
 

I've now (re)added the show_log_file_mode(). It used to be there, but then
at some point I decided to display the value as-is.

While going through it, I moved the _setmode() call for win32 to logfile_open().

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2789,2794  local0.*/var/log/postgresql
--- 2789,2815 
/listitem
   /varlistentry
  
+  varlistentry id=guc-log-file-mode xreflabel=log_file_mode
+   termvarnamelog_file_mode/varname (typeinteger/type)/term
+   indexterm
+primaryvarnamelog_file_mode/ configuration parameter/primary
+   /indexterm
+   listitem
+para
+ On Unix systems this parameter sets the permissions for log files
+ when varnamelogging_collector/varname is enabled. On Microsoft
+ Windows the file mode will be ignored. The value is an octal number
+ consisting of 3 digits signifying the permissions for the user, group
+ and others. Specifying execute permissions for log files results in
+ an error.
+/para
+para
+ This parameter can only be set in the filenamepostgresql.conf/
+ file or on the server command line.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-rotation-age xreflabel=log_rotation_age
termvarnamelog_rotation_age/varname (typeinteger/type)/term
indexterm
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 914,916  show_role(void)
--- 914,956 
  
  	return endptr + 1;
  }
+ 
+ 
+ /*
+  * LOG_FILE_MODE
+  */
+ 
+ extern int Log_file_mode;		/* in guc.c */
+ 
+ /*
+  * assign_log_file_mode: GUC assign_hook for log_file_mode
+  */
+ const char *
+ assign_log_file_mode(const char *value, bool doit, GucSource source)
+ {
+ 	char *endptr;
+ 	long file_mode = strtol(value, endptr, 8);
+ 
+ 	if (!*value || *endptr || file_mode  0 || (file_mode  ~0666) != 0)
+ 	{
+ 		ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(invalid value for parameter \log_file_mode\),
+  errhint(value must be a 3 digit octal number, with the execute bit not set)));
+ 		return NULL;
+ 	}
+ 
+ 	if (doit)
+ 		Log_file_mode = (int) file_mode;
+ 
+ 	return value;
+ }
+ 
+ const char *
+ show_log_file_mode(void)
+ {
+ 	static char buf[5];
+ 
+ 	snprintf(buf, sizeof(buf), %04o, Log_file_mode);
+ 	return buf;
+ }
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***
*** 73,78  int			Log_RotationSize = 10 * 1024;
--- 73,79 
  char	   *Log_directory = NULL;
  char	   *Log_filename = NULL;
  bool		Log_truncate_on_rotation = false;
+ int			Log_file_mode = 0600;
  
  /*
   * Globally visible state (used by elog.c)
***
*** 135,140  static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool am_rotating);
  
  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***
*** 516,530  SysLogger_Start(void)
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = fopen(filename, a);
! 
! 	if (!syslogFile)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(syslogFile, NULL, LBF_MODE, 0);
  
  	pfree(filename);
  
--- 518,524 
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = logfile_open(filename, a, false);
  
  	pfree(filename);
  
***
*** 1004,1027  open_csvlogfile(void)
  
  	filename = 

Re: [HACKERS] log files and permissions

[Tom Lane]

Itagaki Takahiro itagaki.takah...@gmail.com writes:
 ...
 We should also check the value not to be something like 0699.
 How about checking it with (file_mode  ~0666) != 0 ?
 ...
 I want show_log_file_mode to print the setting value in octal format.

It seems like a whole lot of lily-gilding is going on here.  Just make
it work like unix_socket_permissions already does.  That's been there
for years and nobody has complained about it.

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] log files and permissions

[Martin Pihlak]

Itagaki Takahiro wrote:
 I checked log_file_mode GUC patch, and found a couple of Windows-specific
 and translation issues.

Thank you for the review. Attached patch attempts to fix these issues.

 * fchmod() is not available on some platforms, including Windows.
 fh = fopen(filename, mode);
 setvbuf(fh, NULL, LBF_MODE, 0);
 fchmod(fileno(fh), Log_file_mode);
 

I've changed that to using chmod(), that should be available everywhere and
is already used in several places in Postgres code.

 * How does the file mode work on Windows?
 If it doesn't work, we should explain it in docs.

Indeed it seems that chmod() doesn't actually do anything useful on Windows.
So I've added a documentation note about it and put an #ifndef WIN32 around
the chmod() call.

 It might look a duplication of codes, but I think this form is better
 because we can reuse the existing translation catalogs.
 if (am_rotating)
 ereport(FATAL, ... could not create log file ...);
 else
 ereport(LOG, ... could not open new log file ...);
 

Fixed.

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2789,2794  local0.*/var/log/postgresql
--- 2789,2814 
/listitem
   /varlistentry
  
+  varlistentry id=guc-log-file-mode xreflabel=log_file_mode
+   termvarnamelog_file_mode/varname (typeinteger/type)/term
+   indexterm
+primaryvarnamelog_file_mode/ configuration parameter/primary
+   /indexterm
+   listitem
+para
+ On Unix systems this parameter sets the permissions for log files
+ when varnamelogging_collector/varname is enabled. On Microsoft
+ Windows the file mode will be ignored. The value is an octal number
+ consisting of 3 digits signifying the permissions for the user, group
+ and others.
+/para
+para
+ This parameter can only be set in the filenamepostgresql.conf/
+ file or on the server command line.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-rotation-age xreflabel=log_rotation_age
termvarnamelog_rotation_age/varname (typeinteger/type)/term
indexterm
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 914,916  show_role(void)
--- 914,946 
  
  	return endptr + 1;
  }
+ 
+ 
+ /*
+  * LOG_FILE_MODE
+  */
+ 
+ extern int Log_file_mode;		/* in guc.c */
+ 
+ /*
+  * assign_log_file_mode: GUC assign_hook for log_file_mode
+  */
+ const char *
+ assign_log_file_mode(const char *value, bool doit, GucSource source)
+ {
+ 	char *endptr;
+ 	long file_mode = strtol(value, endptr, 8);
+ 
+ 	if (!*value || *endptr || file_mode  0 || file_mode  0777)
+ 	{
+ 		ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(invalid value for parameter \log_file_mode\)));
+ 		return NULL;
+ 	}
+ 
+ 	if (doit)
+ 		Log_file_mode = (int) file_mode;
+ 
+ 	return value;
+ }
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***
*** 73,78  int			Log_RotationSize = 10 * 1024;
--- 73,79 
  char	   *Log_directory = NULL;
  char	   *Log_filename = NULL;
  bool		Log_truncate_on_rotation = false;
+ int			Log_file_mode = 0600;
  
  /*
   * Globally visible state (used by elog.c)
***
*** 135,140  static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool am_rotating);
  
  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***
*** 516,530  SysLogger_Start(void)
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = fopen(filename, a);
! 
! 	if (!syslogFile)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(syslogFile, NULL, LBF_MODE, 0);
  
  	pfree(filename);
  
--- 518,524 
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = logfile_open(filename, a, false);
  
  	pfree(filename);
  
***
*** 1004,1018  open_csvlogfile(void)
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = fopen(filename, a);
! 
! 	if (!fh)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(fh, NULL, LBF_MODE, 0);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
--- 998,1004 
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = logfile_open(filename, a, false);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */

Re: [HACKERS] log files and permissions

[Fujii Masao]

On Mon, Jul 12, 2010 at 7:36 PM, Martin Pihlak martin.pih...@gmail.com wrote:
 Itagaki Takahiro wrote:
 I checked log_file_mode GUC patch, and found a couple of Windows-specific
 and translation issues.

 Thank you for the review. Attached patch attempts to fix these issues.

 + if (!*value || *endptr || file_mode  0 || file_mode  0777)
 + {
 + ereport(GUC_complaint_elevel(source),
 + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 +  errmsg(invalid value for parameter 
 \log_file_mode\)));

The sticky bit cannot be set via log_file_mode. Is this intentional?

 +#ifndef WIN32
 + chmod(filename, Log_file_mode);
 +#endif

Don't we need to check the return value of chmod()?

 +const char *assign_log_file_mode(const char *value,
 + bool doit, GucSource 
 source);
 +const char *show_log_file_mode(void);

You forgot to write the show_log_file_mode()? I was not able to find that
in the patch.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

-- 
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] log files and permissions

[Itagaki Takahiro]

I think the patch is almost ready for committer except the following
three issues:

2010/7/13 Fujii Masao masao.fu...@gmail.com:
 +     if (!*value || *endptr || file_mode  0 || file_mode  0777)
 The sticky bit cannot be set via log_file_mode. Is this intentional?

We should also check the value not to be something like 0699.
How about checking it with (file_mode  ~0666) != 0 ?

 +#ifndef WIN32
 +             chmod(filename, Log_file_mode);
 +#endif
 Don't we need to check the return value of chmod()?

I prefer umask() rather than chmod() here.

 +const char *show_log_file_mode(void);
 You forgot to write the show_log_file_mode()? I was not able to find that
 in the patch.

I want show_log_file_mode to print the setting value in octal format.

-- 
Itagaki Takahiro

-- 
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] log files and permissions

[Itagaki Takahiro]

I checked log_file_mode GUC patch, and found a couple of Windows-specific
and translation issues.

* fchmod() is not available on some platforms, including Windows.
fh = fopen(filename, mode);
setvbuf(fh, NULL, LBF_MODE, 0);
fchmod(fileno(fh), Log_file_mode);

I think umask()-fopen() is better rather than fopen()-chmod().
See codes in DoCopyTo() at commands/copy.c.

* How does the file mode work on Windows?
If it doesn't work, we should explain it in docs.
Description for .pgpass for Windows might be a help.
| http://developer.postgresql.org/pgdocs/postgres/libpq-pgpass.html
| On Microsoft Windows, ... no special permissions check is made.

* This message format is hard to translate.
ereport(am_rotating ? LOG : FATAL,
  (errcode_for_file_access(),
   (errmsg(could not create%slog file \%s\: %m,
   am_rotating ?  new  :  , filename;

It might look a duplication of codes, but I think this form is better
because we can reuse the existing translation catalogs.
if (am_rotating)
ereport(FATAL, ... could not create log file ...);
else
ereport(LOG, ... could not open new log file ...);

-- 
Itagaki Takahiro

-- 
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] log files and permissions

[Martin Pihlak]

Martin Pihlak wrote:
 Attached is a patch that adds a GUC log_file_mode which allows to specify
 the creation mode for the log files. Presently it lacks documentation, which
 I'll add if the idea is generally acceptable.
 

Updated patch attached.

regards,
Martin

*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2789,2794  local0.*/var/log/postgresql
--- 2789,2813 
/listitem
   /varlistentry
  
+  varlistentry id=guc-log-file-mode xreflabel=log_file_mode
+   termvarnamelog_file_mode/varname (typeinteger/type)/term
+   indexterm
+primaryvarnamelog_file_mode/ configuration parameter/primary
+   /indexterm
+   listitem
+para
+ When varnamelogging_collector/varname is enabled,
+ this parameter sets the permissions of the created log files.
+ 	The value is an octal number consisting of 3 digits signifying
+ 	the permissions for the user, group and others.
+/para
+para
+ This parameter can only be set in the filenamepostgresql.conf/
+ file or on the server command line.
+/para
+   /listitem
+  /varlistentry
+ 
   varlistentry id=guc-log-rotation-age xreflabel=log_rotation_age
termvarnamelog_rotation_age/varname (typeinteger/type)/term
indexterm
*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 914,916  show_role(void)
--- 914,946 
  
  	return endptr + 1;
  }
+ 
+ 
+ /*
+  * LOG_FILE_MODE
+  */
+ 
+ extern int Log_file_mode;		/* in guc.c */
+ 
+ /*
+  * assign_log_file_mode: GUC assign_hook for log_file_mode
+  */
+ const char *
+ assign_log_file_mode(const char *value, bool doit, GucSource source)
+ {
+ 	char *endptr;
+ 	long file_mode = strtol(value, endptr, 8);
+ 
+ 	if (!*value || *endptr || file_mode  0 || file_mode  0777)
+ 	{
+ 		ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(invalid value for parameter \log_file_mode\)));
+ 		return NULL;
+ 	}
+ 
+ 	if (doit)
+ 		Log_file_mode = (int) file_mode;
+ 
+ 	return value;
+ }
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***
*** 73,78  int			Log_RotationSize = 10 * 1024;
--- 73,79 
  char	   *Log_directory = NULL;
  char	   *Log_filename = NULL;
  bool		Log_truncate_on_rotation = false;
+ int			Log_file_mode = 0600;
  
  /*
   * Globally visible state (used by elog.c)
***
*** 135,140  static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool am_rotating);
  
  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***
*** 516,530  SysLogger_Start(void)
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = fopen(filename, a);
! 
! 	if (!syslogFile)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(syslogFile, NULL, LBF_MODE, 0);
  
  	pfree(filename);
  
--- 518,524 
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = logfile_open(filename, a, false);
  
  	pfree(filename);
  
***
*** 1004,1018  open_csvlogfile(void)
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = fopen(filename, a);
! 
! 	if (!fh)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(fh, NULL, LBF_MODE, 0);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
--- 998,1004 
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = logfile_open(filename, a, false);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
***
*** 1025,1030  open_csvlogfile(void)
--- 1011,1040 
  }
  
  /*
+  * Open the logfile, set permissions and buffering options.
+  */
+ static FILE *
+ logfile_open(const char *filename, const char *mode, bool am_rotating)
+ {
+ 	FILE   *fh;
+ 
+ 	fh = fopen(filename, mode);
+ 
+ 	if (fh)
+ 	{
+ 		setvbuf(fh, NULL, LBF_MODE, 0);
+ 		fchmod(fileno(fh), Log_file_mode);
+ 	}
+ 	else
+ 		ereport(am_rotating ? LOG : FATAL,
+ (errcode_for_file_access(),
+  (errmsg(could not create%slog file \%s\: %m,
+ 		 am_rotating ?  new  :  , filename;
+ 
+ 	return fh;
+ }
+ 
+ /*
   * perform logfile rotation
   */
  static void
***
*** 1070,1088  logfile_rotate(bool time_based_rotation, int size_rotation_for)
  		if (Log_truncate_on_rotation  time_based_rotation 
  			last_file_name != NULL 
  			strcmp(filename, last_file_name) != 0)
! 			fh = 

[HACKERS] log files and permissions

[Martin Pihlak]

With logging_collector enabled, all the postgres log files are created with
mode 0600.  This makes life complicated if users other than postgres need
to be able to examine the log files as well. Common example of this is when the
database runs under postgres user and DBA-s have named accounts. In order to
examine the log files the DBA then has to go through extra steps to sudo to
postgres or equivalent. Another example would be a monitoring script that
runs as an unprivileged user but needs to tail the log files.

It'd be convenient if the log files would have group read access. Then we could
make all the DBA or monitoring users members of the postgres group and they'd
have direct access to the logs. However, as the group read is not likely a
universally correct setting, the creation mode needs to be configurable.

Attached is a patch that adds a GUC log_file_mode which allows to specify
the creation mode for the log files. Presently it lacks documentation, which
I'll add if the idea is generally acceptable.

PS. I have no idea how all of this would work on Windows, maybe it's not
event relevant there?

regards,
Martin

-- 
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] log files and permissions

[Martin Pihlak]

Martin Pihlak wrote:
 Attached is a patch that adds a GUC log_file_mode which allows to specify
 the creation mode for the log files. Presently it lacks documentation, which
 I'll add if the idea is generally acceptable.
 

Now it really is attached.

regards,
Martin

*** a/src/backend/commands/variable.c
--- b/src/backend/commands/variable.c
***
*** 914,916  show_role(void)
--- 914,947 
  
  	return endptr + 1;
  }
+ 
+ 
+ /*
+  * LOG_FILE_MODE
+  */
+ 
+ extern int Log_file_mode;		/* in guc.c */
+ 
+ /*
+  * assign_log_file_mode: GUC assign_hook for log_file_mode
+  */
+ const char *
+ assign_log_file_mode(const char *value, bool doit, GucSource source)
+ {
+ 	int file_mode;
+ 
+ 	/* Parse the octal mode, complain if invalid */
+ 	if (sscanf(value, %o, file_mode) != 1 || file_mode  0777)
+ 	{
+ 		ereport(GUC_complaint_elevel(source),
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(invalid value for parameter \log_file_mode\)));
+ 		return NULL;
+ 	}
+ 
+ 	if (doit)
+ 		Log_file_mode = file_mode;
+ 
+ 	return value;
+ }
+ 
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***
*** 73,78  int			Log_RotationSize = 10 * 1024;
--- 73,79 
  char	   *Log_directory = NULL;
  char	   *Log_filename = NULL;
  bool		Log_truncate_on_rotation = false;
+ int			Log_file_mode = 0600;
  
  /*
   * Globally visible state (used by elog.c)
***
*** 135,140  static void syslogger_parseArgs(int argc, char *argv[]);
--- 136,142 
  static void process_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void flush_pipe_input(char *logbuffer, int *bytes_in_logbuffer);
  static void open_csvlogfile(void);
+ static FILE *logfile_open(const char *filename, const char *mode, bool die_on_error);
  
  #ifdef WIN32
  static unsigned int __stdcall pipeThread(void *arg);
***
*** 516,530  SysLogger_Start(void)
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = fopen(filename, a);
! 
! 	if (!syslogFile)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(syslogFile, NULL, LBF_MODE, 0);
  
  	pfree(filename);
  
--- 518,524 
  	 */
  	filename = logfile_getname(time(NULL), NULL);
  
! 	syslogFile = logfile_open(filename, a, true);
  
  	pfree(filename);
  
***
*** 1004,1018  open_csvlogfile(void)
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = fopen(filename, a);
! 
! 	if (!fh)
! 		ereport(FATAL,
! (errcode_for_file_access(),
!  (errmsg(could not create log file \%s\: %m,
! 		 filename;
! 
! 	setvbuf(fh, NULL, LBF_MODE, 0);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
--- 998,1004 
  
  	filename = logfile_getname(time(NULL), .csv);
  
! 	fh = logfile_open(filename, a, true);
  
  #ifdef WIN32
  	_setmode(_fileno(fh), _O_TEXT);		/* use CRLF line endings on Windows */
***
*** 1025,1030  open_csvlogfile(void)
--- 1011,1040 
  }
  
  /*
+  * Open the logfile, set permissions and buffering options.
+  */
+ static FILE *
+ logfile_open(const char *filename, const char *mode, bool die_on_error)
+ {
+ 	FILE   *fh;
+ 
+ 	fh = fopen(filename, mode);
+ 
+ 	if (fh)
+ 	{
+ 		setvbuf(fh, NULL, LBF_MODE, 0);
+ 		fchmod(fileno(fh), Log_file_mode);
+ 	}
+ 	else
+ 		ereport(die_on_error ? FATAL : LOG,
+ (errcode_for_file_access(),
+  (errmsg(could not create log file \%s\: %m,
+ 		 filename;
+ 
+ 	return fh;
+ }
+ 
+ /*
   * perform logfile rotation
   */
  static void
***
*** 1070,1088  logfile_rotate(bool time_based_rotation, int size_rotation_for)
  		if (Log_truncate_on_rotation  time_based_rotation 
  			last_file_name != NULL 
  			strcmp(filename, last_file_name) != 0)
! 			fh = fopen(filename, w);
  		else
! 			fh = fopen(filename, a);
  
  		if (!fh)
  		{
  			int			saveerrno = errno;
  
- 			ereport(LOG,
- 	(errcode_for_file_access(),
- 	 errmsg(could not open new log file \%s\: %m,
- 			filename)));
- 
  			/*
  			 * ENFILE/EMFILE are not too surprising on a busy system; just
  			 * keep using the old file till we manage to get a new one.
--- 1080,1093 
  		if (Log_truncate_on_rotation  time_based_rotation 
  			last_file_name != NULL 
  			strcmp(filename, last_file_name) != 0)
! 			fh = logfile_open(filename, w, false);
  		else
! 			fh = logfile_open(filename, a, false);
  
  		if (!fh)
  		{
  			int			saveerrno = errno;
  
  			/*
  			 * ENFILE/EMFILE are not too surprising on a busy system; just
  			 * keep using the old file till we manage to get a new one.
***
*** 1128,1146  logfile_rotate(bool time_based_rotation, int size_rotation_for)
  		if (Log_truncate_on_rotation  time_based_rotation 
  			last_csv_file_name != NULL 
  			strcmp(csvfilename, last_csv_file_name) != 0)
! 			fh = fopen(csvfilename, w);
  		

Re: [HACKERS] log files and permissions

[Tom Lane]

Martin Pihlak martin.pih...@gmail.com writes:
 It'd be convenient if the log files would have group read access. Then we 
 could
 make all the DBA or monitoring users members of the postgres group and they'd
 have direct access to the logs. However, as the group read is not likely a
 universally correct setting, the creation mode needs to be configurable.

It doesn't appear to me that this helps unless you are willing to make
the containing director(ies) group-readable/executable as well, which is
something we've resisted doing.

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] log files and permissions

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote:
 Martin Pihlak martin.pih...@gmail.com writes:
 It'd be convenient if the log files would have group read access.
 Then we could make all the DBA or monitoring users members of the
 postgres group and they'd have direct access to the logs.
 However, as the group read is not likely a universally correct
 setting, the creation mode needs to be configurable.
 
 It doesn't appear to me that this helps unless you are willing to
 make the containing director(ies) group-readable/executable as
 well, which is something we've resisted doing.
 
I just tried creating a symbolic link to the pg_log directory and
flagging the existing logs within it to 640.  As a member of the
group I was able to list and view the contents of log files through
the symbolic link, even though I didn't have any authority to the
PostgreSQL data directory.
 
That seems potentially useful to me.
 
-Kevin

-- 
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] log files and permissions

[Martin Pihlak]

Tom Lane wrote:
 It doesn't appear to me that this helps unless you are willing to make
 the containing director(ies) group-readable/executable as well, which is
 something we've resisted doing.
 

The log can be moved outside of data directory by setting log_directory
to an absolute path. Then the permissions for the log directory can be arbitrary
as the postmaster is only strict about permissions on data directory.

regards,
Martin

-- 
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] log files and permissions

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Martin Pihlak martin.pih...@gmail.com writes:
  It'd be convenient if the log files would have group read access. Then we 
  could
  make all the DBA or monitoring users members of the postgres group and 
  they'd
  have direct access to the logs. However, as the group read is not likely a
  universally correct setting, the creation mode needs to be configurable.
 
 It doesn't appear to me that this helps unless you are willing to make
 the containing director(ies) group-readable/executable as well, which is
 something we've resisted doing.

Perhaps we should have a umask-like GUC instead of this?

In the end, I agree with and completely understand the OP's complaint.
I havn't run into this issue much since, on Debian systems, we use
logrotate to move log files around and use the copy/truncate method
there, so permissions end up being preserved once an admin has decided
to change them.  Might be something to consider, but, really, we should
give the admin some flexibility here, even if the default is the same as
current behaviour.

I'll refrain from bringing up the fact that we're concerned about log
files having group permissions by default, but we ship with trust in
pg_hba.conf...

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] log files and permissions

[Tom Lane]

Martin Pihlak martin.pih...@gmail.com writes:
 Tom Lane wrote:
 It doesn't appear to me that this helps unless you are willing to make
 the containing director(ies) group-readable/executable as well, which is
 something we've resisted doing.

 The log can be moved outside of data directory by setting log_directory
 to an absolute path.

Oh, of course.  We'd need to mention that in the documentation for the
log-file-permission GUC.

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] log files and permissions

[Michael Tharp]

On 07/01/2010 12:56 PM, Kevin Grittner wrote:

I just tried creating a symbolic link to the pg_log directory and
flagging the existing logs within it to 640.  As a member of the
group I was able to list and view the contents of log files through
the symbolic link, even though I didn't have any authority to the
PostgreSQL data directory.

That seems potentially useful to me.


Symlinks are exactly equivalent to using the target of the link. Your 
permissions are probably already arranged so that you (as a group 
member) can access the files. Fedora's initscript seems to deliberately 
revoke group permissions from PGDATA and pg_log so I'm guessing that at 
some point some things were created with some group permissions.


That said, as Martin mentions one can easily place the log directory 
outside of the data directory and set appropriate directory permissions.


-- m. tharp

--
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] log files and permissions

[Stephen J. Butler]

On Thu, Jul 1, 2010 at 12:19 PM, Michael Tharp
g...@partiallystapled.com wrote:
 That said, as Martin mentions one can easily place the log directory outside
 of the data directory and set appropriate directory permissions.

If I can offer my $0.02, I recently solved such a problem on SuSE
Linux with apache logs. I used the ACL support on ext3 to give a
specific group read-only access:

cd /var/log
# Add an ACL for the 'www' user
setfacl -m u:www:r-x apache2
setfacl -m u:www:r-- apache2/*
# Modify the default ACL so that new files get 'r' for user
setfacl -d -m u:www:r-- apache2

Just pointing out that this problem is solvable on systems that
support ACLs w/o patching postgres.

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