BIND 10 #781: Define cryptographic API
BIND 10 Development
do-not-reply at isc.org
Fri Apr 15 09:35:18 UTC 2011
#781: Define cryptographic API
-------------------------------------+-------------------------------------
Reporter: stephen | Owner: jinmei
Type: task | Status: assigned
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 |
-------------------------------------+-------------------------------------
Comment (by 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)
'''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++.
- 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?
- 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?
- 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.
- do we need a python wrapper for the new crypt stuff?
'''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.
- 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).
- 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).
'''crypto.cc'''
- at least within this .cc file I don't see the need for including
iostream, iomanip, or string.
- 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.
- 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.
- 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)
- HMACImpl constructor: this is not exception safe (hmac_ could leak).
- HMACImpl::update/sign/verify: can we assume the underlying Boton
methods don't throw?
- 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.
'''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.
- doHMACTestConv: it's probably better to use 'const string&' for data
and key_str. key can be const. same for doHMACTestDirect() and
doHMACTest().
- (just a comment) when you don't worry about extending the buffer
(and possible failure of it) you can simply pass 0 to the
OutputBuffer constructor.
- UnsupportedAlgorithm case doesn't seem to be tested.
- BadKey test is probably better to be done for HMAC constructor
because it's closer to the point where the exception is thrown.
'''tsigkey.h'''
- the new TSIGKey constructor (doc): it doesn't return anything.
- 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.
'''tsigkey.cc'''
(I've not yet closely looked at the string parser due to the question
about the interface, see above).
- 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:
{{{
vector<uint8_t> secret;
decodeBase64(secret_str, secret);
// (we don't need secret_b)
...
impl_ = new TSIGKeyImpl(key_name, algo_name, &secret[0]
secret.size());
}}}
- in that constructor, is it a good idea to catch all isc exceptions?
can't we simply let various exceptions be propagated?
- in toText(), I'd simplify the vector initialization:
{{{
const vector<uint8_t> secret_v(static_cast<const
uint8_t*>(getSecret()),
static_cast<const
uint8_t*>(getSecret()) +
getSecretLength());
// no for loop is necessary
}}}
'''tsigkey_python_test.py'''
exception cases are missing?
--
Ticket URL: <http://bind10.isc.org/ticket/781#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list