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

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:17 jinmei]:
 > '''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.

 It seems I was already confused at the time after a lot of debugging, I
 overlooked the length is uint32_t already. I was afraid that it could read
 too many bytes from the socket. I reverted that commit, it was correct.
 Sorry for the confusion.

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

 Which change? The only change that happened from last time was adding of
 the parameter for the remote config from master and that one is implicitly
 tested from the usual tests. Actually, it was the tests which brought
 attention to the need to adding the false to the call.

 Or do you mean some other change?

 > '''ccsession_unittests.cc (and relevant change to ccsession.cc)'''
 > - 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.

 I didn't really like the test as well. So I took your version, but I don't
 know if it's enough, it doesn't really cause the bug before the fix.

 Anyway, if we do replace msgq (which I hope we'll do), all this might turn
 out as unneeded, I expect any grown up message queue to be able to listen
 for commands and request blocking answer at the same time.

 >   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

 I did try adding some, but I feel like writing tests for the fake session.
 Do you have any idea for a better test? Or is this enough?

 Replying to [comment:18 jinmei]:
 > '''ccsession.cc'''
 >
 > Refactoring of addRemoteConfig() looks good, but I'd go one step
 > further in order to make sure fetchRemoteSpec() do only one thing and
 > make more things const.  See attached diff.  It may be a matter of
 > thing, however, so I'd leave the decision to you.

 OK, I have no problem with this.

 > Looking at fetchRemoteSpec(), I wonder if we have a test for the case
 > where module != spec.getModuleName().  If not, we should have it.

 Added.

 > BTW, I also had a concern (that I forgot to mention previously) that
 > a variable like spec_is_filename is IMO error prone especially when
 > there's an implicit default.  This variable enables to make
 > addRemoteConfig() do quite different types of jobs, and it's not so
 > clear why it's reasonable to define a default and 'true' is a better
 > default (except, perhaps, for compatibility).  If I were to add this
 > feature, I'd consider an interface where the user of it can be more
 > clearly aware of what they are doing, e.g., by separating the methods
 > name to addRemoteConfigFromFile() and addRemoteConfigForModule(), or
 > if we have a reason we rather want to make these two modes agnostic,
 > by introducing an abstract notion of "spec identifier" and encapsulate
 > the difference there.

 The original purpose of the default value was for compatibility, yes.
 Otherwise, I would let user explicitly specify the boolean. I don't mind
 two different methods, but if you don't mind, I'd put it into a separate
 ticket. I don't like the spec identifier, it looks too complicated for
 such simple thing as passing an parameter.

 > Why do we use a pointer to a shared pointer?  hmm...okay it's
 > explained, but it led to my another level concern I had when I first
 > saw the original implementation: it seemed too low-level to allow
 > applications to refer to keyring object (via a shared pointer)
 > directly.  For example, right now applications can freely add or
 > remove a specific key from a keyring, but should that be allowed (I
 > simply don't know, but at least from the current implementation the
 > intent rather seems to be that keyring is read-only outside the
 > server_common/keyring module).

 Well, yes, modifying the keyring is a bad idea and the code doesn't
 explicitly disallow it. However, I believe that by saying it can be
 replaced by a different one at any time implies that one probably doesn't
 want to do it.

 I know that the pointer to shared pointer looks suspicious, but the
 accessor class looks at last equally inelegant. It looks like too much
 code for something that does nothing and is hard to build flow-graphs in
 the mind when getting trough the levels of such thing.

 > Allowing applications to refer to a non primitive object (in this case
 > a shared pointer object) that is statically defined has another usual
 > problem: static initialization fiasco (see below).

 OK, this one probably needs a solution. Should I do something about it
 now, or open another ticket?

 > BTW, do we have a test of key update cases?

 The tests in server_common contain one such case. I think that updating
 the keys is not a concern of the AuthSrv class, so there's no test for
 that.

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