[3548] Fixes, cleanups and unit-tests in Pkt6::getMACFromDUID()

src/lib/dhcp/dhcp6.h

@@ -104,13 +104,8 @@
 extern const char *dhcpv6_type_names[];
 extern const int dhcpv6_type_name_max;
 
-/* DUID type definitions (RFC3315 section 9).
- */
+// DUID type definitions (RFC3315 section 9).
 // see isc::dhcp::DUID::DUIDType enum in dhcp/duid.h
-// #define DUID_LLT        1
-// #define DUID_EN         2
-// #define DUID_LL         3
-// #define DUID_UUID       4
 
 // Define hardware types
 // Taken from http://www.iana.org/assignments/arp-parameters/

src/lib/dhcp/pkt.cc

@@ -144,9 +144,13 @@ Pkt::getMAC(uint32_t hw_addr_src) {
     // Method 2: Extracted from DUID-LLT or DUID-LL
     if(hw_addr_src & HWADDR_SOURCE_DUID) {
         mac = getMACFromDUID();
-        if (mac){
+        if (mac) {
             return (mac);
-        } else return (HWAddrPtr());
+        } else if (hw_addr_src == HWADDR_SOURCE_DUID) {
+            // If the only source allowed is DUID then we can skip the other
+            // methods.
+            return (HWAddrPtr());
+        }
     }
 
     // Method 3: Extracted from source IPv6 link-local address

src/lib/dhcp/pkt.h

@@ -547,6 +547,19 @@ protected:
     /// @return hardware address (or NULL)
     virtual HWAddrPtr getMACFromIPv6RelayOpt() = 0;
 
+
+    /// @brief Attempts to obtain MAC address from DUID-LL or DUID-LLT
+    ///
+    /// This method is called from getMAC(HWADDR_SOURCE_DUID) and should not be
+    /// called directly. It will attempt to extract MAC address information
+    /// from DUID if its type is LLT or LL. If this method fails, it will
+    /// return NULL.
+    ///
+    /// @note This is a pure virtual method and must be implemented in
+    /// the derived classes. The @c Pkt6 class have respective implementation.
+    /// This method is not applicable to DHCPv4.
+    ///
+    /// @return hardware address (or NULL)
     virtual HWAddrPtr getMACFromDUID() = 0;
 
     /// @brief Attempts to convert IPv6 address into MAC.
@@ -565,8 +578,6 @@ protected:
     HWAddrPtr
     getMACFromIPv6(const isc::asiolink::IOAddress& addr);
 
-
-
     /// Transaction-id (32 bits for v4, 24 bits for v6)
     uint32_t transid_;
 

src/lib/dhcp/pkt4.h

@@ -414,6 +414,13 @@ protected:
         return (HWAddrPtr());
     }
 
+    /// @brief No-op
+    ///
+    /// This method returns hardware address extracted from DUID.
+    /// Currently it is a no-op, even though there's RFC that defines how to
+    /// use DUID in DHCPv4 (see RFC4361). We may implement it one day.
+    ///
+    /// @return always NULL
     virtual HWAddrPtr getMACFromDUID(){
     	return (HWAddrPtr());
     }

src/lib/dhcp/pkt6.cc

