[ovs-dev,2/2] vlog: change log file owner when switching user
diff mbox

Message ID 1444464456-27941-2-git-send-email-azhou@nicira.com
State Superseded
Headers show

Commit Message

Andy Zhou Oct. 10, 2015, 8:07 a.m. UTC
vlog log file can be created when parsing --log-file option, before
switch user, in case the --user option is also specified. This
does not directly causing errors for the running daemons, but leaves
the log files on disk as owned by root. It can be confusing at best.

This patch fixes the log file ownership setting to match with the
daemon that writes to it.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 include/openvswitch/vlog.h |  1 +
 lib/daemon-unix.c          |  7 +++++++
 lib/vlog.c                 | 14 ++++++++++++++
 3 files changed, 22 insertions(+)

Comments

Ansis Atteka Nov. 6, 2015, 7:32 p.m. UTC | #1
On 10 October 2015 at 01:07, Andy Zhou <azhou@nicira.com> wrote:

> vlog log file can be created when parsing --log-file option, before

switch user, in case the --user option is also specified. This
>
this does not read fluently. How about:

s/switch user/switching user?


does not directly causing errors for the running daemons, but leaves
>
s/causing/cause

> the log files on disk as owned by root. It can be confusing at best.
>
> This patch fixes the log file ownership setting to match with the
> daemon that writes to it.


> Signed-off-by: Andy Zhou <azhou@nicira.com>
> ---
>  include/openvswitch/vlog.h |  1 +
>  lib/daemon-unix.c          |  7 +++++++
>  lib/vlog.c                 | 14 ++++++++++++++
>  3 files changed, 22 insertions(+)
>
> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
> index f6bb3ab..139dfb9 100644
> --- a/include/openvswitch/vlog.h
> +++ b/include/openvswitch/vlog.h
> @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg);
>  void vlog_set_pattern(enum vlog_destination, const char *pattern);
>  int vlog_set_log_file(const char *file_name);
>  int vlog_reopen_log_file(void);
> +int vlog_change_owner(uid_t, gid_t);
>
>  /* Configure method how vlog should send messages to syslog server. */
>  void vlog_set_syslog_method(const char *method);
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index cafa397..e31dbc4 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -856,6 +856,13 @@ daemon_become_new_user__(bool access_datapath)
>          return;
>      }
>
> +    /* If vlog file has been created, change its owner to the non-root
> user
> +     * as specifed by the --user option.  */
> +    if (vlog_change_owner(uid, gid)) {
> +        VLOG_FATAL("%s: fail to change owner of the log file from root "
>
s/fail/failed

> +                   "to user %s", pidfile, user);
>
If log file doesn't yet exist on filesystem, wouldn't you call here
VLOG_FATAL? Based on "man 2 chown" it seems that it can return ENOENT if
file does not exist.

> +    }
> +
>      if (LINUX) {
>          if (LIBCAPNG) {
>              daemon_become_new_user_linux(access_datapath);
> diff --git a/lib/vlog.c b/lib/vlog.c
> index da31e6f..56b8db8 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -430,6 +430,20 @@ vlog_reopen_log_file(void)
>      }
>  }
>
> +/* In case a log file exists, change its owner to new 'user' and 'group'.
> + *
> + * This is useful for handling cases where the --log-file option is
> + * specified ahead of the --user option.  */
>

I think we typically define what is return value in function comments (see
CodingStyle.md).

