[2902] Adjust the destination address for response.

Packet will be sent to broadcast address if broadcast flag is set by the
client. Also, DHCPNAK is always sent to broadcast address.

src/bin/dhcp4/dhcp4_srv.cc

@@ -205,9 +205,9 @@ Dhcpv4Srv::run() {
             }
 
             if (rsp) {
-                if (rsp->getRemoteAddr().toText() == "0.0.0.0") {
-                    rsp->setRemoteAddr(query->getRemoteAddr());
-                }
+
+                adjustRemoteAddr(query, rsp);
+
                 if (!rsp->getHops()) {
                     rsp->setRemotePort(DHCP4_CLIENT_PORT);
                 } else {
@@ -358,14 +358,6 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     // relay address
     answer->setGiaddr(question->getGiaddr());
 
-    if (question->getGiaddr().toText() != "0.0.0.0") {
-        // relayed traffic
-        answer->setRemoteAddr(question->getGiaddr());
-    } else {
-        // direct traffic
-        answer->setRemoteAddr(question->getRemoteAddr());
-    }
-
     // Let's copy client-id to response. See RFC6842.
     OptionPtr client_id = question->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
     if (client_id) {
@@ -539,28 +531,6 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
 
         answer->setYiaddr(lease->addr_);
 
-        // If remote address is not set, we are dealing with a directly
-        // connected client requesting new lease. We can send response to
-        // the address assigned in the lease, but first we have to make sure
-        // that IfaceMgr supports responding directly to the client when
-        // client doesn't have address assigned to its interface yet.
-        if (answer->getRemoteAddr().toText() == "0.0.0.0") {
-            if (IfaceMgr::instance().isDirectResponseSupported()) {
-                answer->setRemoteAddr(lease->addr_);
-            } else {
-                // Since IfaceMgr does not support direct responses to
-                // clients not having IP addresses, we have to send response
-                // to broadcast. We don't check whether the use_bcast flag
-                // was set in the constructor, because this flag is only used
-                // by unit tests to prevent opening broadcast sockets, as
-                // it requires root privileges. If this function is invoked by
-                // unit tests, we expect that it sets broadcast address if
-                // direct response is not supported, so as a test can verify
-                // function's behavior, regardless of the use_bcast flag's value.
-                answer->setRemoteAddr(IOAddress("255.255.255.255"));
-            }
-        }
-
         // IP Address Lease time (type 51)
         opt = OptionPtr(new Option(Option::V4, DHO_DHCP_LEASE_TIME));
         opt->setUint32(lease->valid_lft_);
@@ -595,6 +565,60 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     }
 }
 
+void
+Dhcpv4Srv::adjustRemoteAddr(const Pkt4Ptr& question, Pkt4Ptr& msg) {
+    // Let's create static objects representing zeroed and broadcast
+    // addresses. We will use them further in this function to test
+    // other addresses against them. Since they are static, they will
+    // be created only once.
+    static const IOAddress zero_addr("0.0.0.0");
+    static const IOAddress bcast_addr("255.255.255.255");
+
+    // If received relayed message, server responds to the relay address.
+    if (question->getGiaddr() != zero_addr) {
+        msg->setRemoteAddr(question->getGiaddr());
+
+    // If giaddr is 0 but client set ciaddr, server should unicast the
+    // response to ciaddr.
+    } else if (question->getCiaddr() != zero_addr) {
+        msg->setRemoteAddr(question->getCiaddr());
+
+    // We can't unicast the response to the client when sending NAK,
+    // because we haven't allocated address for him. Therefore,
+    // NAK is broadcast.
+    } else if (msg->getType() == DHCPNAK) {
+        msg->setRemoteAddr(bcast_addr);
+
+    // If yiaddr is set it means that we have created a lease for a client.
+    } else if (question->getYiaddr() != zero_addr) {
+        // If the broadcast bit is set in the flags field, we have to
+        // send the response to broadcast address. Client may have requested it
+        // because it doesn't support reception of messages on the interface
+        // which doesn't have an address assigned. The other case when response
+        // must be broadcasted is when our server does not support responding
+        // directly to a client without address assigned.
+        bool bcast_flag = (question->getFlags() >> 0xF) ? true : false;
+        if (!IfaceMgr::instance().isDirectResponseSupported() || bcast_flag) {
+            msg->setRemoteAddr(bcast_addr);
+
+        // Client cleared the broadcast bit and we support direct responses
+        // so we should unicast the response to a newly allocated address -
+        // yiaddr.
+        } else {
+            msg->setRemoteAddr(question->getYiaddr());
+
+        }
+
+    // In most cases, we should have the remote address found already. If we
+    // found ourselves at this point, the rational thing to do is to respond
+    // to the address we got the query from.
+    } else {
+        msg->setRemoteAddr(question->getRemoteAddr());
+
+    }
+}
+
+
 OptionPtr
 Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
     uint32_t netmask = getNetmask4(subnet->get().second);

src/bin/dhcp4/dhcp4_srv.h

@@ -225,6 +225,23 @@ protected:
     /// @param msg_type specifies message type
     void appendDefaultOptions(Pkt4Ptr& msg, uint8_t msg_type);
 
+    /// @brief Sets remote addresses for outgoing packet.
+    ///
+    /// This method sets the local and remote addresses on outgoing packet.
+    /// The addresses being set depend on the following conditions:
+    /// - has incoming packet been relayed,
+    /// - is direct response to a client without address supported,
+    /// - type of the outgoing packet,
+    /// - broadcast flag set in the incoming packet.
+    ///
+    /// @warning This method does not check whether provided packet pointers
+    /// are valid. Make sure that pointers are correct before calling this
+    /// function.
+    ///
+    /// @param question instance of a packet received by a server.
+    /// @param [out] msg response packet which addresses are to be adjusted.
+    void adjustRemoteAddr(const Pkt4Ptr& question, Pkt4Ptr& msg);
+
     /// @brief Returns server-identifier option
     ///
     /// @return server-id option

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -72,6 +72,7 @@ public:
         : Dhcpv4Srv(port, "type=memfile", false, false) {
     }
 
+    using Dhcpv4Srv::adjustRemoteAddr;
     using Dhcpv4Srv::processDiscover;
     using Dhcpv4Srv::processRequest;
     using Dhcpv4Srv::processRelease;
@@ -450,7 +451,7 @@ public:
 
         }
 
