[3242] Select subnet for renewing client using supplied ciaddr.

src/bin/dhcp4/dhcp4_messages.mes

@@ -175,6 +175,13 @@ This warning message is issued when current server configuration specifies
 no interfaces that server should listen on, or specified interfaces are not
 configured to receive the traffic.
 
+% DHCP4_NO_SUBNET_FOR_DIRECT_CLIENT no suitable subnet configured for a direct client sending packet with transaction id %1, on interface %2, received message is dropped
+This info messsage is logged when received a message from a directly connected
+client but there is no suitable subnet configured for the interface on
+which this message has been received. The IPv4 address assigned on this
+interface must belong to one of the configured subnets. Otherwise
+received message is dropped.
+
 % DHCP4_NOT_RUNNING IPv4 DHCP server is not running
 A warning message is issued when an attempt is made to shut down the
 IPv4 DHCP server but it is not running.

src/bin/dhcp4/dhcp4_srv.cc

@@ -260,26 +260,15 @@ Dhcpv4Srv::run() {
             }
         }
 
-        // Check if the DHCPv4 packet has been sent to us or to someone else.
-        // If it hasn't been sent to us, drop it!
-        if (!acceptServerId(query)) {
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_NOT_FOR_US)
-                .arg(query->getTransid())
-                .arg(query->getIface());
-            continue;
-        }
-
-        // When receiving a packet without message type option, getType() will
-        // throw. Let's set type to -1 as default error indicator.
-        int type = -1;
-        try {
-            type = query->getType();
-        } catch (...) {
-            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE)
-                .arg(query->getIface());
+        // Check whether the message should be further processed or discarded.
+        // There is no need to log anything here. This function logs by itself.
+        if (!accept(query)) {
             continue;
         }
 
+        // We have sanity checked (in accept()) that the Message Type option
+        // exists, so we can safely get it here.
+        int type = query->getType();
         LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_RECEIVED)
             .arg(serverReceivedPacketName(type))
             .arg(type)
@@ -1499,22 +1488,35 @@ Dhcpv4Srv::serverReceivedPacketName(uint8_t type) {
 }
 
 Subnet4Ptr
-Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
+Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) const {
 
     Subnet4Ptr subnet;
-    // Is this relayed message?
-    IOAddress relay = question->getGiaddr();
     static const IOAddress notset("0.0.0.0");
+    static const IOAddress bcast("255.255.255.255");
 
-    if (relay != notset) {
-        // Yes: Use relay address to select subnet
-        subnet = CfgMgr::instance().getSubnet4(relay);
+    // If a message is relayed, use the relay (giaddr) address to select subnet
+    // for the client. Note that this may result in exception if the value
+    // of hops does not correspond with the Giaddr. Such message is considered
+    // to be malformed anyway and the message will be dropped by the higher
+    // level functions.
+    if (question->isRelayed()) {
+        subnet = CfgMgr::instance().getSubnet4(question->getGiaddr());
+
+    // The message is not relayed so it is sent directly by a client. But
+    // the client may be renewing its lease and in such case it unicasts
+    // its message to the server. Note that the unicast Request bypasses
+    // relays and the client may be in a different network, so we can't
+    // use IP address on the local interface to get the subnet. Instead,
+    // we rely on the client's address to get the subnet.
+    } else if ((question->getLocalAddr() != bcast) &&
+               (question->getCiaddr() != notset)) {
+        subnet = CfgMgr::instance().getSubnet4(question->getCiaddr());
+
+    // The message has been received from a directly connected client
+    // and this client appears to have no address. The IPv4 address
+    // assigned to the interface on which this message has been received,
+    // will be used to determine the subnet suitable for the client.
     } else {
-
-        // No: the message has been received from a directly connected client.
-        // The IPv4 address assigned to the interface on which this message
-        // has been received, will be used to determine the subnet suitable for
-        // a client.
         subnet = CfgMgr::instance().getSubnet4(question->getIface());
     }
 
@@ -1552,6 +1554,51 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
 }
 
 bool
+Dhcpv4Srv::accept(const Pkt4Ptr& query) const {
+    // Check if the message from directly connected client (if directly
+    // connected) should be dropped or processed.
+    if (!acceptDirectRequest(query)) {
+        LOG_INFO(dhcp4_logger, DHCP4_NO_SUBNET_FOR_DIRECT_CLIENT)
+            .arg(query->getTransid())
+            .arg(query->getIface());
+        return (false);
+    }
+
+    // Check if the DHCPv4 packet has been sent to us or to someone else.
+    // If it hasn't been sent to us, drop it!
+    if (!acceptServerId(query)) {
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_NOT_FOR_US)
+            .arg(query->getTransid())
+            .arg(query->getIface());
+        return (false);
+    }
+
+    // When receiving a packet without message type option, getType() will
+    // throw.
+    try {
+        query->getType();
+    } catch (...) {
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE)
+            .arg(query->getIface());
+        return (false);
+    }
+    return (true);
+}
+
+bool
+Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
+    try {
+        if (pkt->isRelayed()) {
+            return (true);
+        }
+    } catch (const Exception& ex) {
+        return (false);
+    }
+    static const IOAddress bcast("255.255.255.255");
+    return ((pkt->getLocalAddr() == bcast && !selectSubnet(pkt)) ? false : true);
+}
+
+bool
 Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const {
     // This function is meant to be called internally by the server class, so
     // we rely on the caller to sanity check the pointer and we don't check
@@ -1690,8 +1737,8 @@ Dhcpv4Srv::openActiveSockets(const uint16_t port,
 
 size_t
 Dhcpv4Srv::unpackOptions(const OptionBuffer& buf,
-                          const std::string& option_space,
-                          isc::dhcp::OptionCollection& options) {
+                         const std::string& option_space,
+                         isc::dhcp::OptionCollection& options) {
     size_t offset = 0;
 
     OptionDefContainer option_defs;

src/bin/dhcp4/dhcp4_srv.h

@@ -167,6 +167,56 @@ public:
 
 protected:
 
+    /// @brief Checks whether received message should be processed or discarded.
+    ///
+    /// This function checks whether received message should be processed or
+    /// discarded. It should be called on the beginning of message processing
+    /// (just after the message has been decoded). This message calls a number
+    /// of other functions which check whether message should be processed,
+    /// using different criteria.
+    ///
+    /// This function should be extended when new criteria for accepting
+    /// received message have to be implemented. This function is meant to
+    /// aggregate all early filtering checks on the received message. By having
+    /// a single function like this, we are avoiding bloat of the server's main
+    /// loop.
+    ///
+    /// @warning This function should remain exception safe.
+    ///
+    /// @param query Received message.
+    ///
+    /// @return true if the message should be further processed, or false if
+    /// the message should be discarded.
+    bool accept(const Pkt4Ptr& query) const;
+
+    /// @brief Check if a message sent by directly connected client should be
+    /// accepted or discared.
+    ///
+    /// This function checks if the received message is from directly connected
+    /// client. If it is, it checks that it should be processed or discarded.
+    ///
+    /// Note that this function doesn't validate all addresses being carried in
+    /// the message. The primary purpose of this function is to filter out
+    /// direct messages in the local network for which there is no suitable
+    /// subnet configured. For example, this function accepts unicast messages
+    /// because unicasts may be used by clients located in remote networks to
+    /// to renew existing leases. If their notion of address is wrong, the
+    /// server will have to sent a NAK, instead of dropping the message.
+    /// Detailed validation of such messages is performed at later stage of
+    /// processing.
+    ///
+    /// This function accepts the following messages:
+    /// - all valid relayed messages,
+    /// - all unicast messages,
+    /// - all broadcast messages received on the interface for which the
+    /// suitable subnet exists (is configured).
+    ///
+    /// @param pkt Message sent by a client.
+    ///
+    /// @return true if message is accepted for further processing, false
+    /// otherwise.
+    bool acceptDirectRequest(const Pkt4Ptr& pkt) const;
+
     /// @brief Verifies if the server id belongs to our server.
     ///
     /// This function checks if the server identifier carried in the specified
@@ -529,7 +579,7 @@ protected:
     ///
     /// @param question client's message
     /// @return selected subnet (or NULL if no suitable subnet was found)
-    isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& question);
+    isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& question) const;
 
     /// indicates if shutdown is in progress. Setting it to true will
     /// initiate server shutdown procedure.

src/bin/dhcp4/tests/direct_client_unittest.cc

@@ -17,6 +17,7 @@
 #include <dhcp/pkt4.h>
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcpsrv/cfgmgr.h>
+#include <dhcpsrv/lease_mgr_factory.h>
 #include <dhcpsrv/subnet.h>
 #include <dhcp4/config_parser.h>
 #include <dhcp4/tests/dhcp4_test_utils.h>
@@ -68,11 +69,11 @@ public:
     /// @brief Creates simple message from a client.
     ///
     /// This function creates a DHCPv4 message having a specified type
-    /// (e.g. Discover, Request) and sets the interface property of this
-    /// message. The interface property indicates on which interface
-    /// interface a message has been received. The interface is used by
-    /// the DHCPv4 server to determine the subnet from which the address
-    /// should be allocated for the client.
+    /// (e.g. Discover, Request) and sets some properties of this
+    /// message: client identifier, address and interface. The copy of
+    /// this message is then created by parsing wire data of the original
+    /// message. This simulates the case when the message is received and
+    /// parsed by the server.
     ///
     /// @param msg_type Type of the message to be created.
     /// @param iface Name of the interface on which the message has been
@@ -82,6 +83,21 @@ public:
     Pkt4Ptr createClientMessage(const uint16_t msg_type,
                                 const std::string& iface);
 
+    /// @brief Creates simple message from a client.
+    ///
+    /// This function configures a client's message by adding client identifier,
+    /// setting interface and addresses. The copy of this message is then
+    /// created by parsing wire data of the original message. This simulates the
+    /// case when the message is received and parsed by the server.
+    ///
+    /// @param msg Caller supplied message to be configured. This object must
+    /// not be NULL.
+    /// @param iface Name of the interface on which the message has been
+    /// "received" by the server.
+    ///
+    /// @return Configured and parsed message.
+    Pkt4Ptr createClientMessage(const Pkt4Ptr &msg, const std::string& iface);
+
     /// @brief Runs DHCPv4 configuration from the JSON string.
     ///
     /// @param config String holding server configuration in JSON format.
@@ -148,11 +164,16 @@ DirectClientTest:: createClientMessage(const uint16_t msg_type,
                                        const std::string& iface) {
     // Create a source packet.
     Pkt4Ptr msg = Pkt4Ptr(new Pkt4(msg_type, 1234));
+    return (createClientMessage(msg, iface));
+
+}
+
+Pkt4Ptr
+DirectClientTest::createClientMessage(const Pkt4Ptr& msg,
+                                      const std::string& iface) {
     msg->setRemoteAddr(IOAddress("255.255.255.255"));
     msg->addOption(generateClientId());
     msg->setIface(iface);
-    // Create on-wire format of this packet as it has been sent by the client.
-    msg->pack();
 
     // Create copy of this packet by parsing its wire data. Make sure that the
     // local and remote address are set like it was a message sent from the
@@ -238,9 +259,9 @@ TEST_F(DirectClientTest,  twoSubnets) {
 // through an interface for which the subnet has been configured. This
 // interface has IPv4 address assigned which belongs to this subnet.
 // This test also verifies that when the message is received through
-// the interface for which there is no suitable subnet, the NAK
-// is sent back to a client.
-TEST_F(DirectClientTest,  oneSubnet) {
+// the interface for which there is no suitable subnet, the message
+// is discarded.
+TEST_F(DirectClientTest, oneSubnet) {
     // Configure IfaceMgr with fake interfaces lo, eth0 and eth1.
     IfaceMgrTestConfig iface_config(true);
     // After creating interfaces we have to open sockets as it is required
@@ -259,11 +280,12 @@ TEST_F(DirectClientTest,  oneSubnet) {
     // Process clients' messages.
     srv_.run();
 
-    // Check that the server did send reposonses.
-    ASSERT_EQ(2, srv_.fake_sent_.size());
+    // Check that the server sent one response for the message received
+    // through eth0. The other client's message should be dicarded.
+    ASSERT_EQ(1, srv_.fake_sent_.size());
 
-    // Check the first response. The first Discover was sent via eth0
-    // for which the subnet has been configured.
+    // Check the response. The first Discover was sent via eth0 for which
+    // the subnet has been configured.
     Pkt4Ptr response = srv_.fake_sent_.front();
     ASSERT_TRUE(response);
     srv_.fake_sent_.pop_front();
@@ -277,16 +299,124 @@ TEST_F(DirectClientTest,  oneSubnet) {
     ASSERT_TRUE(subnet);
     EXPECT_EQ("10.0.0.0", subnet->get().first.toText());
 
-    // The second Discover was sent through eth1 for which no suitable
-    // subnet exists.
-    response = srv_.fake_sent_.front();
+}
+
+// This test verifies that the server uses ciaddr to select a subnet for a
+// client which renews its lease.
+TEST_F(DirectClientTest, renew) {
+    // Configure IfaceMgr with fake interfaces lo, eth0 and eth1.
+    IfaceMgrTestConfig iface_config(true);
+    // After creating interfaces we have to open sockets as it is required
+    // by the message processing code.
+    ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
+    // Add a subnet.
+    ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
+    // Make sure that the subnet has been really added. Also, the subnet
+    // will be needed to create a lease for a client.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("10.0.0.10"));
+    // Create a lease for a client that we will later renewed. By explicitly
+    // creating a lease we will get to know the lease parameters, such as
+    // leased address etc.
+    const uint8_t hwaddr[] = { 1, 2, 3, 4, 5, 6 };
+    Lease4Ptr lease(new Lease4(IOAddress("10.0.0.10"), hwaddr, sizeof(hwaddr),
+                               &generateClientId()->getData()[0],
+                               generateClientId()->getData().size(),
+                               100, 50, 75, time(NULL),
+                               subnet->getID()));
+    LeaseMgrFactory::instance().addLease(lease);
+
+    // Create a Request to renew client's lease. The renew request is unicast
+    // through eth1. Note, that in case of renewal the client unicasts its
+    // Request and sets the ciaddr. The server is supposed to use ciaddr to
+    // pick the subnet for the client. In order to make sure that the server
+    // uses ciaddr, we simulate reception of the packet through eth1, for which
+    // there is no subnet for directly connected clients.
+    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    req->setCiaddr(IOAddress("10.0.0.10"));
+    req = createClientMessage(req, "eth1");
+    req->setLocalAddr(IOAddress("10.0.0.1"));
+    req->setRemoteAddr(req->getCiaddr());
+
+    srv_.fakeReceive(req);
+
+    // Process clients' messages.
+    srv_.run();
+
+    // Check that the server did send reposonse.
+    ASSERT_EQ(1, srv_.fake_sent_.size());
+    Pkt4Ptr response = srv_.fake_sent_.front();
     ASSERT_TRUE(response);
 
-    // The client should receive NAK as there is no subnet configured
-    // for this network.
-    EXPECT_EQ(DHCPNAK, response->getType());
+    ASSERT_EQ(DHCPACK, response->getType());
+    // Check that the offered address belongs to the suitable subnet.
+    subnet = CfgMgr::instance().getSubnet4(response->getYiaddr());
+    ASSERT_TRUE(subnet);
+    EXPECT_EQ("10.0.0.0", subnet->get().first.toText());
 
 }
 
+// This test verifies that when a client in the Rebinding state broadcasts
+// a Request message through an interface for which a subnet is configured,
+// the server responds to this Request. It also verifies that when such a
+// Request is sent through the interface for which there is no subnet configured
+// the client's message is discarded.
+TEST_F(DirectClientTest, rebind) {
+    // Configure IfaceMgr with fake interfaces lo, eth0 and eth1.
+    IfaceMgrTestConfig iface_config(true);
+    // After creating interfaces we have to open sockets as it is required
+    // by the message processing code.
+    ASSERT_NO_THROW(IfaceMgr::instance().openSockets4());
+    // Add a subnet.
+    ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0"));
+    // Make sure that the subnet has been really added. Also, the subnet
+    // will be needed to create a lease for a client.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("10.0.0.10"));
+    // Create a lease, which will be later renewed. By explicitly creating a
+    // lease we will know the lease parameters, such as leased address etc.
+    const uint8_t hwaddr[] = { 1, 2, 3, 4, 5, 6 };
+    Lease4Ptr lease(new Lease4(IOAddress("10.0.0.10"), hwaddr, sizeof(hwaddr),
+                               &generateClientId()->getData()[0],
+                               generateClientId()->getData().size(),
+                               100, 50, 75, time(NULL),
+                               subnet->getID()));
+    LeaseMgrFactory::instance().addLease(lease);
+
+    // Broadcast Request through an interface for which there is no subnet
+    // configured. This messag should be discarded by the server.
+    Pkt4Ptr req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    req->setCiaddr(IOAddress("10.0.0.10"));
+    req = createClientMessage(req, "eth1");
+    req->setRemoteAddr(req->getCiaddr());
+
+    srv_.fakeReceive(req);
+
+    // Broadcast another Request through an interface for which there is
+    // a subnet configured. The server should generate a response.
+    req = Pkt4Ptr(new Pkt4(DHCPREQUEST, 5678));
+    req->setCiaddr(IOAddress("10.0.0.10"));
+    req = createClientMessage(req, "eth0");
+    req->setRemoteAddr(req->getCiaddr());
+
+    srv_.fakeReceive(req);
+
+    // Process clients' messages.
+    srv_.run();
+
+    // Check that the server did send exactly one reposonse.
+    ASSERT_EQ(1, srv_.fake_sent_.size());
+    Pkt4Ptr response = srv_.fake_sent_.front();
+    ASSERT_TRUE(response);
+
+    // Make sure that the server responsed with ACK, not NAK.
+    ASSERT_EQ(DHCPACK, response->getType());
+    // Make sure that the response is generated for the second Request
+    // (transmitted over eth0).
+    EXPECT_EQ(5678, response->getTransid());
+    // Check that the offered address belongs to the suitable subnet.
+    subnet = CfgMgr::instance().getSubnet4(response->getYiaddr());
+    ASSERT_TRUE(subnet);
+    EXPECT_EQ("10.0.0.0", subnet->get().first.toText());
+
+}
 
 }