| Message ID | 1441323223-11889-3-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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?
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.
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
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(-)