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

[3768] Alloc engine allocats leases to clients with repeating client ids.

parent ba89c257
......@@ -262,7 +262,12 @@ Dhcp4Client::doRequest() {
void
Dhcp4Client::includeClientId(const std::string& clientid) {
clientid_ = ClientId::fromText(clientid);
if (clientid.empty()) {
clientid_.reset();
} else {
clientid_ = ClientId::fromText(clientid);
}
}
void
......@@ -410,7 +415,11 @@ Dhcp4Client::sendMsg(const Pkt4Ptr& msg) {
void
Dhcp4Client::setHWAddress(const std::string& hwaddr_str) {
hwaddr_.reset(new HWAddr(HWAddr::fromText(hwaddr_str)));
if (hwaddr_str.empty()) {
hwaddr_.reset();
} else {
hwaddr_.reset(new HWAddr(HWAddr::fromText(hwaddr_str)));
}
}
} // end of namespace isc::dhcp::test
......
......@@ -222,7 +222,8 @@ public:
/// The generated client id will be added to the client's messages to the
/// server.
///
/// @param clientid Client id in the textual format.
/// @param clientid Client id in the textual format. Use the empty client id
/// value to not include the client id.
void includeClientId(const std::string& clientid);
/// @brief Creates an instance of the Client FQDN option to be included
......@@ -293,7 +294,8 @@ public:
/// @brief Sets the explicit hardware address for the client.
///
/// @param hwaddr_str String representation of the HW address.
/// @param hwaddr_str String representation of the HW address. Use an
/// empty string to set the NULL hardware address.
void setHWAddress(const std::string& hwaddr_str);
/// @brief Sets the interface over which the messages should be sent.
......
......@@ -173,6 +173,16 @@ public:
IfaceMgr::instance().openSockets4();
}
void oneAllocationOverlapTest(const std::string& hwaddr_a,
const std::string& clientid_a,
const std::string& hwaddr_b,
const std::string& clientid_b);
void twoAllocationsOverlapTest(const std::string& hwaddr_a,
const std::string& clientid_a,
const std::string& hwaddr_b,
const std::string& clientid_b);
/// @brief Interface Manager's fake configuration control.
IfaceMgrTestConfig iface_mgr_test_config_;
......@@ -441,26 +451,142 @@ TEST_F(DORATest, ciaddr) {
EXPECT_EQ("0.0.0.0", resp->getCiaddr().toText());
}
TEST_F(DORATest, overlappingClientId) {
Dhcp4Client clientA(Dhcp4Client::SELECTING);
clientA.includeClientId("12:34");
configure(DORA_CONFIGS[0], *clientA.getServer());
ASSERT_NO_THROW(clientA.doDORA());
void
DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a,
const std::string& clientid_a,
const std::string& hwaddr_b,
const std::string& clientid_b) {
// Allocate a lease by client A using the 4-way exchange.
Dhcp4Client client_a(Dhcp4Client::SELECTING);
client_a.includeClientId(clientid_a);
client_a.setHWAddress(hwaddr_a);
configure(DORA_CONFIGS[0], *client_a.getServer());
ASSERT_NO_THROW(client_a.doDORA());
// Make sure that the server responded.
ASSERT_TRUE(clientA.getContext().response_);
Pkt4Ptr respA = clientA.getContext().response_;
ASSERT_TRUE(client_a.getContext().response_);
Pkt4Ptr resp_a = client_a.getContext().response_;
// Make sure that the server has responded with DHCPACK.
ASSERT_EQ(DHCPACK, static_cast<int>(resp_a->getType()));
Lease4Ptr lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
ASSERT_TRUE(lease_a);
// Use the same client identifier by client B.
Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING);
client_b.setHWAddress(hwaddr_b);
client_b.includeClientId(clientid_b);
// Send DHCPDISCOVER and expect the response.
ASSERT_NO_THROW(client_b.doDiscover());
Pkt4Ptr resp_b = client_b.getContext().response_;
// Make sure that the server has responded with DHCPOFFER.
ASSERT_EQ(DHCPOFFER, static_cast<int>(resp_b->getType()));
// The offered address should be different than the address which
// was obtained by the client A.
ASSERT_NE(resp_b->getYiaddr(), client_a.config_.lease_.addr_);
lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
ASSERT_TRUE(lease_a);
// Now that we know that the server will avoid assigning the same
// address that the client A has, use the 4-way exchange to actually
// allocate some address.
ASSERT_NO_THROW(client_b.doDORA());
// Make sure that the server responded.
ASSERT_TRUE(client_b.getContext().response_);
resp_b = client_b.getContext().response_;
// Make sure that the server has responded with DHCPACK.
ASSERT_EQ(DHCPACK, static_cast<int>(respA->getType()));
Dhcp4Client clientB(clientA.getServer(), Dhcp4Client::SELECTING);
clientB.includeClientId("12:34");
ASSERT_NO_THROW(clientB.doDiscover());
ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
// Again, make sure the assigned addresses are different.
ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_);
lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
ASSERT_TRUE(lease_a);
Lease4Ptr lease_b = LeaseMgrFactory::instance().getLease4(client_b.config_.lease_.addr_);
ASSERT_TRUE(lease_b);
// Client B should be able to renew its address.
client_b.setState(Dhcp4Client::RENEWING);
ASSERT_NO_THROW(client_b.doRequest());
ASSERT_TRUE(client_b.getContext().response_);
resp_b = client_b.getContext().response_;
ASSERT_EQ(DHCPACK, static_cast<int>(resp_b->getType()));
ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_);
}
Pkt4Ptr respB = clientB.getContext().response_;
// Make sure that the server has responded with DHCPOFFER.
ASSERT_EQ(DHCPOFFER, static_cast<int>(respB->getType()));
void
DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a,
const std::string& clientid_a,
const std::string& hwaddr_b,
const std::string& clientid_b) {
// Allocate a lease by client A using the 4-way exchange.
Dhcp4Client client_a(Dhcp4Client::SELECTING);
client_a.includeClientId(clientid_a);
client_a.setHWAddress(hwaddr_a);
configure(DORA_CONFIGS[0], *client_a.getServer());
ASSERT_NO_THROW(client_a.doDORA());
// Make sure that the server responded.
ASSERT_TRUE(client_a.getContext().response_);
Pkt4Ptr resp_a = client_a.getContext().response_;
// Make sure that the server has responded with DHCPACK.
ASSERT_EQ(DHCPACK, static_cast<int>(resp_a->getType()));
Lease4Ptr lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_);
ASSERT_TRUE(lease_a);
// Client B sends a DHCPDISCOVER.
Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING);
client_b.setHWAddress(hwaddr_b);
client_b.includeClientId(clientid_b);
// Send DHCPDISCOVER and expect the response.
ASSERT_NO_THROW(client_b.doDiscover());
Pkt4Ptr resp_b = client_b.getContext().response_;
ASSERT_FALSE(resp_b);
client_b.config_.lease_.addr_ = IOAddress::IPV4_ZERO_ADDRESS();
client_b.setState(Dhcp4Client::INIT_REBOOT);
ASSERT_NO_THROW(client_b.doRequest());
resp_b = client_b.getContext().response_;
ASSERT_TRUE(resp_b);
ASSERT_EQ(DHCPNAK, static_cast<int>(resp_b->getType()));
}
// This test checks the server behavior in the following situation:
// - Client A identifies itself to the server using client identifier
// and the hardware address and requests allocation of the new lease.
// - Server allocates the lease to the client.
// - Client B has a different hardware address but is using the same
// client identifier as Client A.
// - Client B sends DHCPDISCOVER.
// - Server should determine that the client B is not client A, because
// it is using a different hadrware address, even though they use the
// same client identifier. As a consequence, the server should offer
// a different address to the client B.
// - The client B performs the 4-way exchange again and the server
// allocates a new address to the client, which should be different
// than the address used by the client A.
// - Client B is in the renewing state and it successfully renews its
// address.
// - Client B goes to INIT_REBOOT state and asks for the address used
// by the client A.
// - The server sends DHCPNAK to refuse the allocation of this address
// to the client B.
TEST_F(DORATest, twoAllocationsOverlap1) {
twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34",
"02:02:03:03:04:04", "12:34");
}
TEST_F(DORATest, twoAllocationsOverlap2) {
twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34",
"01:02:03:04:05:06", "22:34");
}
TEST_F(DORATest, oneAllocationOverlap1) {
oneAllocationOverlapTest("01:02:03:04:05:06", "12:34",
"01:02:03:04:05:06", "");
}
EXPECT_NE(clientA.config_.lease_.addr_, respB->getYiaddr());
TEST_F(DORATest, oneAllocationOverlap2) {
oneAllocationOverlapTest("01:02:03:04:05:06", "",
"01:02:03:04:05:06", "12:34");
}
// This is a simple test for the host reservation. It creates a reservation
......
......@@ -1286,6 +1286,14 @@ AllocEngine::ClientContext4::myLease(const Lease4& lease) const {
return (true);
}
bool
AllocEngine::ClientContext4::isInConflict(const Lease4& lease) const {
return ((!(hwaddr_ && lease.hwaddr_) && (clientid_ && lease.client_id_) &&
(*clientid_ == *lease.client_id_)) ||
(!(clientid_ && lease.client_id_) && (hwaddr_ && lease.hwaddr_) &&
(hwaddr_->hwaddr_ == lease.hwaddr_->hwaddr_)));
}
Lease4Ptr
AllocEngine::allocateLease4(ClientContext4& ctx) {
// The NULL pointer indicates that the old lease didn't exist. It may
......@@ -1347,10 +1355,20 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) {
// to either return this lease to the client or to return it as an old
// (existing) lease if a different one is offered.
Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID());
if (!client_lease && ctx.clientid_) {
if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) {
if (client_lease && ctx.isInConflict(*client_lease)) {
return (Lease4Ptr());
}
client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID());
}
if (client_lease && !ctx.myLease(*client_lease)) {
if (ctx.isInConflict(*client_lease)) {
return (Lease4Ptr());
}
client_lease.reset();
}
// new_lease will hold the pointer to the lease that we will offer to the
// caller.
Lease4Ptr new_lease;
......@@ -1435,13 +1453,17 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) {
LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
// Check if the client has any lease already. This information is needed
// to either return this lease to the client or to delete this lease if
// the new lease is allocated.
// to either return this lease to the client or to return it as an old
// (existing) lease if a different one is offered.
Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID());
if (!client_lease && ctx.clientid_) {
if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) {
client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID());
}
if (client_lease && !ctx.myLease(*client_lease)) {
client_lease.reset();
}
// When the client sends the DHCPREQUEST, it should always specify the
// address which it is requesting or renewing. That is, the client should
// either use the requested IP address option or set the ciaddr. However,
......
......@@ -765,6 +765,8 @@ public:
/// the context has been created, false otherwise.
bool myLease(const Lease4& lease) const;
bool isInConflict(const Lease4& lease) const;
};
/// @brief Pointer to the @c ClientContext4.
......
......@@ -542,11 +542,17 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) {
// client (DHCPDISCOVER case).
TEST_F(AllocEngine4Test, requestOtherClientLease) {
// Create the first lease.
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
&clientid_->getClientId()[0],
clientid_->getClientId().size(),
100, 30, 60, time(NULL), subnet_->getID(),
false, false, ""));
// Create the second lease.
Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, 0, 0,
// Create the second lease. Note that we use the same client id here and
// we expect that the allocation engine will figure out that the hardware
// address is different.
Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_,
&clientid_->getClientId()[0],
clientid_->getClientId().size(),
100, 30, 60, time(NULL), subnet_->getID(),
false, false, ""));
// Add leases for both clients to the Lease Manager.
......@@ -967,7 +973,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseFakeAllocation) {
CfgMgr::instance().commit();
// Create a lease for a different address than reserved.
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
&clientid_->getClientId()[0],
clientid_->getClientId().size(),
100, 30, 60, time(NULL), subnet_->getID(),
false, false, ""));
LeaseMgrFactory::instance().addLease(lease);
......@@ -1077,7 +1085,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) {
CfgMgr::instance().commit();
// Create a lease for a different address than reserved.
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0,
Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_,
&clientid_->getClientId()[0],
clientid_->getClientId().size(),
100, 30, 60, time(NULL), subnet_->getID(),
false, false, ""));
LeaseMgrFactory::instance().addLease(lease);
......
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