BIND 10 #2595: cmdctl with chusr crashes due to permission issue

[BIND 10 Development]

#2595: cmdctl with chusr crashes due to permission issue
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  cmd-ctl       |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''changelog'''

 I'd also mention that this could have happened if BIND 10 is installed
 with a root privilege and then started with changing the user.

 '''bindcmd.py'''

 - docstring for `__try_login`: "to bindctl" should be "to cmdctl"?
 {{{
         Attempts to log in to bindctl by sending a POST with
 }}}

 '''bindctl_test.py'''

 - if we directly test `__try_login`, maybe we should pretend it's
   "inheritable", i.e., rename it to `_try_login` with comment.  But
   this is a minor point so I'd leave it to you.
 - maybe we should also check other exception than socket.error is
   propagated transparently.

 - the warning messages dumped to stdout from tests are a bit noisy.
   ideally we should introduce a hook (intermediate wrapper method for
   `print` or something) so the test can silence or inspect the
   message.  note also that if we inspect the message we can also test
   if the two cases are handled separately (right now we can't
   distinguish them because the exception type is the same).  but this
   is also minor, so I'd leave it to you.

 '''cmdctl.py.in'''

 - I suggest revising the TODO comment as follows (or something):
 {{{#!python
                 # TODO: we only check whether the file exist, is a
                 # file, and is readable; but further check need to be
 done:
                 # eg. whether the private/certificate is valid.
 }}}
   to clarify what's we are doing now and what is actually a TODO.
 - shouldn't we close the socket here?
 {{{#!python
         except (CmdctlException, IOError) as cce:
             logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
             # raise socket error to finish the request
             raise socket.error
 }}}
   like the case of `SSLError`?  In fact, probably due to this bindctl
   now seems to revert to the previous unhelpful error message.
 - If the answer to the previous question is yes, we might unify the
   duplicate code as follows:
 {{{#!python
         except ssl.SSLError as err :
             logger.error(CMDCTL_SSL_SETUP_FAILURE_USER_DENIED, err)
         except (CmdctlException, IOError) as cce:
             logger.error(CMDCTL_SSL_SETUP_FAILURE_READING_CERT, cce)
         self.close_request(sock)
         # raise socket error to finish the request
         raise socket.error
 }}}
 - In the above code segment, the case for `IOError` was added.  What's
   the purpose of it?  At least this case doesn't seem to be tested.

 '''cmdctl_test.py'''

 - Should we care about `CMDCTL_BUILD_PATH` is not in `os.environ`?
 {{{#!python
 BUILD_FILE_PATH = '..' + os.sep
 if 'CMDCTL_BUILD_PATH' in os.environ:
     BUILD_FILE_PATH = os.environ['CMDCTL_BUILD_PATH'] + os.sep
 }}}
   I suspect this (whole) test doesn't work reasonably if not run from
   the Makefile anyway, and when run from Makefile the env variable is
   always set.  Besides, if we really end up using `../`, part of the
   original concern of modifying distributed files still stands.

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