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

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:23 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.
 >

 deferring this for now (as per discussion on jabber)

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

 ok. I must confess I was a bit surprised to see an actual use-case for
 friend myself :)

 BTW, there is supposed to be a way to only provide friend access to
 specific functions, but I haven't figured out how to use that the way i
 want yet (like, ideally, we would only give createHMAC access to the
 constructor, and to the constructor only). Perhaps I misunderstood that
 this is possible.

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

 sure, why not :)

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

 why not, done :) (come to think of it, we could also do the same 'trick'
 for all arrays that have repeated data, but since we have those in there
 now anyway i think we can leave them)

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

 ack

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

 i prefer to have it in there explicitely

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

 oops, I had done this, but had forgotten to push it, sorry for that. Once
 i push now, you'll find it before your editorial changes commit :)

 another change that i then forgot to push as well is the renaming of the
 main files; crypto.cc -> cryptolink.cc and crypto.h -> cryptolink.h (for
 consistency reasons given the class it defines)
 > '''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)

 if that is so, it wouldn't be the code here that causes the leaks, since
 createHMAC should itself not end up with any allocated data if there is an
 exception inside it (you wouldn't be able to delete it, you'd never get
 the pointer).

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