[trac781] review comments pt1

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

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

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

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

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

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

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'

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 + ":" +

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.