BIND 10 #2641: Disable default account, require authentication setup during initialization

[BIND 10 Development]

#2641: Disable default account, require authentication setup during initialization
-------------------------------------+-------------------------------------
            Reporter:  shane         |                        Owner:
                Type:  enhancement   |  jinmei
            Priority:  very high     |                       Status:
           Component:  bind-ctl      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130305
         Sub-Project:  Core          |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:16 muks]:

 > > '''bindcmd.py'''
 > >
 > > - `_have_users` repeats many things of `_try_login`.  Besides, if I
 > >   understand it correctly, the introduction of `_have_users` would
 > >   make the checks in `_try_login` unnecessary.  These should be
 > >   clarified, and the code needs to be cleaned up accordingly.
 >
 > I don't understand how the checks are unnecessary. If these failures
 > occur, they should still cause `FailToLogin` to be raised. But maybe I
 > don't follow properly.

 The checks in `_try_login` was mainly for catching the issue that
 would be reported by `__print_check_ssl_msg`.  But it's very unlikely
 to happen in `_try_login` any more because that case should have been
 eliminated on the successful completion of `_have_users`.  Am I clear
 now?

 But, on thinking about this more closely, a more fundamental point
 occurred to me.  I now think it's probably not really a good idea to
 introduce things like `_have_users` in the first place.  Not being a
 security expert, my general sense of security is that it's generally a
 bad practice to give a possibly untrusted remote user unnecessary
 state at the server side.  For a login session, the should simply say
 the login succeeds or not for the given user name and password; it's
 not wise to tell the user if the password file exists or the list is
 empty.  In that sense, the existing code would also be not really
 good; I guess we shouldn't terminate the connection immediately after
 cmdctl finds the file isn't accessible, but it should reject the login
 request due to that just like the case where the user/password doesn't
 match.

 If we need to give the user a hint for possible diagnosis (like "you
 should probably check the cmdctl log"), that should be a local
 extension to bindctl, not based on a response from cmdctl.

 > > '''cmdctl'''
 > >
 > > - why was cmdctl-accounts.csv moved to tests/testdata?  Also, is the
 > >   now-empty csv file under cmdctl/ used?  And, in any event, the
 > >   changes seem to introduce several non trivial implications.  First,
 > >   how should this work if we run bind10 from source using
 > >   run_bind10.sh?  Using the one in testdata or the empty one?  If it's
 > >   the latter, we can override the file for local experiments/tests and
 > >   can accidentally commit it to the repository, which is not good.
 > >   Also, I personally would like to avoid having hardcoded
 > >   "tests/testdata" in the production version of script; even though
 > >   things like B10_FROM_BUILD or B10_FROM_SOURCE is already a hack,
 > >   embedded string of "test" makes it look even uglier.
 >
 > B10_FROM_SOURCE and B10_FROM_BUILD hotwire stuff for the tests to work.
 > I don't think making use of `tests/testdata/` in this context is too
 > bad, considering the contents of the `.csv` file *are* test data and it
 > belongs in the `tests/testdata/` directory.

 I don't necessarily object to placing the test .csv files under
 tests.  My point is that something like
 `/usr/local/libexec/bind10/b10-cmdctl` should have this line:
 {{{#!python
             accountsfile  = sysconf_path + "tests/testdata/cmdctl-
 accounts.csv"
 }}}
 admittedly, the line before this is also not clean:
 {{{#!python
             sysconf_path = os.environ["B10_FROM_SOURCE"] +
 "/src/bin/cmdctl/"
 }}}
 but I can live it if there's no simpler alternative.  But I don't
 think that can be used as an excuse to make it even uglier.

 The "fundamental point" above may make most of other part of the
 branch moot, so I'm giving the ticket back to you at this point.
 But I'm dumping specific code comments I had so far just for reference
 (note, again, that they may become moot).

 '''bindcmd.py'''

 - `_have_users`: what if json.loads() (or maybe even read() or
   decode()) fails and raises an exception?
 {{{#!python
             if response.status == http.client.OK:
                 return json.loads(response.read().decode())
 }}}
 - _have_users: shouldn't we display what's specifically wrong here?
 {{{#!python
             # if not OK, fall through to raise error
             self._print("Failure in cmdctl when checking if users already
 exist")
 }}}
   (and this line is too long)

 '''bindctl_test.py'''

 - not actually about the test but rather about the main code (and,
   actually it's rather derived from `_try_login`), this message looks
   awkward:
 {{{#!python
             expected_printed_messages.append(
                 'Socket error checking if users exist:  test error')
 }}}
   due to the redundant white space.  Shouldn't the main code be
   actually like this?
 {{{#!python
             self._print("SSL error checking if users exist:", err)
 }}}
   Same for other similar cases.

 - test_have_users has a lot of duplicates with test_try_login.
 - test_have_users: cases where json.loads/response.read/decode raises
   an exception aren't tested.  See also comments on the code.

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