- addressed some review comments in rdata implementation

- fixed formatting error with DNSSEC time
- decodeHex() can read lower-case hex digits now
- added fromWire constructor for RRSIG, and associated unit test


git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@1014 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/dns/cpp/dnstime.cc

@@ -47,9 +47,13 @@ DNSSECTimeToText(const time_t timeval, string& s)
 
     s.reserve(14);              // YYYYMMDDHHmmSS
     ostringstream oss(s);
-    oss << setfill('0') << setw(4) << t->tm_year + 1900
-        << setw(2) << t->tm_mon + 1 << t->tm_mday 
-        << t->tm_hour << t->tm_min << t->tm_sec;
+    oss << setfill('0')
+        << setw(4) << t->tm_year + 1900
+        << setw(2) << t->tm_mon + 1
+        << setw(2) << t->tm_mday 
+        << setw(2) << t->tm_hour
+        << setw(2) << t->tm_min
+        << setw(2) << t->tm_sec;
     s = oss.str();
 }
 

src/lib/dns/cpp/hex.cc

@@ -63,13 +63,25 @@ decodeHex(const std::string& hex, std::vector<uint8_t>& result)
     iss.width(1);
     if ((hex.size() % 2) == 1) {
         iss >> c2;
-        n = strchr(hexdigits, c2) - hexdigits;
+        const char* pos = strchr(hexdigits, toupper(c2));
+        if (!pos) {
+            dns_throw (BadHexString, "Invalid hex digit");
+        }
+
+        n = pos - hexdigits;
         result.push_back(n);
     }
     while (!iss.eof()) {
         iss >> c1 >> c2;;
-        n = (strchr(hexdigits, c1) - hexdigits) << 4;
-        n |= (strchr(hexdigits, c2) - hexdigits);
+
+        const char* pos1 = strchr(hexdigits, toupper(c1));
+        const char* pos2 = strchr(hexdigits, toupper(c2));
+        if (!pos1 || !pos2) {
+            dns_throw (BadHexString, "Invalid hex digit");
+        }
+
+        n = (pos1 - hexdigits) << 4;
+        n |= (pos2 - hexdigits);
 
         if (!iss.bad() && !iss.fail()) {
             result.push_back(n);

src/lib/dns/cpp/rdata/generic/dnskey_48.cc

@@ -74,11 +74,19 @@ DNSKEY::DNSKEY(const string& dnskey_str) :
     vector<uint8_t> keydata;
     decodeBase64(keydatabuf.str(), keydata);
 
+    if (algorithm == 1 && keydata.size() < 3) {
+        dns_throw(InvalidRdataText, "DNSKEY keydata too short");
+    }
+
     impl_ = new DNSKEYImpl(flags, protocol, algorithm, keydata);
 }
 
 DNSKEY::DNSKEY(InputBuffer& buffer, size_t rdata_len)
 {
+    if (rdata_len < 4) {
+        dns_throw(InvalidRdataLength, "DNSKEY too short");
+    }
+
     uint16_t flags = buffer.readUint16();
     uint16_t protocol = buffer.readUint8();
     uint16_t algorithm = buffer.readUint8();
@@ -174,11 +182,7 @@ DNSKEY::getTag() const
 {
     if (impl_->algorithm_ == 1) {
         int len = impl_->keydata_.size();
-        if (len >= 3) {
-            return ((impl_->keydata_[len - 3] << 8) + impl_->keydata_[len - 2]);
-        } else {
-            dns_throw(InvalidRdataText, "Bad keydata");
-        }
+        return ((impl_->keydata_[len - 3] << 8) + impl_->keydata_[len - 2]);
     }
 
     uint32_t ac = impl_->flags_;
@@ -199,7 +203,7 @@ DNSKEY::getFlags() const {
 }
 
 uint8_t
-DNSKEY::getAlg() const {
+DNSKEY::getAlgorithm() const {
     return (impl_->algorithm_);
 }
 

src/lib/dns/cpp/rdata/generic/dnskey_48.h

@@ -46,7 +46,7 @@ public:
     ///
     uint16_t getTag() const;
     uint16_t getFlags() const;
-    uint8_t getAlg() const;
+    uint8_t getAlgorithm() const;
 
 private:
     DNSKEYImpl* impl_;

src/lib/dns/cpp/rdata/generic/ds_43.cc

@@ -78,6 +78,10 @@ DS::DS(const string& ds_str) :
 
 DS::DS(InputBuffer& buffer, size_t rdata_len)
 {
+    if (rdata_len < 4) {
+        dns_throw(InvalidRdataLength, "DS too short");
+    }
+
     uint16_t tag = buffer.readUint16();
     uint16_t algorithm = buffer.readUint8();
     uint16_t digest_type = buffer.readUint8();

src/lib/dns/cpp/rdata/generic/nsec_47.cc

@@ -93,6 +93,10 @@ NSEC::NSEC(InputBuffer& buffer, size_t rdata_len)
     Name nextname(buffer);
     rdata_len -= (buffer.getPosition() - pos);
 
+    if (rdata_len == 0) {
+        dns_throw(InvalidRdataLength, "NSEC too short");
+    }
+
     vector<uint8_t> typebits;
     for (int i = 0; i < rdata_len; i++) {
         typebits.push_back(buffer.readUint8());
@@ -131,14 +135,10 @@ NSEC::toText() const
     int len = 0;
     s << impl_->nextname_;
     for (int i = 0; i < impl_->typebits_.size(); i += len) {
-        if (i + 2 > impl_->typebits_.size()) {
-            dns_throw(InvalidRdataText, "Invalid NSEC Rdata");
-        }
+        assert(i + 2 <= impl_->typebits_.size());
         int window = impl_->typebits_[i];
         len = impl_->typebits_[i + 1];
-        if (len < 0 || len >= 32) {
-            dns_throw(InvalidRdataText, "Invalid NSEC Rdata");
-        }
+        assert(len >= 0 && len < 32);
         i += 2;
         for (int j = 0; j < len; j++) {
             if (impl_->typebits_[i + j] == 0) {

src/lib/dns/cpp/rdata/generic/rrsig_46.cc

@@ -43,11 +43,11 @@ struct RRSIGImpl {
     // straightforward representation of RRSIG RDATA fields
     RRSIGImpl(const RRType& covered, uint8_t algorithm, uint8_t labels,
               uint32_t originalttl, uint32_t timeexpire, uint32_t timeinception,
-              uint16_t keyid, const Name& signer,
+              uint16_t tag, const Name& signer,
               const vector<uint8_t>& signature) :
         covered_(covered), algorithm_(algorithm), labels_(labels),
         originalttl_(originalttl), timeexpire_(timeexpire),
-        timeinception_(timeinception), keyid_(keyid), signer_(signer),
+        timeinception_(timeinception), tag_(tag), signer_(signer),
         signature_(signature)
     {}
 
@@ -57,7 +57,7 @@ struct RRSIGImpl {
     uint32_t originalttl_;
     uint32_t timeexpire_;
     uint32_t timeinception_;
-    uint16_t keyid_;
+    uint16_t tag_;
     const Name signer_;
     const vector<uint8_t> signature_;
 };
@@ -69,11 +69,11 @@ RRSIG::RRSIG(const string& rrsig_str) :
     string covered_txt, signer_txt, expire_txt, inception_txt;
     unsigned int algorithm, labels;
     uint32_t originalttl;
-    uint16_t keyid;
+    uint16_t tag;
     stringbuf signaturebuf;
 
     iss >> covered_txt >> algorithm >> labels >> originalttl
-        >> expire_txt >> inception_txt >> keyid >> signer_txt
+        >> expire_txt >> inception_txt >> tag >> signer_txt
         >> &signaturebuf;
     if (iss.bad() || iss.fail() || !iss.eof()) {
         dns_throw(InvalidRdataText, "Invalid RRSIG text");
@@ -92,12 +92,41 @@ RRSIG::RRSIG(const string& rrsig_str) :
     decodeBase64(signaturebuf.str(), signature);
 
     impl_ = new RRSIGImpl(RRType(covered_txt), algorithm, labels,
-                          originalttl, timeexpire, timeinception, keyid,
+                          originalttl, timeexpire, timeinception, tag,
                           Name(signer_txt), signature);
 }
 
 RRSIG::RRSIG(InputBuffer& buffer, size_t rdata_len)
 {
+    size_t pos = buffer.getPosition();
+
+    if (rdata_len < 18) {
+        dns_throw(InvalidRdataLength, "DS too short");
+    }
+
+    uint16_t typecode = buffer.readUint16();
+    RRType covered(typecode);
+    uint8_t algorithm = buffer.readUint8();
+    uint8_t labels = buffer.readUint8();
+    uint32_t originalttl = buffer.readUint32();
+    uint32_t timeexpire = buffer.readUint32();
+    uint32_t timeinception = buffer.readUint32();
+    uint16_t tag = buffer.readUint16();
+    Name signer(buffer);
+
+    rdata_len -= (buffer.getPosition() - pos);
+    if (rdata_len == 0) {
+        dns_throw(InvalidRdataLength, "DS too short");
+    }
+
+    vector<uint8_t> signature;
+    for (int i = 0; i < rdata_len; i++) {
+        signature.push_back(buffer.readUint8());
+    }
+
+    impl_ = new RRSIGImpl(covered, algorithm, labels,
+                          originalttl, timeexpire, timeinception, tag,
+                          signer, signature);
 }
 
 RRSIG::RRSIG(const RRSIG& source) :
@@ -136,7 +165,7 @@ RRSIG::toText() const
             + " " + boost::lexical_cast<string>(impl_->originalttl_)
             + " " + expire
             + " " + inception
-            + " " + boost::lexical_cast<string>(impl_->keyid_)
+            + " " + boost::lexical_cast<string>(impl_->tag_)
             + " " + impl_->signer_.toText()
             + " " + encodeBase64(impl_->signature_));
 }
@@ -150,7 +179,7 @@ RRSIG::toWire(OutputBuffer& buffer) const
     buffer.writeUint32(impl_->originalttl_);
     buffer.writeUint32(impl_->timeexpire_);
     buffer.writeUint32(impl_->timeinception_);
-    buffer.writeUint16(impl_->keyid_);
+    buffer.writeUint16(impl_->tag_);
     impl_->signer_.toWire(buffer);
     buffer.writeData(&impl_->signature_[0], impl_->signature_.size());
 }
@@ -164,7 +193,7 @@ RRSIG::toWire(MessageRenderer& renderer) const
     renderer.writeUint32(impl_->originalttl_);
     renderer.writeUint32(impl_->timeexpire_);
     renderer.writeUint32(impl_->timeinception_);
-    renderer.writeUint16(impl_->keyid_);
+    renderer.writeUint16(impl_->tag_);
     renderer.writeName(impl_->signer_, false);
     renderer.writeData(&impl_->signature_[0], impl_->signature_.size());
 }
@@ -196,8 +225,8 @@ RRSIG::compare(const Rdata& other) const
         return (impl_->timeinception_ < other_rrsig.impl_->timeinception_ ?
                 -1 : 1);
     }
-    if (impl_->keyid_ != other_rrsig.impl_->keyid_) {
-        return (impl_->keyid_ < other_rrsig.impl_->keyid_ ? -1 : 1);
+    if (impl_->tag_ != other_rrsig.impl_->tag_) {
+        return (impl_->tag_ < other_rrsig.impl_->tag_ ? -1 : 1);
     }
 
     int cmp = compareNames(impl_->signer_, other_rrsig.impl_->signer_);

src/lib/dns/cpp/tests/hex_unittest.cc

@@ -69,7 +69,6 @@ TEST_F(HexTest, decodeHex) {
     decodeHex(hex_txt, result);
     compareData(result);
 
-#if 0
     // lower case hex digits should be accepted
     result.clear();
     decodeHex(hex_txt_lower, result);
@@ -77,8 +76,7 @@ TEST_F(HexTest, decodeHex) {
 
     // Bogus input: should fail
     result.clear();
-    decodeHex("1x", result);
-#endif
+    EXPECT_THROW(decodeHex("1x", result), BadHexString);
 }
 
 }

src/lib/dns/cpp/tests/rdata_rrsig_unittest.cc

@@ -59,4 +59,15 @@ TEST_F(Rdata_RRSIG_Test, toWireRenderer_RRSIG)
     rdata_rrsig.toWire(renderer);
 }
 
+TEST_F(Rdata_RRSIG_Test, createFromWire_RRSIG)
+{
+    string rrsig_txt("A 5 2 43200 20100327070149 20100225070149 2658 isc.org. "
+                "HkJk/xZTvzePU8NENl/ley8bbUumhk1hXciyqhLnz1VQFzkDooej6neX"
+                "ZgWZzQKeTKPOYWrnYtdZW4PnPQFeUl3orgLev7F8J6FZlDn0y/J/ThR5"
+                "m36Mo2/Gdxjj8lJ/IjPVkdpKyBpcnYND8KEIma5MyNCNeyO1UkfPQZGHNSQ=");
+    EXPECT_EQ(rrsig_txt, rdataFactoryFromFile(RRType("RRSIG"), RRClass("IN"),
+                             "testdata/rdata_rrsig_fromWire")->toText());
+}
+
+
 }