If we agree I correctly interpreted the part of RFC in previous mail, that patch should be included too? BR Justin
On Friday, March 24, 2017 at 7:59:27 PM UTC+1, Waldek Kozaczuk wrote: > > I tested this extras compliance patch against InfoBlox and in AWS and it > works. > > Sent from my iPhone > > On Mar 23, 2017, at 07:45, Waldek Kozaczuk <[email protected] > <javascript:>> wrote: > > I will review and test today. > > Sent from my iPhone > > On Mar 23, 2017, at 04:16, Nadav Har'El <[email protected] <javascript:>> > wrote: > > Thanks. I'm really not versed enough in the DHCP RFC to make much sense > out of this. Waldemar, can you please review? Thanks. > > > -- > Nadav Har'El > [email protected] <javascript:> > > On Thu, Mar 23, 2017 at 12:41 AM, Justin Cinkelj <[email protected] > <javascript:>> wrote: > >> The patch works OK for me, tested agains dnsmasq. >> >> One possible improvements. From RFC 2131, page 30-31 >> o DHCPREQUEST generated during SELECTING state: >> Client inserts the address of the selected server in 'server >> identifier', 'ciaddr' MUST be zero, 'requested IP address MUST be >> filled in with the yiaddr value from the chosen DHCPOFFER. >> >> o DHCPREQUEST generated during RENEWING state: >> 'server identifier' MUST NOT be filled in, 'requested IP address' >> option MUST NOT be filled in, 'ciaddr' MUST be filled in with >> client's IP address. >> >> I understand "'server identifier'/'requested IP address' MUST NOT be >> filled in" >> as instruction to not include those two DHCP OPTIONS into >> RENEWING/REBINDING DHCPREQUEST. >> So I tried to add additional change, as an attempt to be more RFC >> compliant. >> After that change, code still works - well, at least against dnsmasq. >> >> diff --git a/core/dhcp.cc b/core/dhcp.cc >> index 12f27bb..5d3ec9a 100644 >> --- a/core/dhcp.cc >> +++ b/core/dhcp.cc >> @@ -253,12 +253,16 @@ namespace dhcp { >> ip::address_v4::bytes_type dhcp_server_ip = sip.to_bytes(); >> ip::address_v4::bytes_type requested_ip = yip.to_bytes(); >> options = add_option(options, DHCP_OPTION_MESSAGE_TYPE, 1, >> DHCP_MT_REQUEST); >> - options = add_option(options, DHCP_OPTION_DHCP_SERVER, 4, >> (u8*)&dhcp_server_ip); >> + if(request_packet_type == DHCP_REQUEST_SELECTING) { >> + options = add_option(options, DHCP_OPTION_DHCP_SERVER, 4, >> (u8*)&dhcp_server_ip); >> + } >> char hostname[256]; >> if (0 == gethostname(hostname, sizeof(hostname))) { >> options = add_option(options, DHCP_OPTION_HOSTNAME, >> strlen(hostname), (u8*)hostname); >> } >> - options = add_option(options, DHCP_OPTION_REQUESTED_ADDRESS, 4, >> (u8*)&requested_ip); >> + if(request_packet_type == DHCP_REQUEST_SELECTING) { >> + options = add_option(options, DHCP_OPTION_REQUESTED_ADDRESS, >> 4, (u8*)&requested_ip); >> + } >> options = add_option(options, DHCP_OPTION_PARAMETER_REQUEST_LIST, >> sizeof(requested_options), requested_options); >> *options++ = DHCP_OPTION_END; >> >> >> >> ----- Original Message ----- >> > From: "Waldek Kozaczuk" <[email protected] <javascript:>> >> > To: "Nadav Har'El" <[email protected] <javascript:>> >> > Cc: "justinc1" <[email protected] <javascript:>>, "Osv Dev" < >> [email protected] <javascript:>> >> > Sent: Wednesday, March 22, 2017 2:54:43 PM >> > Subject: Re: [PATCH] Changed DHCP client logic to properly broadcast >> DHCPREQUEST during >> > >> > I forgot to mention that I tested this patch on both VMware with >> InfoBlox >> > DHCP server and EC2. >> > >> > Sent from my iPhone >> > >> > > On Mar 22, 2017, at 09:18, Nadav Har'El <[email protected] >> <javascript:>> wrote: >> > > >> > > Thanks. Looks good to me. Justin, can you please also have a look? >> > > >> > > >> > > -- >> > > Nadav Har'El >> > > [email protected] <javascript:> >> > > >> > >> On Wed, Mar 22, 2017 at 2:57 PM, Waldemar Kozaczuk < >> [email protected] <javascript:>> >> > >> wrote: >> > >> From: wkozaczuk <[email protected] <javascript:>> >> > >> >> > >> The patch addressing issue #816 changed dhcp_mbuf::compose_request() >> > >> method to >> > >> make it unicast DHCPREQUEST message to a DHCP server. This is >> incorrect >> > >> per RCF-2131 >> > >> which requires DHCP clients to broadcast messages until IP address is >> > >> bound which >> > >> is a case during RENEWING phase but not SELECTING one. Now the >> > >> dhcp_mbuf::compose_request() >> > >> takes extra enum parameter which specifies which phase (per page 34 >> of >> > >> https://www.ietf.org/rfc/rfc2131.txt) >> > >> the message is being composed for. >> > >> >> > >> Fixes #864 >> > >> >> > >> Signed-off-by: Waldemar Kozaczuk <[email protected] <javascript:>> >> > >> --- >> > >> core/dhcp.cc | 55 >> > >> ++++++++++++++++++++++++++++++++++++++++++++++------- >> > >> include/osv/dhcp.hh | 10 +++++++++- >> > >> 2 files changed, 57 insertions(+), 8 deletions(-) >> > >> >> > >> diff --git a/core/dhcp.cc b/core/dhcp.cc >> > >> index 1b90adb..12f27bb 100644 >> > >> --- a/core/dhcp.cc >> > >> +++ b/core/dhcp.cc >> > >> @@ -81,11 +81,26 @@ namespace dhcp { >> > >> >> > >> const char * dhcp_options_magic = "\x63\x82\x53\x63"; >> > >> >> > >> + static std::map<dhcp_message_type,const char*> >> > >> dhcp_message_type_name_by_id = { >> > >> + {DHCP_MT_DISCOVER, "DHCPDISCOVER"}, >> > >> + {DHCP_MT_OFFER, "DHCPOFFER"}, >> > >> + {DHCP_MT_REQUEST, "DHCPREQUEST"}, >> > >> + {DHCP_MT_DECLINE, "DHCPDECLINE"}, >> > >> + {DHCP_MT_ACK, "DHCPACK"}, >> > >> + {DHCP_MT_NAK, "DHCPNAK"}, >> > >> + {DHCP_MT_RELEASE, "DHCPRELEASE"}, >> > >> + {DHCP_MT_INFORM, "DHCPINFORM"}, >> > >> + {DHCP_MT_LEASEQUERY, "DHCPLEASEQUERY"}, >> > >> + {DHCP_MT_LEASEUNASSIGNED, "DHCPLEASEUNASSIGNED"}, >> > >> + {DHCP_MT_LEASEUNKNOWN, "DHCPLEASEUNKNOWN"}, >> > >> + {DHCP_MT_LEASEACTIVE, "DHCPLEASEACTIVE"}, >> > >> + {DHCP_MT_INVALID, "DHCPINVALID"} >> > >> + }; >> > >> + >> > >> >> /////////////////////////////////////////////////////////////////////////// >> > >> >> > >> bool dhcp_socket::dhcp_send(dhcp_mbuf& packet) >> > >> { >> > >> - >> > >> struct bsd_sockaddr dst = {}; >> > >> struct mbuf *m = packet.get(); >> > >> >> > >> @@ -205,7 +220,8 @@ namespace dhcp { >> > >> void dhcp_mbuf::compose_request(struct ifnet* ifp, >> > >> u32 xid, >> > >> ip::address_v4 yip, >> > >> - ip::address_v4 sip) >> > >> + ip::address_v4 sip, >> > >> + dhcp_request_packet_type >> > >> request_packet_type) >> > >> { >> > >> size_t dhcp_len = sizeof(struct dhcp_packet); >> > >> struct dhcp_packet* pkt = pdhcp(); >> > >> @@ -222,7 +238,11 @@ namespace dhcp { >> > >> memcpy(pkt->chaddr, IF_LLADDR(ifp), ETHER_ADDR_LEN); >> > >> ulong yip_n = htonl(yip.to_ulong()); >> > >> ulong sip_n = htonl(sip.to_ulong()); >> > >> - memcpy(&pkt->ciaddr.s_addr, &yip_n, 4); >> > >> + if(request_packet_type == DHCP_REQUEST_RENEWING || >> > >> request_packet_type == DHCP_REQUEST_REBINDING) { >> > >> + // ciaddr should only be set to if RENEWING or REBINDING >> > >> + // See pages 21 and 30-31 in >> > >> https://www.ietf.org/rfc/rfc2131.txt >> > >> + memcpy(&pkt->ciaddr.s_addr, &yip_n, 4); >> > >> + } >> > >> >> > >> // Options >> > >> u8* options_start = reinterpret_cast<u8*>(pkt+1); >> > >> @@ -244,7 +264,14 @@ namespace dhcp { >> > >> *options++ = DHCP_OPTION_END; >> > >> >> > >> dhcp_len += options - options_start; >> > >> - build_udp_ip_headers(dhcp_len, yip_n, sip_n); >> > >> + >> > >> + // See page 33 in https://www.ietf.org/rfc/rfc2131.txt >> > >> + if(request_packet_type == DHCP_REQUEST_RENEWING) { >> > >> + build_udp_ip_headers(dhcp_len, yip_n, sip_n); >> > >> + } >> > >> + else { >> > >> + build_udp_ip_headers(dhcp_len, INADDR_ANY, >> INADDR_BROADCAST); >> > >> + } >> > >> } >> > >> >> > >> void dhcp_mbuf::compose_release(struct ifnet* ifp, >> > >> @@ -420,6 +447,9 @@ namespace dhcp { >> > >> options += op_len; >> > >> } >> > >> >> > >> + dhcp_i( "Received %s message from DHCP server: %s regarding >> > >> offerred IP address: %s", >> > >> + dhcp_message_type_name_by_id[_message_type], >> > >> _dhcp_server_ip.to_string().c_str(), >> > >> + _your_ip.to_string().c_str()); >> > >> return true; >> > >> } >> > >> >> > >> @@ -519,6 +549,7 @@ namespace dhcp { >> > >> // Save transaction id & send >> > >> _xid = dm.get_xid(); >> > >> _client_addr = _server_addr = ipv4_zero; >> > >> + dhcp_i( "Broadcasting DHCPDISCOVER message with xid: >> [%d]",_xid); >> > >> _sock->dhcp_send(dm); >> > >> } >> > >> >> > >> @@ -533,6 +564,8 @@ namespace dhcp { >> > >> >> > >> // Save transaction id & send >> > >> _xid = dm.get_xid(); >> > >> + dhcp_i( "Unicasting DHCPRELEASE message with xid: [%d] from >> > >> client: %s to server: %s", >> > >> + _xid, _client_addr.to_string().c_str(), >> > >> _server_addr.to_string().c_str()); >> > >> _sock->dhcp_send(dm); >> > >> // IP and routes have to be removed >> > >> osv::stop_if(_ifp->if_xname, >> _client_addr.to_string().c_str()); >> > >> @@ -553,9 +586,13 @@ namespace dhcp { >> > >> _xid = rand(); >> > >> dm.compose_request(_ifp, >> > >> _xid, >> > >> - _client_addr, _server_addr); >> > >> + _client_addr, >> > >> + _server_addr, >> > >> + dhcp_mbuf::DHCP_REQUEST_RENEWING); >> > >> >> > >> // Send >> > >> + dhcp_i( "Unicasting DHCPREQUEST message with xid: [%d] from >> > >> client: %s to server: %s in order to RENEW lease of: %s", >> > >> + _xid, _client_addr.to_string().c_str(), >> > >> _server_addr.to_string().c_str(), _client_addr.to_string().c_str()); >> > >> _sock->dhcp_send(dm); >> > >> } >> > >> >> > >> @@ -600,14 +637,18 @@ namespace dhcp { >> > >> dm_req.compose_request(_ifp, >> > >> _xid, >> > >> dm.get_your_ip(), >> > >> - dm.get_dhcp_server_ip()); >> > >> + dm.get_dhcp_server_ip(), >> > >> + dhcp_mbuf::DHCP_REQUEST_SELECTING); >> > >> + dhcp_i( "Broadcasting DHCPREQUEST message with xid: [%d] to >> > >> SELECT offered IP: %s", >> > >> + _xid, dm.get_your_ip().to_string().c_str()); >> > >> _sock->dhcp_send(dm_req); >> > >> } >> > >> >> > >> void dhcp_interface_state::state_request(dhcp_mbuf &dm) >> > >> { >> > >> if (dm.get_message_type() == DHCP_MT_ACK) { >> > >> - dhcp_i("Server acknowledged IP for interface %s", >> > >> _ifp->if_xname); >> > >> + dhcp_i("Server acknowledged IP %s for interface %s with >> time >> > >> to lease in seconds: %d", >> > >> + dm.get_your_ip().to_string().c_str(), >> _ifp->if_xname, >> > >> dm.get_lease_time_sec()); >> > >> _state = DHCP_ACKNOWLEDGE; >> > >> _client_addr = dm.get_your_ip(); >> > >> _server_addr = dm.get_dhcp_server_ip(); >> > >> diff --git a/include/osv/dhcp.hh b/include/osv/dhcp.hh >> > >> index 3db6df8..6797b7f 100644 >> > >> --- a/include/osv/dhcp.hh >> > >> +++ b/include/osv/dhcp.hh >> > >> @@ -117,6 +117,13 @@ namespace dhcp { >> > >> DHCP_REPLY = 2, >> > >> }; >> > >> >> > >> + enum dhcp_request_packet_type { >> > >> + DHCP_REQUEST_INIT_REBOOT = 1, >> > >> + DHCP_REQUEST_SELECTING = 2, >> > >> + DHCP_REQUEST_RENEWING = 3, >> > >> + DHCP_REQUEST_REBINDING = 4, >> > >> + }; >> > >> + >> > >> dhcp_mbuf(bool attached = true, struct mbuf* m = nullptr); >> > >> ~dhcp_mbuf(); >> > >> >> > >> @@ -129,7 +136,8 @@ namespace dhcp { >> > >> void compose_request(struct ifnet* ifp, >> > >> u32 xid, >> > >> boost::asio::ip::address_v4 yip, >> > >> - boost::asio::ip::address_v4 sip); >> > >> + boost::asio::ip::address_v4 sip, >> > >> + dhcp_request_packet_type >> > >> request_packet_type); >> > >> void compose_release(struct ifnet* ifp, >> > >> boost::asio::ip::address_v4 yip, >> > >> boost::asio::ip::address_v4 sip); >> > >> -- >> > >> 2.7.4 >> > >> >> > >> -- >> > >> You received this message because you are subscribed to the Google >> Groups >> > >> "OSv Development" group. >> > >> To unsubscribe from this group and stop receiving emails from it, >> send an >> > >> email to [email protected] <javascript:>. >> > >> For more options, visit https://groups.google.com/d/optout. >> > > >> > >> > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
