From: Francis Dupont Date: Tue, 14 Mar 2023 10:17:04 +0000 (+0100) Subject: [#2766] Fixed advertise with zero lifetimes X-Git-Tag: Kea-2.3.6~72 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=262057546d2e50f0cfafb0bffbd647e59942bd3e;p=thirdparty%2Fkea.git [#2766] Fixed advertise with zero lifetimes --- diff --git a/ChangeLog b/ChangeLog index a318aad0dc..746e0afbe0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2111. [bug] fdupont + Fixed a bug which advertised re-assigned released leases + with zero lifetimes. + (Gitlab #2766) + 2110. [func] fdupont A new boolean configuration flag called "never-send" has been added to the option data scope. When enabled, the option is not diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 2c63c5623c..64c974b1b5 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2073,6 +2073,34 @@ TEST_F(Dhcpv6SrvTest, ReleaseBasicNoDelete) { IOAddress("2001:db8:1:1::cafe:babe"), LEASE_AFFINITY_ENABLED); } +// This test verifies that after a RELEASE which expired a lease the response +// to a SOLICIT does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, ReleaseBasicNoDeleteSolicit) { + testReleaseNoDelete(Lease::TYPE_NA, IOAddress("2001:db8:1:1::cafe:babe"), + DHCPV6_SOLICIT); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a REQUEST does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, ReleaseBasicNoDeleteRequest) { + testReleaseNoDelete(Lease::TYPE_NA, IOAddress("2001:db8:1:1::cafe:babe"), + DHCPV6_REQUEST); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a RENEW does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, ReleaseBasicNoDeleteRenew) { + testReleaseNoDelete(Lease::TYPE_NA, IOAddress("2001:db8:1:1::cafe:babe"), + DHCPV6_REQUEST); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a REBIND does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, ReleaseBasicNoDeleteRebind) { + testReleaseNoDelete(Lease::TYPE_NA, IOAddress("2001:db8:1:1::cafe:babe"), + DHCPV6_REBIND); +} + // This test verifies that incoming (positive) RELEASE with prefix can be // handled properly, that a REPLY is generated, that the response has // status code and that the lease is indeed removed from the database. @@ -2103,6 +2131,34 @@ TEST_F(Dhcpv6SrvTest, pdReleaseBasicNoDelete) { IOAddress("2001:db8:1:2::"), LEASE_AFFINITY_ENABLED); } +// This test verifies that after a RELEASE which expired a lease the response +// to a SOLICIT does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, pdReleaseBasicNoDeleteSolicit) { + testReleaseNoDelete(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"), + DHCPV6_REQUEST); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a REQUEST does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, pdReleaseBasicNoDeleteRequest) { + testReleaseNoDelete(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"), + DHCPV6_REQUEST); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a RENEW does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, pdReleaseBasicNoDeleteRenew) { + testReleaseNoDelete(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"), + DHCPV6_REQUEST); +} + +// This test verifies that after a RELEASE which expired a lease the response +// to a REBIND does not get zero lifetimes. +TEST_F(Dhcpv6SrvTest, pdReleaseBasicNoDeleteRebind) { + testReleaseNoDelete(Lease::TYPE_PD, IOAddress("2001:db8:1:2::"), + DHCPV6_REBIND); +} + // This test verifies that incoming (invalid) RELEASE with an address // can be handled properly. // diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index bf78974d58..5749e0129f 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -675,6 +675,125 @@ Dhcpv6SrvTest::testReleaseBasic(Lease::Type type, const IOAddress& existing, } } +void +Dhcpv6SrvTest::testReleaseNoDelete(Lease::Type type, const IOAddress& addr, + uint8_t qtype) { + NakedDhcpv6Srv srv(0); + + const uint32_t iaid = 234; + + uint8_t prefix_len; + if (type == Lease::TYPE_NA) { + prefix_len = 128; + } else { + prefix_len = pd_pool_->getLength(); + } + + // 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(type, addr)); + + // Let's prepopulate the database + Lease6Ptr lease(new Lease6(type, addr, duid_, iaid, + 501, 502, subnet_->getID(), + HWAddrPtr(), prefix_len)); + ASSERT_TRUE(LeaseMgrFactory::instance().addLease(lease)); + + // Check that the lease is really in the database + Lease6Ptr l = LeaseMgrFactory::instance().getLease6(type, addr); + ASSERT_TRUE(l); + + // Let's create a RELEASE + Pkt6Ptr rel = createMessage(DHCPV6_RELEASE, type, addr, prefix_len, iaid); + rel->addOption(clientid); + rel->addOption(srv.getServerID()); + + // Pass it to the server and hope for a REPLY + Pkt6Ptr reply = srv.processRelease(rel); + + // Check if we get response at all + checkResponse(reply, DHCPV6_REPLY, 1234); + checkMsgStatusCode(reply, STATUS_Success); + + // Check DUIDs + checkServerId(reply, srv.getServerID()); + checkClientId(reply, clientid); + + // Check lease + l = LeaseMgrFactory::instance().getLease6(type, *duid_, iaid, + subnet_->getID()); + ASSERT_TRUE(l); + EXPECT_EQ(l->valid_lft_, 0); + EXPECT_EQ(l->preferred_lft_, 0); + + // Create query + Pkt6Ptr query; + if (qtype != DHCPV6_SOLICIT) { + query = createMessage(qtype, type, addr, prefix_len, iaid); + query->addOption(srv.getServerID()); + } else { + query = createMessage(qtype, type, IOAddress::IPV6_ZERO_ADDRESS(), + prefix_len, iaid); + } + query->addOption(clientid); + + // Process query + switch (qtype) { + case DHCPV6_SOLICIT: + reply = srv.processSolicit(query); + break; + case DHCPV6_REQUEST: + reply = srv.processRequest(query); + break; + case DHCPV6_RENEW: + reply = srv.processRenew(query); + break; + case DHCPV6_REBIND: + reply = srv.processRebind(query); + break; + default: + reply.reset(); + break; + } + + // Check reply + if (qtype == DHCPV6_SOLICIT) { + checkResponse(reply, DHCPV6_ADVERTISE, 1234); + } else { + checkResponse(reply, DHCPV6_REPLY, 1234); + } + checkServerId(reply, srv.getServerID()); + checkClientId(reply, clientid); + checkMsgStatusCode(reply, STATUS_Success); + if (type == Lease::TYPE_NA) { + Option6IAAddrPtr iaaddr = checkIA_NA(reply, iaid, subnet_->getT1(), + subnet_->getT2()); + ASSERT_TRUE(iaaddr); + checkIAAddr(iaaddr, addr, type, subnet_->getPreferred(), + subnet_->getValid()); + } else { + Option6IAPrefixPtr iapref = checkIA_PD(reply, iaid, subnet_->getT1(), + subnet_->getT2()); + ASSERT_TRUE(iapref); + checkIAAddr(iapref, addr, type, subnet_->getPreferred(), + subnet_->getValid()); + } + + // Check lease + l = LeaseMgrFactory::instance().getLease6(type, *duid_, iaid, + subnet_->getID()); + ASSERT_TRUE(l); + if (qtype == DHCPV6_SOLICIT) { + EXPECT_EQ(l->valid_lft_, 0); + EXPECT_EQ(l->preferred_lft_, 0); + } else { + EXPECT_EQ(l->valid_lft_, subnet_->getValid()); + EXPECT_EQ(l->preferred_lft_, subnet_->getPreferred()); + } +} + void Dhcpv6SrvTest::testReleaseReject(Lease::Type type, const IOAddress& addr) { NakedDhcpv6Srv srv(0); diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.h b/src/bin/dhcp6/tests/dhcp6_test_utils.h index 65e833f82c..161fb5ae6c 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.h +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.h @@ -208,10 +208,24 @@ public: isc::dhcp::LeaseMgrFactory::destroy(); } + /// @brief Processes incoming Solicit message. + /// + /// @param request a message received from client + /// @return REPLY message or null + Pkt6Ptr processSolicit(const Pkt6Ptr& solicit) { + AllocEngine::ClientContext6 ctx; + bool drop = !earlyGHRLookup(solicit, ctx); + initContext(solicit, ctx, drop); + if (drop) { + return (Pkt6Ptr()); + } + return (processSolicit(ctx)); + } + /// @brief Processes incoming Request message. /// /// @param request a message received from client - /// @return REPLY message or NULL + /// @return REPLY message or null Pkt6Ptr processRequest(const Pkt6Ptr& request) { AllocEngine::ClientContext6 ctx; bool drop = !earlyGHRLookup(request, ctx); @@ -225,7 +239,7 @@ public: /// @brief Processes incoming Renew message. /// /// @param renew a message received from client - /// @return REPLY message or NULL + /// @return REPLY message or null Pkt6Ptr processRenew(const Pkt6Ptr& renew) { AllocEngine::ClientContext6 ctx; bool drop = !earlyGHRLookup(renew, ctx); @@ -239,7 +253,7 @@ public: /// @brief Processes incoming Rebind message. /// /// @param rebind a message received from client - /// @return REPLY message or NULL + /// @return REPLY message or null Pkt6Ptr processRebind(const Pkt6Ptr& rebind) { AllocEngine::ClientContext6 ctx; bool drop = !earlyGHRLookup(rebind, ctx); @@ -253,7 +267,7 @@ public: /// @brief Processes incoming Release message. /// /// @param release a message received from client - /// @return REPLY message or NULL + /// @return REPLY message or null Pkt6Ptr processRelease(const Pkt6Ptr& release) { AllocEngine::ClientContext6 ctx; bool drop = !earlyGHRLookup(release, ctx); @@ -267,7 +281,7 @@ public: /// @brief Processes incoming Decline message. /// /// @param decline a message received from client - /// @return REPLY message or NULL + /// @return REPLY message or null Pkt6Ptr processDecline(const Pkt6Ptr& decline) { AllocEngine::ClientContext6 ctx; bool drop = !earlyGHRLookup(decline, ctx); @@ -742,8 +756,8 @@ public: /// two different instances of an option that has identical content /// will return true. /// - /// It is safe to pass NULL pointers. Two NULL pointers are equal. - /// NULL pointer and non-NULL pointers are obviously non-equal. + /// It is safe to pass null pointers. Two null pointers are equal. + /// null pointer and non-null pointers are obviously non-equal. /// /// @param option1 pointer to the first option /// @param option2 @@ -833,6 +847,19 @@ public: const isc::asiolink::IOAddress& release_addr, const LeaseAffinity lease_affinity); + /// @brief Checks that reassignement of a released-expired lease + /// does not lead to zero lifetimes. + /// + /// This method does not throw, but uses gtest macros to signify failures. + /// + /// @param type lease type (TYPE_NA or TYPE_PD). + /// @param addr lease address. + /// @param qtype query type. + void + testReleaseNoDelete(isc::dhcp::Lease::Type type, + const isc::asiolink::IOAddress& addr, + uint8_t qtype); + /// @brief Performs negative RELEASE test /// /// See releaseReject and pdReleaseReject tests for detailed diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index c36cdc9ff1..48b8aebdbf 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -2278,6 +2278,11 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases lease->fqdn_fwd_ = ctx.fwd_dns_update_; lease->fqdn_rev_ = ctx.rev_dns_update_; lease->hostname_ = ctx.hostname_; + uint32_t current_preferred_lft = lease->preferred_lft_; + if (lease->valid_lft_ == 0) { + // The lease was expired by a release: reset zero lifetimes. + getLifetimes6(ctx, lease->preferred_lft_, lease->valid_lft_); + } if (!ctx.fake_allocation_) { bool update_stats = false; @@ -2298,7 +2303,6 @@ AllocEngine::updateLeaseData(ClientContext6& ctx, const Lease6Collection& leases lease->cltt_ = time(NULL); if (!fqdn_changed) { - uint32_t current_preferred_lft = lease->preferred_lft_; setLeaseReusable(lease, current_preferred_lft, ctx); } if (lease->reuseable_valid_lft_ == 0) {