#4268 closed enhancement (complete)

Extract constant length fields from DHCPv4 packet

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: classification 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: 3
Total Hours: 21 Internal?: no

Description

The ClientClassificationDesign calls for implementing methods to extract fixed length fields from the DHCPv4 packet. In particular, the following fields must be accessible: chaddr, giaddr, ciaddr, yiaddr, siaddr, hlen, htype.

Subtickets

Change History (22)

comment:1 Changed 22 months ago by tomek

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

comment:2 Changed 21 months ago by tomek

  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing

There is one thing that I'd like to bring to reviewer's attention. I'm not sure how to implement the integer values of hlen and htype fields. They're currently treated as strings, so user has to specify:

pkt4.hlen == "6"

rather than

pkt4.hlen == 6

IMHO we should stay with this until we decide to provide a full arithmetic.
Another point: if/when we'll add integer literals we'll have to solve the issue of hexadecimal integer constants...

(note it is not a review: it is only a comment).

Last edited 21 months ago by fdupont (previous) (diff)

comment:3 Changed 21 months ago by tomek

  • Total Hours changed from 0 to 16

comment:4 Changed 21 months ago by sar

  • Owner changed from Unassigned to sar

comment:5 follow-up: Changed 21 months ago by sar

  • Add Hours to Ticket changed from 0 to 3
  • Owner changed from sar to tomek

I have fixed some minor items, please do a pull before proceeding and verify they are correct.

doc/guide/classify.cml

I think the example for the mac address should be in quotes and have updated it accordingly
'01:02:03:04:05:06:07'

I think relay should be relay4 in the example expression and have updated it.

src/lib/eval/parser.yy

There is a comment about the addition of boolean expressions with a commented out line of code. Should this be uncommented? and will it create problems for validating an expression during initialization?

src/lib/eval/token.cc

In TokenPkt4::evaluate(), after the end of the switch statement there is the line
values.push(pkt4.toText());
I don't think this will be doing anything as we should exit the switch either due to a return or a throw and not get there.

src/lib/eval/tests/context_unittest.cc

The comment in testPkt4Field at line 167 is wrong - it says there should be only 1 token. The code correctly checks for the expected number of tokens.

src/lib/eval/tests/token_unittest.cc

There should be a test to verify that using a pkt6 causes an error.

comment:6 in reply to: ↑ 5 Changed 21 months ago by fdupont

Replying to sar:

doc/guide/classify.xml

I think relay should be relay4 in the example expression and have updated it.

=> note this was fixed in master after trac4268 branching.

src/lib/eval/parser.yy

There is a comment about the addition of boolean expressions with a commented out line of code. Should this be uncommented? and will it create problems for validating an expression during initialization?

=> the comment is highly incorrect and should be removed including the line of code.

In fact I strongly recommend to rebase this branch because there are many details which IMHO should be reviewed before merging vs be done during the conflict resolution during merging.

I have another concern about the representation of addresses (there should be no real issue for IPv4, more for MAC and definitely one for IPv6) and the design document says nothing about this: the current proposal seems to return the address in its textual form. I believe it is better to return it in its binary form and to introduce address constants, cf #4232.
There are other solutions but with other words the only two ways to reliably compare two addresses is either to compare the binary values, or to compare the result of inet_ntop() on both. The first solution is obviously simpler so I propose to use inet_pton() in the configuration phase.
Note something similar should be done for MAC addresses (BTW text to binary conversion functions exist but are not standard).

comment:7 follow-up: Changed 20 months ago by sar

After some discussion around Feb 25 the consensus is to push a binary string on to the value stack for IP addresses. So an IPv4 address of 10.0.0.1 would be 0x0A000001.

comment:8 in reply to: ↑ 7 Changed 20 months ago by fdupont

Replying to sar:

After some discussion around Feb 25 the consensus is to push a binary string on to the value stack for IP addresses. So an IPv4 address of 10.0.0.1 would be 0x0A000001.

=> note I've just merged #4232 aka IP address literals. This should make unit tests far easier to write according to my experience.

comment:9 Changed 20 months ago by fdupont

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

comment:10 Changed 20 months ago by fdupont

Finished rebase into trac4268a. Should be ready for review.
Note all fields are extracted into binary, this works well with addresses but the lack of first order integer literal can be considered as an issue (and for #4269 too).

comment:11 Changed 20 months ago by fdupont

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

comment:12 Changed 19 months ago by sar

  • Owner changed from UnAssigned to sar

comment:13 follow-ups: Changed 19 months ago by sar

  • Owner changed from sar to fdupont

It will require a ChangeLog? entry I suggest

Added support for extracting constant length fields from a DHCPv4 packet

The integers htype and hlen need to be updated to produce 4 byte strings with the tests and documentation updated as appropriate.

doc/guide/classify.xml

The descriptions of the integer and IP address fields should include an indication of the string length.

src/lib/eval/parser.yy

I preferred the more compact layout for describing pkt4_field - is there a reason to use the expanded one?

src/lib/eval/tests/token_unittest.cc

The code to test the address based fields is pretty much the same for all of them, perhaps create a small test routine that can be called with either the full ip address or even just the last byte of the ip address and the field type.

comment:14 Changed 19 months ago by sar

  • Total Hours changed from 16 to 18

comment:15 in reply to: ↑ 13 Changed 19 months ago by fdupont

Replying to sar:

=> other comments are about the fact the code was put under review before Tomek's "compromise".
(PS: I'll addressed them later)

src/lib/eval/parser.yy

I preferred the more compact layout for describing pkt4_field - is there a reason to use the expanded one?

=> yes: it is very easy to write unreadable parsers so it is IMHO important to follow strict rules and never use "short cuts". As the bison and C++ codes are mixed they must be clearly separated, i.e., not on the same line. Unfortunately for C++ actions the transition characters { and } are shared with the bison syntax so IMHO they must be left alone on a line. If (when!) the syntax will become more complex I think these rules will be welcomed (of course if and only if they are followed).

comment:16 in reply to: ↑ 13 ; follow-up: Changed 19 months ago by fdupont

  • Owner changed from fdupont to sar

Replying to sar:

It will require a ChangeLog? entry I suggest

Added support for extracting constant length fields from a DHCPv4 packet

=> I'll use this (BTW #4269's log gives the (far shorter) list).

The integers htype and hlen need to be updated to produce 4 byte strings with the tests and documentation updated as appropriate.

=> done. BTW should I add than the hlen in chaddr belongs to [0..16] (it is in code comments so should it be in the guide too?)

doc/guide/classify.xml

The descriptions of the integer and IP address fields should include an indication of the string length.

=> done.

src/lib/eval/parser.yy

I preferred the more compact layout for describing pkt4_field - is there a reason to use the expanded one?

=> already answered.

src/lib/eval/tests/token_unittest.cc

The code to test the address based fields is pretty much the same for all of them, perhaps create a small test routine that can be called with either the full ip address or even just the last byte of the ip address and the field type.

=> tests should not be extended or reused so IMHO this idea comes too late. But if we reorganize field extraction tests it should be fine to have a more generic way to do them...

Ready for another (fast/easy) review round. BTW some previous merges didn't follow the proposed organization: I can fix this either at #4268 merge time (i.e., moving blocks of code) or it can be done as part of #4272.

comment:17 Changed 19 months ago by fdupont

  • Total Hours changed from 18 to 20

comment:18 in reply to: ↑ 16 ; follow-up: Changed 19 months ago by sar

  • Owner changed from sar to fdupont
  • Total Hours changed from 20 to 21

Replying to fdupont:

Replying to sar:

It will require a ChangeLog? entry I suggest

Added support for extracting constant length fields from a DHCPv4 packet

=> I'll use this (BTW #4269's log gives the (far shorter) list).

If you wish to add the v4 list I'm fine with that but don't think it is required.

The integers htype and hlen need to be updated to produce 4 byte strings with the tests and documentation updated as appropriate.

=> done. BTW should I add than the hlen in chaddr belongs to [0..16] (it is in code comments so should it be in the guide too?)

I've added that as well as changed the text for the 4 byte lengths just a bit. Please do a pull and check my changes.

doc/guide/classify.xml

The descriptions of the integer and IP address fields should include an indication of the string length.

=> done.

src/lib/eval/parser.yy

I preferred the more compact layout for describing pkt4_field - is there a reason to use the expanded one?

=> already answered.

src/lib/eval/tests/token_unittest.cc

The code to test the address based fields is pretty much the same for all of them, perhaps create a small test routine that can be called with either the full ip address or even just the last byte of the ip address and the field type.

=> tests should not be extended or reused so IMHO this idea comes too late. But if we reorganize field extraction tests it should be fine to have a more generic way to do them...

Ready for another (fast/easy) review round. BTW some previous merges didn't follow the proposed organization: I can fix this either at #4268 merge time (i.e., moving blocks of code) or it can be done as part of #4272.

Which organization? If you mean the spacing for the classification text, yes that should get updated as appropriate. If you are thinking of something else we should probably put that into another ticket either 4272 or a new one.

The rest of the changes look fine and I was able to compile and run make check.

comment:19 in reply to: ↑ 18 Changed 19 months ago by fdupont

Replying to sar:

=> done. BTW should I add than the hlen in chaddr belongs to [0..16] (it is in code comments so should it be in the guide too?)

I've added that as well as changed the text for the 4 byte lengths just a bit. Please do a pull and check my changes.

=> checked.

Ready for another (fast/easy) review round. BTW some previous merges didn't follow the proposed organization: I can fix this either at #4268 merge time (i.e., moving blocks of code) or it can be done as part of #4272.

Which organization? If you mean the spacing for the classification text, yes that should get updated as appropriate. If you are thinking of something else we should probably put that into another ticket either #4272 or a new one.

=> I mean the order in code, cf token.h line 42 (you can simply grep order in this file). I'll do it in #4272.

The rest of the changes look fine and I was able to compile and run make check.

=> so it is ready for merge, isn't it?

comment:20 Changed 19 months ago by fdupont

  • Owner changed from fdupont to sar

comment:21 Changed 19 months ago by sar

  • Owner changed from sar to fdupont

It is ready for merge

comment:22 Changed 19 months ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.