BIND 10 #268: review: setuid via -u on b10-auth

[BIND 10 Development]

#268: review: setuid via -u on b10-auth
------------------------------+---------------------------------------------
      Reporter:  shane        |        Owner:  jinmei                     
          Type:  enhancement  |       Status:  reviewing                  
      Priority:  major        |    Milestone:  06. 4th Incremental Release
     Component:  b10-auth     |   Resolution:                             
      Keywords:               |    Sensitive:  0                          
Estimatedhours:  0.0          |        Hours:  1.0                        
      Billable:  1            |   Totalhours:                             
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by shane):

  * hours:  0.0 => 1.0
  * owner:  shane => jinmei
  * totalhours:  0.0 => 1.0


Comment:

 One bug:

 If the user name does not exist on the system, then `getpwnam()` will
 return `NULL`. The code will then try to use value as a PID. If this is
 not a valid PID then we will get a casting error and raise a "Failed to
 parse" exception. We should probably not raise an "Failed to parse"
 exception but rather let the code fall through and raise the "Unknown user
 name or UID" exception. This is because a valid - but non-existent - user
 name should not signal a parse error.

 Concern about reentrancy:

 The code uses `getpwnam()` and `getpwuid()`, neither of which is re-
 entrant. While we don't expect this code to be used generally, it is
 probably worth it to use `getpwnam_r()` and `getpwuid_r()` instead.

 Proposal:

 When the code fails after `setuid()`, we leave the group ID set. Perhaps
 we can try to change back to the original group ID in that case? It's an
 attempt to have fewer side effects...

 !ChangeLog:

 The proposed !ChangeLog update mentions ''effective'' user ID. However we
 change the ''real'' user ID. I suggest just removing the word "effective".

 Documentation:

 The b10-auth.8 page is not updated. Maybe this is something to leave for
 Jeremy, but our review procedure mentions documentation. `:)`

 Note about tests:

 The boss process tests do not check the user of the subprocesses. I
 couldn't think of a portable way to do this. It's not important - no
 changes to this patch are necessary- but thought I would mention it.

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