Commit 83d5b633 authored by Thomas Markwalder's avatar Thomas Markwalder

[#821,!501] kea-dhcp4 now sanity checks inbound messages

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::processRequest()
    Dhcpv4Srv::processRelease()
    Dhcpv4Srv::processDecline()
    Dhcpv4Srv::processInform() - now all call sanityCheck()

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
    TEST_F(Dhcpv4SrvTest, sanityCheckDiscover)
    TEST_F(Dhcpv4SrvTest, sanityCheckRequest)
    TEST_F(Dhcpv4SrvTest, sanityCheckDecline)
    TEST_F(Dhcpv4SrvTest, sanityCheckRelease)
    TEST_F(Dhcpv4SrvTest, sanityCheckInform) - new tests

src/lib/testutils/gtest_utils.h
    New file with handy new test macros:
        EXPECT_THROW_MSG()
        ASSERT_THROW_MSG()

src/lib/testutils/Makefile.am
    Added new file gtest_utils.h

Added a ChangeLog entry
parent 8d8edede
1661. [bug] tmark
kea-dhcp4 now rejects inbound client messages that have
neither a hardware address nor a client identifier.
(Gitlab #821,!501, git TBD)
1660. [func] franek
Statistics of the DHCP packets are now initialized upon the
server startup. This makes the statistics available for fetching
......
......@@ -2661,6 +2661,7 @@ Dhcpv4Srv::getNetmaskOption(const Subnet4Ptr& subnet) {
Pkt4Ptr
Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
// server-id is forbidden.
sanityCheck(discover, FORBIDDEN);
bool drop = false;
......@@ -2720,8 +2721,9 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) {
Pkt4Ptr
Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& context) {
/// @todo Uncomment this (see ticket #3116)
/// sanityCheck(request, MANDATORY);
// Since we cannot distinguish between client states
// we'll make server-id is optional for REQUESTs.
sanityCheck(request, OPTIONAL);
bool drop = false;
Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request, drop));
......@@ -2782,8 +2784,9 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request, AllocEngine::ClientContext4Ptr& cont
void
Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& context) {
/// @todo Uncomment this (see ticket #3116)
/// sanityCheck(release, MANDATORY);
// Server-id is mandatory in DHCPRELEASE (see table 5, RFC2131)
// but ISC DHCP does not enforce this, so we'll follow suit.
sanityCheck(release, OPTIONAL);
// Try to find client-id. Note that for the DHCPRELEASE we don't check if the
// match-client-id configuration parameter is disabled because this parameter
......@@ -2897,10 +2900,9 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release, AllocEngine::ClientContext4Ptr& cont
void
Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& context) {
// Server-id is mandatory in DHCPDECLINE (see table 5, RFC2131)
/// @todo Uncomment this (see ticket #3116)
// sanityCheck(decline, MANDATORY);
// but ISC DHCP does not enforce this, so we'll follow suit.
sanityCheck(decline, OPTIONAL);
// Client is supposed to specify the address being declined in
// Requested IP address option, but must not set its ciaddr.
......@@ -2911,7 +2913,7 @@ Dhcpv4Srv::processDecline(Pkt4Ptr& decline, AllocEngine::ClientContext4Ptr& cont
if (!opt_requested_address) {
isc_throw(RFCViolation, "Mandatory 'Requested IP address' option missing"
"in DHCPDECLINE sent from " << decline->getLabel());
" in DHCPDECLINE sent from " << decline->getLabel());
}
IOAddress addr(opt_requested_address->readAddress());
......@@ -3047,8 +3049,9 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline,
Pkt4Ptr
Dhcpv4Srv::processInform(Pkt4Ptr& inform) {
// DHCPINFORM MUST not include server identifier.
sanityCheck(inform, FORBIDDEN);
// server-id is supposed to be forbidden (as is requested address)
// but ISC DHCP does not enforce either. So neither will we.
sanityCheck(inform, OPTIONAL);
bool drop = false;
Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform, drop));
......@@ -3319,15 +3322,15 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) {
switch (serverid) {
case FORBIDDEN:
if (server_id) {
isc_throw(RFCViolation, "Server-id option was not expected, but "
<< "received in "
isc_throw(RFCViolation, "Server-id option was not expected, but"
<< " received in message "
<< query->getName());
}
break;
case MANDATORY:
if (!server_id) {
isc_throw(RFCViolation, "Server-id option was expected, but not "
isc_throw(RFCViolation, "Server-id option was expected, but not"
" received in message "
<< query->getName());
}
......@@ -3349,7 +3352,7 @@ Dhcpv4Srv::sanityCheck(const Pkt4Ptr& query, RequirementLevel serverid) {
// If there's no client-id (or a useless one is provided, i.e. 0 length)
if (!client_id || client_id->len() == client_id->getHeaderLen()) {
isc_throw(RFCViolation, "Missing or useless client-id and no HW address "
isc_throw(RFCViolation, "Missing or useless client-id and no HW address"
" provided in message "
<< query->getName());
}
......
......@@ -32,8 +32,8 @@
#include <dhcpsrv/lease_mgr_factory.h>
#include <dhcpsrv/utils.h>
#include <dhcpsrv/host_mgr.h>
#include <gtest/gtest.h>
#include <stats/stats_mgr.h>
#include <testutils/gtest_utils.h>
#include <util/encode/hex.h>
#include <boost/scoped_ptr.hpp>
......@@ -717,12 +717,212 @@ TEST_F(Dhcpv4SrvTest, processRequest) {
testDiscoverRequest(DHCPREQUEST);
}
TEST_F(Dhcpv4SrvTest, processRelease) {
// Verifies that DHCPDISCOVERs are sanity checked correctly.
// 1. They must have either hardware address or client id
// 2. They must not have server id
TEST_F(Dhcpv4SrvTest, sanityCheckDiscover) {
NakedDhcpv4Srv srv;
Pkt4Ptr pkt(new Pkt4(DHCPDISCOVER, 1234));
// Should throw, no hardware address or client id
ASSERT_THROW_MSG(srv.processDiscover(pkt), RFCViolation,
"Missing or useless client-id and no HW address"
" provided in message DHCPDISCOVER");
// Add a hardware address. This should not throw.
uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER));
pkt->setHWAddr(hwaddr);
ASSERT_NO_THROW(srv.processDiscover(pkt));
// Now let's make a new pkt with client-id only, it should not throw.
pkt.reset(new Pkt4(DHCPDISCOVER, 1234));
pkt->addOption(generateClientId());
ASSERT_NO_THROW(srv.processDiscover(pkt));
// Now let's add a server-id. This should throw.
OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_SERVER_IDENTIFIER);
ASSERT_TRUE(server_id_def);
OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4));
server_id->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(server_id);
EXPECT_THROW_MSG(srv.processDiscover(pkt), RFCViolation,
"Server-id option was not expected,"
" but received in message DHCPDISCOVER");
}
// Verifies that DHCPREQEUSTs are sanity checked correctly.
// 1. They must have either hardware address or client id
// 2. They must have a requested address
// 3. They may or may not have a server id
TEST_F(Dhcpv4SrvTest, sanityCheckRequest) {
NakedDhcpv4Srv srv;
Pkt4Ptr pkt(new Pkt4(DHCPREQUEST, 1234));
// Should throw, no hardware address or client id
ASSERT_THROW_MSG(srv.processRequest(pkt), RFCViolation,
"Missing or useless client-id and no HW address"
" provided in message DHCPREQUEST");
// Add a hardware address. Should not throw.
uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER));
pkt->setHWAddr(hwaddr);
EXPECT_NO_THROW(srv.processRequest(pkt));
// Now let's add a requested address. This should not throw.
OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_REQUESTED_ADDRESS);
ASSERT_TRUE(req_addr_def);
OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4));
req_addr->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processRequest(pkt));
// Now let's make a new pkt with client-id only and an address, it should not throw.
pkt.reset(new Pkt4(DHCPREQUEST, 1234));
pkt->addOption(generateClientId());
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processRequest(pkt));
// Now let's add a server-id. This should not throw.
OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_SERVER_IDENTIFIER);
ASSERT_TRUE(server_id_def);
OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4));
server_id->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(server_id);
EXPECT_NO_THROW(srv.processRequest(pkt));
}
// Verifies that DHCPDECLINEs are sanity checked correctly.
// 1. They must have either hardware address or client id
// 2. They must have a requested address
// 3. They may or may not have a server id
TEST_F(Dhcpv4SrvTest, sanityCheckDecline) {
NakedDhcpv4Srv srv;
Pkt4Ptr pkt(new Pkt4(DHCPDECLINE, 1234));
// Should throw, no hardware address or client id
ASSERT_THROW_MSG(srv.processDecline(pkt), RFCViolation,
"Missing or useless client-id and no HW address"
" provided in message DHCPDECLINE");
// Add a hardware address. Should throw because of missing address.
uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER));
pkt->setHWAddr(hwaddr);
ASSERT_THROW_MSG(srv.processDecline(pkt), RFCViolation,
"Mandatory 'Requested IP address' option missing in DHCPDECLINE"
" sent from [hwtype=1 00:fe:fe:fe:fe:fe], cid=[no info], tid=0x4d2");
// Now let's add a requested address. This should not throw.
OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_REQUESTED_ADDRESS);
ASSERT_TRUE(req_addr_def);
OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4));
req_addr->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processDecline(pkt));
// Now let's make a new pkt with client-id only and an address, it should not throw.
pkt.reset(new Pkt4(DHCPDECLINE, 1234));
pkt->addOption(generateClientId());
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processDecline(pkt));
// Now let's add a server-id. This should not throw.
OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_SERVER_IDENTIFIER);
ASSERT_TRUE(server_id_def);
OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4));
server_id->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(server_id);
EXPECT_NO_THROW(srv.processDecline(pkt));
}
// Verifies that DHCPRELEASEs are sanity checked correctly.
// 1. They must have either hardware address or client id
// 2. They may or may not have a server id
TEST_F(Dhcpv4SrvTest, sanityCheckRelease) {
NakedDhcpv4Srv srv;
Pkt4Ptr pkt(new Pkt4(DHCPRELEASE, 1234));
// Should not throw
// Should throw, no hardware address or client id
ASSERT_THROW_MSG(srv.processRelease(pkt), RFCViolation,
"Missing or useless client-id and no HW address"
" provided in message DHCPRELEASE");
// Add a hardware address. Should not throw.
uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER));
pkt->setHWAddr(hwaddr);
EXPECT_NO_THROW(srv.processRelease(pkt));
// Make a new pkt with client-id only. Should not throw.
pkt.reset(new Pkt4(DHCPRELEASE, 1234));
pkt->addOption(generateClientId());
ASSERT_NO_THROW(srv.processRelease(pkt));
// Now let's add a server-id. This should not throw.
OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_SERVER_IDENTIFIER);
ASSERT_TRUE(server_id_def);
OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4));
server_id->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(server_id);
EXPECT_NO_THROW(srv.processRelease(pkt));
}
// Verifies that DHCPINFORMs are sanity checked correctly.
// 1. They must have either hardware address or client id
// 2. They may or may not have requested address
// 3. They may or may not have a server id
TEST_F(Dhcpv4SrvTest, sanityCheckInform) {
NakedDhcpv4Srv srv;
Pkt4Ptr pkt(new Pkt4(DHCPINFORM, 1234));
// Should throw, no hardware address or client id
ASSERT_THROW_MSG(srv.processInform(pkt), RFCViolation,
"Missing or useless client-id and no HW address"
" provided in message DHCPINFORM");
// Add a hardware address. Should not throw.
uint8_t data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(data, sizeof(data), HTYPE_ETHER));
pkt->setHWAddr(hwaddr);
ASSERT_NO_THROW(srv.processInform(pkt));
// Now let's add a requested address. This should not throw.
OptionDefinitionPtr req_addr_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_REQUESTED_ADDRESS);
ASSERT_TRUE(req_addr_def);
OptionCustomPtr req_addr(new OptionCustom(*req_addr_def, Option::V4));
req_addr->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processInform(pkt));
// Now let's make a new pkt with client-id only and an address, it should not throw.
pkt.reset(new Pkt4(DHCPINFORM, 1234));
pkt->addOption(generateClientId());
pkt->addOption(req_addr);
ASSERT_NO_THROW(srv.processInform(pkt));
// Now let's add a server-id. This should not throw.
OptionDefinitionPtr server_id_def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
DHO_DHCP_SERVER_IDENTIFIER);
ASSERT_TRUE(server_id_def);
OptionCustomPtr server_id(new OptionCustom(*server_id_def, Option::V4));
server_id->writeAddress(IOAddress("192.0.2.3"));
pkt->addOption(server_id);
EXPECT_NO_THROW(srv.processInform(pkt));
}
// This test verifies that incoming DISCOVER can be handled properly, that an
......
......@@ -14,6 +14,7 @@ libkea_testutils_la_SOURCES += test_to_element.cc test_to_element.h
libkea_testutils_la_SOURCES += threaded_test.cc threaded_test.h
libkea_testutils_la_SOURCES += unix_control_client.cc unix_control_client.h
libkea_testutils_la_SOURCES += user_context_utils.cc user_context_utils.h
libkea_testutils_la_SOURCES += gtest_utils.h
libkea_testutils_la_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
libkea_testutils_la_LIBADD = $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libkea_testutils_la_LIBADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
......
// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.
#ifndef GTEST_UTIL_H
#define GTEST_UTIL_H
#include <gtest/gtest.h>
namespace isc {
namespace test {
/// @brief Verifies an expected exception type and message
///
/// If the statement does not generate the expected exception
/// containing the expected message it will generate a non-fatal
/// failure.
///
/// @param statement - statement block to execute
/// @param etype - type of exception expected
/// @param emsg - exact content expected to be returned by ex.what()
#define EXPECT_THROW_MSG(statement,etype,emsg) \
{ \
try { \
statement; \
ADD_FAILURE() << "no exception, expected:" << #etype; \
} catch (const etype& ex) { \
EXPECT_EQ(std::string(ex.what()), emsg); \
} catch (...) { \
ADD_FAILURE() << "wrong exception type thrown, expected " << #etype; \
} \
} \
/// @brief Verifies an expected exception type and message
///
/// If the statement does not generate the expected exception
/// containing the expected message it will generate a fatal
/// failure.
///
/// @param statement - statement block to execute
/// @param etype - type of exception expected
/// @param emsg - exact content expected to be returned by ex.what()
#define ASSERT_THROW_MSG(statement,etype,emsg) \
{ \
try { \
statement; \
GTEST_FAIL() << "no exception, expected:" << #etype; \
} catch (const etype& ex) { \
ASSERT_EQ(std::string(ex.what()), emsg); \
} catch (...) { \
GTEST_FAIL() << "wrong exception type thrown, expected " << #etype; \
} \
} \
}; // end of isc::test namespace
}; // end of isc namespace
#endif // GTEST_UTIL_H
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment