BIND 10 #2892: blank client_id/server_id don't causes DHCPv6 server to discard messages

[BIND 10 Development]

#2892: blank client_id/server_id don't causes DHCPv6 server to discard messages
-------------------------------------+-------------------------------------
            Reporter:  wlodekwencel  |                        Owner:
                Type:  defect        |  wlodekwencel
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:  dhcp,         |                    Milestone:  DHCP-
  client_id, server_id               |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:
         Total Hours:  0             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => wlodekwencel


Comment:

 Reviewed commit ec47fdb14d1fd03d386147c1c6cf4afdd118c75f

 A couple of good practices with respect to use of git and code
 development:

 1. Commit descriptions should include the ticket number like this:
 {{{
 [2892] Added a unit test for test serverId function

 }}}

 2. Commit descriptions should not be meaningful. Usually, descriptions are
 limited to one line:
 {{{
 [2892] Add server id checking for DHCPv4 server.
 }}}

 Sometimes, the developer decides to extend the description if he feels
 that this particular commit may be of some interest to others. In this
 case, the one line description comes first, followed by the blank line and
 the detailed description:
 {{{
     [3088] Added DNS request construction to d2::NameRemoveTransaction

     Added methods for constructing all three types of DNS update requests
     required by d2::NameRemoveTransaction to complete the implementation
 of its
     state machine.  Also refactored some unit test code into
 nc_test_utils.h
     and .cc. Renamed request verification functions in nc_test_utils to
 match
     the build request function names.
 }}}

 It doesn't make sense to put commit description like this one:
 {{{
 commit bb339dd52f357cf97a3bfc15d68650d9fe2b577d
 Author: wlodek <wlodek.wencel at gmail.com>
 Date:   Mon Jan 13 21:18:59 2014 +0100

     code update
 }}}

 This is clearly because almost every commit we make is about code update!
 :)

 3. All developers at ISC use full name, e.g. "Marcin Siodelski" (and ISC
 email) for git commits. Suggest that you configure it in your .git/config.

 '''src/bin/dhcp6/dhcp6_srv.h'''

 A year in copytright header should be updated to 2014.

 Is there any reason for the testServerid to be public? The function is
 meant to be called internally by the Dhcpv6Srv class so it should rather
 be in private or protected scope (rather protected assuming that it has to
 be unit tested).

 I also suggest that the ''testServerid'' is renamed to ''testServerId'' or
 ''testServerID''.

 The brief description of the ''testServerid'' is acceptable. I suggest
 that it starts with capital letter, though:
 {{{
 /// @brief Compares received server id with our server id.
 }}}

 Also, it should be !''our!'' not !''ours!''.

 The second part of the function description should be more meaningful. I
 suggest something like this:
 {{{
 /// Checks if the server id carried in a query from a client matches
 /// server identifier being used by the server.
 ///
 /// @param pkt DHCPv6 packet carrying server identifier to be checked.
 }}}

 '''src/bin/dhcp6/dhcp6_srv.cc'''
 testServerid: Instead of negating the equality operator result you could
 simply use inequality operator:
 {{{
 if (getServerID()->getData() != serverid->getData())
     isc_throw(ServerID_mismatch, "Receievd serverid isn't ours");

 }}}

 I don't really see a reason for this function to throw an exception.
 Please note, that it is a normal condition from the server perspective
 that the received packet is for someone else.   Exceptions are typically
 thrown when an error occurs, e.g. a packet being processed is malformed
 etc. The code will be more efficient if this function returns a boolean
 value to indicate that the server id is ours not not.

 This actually leads to another point, which I had.... Why should this
 function be called from ''sanityCheck''?

 The ''sanityCheck'' function (per its documentation) verifies if specified
 packet meets RFC requirements. The fact that the packet is targeted to
 some other server (its server id doesn't match with server id of our
 server), doesn't really mean that the packet doesn't meet the RFC
 requirements!

 Also, if the ''testServerid'' is being called from the ''processSolicit'',
 ''processRequest'' etc., it allows the packet to be processed by the
 !''query6!'' callout, which is called before ''processSolicit'' and
 ''processRequest'' functions. IMHO, the packet with non-matching server id
 should be filtered out before this callout! So, just after this section:
 {{{
         if (!skip_unpack) {
             if (!query->unpack()) {
                 LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL,
                           DHCP6_PACKET_PARSE_FAIL);
                 continue;
             }
         }

 }}}
 If you follow this suggestion, it means that you will have to remove the
 call to ''testServerid'' from the ''sanityCheck'' function and therefore
 there is no need for the ''testServerId'' to throw an exception, but
 simply return the boolean value.

 Also, there should be a debug message logged when the packet is discarded
 because of the server identifier mismatch. The logger message could
 contain the transaction id and interface on which the packet has been
 received.

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 testServerid unit test: The ''testServerid'' function is expected to check
 server identifier only. It doesn't check client id, nor hint. For this
 reason, it should be ok not to set ia, client identifier and hint which
 would make the test code a little more readable.

 This test should check the case when server identifier is not present in
 the query. This is perfectly valid case for Solicit.

 Note that each test has a brief description. This test should also have a
 brief description.

 The copyright header should be updated for all files.

 '''src/lib/dhcpsrv/utils.h'''
 If you modify the ''testServerid'' function to return the boolean value
 instead of thrown an exception there will be no need to keep this new
 exception. However, let me just mention that the names of the exceptions
 should follow this convention: ''ServerIdMismatch''.

 The ticket introduces quite important change to the server's behavior, so
 the !ChangeLog entry should be proposed for it.

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