BIND 10 #781: Define cryptographic API

[BIND 10 Development]

#781: Define cryptographic API
-------------------------------------+-------------------------------------
                 Reporter:  stephen  |                Owner:  jinmei
                     Type:  task     |               Status:  reviewing
                 Priority:  blocker  |            Milestone:
                Component:           |  Sprint-20110419
  Unclassified                       |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  6.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:15 jinmei]:
 > '''Trivial things'''
 > - I made some trivial editorial changes.
 > - I also changed 'catch exception by object' to 'by const reference'.
 >   (I suggest you try cppcheck.  It's very good at finding this issue)
 >

 thanks :) (i don't really use cppcheck much yet since it is a tad noisy
 here, we should probably put in a bit of effort to make it clean and keep
 it that way)

 > '''Some higher level discussions'''
 >
 > - I'm okay with the design decision of making this stuff DNS related
 >   things only (but IMO it should be noted in the documentation).  But
 >   in that case should it better be in libdns++?  My question here is
 >   more about design choice, but there's also practical issues: right
 >   now there's a mutual dependency between libdns++ and lib(b)crypto,
 >   which would cause various troubles.  For example, as soon as we have
 >   more TSIG stuff libdns++ tests would need to link lib(b)crypto.  But
 >   lib(b)crypto tests also need to link libdns++.

 Right. Originally I did not intend to make libdns make direct use of
 libcrypto, so having a dependency the other way wouldn't be a problem.
 Right now we have no plans to use cryptographic functionality outside of
 libdns++ (as far as i'm aware), but there is quite a big chance this may
 change in the future. Another question is how 'clean' we want libdns++ to
 be regarding non-dns specific things (like crypto :)).

 Since using this directly from libdns++ seems to be the direction, I've
 removed the TSIGKey reference. In this branch, it is still dependent on
 libdns because of the outputbuffer (even though i added other sign
 options, see below), but outputbuffer is about to be moved out of libdns
 anyway, when it has been, there will no dependency on libdns++ anymore.

 > - Based on this decision, passing TSIGKey to hmac may be okay, but
 >   (just to be sure) is TSIG the only thing in the DNS world that
 >   requires HMAC?

 AFAIK it is. We may want it in the future for other things, for instance
 to secure command channels. But this is moot now :)

 > - I'm still not sure why we need to use OutputBuffer to store
 >   results.  Isn't a simple vector (or even a plain old array because
 >   we should be able to know the size) sufficient?

 We don't *need* it, but i figured outputbuffer would me the most logical
 type to have for this, which would save the caller from another
 checkoutputlength/alloc set of calls. A vector would do that as well, but
 will cause yet another copy afterwards. This was under the assumption that
 the caller would have an outputbuffer already anyway, so as i noted in my
 previous comment, it should be pretty easy to change or add sign() calls
 with the needed types.

 Anyhow, I added both for flexibility. I also added a size_t len argument
 for all three, so we can support truncated signatures.

 > - I noticed the new library is named "libbcrypto" (double b).
 >   Is the "redundant b" intentional?  Perhaps you meant b as in "bind"
 >   to avoid name conflict?  If so, I'd consider a better name because
 >   it would be very easy to confuse it with libcrypto (single b).  how
 >   about libdnscrypto, considering the fact that it's designed to be
 >   DNS specific?  But note also that the mutual dependency issue above.

 It was intentional, what about libb10crypto? (to make it more clear that
 it is not a typo :), and leaves the option open to use it for non-dns
 things)

 Changed it to that now.

 > - do we need a python wrapper for the new crypt stuff?
 >

 Probably, but it seemed like a good idea to agree on the API first.

 > '''crypto.h'''
 > - Mostly a matter of taste, but I found doxygen's "\exception" markup
 >   is useful to describe possible exceptions thrown from a
 >   method/function.

 Ack, changed.

 > - The HMAC class definition: I'd see some design level discussions.
 >   e.g. this is intended to be used for DNS related hmac.  I'd be also
 >   like to know if 'key' passed to the constructor can be destroyed
 >   once the constructor returns (apparently it can).

 Ack, but also moot now

 > - HMAC class: when using pimpl, copy and assignment is (normally) not
 >   trivial.  we should either provide custom copy
 >   constructor/assignment operator or make the class non copyable.
 >   (I guess in the case of this class the latter is probably fine).
 >

 ack

 > '''crypto.cc'''
 > - at least within this .cc file I don't see the need for including
 >   iostream, iomanip, or string.

 ack, removed

 > - getHash(): can we assume Botan::get_hash() never throws a
 >   (Botan-specific) exception?  Otherwise we need to catch and convert
 >   it in the HMACImpl constructor.

 With the use as it was, yes, but when we add other algorithms it might
 throw an AlgorithmNotFound, so i've added a catch for that.

 > - getHash(): is the overhead of this function acceptable? (pure
 >   question, I'm not sure).  For sha256, we need name comparison
 >   (relatively expensive) three times, and it happens every time we
 >   need to sign/verify hmac.

 Well, this comparison has to occur somewhere when going from a TSIGKey to
 an underlying algorithm. Since for the TSIGKey removal I introduced an
 enum for hmac hash algorithms, this is no longer a problem of this
 specific lib :)

 > - I suspect the way of defining 'init' can cause a static
 >   initialization fiasco (of course it depends on the details of
 >   LibraryInitializer, which I didn't dig into.  this is a general
 >   concern)

 I 'copied' the approach from botan; i.e. there is a RAII object for this
 now, and the problem is passed off to the program using this lib. We need
 something like this in the future anyway for initialization options.

 > - HMACImpl constructor: this is not exception safe (hmac_ could leak).

 added delete

 > - HMACImpl::update/sign/verify: can we assume the underlying Boton
 >   methods don't throw?

 only invalidkeylength, afaict, should we add 'general' Botan::Exception
 catches?


 > - HMACImpl::sign/verify: I suspect these cannot be a const member
 >   function semantically (even though hmac_ (a pointer) is intact,
 >   "*hmac_" would be modified by the underlying methods).  Likewise,
 >   HMAC::sign/verify cannot be a const member function, I suspect.

 removed

 > '''crypto_unittests.cc'''
 > - checkBuffer: in ASSERT_EQ() the expected data is the first
 >   parameter.  it seems to be reversed.  Also, you may want to use
 >   UnitTestUtil::matchWireData in lib/dns/tests for this type of test.

 That would introduce a build-order-dependency conflict again :)

 > - doHMACTestConv: it's probably better to use 'const string&' for data
 >   and key_str.  key can be const.  same for doHMACTestDirect() and
 >   doHMACTest().

 changed

 > - UnsupportedAlgorithm case doesn't seem to be tested.

 The old API made it impossible to get to that state, since it was not
 possible to create a TSIGKey with an unknown algorithm. But since we now
 pass an identifier (and 'unknown' is a possible value), i added a test
 now.

 > - BadKey test is probably better to be done for HMAC constructor
 >   because it's closer to the point where the exception is thrown.
 >

 added

 > '''tsigkey.h'''
 > - the new TSIGKey constructor (doc): it doesn't return anything.

 removed line

 > - I'm not sure if the :-separated notation is a good one.  What if
 >   <name> contains ':'?  wouldn't it be better (and easy to implement)
 >   to have it three string parameters? (and allow an empty string for
 >   algorithm).  BTW, it should describe which algorithm is used when
 >   it's omitted.
 >

 we already have the three-part constructor (the original one with Name
 args). This is simply to make it possible to use the de facto standard for
 the string representation, as introduced by dig and copied by multiple
 other implementations. If the name contains a ':', it will simply fail to
 create a key using that representation. We could add logic for escapes
 etc. But we could also simply say not to use names with :.

 > '''tsigkey.cc'''
 > - in the 'from string' TSIGKey constructor, variable length array is
 >   not very portable.  in fact, in this case this part of logic can
 >   just be simpler:

 ack, updated

 > - in that constructor, is it a good idea to catch all isc exceptions?

 No, but it's better than enumerating them all and catching them one by one
 (and almost certainly missing one or two). I think we should have a
 hierarchy of exceptions actually (like nameparseerror that is a superclass
 for emptylabl, toolonglabel, etc.etc.). Probably something to discuss
 and/or make a task for.

 >   can't we simply let various exceptions be propagated?

 Then the caller would have the same problem. This way I can honestly say
 that the \exception list in the docs are complete :) (and, more
 importantly, that the caller does not have to take 15 different exceptions
 into account, which may or may not change, not to mention the fact that
 the documentation will probably not be updated if it does indeed change).

 > - in toText(), I'd simplify the vector initialization:

 ack, done

 >
 > '''tsigkey_python_test.py'''
 >
 > exception cases are missing?

 ack, added

 proposed changelog entry:
 [func] Introduced an API for cryptographic operations. Currently it only
 supports HMAC, intended for use with TSIG. The current implementation uses
 Botan as the backend library.

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