Skip to content
GitLab
Commit 94949363 authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3747] Modification to how server identifies the client for Release. 

This change comes along with the new tests for DHCPRELEASE in the
separate file, and having a separate test fixture.
parent cc0c0eaf
Hide whitespace changes
Inline Side-by-side
src/bin/dhcp4/dhcp4_messages.mes
View file @ 94949363
......@@ -349,20 +349,6 @@ removed from the database during RELEASE message processing.
This warning message is printed when client attempts to release a lease,
but no such lease is known to the server.
% DHCP4_RELEASE_FAIL_WRONG_CLIENT_ID client (client-id %2) tried to release address %1, but it belongs to client (client-id %3)
This warning message indicates that client tried to release an address
that belongs to a different client. This should not happen in normal
circumstances and may indicate a misconfiguration of the client. However,
since the client releasing the address will stop using it anyway, there
is a good chance that the situation will correct itself.
% DHCP4_RELEASE_FAIL_WRONG_HWADDR client (client-id %2) tried to release address %1, but sent from a wrong hardware address (%3)
This warning message indicates that client tried to release an address
that does belong to it, but the lease information was associated with
a different hardware address. One possible reason for using different
hardware address is that a cloned virtual machine was not updated and
both clones use the same client-id.
% DHCP4_REQUEST_CLASS_PROCESSING_FAILED client class specific processing failed for DHCPREQUEST
This debug message means that the server processing that is unique for each
client class has reported a failure. The response packet will not be sent.
......
src/bin/dhcp4/dhcp4_srv.cc
View file @ 94949363
......@@ -1579,26 +1579,9 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
return;
}
// Does the hardware address match? We don't want one client releasing
// second client's leases. Note that we're comparing the hardware
// addresses only, not hardware types or sources of the hardware
// addresses. Thus we don't use HWAddr::equals().
if (lease->hwaddr_->hwaddr_ != release->getHWAddr()->hwaddr_) {
/// @todo: Print hwaddr from lease as part of ticket #2589
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_HWADDR)
.arg(release->getCiaddr().toText())
.arg(client_id ? client_id->toText() : "(no client-id)")
.arg(release->getHWAddr()->toText());
return;
}
// Does the lease have client-id info? If it has, then check it with what
// the client sent us.
if (lease->client_id_ && client_id && *lease->client_id_ != *client_id) {
LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_RELEASE_FAIL_WRONG_CLIENT_ID)
.arg(release->getCiaddr().toText())
.arg(client_id->toText())
.arg(lease->client_id_->toText());
if (!lease->belongsToClient(release->getHWAddr(), client_id)) {
/// @todo add debug message here when we merge the code with the
/// changes from #3806.
return;
}
......
src/bin/dhcp4/dhcp4_srv.h
View file @ 94949363
......@@ -397,7 +397,7 @@ protected:
/// @return ACK or NAK message
Pkt4Ptr processRequest(Pkt4Ptr& request);
/// @brief Stub function that will handle incoming RELEASE messages.
/// @brief Processes incoming DHCPRELEASE messages.
///
/// In DHCPv4, server does not respond to RELEASE messages, therefore
/// this function does not return anything.
......@@ -410,9 +410,11 @@ protected:
/// @param decline message received from client
void processDecline(Pkt4Ptr& decline);
/// @brief Stub function that will handle incoming INFORM messages.
/// @brief Processes incoming DHCPINFORM messages.
///
/// @param inform message received from client
///
/// @return DHCPACK to be sent to the client.
Pkt4Ptr processInform(Pkt4Ptr& inform);
/// @brief Appends options requested by client.
......
src/bin/dhcp4/tests/Makefile.am
View file @ 94949363
......@@ -87,6 +87,7 @@ dhcp4_unittests_SOURCES += marker_file.cc
dhcp4_unittests_SOURCES += dhcp4_client.cc dhcp4_client.h
dhcp4_unittests_SOURCES += inform_unittest.cc
dhcp4_unittests_SOURCES += dora_unittest.cc
dhcp4_unittests_SOURCES += release_unittest.cc
if CONFIG_BACKEND_BUNDY
# For Bundy backend, we only need to run the usual tests. There are no
......
src/bin/dhcp4/tests/dhcp4_client.cc
View file @ 94949363
......@@ -253,6 +253,25 @@ Dhcp4Client::doInform(const bool set_ciaddr) {
}
}
void
Dhcp4Client::doRelease() {
if (config_.lease_.addr_ == IOAddress::IPV4_ZERO_ADDRESS()) {
isc_throw(Dhcp4ClientError, "failed to send the release"
" message because client doesn't have a lease");
}
context_.query_ = createMsg(DHCPRELEASE);
// Set ciaddr to the address which we want to release.
context_.query_->setCiaddr(config_.lease_.addr_);
// Include client identifier.
appendClientId();
// Remove configuration.
config_.reset();
// Send the message to the server.
sendMsg(context_.query_);
}
void
Dhcp4Client::doRequest() {
context_.query_ = createMsg(DHCPREQUEST);
......
src/bin/dhcp4/tests/dhcp4_client.h
View file @ 94949363
......@@ -166,6 +166,12 @@ public:
/// an error occurs.
void doInform(const bool set_ciaddr = true);
/// @brief Sends DHCPRELEASE Message to the server.
///
/// This method simulates sending the DHCPRELEASE message to the server.
/// The released lease is removed from the client's configuration.
void doRelease();
/// @brief Sends DHCPREQUEST Message to the server and receives a response.
///
/// This method simulates sending the DHCPREQUEST message to the server and
......
src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
View file @ 94949363
......@@ -943,169 +943,6 @@ TEST_F(Dhcpv4SrvTest, sanityCheck) {
RFCViolation);
}
// This test verifies that incoming (positive) RELEASE can be handled properly.
// As there is no REPLY in DHCPv4, the only thing to verify here is that
// the lease is indeed removed from the database.
TEST_F(Dhcpv4SrvTest, ReleaseBasic) {
boost::scoped_ptr<NakedDhcpv4Srv> srv;
ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0)));
const IOAddress addr("192.0.2.106");
const uint32_t temp_t1 = 50;
const uint32_t temp_t2 = 75;
const uint32_t temp_valid = 100;
const time_t temp_timestamp = time(NULL) - 10;
// Generate client-id also duid_
OptionPtr clientid = generateClientId();
// Check that the address we are about to use is indeed in pool
ASSERT_TRUE(subnet_->inPool(Lease::TYPE_V4, addr));
// Let's create a lease and put it in the LeaseMgr
uint8_t hwaddr_data[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr hwaddr(new HWAddr(hwaddr_data, sizeof(hwaddr_data), HTYPE_ETHER));
Lease4Ptr used(new Lease4(addr, hwaddr,
&client_id_->getDuid()[0], client_id_->getDuid().size(),
temp_valid, temp_t1, temp_t2, temp_timestamp,
subnet_->getID()));
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
// Check that the lease is really in the database
Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
ASSERT_TRUE(l);
// Let's create a RELEASE
// Generate client-id also duid_
Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
rel->setRemoteAddr(addr);
rel->setCiaddr(addr);
rel->addOption(clientid);
rel->addOption(srv->getServerID());
rel->setHWAddr(hwaddr);
rel->setIface("eth0");
// Pass it to the server and hope for a REPLY
// Note: this is no response to RELEASE in DHCPv4
EXPECT_NO_THROW(srv->processRelease(rel));
// The lease should be gone from LeaseMgr
l = LeaseMgrFactory::instance().getLease4(addr);
EXPECT_FALSE(l);
// Try to get the lease by hardware address
Lease4Collection leases = LeaseMgrFactory::instance().getLease4(*hwaddr);
EXPECT_EQ(leases.size(), 0);
// Try to get it by hw/subnet_id combination
l = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet_->getID());
EXPECT_FALSE(l);
// Try by client-id
leases = LeaseMgrFactory::instance().getLease4(*client_id_);
EXPECT_EQ(leases.size(), 0);
// Try by client-id/subnet-id
l = LeaseMgrFactory::instance().getLease4(*client_id_, subnet_->getID());
EXPECT_FALSE(l);
// Ok, the lease is *really* not there.
}
// This test verifies that incoming (invalid) RELEASE can be handled properly.
//
// This test checks 3 scenarios:
// 1. there is no such lease at all
// 2. there is such a lease, but it is assigned to a different IAID
// 3. there is such a lease, but it belongs to a different client
TEST_F(Dhcpv4SrvTest, ReleaseReject) {
boost::scoped_ptr<NakedDhcpv4Srv> srv;
ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0)));
const IOAddress addr("192.0.2.106");
const uint32_t t1 = 50;
const uint32_t t2 = 75;
const uint32_t valid = 100;
const time_t timestamp = time(NULL) - 10;
// Let's create a lease and put it in the LeaseMgr
uint8_t bogus_mac_addr[] = { 0, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe};
HWAddrPtr bogus_hw(new HWAddr(bogus_mac_addr, sizeof(bogus_mac_addr), HTYPE_ETHER));
OptionPtr bogus_clientid = generateClientId(7); // different length
// Generate client-id also duid_
OptionPtr clientid = generateClientId();
// Check that the address we are about to use is indeed in pool
ASSERT_TRUE(subnet_->inPool(Lease::TYPE_V4, addr));
// Let's create a RELEASE
// Generate client-id also duid_
Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
rel->setRemoteAddr(addr);
rel->setCiaddr(addr);
rel->addOption(clientid);
rel->addOption(srv->getServerID());
rel->setHWAddr(bogus_hw);
rel->setIface("eth0");
// Case 1: No lease known to server
SCOPED_TRACE("CASE 1: Lease is not known to the server");
// There is nothing to check here. The lease is not there and server does
// not send anything back. This case is enumerated here just for keeping
// parity with similar test in DHCPv6.
EXPECT_NO_THROW(srv->processRelease(rel));
// CASE 2: Lease is known and belongs to this client, but to a different hardware
SCOPED_TRACE("CASE 2: Lease is known and belongs to this client, but uses different HW addr");
// Let's create a lease and put it in the LeaseMgr
uint8_t mac_addr[] = { 0, 0x1, 0x2, 0x3, 0x4, 0x5};
HWAddrPtr hw(new HWAddr(mac_addr, sizeof(mac_addr), HTYPE_ETHER));
Lease4Ptr used(new Lease4(addr, hw,
&client_id_->getDuid()[0], client_id_->getDuid().size(),
valid, t1, t2, timestamp, subnet_->getID()));
ASSERT_TRUE(LeaseMgrFactory::instance().addLease(used));
// Check that the lease is really in the database
Lease4Ptr l = LeaseMgrFactory::instance().getLease4(addr);
ASSERT_TRUE(l);
rel->setHWAddr(bogus_hw);
EXPECT_NO_THROW(srv->processRelease(rel));
// Check that the lease was not removed (due to hardware address mis-match)
l = LeaseMgrFactory::instance().getLease4(addr);
ASSERT_TRUE(l);
// CASE 3: Lease belongs to a client with different client-id
SCOPED_TRACE("CASE 3: Lease belongs to a client with different client-id");
rel->setHWAddr(hw); // proper HW address this time
rel->delOption(DHO_DHCP_CLIENT_IDENTIFIER);
rel->addOption(bogus_clientid); // but invalid client-id
OptionPtr x = rel->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
EXPECT_NO_THROW(srv->processRelease(rel));
// Check that the lease is still there
l = LeaseMgrFactory::instance().getLease4(addr);
ASSERT_TRUE(l);
// Final sanity check. Verify that with valid hw and client-id release is successful
rel->delOption(DHO_DHCP_CLIENT_IDENTIFIER);
rel->addOption(clientid);
// It should work this time
EXPECT_NO_THROW(srv->processRelease(rel));
// Check that the lease is not there
l = LeaseMgrFactory::instance().getLease4(addr);
EXPECT_FALSE(l);
}
// Checks if received relay agent info option is echoed back to the client
TEST_F(Dhcpv4SrvTest, relayAgentInfoEcho) {
IfaceMgrTestConfig test_config(true);
......
src/bin/dhcp4/tests/release_unittest.cc 0 → 100644
View file @ 94949363
// Copyright (C) 2015 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
// copyright notice and this permission notice appear in all copies.
//
// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <config.h>
#include <asiolink/io_address.h>
#include <cc/data.h>
#include <dhcp/dhcp4.h>
#include <dhcp/tests/iface_mgr_test_config.h>
#include <dhcpsrv/cfgmgr.h>
#include <dhcpsrv/subnet_id.h>
#include <dhcp4/tests/dhcp4_test_utils.h>
#include <dhcp4/tests/dhcp4_client.h>
#include <boost/shared_ptr.hpp>
using namespace isc;
using namespace isc::asiolink;
using namespace isc::data;
using namespace isc::dhcp;
using namespace isc::dhcp::test;
namespace {
/// @brief Set of JSON configurations used throughout the Release tests.
///
/// - Configuration 0:
/// - Used for testing Release message processing
/// - 1 subnet: 10.0.0.0/24
/// - 1 pool: 10.0.0.10-10.0.0.100
/// - Router option present: 10.0.0.200 and 10.0.0.201
const char* RELEASE_CONFIGS[] = {
// Configuration 0
"{ \"interfaces-config\": {"
" \"interfaces\": [ \"*\" ]"
"},"
"\"valid-lifetime\": 600,"
"\"subnet4\": [ { "
" \"subnet\": \"10.0.0.0/24\", "
" \"id\": 1,"
" \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
" \"option-data\": [ {"
" \"name\": \"routers\","
" \"code\": 3,"
" \"data\": \"10.0.0.200,10.0.0.201\","
" \"csv-format\": true,"
" \"space\": \"dhcp4\""
" } ]"
" } ]"
"}",
// Configuration 1
"{ \"interfaces-config\": {"
" \"interfaces\": [ \"*\" ]"
"},"
"\"valid-lifetime\": 600,"
"\"subnet4\": [ { "
" \"subnet\": \"192.0.2.0/24\", "
" \"option-data\": [ {"
" \"name\": \"routers\","
" \"code\": 3,"
" \"data\": \"192.0.2.200,192.0.2.201\","
" \"csv-format\": true,"
" \"space\": \"dhcp4\""
" },"
" {"
" \"name\": \"domain-name-servers\","
" \"code\": 6,"
" \"data\": \"192.0.2.202,192.0.2.203\","
" \"csv-format\": true,"
" \"space\": \"dhcp4\""
" },"
" {"
" \"name\": \"log-servers\","
" \"code\": 7,"
" \"data\": \"10.0.0.200,10.0.0.201\","
" \"csv-format\": true,"
" \"space\": \"dhcp4\""
" },"
" {"
" \"name\": \"cookie-servers\","
" \"code\": 8,"
" \"data\": \"10.0.0.202,10.0.0.203\","
" \"csv-format\": true,"
" \"space\": \"dhcp4\""
" } ]"
" } ]"
"}",
// Configuration 2
"{ \"interfaces-config\": {"
" \"interfaces\": [ \"*\" ]"
"},"
"\"valid-lifetime\": 600,"
"\"subnet4\": [ { "
" \"subnet\": \"10.0.0.0/24\", "
" \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
" \"reservations\": [ "
" {"
" \"hw-address\": \"aa:bb:cc:dd:ee:ff\","
" \"ip-address\": \"10.0.0.7\""
" }"
" ]"
"} ]"
"}"
};
/// @brief Test fixture class for testing 4-way (DORA) exchanges.
///
/// @todo Currently there is a limit number of test cases covered here.
/// In the future it is planned that the tests from the
/// dhcp4_srv_unittest.cc will be migrated here and will use the
/// @c Dhcp4Client class.
class ReleaseTest : public Dhcpv4SrvTest {
public:
/// @brief Constructor.
///
/// Sets up fake interfaces.
ReleaseTest()
: Dhcpv4SrvTest(),
iface_mgr_test_config_(true) {
IfaceMgr::instance().openSockets4();
}
/// @brief Performs 4-way exchange to obtain new lease.
///
/// @param client Client to be used to obtain a lease.
void acquireLease(Dhcp4Client& client);
/// @brief Test successful release for different identifiers.
///
/// @param hw_address_1 HW Address to be used to acquire the lease.
/// @param client_id_1 Client id to be used to acquire the lease.
/// @param hw_address_2 HW Address to be used to release the lease.
/// @param client_id_2 Client id to be used to release the lease.
void successfulRelease(const std::string& hw_address_1,
const std::string& client_id_1,
const std::string& hw_address_2,
const std::string& client_id_2);
/// @brief Test unsuccessful release for different identifiers.
///
/// @param hw_address_1 HW Address to be used to acquire the lease.
/// @param client_id_1 Client id to be used to acquire the lease.
/// @param hw_address_2 HW Address to be used to release the lease.
/// @param client_id_2 Client id to be used to release the lease.
void unsuccessfulRelease(const std::string& hw_address_1,
const std::string& client_id_1,
const std::string& hw_address_2,
const std::string& client_id_2);
/// @brief Interface Manager's fake configuration control.
IfaceMgrTestConfig iface_mgr_test_config_;
};
void
ReleaseTest::acquireLease(Dhcp4Client& client) {
// Perform 4-way exchange with the server but to not request any
// specific address in the DHCPDISCOVER message.
ASSERT_NO_THROW(client.doDORA());
// Make sure that the server responded.
ASSERT_TRUE(client.getContext().response_);
Pkt4Ptr resp = client.getContext().response_;
// Make sure that the server has responded with DHCPACK.
ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
// Response must not be relayed.
EXPECT_FALSE(resp->isRelayed());
// Make sure that the server id is present.
EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText());
// Make sure that the client has got the lease with the requested address.
ASSERT_NE(client.config_.lease_.addr_.toText(), "0.0.0.0");
Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(client.config_.lease_.addr_);
ASSERT_TRUE(lease);
}
void
ReleaseTest::successfulRelease(const std::string& hw_address_1,
const std::string& client_id_1,
const std::string& hw_address_2,
const std::string& client_id_2) {
Dhcp4Client client(Dhcp4Client::SELECTING);
// Configure DHCP server.
configure(RELEASE_CONFIGS[0], *client.getServer());
// Explicitly set the client id.
client.includeClientId(client_id_1);
// Explicitly set the HW address.
client.setHWAddress(hw_address_1);
// Perform 4-way exchange to obtain a new lease.
acquireLease(client);
// Remember the acquired address.
IOAddress leased_address = client.config_.lease_.addr_;
// Explicitly set the client id for DHCPRELEASE.
client.includeClientId(client_id_2);
// Explicitly set the HW address for DHCPRELEASE.
client.setHWAddress(hw_address_2);
// Send the release and make sure that the lease is removed from the
// server.
ASSERT_NO_THROW(client.doRelease());
Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(leased_address);
ASSERT_FALSE(lease);
}
void
ReleaseTest::unsuccessfulRelease(const std::string& hw_address_1,
const std::string& client_id_1,
const std::string& hw_address_2,
const std::string& client_id_2) {
Dhcp4Client client(Dhcp4Client::SELECTING);
// Configure DHCP server.
configure(RELEASE_CONFIGS[0], *client.getServer());
// Explicitly set the client id.
client.includeClientId(client_id_1);
// Explicitly set the HW address.
client.setHWAddress(hw_address_1);
// Perform 4-way exchange to obtain a new lease.
acquireLease(client);
// Remember the acquired address.
IOAddress leased_address = client.config_.lease_.addr_;
// Explicitly set the client id for DHCPRELEASE.
client.includeClientId(client_id_2);
// Explicitly set the HW address for DHCPRELEASE.
client.setHWAddress(hw_address_2);
// Send DHCPRELEASE and make sure it was unsuccessful, i.e. the lease
// remains in the database.
ASSERT_NO_THROW(client.doRelease());
Lease4Ptr lease = LeaseMgrFactory::instance().getLease4(leased_address);
ASSERT_TRUE(lease);
}
// This test checks that the client can acquire and release the lease.
TEST_F(ReleaseTest, releaseNoIdentifierChange) {
successfulRelease("01:02:03:04:05:06", "12:14",
"01:02:03:04:05:06", "12:14");
}
// This test verifies the release correctness in the following case:
// - Client acquires new lease using HW address only
// - Client sends the DHCPRELEASE with valid HW address and without
// client identifier.
// - The server successfully releases the lease.
TEST_F(ReleaseTest, releaseHWAddressOnly) {
successfulRelease("01:02:03:04:05:06", "",
"01:02:03:04:05:06", "");
}
// This test verifies the release correctness in the following case:
// - Client acquires new lease using the client identifier and HW address
// - Client sends the DHCPRELEASE with valid HW address but with no
// client identifier.
// - The server successfully releases the lease.
TEST_F(ReleaseTest, releaseNoClientId) {
successfulRelease("01:02:03:04:05:06", "12:14",
"01:02:03:04:05:06", "");
}
// This test verifies the release correctness in the following case:
// - Client acquires new lease using HW address
// - Client sends the DHCPRELEASE with valid HW address and some
// client identifier.
// - The server identifies the lease using HW address and releases
// this lease.
TEST_F(ReleaseTest, releaseNoClientId2) {
successfulRelease("01:02:03:04:05:06", "",
"01:02:03:04:05:06", "12:14");
}
// This test checks the server's behavior in the following case:
// - Client acquires new lease using the client identifier and HW address
// - Client sends the DHCPRELEASE with the valid HW address but with invalid
// client identifier.
// - The server should not remove the lease.
TEST_F(ReleaseTest, releaseNonMatchingClientId) {
unsuccessfulRelease("01:02:03:04:05:06", "12:14",
"01:02:03:04:05:06", "12:15:16");
}
// This test checks the server's behavior in the following case:
// - Client acquires new lease using client identifier and HW address
// - Client sends the DHCPRELEASE with the same client identifier but
// different HW address.
// - The server uses client identifier to find the client's lease and
// releases it.
TEST_F(ReleaseTest, releaseNonMatchingHWAddress) {
successfulRelease("01:02:03:04:05:06", "12:14",
"06:06:06:06:06:06", "12:14");
}
// This test checks the server's behavior in the following case:
// - Client acquires new lease.
// - Client sends DHCPRELEASE with the ciaddr set to a different
// address than it has acquired from the server.