From: Piotrek Zadroga Date: Thu, 13 Apr 2023 13:25:31 +0000 (+0200) Subject: [#2536] Implementing DNRv6 Option with TDD X-Git-Tag: Kea-2.3.8~180 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=92c5cf72f13db3f56734c35bab62ac87ec0b335d;p=thirdparty%2Fkea.git [#2536] Implementing DNRv6 Option with TDD --- diff --git a/src/lib/dhcp/option_dnr.cc b/src/lib/dhcp/option_dnr.cc index 0ed9d6c047..314327a32c 100644 --- a/src/lib/dhcp/option_dnr.cc +++ b/src/lib/dhcp/option_dnr.cc @@ -60,18 +60,18 @@ OptionDnr6::packAdn(util::OutputBuffer& buf) const { void OptionDnr6::packAddresses(util::OutputBuffer& buf) const { - for (auto address = ipv6_addresses_.begin(); address != ipv6_addresses_.end(); ++address) { - if (!address->isV6()) { - isc_throw(isc::BadValue, address->toText() << " is not an IPv6 address"); + for (const auto& address : ipv6_addresses_) { + if (!address.isV6()) { + isc_throw(isc::BadValue, address.toText() << " is not an IPv6 address"); } - buf.writeData(&address->toBytes()[0], V6ADDRESS_LEN); + buf.writeData(&address.toBytes()[0], V6ADDRESS_LEN); } } void OptionDnr6::packSvcParams(util::OutputBuffer& buf) const { if (svc_params_length_ > 0) { - buf.writeData(&*svc_params_.begin(), svc_params_length_); + buf.writeData(&(*svc_params_.begin()), svc_params_length_); } } @@ -84,7 +84,7 @@ OptionDnr6::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { } setData(begin, end); // First two octets of Option data is Service Priority - this is mandatory field. - service_priority_ = isc::util::readUint16((&*begin), SERVICE_PRIORITY_SIZE); + service_priority_ = isc::util::readUint16(&(*begin), SERVICE_PRIORITY_SIZE); begin += SERVICE_PRIORITY_SIZE; // Next come two octets of ADN Length plus the ADN data itself (variable length). @@ -100,7 +100,7 @@ OptionDnr6::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { } // Let's try to extract ADN FQDN data. - isc::util::InputBuffer name_buf(&adn_tuple.getData()[0], adn_length_); + isc::util::InputBuffer name_buf(&(*adn_tuple.getData().begin()), adn_length_); try { adn_.reset(new isc::dns::Name(name_buf, true)); } catch (const Exception& ex) { @@ -122,13 +122,30 @@ OptionDnr6::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { "2 bytes long Addr Length field"); } // Next come two octets of Addr Length. - addr_length_ = isc::util::readUint16((&*begin), ADDR_LENGTH_SIZE); + addr_length_ = isc::util::readUint16(&(*begin), ADDR_LENGTH_SIZE); begin += ADDR_LENGTH_SIZE; // It MUST be a multiple of 16. if ((addr_length_ % V6ADDRESS_LEN) != 0) { isc_throw(OutOfRange, "DHCPv6 Encrypted DNS Option (" << type_ << ")" << " malformed: Addr Len=" << addr_length_ - << " is not divisible by 16."); + << " is not divisible by 16"); + } + + // As per draft-ietf-add-dnr 3.1.8: + // If additional data is supplied (i.e. not ADN only mode), + // the option includes at least one valid IP address. + if (addr_length_ == 0) { + isc_throw(OutOfRange, "DHCPv6 Encrypted DNS Option (" << type_ << ")" + << " malformed: Addr Len=" << addr_length_ + << " is not greater than 0"); + } + + // Check if IPv6 Address(es) field is not truncated. + if (std::distance(begin, end) < addr_length_) { + isc_throw(OutOfRange, "DHCPv6 Encrypted DNS Option (" << type_ << ")" + << " malformed: Addr Len=" << addr_length_ + << " but IPv6 address(es) are truncated to len=" + << std::distance(begin, end)); } // Let's unpack the ipv6-address(es). @@ -138,17 +155,36 @@ OptionDnr6::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) { begin += V6ADDRESS_LEN; } - // SvcParams (variable length) field is last + // SvcParams (variable length) field is last. svc_params_length_ = std::distance(begin, end); if (svc_params_length_ > 0) { - // for now let's keep SvcParams as binary data in buffer svc_params_.assign(begin, end); + checkSvcParams(); } } std::string OptionDnr6::toText(int indent) const { - return Option::toText(indent); + std::ostringstream stream; + std::string in(indent, ' '); // base indentation + stream << in << "type=" << type_ << "(V6_DNR), " + << "len=" << (len() - getHeaderLen()) << ", " + << "service_priority=" << service_priority_ << ", " + << "adn_length=" << adn_length_ << ", " + << "adn='" << getAdn() << "'"; + if (!adn_only_mode_) { + stream << ", addr_length=" << addr_length_ + << ", address(es):"; + for (const auto& address : ipv6_addresses_) { + stream << " " << address.toText(); + } + + if (svc_params_length_ > 0) { + stream << ", svc_params='" << svc_params_ << "'"; + } + } + + return (stream.str()); } uint16_t @@ -168,6 +204,11 @@ OptionDnr6::getAdn() const { return (""); } +void +OptionDnr6::checkSvcParams() const { + // TODO: check SvcParams and throw in case something is wrong +} + OptionDnr4::OptionDnr4() : Option(V4, DHO_V4_DNR) { } diff --git a/src/lib/dhcp/option_dnr.h b/src/lib/dhcp/option_dnr.h index 54a46556bf..efc1c40c5b 100644 --- a/src/lib/dhcp/option_dnr.h +++ b/src/lib/dhcp/option_dnr.h @@ -61,7 +61,7 @@ public: virtual OptionPtr clone() const; virtual void pack(util::OutputBuffer& buf, bool check) const; virtual void unpack(OptionBufferConstIter begin, OptionBufferConstIter end); - virtual std::string toText(int indent) const; + virtual std::string toText(int indent = 0) const; virtual uint16_t len() const; /// @brief Getter of the @c service_priority_. @@ -147,7 +147,7 @@ protected: /// /// Specifies a set of service parameters that are encoded /// following the rules in Section 2.1 of [I-D.ietf-dnsop-svcb-https]. - std::vector svc_params_; + std::string svc_params_; private: /// @brief Returns minimal length of the option data (without headers) in octets. @@ -185,6 +185,12 @@ private: /// /// @param [out] buf buffer where SvcParams will be written. void packSvcParams(isc::util::OutputBuffer& buf) const; + + /// @brief Checks SvcParams field if encoded correctly and throws in case of issue found. + /// + /// The field should be encoded following the rules in + /// Section 2.1 of [I-D.ietf-dnsop-svcb-https]. + void checkSvcParams() const; }; class OptionDnr4 : public Option { diff --git a/src/lib/dhcp/tests/option_dnr_unittest.cc b/src/lib/dhcp/tests/option_dnr_unittest.cc index dc3d4e96a1..75ff6556e9 100644 --- a/src/lib/dhcp/tests/option_dnr_unittest.cc +++ b/src/lib/dhcp/tests/option_dnr_unittest.cc @@ -47,9 +47,21 @@ TEST(OptionDnr6Test, constructorAdnOnlyMode) { EXPECT_EQ(0x8001, option->getServicePriority()); EXPECT_EQ(20, option->getAdnLength()); EXPECT_EQ("myhost.example.com.", option->getAdn()); + + // This is ADN only mode, so Addr Length and SvcParams Length + // are both expected to be zero. EXPECT_EQ(0, option->getAddrLength()); EXPECT_EQ(0, option->getSvcParamsLength()); + + // BTW let's check if len() works ok. + // expected len: 20 (FQDN) + 2 (ADN Len) + 2 (Service priority) + 4 (headers) = 28. EXPECT_EQ(28, option->len()); + + // BTW let's check if toText() works ok. + // toText() len does not count in headers len. + EXPECT_EQ("type=144(V6_DNR), len=24, " + "service_priority=32769, adn_length=20, " + "adn='myhost.example.com.'", option->toText()); } TEST(OptionDnr6Test, constructorDataTruncated) { @@ -141,22 +153,10 @@ TEST(OptionDnr6Test, addrLenZero) { }; OptionBuffer buf(buf_data, buf_data + sizeof(buf_data)); - // Create option instance. Check that constructor doesn't throw. + // Create option instance. Check that constructor throws OutOfRange exception. scoped_ptr option; - EXPECT_NO_THROW(option.reset(new OptionDnr6(buf.begin(), buf.end()))); - ASSERT_TRUE(option); - - // Check if member variables were correctly set by ctor. - EXPECT_EQ(Option::V6, option->getUniverse()); - EXPECT_EQ(D6O_V6_DNR, option->getType()); - - // Check if data was unpacked correctly from wire data. - EXPECT_EQ(0x8001, option->getServicePriority()); - EXPECT_EQ(20, option->getAdnLength()); - EXPECT_EQ("myhost.example.com.", option->getAdn()); - EXPECT_EQ(0, option->getAddrLength()); - EXPECT_EQ(0, option->getSvcParamsLength()); - EXPECT_EQ(30, option->len()); + EXPECT_THROW(option.reset(new OptionDnr6(buf.begin(), buf.end())), OutOfRange); + ASSERT_FALSE(option); } TEST(OptionDnr6Test, addrLenNot16Modulo) { @@ -215,7 +215,42 @@ TEST(OptionDnr6Test, validIpV6Addresses) { EXPECT_EQ("ff02::face:b00c", addresses[1].toText()); EXPECT_EQ("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", addresses[2].toText()); EXPECT_EQ(0, option->getSvcParamsLength()); + + // BTW let's check if len() works ok. + // expected len: 20 (FQDN) + 2 (ADN Len) + 2 (Service priority) + 4 (headers) = 28 + // + 48 (3 IP addresses) + 2 (Addr Len) = 78 EXPECT_EQ(78, option->len()); + + // BTW let's check if toText() works ok. + // toText() len does not count in headers len. + EXPECT_EQ("type=144(V6_DNR), len=74, " + "service_priority=32769, adn_length=20, " + "adn='myhost.example.com.', " + "addr_length=48, " + "address(es): 2001:db8:1::dead:beef " + "ff02::face:b00c " + "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", option->toText()); +} + +TEST(OptionDnr6Test, truncatedIpV6Addresses) { + // Prepare data to decode + const uint8_t buf_data[] = { + 0x80, 0x01, // Service priority is 32769 dec + 0x00, 0x14, // ADN Length is 20 dec + 0x06, 0x4D, 0x79, 0x68, 0x6F, 0x73, 0x74, // FQDN: Myhost. + 0x07, 0x45, 0x78, 0x61, 0x6D, 0x70, 0x6C, 0x65, // Example. + 0x03, 0x43, 0x6F, 0x6D, 0x00, // Com. + 0x00, 0x30, // Addr Len field value = 48 dec + 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01, 0x00, 0x00, // 2001:db8:1::dead:beef + 0x00, 0x00, 0x00, 0x00, 0xde, 0xad, 0xbe, 0xef, + 0xff, 0x02, 0x00 // IPv6 address truncated + }; + + OptionBuffer buf(buf_data, buf_data + sizeof(buf_data)); + // Create option instance. Check that constructor throws OutOfRange exception. + scoped_ptr option; + EXPECT_THROW(option.reset(new OptionDnr6(buf.begin(), buf.end())), OutOfRange); + ASSERT_FALSE(option); } TEST(OptionDnr6Test, svcParamsIncluded) { @@ -229,7 +264,7 @@ TEST(OptionDnr6Test, svcParamsIncluded) { 0x00, 0x10, // Addr Len field value = 48 dec 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01, 0x00, 0x00, // 2001:db8:1::dead:beef 0x00, 0x00, 0x00, 0x00, 0xde, 0xad, 0xbe, 0xef, - 0xff, 0xff // example SvcParams data + 'a', 'b', 'c' // example SvcParams data }; OptionBuffer buf(buf_data, buf_data + sizeof(buf_data)); @@ -250,8 +285,21 @@ TEST(OptionDnr6Test, svcParamsIncluded) { const OptionDnr6::AddressContainer& addresses = option->getAddresses(); EXPECT_EQ(1, addresses.size()); EXPECT_EQ("2001:db8:1::dead:beef", addresses[0].toText()); - EXPECT_EQ(2, option->getSvcParamsLength()); - EXPECT_EQ(48, option->len()); + EXPECT_EQ(3, option->getSvcParamsLength()); + + // BTW let's check if len() works ok. + // expected len: 20 (FQDN) + 2 (ADN Len) + 2 (Service priority) + 4 (headers) = 28 + // + 16 (IP address) + 2 (Addr Len) + 3 (SvcParams) = 49 + EXPECT_EQ(49, option->len()); + + // BTW let's check if toText() works ok. + // toText() len does not count in headers len. + EXPECT_EQ("type=144(V6_DNR), len=45, " + "service_priority=32769, adn_length=20, " + "adn='myhost.example.com.', " + "addr_length=16, " + "address(es): 2001:db8:1::dead:beef, " + "svc_params='abc'", option->toText()); } } // namespace \ No newline at end of file