Commit f22c490b authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3242] Addressed review comments.

parent 1b522d63
......@@ -135,6 +135,11 @@ A "libreload" command was issued to reload the hooks libraries but for
some reason the reload failed. Other error messages issued from the
hooks framework will indicate the nature of the problem.
% DHCP4_UNRECOGNIZED_RCVD_PACKET_TYPE received message (transaction id %1) has unregonized type %2
This debug message indicates that the message type carried in DHCPv4 option
53 is unrecognized by the server. The valid message types are available
on the IANA website. The message will not be processed by the server.
% DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3)
This debug message indicates that the server successfully advertised
a lease. It is up to the client to choose one server out of othe advertised
......
......@@ -266,7 +266,7 @@ Dhcpv4Srv::run() {
continue;
}
// We have sanity checked (in accept()) that the Message Type option
// 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)
......@@ -1573,15 +1573,12 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const {
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());
// Check that the message type is accepted by the server. We rely on the
// function called to log a message if needed.
if (!acceptMessageType(query)) {
return (false);
}
return (true);
}
......@@ -1595,11 +1592,51 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const {
return (false);
}
static const IOAddress bcast("255.255.255.255");
return ((pkt->getLocalAddr() == bcast && !selectSubnet(pkt)) ? false : true);
return ((pkt->getLocalAddr() != bcast || selectSubnet(pkt)));
}
bool
Dhcpv4Srv::acceptMessageType(const Pkt4Ptr& query) const {
// When receiving a packet without message type option, getType() will
// throw.
int type;
try {
type = query->getType();
} catch (...) {
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE)
.arg(query->getIface());
return (false);
}
// If we receive a message with a non-existing type, we are logging it.
if (type > DHCPLEASEQUERYDONE) {
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL,
DHCP4_UNRECOGNIZED_RCVD_PACKET_TYPE)
.arg(type)
.arg(query->getTransid());
return (false);
}
// Once we know that the message type is within a range of defined DHCPv4
// messages, we do a detailed check to make sure that the received message
// is targeted at server. Note that we could have received some Offer
// message broadcasted by the other server to a relay. Even though, the
// server would rather unicast its response to a relay, let's be on the
// safe side. Also, we want to drop other messages which we don't support.
// All these valid messages that we are not going to process are dropped
// silently.
if ((type != DHCPDISCOVER) && (type != DHCPREQUEST) &&
(type != DHCPRELEASE) && (type != DHCPDECLINE) &&
(type != DHCPINFORM)) {
return (false);
}
return (true);
}
bool
Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const {
Dhcpv4Srv::acceptServerId(const Pkt4Ptr& query) 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
// it here.
......@@ -1609,7 +1646,7 @@ Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const {
// Note that we don't check cases that server identifier is mandatory
// but not present. This is meant to be sanity checked in other
// functions.
OptionPtr option = pkt->getOption(DHO_DHCP_SERVER_IDENTIFIER);
OptionPtr option = query->getOption(DHO_DHCP_SERVER_IDENTIFIER);
if (!option) {
return (true);
}
......
......@@ -167,6 +167,13 @@ public:
protected:
/// @name Functions filtering and sanity-checking received messages.
///
/// @todo These functions are supposed to be moved to a new class which
/// will manage different rules for accepting and rejecting messages.
/// Perhaps ticket #3116 is a good opportunity to do it.
///
//@{
/// @brief Checks whether received message should be processed or discarded.
///
/// This function checks whether received message should be processed or
......@@ -211,11 +218,31 @@ protected:
/// - all broadcast messages received on the interface for which the
/// suitable subnet exists (is configured).
///
/// @param pkt Message sent by a client.
/// @param query Message sent by a client.
///
/// @return true if message is accepted for further processing, false
/// otherwise.
bool acceptDirectRequest(const Pkt4Ptr& pkt) const;
bool acceptDirectRequest(const Pkt4Ptr& query) const;
/// @brief Check if received message type is valid for the server to
/// process.
///
/// This function checks that the received message type belongs to the range
/// of types regonized by the server and that the message of this type
/// should be processed by the server.
///
/// The messages types accepted for processing are:
/// - Discover
/// - Request
/// - Release
/// - Decline
/// - Inform
///
/// @param query Message sent by a client.
///
/// @return true if message is accepted for further processing, false
/// otherwise.
bool acceptMessageType(const Pkt4Ptr& query) const;
/// @brief Verifies if the server id belongs to our server.
///
......@@ -231,6 +258,7 @@ protected:
/// @return true, if the server identifier is absent or matches one of the
/// server identifiers that the server is using; false otherwise.
bool acceptServerId(const Pkt4Ptr& pkt) const;
//@}
/// @brief verifies if specified packet meets RFC requirements
///
......
......@@ -3361,4 +3361,46 @@ TEST_F(Dhcpv4SrvTest, acceptDirectRequest) {
}
// This test checks that the server rejects a message with invalid type.
TEST_F(Dhcpv4SrvTest, acceptMessageType) {
IfaceMgrTestConfig test_config(true);
IfaceMgr::instance().openSockets4();
NakedDhcpv4Srv srv(0);
// Specify messages to be accepted by the server.
int allowed[] = {
DHCPDISCOVER,
DHCPREQUEST,
DHCPRELEASE,
DHCPDECLINE,
DHCPINFORM
};
size_t allowed_size = sizeof(allowed) / sizeof(allowed[0]);
// Check that the server actually accepts these message types.
for (int i = 0; i < allowed_size; ++i) {
EXPECT_TRUE(srv.acceptMessageType(Pkt4Ptr(new Pkt4(allowed[i], 1234))))
<< "Test failed for message type " << i;
}
// Specify messages which server is supposed to drop.
int not_allowed[] = {
DHCPOFFER,
DHCPACK,
DHCPNAK,
DHCPLEASEQUERY,
DHCPLEASEUNASSIGNED,
DHCPLEASEUNKNOWN,
DHCPLEASEACTIVE,
DHCPBULKLEASEQUERY,
DHCPLEASEQUERYDONE
};
size_t not_allowed_size = sizeof(not_allowed) / sizeof(not_allowed[0]);
// Actually check that the server will drop these messages.
for (int i = 0; i < not_allowed_size; ++i) {
EXPECT_FALSE(srv.acceptMessageType(Pkt4Ptr(new Pkt4(not_allowed[i],
1234))))
<< "Test failed for message type " << i;
}
}
}; // end of anonymous namespace
// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
//
// Permission to use, copy, modify, and/or distribute this software for any
// purpose with or without fee is hereby granted, provided that the above
......@@ -15,7 +15,9 @@
#include <config.h>
#include <asiolink/io_address.h>
#include <cc/data.h>
#include <config/ccsession.h>
#include <dhcp4/config_parser.h>
#include <dhcp4/tests/dhcp4_test_utils.h>
#include <dhcp/option4_addrlst.h>
#include <dhcp/option_int_array.h>
......@@ -29,14 +31,14 @@
using namespace std;
using namespace isc::asiolink;
using namespace isc::data;
namespace isc {
namespace dhcp {
namespace test {
Dhcpv4SrvTest::Dhcpv4SrvTest()
:rcode_(-1) {
:rcode_(-1), srv_(0) {
subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1000,
2000, 3000));
pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), IOAddress("192.0.2.110")));
......@@ -553,6 +555,20 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) {
EXPECT_TRUE(noBasicOptions(rsp));
}
void
Dhcpv4SrvTest::configure(const std::string& config) {
ElementPtr json = Element::fromJSON(config);
ConstElementPtr status;
// Configure the server and make sure the config is accepted
EXPECT_NO_THROW(status = configureDhcp4Server(srv_, json));
ASSERT_TRUE(status);
int rcode;
ConstElementPtr comment = config::parseAnswer(rcode, status);
ASSERT_EQ(0, rcode);
}
}; // end of isc::dhcp::test namespace
}; // end of isc::dhcp namespace
}; // end of isc namespace
......@@ -87,6 +87,120 @@ public:
typedef boost::shared_ptr<PktFilterTest> PktFilterTestPtr;
/// @brief "Naked" DHCPv4 server, exposes internal fields
class NakedDhcpv4Srv: public Dhcpv4Srv {
public:
/// @brief Constructor.
///
/// This constructor disables default modes of operation used by the
/// Dhcpv4Srv class:
/// - Send/receive broadcast messages through sockets on interfaces
/// which support broadcast traffic.
/// - Direct DHCPv4 traffic - communication with clients which do not
/// have IP address assigned yet.
///
/// Enabling these modes requires root privilges so they must be
/// disabled for unit testing.
///
/// Note, that disabling broadcast options on sockets does not impact
/// the operation of these tests because they use local loopback
/// interface which doesn't have broadcast capability anyway. It rather
/// prevents setting broadcast options on other (broadcast capable)
/// sockets which are opened on other interfaces in Dhcpv4Srv constructor.
///
/// The Direct DHCPv4 Traffic capability can be disabled here because
/// it is tested with PktFilterLPFTest unittest. The tests which belong
/// to PktFilterLPFTest can be enabled on demand when root privileges can
/// be guaranteed.
///
/// @param port port number to listen on; the default value 0 indicates
/// that sockets should not be opened.
NakedDhcpv4Srv(uint16_t port = 0)
: Dhcpv4Srv(port, "type=memfile", false, false) {
// Create fixed server id.
server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER,
asiolink::IOAddress("192.0.3.1")));
}
/// @brief Returns fixed server identifier assigned to the naked server
/// instance.
OptionPtr getServerID() const {
return (server_id_);
}
/// @brief fakes packet reception
/// @param timeout ignored
///
/// The method receives all packets queued in receive queue, one after
/// another. Once the queue is empty, it initiates the shutdown procedure.
///
/// See fake_received_ field for description
virtual Pkt4Ptr receivePacket(int /*timeout*/) {
// If there is anything prepared as fake incoming traffic, use it
if (!fake_received_.empty()) {
Pkt4Ptr pkt = fake_received_.front();
fake_received_.pop_front();
return (pkt);
}
// If not, just trigger shutdown and return immediately
shutdown();
return (Pkt4Ptr());
}
/// @brief fake packet sending
///
/// Pretend to send a packet, but instead just store it in fake_send_ list
/// where test can later inspect server's response.
virtual void sendPacket(const Pkt4Ptr& pkt) {
fake_sent_.push_back(pkt);
}
/// @brief adds a packet to fake receive queue
///
/// See fake_received_ field for description
void fakeReceive(const Pkt4Ptr& pkt) {
fake_received_.push_back(pkt);
}
virtual ~NakedDhcpv4Srv() {
}
/// @brief Dummy server identifier option used by various tests.
OptionPtr server_id_;
/// @brief packets we pretend to receive
///
/// Instead of setting up sockets on interfaces that change between OSes, it
/// is much easier to fake packet reception. This is a list of packets that
/// we pretend to have received. You can schedule new packets to be received
/// using fakeReceive() and NakedDhcpv4Srv::receivePacket() methods.
std::list<Pkt4Ptr> fake_received_;
std::list<Pkt4Ptr> fake_sent_;
using Dhcpv4Srv::adjustIfaceData;
using Dhcpv4Srv::appendServerID;
using Dhcpv4Srv::processDiscover;
using Dhcpv4Srv::processRequest;
using Dhcpv4Srv::processRelease;
using Dhcpv4Srv::processDecline;
using Dhcpv4Srv::processInform;
using Dhcpv4Srv::processClientName;
using Dhcpv4Srv::computeDhcid;
using Dhcpv4Srv::createNameChangeRequests;
using Dhcpv4Srv::acceptServerId;
using Dhcpv4Srv::sanityCheck;
using Dhcpv4Srv::srvidToString;
using Dhcpv4Srv::unpackOptions;
using Dhcpv4Srv::name_change_reqs_;
using Dhcpv4Srv::classifyPacket;
using Dhcpv4Srv::accept;
using Dhcpv4Srv::acceptMessageType;
};
class Dhcpv4SrvTest : public ::testing::Test {
public:
......@@ -280,6 +394,11 @@ public:
/// @param msg_type DHCPDISCOVER or DHCPREQUEST
void testDiscoverRequest(const uint8_t msg_type);
/// @brief Runs DHCPv4 configuration from the JSON string.
///
/// @param config String holding server configuration in JSON format.
void configure(const std::string& config);
/// @brief This function cleans up after the test.
virtual void TearDown();
......@@ -296,119 +415,9 @@ public:
isc::data::ConstElementPtr comment_;
};
/// @brief "Naked" DHCPv4 server, exposes internal fields
class NakedDhcpv4Srv: public Dhcpv4Srv {
public:
/// @brief Server object under test.
NakedDhcpv4Srv srv_;
/// @brief Constructor.
///
/// This constructor disables default modes of operation used by the
/// Dhcpv4Srv class:
/// - Send/receive broadcast messages through sockets on interfaces
/// which support broadcast traffic.
/// - Direct DHCPv4 traffic - communication with clients which do not
/// have IP address assigned yet.
///
/// Enabling these modes requires root privilges so they must be
/// disabled for unit testing.
///
/// Note, that disabling broadcast options on sockets does not impact
/// the operation of these tests because they use local loopback
/// interface which doesn't have broadcast capability anyway. It rather
/// prevents setting broadcast options on other (broadcast capable)
/// sockets which are opened on other interfaces in Dhcpv4Srv constructor.
///
/// The Direct DHCPv4 Traffic capability can be disabled here because
/// it is tested with PktFilterLPFTest unittest. The tests which belong
/// to PktFilterLPFTest can be enabled on demand when root privileges can
/// be guaranteed.
///
/// @param port port number to listen on; the default value 0 indicates
/// that sockets should not be opened.
NakedDhcpv4Srv(uint16_t port = 0)
: Dhcpv4Srv(port, "type=memfile", false, false) {
// Create fixed server id.
server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER,
asiolink::IOAddress("192.0.3.1")));
}
/// @brief Returns fixed server identifier assigned to the naked server
/// instance.
OptionPtr getServerID() const {
return (server_id_);
}
/// @brief fakes packet reception
/// @param timeout ignored
///
/// The method receives all packets queued in receive queue, one after
/// another. Once the queue is empty, it initiates the shutdown procedure.
///
/// See fake_received_ field for description
virtual Pkt4Ptr receivePacket(int /*timeout*/) {
// If there is anything prepared as fake incoming traffic, use it
if (!fake_received_.empty()) {
Pkt4Ptr pkt = fake_received_.front();
fake_received_.pop_front();
return (pkt);
}
// If not, just trigger shutdown and return immediately
shutdown();
return (Pkt4Ptr());
}
/// @brief fake packet sending
///
/// Pretend to send a packet, but instead just store it in fake_send_ list
/// where test can later inspect server's response.
virtual void sendPacket(const Pkt4Ptr& pkt) {
fake_sent_.push_back(pkt);
}
/// @brief adds a packet to fake receive queue
///
/// See fake_received_ field for description
void fakeReceive(const Pkt4Ptr& pkt) {
fake_received_.push_back(pkt);
}
virtual ~NakedDhcpv4Srv() {
}
/// @brief Dummy server identifier option used by various tests.
OptionPtr server_id_;
/// @brief packets we pretend to receive
///
/// Instead of setting up sockets on interfaces that change between OSes, it
/// is much easier to fake packet reception. This is a list of packets that
/// we pretend to have received. You can schedule new packets to be received
/// using fakeReceive() and NakedDhcpv4Srv::receivePacket() methods.
std::list<Pkt4Ptr> fake_received_;
std::list<Pkt4Ptr> fake_sent_;
using Dhcpv4Srv::adjustIfaceData;
using Dhcpv4Srv::appendServerID;
using Dhcpv4Srv::processDiscover;
using Dhcpv4Srv::processRequest;
using Dhcpv4Srv::processRelease;
using Dhcpv4Srv::processDecline;
using Dhcpv4Srv::processInform;
using Dhcpv4Srv::processClientName;
using Dhcpv4Srv::computeDhcid;
using Dhcpv4Srv::createNameChangeRequests;
using Dhcpv4Srv::acceptServerId;
using Dhcpv4Srv::sanityCheck;
using Dhcpv4Srv::srvidToString;
using Dhcpv4Srv::unpackOptions;
using Dhcpv4Srv::name_change_reqs_;
using Dhcpv4Srv::classifyPacket;
using Dhcpv4Srv::accept;
};
}; // end of isc::dhcp::test namespace
......
......@@ -12,7 +12,6 @@
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <cc/data.h>
#include <dhcp/iface_mgr.h>
#include <dhcp/pkt4.h>
#include <dhcp/tests/iface_mgr_test_config.h>
......@@ -50,7 +49,9 @@ public:
/// a specified prefix.
///
/// The subnet parameters (such as options, timers etc.) are aribitrarily
/// selected. The subnet and pool mask is always /24.
/// selected. The subnet and pool mask is always /24. The real configuration
/// would exclude .0 (network address) and .255 (broadcast address), but we
/// ignore that fact for the sake of test simplicity.
///
/// @param prefix Prefix for a subnet.
void configureSubnet(const std::string& prefix);
......@@ -59,7 +60,9 @@ public:
///
/// This function configures DHCPv4 server with two different subnets.
/// The subnet parameters (such as options, timers etc.) are aribitrarily
/// selected. The subnet and pool mask is /24.
/// selected. The subnet and pool mask is /24. The real configuration
/// would exclude .0 (network address) and .255 (broadcast address), but we
/// ignore that fact for the sake of test simplicity.
///
/// @param prefix1 Prefix of the first subnet to be configured.
/// @param prefix2 Prefix of the second subnet to be configured.
......@@ -98,18 +101,9 @@ public:
/// @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.
void configure(const std::string& config);
/// @brief Server object to be unit tested.
NakedDhcpv4Srv srv_;
};
DirectClientTest::DirectClientTest()
: srv_(0) {
DirectClientTest::DirectClientTest() : Dhcpv4SrvTest() {
}
void
......@@ -187,19 +181,6 @@ DirectClientTest::createClientMessage(const Pkt4Ptr& msg,
return (received);
}
void
DirectClientTest::configure(const std::string& config) {
ElementPtr json = Element::fromJSON(config);
ConstElementPtr status;
// Configure the server and make sure the config is accepted
EXPECT_NO_THROW(status = configureDhcp4Server(srv_, json));
ASSERT_TRUE(status);
int rcode;
ConstElementPtr comment = config::parseAnswer(rcode, status);
ASSERT_EQ(0, rcode);
}
// This test checks that the message from directly connected client
// is processed and that client is offered IPv4 address from the subnet which
// is suitable for the local interface on which the client's message is
......
/*
* Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC")
* Copyright (c) 2004-2011, 2014 by Internet Systems Consortium, Inc. ("ISC")
* Copyright (c) 1995-2003 by Internet Software Consortium
*
* Permission to use, copy, modify, and distribute this software for any
......@@ -154,7 +154,9 @@ enum DHCPMessageType {
DHCPLEASEQUERY = 10,
DHCPLEASEUNASSIGNED = 11,
DHCPLEASEUNKNOWN = 12,
DHCPLEASEACTIVE = 13
DHCPLEASEACTIVE = 13,
DHCPBULKLEASEQUERY = 14,
DHCPLEASEQUERYDONE = 15
};
static const uint16_t DHCP4_CLIENT_PORT = 68;
......
......@@ -396,6 +396,7 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) {
case DHCPRELEASE:
case DHCPINFORM:
case DHCPLEASEQUERY:
case DHCPBULKLEASEQUERY:
return (BOOTREQUEST);
case DHCPACK:
......@@ -404,6 +405,7 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) {
case DHCPLEASEUNASSIGNED:
case DHCPLEASEUNKNOWN:
case DHCPLEASEACTIVE:
case DHCPLEASEQUERYDONE:
return (BOOTREPLY);
default:
......
......@@ -25,6 +25,13 @@ TESTS_ENVIRONMENT = \
TESTS =