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

[BIND 10 Development]

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

  * owner:  jinmei => shane


Comment:

 Replying to [comment:7 shane]:

 Thanks for the review.

 I've adopted some of the suggestions, and in what follows explained my
 thoughts about the comments I didn't apply for now.  Please check.

 > 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.
 >
 Changed the code this way. r2571.

 > !ChangeLog:
 >
 > The proposed !ChangeLog update mentions ''effective'' user ID. However
 we change the ''real'' user ID. I suggest just removing the word
 "effective".
 >
 Okay.  I've also updated the description in change_user.h.

 I've not made changes to the following at the moment:

 > 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.
 >
 I noticed the reentrancy issue, but chose to not use the _r version for
 simplicity.  In practice, we won't even call this function more than once,
 and this function may be a shortterm workaround and isn't intended to be
 called outside the auth server implementation, so I didn't think it makes
 much sense to make it perfect with additional code.  Maybe we should note
 this in the function document though.

 But if you think we should still ensure the reentrancy, I don't oppose
 to that.

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

 Like the issue of reentrancy, I personally don't think the gain isn't
 worth the complexity of the additional code.  I generally prefer
 providing stronger exception/failure guarantees, but, realistically,
 if setuid() fails the server will simply die.  So, I'd like to simply
 note this function only provides a weak (basic) guarantee and an
 intermediate change could remain on failure (and in any case the
 expected behavior is to terminate the process at the caller side).

 Again, I don't necessarily oppose to providing a stronger guarantee if
 you strongly want.

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

 Okay, added.  r2572.
 (there are other options that are not documented)

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

 I noticed the boss process didn't have tests for '-u' and understood
 it wouldn't be easy.  Also, the test wouldn't specific to this option
 for b10-auth.  So (while I knew it was not ideal) I didn't touch this
 part this time.

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