[2522] TSIG review updates

src/lib/dns/rdata/any_255/tsig_250.cc

@@ -19,7 +19,6 @@
 #include <boost/lexical_cast.hpp>
 
 #include <util/buffer.h>
-#include <util/strutil.h>
 #include <util/encode/base64.h>
 
 #include <dns/messagerenderer.h>
@@ -34,7 +33,6 @@ using namespace std;
 using boost::lexical_cast;
 using namespace isc::util;
 using namespace isc::util::encode;
-using namespace isc::util::str;
 using namespace isc::dns;
 using isc::dns::rdata::generic::detail::createNameFromLexer;
 
@@ -74,18 +72,18 @@ struct TSIGImpl {
 
 // helper function for string and lexer constructors
 TSIGImpl*
-TSIG::constructFromLexer(MasterLexer& lexer) {
+TSIG::constructFromLexer(MasterLexer& lexer, const Name* origin) {
     // RFC2845 defines Algorithm Name to be "in domain name syntax",
     // but it's not actually a domain name, so we allow it to be not
     // fully qualified.
-    const Name root(".");
-    const Name& algorithm = createNameFromLexer(lexer, &root);
+    const Name& algorithm =
+        createNameFromLexer(lexer, origin ? origin : &Name::ROOT_NAME());
 
-    const string time_str =
+    const string& time_txt =
         lexer.getNextToken(MasterToken::STRING).getString();
     uint64_t time_signed;
     try {
-        time_signed = boost::lexical_cast<uint64_t>(time_str);
+        time_signed = boost::lexical_cast<uint64_t>(time_txt);
     } catch (const boost::bad_lexical_cast&) {
         isc_throw(InvalidRdataText, "Invalid TSIG Time");
     }
@@ -103,7 +101,7 @@ TSIG::constructFromLexer(MasterLexer& lexer) {
         isc_throw(InvalidRdataText, "TSIG MAC Size out of range");
     }
 
-    const string mac_txt = (macsize > 0) ?
+    const string& mac_txt = (macsize > 0) ?
             lexer.getNextToken(MasterToken::STRING).getString() : "";
     vector<uint8_t> mac;
     decodeBase64(mac_txt, mac);
@@ -117,9 +115,9 @@ TSIG::constructFromLexer(MasterLexer& lexer) {
         isc_throw(InvalidRdataText, "TSIG Original ID out of range");
     }
 
-    const string error_txt =
+    const string& error_txt =
         lexer.getNextToken(MasterToken::STRING).getString();
-    uint16_t error = 0;
+    uint32_t error = 0;
     // XXX: In the initial implementation we hardcode the mnemonics.
     // We'll soon generalize this.
     if (error_txt == "NOERROR") {
@@ -132,13 +130,16 @@ TSIG::constructFromLexer(MasterLexer& lexer) {
         error = TSIGError::BAD_TIME_CODE;
     } else {
         try {
-            error = boost::lexical_cast<uint16_t>(error_txt);
+            error = boost::lexical_cast<uint32_t>(error_txt);
         } catch (const boost::bad_lexical_cast&) {
             isc_throw(InvalidRdataText, "Invalid TSIG Error");
         }
+        if (error > 0xffff) {
+            isc_throw(InvalidRdataText, "TSIG Error out of range");
+        }
     }
 
-    const int32_t otherlen =
+    const uint32_t otherlen =
         lexer.getNextToken(MasterToken::NUMBER).getNumber();
     if (otherlen > 0xffff) {
         isc_throw(InvalidRdataText, "TSIG Other Len out of range");
@@ -151,7 +152,8 @@ TSIG::constructFromLexer(MasterLexer& lexer) {
         isc_throw(InvalidRdataText,
                   "TSIG Other Data length does not match Other Len");
     }
-    // also verify Error == BADTIME?
+    // RFC2845 says Other Data is "empty unless Error == BADTIME".
+    // However, we don't enforce that.
 
     return (new TSIGImpl(algorithm, time_signed, fudge, mac, orig_id,
                          error, other_data));
@@ -212,7 +214,7 @@ TSIG::TSIG(const std::string& tsig_str) : impl_(NULL) {
         MasterLexer lexer;
         lexer.pushSource(ss);
 
-        impl_ptr.reset(constructFromLexer(lexer));
+        impl_ptr.reset(constructFromLexer(lexer, NULL));
 
         if (lexer.getNextToken().getType() != MasterToken::END_OF_FILE) {
             isc_throw(InvalidRdataText,
@@ -242,9 +244,9 @@ TSIG::TSIG(const std::string& tsig_str) : impl_(NULL) {
 ///
 /// \param lexer A \c MasterLexer object parsing a master file for the
 /// RDATA to be created
-TSIG::TSIG(MasterLexer& lexer, const Name*,
-               MasterLoader::Options, MasterLoaderCallbacks&) :
-    impl_(constructFromLexer(lexer))
+TSIG::TSIG(MasterLexer& lexer, const Name* origin,
+           MasterLoader::Options, MasterLoaderCallbacks&) :
+    impl_(constructFromLexer(lexer, origin))
 {
 }
 
@@ -268,7 +270,9 @@ TSIG::TSIG(MasterLexer& lexer, const Name*,
 /// But this constructor does not use this parameter; if necessary, the caller
 /// must check consistency between the length parameter and the actual
 /// RDATA length.
-TSIG::TSIG(InputBuffer& buffer, size_t) : impl_(NULL) {
+TSIG::TSIG(InputBuffer& buffer, size_t) :
+    impl_(NULL)
+{
     Name algorithm(buffer);
 
     uint8_t time_signed_buf[6];

src/lib/dns/rdata/any_255/tsig_250.h

@@ -142,7 +142,7 @@ public:
     /// This method never throws an exception.
     const void* getOtherData() const;
 private:
-    TSIGImpl* constructFromLexer(isc::dns::MasterLexer& lexer);
+    TSIGImpl* constructFromLexer(MasterLexer& lexer, const Name* origin);
 
     TSIGImpl* impl_;
 };

src/lib/dns/rdata/generic/sshfp_44.cc

@@ -135,9 +135,8 @@ SSHFP::SSHFP(const string& sshfp_str) :
 /// RDATA to be created
 SSHFP::SSHFP(MasterLexer& lexer, const Name*,
              MasterLoader::Options, MasterLoaderCallbacks&) :
-    impl_(NULL)
+    impl_(constructFromLexer(lexer))
 {
-    impl_ = constructFromLexer(lexer);
 }
 
 /// \brief Constructor from InputBuffer.

src/lib/dns/tests/rdata_tsig_unittest.cc

@@ -38,23 +38,22 @@ using namespace isc::util;
 using namespace isc::dns::rdata;
 
 namespace {
-const char* const valid_text1 = "hmac-md5.sig-alg.reg.int. 1286779327 300 "
+const string valid_text1 = "hmac-md5.sig-alg.reg.int. 1286779327 300 "
     "0 16020 BADKEY 0";
-const char* const valid_text2 = "hmac-sha256. 1286779327 300 12 "
+const string valid_text2 = "hmac-sha256. 1286779327 300 12 "
     "FAKEFAKEFAKEFAKE 16020 BADSIG 0";
-
-const char* const valid_text3 = "hmac-sha1. 1286779327 300 12 "
+const string valid_text3 = "hmac-sha1. 1286779327 300 12 "
     "FAKEFAKEFAKEFAKE 16020 BADTIME 6 FAKEFAKE";
-const char* const valid_text4 = "hmac-sha1. 1286779327 300 12 "
+const string valid_text4 = "hmac-sha1. 1286779327 300 12 "
     "FAKEFAKEFAKEFAKE 16020 BADSIG 6 FAKEFAKE";
-const char* const valid_text5 = "hmac-sha256. 1286779327 300 12 "
+const string valid_text5 = "hmac-sha256. 1286779327 300 12 "
     "FAKEFAKEFAKEFAKE 16020 2845 0"; // using numeric error code
-const char* const too_long_label = "012345678901234567890123456789"
-    "0123456789012345678901234567890123";
 
 class Rdata_TSIG_Test : public RdataTest {
 protected:
-    Rdata_TSIG_Test() : rdata_tsig(valid_text1) {}
+    Rdata_TSIG_Test() :
+        rdata_tsig(valid_text1)
+    {}
 
     void checkFromText_InvalidText(const string& rdata_str) {
         checkFromText<any::TSIG, InvalidRdataText, InvalidRdataText>(
@@ -82,6 +81,12 @@ protected:
             rdata_str, rdata_tsig, true, true);
     }
 
+    void checkFromText_BadString(const string& rdata_str) {
+        checkFromText
+            <any::TSIG, InvalidRdataText, isc::Exception>(
+                rdata_str, rdata_tsig, true, false);
+    }
+
     template <typename Output>
     void toWireCommonChecks(Output& output) const;
 
@@ -118,11 +123,13 @@ TEST_F(Rdata_TSIG_Test, fromText) {
 
 TEST_F(Rdata_TSIG_Test, badText) {
     // too many fields
-    EXPECT_THROW(any::TSIG("foo 0 0 0 0 BADKEY 0 0"), InvalidRdataText);
+    checkFromText_BadString(valid_text1 + " 0 0");
     // not enough fields
     checkFromText_LexerError("foo 0 0 0 0 BADKEY");
     // bad domain name
-    checkFromText_TooLongLabel(string(too_long_label) + "0 0 0 0 BADKEY 0");
+    checkFromText_TooLongLabel(
+        "0123456789012345678901234567890123456789012345678901234567890123"
+        " 0 0 0 0 BADKEY 0");
     checkFromText_EmptyLabel("foo..bar 0 0 0 0 BADKEY");
     // time is too large (2814...6 is 2^48)
     checkFromText_InvalidText("foo 281474976710656 0 0 0 BADKEY 0");
@@ -150,8 +157,12 @@ TEST_F(Rdata_TSIG_Test, badText) {
     checkFromText_InvalidText("foo 0 0 0 0 TEST 0");
     // Numeric error code is too large
     checkFromText_InvalidText("foo 0 0 0 0 65536 0");
+    // Numeric error code is negative
+    checkFromText_InvalidText("foo 0 0 0 0 -1 0");
     // Other len is too large
     checkFromText_InvalidText("foo 0 0 0 0 NOERROR 65536 FAKE");
+    // Other len is negative
+    checkFromText_LexerError("foo 0 0 0 0 NOERROR -1 FAKE");
     // invalid Other len
     checkFromText_LexerError("foo 0 0 0 0 NOERROR LEN FAKE");
     // Other len and data mismatch
@@ -284,16 +295,6 @@ TEST_F(Rdata_TSIG_Test, createFromParams) {
                  isc::InvalidParameter);
 }
 
-TEST_F(Rdata_TSIG_Test, createFromLexer) {
-    EXPECT_EQ(0, rdata_tsig.compare(
-        *test::createRdataUsingLexer(RRType::TSIG(), RRClass::ANY(),
-                                     valid_text1)));
-
-    // Exceptions cause NULL to be returned.
-    EXPECT_FALSE(test::createRdataUsingLexer(RRType::TSIG(), RRClass::ANY(),
-                                             "foo 0 0 0 0 BADKEY 0 0"));
-}
-
 TEST_F(Rdata_TSIG_Test, assignment) {
     any::TSIG copy((string(valid_text2)));
     copy = rdata_tsig;