Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#2615 closed defect (fixed)

Server should check address validity before renewing lease

Reported by: stephen Owned by: tmark
Priority: medium Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This ticket has been created in response to an issue raised in the discussion on ticket #2488.

If a DHCP server hands out a lease for an address to a client, and is then reconfigured so that the address is removed from the pool of addresses available to the server, it should refuse to renew the lease for that address. At present, the leases is renewed with no checks on address validity.

Subtickets

Change History (13)

comment:1 Changed 6 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Kea-proposed

comment:2 Changed 6 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

comment:3 Changed 4 years ago by tmark

  • Version set to git

This a similiar topic reported in ISC_DHCP under https://bugs.isc.org/Ticket/Display.html?id=27372. This ticket talks about ensuring the DDNS clean up is done, when releasing or renewing an out-of-range lease. We need to make sure Kea does the proper thing here.

comment:4 Changed 4 years ago by marcin

From what I see in the code, this issue doesn't seem to be present in the DHCPv4 code. I am not so sure about DHCPv6 code.

comment:5 Changed 4 years ago by tmark

  • Owner set to tmark
  • Status changed from new to accepted

comment:6 Changed 4 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from accepted to reviewing

Ticket is ready for review:

For the DHCPv4 server, the bulk of the effort was creating unit tests to ensure we are handling renews and releases of out-of-range addresses correctly. The only behavioral difference made was to allow clients to release leases whose subnet no longer exists. Prior to this the server was specifically rejecting releases which could not be matched to a subnet. By removing this check, clients can clean up after themselves without having to rely on lease expiration.

The new unit tests are contained with src/bin/dhcp4/tests/out_of_range_unittest.cc

Proposed ChangeLog:

xxx.    [func]      tmark    
    The DHCPv4 server will now honor DHCPRELEASEs for leased addresses which
    cannot be matched to subnet.  This allows leases to be released after
    configuration changes have eliminated their subnet.  Prior to this the
    server would reject the release and emit a DHCP4_RELEASE_FAIL_NO_SUBNET
    log message.
    (Trac #2615, git TBD)

I have created #4011 to cover the DHCPv6 Server, in order to keep ticket scope manageable.

comment:7 Changed 4 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:8 Changed 4 years ago by tmark

  • Owner changed from tmark to UnAssigned

comment:9 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:10 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit c114322a51071a5b37bbd97a890553f945388185

ChangeLog
It should be marked that it is backward incompatible change with an asterisk near [func]. The log message has been removed and the behavior has changed.

src/bin/dhcp4/tests/out_of_range_unittest.cc
The new test is well structured and it is nice it is handled by a single function which is used in multiple tests. Good job.

I think the following headers don't need to be included:

  • host.h
  • host_mgr.h
  • shared_ptr.hpp

And probably no need for using namespace isc::data?

Configuration 1 description says "Same as configuration 1 ..." which causes infinite recursion :-)

CfgIndex: not a big deal but there is no need for setting "0" for REF_CFG.

src/bin/dhcp4/tests/release_unittest.cc
The comment for the ReleaseTest.releaseNoSubnet test sounds odd - some part of this comment is missing perhaps.

src/bin/dhcp6/tests/dhcp6_client.h
You have added doRelease function in the header, but there is no implementation of this function. Also, I don't understand why it has been added as part of this ticket. Perhaps, it should be removed for the time being?

After applying the appropriate changes you're welcome to merge the code.

comment:11 in reply to: ↑ 10 Changed 4 years ago by tmark

Replying to marcin:

Reviewed commit c114322a51071a5b37bbd97a890553f945388185

ChangeLog
It should be marked that it is backward incompatible change with an asterisk near [func]. The log message has been removed and the behavior has changed.

src/bin/dhcp4/tests/out_of_range_unittest.cc
The new test is well structured and it is nice it is handled by a single function which is used in multiple tests. Good job.

Why thank you! There were too many permutations to keep repeating the code, plus I was having a hard time keeping track of which test did what.

I think the following headers don't need to be included:

  • host.h
  • host_mgr.h
  • shared_ptr.hpp

Gone. Thanks, I forgot to go back and clean these up.

And probably no need for using namespace isc::data?


Removed it

Configuration 1 description says "Same as configuration 1 ..." which causes infinite recursion :-)

Oops. Fixed em up.

CfgIndex: not a big deal but there is no need for setting "0" for REF_CFG.

No, but I prefer to do it when using an enum as index.

src/bin/dhcp4/tests/release_unittest.cc
The comment for the ReleaseTest.releaseNoSubnet test sounds odd - some part of this comment is missing perhaps.

Fixed it.


src/bin/dhcp6/tests/dhcp6_client.h
You have added doRelease function in the header, but there is no implementation of this function. Also, I don't understand why it has been added as part of this ticket. Perhaps, it should be removed for the time being?

Oops. This shouldn't have been there. I've removed it.

After applying the appropriate changes you're welcome to merge the code.

comment:12 Changed 4 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed

Changes merged with git eeebf9f68cf5be6a0f7eefc78832d664361c4990
Added ChangeLog entry 999, marked with an asterisk as suggested.

Ticket is closed.

comment:13 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.