Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#3773 closed defect (fixed)

Cleanup "missing DUID" logic in Dhcpv6Srv

Reported by: tmark Owned by: fdupont
Priority: medium Milestone: Kea1.0-beta
Component: dhcp6 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

Reconcile the logic which detects when client has not supplied a client id (DUID). Dhcp6Srv::assignLeases() throws an RFCViolation exception while Dhcp6Srv::extendLeases() returns a reply with a failed status code/message.
There is a todo in Dhcp6Srv::copyClientOptions() stating it should throw
if there is no client-id...

This needs to be cleaned up, and probably handled in sanityChecking or createContext().

Subtickets

Change History (14)

comment:1 Changed 5 years ago by wlodekwencel

I would like to point out that server should also throw exception if client-id option isn't in the message not only when client-id option does not include DUID.

List of Forge tests which are related and failing on Jenkins:
v6.confirm.invalid.without_client_id
v6.decline.invalid.without_client_id
v6.decline.invalid.blank_client_id
v6.rebind.invalid.without_client_id

comment:2 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.2
  • Priority changed from medium to low

comment:3 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9.2 to Kea0.9.2-final

comment:4 Changed 5 years ago by marcin

  • Priority changed from low to very low

Moving to "very-low" per post-beta 0.9.2 tickets scrub.

comment:5 Changed 5 years ago by fdupont

sanityCheck() was written for this. I propose:

  • add missing calls to sanityCheck() for confirm, rebind, decline and inform
  • cleanup/update comments outside processXXX.
  • add missing unit tests.

If this plan is good please comment (and fix it if it is not :-).

comment:6 Changed 5 years ago by marcin

  • Milestone changed from Kea0.9.2 to Kea1.0
  • Priority changed from very low to low

Moving to 1.0 low as per ticket crub on 07/31/2015

comment:7 Changed 5 years ago by fdupont

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

As this ticket has at least to strongly related tickets in Kea1.0 milestone I am addressing it.

comment:8 Changed 5 years ago by fdupont

  • Priority changed from low to medium

Put the same priority than other tickets.

comment:9 Changed 5 years ago by fdupont

Decline unit tests should be added with decline code.

comment:10 Changed 5 years ago by fdupont

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

Done, ready for review. Missing:

  • decline code (should be merged with the new decline code when it will be available).
  • ChangeLog entry: something like "Added missing calls to sanityCheck() for confirm, rebind and inform DHCPv6 incoming messages."

comment:11 Changed 5 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:12 follow-up: Changed 5 years ago by tomek

  • Owner changed from tomek to fdupont

Thanks for doing this work. Here's my review:

confirm_unittest.cc
Comments in ConfirmTest?.sanityCheck should say:

No clientID should fail =>
A message with no clientID should fail

A clientID should succeed =>
A message with a single client-id should succeed

A serverID should fail =>
A message with server-id present should fail

ConfirmTest?.sanityCheck should check that Confirm
with multiple client-ids or multiple server-ids is
rejected.

rebind_unittest.cc

See comments for confirm_unittest.cc.

Can you add similar unit-tests as for Confirm and
Rebind for Solicit, Request, Renew, InfRequest?
and Release?


Code compiled and unit-tests passed on Ubuntu 15.04 x64.


I would update the ChangeLog? entry slightly. Users can
only guess what the sanityCheck() method does. How about this:

9XX.	[func]		marcin
	Incoming Confirm, Rebind and Information-Request messages are
	now more thoroughly checked against presence of client-id
        and server-id options.
	(Trac #3773, git tbd)

If you agree with my all my comments, I don't see to see this
ticket again. If not, let's continue our discussion.

comment:13 in reply to: ↑ 12 Changed 5 years ago by fdupont

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

Replying to tomek:

Thanks for doing this work. Here's my review:

confirm_unittest.cc
Comments in ConfirmTest?.sanityCheck should say:

No clientID should fail =>
A message with no clientID should fail

A clientID should succeed =>
A message with a single client-id should succeed

A serverID should fail =>
A message with server-id present should fail

ConfirmTest?.sanityCheck should check that Confirm
with multiple client-ids or multiple server-ids is
rejected.

rebind_unittest.cc

See comments for confirm_unittest.cc.

=> done. I use client-id (vs clientID) in comments.

Can you add similar unit-tests as for Confirm and
Rebind for Solicit, Request, Renew, InfRequest?
and Release?

=> there is nothing to test for InfRequest? and
for other types this was already tested.
(in fact only decline has to be done now).


Code compiled and unit-tests passed on Ubuntu 15.04 x64.


I would update the ChangeLog? entry slightly. Users can
only guess what the sanityCheck() method does. How about this:

9XX.	[func]		marcin
	Incoming Confirm, Rebind and Information-Request messages are
	now more thoroughly checked against presence of client-id
        and server-id options.
	(Trac #3773, git tbd)

If you agree with my all my comments, I don't see to see this
ticket again. If not, let's continue our discussion.

=> Merged.

comment:14 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.