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:20 vorner]:

 '''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.

 See my previous short response, but I didn't want to delay the process
 due to this further, so I wrote the test myself (e9975a7).  As noted
 in the code, this means if the default could ever be non empty list
 the keyring module would need to extract the default value explicitly.
 I personally think this is a flaw of the config module interface (it
 simply doesn't make sense to me to put such a burden to the
 application; it shouldn't be bothered with what's the default, it
 should only need to deal with the "currently effective" set of
 configuration), but that's totally a different topic.

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

 > >   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?

 I've added a test to reproduce the situation we had: call
 addRemoteConfig() without delaying asynchronous read on
 ModuleCCSession and confirm it results in an exception.

 I also added a note to the addRemoteConfig() documentation about this
 restriction.

 These are c8cf644 and 1ccec99.

 And, on writing these, I'd now rather think we should be more
 explicit: let addRemoteConfig() throw an exception unless
 ModuleCCSession hasn't been "started".  Then if some other app
 incorrectly tries to use addRemoteConfig() it can see what's wrong
 sooner and more explicitly (rather than via scratching the head in
 seeing the strange timeout, etc).  I'm attaching a diff of this
 version of change.  Note that with this change the above added test
 won't work as expected any more.

 I'd leave it to you whether to make this additional change.

 > > BTW, I also had a concern (that I forgot to mention previously) that

 > 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.

 Deferring it to a separate ticket is fine by me (actually I don't like
 to delay this ticket due to this).  Note also that I'm not necessarily
 sticking to the specific idea of separating the methods.  My point is
 that the current interface is not very intuitive and is error prone.
 Any solution to address this concern is good to me.

 > > Why do we use a pointer to a shared pointer?  hmm...okay it's
 [...]
 > 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'd say that's naive (or I simply don't trust people being smart
 enough:-).  I'm okay with leaving the design for now as I already
 said, but please add an explicit comment about this restriction in
 keyring.h (oh btw maybe the top description of keyring.h is now a bit
 stale due to the recent change).

 > 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.

 The awkwardness of a pointer to shared pointer is not the main issue
 to me.  It's more that the interface is fragile and error prone (at
 least IMO).  While I kind of admit the sample access class doesn't
 look so sophisticated, I still personally believe this type of
 solution is much better.  But, again, I'm okay with (or actually
 rather prefer) leaving this out of this ticket.  If you agree this is
 worth further discussion, please create a separate file.  If you don't
 even agree with that, that's also fine for now.  I may or may not
 create one myself if and when I see a stronger need for it.

 > > 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?

 Another ticket please:-)  It's not a problem right now.
 >
 > > 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.

 Ah, okay I missed that.  I didn't mean a new test to AuthSrv, so this
 point is okay.

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