BIND 10 #2600: DHCP Testing: V4 features - robustness
BIND 10 Development
do-not-reply at isc.org
Thu Mar 21 13:30:37 UTC 2013
#2600: DHCP Testing: V4 features - robustness
-------------------------------------+-------------------------------------
Reporter: stephen | Owner: tmark
Type: task | Status:
Priority: medium | reviewing
Component: dhcp | Milestone:
Keywords: | Sprint-DHCP-20130328
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tmark
Comment:
Reviewed commit 43322ef85b573c27ae669d35dc52d8e06d75a928
'''Gerenal comment''': As in the case of #2607 I would suggest to avoid
using examples of configuration but rather mandate use of the
172.16.1.0/24 subnet and add a comment saying that it can be changed if
this configuration does not fit into the environment used to execute the
test. However, it should be noted that configuration file must be changed
accordingly.
'''v4.valid_only.1''':
(1.3): The following address 172.16.1.1 is reserved to the server, the
172.16.1.4 is reserved for the relay. Thus, the configured address pool
should exclude these addresses so as they are not assigned to the
requesting client. Although, it may not be a harm here because scapy would
not assign any addresses to the interface but as a general rule it is
probably better to avoid situations when the server leases his own address
to the client. Note, that there may be some real clients in this network
and they may !''accidentally!'' request address from the server under
test.
(3.): This section should give more detailed instructions how to run the
client with scapy. Ideally, the exact command line is specified which
redirects the contents of the python script to scapy. Alternatively, the
text could mention that the script contents needs to be copied over to the
interactive shell.
Also, the script assumes that my interface is called !''vlan6!'' which in
most of the time is not the case. There is no step in the test procedure
that instructs to replace the default interface name with my interface
name. Same applies to the address to be configured for the interface.
'''v4.improper_addresses.1.txt'''
The comments for the previous test apply here too.
(Test Cases):
I verified correctness of these test cases by referring to RF2131, section
4.3.2.
The first two test cases are aligned with the paragraph !''DHCPREQUEST
generated during INIT-REBOOT state!'' in this document because:
- server identifier is not filled in the REQUEST message
- requested IP address is filled in
'''My interpretation''' (I am not fully convinced it is right) of this
paragraph is that server MUST not respond if it doesn't recognize the
client. In other words, the lease for this client MUST appear in the
database (even expired lease). If there is no record of the client it is
possible that client is trying to renew lease from different server in the
same network and this server should respond to the request. The following
text in the RFC discusses this:
''If the DHCP server has no record of this client, then it MUST remain
silent, and MAY output a warning to the network administrator. This
behavior is necessary for peaceful coexistence of non-communicating DHCP
servers on the same wire.''
It is true that earlier part of the text instructs that server sends NAK
if the network is incorrect (wrong giaddr) or requested address is
incorrect but in my opinion this only works if there is a record of the
client and you can actually verify that the requested address is wrong.
As to the last test case... The inbound packet in test procedure is
DISCOVER, while in the script it is REQUEST. Which one was intended?
I think it might be worth to add some test cases that involve returning
clients. So, for example:
- scapy initiates normal 4-way exchange and obtains a lease
- scapy initiates REQUEST with valid server id etc. but with invalid
address
'''v4.malformed_packets.1.txt'''
The same comments apply as for the other test procedures.
The interpretation of specific test cases....
(1.) The blank chaddr is in this case 00:00:00:00:00:00. This is in my
opinion valid HW address so the server should respond with OFFER which it
does. Maybe more interesting case would be to include no HW address at
all?
(2.) The giaddr blank is as it was not going through relay. The blank
(zeroed) HW address is a valid HW address so OFFER response is probably
valid. Also, it is a direct, not broadcast traffic coming out from client
but it is allowed by the RFC 2131.
(3.) Client MUST include 'END' option. If it doesn't the packet is
malformed should be probably dropped. This fact could be logged to notify
administrator. It is up to further discussion whether we want the server
to be strict or not.
(4.) In the section 3. of RFC 2131 it is mentioned that client uses opcode
1 and the server uses opcode 2. The RFC does not use !''MUST!'' here so it
is hard to say whether it is a strict requirement. However, based on our
discussion on jabber it may be worth to start with strict rules for our
server and maybe become less strict if required. I would say that this is
malformed packet that should be dropped.
(5.) According to RFC 2131, section 4.3.1. If the requested address is
invalid, server allocates new address from the pool in response to
DISCOVER. So, OFFER is expected response in my opnion.
(6.) By setting flags to 0xFFFF you're setting reserved bits which MUST be
zero according to section 2 of RFC 2131. Thus, the packet is malformed
and should be dropped in my opinion.
(7.) Server was unable to parse the malformed packet so no response is
probably expected behavior.
(8.) According to RFC 3046, section 2.2. the server may be unaware of the
Agent Information Option. Thus, it is not even obliged to echo it back to
the relay. The fact that the sub-option of AI Option is broken is not a
problem because server doesn't have to interpret the option contents so it
will never discover that it is broken. In this case, it is ok that the
server responds with OFFER.
(9.) IMHO, it is ok to request unassigned/non-standard options. Note, that
DHCP implementations allow to define proprietary options. Suppose that
there is new DHCP standard released which introduces option X. The server
implementation may not support this option natively, so the administrator
may want to create option definition using configuration mechanism to
start using it until support is not added into the code.
If option definition is not yet configured it is ok that client asks for
this option anyway. He will get it once it is configured. As long as it is
not configured he will get everything else but this option. So again,
OFFER is ok.
(10.) The minimum length of the Root Path option is 1. In this sense, the
packet is malformed and assuming strict validation rules it should be
dropped.
(11.) Lack of response seems to be ok.
(12.) I would say that server should ignore this request. The client
neither includes requested IP address nor server id. This fits to the case
described in RFC 2131, section 4.3.2, bullet !'' DHCPREQUEST generated
during RENEWING state!''. Unfortunately, this section does not say what
server should do if there is no record of the client (which is the case
here). But hey! If it doesn't have the record of the client then probably
other server had given him a lease he is trying to renew. Then, the best
thing to do is do not respond. NAK is probably inappropriate because it is
used to decline the address being requested. But here, we don't request
address. Server should not respond to it in my opinion.
----
All comments about the accuracy of the server responses are based on my
understanding/interpretation of RFCs. They may be wrong to some extent.
Also, we may want to discuss if we want to be strict in interpretation of
RFCs or not. Being strict means more checks on incoming packets on the
server side which may affect its performance. On the other hand, the
performance doesn't seem to be limited by the CPU utilization so rise of
CPU utilization is probably not a high price. Also, I think that Tomek had
a good point when saying that we should be strict on the beginning and
then maybe become less strict over time if somebody comes up with
legitimate explanation why we shouldn't be strict. So, I would recommend
to modify the test to assume strict rules for now.
----
My understanding of the test development process is that it should follow
the certain sequence of steps:
- Read through the documentation and identify requirements
- Create list of tests that cover these requirements
- Run tests and collect results
- Confront results with the expected results from the requirements
IMHO, all these steps should be taken by the ticket implementor. The
reviewer should only check that there were no mistakes made during this
process and that all required steps have been taken.
With this ticket we have done that differently. You created tests,
executed these tests and collected server responses. Myself (as a
reviewer) checked responses against the RFCs. That way, only one person
(myself) checked what the responses should be while the idea behind doing
reviews is that the one person checks if the other person has done that
right. In our case, it seems that you should now review RFCs to verify my
understanding of RFCs which puts each of us in the position of reviewer
and implementer. :-)
--
Ticket URL: <http://bind10.isc.org/ticket/2600#comment:5>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list