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-20130122
         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):

 '''general'''

 - I think we need a changelog for this ticket.

 - As for whether to handle it as fatal: I'm not necessarily pushing
   the idea of not doing so, but I simply don't see the point of
   handling it as fatal.  This check is local to that particular
   request handling.  And, as far as I can see there wouldn't be any
   state left in the cmdctl even if we simply reset the connection and
   wait for the next request (which will still fail unless the admin
   fixes the file related issues).  On the other hand, if we handle it
   as fatal, bind10 will restart cmdctl anyway, and cmdctl is in the
   same state of waiting the next request.  So, to me, the "fatal"
   behavior simply involves kill/restart overhead without any benefit.
   Furthermore, although it's not immediately clear to me how/whether
   multiple concurrent clients are handled due to the usual obfuscation
   of Python server classes, if this means threads are used to accept
   and handle multiple concurrent clients:
 {{{#!python
 class SecureHTTPServer(socketserver_mixin.NoPollMixIn,
                        socketserver.ThreadingMixIn,
                        http.server.HTTPServer):
 }}}
   killing the entire process due to a failure of single request
   doesn't seem to be a good idea in general (although, in practice, if
   this is about a permission of the cert file, it's quite unlikely
   that other clients are being handled when the error is detected -
   but it could still happen).  Overall, I can only see disadvantages
   of treating it as fatal with no clear benefit.  Is there any real
   advantage of handling it as a fatal event?

 '''bindcmd.py'''

 - Is it okay to not have a test for it?
 - This branch slightly changes failure mode if the following after
   `__try_login` fails with socket.error:
 {{{#!python
             data = response.read().decode()
 }}}
   Previously it's immediately caught and converted to `FailToLogin`.
   Now it's propagated to the upper layer and I guess caught here:
 {{{#!python
         except socket.error as err:
             print('Failed to send request, the connection is closed')
             return 1
 }}}

 - `_config_data_check`: This comment seems to have to be updated:
 {{{#!python
                 #TODO, only check whether the file exist,
                 # further check need to be done: eg. whether
                 # the private/certificate is valid.
 }}}
  (it at least does readability check too)

 '''cmdctl_messages.mes'''

 - missing period?
 {{{
 While running, b10-cmdctl encountered a fatal exception and it will shut
 down
 }}}
 - CMDCTL_SSL_SETUP_FAILURE_READING_CERT message: I'd note that
   depending on whether to treat this case fatal it may have to be
   updated.

 '''cmdctl_test.py'''

 - test_check_file: it modifies (the attributes of) a file in the
   source tree:
 {{{#!python
         file_name = SRC_FILE_PATH + 'cmdctl-keyfile.pem'
         check_file(file_name)
         with UnreadableFile(file_name):
             self.assertRaises(CmdctlException, check_file, file_name)
 }}}
   Does it work with distcheck?

 - test_wrap_sock_in_ssl_context: not really the subject of this task,
   but the test doesn't seem to be sufficient: its return value isn't
   checked; the ssl.SSLError case isn't tested.

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