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 <jwkoz...@gmail.com
> <javascript:>> wrote:
>
> I will review and test today.
>
> Sent from my iPhone
>
> On Mar 23, 2017, at 04:16, Nadav Har'El <n...@scylladb.com <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
> n...@scylladb.com <javascript:>
>
> On Thu, Mar 23, 2017 at 12:41 AM, Justin Cinkelj <justin....@xlab.si
> <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" <jwkoz...@gmail.com <javascript:>>
>> > To: "Nadav Har'El" <n...@scylladb.com <javascript:>>
>> > Cc: "justinc1" <justin....@xlab.si <javascript:>>, "Osv Dev" <
>> osv...@googlegroups.com <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 <n...@scylladb.com
>> <javascript:>> wrote:
>> > >
>> > > Thanks. Looks good to me. Justin, can you please also have a look?
>> > >
>> > >
>> > > --
>> > > Nadav Har'El
>> > > n...@scylladb.com <javascript:>
>> > >
>> > >> On Wed, Mar 22, 2017 at 2:57 PM, Waldemar Kozaczuk <
>> jwkoz...@gmail.com <javascript:>>
>> > >> wrote:
>> > >> From: wkozaczuk <jwkoz...@gmail.com <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 <jwkoz...@gmail.com <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 osv-dev+u...@googlegroups.com <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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
