Opened 5 years ago

Last modified 4 years ago

#3497 reviewing enhancement

Pkt4 class enhancements (contributed patch)

Reported by: tomek Owned by: contributor
Priority: medium Milestone: Outstanding Tasks
Component: dhcp4 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 (last modified by tomek)

This is a seventh ticket created for a patch submitted by Shawn Lewis.

Shawn:
In some network scenarios where the relay server does not have a globally reachable IPv4 address, it may
need to relay the DHCP messages to a DHCP server via a GRE tunnel between the relay server and the
DHCP server. Then network headers from lower layers (IP layer, Ethernet layer) could be included in the
DHCP messages. In such cases, the Pkt4::unpack() function may need to skip these headers at the
beginning of the packet. Thus it would be handy to redefine the unpack function as Pkt4::unpack(int index),
where index has a default value of zero. In this way, a packet will be decoded from the beginning by
default. But the buffer4_recv() callout will have the option to skip several bytes if necessary.

Subtickets

Change History (9)

comment:1 Changed 5 years ago by tomek

  • Description modified (diff)
  • Milestone changed from Kea-proposed to Kea1.0

As discussed on Kea meeting (2014-07-16) we not include this ticket in 0.9, due to lack of available processing time. Therefore moving to 1.0.

comment:2 Changed 5 years ago by tomek

  • Version set to git

See ticket #3491 for the original patch.

comment:3 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1

We haven't managed to address this one, moving to 0.9.1.

comment:4 Changed 5 years ago by marcin

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

comment:5 Changed 5 years ago by marcin

I reviewed the portion of the patch which introduces the enhancements to the Pkt4 class.

Thanks for doing the work and sharing use cases. My comments follow.

Skipping part of the buffer with unpack()
A couple of questions:

  • This change has been introduced for DHCPv4. But, would it be relevant for DHCPv6 too?
  • This change applies to the received packets. But, what about the responses from the server to a relay? Don't they require any additional encapsulation. If so, where the encapsulation is taking place. Is it done by the system kernel?
  • If I understand that correctly, on some installation there may be additional layers encapsulating the IP/UDP headers and with the current code, if the IP/UDP headers are not removed, the unpack() function would wrongly start parsing an IP header as a DHCP packet and would obviously fall over. So, is it only that the IP/UDP header is redundant and has to be removed or it can be used in a callout for anything?

I think it is not a responsibility/purpose of the !Pkt4 class to remove the overhead from the packet. For example, we already have classes, e.g. !PktFilterLPF or !PktFilterBPF that receive packets over the raw sockets and remove the IP/UDP headers before the !Pkt4 instance is created. Your case seems to be similar and I think there should be a separate class created to handle the case of GRE tunnels.

So instead of doing this:

    int buffer_size;
    char* buffer = rceive_from_GRE_tunnel(buffer_size);
    Pkt4Ptr pkt4(buffer, buffer_size);
    pkt4->unpack(ip_udp_length);

you could simply do:

    int buffer_size;
    char* buffer = rceive_from_GRE_tunnel(buffer_size);
    Pkt4Ptr pkt4(buffer + ip_udp_length, buffer_size - ip_udp_length);
    pkt4->unpack();

The danger I see with the idea of passing extraneous data in the input buffer is that the packet can be passed between various functions and, by itself, it doesn't carry any information where the useful data is located. So, unless the instance which created the packet calls unpack() the packet becomes useless for the methods which receive it as an argument because they don't know the offset with which they should call unpack(). More over, they may assume that the offset is 0 which will cause a parsing failure.

So, I would prefer to guarantee that the first byte of the packet is a beginning of the DHCP message unless there is really a good reason to not truncate the packet at the construction time. If there is, the offset should rather be conveyed in the Pkt4 object, not passed as an argument to unpack.

Extension of isRelayed function
I tend to agree that it may be useful to disable a strict check. Did you also observe cases that the giaddr is non-zero and the hops is zero?

getOption82
I see no reason why this should be limited to option 82. It would actually be beneficial to have the function that would return instances of all options having a specific option code. Handling mulitple options 82 would be just a special case.

comment:6 Changed 5 years ago by marcin

  • Owner changed from marcin to contributor

comment:7 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9.1 to Kea0.9.2

Moving to 0.9.2 as discussed on Kea weekly call today (2014-10-08).

If we hear back from the contributor and receive updated patch, we'll move it back to 0.9.1.

comment:8 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.2 to DHCP Outstanding Tasks

comment:9 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

Note: See TracTickets for help on using tickets.