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:  tomek
                Type:  defect        |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcp6         |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  29            |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  4
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * hours:  5 => 4
 * owner:  marcin => tomek
 * totalhours:  25 => 29


Comment:

 Replying to [comment:9 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)
 }}}

 >
 >
 >

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