-        if (relay_addr.toText() != "0.0.0.0") {
+        /*        if (relay_addr.toText() != "0.0.0.0") {
             // This is relayed message. It should be sent brsp to relay address.
             EXPECT_EQ(req->getGiaddr().toText(),
                       rsp->getRemoteAddr().toText());
@@ -475,7 +476,7 @@ public:
 
             }
 
-        }
+            } */
 
         messageCheck(req, rsp);
 
@@ -514,6 +515,28 @@ public:
 
     }
 
+    void testAdjustRemoteAddress(const uint8_t in_msg_type,
+                                 const uint8_t out_msg_type,
+                                 const std::string& giaddr,
+                                 const std::string& ciaddr,
+                                 const std::string& yiaddr,
+                                 const uint16_t flags) {
+        boost::scoped_ptr<NakedDhcpv4Srv> srv(new NakedDhcpv4Srv(0));
+
+        boost::shared_ptr<Pkt4> req(new Pkt4(in_msg_type, 1234));
+        req->setGiaddr(IOAddress(giaddr));
+        req->setCiaddr(IOAddress(ciaddr));
+        req->setFlags(flags);
+
+        boost::shared_ptr<Pkt4> resp(new Pkt4(out_msg_type, 1234));
+        resp->setYiaddr(IOAddress(yiaddr));
+
+        ASSERT_NO_THROW(srv->adjustRemoteAddr(req, resp));
+
+
+        
+    }
+
     ~Dhcpv4SrvTest() {
         CfgMgr::instance().deleteSubnets4();
 
@@ -558,6 +581,22 @@ TEST_F(Dhcpv4SrvTest, basic) {
     delete naked_srv;
 }
 
+TEST_F(Dhcpv4SrvTest, adjustRemoteAddressRelay) {
+    boost::scoped_ptr<NakedDhcpv4Srv> srv(new NakedDhcpv4Srv(0));
+
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+    req->setGiaddr(IOAddress("192.0.2.1"));
+    req->setCiaddr(IOAddress("0.0.0.0"));
+    req->setFlags(flags);
+    
+    boost::shared_ptr<Pkt4> resp(new Pkt4(OFFER, 1234));
+    resp->setYiaddr(IOAddress("192.0.2.100"));
+
+    ASSERT_NO_THROW(srv->adjustRemoteAddr(req, resp));
+
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+}
+
 // Verifies that DISCOVER received via relay can be processed correctly,
 // that the OFFER message generated in response is valid and
 // contains necessary options.