Opened 9 years ago

Closed 7 years ago

#991 closed enhancement (complete)

Method to send a IPv4 packet to a client without an address

Reported by: shane Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130411
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by marcin)

Fo33ffc9a750cd3fb34158ef676aab6b05df0302e2r the DHCP IPv4 server to reply to clients, it needs to be able to send messages to a client that does not have an address.

One possibility for this is manipulating the ARP table. This was discussed years ago on the dhcp-workers list:

https://lists.isc.org/pipermail/dhcp-workers/2006-May/000030.html

Basically the trick is to use the SIOCSARP ioctl() to manipulate the ARP table. Seems to be portable to Linux and Solaris, but I'm not sure about BSD.

Otherwise we'll have to look at some raw packet injection, probably.

Subtickets

Change History (14)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 9 years ago by shane

  • Milestone changed from Year 3 Task Backlog to DHCP 2011

comment:3 Changed 7 years ago by marcin

The work in this ticket will be limited to support Linux (using Linux Packet Filtering). Support for other OSes will be handled in different tickets.

comment:4 Changed 7 years ago by marcin

  • Component changed from Unclassified to dhcp4

comment:5 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130411

comment:6 Changed 7 years ago by marcin

  • Owner set to marcin
  • Status changed from new to assigned

comment:7 Changed 7 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from assigned to reviewing

Adding support to respond to clients without address requires use of raw sockets. Such sockets can be only created by privileged user. Therefore, there is no easy way to unit test this code. The likely approach is to create mock object that mimics the operation of the raw socket. This would eliminate the need to open a real socket in unit tests. Also, it is desired to keep existing implementation around which doesn't use raw sockets for the OSes that don't have support for anything else (yet). This caused a need to make IfaceMgr a little bit more modular and abstract the code which opens sockets and sends/receives packets into a separate (replaceble) class. This required significant number of changes to IfaceMgr, so I decided not to do further implementation (of raw sockets etc.) in this ticket. Otherwise it would grow too large and would cause serious difficulties to review it.

Apart from the changes described above I implemented the short path to respond to the client, not having an address, by responding to broadcast address. This is proposed by RFC 951 if there is no way to respond directly. Obviously, once we implement direct responses they will take precedence over responding to broadcast address. Nevertheless, the approach has been tested on my local machine and it worked fine.

comment:8 Changed 7 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:9 follow-up: Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

Review Comments:

  1. dhcp4_srv.cc - Comment wording is wrong:

line 521 - "...client requesting new lease. We scan end response to"
s/scan end/can send/

  1. dchp4_srv.cc - else clause comment (line 781) says

"...On the other hand, if we fail to set remote
address to broadcast here, we can't unit-test the case when
server doesn't support direct responses to the client
which doesn't have address yet."

Does this imply that answer-setRemoteAddr() can fail? If so should we be
checking for that failure?

  1. dhcp4_srv_unittest.cc - in testDiscoverRequest(), there are 5 more tests

(lines 437-446) of response content following the call to messageCheck(). Why
aren't these in messageCheck() it seems like they belong there.

  1. dhcp4_srv_unittest.cc - You "repeat the test" on line 453. Won't this will

call processRequest for both a DISCOVER and a REQUEST type, regardless of the
value of msg_type?

  1. pkt_filter, general -

Doesn't this concept also apply to IPv6? The implementation for pkt_filter_inet.h
is IPv4 specific. Currently, an IfaceMgr? isn't instantiated only for 4 or 6,
rather it supports both based on the send or receive methods invoked. It would
seem that if packet fitlers will be needed for v6: then either there should be
protocol-specific derivations of pkt_filter and IfaceMgr? will need an
instance of each; or pkt_filter needs protocol specific sends/receives.

  1. PktFilterInet::openSocket() - if IP_PKTINFO is supported we try set socket

option for it and throw if it fails. What if IP_PKTINFO isn't defined? Isn't
that just as "fatal"?

  1. PktFilterInet::openSocket() - exception message for SO_BROADCAST option failure says "Failed to set SO_BINDTODEVICE".
  1. Code does not compile under OS-X: (I suspect it won't compile under Solaris

either).

libtool: compile: g++ -DHAVE_CONFIG_H -I. -I../../.. -I../../../src/lib -I../../../src/lib -I/opt/local/include -I/opt/local/include -DAPPLE_USE_RFC_3542 -DOS_BSD -I../../../ext/asio -I../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -Wno-missing-field-initializers -g -O2 -MT libb10_dhcp_la-iface_mgr_bsd.lo -MD -MP -MF .deps/libb10_dhcp_la-iface_mgr_bsd.Tpo -c iface_mgr_bsd.cc -fno-common -DPIC -o .libs/libb10_dhcp_la-iface_mgr_bsd.o
iface_mgr_bsd.cc: In member function 'int isc::dhcp::IfaceMgr::openSocket4(isc::dhcp::Iface&, const isc::asiolink::IOAddress&, uint16_t, bool, bool)':
iface_mgr_bsd.cc:54: error: 'socket_handler_' was not declared in this scope
iface_mgr_bsd.cc:61: error: 'SO_BINDTODEVICE' was not declared in this scope

Under Debian everything builds, unit tests pass. I even ran a test with perfdchp and every
thing works fine.

comment:10 in reply to: ↑ 9 Changed 7 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Review Comments:

  1. dhcp4_srv.cc - Comment wording is wrong:

line 521 - "...client requesting new lease. We scan end response to"
s/scan end/can send/

Fixed.

  1. dchp4_srv.cc - else clause comment (line 781) says

"...On the other hand, if we fail to set remote
address to broadcast here, we can't unit-test the case when
server doesn't support direct responses to the client
which doesn't have address yet."