@@ -442,35 +442,50 @@ Pkt6::unpackTCP() {
 
 HWAddrPtr
 Pkt6::getMACFromDUID() {
-	OptionPtr opt_duid = getOption(D6O_CLIENTID);
-	uint8_t hlen = opt_duid->getData().size();
-	vector<uint8_t> hw_addr(hlen, 0);
-	std::vector<unsigned char> duid_data = opt_duid->getData();
-
+    OptionPtr opt_duid = getOption(D6O_CLIENTID);
+    if (!opt_duid) {
+        return (HWAddrPtr());
+    }
 
-	uint16_t hwtype = 0;
-	std::memcpy(&hwtype, &duid_data[2], 2);		// TODO nie jestem tego pewien
+    uint8_t hlen = opt_duid->getData().size();
+    vector<uint8_t> hw_addr(hlen, 0);
+    std::vector<unsigned char> duid_data = opt_duid->getData();
 
+    // Read the first two bytes. That duid type.
+    uint16_t duid_type = util::readUint16(&duid_data[0], duid_data.size());
 
-	// checking if DUID is LLT or LL type
-	if ((duid_data[0] == 0x00) && (duid_data[1]== 0x01)){
-		hlen -= 8;
-		std::memcpy(&hw_addr, &duid_data[8], hlen);
+    switch (duid_type) {
+    case DUID::DUID_LL:
+    {
+        // 2 bytes of duid type, 2 bytes of hardware type and at least
+        // 1 byte of actual identification
+        if (duid_data.size() < 5) {
+            // This duid is truncated. We can't extract anything from it.
+            return (HWAddrPtr());
+        }
 
-	}
-	else if ((duid_data[0] == 0x00) && (duid_data[1]== 0x03)){
-			hlen -= 4;
-			std::memcpy(&hw_addr, &duid_data[4], hlen);
-	}
-	// if none of them return NULL
-	else return HWAddrPtr();
+        uint16_t hwtype = util::readUint16(&duid_data[2], duid_data.size() - 2);
+        return (HWAddrPtr(new HWAddr(&duid_data[4], duid_data.size() - 4,
+                                     hwtype)));
+    }
+    case DUID::DUID_LLT:
+    {
+        // 2 bytes of duid type, 2 bytes of hardware, 4 bytes for timestamp,
+        // and at least 1 byte of actual identification
+        if (duid_data.size() < 9) {
+            // This duid is truncated. We can't extract anything from it.
+            return (HWAddrPtr());
+        }
 
-	hw_addr.resize(hlen);
-	hwaddr_= HWAddrPtr(new HWAddr(hw_addr, hwtype));
-	return (hwaddr_);
+        uint16_t hwtype = util::readUint16(&duid_data[2], duid_data.size() - 2);
+        return (HWAddrPtr(new HWAddr(&duid_data[8], duid_data.size() - 8,
+                                     hwtype)));
+    }
+    default:
+        return (HWAddrPtr());
+    }
 }
 
-
 std::string
 Pkt6::toText() {
     stringstream tmp;

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -1069,7 +1069,7 @@ TEST_F(Pkt6Test, getMACFromIPv6RelayOpt_singleRelay) {
         0x0a, 0x1b, 0x0b, 0x01, 0xca, 0xfe // MAC
     };
     OptionPtr relay_opt(new Option(Option::V6, 79,
-    		            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
+                            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
     info.options_.insert(make_pair(relay_opt->getType(), relay_opt));
 
     pkt.addRelayInfo(info);
@@ -1098,7 +1098,7 @@ TEST_F(Pkt6Test, getMACFromIPv6RelayOpt_multipleRelay) {
         0x1a, 0x30, 0x0b, 0xfa, 0xc0, 0xfe // MAC
     };
     OptionPtr relay_opt1(new Option(Option::V6, D6O_CLIENT_LINKLAYER_ADDR,
-    		            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
+                            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
 
     info1.options_.insert(make_pair(relay_opt1->getType(), relay_opt1));
     pkt.addRelayInfo(info1);
@@ -1117,7 +1117,7 @@ TEST_F(Pkt6Test, getMACFromIPv6RelayOpt_multipleRelay) {
     // We reuse the option and modify the MAC to be sure we get the right address
     opt_data[2] = 0xfa;
     OptionPtr relay_opt3(new Option(Option::V6, D6O_CLIENT_LINKLAYER_ADDR,
-    		            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
+                            OptionBuffer(opt_data, opt_data + sizeof(opt_data))));
     info3.options_.insert(make_pair(relay_opt3->getType(), relay_opt3));
     pkt.addRelayInfo(info3);
     ASSERT_EQ(3, pkt.relay_info_.size());
@@ -1131,4 +1131,56 @@ TEST_F(Pkt6Test, getMACFromIPv6RelayOpt_multipleRelay) {
     EXPECT_EQ(tmp.str(), found->toText(true));
 }
 
+TEST_F(Pkt6Test, getMACFromDUID) {
+    Pkt6 pkt(DHCPV6_ADVERTISE, 1234);
+
+    // Although MACs are typically 6 bytes long, let's make this test a bit
+    // more challenging and use odd MAC lengths.
+
+    uint8_t duid_llt[] = { 0, 1, // type (DUID-LLT)
+                           0, 7, // hwtype (7 - just a randomly picked value)
+                           1, 2, 3, 4, // timestamp
+                           0xa, 0xb, 0xc, 0xd, 0xe, 0xf, 0x10 // MAC address (7 bytes)
+    };
+
+    uint8_t duid_ll[] = { 0, 3, // type (DUID-LL)
+                        0, 11, // hwtype (11 - just a randomly picked value)
+                        0xa, 0xb, 0xc, 0xd, 0xe // MAC address (5 bytes)
+    };
+
+    uint8_t duid_en[] = { 0, 2, // type (DUID-EN)
+                        1, 2, 3, 4, // enterprise-id
+                        0xa, 0xb, 0xc // opaque data
+    };
+
+    OptionPtr clientid1(new Option(Option::V6, D6O_CLIENTID, OptionBuffer(
+                                       duid_llt, duid_llt + sizeof(duid_llt))));
+    OptionPtr clientid2(new Option(Option::V6, D6O_CLIENTID, OptionBuffer(
+                                       duid_ll, duid_ll + sizeof(duid_ll))));
+    OptionPtr clientid3(new Option(Option::V6, D6O_CLIENTID, OptionBuffer(
+                                       duid_en, duid_en + sizeof(duid_en))));
+
+    // Packet does not have any client-id, this call should fail
+    EXPECT_FALSE(pkt.getMAC(Pkt::HWADDR_SOURCE_DUID));
+
+    // Let's test DUID-LLT. This should work.
+    pkt.addOption(clientid1);
+    HWAddrPtr mac = pkt.getMAC(Pkt::HWADDR_SOURCE_DUID);
+    ASSERT_TRUE(mac);
+    EXPECT_EQ("htype=7 0a:0b:0c:0d:0e:0f:10", mac->toText(true));
+
+    // Let's test DUID-LL. This should work.
+    ASSERT_TRUE(pkt.delOption(D6O_CLIENTID));
+    pkt.addOption(clientid2);
+    mac = pkt.getMAC(Pkt::HWADDR_SOURCE_DUID);
+    ASSERT_TRUE(mac);
+    EXPECT_EQ("htype=11 0a:0b:0c:0d:0e", mac->toText(true));
+
+    // Finally, let's try DUID-EN. This should fail, as EN type does not
+    // contain any MAC address information.
+    ASSERT_TRUE(pkt.delOption(D6O_CLIENTID));
+    pkt.addOption(clientid3);
+    EXPECT_FALSE(pkt.getMAC(Pkt::HWADDR_SOURCE_DUID));
+}
+
 }