[ovs-dev,3/3] ovsdb-server: support --user option
diff mbox

Message ID 1441323223-11889-3-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Sept. 3, 2015, 11:33 p.m. UTC
Add support for running ovsdb-server as a non-root user, specified
by the --user option. If specified, all I/O access and all
sub-processes will be perfromed as the new user.

VMware-BZ: #1499254
Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 NEWS                 | 1 +
 lib/daemon.man       | 8 ++++++++
 ovsdb/ovsdb-server.c | 6 +++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

Comments

Ansis Atteka Sept. 7, 2015, 11:09 p.m. UTC | #1
Thanks, Andy, for working on this!


On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote:

> Add support for running ovsdb-server as a non-root user, specified
> by the --user option. If specified, all I/O access and all
> sub-processes will be perfromed as the new user.
>
> VMware-BZ: #1499254
> Signed-off-by: Andy Zhou <azhou@nicira.com>
> ---
>  NEWS                 | 1 +
>  lib/daemon.man       | 8 ++++++++
>  ovsdb/ovsdb-server.c | 6 +++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index ca22c8e..5192ac1 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -21,6 +21,7 @@ Post-v2.4.0
>       targets to run a new system testsuite.  These tests can be run inside
>       a Vagrant box.  See INSTALL.md for details
>     - Dropped support for GRE64 tunnel.
> +   - Added --user option to ovsdb-server.
>
>
>  v2.4.0 - 20 Aug 2015
> diff --git a/lib/daemon.man b/lib/daemon.man
> index 4ab9823..d7e2968 100644
> --- a/lib/daemon.man
> +++ b/lib/daemon.man
> @@ -50,3 +50,11 @@ core dumps into the current working directory and the
> root directory
>  is not a good directory to use.
>  .IP
>  This option has no effect when \fB\-\-detach\fR is not specified.
> +.
> +.TP
> +\fB\-\-user\fR
> +Causes \fB\*(PN\fR to run as a new user specified in "user:group". Short
> +forms "user" and ":group" are also allowed, with current user or group
> +are assumed respectively. Only root process accepts this argument.
>

1. I see this --user flag also in ovs-vswitchd man page. Is this expected?
2. I think that the paragraph above would read a little more fluently, if
you stated "run as a different user" opposed to "run as new user". When you
say "new user" I am tempted to think that this will be a new user created
on the system instead of an already existing user.
3. Would the last sentence become more readable if it would be replaced
with "The process must be started by root user to make use of this command
line option".



> +.IP
> +Currently only ovsdb-server actually implements this option.
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 4088d85..fdeecd2 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -221,6 +221,10 @@ main(int argc, char *argv[])
>      process_init();
>
>      parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command);
> +    /* Drop root privileges and become the new user as soon as possible.
> +     * OVSDB server does not need root privileges. If --user option is
> +     * not specified, the following function is essentially no-op.  */
> +    daemon_become_new_user();
>
>      /* Create and initialize 'config_tmpfile' as a temporary file to hold
>       * ovsdb-server's most basic configuration, and then save our initial


I don't see changes to init.d scripts that would actually start
ovsdb-server with --user flag. Don't we want this feature to be used by
default?

To do this you would have to call adduser and addgroup from package's
postinst script.
Andy Zhou Sept. 8, 2015, 7:34 p.m. UTC | #2
On Mon, Sep 7, 2015 at 4:09 PM, Ansis Atteka <ansisatteka@gmail.com> wrote:
> Thanks, Andy, for working on this!
>
>
> On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote:
>>
>> Add support for running ovsdb-server as a non-root user, specified
>> by the --user option. If specified, all I/O access and all
>> sub-processes will be perfromed as the new user.
>>
>> VMware-BZ: #1499254
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>> ---
>>  NEWS                 | 1 +
>>  lib/daemon.man       | 8 ++++++++
>>  ovsdb/ovsdb-server.c | 6 +++++-
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/NEWS b/NEWS
>> index ca22c8e..5192ac1 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -21,6 +21,7 @@ Post-v2.4.0
>>       targets to run a new system testsuite.  These tests can be run
>> inside
>>       a Vagrant box.  See INSTALL.md for details
>>     - Dropped support for GRE64 tunnel.
>> +   - Added --user option to ovsdb-server.
>>
>>
>>  v2.4.0 - 20 Aug 2015
>> diff --git a/lib/daemon.man b/lib/daemon.man
>> index 4ab9823..d7e2968 100644
>> --- a/lib/daemon.man
>> +++ b/lib/daemon.man
>> @@ -50,3 +50,11 @@ core dumps into the current working directory and the
>> root directory
>>  is not a good directory to use.
>>  .IP
>>  This option has no effect when \fB\-\-detach\fR is not specified.
>> +.
>> +.TP
>> +\fB\-\-user\fR
>> +Causes \fB\*(PN\fR to run as a new user specified in "user:group". Short
>> +forms "user" and ":group" are also allowed, with current user or group
>> +are assumed respectively. Only root process accepts this argument.
>
>
> 1. I see this --user flag also in ovs-vswitchd man page. Is this expected?
I assume that all daemon can benefit from supporting this option.
Obviously they will
work differently than OVSDB. In the mean time,  the next line in the
man page points out
that this option only works for OVSDB.
On the other hand, if we decide to only support OVSDB, we will need to
fix the man page.

> 2. I think that the paragraph above would read a little more fluently, if
> you stated "run as a different user" opposed to "run as new user". When you
> say "new user" I am tempted to think that this will be a new user created on
> the system instead of an already existing user.
How about "run as a non-root user"?

> 3. Would the last sentence become more readable if it would be replaced with
> "The process must be started by root user to make use of this command line
> option".
Sure.
>
>
>>
>> +.IP
>> +Currently only ovsdb-server actually implements this option.
>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
>> index 4088d85..fdeecd2 100644
>> --- a/ovsdb/ovsdb-server.c
>> +++ b/ovsdb/ovsdb-server.c
>> @@ -1,4 +1,4 @@
>> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -221,6 +221,10 @@ main(int argc, char *argv[])
>>      process_init();
>>
>>      parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command);
>> +    /* Drop root privileges and become the new user as soon as possible.
>> +     * OVSDB server does not need root privileges. If --user option is
>> +     * not specified, the following function is essentially no-op.  */
>> +    daemon_become_new_user();
>>
>>      /* Create and initialize 'config_tmpfile' as a temporary file to hold
>>       * ovsdb-server's most basic configuration, and then save our initial
>
>
> I don't see changes to init.d scripts that would actually start ovsdb-server
> with --user flag. Don't we want this feature to be used by default?
>
Yes, we have change the init.d scripts to make this option useful. However,
I don't think it needs to be part of this patch, since this feature can be used
stand alone.

> To do this you would have to call adduser and addgroup from package's
> postinst script.
Should we assume the valid user and group have been created already?
Ben Pfaff Sept. 9, 2015, 12:30 a.m. UTC | #3
On Thu, Sep 03, 2015 at 04:33:43PM -0700, Andy Zhou wrote:
> Add support for running ovsdb-server as a non-root user, specified
> by the --user option. If specified, all I/O access and all
> sub-processes will be perfromed as the new user.
> 
> VMware-BZ: #1499254
> Signed-off-by: Andy Zhou <azhou@nicira.com>

I don't have anything to add to Ansis's comments on this patch.

Patch
diff mbox

diff --git a/NEWS b/NEWS
index ca22c8e..5192ac1 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,7 @@  Post-v2.4.0
      targets to run a new system testsuite.  These tests can be run inside
      a Vagrant box.  See INSTALL.md for details
    - Dropped support for GRE64 tunnel.
+   - Added --user option to ovsdb-server.
 
 
 v2.4.0 - 20 Aug 2015
diff --git a/lib/daemon.man b/lib/daemon.man
index 4ab9823..d7e2968 100644
--- a/lib/daemon.man
+++ b/lib/daemon.man
@@ -50,3 +50,11 @@  core dumps into the current working directory and the root directory
 is not a good directory to use.
 .IP
 This option has no effect when \fB\-\-detach\fR is not specified.
+.
+.TP
+\fB\-\-user\fR
+Causes \fB\*(PN\fR to run as a new user specified in "user:group". Short
+forms "user" and ":group" are also allowed, with current user or group
+are assumed respectively. Only root process accepts this argument.
+.IP
+Currently only ovsdb-server actually implements this option.
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 4088d85..fdeecd2 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -221,6 +221,10 @@  main(int argc, char *argv[])
     process_init();
 
     parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command);
+    /* Drop root privileges and become the new user as soon as possible.
+     * OVSDB server does not need root privileges. If --user option is
+     * not specified, the following function is essentially no-op.  */
+    daemon_become_new_user();
 
     /* Create and initialize 'config_tmpfile' as a temporary file to hold
      * ovsdb-server's most basic configuration, and then save our initial