BIND 10 #781: Define cryptographic API

[BIND 10 Development]

#781: Define cryptographic API
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20110503
  blocker                            |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:22 jelte]:

 > Okay, so here's the other Big change; We now provide a singleton factory
 object, that is automatically initialized on the first call to
 CryptoLink::getCryptoLink() (which is the only call that will give you
 access to anything), but can be initialized manually beforehand (which
 isn't necessary right now but will be later if we want to pass
 initialization arguments). This object is the only entry point for all the
 classes in the cryptolink namespace. Right now that is only HMAC. So it
 contains a createHMAC object (which returns a raw pointer, because HMAC
 itself is noncopyable, i've added documentation that suggests to place it
 into a scoped_ptr).

 I have a concern about the approach of returning a bare pointer and
 moving the cleanup responsibility to the caller.  In general, it's
 fragile if the module that performs 'new' is different from the one
 performing 'delete', because these two implementations may be
 different and incompatible.  In the case of createHMAC() (or future
 similar methods) this seems to be a real concern because it looks like
 we expect dynamically loadable modules to use this API (and some of
 which could be built separately from BIND 10).

 I myself am not so comfortable with relying on smart pointers too much
 (and I chose to return a bare pointer in a factory function of
 edns.cc), but I suspect in this case we need to return a smart pointer
 instead of the bare one.

 > Another 'protection' mechanism is that HMAC itself is not directly
 constructable; it's constructor is private. Only CryptoLink can create
 them now (i've made it a friend). And the intention is to do this for all
 classes we make that wrap backend library objects (i.e. all should be
 nonconstructible, and probably noncopyable as well, and have a factory
 function in CryptoLink if they must be created by the caller).

 Using friend always makes me nervous, due to its excessive power:-)
 But this is probably one of the rare cases where the use of it is
 justified in that it would be far better than leaving the room of an
 HMAC object being constructed without initialization accidentally and
 in that we can centralize the necessary initialization.  But please
 add some documentation about such consideration, maybe to the general
 description of the CryptoLink class.

 Other parts of the code mostly looks okay with some minor points.
 I've directly pushed changes for trivial and editorial things.
 Other points follow:


 '''crypto_unittest'''
 - 'will leak' sounds a bit too strong because it will normally be
    freed.  s/will/can/?
 {{{
         // note: this is not exception-safe, and will leak, but
 }}}
 - now looking at the revised code with simplified std::string, I also
   noticed we could even omit some of the variables.  e.g. instead of
 {{{
     const std::string data4(50, 0xcd);
 ...
     doHMACTest(data4, secret4, 25, HMAC::MD5, hmac_expected4, 16);
 }}}
   we can say
 {{{
     doHMACTest(std::string(50, 0xcd), secret4, 25, HMAC::MD5,
 hmac_expected4, 16);
 }}}
   (sorry for not noticing this in the previous review, but I guess
   this is how experimental refactoring works...)  But if you think the
   separate variable makes the code more readable, I'm okay with that
   (I'd probably object if it were a non const variable, but with a
   const variable it's mostly a matter of preference)

 '''tsigkey.cc'''
 - the proposed patch being adopted, it's probably safer to include
   <sstream> explicitly.

 '''CryptoLink and HMAC classes'''
 - private inheritance is the default and can be omitted, although I
   guess you might intentionally want to be explicit and in that
   case I'm okay with that.

 '''(new) crypto.cc/crypto.h'''
 - brief description is missing for CryptoLink:
 {{{
 class CryptoLinkImpl;

 /// \brief
 }}}
 - many of the include files now don't seem to be necessary: most, if
   not all DNS related ones, and maybe some botan and std ones

 '''crypto_unittests'''
 - This code leaks hmac:
 {{{
     EXPECT_THROW(crypto.createHMAC(NULL, 0, HMAC::MD5), BadKey);
     EXPECT_THROW(crypto.createHMAC(NULL, 0, HMAC::UNKNOWN),
 UnsupportedAlgorithm);
 }}}
  (but see also the bigger issue above)

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