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:26 jelte]:

 > deferring this for now (as per discussion on jabber)

 Okay (I plan to explain my point in more detail in a separate comment).

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

 Hmm, "a member function of one class can be the firend of another"
 (by the Stroustrup book), so it should be possible to say like this:
 {{{
     //friend class CryptoLink;
     friend HMAC* CryptoLink::createHMAC(const void*, size_t,
                                         const HashAlgorithm);
 }}}

 but when I tried to confirm this I found a larger issue: cryptolink.h
 includes crypto_hmac.h, and vice versa.  Such mutual-inclusion is not
 necessarily invalid, but can easily make the design fragile in
 general, and in this particular case, actually makes the above friend
 declaration non-compilable (the friend declaration requires the
 definition of CryptoLink, which is in cryptolink.h, which includes
 crypto_hmac.h, which defines HMAC with the friend declaration...)

 Looking at it more closely, and based on the observation that hash
 algorithms are not specific to HMAC (for example, we may want to
 replace our homemade sha1 implementation using this framework, or we
 may want to integrate it to this framework), I'd suggest moving
 HashAlgorithm to hmaclink.h (or a separate crypto_hash.h or
 something).  Then, in cryptolink.h, we can only put a forward
 declaration of 'class HMAC' and avoid including crypto_hmac.h.

 Whether or not we use function-based friend (on which I don't have a
 strong opinion) I think this is a good change.

 And (yet) another point I noticed while I was looking at the code: it
 looks possible to change the return value of
 CryptoLink::getCryptoLink() to 'const CryptoLink&' (and then we have
 to change CryptoLink::createHMAC() a const member function).

 Other changes look okay.  One follow up comment:

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

 Ah, okay, you're right.  (Though, as you originally said 'will leak'
 (in error cases), if createHMAC() doesn't throw as expected the
 allocated memory will leak.  But I'm okay with ignoring this scenario)

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