BIND 10 #3316: Kea not able to parse vendor class option

[BIND 10 Development]

#3316: Kea not able to parse vendor class option
-------------------------------------+-------------------------------------
            Reporter:  wlodekwencel  |                        Owner:
                Type:  defect        |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  25            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  5
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  7 => 5
 * owner:  tomek => marcin
 * totalhours:  20 => 25


Comment:

 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.

-- 
Ticket URL: <https://bind10.isc.org/ticket/3316#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development