[Nut-upsdev] Re: [nut-commits] svn commit r731

[Peter Selinger]

Arjen de Korte wrote:
> 
> 
> > in server/upsd.c r731, you moved the call conf_load() from after
> > check_perms() (~ l.1020) to before setupsignals() (~ l.989).
> 
> Yes. In order to be able to specify multiple addresses to bind to (the new
> LISTEN parameter in upsd.conf), I needed to parse the upsd.conf
> configuration file before setting up listening sockets, so before the call
> to setuptcp().
> 
> > The problem is that conf_load() needs to open the ups driver socket,
> > and it assumes that STATEPATH is the current working directory. The
> > directory is only set in l.1016. Therefore, the first attempt to open
> > a socket will always fail. From the user's point of view:
> >
> > Can't connect to UPS [mge] (mge): No such file or directory
> 
> I already wondered where that was coming from. :-)
> 
> > The socket is later opened on the second attempt, but only after upsd
> > has forked into the background, so the command-line user is left with
> > no further information after the first error message.
> 
> That is bad. :-(
> 
> > Why was it necessary to move conf_load()?  In this case,
> > chdir(statepath) should be moved before it.
> 
> No, it is possible to specify a STATEPATH in upsd.conf, so
> chdir(statepath) *must* be after parsing the upsd.conf file (unlike what
> it used to be). See 'man 5 upsd.conf'. Which needs updating by the way, I
> just noticed that the DATAPATH and CERTFILE parameters are not documented
> there. Oops.

Fair enough. However, I don't see why it makes sense to specify
STATEPATH in upsd.conf. The drivers also have to know the STATEPATH,
and as far as I know, there is no way to specify it in ups.conf, is
there? Do the drivers loop in upsd.conf? 
 
> > But the problem is that
> > become_user() has to be before chdir (to check permission), and
> > chroot_start() has to be before become_user(), and this might perhaps
> > mess up listen_add() (does listen_add() require root access)?
> 
> The listen_add() function doesn't need root access, so this shouldn't be a
> problem. Provided the listening socket is above 1023, setuptcp() doesn't
> need root access either. However I don't want to limit ourselves here
> (there may be people wanting to setup a low port), so I want to setup the
> server listening sockets as root at least at startup of upsd.
> 
> > This looks like a catch-22 to me, but I don't understand the code as
> > well as you do. Is there some sequence of events that will work?
> 
> I have some interesting tinkering to do this evening I guess. Specifying a
> STATEPATH in upsd.conf would have caused a similar error message before,
> but so far, I have not seen any complaints (not surprising, since this
> will be resolved later on). This looks like something that has been wrong
> for quite some time already and only now pops up because I moved
> conf_load() up.
> 
> Probably the best solution would be to split up the call to load_conf()
> and handle parsing of the upsd.conf (and possibly upsd.users)
> configuration file quite early in the startup of upsd. This makes it
> possible to override some settings through command line options and handle
> ups.conf where it used to be before I moved conf_load(). There is another
> benefit to this, we parse the upsd.* configuration files before
> backgrounding. If there are errors in them, we still have the console to
> write error messages to. The latter is not that important for ups.conf,
> since this will be parsed by the startup of the driver(s), which
> apparently was successful (otherwise we wouldn't be starting upsd).

Currently we are reading ups.conf before backgrounding; this was the
case even before 731. It is useful for the user to see "Connected to
UPS [mge]: mge" before the server backgrounds. 

Perhaps a better solution would be to separate reading the config
files (upsd.conf, ups.conf) from the actual opening of local sockets.
I don't see why conf_load() should open the driver sockets; that could
be done later.
 
> There is one catch when reloading though. We need to close the upsd server
> sockets and open new ones when the uspd.conf file is reloaded, since new
> LISTEN addresses may be specified. This is doable however, provided that
> we're not using a low port number (less than 1024). Therefor, if you need
> a low port number, you can't use reload anymore.

I don't much see the point of reloading anyway. It seems better to
kill upsd and upsmon and start new ones, after making a significant
configuration change.

> > Moreover, conf_load() is now called before the chroot() and
> > become_user(), which means that ups.conf is read by root and in the
> > raw directory tree. This is not how it was intended; perhaps even a
> > potential security problem.
> 
> It has been like that for quite some time earlier on, before we decided it
> was better to move it to a later stage, so that it was possible to check
> permissions on the configuration files in order to make reloading the
> configuration possible.

OK, but whether conf_load() is before or after chroot() affects the
location of the configuration files! Are they in the original
filesystem, or in the chroot-ed one? It would seem to have to be the
latter, or else reloading won't work. 

1 So chroot() should happen first,
2 then reading upsd.conf as root, to decide which user to run as and which
  sockets to listen to,
3 then setting up the sockets,
4 then dropping privileges
5 then reading ups.conf as the user
6 then chdir() to STATEPATH as the user
7 then opening the local driver sockets as the user. 
8 then forking into the background. 

I think this sequence would work, but I have not considered all the
consequences. There might be other dependencies that I missed. It
requires that the function conf_load() has to be split up into three
separate functions performing steps 2, 5, and 7. Perhaps the order of
5 and 6 can be swapped, so 5 and 7 might be combined into a single
function.

> I'm not that worried about security here. We have quite a few other
> programs that read the configuration files as root, for instance
> upsmon.conf (upsmon) and ups.conf (upsdrvctl). This is by design, because
> we want to be able to specify the user they run as in the configuration
> file. I think it is an omission in upsd that it is not possible to specify
> the user to run as in upsd.conf, like in the others. I will add that
> functionality too.
> 
> Best regards, Arjen
> -
> Eindhoven - The Netherlands
> Key fingerprint - 66 4E 03 2C 9D B5 CB 9B  7A FE 7E C1 EE 88 BC 57
>