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

 Comments on the changes to the point of merging with master (857abf9).

 First, I made some editorial cleanups and directly pushed them.

 '''session.cc'''

 I don't understand exactly what kind of problem that the change from
 size_t to uint32_t tried to solve.  The change even seems to be more
 slippery in that it now passes a uint32_t to asio::buffer(), which
 expects a size_t value.

 If the size of size_t can be smaller than 32 bits (I don't know if the
 standard defines that) I see there's a problem, but as far as I can
 see the change doesn't solve that problem either.

 To me, the current implementation is safer in that we limit the use of
 fixed size variable to the point exactly where we need to do that and
 use the generic size_t (which many other APIs expect) everywhere else.
 If we wanted to deal with issues due to the possible size difference
 between uint32_t and size_t, I'd rather catch and handle it within
 SessionImpl::readDataLength().

 '''server_common/keyring.cc'''
 - do we have a test for the change to keyring.cc?

 '''ccsession_unittests.cc (and relevant change to ccsession.cc)'''

 - I'd add a comment about why we need to include asio.hpp (with a note
   of understanding this is generally discouraged unless there's a
   strong need for it), but see below for the delayedStart test first.

 - CCSessionTest::remoteConfig: at first glance it was not clear to me
   where the value of 1 for "item1" comes from:
 {{{
         EXPECT_NO_THROW(item1 =
                         mccs.getRemoteConfigValue(module_name,
                                                   "item1")->intValue());
         EXPECT_EQ(1, item1);
 }}}
   due to the fact that this test passes an empty config and due to the
   following comment in the ccsession.cc
 {{{
         // Merge the received config into the defaults
 }}}
   Then I found this comment was actually incorrect (or very confusing
   at best).  I fixed the comment and added a clarification comment to
   the test and pushed the changes (e37672e).

 - delayedStart: this test seems to be too tricky (and have larger side
   effects such as requiring asio.hpp or adding a test-only parameter
   to a public method) to me.  At a higher level this problem is that
   we try to do (a variant of) recvmsg() after starting asynchronous
   read on the underlying Session.  I'd write a test at that level,
   rather than going deeper into the lower IPC level.  I'm attaching a
   sample diff to implement this idea.

   On one hand, this alternative test doesn't directly test what kind
   of bad thing happens at the lower level if we do recvmsg() while we
   are in the asynchronous mode, but IMO if we really want to test
   that, that test should go to session_unittests.  On the other hand,
   the alternative approach is even better because it has a larger
   potential of catching similar types of bugs within ModuleCCSession.

   Please look at the diff.  If you agree with the general idea, please
   apply it (with cleanups: some explanation in comments, throw an
   exception instead of assert(), write some more supplemental tests to
   catch the exception, etc) instead of the current delayedStart.  You
   may still want to add tests the lower level thing at the session
   class level, and if so, go ahead, but I'd suggest deferring it to a
   separate ticket (this ticket has already do too many things for the
   original purpose of TSIG support in b10-auth).

   If you don't agree and still argue keeping delayedStart, let's
   discuss it.

 - I'd add a test for ModuleCCSession::start() (to see if it throws when
   called twice, either implicitly or explicitly)

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