Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#3200 closed defect (fixed)

DHCPv4 Parameter Request List Option is not handled correctly.

Reported by: marcin Owned by: marcin
Priority: high Milestone: Kea0.8
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: High
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The DHCPv4 PRL Option is represented by the OptionPtr? class, but the DHCPv4 server code casts it to Option8UintArray which results in NULL pointer and inability to retrieve options requested by a client.

Subtickets

Change History (6)

comment:1 Changed 6 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20131016
  • Owner set to UnAssigned
  • Status changed from new to reviewing

The PRL option is now represented by the OptionUint8Array class. The existing unit tests for PRL option were insufficient, because they were not able to detect that the type of the option was invalid. I fixed the test so as it now unpacks the option from the buffer holding an option in the wire format. If there is a mismatch between the option definition and the type to which the option is cast, the test will now catch the resulting option instance being NULL.

The proposed changelog entry:

6XX.	[bug]		marcin
	b10-dhcp4: Fix a bug whereby the Parameter Request List was not parsed
	by the server and requested DHCPv4 options were not returned to the
	client.
	(Trac #3200, git abcd)

comment:2 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to marcin

dhcp4_messages.msg
It may be confusing whether the yiaddr is what client sent or what
server assigned. To remove this ambiguity, it is better to rephrase
the message as:

% DHCP4_LEASE_ADVERT_FAIL failed to advertise a lease for client
client-id %1, hwaddr %2, client sent yiaddr %3

dhcp4_test_utils.cc
In !Dhcpv4SrvTest::testDiscoverRequest you may replace

    EXPECT_FALSE(rsp->getOption(DHO_LOG_SERVERS));
    EXPECT_FALSE(rsp->getOption(DHO_COOKIE_SERVERS));
    EXPECT_FALSE(rsp->getOption(DHO_LPR_SERVERS));

with noOptionsCheck() call.

dhcp4_srv_unittest.cc
We may add a test that uses actual packet capture with PRL. I vaguely
remember that I wrote such a test last week, but it is apparently not
on this branch. So for now, please add a @todo somewhere
and mention that it should check if packet returned by
captureRelayedDiscover() would be handled correctly (and options
requested in PRL be returned). It will be a test very similar to
Dhcpv4SrvTest.relayAgentInfoEcho. On one branch, there is also additional
capture added to wireshark.cc, so can use that, too. But definitely
that's a thing we can do later. A simple @todo will suffice for
now.

option_definition.cc
With the change in option_definition.cc, I just realized that this
change will affect how we handle v4 vendor options that uses
OPT_UINT8_TYPE array option. When tweaking #3194, you'll probably
need to merge #3200 on it.

Make sure to mention in changelog that options are no longer sent
back to the client if server failed to assign a lease. This is a user
visible change, so we should mention that.

If you agree with my suggestions and you apply them, you can merge
the ticket.

comment:3 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

I addressed your comments. Also, I did some modification to the dhcp4_test_util by splitting functions which check presence of the Basic options and Requested options.

Please review again.

comment:4 Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

dhcp4_test_utils.h
optionsCheck(), noOptionsCheck - extended description has "options" word missing.

Changes look good. Make sure you update ChangeLog? before merging.

comment:5 Changed 6 years ago by marcin

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

Merged with commit 50d91e4c069c6de13680bfaaee3c56b68d6e4ab1

comment:6 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20131016 to DHCP-Kea1.0-alpha
Note: See TracTickets for help on using tickets.