+int
> +vlog_change_owner(uid_t user, gid_t group)
> +{
> +    ovs_mutex_lock(&log_file_mutex);
> +    int error = log_file_name ? chown(log_file_name, user, group) : 0;
>
+    ovs_mutex_unlock(&log_file_mutex);
> +
> +    return error;
> +}
> +
>  /* Set debugging levels.  Returns null if successful, otherwise an error
>   * message that the caller must free(). */
>  char *
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
Andy Zhou Nov. 9, 2015, 6:43 p.m. UTC | #2
On Fri, Nov 6, 2015 at 11:32 AM, Ansis Atteka <ansisatteka@gmail.com> wrote:
>
>
> On 10 October 2015 at 01:07, Andy Zhou <azhou@nicira.com> wrote:
>>
>> vlog log file can be created when parsing --log-file option, before
>>
>> switch user, in case the --user option is also specified. This
>
> this does not read fluently. How about:
>
> s/switch user/switching user?
>
>
>> does not directly causing errors for the running daemons, but leaves
>
> s/causing/cause
>>
>> the log files on disk as owned by root. It can be confusing at best.
>>
>> This patch fixes the log file ownership setting to match with the
>> daemon that writes to it.
>>
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>> ---
>>  include/openvswitch/vlog.h |  1 +
>>  lib/daemon-unix.c          |  7 +++++++
>>  lib/vlog.c                 | 14 ++++++++++++++
>>  3 files changed, 22 insertions(+)
>>
>> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
>> index f6bb3ab..139dfb9 100644
>> --- a/include/openvswitch/vlog.h
>> +++ b/include/openvswitch/vlog.h
>> @@ -143,6 +143,7 @@ void vlog_set_verbosity(const char *arg);
>>  void vlog_set_pattern(enum vlog_destination, const char *pattern);
>>  int vlog_set_log_file(const char *file_name);
>>  int vlog_reopen_log_file(void);
>> +int vlog_change_owner(uid_t, gid_t);
>>
>>  /* Configure method how vlog should send messages to syslog server. */
>>  void vlog_set_syslog_method(const char *method);
>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
>> index cafa397..e31dbc4 100644
>> --- a/lib/daemon-unix.c
>> +++ b/lib/daemon-unix.c
>> @@ -856,6 +856,13 @@ daemon_become_new_user__(bool access_datapath)
>>          return;
>>      }
>>
>> +    /* If vlog file has been created, change its owner to the non-root
>> user
>> +     * as specifed by the --user option.  */
>> +    if (vlog_change_owner(uid, gid)) {
>> +        VLOG_FATAL("%s: fail to change owner of the log file from root "
>
> s/fail/failed
>>
>> +                   "to user %s", pidfile, user);
>
> If log file doesn't yet exist on filesystem, wouldn't you call here
> VLOG_FATAL? Based on "man 2 chown" it seems that it can return ENOENT if
> file does not exist.

If the log file does not yet exist, vlog_change_owner suppose to return 0.
>>
>> +    }
>> +
>>      if (LINUX) {
>>          if (LIBCAPNG) {
>>              daemon_become_new_user_linux(access_datapath);
>> diff --git a/lib/vlog.c b/lib/vlog.c
>> index da31e6f..56b8db8 100644
>> --- a/lib/vlog.c
>> +++ b/lib/vlog.c
>> @@ -430,6 +430,20 @@ vlog_reopen_log_file(void)
>>      }
>>  }
>>
>> +/* In case a log file exists, change its owner to new 'user' and 'group'.
>> + *
>> + * This is useful for handling cases where the --log-file option is
>> + * specified ahead of the --user option.  */
>
>
> I think we typically define what is return value in function comments (see
> CodingStyle.md).

Sure, I will add a comment about the return value.

>
>> +int
>> +vlog_change_owner(uid_t user, gid_t group)
>> +{
>> +    ovs_mutex_lock(&log_file_mutex);
>> +    int error = log_file_name ? chown(log_file_name, user, group) : 0;
>>
>> +    ovs_mutex_unlock(&log_file_mutex);
>> +
>> +    return error;
>> +}
>> +
>>  /* Set debugging levels.  Returns null if successful, otherwise an error
>>   * message that the caller must free(). */
>>  char *
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>

Patch
diff mbox

diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index f6bb3ab..139dfb9 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -143,6 +143,7 @@  void vlog_set_verbosity(const char *arg);
 void vlog_set_pattern(enum vlog_destination, const char *pattern);
 int vlog_set_log_file(const char *file_name);
 int vlog_reopen_log_file(void);
+int vlog_change_owner(uid_t, gid_t);
 
 /* Configure method how vlog should send messages to syslog server. */
 void vlog_set_syslog_method(const char *method);
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index cafa397..e31dbc4 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -856,6 +856,13 @@  daemon_become_new_user__(bool access_datapath)
         return;
     }
 
+    /* If vlog file has been created, change its owner to the non-root user
+     * as specifed by the --user option.  */
+    if (vlog_change_owner(uid, gid)) {
+        VLOG_FATAL("%s: fail to change owner of the log file from root "
+                   "to user %s", pidfile, user);
+    }
+
     if (LINUX) {
         if (LIBCAPNG) {
             daemon_become_new_user_linux(access_datapath);
diff --git a/lib/vlog.c b/lib/vlog.c
index da31e6f..56b8db8 100644
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -430,6 +430,20 @@  vlog_reopen_log_file(void)
     }
 }
 
+/* In case a log file exists, change its owner to new 'user' and 'group'.
+ *
+ * This is useful for handling cases where the --log-file option is
+ * specified ahead of the --user option.  */
+int
+vlog_change_owner(uid_t user, gid_t group)
+{
+    ovs_mutex_lock(&log_file_mutex);
+    int error = log_file_name ? chown(log_file_name, user, group) : 0;
+    ovs_mutex_unlock(&log_file_mutex);
+
+    return error;
+}
+
 /* Set debugging levels.  Returns null if successful, otherwise an error
  * message that the caller must free(). */
 char *