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 rest of the changes.  This completes my review.

 The comments contain subtle and perhaps controversial design points.
 I believe some are worth discussing, but don't think we should tweak
 this branch too much at this stage.  It already did too many things
 for the original purpose of the ticket.

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

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

 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.

 You may or may not agree with this view, and in any case it's far
 beyond the scope of this ticket.  If you think this makes sense,
 please create a separate ticket; if you don't, I don't fight against
 that at this moment.

 '''auth_srv (and test)'''

 First, let me be clear: this part of code is okay.  I don't require a
 change for merging this ticket.  On top that...

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

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

 So, if I were to implement this stuff, I'd introduce one higher level
 of abstraction, for example:

 Define a new class KeyRingAccessor like this
 {{{
 class KeyRingAccessor {
 public:
     const KeyRing& get() const {
         if (!keyring_) {
             create();
         }
         return (*keyring_);
     }
     keyRing& getMutable() {
         if (!keyring) {
             create();
         }
         return (*keyring_);
     }
     void create() {        // this also removes existing one if there is
         keyring_.reset(new TSIGKeyRing);
     }
 private:
     scoped_ptr<KeyRing> keyring_;
 };
 }}}

 Within keyring.cc define a factory function that returns a reference
 to a singleton const KeyRingAccessor object.  Also, keyring.cc itself
 has a way to get access to the single object as a mutable object.

 AuthSrv is constructed with that reference, and it always refers to
 the global KeyRing via the accessor: accessor_.get().  (As a bonus the
 server doesn't have to handle the case where keyring is NULL).

 Tests create their own accessor and constructs AuthSrv with it.

 Like the discussion for addRemoteConfig(), I recognize this is far
 beyond the scope of this ticket.  For this topic I have a relatively
 stronger opinion, but I'd defer further discussion whether or not you
 agree with this proposal.

 '''Miscellaneous final things'''

 - server_common/keyring.cc: (not directly related, but happen to
   notice in review) keyring (a shared pointer) is defined as a
   namespace scope static object.  Such an object can easily cause
   static initialization fiasco.  Consider some application defines its
   own static object that relies on that pointer:
 {{{
 #include <server_common/keyring.h>

 namespace {
 SomeSingleton singleton(isc::server_common::keyring);
 }
 }}}

 Of course, the other namespace scope static object is also an example
 of not-so-good practice, and/or we could say don't initialize a static
 object with isc::server_common::keyring, but in general it's better to
 ensure that doesn't happen, especially for a public library.

 An additional indirection like KeyRingAccessor (see above) can prevent
 this issue.

 Once again, whether or not you agree this is an issue to be addressed,
 I think this is beyond the scope of this ticket.

 - As for updating the key ring:

 > And, I forgot to mention, reloading keys works even with your
 > version, I both tried it and it makes sense in theory ‒ the change
 > comes as a command from cfgmgr.

 Oh, okay, understood.  But (with my workaround) I suspect we still
 cannot do addRemoteConfig() after starting the server (we don't have
 to be able to do this today, but for the dynamic nature of BIND 10 I
 can easily imagine a future possibility for that extension).  So, IMO,
 we should eventually make the internal send/recv in addRemoteConfig()
 asynchronous (or define an asynchronous version of addRemoteConfig()),
 and adjust the key initialization code accordingly.

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

 - changelog

 Regarding the use of sha-1: I think we should actually provide
 compatibility for both dig and drill, but that's a different topic,
 and okay to keep the current (revised) text on this point.

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