BIND 10 #931: Implement signing part in b10-auth

[BIND 10 Development]

#931: Implement signing part in b10-auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110531
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:  tsig   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:4 vorner]:

 > There are few unrelated bug fixes, but it didn't work without them.
 Maybe they could be done better or might need some tests for them (I
 tested it when it stopped crashing, instead of writing tests first, it's
 faster when trying to find the source of problem). Is it OK to do it in
 another ticket?

 Okay, I've finally completed my review on this part.

 In short, I'd not give it a go as it is.  I have some counter
 proposals (and some additional patch, the bug that it fixes should be
 fixed anyway whether or not by this patch).  If we can quickly agree
 on the proposed fix, I'd say it can be merged.  If you don't agree
 with some part of it and want to discuss it more, that's fine, but in
 that case I have to say we should delay the merge.

 As for lacking tests...normally I wouldn't accept a proposal like a
 patch without tests "because it's working".  But in this case I'd like
 as much to see the feature to be included in the release...so, I'd
 personally loosen my usual criterion.  I'd suggest you to check if
 it's okay with others on jabber, though.

 Now, about the code:

 '''cfgmgr.py'''

 Your patch changed the semantics of get_module_spec(): it now returns
 an empty ModuleSpec object when "module_name && module_name not in
 self.module_specs".  When I first looked at it I failed to understand
 how such a radical change works without tweaking other parts...then I
 noticed it was due to lack of tests.  See the additional tests in
 cfgmgr.diff.  It would fail with the original patch.

 I'm not sure whether there exist other module that relies on the
 original behavior, but especially since we're trying to make a last
 minute change without many tests, I think we should be careful about
 compatibility.  I'm also not sure if returning an "empty ModuleSpec"
 is safe.  The caller would just consider it a successful result and
 would try normal methods like get_module_name(), and then see
 surprising disruption.

 My counter proposal patch in cfgmgr.diff tries to achieve the same
 thing that your fix seemingly tried to achieve with preserving the
 original behavior as much as possible.  Actually, my proposed patch is
 uglier, so, if we can find a cleaner and still safe solution later
 (with more tests), I'm okay with changing it.

 '''ccsession.cc'''

 The fix to addRemoteConfig() looks correct.

 But, this method is quite unreadable to me, which I suspect is a
 background reason why we had this type of bug.  Specifically, it's not
 really clear at a glance how module_name and rmod_spec are set to the
 expected values and whether the processing is correct, especially in
 the !spec_is_filename case.  The method is also too long (at least in
 my sense).

 If we have enough time, I'd further refactor this method, but,
 considering we don't have time, that would be a subject of a separate
 ticket.

 '''session.cc'''

 I'm afraid this change is too tricky and affects a so fundamental part
 of the system at this critical time.  I think I understand the problem
 correctly, and would like to proposal an alternative fix.  It's in
 proposal.diff (for main.cc, ccsession.cc, ccsession.h, and, it assumes
 we use the unmodified session.cc although the diff file doesn't
 include that part of diff for brevity).  I believe the intent is clear
 from the patch, but to explain it the idea is to delay starting
 asynchronous read until the auth server actually starts asio loop.

 Of course, this is not a complete fix.  For example, we wouldn't be
 able to follow changes to the keyring (although I'm not sure if it was
 really possible in the original patch).  And, my patch doesn't come
 with tests either, which is bad.  We'll need to develop a better
 solution pretty soon, and, of course, with a sufficient number of
 tests.

 '''Others'''

 I also noticed some other problems and fixed them locally.  The fix is
 in propsal.diff.

 - the original code crashed if we didn't include tsig_key in the
   config.  the change to keyring.cc fixes that (again, we need tests)
 - the original code prevent system tests from working correctly.
   change to bind10_config.py.in fixes that.

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