diff options
author | Marcin Siodelski <marcin@isc.org> | 2015-05-11 14:24:58 +0200 |
---|---|---|
committer | Marcin Siodelski <marcin@isc.org> | 2015-05-11 17:14:29 +0200 |
commit | e0bbec0e10b1539df28fa987a2b21326c0fff0e5 (patch) | |
tree | 2af01416be647a76f9cf571b151cd682d9928717 /src/lib | |
parent | [3747] Added method which checks if lease belongs to the client. (diff) | |
download | kea-e0bbec0e10b1539df28fa987a2b21326c0fff0e5.tar.xz kea-e0bbec0e10b1539df28fa987a2b21326c0fff0e5.zip |
[3747] Use client identifier over MAC to get existing client's lease.
Diffstat (limited to 'src/lib')
-rw-r--r-- | src/lib/dhcpsrv/alloc_engine.cc | 128 | ||||
-rw-r--r-- | src/lib/dhcpsrv/alloc_engine.h | 39 | ||||
-rw-r--r-- | src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc | 62 | ||||
-rw-r--r-- | src/lib/dhcpsrv/tests/alloc_engine_utils.cc | 2 | ||||
-rw-r--r-- | src/lib/dhcpsrv/tests/alloc_engine_utils.h | 1 |
5 files changed, 97 insertions, 135 deletions
diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index eabfc8e6fe..53d6cb0fe6 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1228,69 +1228,43 @@ hasAddressReservation(const AllocEngine::ClientContext4& ctx) { return (ctx.host_ && !ctx.host_->getIPv4Reservation().isV4Zero()); } -/// @brief Check if there is a lease for the client which message is processed. +/// @brief Finds existing lease in the database. /// -/// This function searches the lease database to find existing lease for the client. -/// It finds the lease using the client's HW address first. If the lease exists and -/// appears to belong to the client the lease is returned. Otherwise, the function -/// will search for the lease using the client identifier (if supplied). If the -/// lease exists and appears to belong to the client, it is returned. +/// This function searches for the lease in the database which belongs to the +/// client requesting allocation. If the client has supplied the client +/// identifier this identifier is used to look up the lease. If the lease is +/// not found using the client identifier, an additional lookup is performed +/// using the HW address, if supplied. If the lease is found using the HW +/// address, the function also checks if the lease belongs to the client, i.e. +/// there is no conflict between the client identifiers. /// -/// This function also identifies the conflicts between existing leases and the -/// lease to be allocated for the client, when the client is using a HW address -/// or client identifier which is already in use by the client having a lease in -/// the database. If the client uses an identifier which is already used by another -/// client and no other unique identifier which could be used to identify the client's -/// lease this function signals the conflict by returning 'true'. -/// -/// @param ctx Client context. -/// @param [out] client_lease Client's lease found in the database. -/// -/// @return true if there is a conflict of identifiers (HW address or client id) -/// between the client which message is being processed and the client which has -/// a lease in the database. When the value 'true' is returned, the caller should -/// cease the lease allocation for the client. -bool matchClientLease(const AllocEngine::ClientContext4& ctx, Lease4Ptr& client_lease) { - // Obtain the sole instance of the LeaseMgr. +/// @param ctx Context holding data extracted from the client's message, +/// including the HW address and client identifier. +/// @param [out] client_lease A pointer to the lease returned by this function +/// or null value if no has been lease found. +void findClientLease(const AllocEngine::ClientContext4& ctx, Lease4Ptr& client_lease) { LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); - - // The server should hand out existing lease to the client, so we have to check - // if there is one. First, try to use the client's HW address. - client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID()); - // If there is no lease for this HW address or the lease doesn't seem to be ours, - // we will have to use the client identifier. Note that in some situations two - // clients may use the same HW address so even if we find the lease for the HW - // address it doesn't mean it is ours, because client identifier may not match. - if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) { - // Check if the lease is in conflict with the lease that we want to allocate. - // If the lease is in conflict because of using overlapping HW address or - // client identifier, we can't allocate the lease for this client. - if (client_lease && ctx.isInConflict(*client_lease)) { - return (true); - } - // There is no lease or the lease we found is not conflicting with the lease - // which we have found for the HW address, so there is still a chance that - // we will allocate the lease. Check if there is a lease using the client - // identifier. + // If client identifier has been supplied, use it to lookup the lease. This + // search will return no lease if the client doesn't have any lease in the + // database or if the client didn't use client identifier to allocate the + // existing lease (this include cases when the server was explicitly + // configured to ignore client identifier). + if (ctx.clientid_) { client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID()); } - // Check if the lease we have found belongs to us. - if (client_lease && !ctx.myLease(*client_lease)) { - // If the lease doesn't belong to us, check if we can add new lease for - // the client which message we're processing, or its identifiers are - // in conflict with this lease. - if (ctx.isInConflict(*client_lease)) { - return (true); + // If no lease found using the client identifier, try the lookup using + // the HW address. + if (!client_lease && ctx.hwaddr_) { + client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID()); + // This lookup may return the lease which has conflicting client + // identifier and thus is considered to belong to someone else. + // If this is the case, we need to toss the result and force the + // Allocation Engine to allocate another lease. + if (client_lease && !client_lease->belongsToClient(ctx.hwaddr_, ctx.clientid_)) { + client_lease.reset(); } - // If there is no conflict we can proceed and try to find the appropriate - // lease but we don't use the one we found, because it is assigned to - // someone else. Reset the pointer to indicate that we're not - // renewing this lease. - client_lease.reset(); } - - return (false); } } // end of anonymous namespace @@ -1321,37 +1295,6 @@ AllocEngine::ClientContext4::ClientContext4(const Subnet4Ptr& subnet, fake_allocation_(fake_allocation), old_lease_(), host_() { } - - -bool -AllocEngine::ClientContext4::myLease(const Lease4& lease) const { - if ((!hwaddr_ && lease.hwaddr_) || (hwaddr_ && !lease.hwaddr_)) { - return (false); - } - - if ((hwaddr_ && lease.hwaddr_) && (hwaddr_->hwaddr_ != lease.hwaddr_->hwaddr_)) { - return (false); - } - - if ((!clientid_ && lease.client_id_) || (clientid_ && !lease.client_id_)) { - return (false); - } - - if ((clientid_ && lease.client_id_) && (*clientid_ != *lease.client_id_)) { - return (false); - } - - 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 @@ -1410,10 +1353,7 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // if there is a conflict with existing lease and the allocation should // not be continued. Lease4Ptr client_lease; - bool conflict = matchClientLease(ctx, client_lease); - if (conflict) { - return (Lease4Ptr()); - } + findClientLease(ctx, client_lease); // new_lease will hold the pointer to the lease that we will offer to the // caller. @@ -1499,10 +1439,7 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // if there is a conflict with existing lease and the allocation should // not be continued. Lease4Ptr client_lease; - bool conflict = matchClientLease(ctx, client_lease); - if (conflict) { - return (Lease4Ptr()); - } + findClientLease(ctx, client_lease); // Obtain the sole instance of the LeaseMgr. LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); @@ -1537,7 +1474,8 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { // if the address is in use by our client or another client. // If it is in use by another client, the address can't be // allocated. - if (existing && !existing->expired() && !ctx.myLease(*existing)) { + if (existing && !existing->expired() && + !existing->belongsToClient(ctx.hwaddr_, ctx.clientid_)) { return (Lease4Ptr()); } diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index cb012b9af0..a93ef6205b 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -760,45 +760,6 @@ public: const bool fwd_dns_update, const bool rev_dns_update, const std::string& hostname, const bool fake_allocation); - /// @brief Check if the specified lease belongs to the client. - /// - /// This method compares the hardware address and the client id - /// in the lease with the relevant values in the context. That - /// way the method determines whether the lease belongs to the - /// client which message the server is processing. - /// - /// @return true if the lease belongs to the client for which - /// the context has been created, false otherwise. - bool myLease(const Lease4& lease) const; - - /// @brief Check if the lease conflicts with the context. - /// - /// This method is used in cases when there is a lease in the lease database - /// for the client and the server receives the message from another client - /// which may be potentially using the same client identifier or HW address. - /// - /// Currently the allocation engine allows for allocating a lease for the - /// client which is using the same HW address or client identifier as the - /// client which already has a lease in the database. However, the value of - /// one of the identifiers must be unique. In other words, two clients may - /// have the same client identifier but different HW addresses and the - /// leases can be allocated for both (there is no conflict). - /// - /// The other possible case is that one of the clients uses both HW address - /// and client identifier, and another client is using only one of those - /// and its value is the same as for the former client. The allocation - /// engine treats it as a conflict because there is no unique value in the - /// lease database by which the server could distinguish the clients. - /// Hence, it doesn't allocate the lease from the context in conflict. - /// - /// This method detects the conflict described above. - /// - /// @param lease Existing lease to be checked. - /// - /// @return true if the lease is in conflict with the context, false - /// otherwise. - bool isInConflict(const Lease4& lease) const; - }; /// @brief Pointer to the @c ClientContext4. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 7935aa65dc..4c36408624 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -536,6 +536,65 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) { EXPECT_TRUE(*ctx.old_lease_ == original_lease); } +// This test checks that the Allocation Engine correcly identifies the +// existing client's lease in the lease database, using the client +// identifier and HW address. +TEST_F(AllocEngine4Test, identifyClientLease) { + Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, clientid_, + 100, 30, 60, time(NULL), subnet_->getID())); + LeaseMgrFactory::instance().addLease(lease); + + AllocEngine engine(AllocEngine::ALLOC_ITERATIVE, 100, false); + AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, + IOAddress::IPV4_ZERO_ADDRESS(), + false, false, "", true); + + Lease4Ptr identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + + ctx.hwaddr_ = hwaddr2_; + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + + ctx.hwaddr_ = hwaddr_; + ctx.clientid_ = clientid2_; + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); + + ctx.clientid_.reset(); + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + + ctx.hwaddr_ = hwaddr2_; + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); + + lease->client_id_.reset(); + ASSERT_NO_THROW(LeaseMgrFactory::instance().updateLease4(lease)); + + ctx.hwaddr_ = hwaddr_; + ctx.clientid_ = clientid_; + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + + ctx.clientid_.reset(); + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_EQ("192.0.2.101", identified_lease->addr_.toText()); + + ctx.clientid_ = clientid_; + ctx.hwaddr_ = hwaddr2_; + identified_lease = engine.allocateLease4(ctx); + ASSERT_TRUE(identified_lease); + EXPECT_NE(identified_lease->addr_.toText(), "192.0.2.101"); +} + // This test checks that when the client requests the address which belongs // to another client, the allocation engine returns NULL (for the // DHCPREQUEST case) or a lease for the address which belongs to this @@ -924,7 +983,7 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseInvalidHint) { CfgMgr::instance().commit(); // Create a lease for the client 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_, ClientIdPtr(), 100, 30, 60, time(NULL), subnet_->getID(), false, false, "")); LeaseMgrFactory::instance().addLease(lease); @@ -946,6 +1005,7 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseInvalidHint) { AllocEngine::ClientContext4 ctx2(subnet_, clientid_, hwaddr_, IOAddress("192.0.2.101"), false, false, "", false); + AllocEngine::findReservation(ctx2); allocated_lease = engine.allocateLease4(ctx2); // The client has reservation so the server wants to allocate a // reserved address and doesn't want to renew the address that the diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc index f0ff369ab6..1f20085dca 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc @@ -351,6 +351,8 @@ AllocEngine4Test::AllocEngine4Test() { HostMgr::instance().create(); clientid_ = ClientIdPtr(new ClientId(vector<uint8_t>(8, 0x44))); + clientid2_ = ClientIdPtr(new ClientId(vector<uint8_t>(8, 0x56))); + uint8_t mac[] = { 0, 1, 22, 33, 44, 55}; // Let's use odd hardware type to check if there is no Ethernet diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.h b/src/lib/dhcpsrv/tests/alloc_engine_utils.h index 80f159dad5..bc0728aa57 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.h +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.h @@ -383,6 +383,7 @@ public: } ClientIdPtr clientid_; ///< Client-identifier (value used in tests) + ClientIdPtr clientid2_; ///< Alternative client-identifier. HWAddrPtr hwaddr_; ///< Hardware address (value used in tests) HWAddrPtr hwaddr2_; ///< Alternative hardware address. Subnet4Ptr subnet_; ///< Subnet4 (used in tests) |