BIND 10 trac781, updated. 122d9611bd552c3113eca8aa5e91d2b8a3018210 [trac781] review comments pt1
BIND 10 source code commits
bind10-changes at lists.isc.org
Sun Apr 17 15:51:17 UTC 2011
The branch, trac781 has been updated
via 122d9611bd552c3113eca8aa5e91d2b8a3018210 (commit)
from 231b29911894b0f5ddbd0d1444d9dfdf9cb5abb3 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 122d9611bd552c3113eca8aa5e91d2b8a3018210
Author: Jelte Jansen <jelte at isc.org>
Date: Sun Apr 17 17:42:58 2011 +0200
[trac781] review comments pt1
-----------------------------------------------------------------------
Summary of changes:
doc/Doxyfile | 2 +-
src/lib/crypto/Makefile.am | 4 +-
src/lib/crypto/crypto.cc | 30 +++++++++++++---------
src/lib/crypto/crypto.h | 28 +++++++++++----------
src/lib/crypto/tests/Makefile.am | 2 +-
src/lib/crypto/tests/crypto_unittests.cc | 25 ++++++++++++-------
src/lib/dns/python/tests/tsigkey_python_test.py | 13 ++++++++++
src/lib/dns/tsigkey.cc | 20 +++++----------
src/lib/dns/tsigkey.h | 8 ++++-
9 files changed, 79 insertions(+), 53 deletions(-)
-----------------------------------------------------------------------
diff --git a/doc/Doxyfile b/doc/Doxyfile
index 46aa178..6cb4174 100644
--- a/doc/Doxyfile
+++ b/doc/Doxyfile
@@ -568,7 +568,7 @@ WARN_LOGFILE =
# directories like "/usr/src/myproject". Separate the files or directories
# with spaces.
-INPUT = ../src/lib/cc ../src/lib/config ../src/lib/dns ../src/lib/exceptions ../src/lib/datasrc ../src/bin/auth ../src/bin/resolver ../src/lib/bench ../src/lib/log ../src/lib/asiolink/ ../src/lib/nsas ../src/lib/testutils ../src/lib/cache ../src/lib/server_common/
+INPUT = ../src/lib/cc ../src/lib/config ../src/lib/dns ../src/lib/crypto ../src/lib/exceptions ../src/lib/datasrc ../src/bin/auth ../src/bin/resolver ../src/lib/bench ../src/lib/log ../src/lib/asiolink/ ../src/lib/nsas ../src/lib/testutils ../src/lib/cache ../src/lib/server_common/
# This tag can be used to specify the character encoding of the source files
# that doxygen parses. Internally doxygen uses the UTF-8 encoding, which is
diff --git a/src/lib/crypto/Makefile.am b/src/lib/crypto/Makefile.am
index 63bd750..e953284 100644
--- a/src/lib/crypto/Makefile.am
+++ b/src/lib/crypto/Makefile.am
@@ -6,6 +6,6 @@ AM_CXXFLAGS = $(B10_CXXFLAGS)
CLEANFILES = *.gcno *.gcda
-lib_LTLIBRARIES = libbcrypto.la
+lib_LTLIBRARIES = libb10crypto.la
-libbcrypto_la_SOURCES = crypto.h crypto.cc
+libb10crypto_la_SOURCES = crypto.h crypto.cc
diff --git a/src/lib/crypto/crypto.cc b/src/lib/crypto/crypto.cc
index 214eddf..9af9863 100644
--- a/src/lib/crypto/crypto.cc
+++ b/src/lib/crypto/crypto.cc
@@ -35,15 +35,20 @@ using namespace isc::dns;
namespace {
Botan::HashFunction*
getHash(const Name& hash_name) {
- if (hash_name == TSIGKey::HMACMD5_NAME()) {
- return (Botan::get_hash("MD5"));
- } else if (hash_name == TSIGKey::HMACSHA1_NAME()) {
- return (Botan::get_hash("SHA-1"));
- } else if (hash_name == TSIGKey::HMACSHA256_NAME()) {
- return (Botan::get_hash("SHA-256"));
- } else {
+ try {
+ if (hash_name == TSIGKey::HMACMD5_NAME()) {
+ return (Botan::get_hash("MD5"));
+ } else if (hash_name == TSIGKey::HMACSHA1_NAME()) {
+ return (Botan::get_hash("SHA-1"));
+ } else if (hash_name == TSIGKey::HMACSHA256_NAME()) {
+ return (Botan::get_hash("SHA-256"));
+ } else {
+ isc_throw(isc::crypto::UnsupportedAlgorithm,
+ "Unknown Hash type " + hash_name.toText());
+ }
+ } catch (const Botan::Algorithm_Not_Found) {
isc_throw(isc::crypto::UnsupportedAlgorithm,
- "Unknown Hash type " + hash_name.toText());
+ "Algorithm not supported by boost: " + hash_name.toText());
}
}
@@ -77,6 +82,7 @@ public:
key.getSecretLength());
}
} catch (const Botan::Invalid_Key_Length& ikl) {
+ delete hmac_;
isc_throw(BadKey, ikl.what());
}
}
@@ -88,7 +94,7 @@ public:
hmac_->update(static_cast<const Botan::byte*>(data), len);
}
- void sign(isc::dns::OutputBuffer& result) const {
+ void sign(isc::dns::OutputBuffer& result) {
// And generate the mac
Botan::SecureVector<Botan::byte> b_result(hmac_->final());
@@ -96,7 +102,7 @@ public:
result.writeData(b_result.begin(), b_result.size());
}
- bool verify(const void* sig, const size_t len) const {
+ bool verify(const void* sig, const size_t len) {
return (hmac_->verify_mac(static_cast<const Botan::byte*>(sig), len));
}
@@ -118,12 +124,12 @@ HMAC::update(const void* data, const size_t len) {
}
void
-HMAC::sign(isc::dns::OutputBuffer& result) const {
+HMAC::sign(isc::dns::OutputBuffer& result) {
impl_->sign(result);
}
bool
-HMAC::verify(const void* sig, const size_t len) const {
+HMAC::verify(const void* sig, const size_t len) {
return (impl_->verify(sig, len));
}
diff --git a/src/lib/crypto/crypto.h b/src/lib/crypto/crypto.h
index 3f32615..f01d037 100644
--- a/src/lib/crypto/crypto.h
+++ b/src/lib/crypto/crypto.h
@@ -17,6 +17,8 @@
#include <dns/tsigkey.h>
#include <exceptions/exceptions.h>
+#include <boost/noncopyable.hpp>
+
#ifndef _ISC_CRYPTO_H
#define _ISC_CRYPTO_H
@@ -54,14 +56,14 @@ class HMACImpl;
///
/// This class is used to create and verify HMAC signatures
///
-class HMAC {
+class HMAC : public boost::noncopyable {
public:
/// \brief Constructor from a key
///
- /// Raises an UnsupportedAlgorithmException if the given key
+ /// \exception UnsupportedAlgorithmException if the given key
/// is for an algorithm that is not supported by the underlying
/// library
- /// Raises an InvalidKeyLength if the given key has a bad length
+ /// \exception InvalidKeyLength if the given key has a bad length
///
/// Notes: if the key is longer than the block size of its
/// algorithm, the constructor will run it through the hash
@@ -84,14 +86,14 @@ public:
/// The result will be appended to the given outputbuffer
///
/// \param result The OutputBuffer to append the result to
- void sign(isc::dns::OutputBuffer& result) const;
+ void sign(isc::dns::OutputBuffer& result);
/// \brief Verify an existing signature
///
/// \param sig The signature to verify
/// \param len The length of the sig
/// \return true if the signature is correct, false otherwise
- bool verify(const void* sig, size_t len) const;
+ bool verify(const void* sig, size_t len);
private:
HMACImpl* impl_;
@@ -104,10 +106,10 @@ private:
/// creating an HMAC object, feeding it the data, and calculating the
/// resulting signature.
///
-/// Raises an UnsupportedAlgorithm if we do not support the given
-/// algorithm. Raises a BadKey exception if the underlying library
-/// cannot handle the given TSIGKey (for instance if it has a bad
-/// length).
+/// \exception UnsupportedAlgorithm if we do not support the given
+/// algorithm.
+/// \exception BadKey if the underlying library cannot handle the
+/// given TSIGKey (for instance if it has a bad length).
///
/// \param data The data to sign
/// \param data_len The length of the data
@@ -125,10 +127,10 @@ void signHMAC(const void* data,
/// creating an HMAC object, feeding it the data, and checking the
/// resulting signature.
///
-/// Raises an UnsupportedAlgorithm if we do not support the given
-/// algorithm. Raises a BadKey exception if the underlying library
-/// cannot handle the given TSIGKey (for instance if it has a bad
-/// length).
+/// \exception UnsupportedAlgorithm if we do not support the given
+/// algorithm.
+/// \exception BadKey exception if the underlying library cannot
+/// handle the given TSIGKey (for instance if it has a bad length).
///
/// \param data The data to verify
/// \param data_len The length of the data
diff --git a/src/lib/crypto/tests/Makefile.am b/src/lib/crypto/tests/Makefile.am
index 263a090..1777f2b 100644
--- a/src/lib/crypto/tests/Makefile.am
+++ b/src/lib/crypto/tests/Makefile.am
@@ -18,7 +18,7 @@ run_unittests_SOURCES += crypto_unittests.cc
run_unittests_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
run_unittests_LDFLAGS = $(AM_LDFLAGS) $(GTEST_LDFLAGS)
run_unittests_LDADD = $(GTEST_LDADD)
-run_unittests_LDADD += $(top_builddir)/src/lib/crypto/libbcrypto.la
+run_unittests_LDADD += $(top_builddir)/src/lib/crypto/libb10crypto.la
run_unittests_LDADD += $(top_builddir)/src/lib/dns/libdns++.la
run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la
endif
diff --git a/src/lib/crypto/tests/crypto_unittests.cc b/src/lib/crypto/tests/crypto_unittests.cc
index d857be7..00d42f2 100644
--- a/src/lib/crypto/tests/crypto_unittests.cc
+++ b/src/lib/crypto/tests/crypto_unittests.cc
@@ -25,16 +25,18 @@ using namespace isc::crypto;
namespace {
void checkBuffer(const OutputBuffer& buf, uint8_t *data, size_t len) {
- ASSERT_EQ(buf.getLength(), len);
+ ASSERT_EQ(len, buf.getLength());
const uint8_t* buf_d = static_cast<const uint8_t*>(buf.getData());
for (size_t i = 0; i < len; ++i) {
- ASSERT_EQ(buf_d[i], data[i]);
+ ASSERT_EQ(data[i], buf_d[i]);
}
}
// Sign and verify with the convenience functions
- void doHMACTestConv(std::string data, std::string key_str,
- uint8_t* expected_hmac, size_t hmac_len) {
+ void doHMACTestConv(const std::string& data,
+ const std::string& key_str,
+ uint8_t* expected_hmac,
+ size_t hmac_len) {
OutputBuffer data_buf(data.size());
data_buf.writeData(data.c_str(), data.size());
OutputBuffer hmac_sig(1);
@@ -62,8 +64,10 @@ namespace {
}
// Sign and verify with an instantiation of an HMAC object
- void doHMACTestDirect(std::string data, std::string key_str,
- uint8_t* expected_hmac, size_t hmac_len) {
+ void doHMACTestDirect(const std::string& data,
+ const std::string& key_str,
+ uint8_t* expected_hmac,
+ size_t hmac_len) {
OutputBuffer data_buf(data.size());
data_buf.writeData(data.c_str(), data.size());
OutputBuffer hmac_sig(1);
@@ -91,8 +95,10 @@ namespace {
hmac_sig.getLength()));
}
- void doHMACTest(std::string data, std::string key_str,
- uint8_t* expected_hmac, size_t hmac_len) {
+ void doHMACTest(const std::string& data,
+ const std::string& key_str,
+ uint8_t* expected_hmac,
+ size_t hmac_len) {
doHMACTestConv(data, key_str, expected_hmac, hmac_len);
doHMACTestDirect(data, key_str, expected_hmac, hmac_len);
}
@@ -325,8 +331,9 @@ TEST(CryptoTest, BadKey) {
NULL, 0);
OutputBuffer data_buf(0);
- OutputBuffer hmac_sig(1);
+ OutputBuffer hmac_sig(0);
+ EXPECT_THROW(new HMAC(bad_key), BadKey);
EXPECT_THROW(signHMAC(data_buf.getData(), data_buf.getLength(),
bad_key, hmac_sig), BadKey);
EXPECT_THROW(verifyHMAC(data_buf.getData(), data_buf.getLength(),
diff --git a/src/lib/dns/python/tests/tsigkey_python_test.py b/src/lib/dns/python/tests/tsigkey_python_test.py
index 023b9e6..97be501 100644
--- a/src/lib/dns/python/tests/tsigkey_python_test.py
+++ b/src/lib/dns/python/tests/tsigkey_python_test.py
@@ -53,6 +53,19 @@ class TSIGKeyTest(unittest.TestCase):
self.assertEqual('test.example.:CwsLCwsLCwsLCwsLCwsLCw==:hmac-md5.sig-alg.reg.int.',
k1.to_text())
+ self.assertRaises(InvalidParameter, TSIGKey,
+ 'test.example:CwsLCwsLCwsLCwsLCwsLCw==:unsupported')
+ self.assertRaises(InvalidParameter, TSIGKey,
+ '::')
+ self.assertRaises(InvalidParameter, TSIGKey,
+ 'test.example:')
+ self.assertRaises(InvalidParameter, TSIGKey,
+ 'test.example:%bad_base_64%')
+ self.assertRaises(InvalidParameter, TSIGKey,
+ 'test.example:CwsLCwsLCwsLCwsLCwsLCw==:')
+ self.assertRaises(InvalidParameter, TSIGKey,
+ 'test.:example:CwsLCwsLCwsLCwsLCwsLCw==')
+
class TSIGKeyRingTest(unittest.TestCase):
key_name = Name('example.com')
secret = b'someRandomData'
diff --git a/src/lib/dns/tsigkey.cc b/src/lib/dns/tsigkey.cc
index d35fde3..74d21af 100644
--- a/src/lib/dns/tsigkey.cc
+++ b/src/lib/dns/tsigkey.cc
@@ -83,13 +83,6 @@ TSIGKey::TSIGKey(const std::string& str) : impl_(NULL) {
std::string secret_str = str.substr(pos + 1, pos2 - pos - 1);
- vector<uint8_t> secret;
- decodeBase64(secret_str, secret);
- unsigned char secret_b[secret.size()];
- for (size_t i=0; i < secret.size(); ++i) {
- secret_b[i] = secret[i];
- }
-
if (algo_name != HMACMD5_NAME() &&
algo_name != HMACSHA1_NAME() &&
algo_name != HMACSHA256_NAME()) {
@@ -97,7 +90,10 @@ TSIGKey::TSIGKey(const std::string& str) : impl_(NULL) {
algo_name);
}
- impl_ = new TSIGKeyImpl(key_name, algo_name, secret_b,
+ vector<uint8_t> secret;
+ decodeBase64(secret_str, secret);
+
+ impl_ = new TSIGKeyImpl(key_name, algo_name, &secret[0],
secret.size());
} catch (const Exception& e) {
// 'reduce' the several types of exceptions name parsing and
@@ -149,11 +145,9 @@ TSIGKey::getSecretLength() const {
std::string
TSIGKey::toText() const {
- const uint8_t* secret_b = static_cast<const uint8_t*>(getSecret());
- vector<uint8_t> secret_v;
- for (size_t i=0; i < getSecretLength(); ++i) {
- secret_v.push_back(secret_b[i]);
- }
+ const vector<uint8_t> secret_v(static_cast<const uint8_t*>(getSecret()),
+ static_cast<const uint8_t*>(getSecret()) +
+ getSecretLength());
std::string secret_str = encodeBase64(secret_v);
return (getKeyName().toText() + ":" + secret_str + ":" +
diff --git a/src/lib/dns/tsigkey.h b/src/lib/dns/tsigkey.h
index 8e322a1..5936e0f 100644
--- a/src/lib/dns/tsigkey.h
+++ b/src/lib/dns/tsigkey.h
@@ -97,12 +97,16 @@ public:
/// Where <name> is a domain name for the key, <secret> is a
/// base64 representation of the key secret, and the optional
/// algorithm is an algorithm identifier as specified in RFC4635
+ /// The default algorithm is hmac-md5.sig-alg.reg.int.
///
- /// Raises an InvalidParameter exception if the input string is
+ /// Since ':' is used as a separator here, it is not possible to
+ /// use this constructor to create keys with a ':' character in
+ /// their name.
+ ///
+ /// \exception InvalidParameter exception if the input string is
/// invalid.
///
/// \param str The string to make a TSIGKey from
- /// \return The TSIGKey build from the string
TSIGKey(const std::string& str);
/// \brief The copy constructor.
More information about the bind10-changes
mailing list