[ovs-dev] [additional --user changes v4 2/3] vlog: change log file owner when switching user

[Andy Zhou]

On Wed, Nov 11, 2015 at 2:48 PM, Ansis Atteka <ansisatteka at gmail.com> wrote:
>
>
> On 11 November 2015 at 14:13, Andy Zhou <azhou at nicira.com> wrote:
>>
>> vlog log file can be created when parsing --log-file option, before
>> switching user, in case the --user option is also specified. While this
>> does not directly cause errors for the running daemons, it can
>> leave the log files on the disk as created under the "root" user.
>> This patch fix the log file ownership to the user specified with --user.
>>
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>>
>> ---
>> v1->v2: Add a comment on vlog_change_owner return code.
>> v2->v3: no change
>> v3->v4: Reword the commit message.  change vlog_change_owner() to void.
>> ---
>>  include/openvswitch/vlog.h |  1 +
>>  lib/daemon-unix.c          |  6 +++++-
>>  lib/vlog.c                 | 22 +++++++++++++++++++++-
>>  3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
>> index f6bb3ab..bc16590 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);
>> +void 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 0125745..e69517a 100644
>> --- a/lib/daemon-unix.c
>> +++ b/lib/daemon-unix.c
>> @@ -739,7 +739,7 @@ daemon_switch_group(gid_t real, gid_t effective,
>>  {
>>      if ((setresgid(real, effective, saved) == -1) ||
>>          !gid_verify(real, effective, saved)) {
>> -        VLOG_FATAL("%s: fail to switch group to gid as %d, aborting",
>> +        VLOG_FATAL("%s: failed to switch group to gid as %d, aborting",
>>                     pidfile, gid);
>>      }
>>  }
>> @@ -847,6 +847,10 @@ daemon_become_new_user_linux(bool access_datapath
>> OVS_UNUSED)
>>  static void
>>  daemon_become_new_user__(bool access_datapath)
>>  {
>> +    /* If vlog file has been created, change its owner to the non-root
>> user
>> +     * as specifed by the --user option.  */
>> +    vlog_change_owner(uid, gid);
>> +
>>      if (LINUX) {
>>          if (LIBCAPNG) {
>>              daemon_become_new_user_linux(access_datapath);
>> diff --git a/lib/vlog.c b/lib/vlog.c
>> index da31e6f..ade0a45 100644
>> --- a/lib/vlog.c
>> +++ b/lib/vlog.c
>> @@ -105,7 +105,7 @@ DEFINE_STATIC_PER_THREAD_DATA(unsigned int, msg_num,
>> 0);
>>   * All of the following is protected by 'log_file_mutex', which nests
>> inside
>>   * pattern_rwlock. */
>>  static struct ovs_mutex log_file_mutex = OVS_MUTEX_INITIALIZER;
>> -static char *log_file_name OVS_GUARDED_BY(log_file_mutex);
>> +static char *log_file_name = NULL OVS_GUARDED_BY(log_file_mutex);
>>  static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1;
>>  static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex);
>>  static bool log_async OVS_GUARDED_BY(log_file_mutex);
>> @@ -430,6 +430,26 @@ 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.  */
>> +void
>> +vlog_change_owner(uid_t user, gid_t group)
>> +{
>> +    int error = 0;
>> +
>> +    if (log_file_name) {
>> +        ovs_mutex_lock(&log_file_mutex);
>> +        error = chown(log_file_name, user, group);
>> +        ovs_mutex_unlock(&log_file_mutex);
>> +    }
>> +
>> +    if (error) {
>> +        VLOG_FATAL("Failed to change log file ownership.");
>
> I would print errno value here and the file name you are actually trying to
> change the ownership for. It would simply provide a hint to the users on
> what was actually wrong, if it failed.
>
> VLOG_FATAL("Failed to change %s ownership: %s", log_file_name,
> ovs_strerror(errno));
>
> And early return from function if log_file_name is NULL to make code look
> better.
>

Pushed to master with both changes as suggested.
>> +    }
>> +}
>> +
>
> Otherwise, Acked-by: Ansis Atteka <aatteka at nicira.com>
>
> Thanks for working on this, Andy.

Thanks for your review.