Skip to content

Add Dhcp packet#386

Closed
alexander-clarke wants to merge 1 commit intolibpnet:masterfrom
alexander-clarke:master
Closed

Add Dhcp packet#386
alexander-clarke wants to merge 1 commit intolibpnet:masterfrom
alexander-clarke:master

Conversation

@alexander-clarke
Copy link
Copy Markdown

Hi,
This pull request adds initial support for the Dhcp packet
Dhcp options are not yet supported as I wasn't sure what the best method to do this would be, as such they are in the payload at this moment in time
Feedback welcome

@JuxhinDB
Copy link
Copy Markdown
Member

JuxhinDB commented Oct 8, 2019

Thanks for this, would you be able to add some tests prior to merging?

@kishiguro kishiguro added this to the libpnet 0.27 milestone May 1, 2020

/// Represents the Dhcp hardware types.
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct DhcpHardwareType(pub u8);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this reuse the values from arp hardware?

For reference, pls see: https://tools.ietf.org/html/rfc5494

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvinci: Feel free to be less humble and/or less modest.

If your message is

In section 2. IANA Considerations of https://tools.ietf.org/html/rfc5494 is ar$hrd (16 bits) Hardware address space, that will not fit the u8 of DhcpHardwareType.

just say so.

Copy link
Copy Markdown

@Biogen Biogen Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardware type has 16 bit for DHCP according to RFC-2131. There are 2 fields in author struct: 8-bit for hwtype and 8-bit for hwlen.
I understand your point: ARP hwtype has 16-bit AND 8-bit for len, but from the point of view dhcp protocol current HW fields are correct too, aren't?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stappersg, I don't understand your comment. My comment was intended to be open ended to start a discussion. My suggestion is that we just use the lower 8 bits of the existing list and avoid the hygiene related effort of maintaining multiple types. Practically speaking, the list is small and changes infrequently.

@Biogen,
To answer your question, htype is 1 octet in RFC 2131. Top of page 9:

htype         1  Hardware address type, see ARP section in "Assigned
                    Numbers" RFC; e.g., '1' = 10mb ethernet.

hwlen is the number of octets in the hw address, ie 6 for a mac address.

For further clarification on the values of hwlen, see rfc5494, sec. 2. Values for dhcp will be assigned to fit in a u8.

The following rules apply to the fields of ARP:

   ar$hrd (16 bits) Hardware address space

      Requests for ar$hrd values below 256 or for a batch of more than
      one new value are made through Expert Review [RFC5226].

      Note that certain protocols, such as BOOTP and DHCPv4, employ
      these values within an 8-bit field.  The expert should determine
      that a need to allocate the new values exists and that the
      existing values are insufficient to represent the new hardware
      address types.  

@stappersg
Copy link
Copy Markdown
Contributor

@alexander-clarke about your

Feedback welcome

@JuxhinDB replied with 24 hours with

Thanks for this, would you be able to add some tests prior to merging?

@Biogen
Copy link
Copy Markdown

Biogen commented Apr 22, 2021

@alexander-clarke will you complete pr?

I just noticed that I need DHCP. so instead of creating a new solution I decided to clarify the current status 😄

@stappersg
Copy link
Copy Markdown
Contributor

I don't understand your comment. My comment was intended to be open ended to start a discussion.

Ah. I think that @pvinci just learnt that the discussion was indeed (re)started. And I think we should move away from an open end, that we aim for a conclusion.

My suggestion is that we just use the lower 8 bits of the existing list and avoid the hygiene related effort of maintaining multiple types. Practically speaking, the list is small and changes infrequently.

The just use the lower 8 bits do I read as just ignore the RFC.

@alexander-clarke please help us by transforming your I have moved on into I have closed my old pull request, so now we can all move on.

@pvinci
Copy link
Copy Markdown

pvinci commented Apr 25, 2021

@stappersg
My intention was to convey the opposite. My perspective was that using the lower 8 bits is following the rfc, not ignoring it. That was why I quoted the section from rfc 5495, clarifying that dhcp employable values are to be assigned from the lower 8 bit range.

Let me try again.
Is it better to add a u8 primitive value to the existing ArpHardwareType than to create a distinct new DhcpHardwareType and maintain two lists?
I'm referencing this section of code:
https://github.com/libpnet/libpnet/blob/master/pnet_packet/src/arp.rs.in#L57

@mrmonday
Copy link
Copy Markdown
Contributor

mrmonday commented May 9, 2021

It looks like the original author of the MR hasn't responded in a while. I'm going to close this MR, but if someone would like to rebase it and open a fresh one I'll take a look when I can.

@mrmonday mrmonday closed this May 9, 2021
Martichou added a commit to Martichou/libpnet that referenced this pull request Aug 31, 2022
Martichou pushed a commit to Martichou/libpnet that referenced this pull request Sep 2, 2022
(see libpnet#386)

Signed-off-by: Martin André <martin.andre@tessares.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants