Opened 6 years ago

Closed 6 years ago

#3316 closed defect (fixed)

Kea not able to parse vendor class option

Reported by: wlodekwencel Owned by: marcin
Priority: medium Milestone: Kea0.8
Component: dhcp6 Version:
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: 32 Internal?: no

Description

After #3203 Kea not responding on messages that includes 'vendor class' option.
bind logs:

[b10-dhcp6.dhcp6/14423] DHCP6_PACKET_PARSE_FAIL failed to parse incoming packet

But working fine with 'vendor specific information' option.

attached:

  1. logs/config file/packet capture with vendor class
  2. logs/config file/packet capture with vendor specific information

Subtickets

Attachments (2)

1.tar.gz (2.2 KB) - added by wlodekwencel 6 years ago.
2.tar.gz (2.8 KB) - added by wlodekwencel 6 years ago.

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by wlodekwencel

Changed 6 years ago by wlodekwencel

comment:1 Changed 6 years ago by wlodekwencel

  • Summary changed from Kea are not able to parse vendor class option to Kea not able to parse vendor class option

comment:2 Changed 6 years ago by wlodekwencel

  • Milestone changed from DHCP-QA Defects to DHCP-Kea-proposed

comment:3 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea-proposed to DHCP-Kea0.9-alpha

Accepted as med priority (as discussed on 2014-02-05 Kea call)

comment:4 Changed 6 years ago by marcin

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

comment:5 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 0 to 9
  • Total Hours changed from 0 to 9

comment:6 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 9 to 4
  • Total Hours changed from 9 to 13

comment:7 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 4 to 7
  • Owner changed from marcin to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 13 to 20

I implemented the new class OptionVendorClass which encapsulates both DHCPv6 Vendor Class option (16) and DHCPv4 V-I Vendor Class option (124). This option is capable to decode multiple instances of the vendor-class data in it.

Please review.

Proposed ChangeLog:

XXX.	[bug]		marcin
	b10-dhcp6: server parses DHCPv6 Vendor Class option. Previously
	the server failed to parse Vendor Class option having empty opaque
	data field because of the invalid definition in libdhcp++. The
	DHCPv6 Vendor Class option and DHCPv4 V-I Vendor Class option is
	now represented by the new OptionVendorClass.
	(Trac #3316, git abcd)

comment:8 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:9 follow-up: Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 7 to 5
  • Owner changed from tomek to marcin
  • Total Hours changed from 20 to 25

Reviewing changes on trac3316, commit 282416056d98ce14b30998ae730c2ce1a4358dd0.

src/bin/dhcp6/dhcp6_srv.cc
classifyPacket(): I was thinking about the classification. Can you
modify this method to add VENDOR_CLASS prefix to the content of the
option?

        pkt->addClass("VENDOR_CLASS_" + DOCSIS3_CLASS_MODEM);

instead of

        pkt->addClass(DOCSIS3_CLASS_MODEM);

This is to prevent the client from faking his class. If you think this
is too much work, we'll need to do it in a separate ticket.

src/bin/dhcp6/tests/wireshark.cc
Please update copyright year.

src/lib/dhcp/opaque_data_tuple.h
@brief Exception to be thrown when there operation on ...
there => the

I have mixed feelings about having default value for length_field_type
in the constructor. This may lead to problems if someone forgets to
specify. It is similar to universe parameter in Option. We don't have
a default value there for a reason.

Do we need OpaqueDataTuple::Buffer type? Can't you just use OptionBuffer?
If you don't have strong opinion on this one, I'd prefer to keep the
number of new types to minimum.

OpaqueDataTuple templated constructor:
begining => beginning

append() instead of saying that the behavior is undefined if there is
too much data added, it would be better to just throw an exception.

OpaqueDataTuple is quite similar to Option class. Note that Option is
essentially a DataTuple with option code and sub-options added. So in
theory it would make sense to derive Option from DataTuple?. But I
think it would be too much work without much gain at this phase. In
other words, nothing more to do here.

src/lib/dhcp/opaque_data_tuple.cc

OpaqueDataTuble::pack()
Suggest parthentheses around 1 << (getDataFieldSize() * 8).
It is not immediately clear which operators takes precedence. Cool way
to calculate the max size, though.

append(): I'm thinking whether it should throw if the total length
after append is over limit (256 or 65536 bytes). That would pick up
any attempts earlier. Otherwise the size overflow will be detected
only in pack(), which may be in completely different place in the
code.

src/lib/dhcp/tests/opaque_data_tuple_unittest.cc
Line 101: thet => that

Comment for OpaqueDataTuple.equals() should mention that the
assignment operator is tested.

Line 173 (tuple = "xyz";) could use EXPECT_NO_THROW().

OpaqueDataTuple.pack2Bytes should check that trying to assign a buffer
over 65535 bytes will throw.

Comment for OpaqueDataTuple.unpack1ByteTruncatedBuffer:
if => is

The unit-tests are very thorough. Good work.

src/lib/dhcp/option_definition.cc
Changes in factorySpecialFormatOption(): I must admit that the
vendor-related option names are confusing. Do you think it would be ok
to add the option codes in comments? This is an honest question. Feel
free to ignore it.

src/lib/dhcp/option_vendor_class.h
Please mention option codes (that actual values) in class comment. I
confuse vendor options all the time. I'm sure I'm not the only one :(

Shouldn't the TuplesCollection type be moved to opaque_data_tuple.h?
There may be other places that will operate on tuple collections.

src/lib/dhcp/option_vendor_class.cc

getTuple()
exception thrown could also say how many tuples are there.

OptionVendorClass::toText() - Thanks for implementing this method, but
I was thinking about something slightly simpler: enterprise-id printed
as a regular decimal number, followed by a coma and concatenated
values, perhaps separated by space or comas. Once we get
user-configurable classification I was thinking about user being able
to say the following:

if (vendor-class==4491,docsis3) ... "

Here's an example what ISC-DHCP can do. We're not going to provide
exactly the same capability in Kea, but we should try to get as close
as reasonably possible:

https://pubs.vmware.com/vsphere-4-esx-vcenter/index.jsp?topic=/com.vmware.vsphere.installclassic.doc_41/install/boot_esx_install/c_sample_dhcp.html

src/lib/dhcp/tests/option_vendor_class_unittest.cc

OptionVendorClass.constructor4(). Thanks for explaining the reasons
why v4 constructors adds an empty tuple over jabber. Perhaps adding a
reference to RFC3925, section 3 would be useful here? Or maybe for
such a comment would be the constructor itself...

Some explanation about differences between unpack4NoTuple and
unpack6NoTuple would be useful. If you add a comment to the
constructor, a simple "see OptionVendorClass constructor for
explanation" would be sufficient.


Finally, a general comment. This bug (two bugs, actually) was discovered
by the fact that the tests send short vendor-class option. The first
bug was that our code was not able to parse specific option. You fixed
that bug. There is another bug - if for whatever reason we can't parse
an option, we should ignore just that option, not the whole message.

This has to be fixed. I don't have any preference if this is done in
this ticket or as a separate ticket.


The ChangeLog? entry looks almost fine, but it should start with b10-dhcp4,
b10-dhcp6 as the v4 implementation is also affected.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 5 to 4
  • Owner changed from marcin to tomek
  • Total Hours changed from 25 to 29

Replying to tomek:

Reviewing changes on trac3316, commit 282416056d98ce14b30998ae730c2ce1a4358dd0.

src/bin/dhcp6/dhcp6_srv.cc
classifyPacket(): I was thinking about the classification. Can you
modify this method to add VENDOR_CLASS prefix to the content of the
option?

        pkt->addClass("VENDOR_CLASS_" + DOCSIS3_CLASS_MODEM);

instead of

        pkt->addClass(DOCSIS3_CLASS_MODEM);

This is to prevent the client from faking his class. If you think this
is too much work, we'll need to do it in a separate ticket.

Added. As we agreed on jabber, you'll need to provide some text about why we're doing it.

src/bin/dhcp6/tests/wireshark.cc
Please update copyright year.

updated.

src/lib/dhcp/opaque_data_tuple.h
@brief Exception to be thrown when there operation on ...
there => the

Fixed.

I have mixed feelings about having default value for length_field_type
in the constructor. This may lead to problems if someone forgets to
specify. It is similar to universe parameter in Option. We don't have
a default value there for a reason.

On reflection I think you're right. Removed the default.

Do we need OpaqueDataTuple::Buffer type? Can't you just use OptionBuffer?
If you don't have strong opinion on this one, I'd prefer to keep the
number of new types to minimum.

We could, but the OptionBuffer is defined in Option class and I would need to include the option.h in the opaque_data_tuple.h which would cause a circular dependency. Or, I could do forward declaration which I dislike, as it causes troubles. Alternatively, I could move the OptionBuffer? definition to a separate header file for this typedef. I think it is too much burden. And in fact, the OpaqueDataTuple is independent from any !Option class and I prefer it to remain that way.

OpaqueDataTuple templated constructor:
begining => beginning

Fixed.

append() instead of saying that the behavior is undefined if there is
too much data added, it would be better to just throw an exception.

I made it a template function so as I can provide any type of iterator as function, including a raw pointer. For raw pointers I have no means to check whether the length is correct or not. That is the down side of using non-explicit data type but I think it has more benefits.

OpaqueDataTuple is quite similar to Option class. Note that Option is
essentially a DataTuple with option code and sub-options added. So in
theory it would make sense to derive Option from DataTuple?. But I
think it would be too much work without much gain at this phase. In
other words, nothing more to do here.

Yes. Although the mechanics of both of them is similar, I prefer to keep them separate. There are quite a lot of functions in OptionDataTuple which are not appropriate for options, such as operator = for string etc. So, with the current implementation of the tuple, we shouldn't really do that.

src/lib/dhcp/opaque_data_tuple.cc

OpaqueDataTuble::pack()
Suggest parthentheses around 1 << (getDataFieldSize() * 8).
It is not immediately clear which operators takes precedence. Cool way
to calculate the max size, though.

Added braces.

append(): I'm thinking whether it should throw if the total length
after append is over limit (256 or 65536 bytes). That would pick up
any attempts earlier. Otherwise the size overflow will be detected
only in pack(), which may be in completely different place in the
code.

I was thinking about it but there are quite a few functions which set the data and for each of them I'd need to validate. I can certainly do that but I am wondering about the benefit. If you are completely sure that it is required, I can add the call to len().

src/lib/dhcp/tests/opaque_data_tuple_unittest.cc
Line 101: thet => that

Fixed.

Comment for OpaqueDataTuple.equals() should mention that the
assignment operator is tested.

Added.

Line 173 (tuple = "xyz";) could use EXPECT_NO_THROW().

Added here and in a couple of other places.

OpaqueDataTuple.pack2Bytes should check that trying to assign a buffer
over 65535 bytes will throw.

It checks now.

Comment for OpaqueDataTuple.unpack1ByteTruncatedBuffer:
if => is

Fixed.

The unit-tests are very thorough. Good work.

Thanks. I got sweat when I was writing them.

src/lib/dhcp/option_definition.cc
Changes in factorySpecialFormatOption(): I must admit that the
vendor-related option names are confusing. Do you think it would be ok
to add the option codes in comments? This is an honest question. Feel
free to ignore it.

Added option codes in the comments inside the function body.

src/lib/dhcp/option_vendor_class.h
Please mention option codes (that actual values) in class comment. I
confuse vendor options all the time. I'm sure I'm not the only one :(

Added.

Shouldn't the TuplesCollection type be moved to opaque_data_tuple.h?
There may be other places that will operate on tuple collections.

It could. But why should I mandate the use of the vector as a collection of tuples? If there is some other class that needs a different type of container (e.g. a list or other proprietary container) I should let it have it. Also, at the moment we have just one class using tuples so there is no urgency in having it in the common place.

src/lib/dhcp/option_vendor_class.cc

getTuple()
exception thrown could also say how many tuples are there.

Extended the exception string.

OptionVendorClass::toText() - Thanks for implementing this method, but
I was thinking about something slightly simpler: enterprise-id printed
as a regular decimal number, followed by a coma and concatenated
values, perhaps separated by space or comas. Once we get
user-configurable classification I was thinking about user being able
to say the following:

if (vendor-class==4491,docsis3) ... "

Here's an example what ISC-DHCP can do. We're not going to provide
exactly the same capability in Kea, but we should try to get as close
as reasonably possible:

https://pubs.vmware.com/vsphere-4-esx-vcenter/index.jsp?topic=/com.vmware.vsphere.installclassic.doc_41/install/boot_esx_install/c_sample_dhcp.html

It seems that modifying the !OptionVendorClass::toText() is not enough as we will need to have similar capability for other options. Also, the toText() function for other options presents data fields using data=value notation, so I don't really see a point to make it differently for OptionVendorClass. If we want to have something returning the text in the parsable format we should add a different method.

src/lib/dhcp/tests/option_vendor_class_unittest.cc

OptionVendorClass.constructor4(). Thanks for explaining the reasons
why v4 constructors adds an empty tuple over jabber. Perhaps adding a
reference to RFC3925, section 3 would be useful here? Or maybe for
such a comment would be the constructor itself...

I added a comment into the constructor.

Some explanation about differences between unpack4NoTuple and
unpack6NoTuple would be useful. If you add a comment to the
constructor, a simple "see OptionVendorClass constructor for
explanation" would be sufficient.

Added explanation.


Finally, a general comment. This bug (two bugs, actually) was discovered
by the fact that the tests send short vendor-class option. The first
bug was that our code was not able to parse specific option. You fixed
that bug. There is another bug - if for whatever reason we can't parse
an option, we should ignore just that option, not the whole message.

This has to be fixed. I don't have any preference if this is done in
this ticket or as a separate ticket.

Note that it is not a problem specific to OptionVendorClass. We have plenty of other options - for some of them we should drop the packet, for some of them just an option. Also, some of the errors in parsing options may result in inability to parse the rest of the packet. There is a question of what we should do in such case.

All in all, the problem is much more complex in my opinion and we should not be trying to solve it here. One of the possible ways would be to define different types of exceptions for options, e.g. OptionFatalError, OptionNonFatalError or so, to inform the caller whether it should drop a packet or just an option.


The ChangeLog? entry looks almost fine, but it should start with b10-dhcp4,
b10-dhcp6 as the v4 implementation is also affected.

I can't add b10-dhcp4 because the text is mostly about fixing the bug that we discovered in b10-dhcp6. So I added the last sentence which explains that b10-dhcp4 is also affected.

XXX.	[bug]		marcin
	b10-dhcp6: server parses DHCPv6 Vendor Class option. Previously
	the server failed to parse Vendor Class option having empty opaque
	data field because of the invalid definition in libdhcp++. The
	DHCPv6 Vendor Class option and DHCPv4 V-I Vendor Class option is
	now represented by the new OptionVendorClass. The b10-dhcp4 is
	affected by this change such that it uses new class to parse the
	DHCPv4 V-I Vendor Class option.
	(Trac #3316, git abcd)

comment:11 in reply to: ↑ 10 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 4 to 3
  • Owner changed from tomek to marcin
  • Total Hours changed from 29 to 32

Replying to marcin:

Replying to tomek:
Added. As we agreed on jabber, you'll need to provide some text about why we're doing it.

There's slight complication. The section about client classification is on master, but not on trac3316. We'll decide what to do with it over jabber.

Do we need OpaqueDataTuple::Buffer type? Can't you just use OptionBuffer?
If you don't have strong opinion on this one, I'd prefer to keep the
number of new types to minimum.

We could, but the OptionBuffer is defined in Option class and I would need to include the option.h in the opaque_data_tuple.h which would cause a circular dependency. Or, I could do forward declaration which I dislike, as it causes troubles. Alternatively, I could move the OptionBuffer? definition to a separate header file for this typedef. I think it is too much burden. And in fact, the OpaqueDataTuple is independent from any !Option class and I prefer it to remain that way.

Ok. Let the code stay as it is now.

append() instead of saying that the behavior is undefined if there is
too much data added, it would be better to just throw an exception.

I made it a template function so as I can provide any type of iterator as function, including a raw pointer. For raw pointers I have no means to check whether the length is correct or not. That is the down side of using non-explicit data type but I think it has more benefits.

I see. I never hid the fact that I'm not a great enthusiast of templates...
Let the code stay as it is now.

Shouldn't the TuplesCollection type be moved to opaque_data_tuple.h?
There may be other places that will operate on tuple collections.

It could. But why should I mandate the use of the vector as a collection of tuples? If there is some other class that needs a different type of container (e.g. a list or other proprietary container) I should let it have it. Also, at the moment we have just one class using tuples so there is no urgency in having it in the common place.

If I did the code, I would define a type in opaque_data_tuple.h and encourage others to use that type (and change it a different container, if needed). But that's a matter of personal preference. So let's keep the code as it is now.

It seems that modifying the !OptionVendorClass::toText() is not enough as we will need to have similar capability for other options. Also, the toText() function for other options presents data fields using data=value notation, so I don't really see a point to make it differently for OptionVendorClass. If we want to have something returning the text in the parsable format we should add a different method.

I'm afraid that is true. So we will probably need to add a string getTextValue() or something similar to all classes. But that is definitely not something we should consider now. We'll discuss that during F2F.

Finally, a general comment. This bug (two bugs, actually) was discovered
by the fact that the tests send short vendor-class option. The first
bug was that our code was not able to parse specific option. You fixed
that bug. There is another bug - if for whatever reason we can't parse
an option, we should ignore just that option, not the whole message.

This has to be fixed. I don't have any preference if this is done in
this ticket or as a separate ticket.

Note that it is not a problem specific to OptionVendorClass. We have plenty of other options - for some of them we should drop the packet, for some of them just an option. Also, some of the errors in parsing options may result in inability to parse the rest of the packet. There is a question of what we should do in such case.

All in all, the problem is much more complex in my opinion and we should not be trying to solve it here. One of the possible ways would be to define different types of exceptions for options, e.g. OptionFatalError, OptionNonFatalError or so, to inform the caller whether it should drop a packet or just an option.

Fair enough. Let's discuss this on a meeting tomorrow. I think the fact that we can receive a single option that we can't parse causes us to drop the whole packet is an issue. One way or the other, it is definitely a matter for separate ticket.

XXX.	[bug]		marcin
	b10-dhcp6: server parses DHCPv6 Vendor Class option. Previously
	the server failed to parse Vendor Class option having empty opaque
	data field because of the invalid definition in libdhcp++. The
	DHCPv6 Vendor Class option and DHCPv4 V-I Vendor Class option is
	now represented by the new OptionVendorClass. The b10-dhcp4 is
	affected by this change such that it uses new class to parse the
	DHCPv4 V-I Vendor Class option.
	(Trac #3316, git abcd)

That looks fine.

I've committed a change regarding VENDOR_CLASS prefix. It is now added to all classes, in v4 and v6. Please review. If you think it is ok, please merge. If not, pass the ticket back to me.

I think we can add the client classification doc update as a separate, super-small ticket.

comment:12 Changed 6 years ago by marcin

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

Merged as 1e61d7db5b8dc76682aa568cd62bfae0eeff46e3

Note: See TracTickets for help on using tickets.