BIND 10 #180: b10 start auth server drop privs asap

[BIND 10 Development]

#180: b10 start auth server drop privs asap
--------------------------+-------------------------------------------------
 Reporter:  shane         |        Owner:  jelte                                         
     Type:  enhancement   |       Status:  reviewing                                     
 Priority:  major         |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  Boss of BIND  |   Resolution:                                                
 Keywords:                |    Sensitive:  0                                             
--------------------------+-------------------------------------------------
Changes (by shane):

  * owner:  shane => jelte


Comment:

 Replying to [comment:7 jelte]:
 >
 > bind10.py.in:
 >
 > i personally think 'if foo not in bar' and 'if foo is not None' reads
 better than 'if not foo in bar' and 'if not foo is None'. We don't really
 have a style thingy about this, and both versions appear in the code. It's
 also quite minor :)

 Okay, I've updated this to your preferred style. I've post a question to
 the bind10-dev list to see if there is a preference.

 > _setuid(self) has two write statements that don't prefix with the
 module. I suspect at least one of them is debug leftover, and shouldn't
 need to be there at all (perhaps even both of them).

 Whoops! Removed.

 > So the initial few 'downsetuidable' processes get the given uid as an
 argument, but after that you set the uid of bob himself, and then simply
 don't pass it along anymore. I don't think this is a very good approach,
 it's inconsistent at the very least, but it also gives us a problem
 (restart of auth will fail if it uses privileged port, and keep on failing
 after that). The 'full' way would be to not setuid of bob himself, only
 for subprocesses (either way, this should all change when we have our holy
 socket creator).

 This is a design choice to minimize the amount of code running as root.
 The bob process should probably not run as root, which is why it calls
 setuid() on itself. After that point no further setuid() is possible. And
 yes - it means we cannot restart the auth process properly. That was the
 reason we decided on the PriviligedSocketCreator for the long-term
 solution.

 So, I'd prefer to keep this patch "as is" in this regard.

 > Don't know if it's out of scope, but the process starting code contains
 nothing but replication, so a refactor would be nice ;)

 It's a separate ticket - something to be done when this is read from the
 cfgmgr rather than hard-coded.

 > Please document the return values of restart_processes (0, not-0 and
 None?) (more generally, i think we can do a better job of documenting
 arguments and return values in our python code)

 Done, and agreed (although I didn't go through everything and check it).

 > If you give an existing user, but don't start as root, the error message
 is a bit confusing (simply says 'operation not permitted', not what
 operation that is)

 Okay, I fixed this to throw a more readable exception.

 > bind10_test.in:
 >
 > Do we still use the (generated) test scripts? (we have make check for
 that now, right?)

 Um... in this case it seems to still be used. I'll ask Jeremy if this is
 bad style or what.

 > args_test.py:
 >
 > You mention this in your own comment, but "shane" does not exist on my
 system so this test fails... i don't think 'nobody' will work. Perhaps
 take the username of the user that runs configure?


 > should this one also test other arguments while we're at it?

 It's out of scope for this ticket, but I have created a separate one for
 that (ticket #258). It's non-trivial due to needing to check ports and
 addresses and suchlike.

 I've committed the changes made, and will attach a diff here as well for
 review.

-- 
Ticket URL: <http://bind10.isc.org/ticket/180#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development