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
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:9 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.
 >

 ok

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

 ack

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

 sure

 > - maybe we should also check other exception than socket.error is
 >   propagated transparently.
 >

 ack, randomly chose an ImportError (since it is quite unlikely that that
 exception is suddenly raised)

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

 right, something like that was tested in an existing different test
 already, but by temporarily replacing a standard stream. Replaced it with
 _print(), and checking output for the _try_login calls

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

 ack, done

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

 doh, of course

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

 done, added a small comment above the return in the try block that it
 really does need to return :)

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

 IOError is what ssl.wrap_socket() raises if it has a problem reading the
 files. Chances are quite small that this would happen but in theory it is
 still possible. Added test

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

 I suspect almost none of the python tests do :)

 Making it fail if those values aren't set.

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