Does this imply that answer-setRemoteAddr() can fail? If so should we be
checking for that failure?

No. setRemoteAddress() is fail safe, because it simply assigns existing !IOAddress object to a class member. I modified the comment to better explain what I had in mind.

  1. dhcp4_srv_unittest.cc - in testDiscoverRequest(), there are 5 more tests

(lines 437-446) of response content following the call to messageCheck(). Why
aren't these in messageCheck() it seems like they belong there.

Yes. I have moved them there.

  1. dhcp4_srv_unittest.cc - You "repeat the test" on line 453. Won't this will

call processRequest for both a DISCOVER and a REQUEST type, regardless of the
value of msg_type?

Good catch. Such things usually happen when there is a lot of copy-paste done.

  1. pkt_filter, general -

Doesn't this concept also apply to IPv6? The implementation for pkt_filter_inet.h
is IPv4 specific. Currently, an IfaceMgr? isn't instantiated only for 4 or 6,
rather it supports both based on the send or receive methods invoked. It would
seem that if packet fitlers will be needed for v6: then either there should be
protocol-specific derivations of pkt_filter and IfaceMgr? will need an
instance of each; or pkt_filter needs protocol specific sends/receives.

Possibly, but I haven't done that for V6 intentionally:

  • Direct V4 traffic requires ''manual'' construction of ip headers to respond to the client which doesn't have address. This is not the issue for IPv6 so implementation for IPv6 it is simpler and more generic (cross OSes). What we have today works well on either Linux or BSD.
  • There is a lot of changes to packet handling routines for V4. I don't want to do revolution in IPv6 domain while we have to do revolution in V4 domain. If we want, we can do it later, once IPv4 Direct traffic works.
  1. PktFilterInet::openSocket() - if IP_PKTINFO is supported we try set socket

option for it and throw if it fails. What if IP_PKTINFO isn't defined? Isn't
that just as "fatal"?

It is not. It may be undefined on some OSes and we still want the server to work. For sure it is not fatal, maybe just need another way to get the information about incoming packet (but this is out of scope in this ticket).

  1. PktFilterInet::openSocket() - exception message for SO_BROADCAST option failure says "Failed to set SO_BINDTODEVICE".

Fixed.

  1. Code does not compile under OS-X: (I suspect it won't compile under Solaris

either).

libtool: compile: g++ -DHAVE_CONFIG_H -I. -I../../.. -I../../../src/lib -I../../../src/lib -I/opt/local/include -I/opt/local/include -DAPPLE_USE_RFC_3542 -DOS_BSD -I../../../ext/asio -I../../../ext/coroutine -DASIO_DISABLE_THREADS=1 -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -Wno-missing-field-initializers -g -O2 -MT libb10_dhcp_la-iface_mgr_bsd.lo -MD -MP -MF .deps/libb10_dhcp_la-iface_mgr_bsd.Tpo -c iface_mgr_bsd.cc -fno-common -DPIC -o .libs/libb10_dhcp_la-iface_mgr_bsd.o
iface_mgr_bsd.cc: In member function 'int isc::dhcp::IfaceMgr::openSocket4(isc::dhcp::Iface&, const isc::asiolink::IOAddress&, uint16_t, bool, bool)':
iface_mgr_bsd.cc:54: error: 'socket_handler_' was not declared in this scope
iface_mgr_bsd.cc:61: error: 'SO_BINDTODEVICE' was not declared in this scope

Under Debian everything builds, unit tests pass. I even ran a test with perfdchp and every
thing works fine.

It is fixed now.

Proposed ChangeLog enytry:

595.	[func]		marcin
	libdhcp++: abstracted methods which open sockets and send/receive
	DHCP4 packets to a separate class. Other classes will be derived
	from it to implement OS-specific methods of DHCPv4 packets filtering.
	The primary purpose for this change is to add support for Direct
	DHCPv4 response to a client which doesn't have an address yet on
	different OSes.
	(Trac #2991, git abcd)

comment:11 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

The changes and comments above are satisfactory. However, there remains an issue.
While the code now compiles under OS-X, one of the lib/dhcp unit tests fails under
OS-X:
:
[ RUN ] IfaceMgrTest?.sendReceive4
iface_mgr_unittest.cc:780: Failure
Value of: rcvPkt->getRemoteAddr().toText()

Actual: "129.255.255.255"

Expected: "127.0.0.1"
[ FAILED ] IfaceMgrTest?.sendReceive4 (0 ms)
[ RUN ] IfaceMgrTest?.setPacketFilter
:
It passes under Debian 7.

The ChangeLog? entry should perhaps mention the new base class PacketFilter? by name.

comment:12 Changed 7 years ago by marcin

  • Owner changed from marcin to tmark

Thanks for catching this. I disabled the culprit code for the OSes other than Linux - this is how it used to be in the previous version of IfaceMgr. Please verify that it works on your OS-X system.

The modified ChangeLog entry

595.	[func]		marcin
	libdhcp++: abstracted methods which open sockets and send/receive
	DHCP4 packets to a separate class called PktFilter. Other classes
        will be derived from it to implement OS-specific methods of DHCPv4
        packets filtering. The primary purpose for this change is to add
        support for Direct DHCPv4 response to a client which doesn't have
        an address yet on different OSes.
	(Trac #991, git abcd)

comment:13 Changed 7 years ago by tmark

  • Owner changed from tmark to marcin

Unit Tests now pass on OS-X and still pass on Debian, so merge in the changes please.

comment:14 Changed 7 years ago by marcin

  • Description modified (diff)
  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit 33ffc9a750cd3fb34158ef676aab6b05df0302e2.

Note: See TracTickets for help on using tickets.