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

[Arjen de Korte]

> 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.

> 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).

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.

> 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.

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