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:  2             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  2
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * hours:  0 => 2
 * owner:  marcin => wlodekwencel
 * totalhours:  0 => 2


Comment:

 Reviewed commit dbf249256db006eef0111cff5dec4e644706ce54

 You're missing space between your name and last name in git commits:
 {{{
 commit dbf249256db006eef0111cff5dec4e644706ce54
 Author: WlodekWencel <wlodek at isc.org>
 Date:   Tue Jan 14 19:22:48 2014 +0100

     [2892] removed exception, copyright date changed

 }}}

 '''src/bin/dhcp6/dhcp6_messages.mes'''
 IMHO, there is no need to refer to server identifier option as
 !''ServerID!''. The name of the option, per RFC3315, is !''Server
 Identifier!''. You could use this name or shorten it to !''server id!''.

 It would be useful for the person looking at logs to know what happened
 with the packet that had mismatched server identifier. From the log
 message it is not clear whether we process this packet or drop it. I
 suggest the message be reworded to:

 {{{
 % DHCP6_PACKET_MISMATCH_SERVERID_DROP dropping packet %1 (transid=%2,
 interface=%3) having mismatched server identifier
 }}}

 '''src/bin/dhcp6/dhcp6_srv.cc'''
 testServerID: I think there is no need to call ''!Pkt6::getOptions'' to
 check the number of server identifier options. We already do it in
 ''sanityCheck''. At the same time, the call to ''!Pkt6::getOptions''
 creates the local collection of server identifiers and returns it by value
 which makes the whole testServerID a little bit inefficient. I suggest
 that you simply use the ''!Pkt6::getOption'' to get the server identifier
 and if it is NULL, server identifier is not there (packet should be
 accepted), if non NULL take this server identifier and test it. If there
 are multiple server identifiers you check the first one (returned by
 ''getOption'') and if it matches you let ''sanityCheck'' deal with that.

 I suggest that todo comment is reworded:
 {{{
 /// @todo Currently we always check server identifier regardless if
 /// it is allowed in the received message or not (per RFC3315).
 /// If the server identifier is not allowed in the message, the
 /// sanityCheck function should deal with it. We may rethink this
 /// design if we decide that it is appropriate to check at this stage
 /// of message processing that the server identifier must or must not
 /// be present. In such case however, the logic checking server id
 /// will have to be removed from sanityCheck and placed here instead,
 /// to avoid duplicate checks.
 }}}

 The returned objects should be enclosed in brackets (bind10 coding
 guidelines):
 {{{
 return (false);
 }}}

 Typo in the comment:
 {{{
 // retunr True if: no serverid received or ServerIDs matching
 }}}

 Dhcpv6::run: language errors in !''Check received query if it carry
 ServerID that matches ServerID that beeing used by the server!''.

 It should rather be: !''Check if received query carries server identifier
 matching server identifier being used by the server!''.

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

 testServerID: The @return tag is wrong. It should tell when the function
 returns !''true!'' and when it returns !''false!''. For example:
 {{{
 /// @return true if server id carried in the query matches server id
 /// used by the server; false otherwise.
 }}}

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 language errors in description of the !Dhcpv6SrvTest::testServerID.
 Suggest that it is
 {{{
 // Check that the server is testing if server identifier received in the
 // query, matches server identifier used by the server.
 }}}

 Expected space between the bracket (see below):
 {{{
 TEST_F(Dhcpv6SrvTest, testServerID) {
 }}}

 The following comment:
 {{{
 // server-id is MANDATORY in REQUEST
 // but add there something else
 }}}

 is confusing. I read it: !''server id is mandatory in request but let's
 add some other option!''. In fact you don't add something else but you do
 add server id but with a value that doesn't match server's server id.

 All comments should have a space after opening a comment. So for example:
 {{{
 // diud_llt with time = 0, macaddress = 00:00:00:00:00:00
 }}}

 I would also modify this comment a little bit:
 {{{
 // server-id is FORBIDDEN in SOLICIT, so check if server is not
 // dropping corect message
 }}}

 Maybe something like this:
 {{{
 /// server-id MUST NOT appear in Solicit, so check if server is
 /// not dropping a message without server id.
 }}}


 Again, there is no reason to refer to server identifier as ServerID.
 Rather !''server id!'' or !''server identifier!''.

 '''src/lib/dhcpsrv/utils.h'''
 The exception has been removed, but the copyright date remains set to
 2014, which is wrong after removing an exception.

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