From: Marcin Siodelski Date: Mon, 3 Feb 2014 17:40:36 +0000 (+0100) Subject: [3242] Select subnet for renewing client using supplied ciaddr. X-Git-Tag: bind10-1.2.0beta1-release~63^2~9 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0b6ff4416ca1c990456d05d4a7d5417c523fdbc6;p=thirdparty%2Fkea.git [3242] Select subnet for renewing client using supplied ciaddr. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 297acfdc75..b2a8adb3e2 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/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. diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index c3376557dc..abd9405e2f 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/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()); } @@ -1551,6 +1553,51 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) { return (subnet); } +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 @@ -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; diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index ec63e7fcef..3649ce1eed 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/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. diff --git a/src/bin/dhcp4/tests/direct_client_unittest.cc b/src/bin/dhcp4/tests/direct_client_unittest.cc index 29d0f423ec..694c848fb4 100644 --- a/src/bin/dhcp4/tests/direct_client_unittest.cc +++ b/src/bin/dhcp4/tests/direct_client_unittest.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -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()); + +} }