Rework in which state to add and not add the server-ip and

requested-ip option as well as setting ciaddr.

This started with joel@ pointing out that their CPE is ignoring
RENEWING and REBINDING requests when ciaddr was not set.

RFC 2131 4.3.6, Table 4 has a good overview, we got a bunch of it
wrong.

Previously the logic for this was all over the place which made it
difficult to reason about, it is now contained in the engine process
in request_dhcp_request() and request_dhcp_discover().

Problem pointed out by, lots of testing and review as well as OK joel@
Additional testing and 50% review benno@

diff --git a/sbin/dhcpleased/dhcpleased.h b/sbin/dhcpleased/dhcpleased.h
index b3b4938..02121c1 100644 (file)
--- a/sbin/dhcpleased/dhcpleased.h
+++ b/sbin/dhcpleased/dhcpleased.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: dhcpleased.h,v 1.11 2021/08/12 12:41:08 florian Exp $ */
+/*     $OpenBSD: dhcpleased.h,v 1.12 2021/12/09 16:20:12 florian Exp $ */
 
 /*
  * Copyright (c) 2017, 2021 Florian Obser <florian@openbsd.org>
@@ -287,15 +287,10 @@ struct imsg_dhcp {
        uint8_t                 packet[1500];
 };
 
-
-struct imsg_req_discover {
-       uint32_t                if_index;
-       uint32_t                xid;
-};
-
-struct imsg_req_request {
+struct imsg_req_dhcp {
        uint32_t                if_index;
        uint32_t                xid;
+       struct in_addr          ciaddr;
        struct in_addr          requested_ip;
        struct in_addr          server_identifier;
        struct in_addr          dhcp_server;

diff --git a/sbin/dhcpleased/engine.c b/sbin/dhcpleased/engine.c
index 17e65fb..09867c5 100644 (file)
--- a/sbin/dhcpleased/engine.c
+++ b/sbin/dhcpleased/engine.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: engine.c,v 1.29 2021/11/14 18:13:19 florian Exp $     */
+/*     $OpenBSD: engine.c,v 1.30 2021/12/09 16:20:12 florian Exp $     */
 
 /*
  * Copyright (c) 2017, 2021 Florian Obser <florian@openbsd.org>
@@ -1170,6 +1170,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp)
                        return;
                }
                iface->server_identifier.s_addr = server_identifier.s_addr;
+               iface->dhcp_server.s_addr = server_identifier.s_addr;
                iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
                state_transition(iface, IF_REQUESTING);
                break;
@@ -1228,6 +1229,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp)
 
                clock_gettime(CLOCK_MONOTONIC, &iface->request_time);
                iface->server_identifier.s_addr = server_identifier.s_addr;
+               iface->dhcp_server.s_addr = server_identifier.s_addr;
                iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr;
                iface->mask.s_addr = subnet_mask.s_addr;
 #ifndef SMALL
@@ -1354,11 +1356,8 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
        case IF_REBOOTING:
                if (old_state == IF_REBOOTING)
                        iface->timo.tv_sec *= 2;
-               else {
-                       /* make sure we send broadcast */
-                       iface->dhcp_server.s_addr = INADDR_ANY;
+               else
                        iface->timo.tv_sec = START_EXP_BACKOFF;
-               }
                request_dhcp_request(iface);
                break;
        case IF_REQUESTING:
@@ -1377,10 +1376,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
                break;
        case IF_RENEWING:
                if (old_state == IF_BOUND) {
-                       iface->dhcp_server.s_addr =
-                           iface->server_identifier.s_addr;
-                       iface->server_identifier.s_addr = INADDR_ANY;
-
                        iface->timo.tv_sec = (iface->rebinding_time -
                            iface->renewal_time) / 2; /* RFC 2131 4.4.5 */
                } else
@@ -1392,7 +1387,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
                break;
        case IF_REBINDING:
                if (old_state == IF_RENEWING) {
-                       iface->dhcp_server.s_addr = INADDR_ANY;
                        iface->timo.tv_sec = (iface->lease_time -
                            iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */
                } else
@@ -1470,28 +1464,90 @@ iface_timeout(int fd, short events, void *arg)
 void
 request_dhcp_discover(struct dhcpleased_iface *iface)
 {
-       struct imsg_req_discover         imsg_req_discover;
+       struct imsg_req_dhcp     imsg;
 
-       imsg_req_discover.if_index = iface->if_index;
-       imsg_req_discover.xid = iface->xid = arc4random();
-       engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg_req_discover,
-           sizeof(imsg_req_discover));
+       memset(&imsg, 0, sizeof(imsg));
+
+       iface->xid = arc4random();
+       imsg.if_index = iface->if_index;
+       imsg.xid = iface->xid;
+
+       /*
+        * similar to RFC 2131 4.3.6, Table 4 for DHCPDISCOVER
+        * ------------------------------
+        * |              | INIT         |
+        * ------------------------------
+        * |broad/unicast | broadcast    |
+        * |server-ip     | MUST NOT     |
+        * |requested-ip  | MAY          |
+        * |ciaddr        | zero         |
+        * ------------------------------
+        *
+        * Leaving everything at 0 from the memset results in this table with
+        * requested-ip not set.
+       */
+
+       engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg, sizeof(imsg));
 }
 
 void
 request_dhcp_request(struct dhcpleased_iface *iface)
 {
-       struct imsg_req_request  imsg_req_request;
+       struct imsg_req_dhcp     imsg;
 
        iface->xid = arc4random();
-       imsg_req_request.if_index = iface->if_index;
-       imsg_req_request.xid = iface->xid;
-       imsg_req_request.server_identifier.s_addr =
-           iface->server_identifier.s_addr;
-       imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
-       imsg_req_request.dhcp_server.s_addr =  iface->dhcp_server.s_addr;
-       engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg_req_request,
-           sizeof(imsg_req_request));
+       imsg.if_index = iface->if_index;
+       imsg.xid = iface->xid;
+
+       /*
+        * RFC 2131 4.3.6, Table 4
+        * ---------------------------------------------------------------------
+        * |              |REBOOTING    |REQUESTING   |RENEWING     |REBINDING |
+        * ---------------------------------------------------------------------
+        * |broad/unicast |broadcast    |broadcast    |unicast      |broadcast |
+        * |server-ip     |MUST NOT     |MUST         |MUST NOT     |MUST NOT  |
+        * |requested-ip  |MUST         |MUST         |MUST NOT     |MUST NOT  |
+        * |ciaddr        |zero         |zero         |IP address   |IP address|
+        * ---------------------------------------------------------------------
+       */
+       switch (iface->state) {
+       case IF_DOWN:
+               fatalx("invalid state IF_DOWN in %s", __func__);
+               break;
+       case IF_INIT:
+               fatalx("invalid state IF_INIT in %s", __func__);
+               break;
+       case IF_BOUND:
+               fatalx("invalid state IF_BOUND in %s", __func__);
+               break;
+       case IF_REBOOTING:
+               imsg.dhcp_server.s_addr = INADDR_ANY;           /* broadcast */
+               imsg.server_identifier.s_addr = INADDR_ANY;     /* MUST NOT */
+               imsg.requested_ip = iface->requested_ip;        /* MUST */
+               imsg.ciaddr.s_addr = INADDR_ANY;                /* zero */
+               break;
+       case IF_REQUESTING:
+               imsg.dhcp_server.s_addr = INADDR_ANY;           /* broadcast */
+               imsg.server_identifier =
+                   iface->server_identifier;                   /* MUST */
+               imsg.requested_ip = iface->requested_ip;        /* MUST */
+               imsg.ciaddr.s_addr = INADDR_ANY;                /* zero */
+               break;
+       case IF_RENEWING:
+               imsg.dhcp_server = iface->dhcp_server;          /* unicast */
+               imsg.server_identifier.s_addr = INADDR_ANY;     /* MUST NOT */
+               imsg.requested_ip.s_addr = INADDR_ANY;          /* MUST NOT */
+               imsg.ciaddr = iface->requested_ip;              /* IP address */
+               break;
+       case IF_REBINDING:
+               imsg.dhcp_server.s_addr = INADDR_ANY;           /* broadcast */
+               imsg.server_identifier.s_addr = INADDR_ANY;     /* MUST NOT */
+               imsg.requested_ip.s_addr = INADDR_ANY;          /* MUST NOT */
+               imsg.ciaddr = iface->requested_ip;              /* IP address */
+               break;
+       }
+
+       engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg, sizeof(imsg));
 }
 
 void

diff --git a/sbin/dhcpleased/frontend.c b/sbin/dhcpleased/frontend.c
index bd1e37f..57e60c5 100644 (file)
--- a/sbin/dhcpleased/frontend.c
+++ b/sbin/dhcpleased/frontend.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: frontend.c,v 1.24 2021/11/20 17:54:40 florian Exp $   */
+/*     $OpenBSD: frontend.c,v 1.25 2021/12/09 16:20:12 florian Exp $   */
 
 /*
  * Copyright (c) 2017, 2021 Florian Obser <florian@openbsd.org>
@@ -70,6 +70,7 @@ struct iface {
        struct imsg_ifinfo       ifinfo;
        int                      send_discover;
        uint32_t                 xid;
+       struct in_addr           ciaddr;
        struct in_addr           requested_ip;
        struct in_addr           server_identifier;
        struct in_addr           dhcp_server;
@@ -90,10 +91,10 @@ int          get_xflags(char *);
 struct iface   *get_iface_by_id(uint32_t);
 void            remove_iface(uint32_t);
 void            set_bpfsock(int, uint32_t);
+void            iface_data_from_imsg(struct iface*, struct imsg_req_dhcp *);
 ssize_t                 build_packet(uint8_t, char *, uint32_t, struct ether_addr *,
-                    struct in_addr *, struct in_addr *);
-void            send_discover(struct iface *);
-void            send_request(struct iface *);
+                    struct in_addr *, struct in_addr *, struct in_addr *);
+void            send_packet(uint8_t, struct iface *);
 void            bpf_send_packet(struct iface *, uint8_t *, ssize_t);
 int             udp_send_packet(struct iface *, uint8_t *, ssize_t);
 #ifndef SMALL
@@ -480,39 +481,39 @@ frontend_dispatch_engine(int fd, short event, void *bula)
                        break;
 #endif /* SMALL */
                case IMSG_SEND_DISCOVER: {
-                       struct imsg_req_discover         imsg_req_discover;
-                       if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_discover))
+                       struct imsg_req_dhcp     imsg_req_dhcp;
+                       if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
                                fatalx("%s: IMSG_SEND_DISCOVER wrong "
                                    "length: %lu", __func__,
                                    IMSG_DATA_SIZE(imsg));
-                       memcpy(&imsg_req_discover, imsg.data,
-                           sizeof(imsg_req_discover));
-                       iface = get_iface_by_id(imsg_req_discover.if_index);
-                       if (iface != NULL) {
-                               iface->xid = imsg_req_discover.xid;
-                               send_discover(iface);
-                       }
+                       memcpy(&imsg_req_dhcp, imsg.data,
+                           sizeof(imsg_req_dhcp));
+
+                       iface = get_iface_by_id(imsg_req_dhcp.if_index);
+
+                       if (iface == NULL)
+                               break;
+
+                       iface_data_from_imsg(iface, &imsg_req_dhcp);
+                       send_packet(DHCPDISCOVER, iface);
                        break;
                }
                case IMSG_SEND_REQUEST: {
-                       struct imsg_req_request  imsg_req_request;
-                       if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_request))
+                       struct imsg_req_dhcp     imsg_req_dhcp;
+                       if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
                                fatalx("%s: IMSG_SEND_REQUEST wrong "
                                    "length: %lu", __func__,
                                    IMSG_DATA_SIZE(imsg));
-                       memcpy(&imsg_req_request, imsg.data,
-                           sizeof(imsg_req_request));
-                       iface = get_iface_by_id(imsg_req_request.if_index);
-                       if (iface != NULL) {
-                               iface->xid = imsg_req_request.xid;
-                               iface->requested_ip.s_addr =
-                                   imsg_req_request.requested_ip.s_addr;
-                               iface->server_identifier.s_addr =
-                                   imsg_req_request.server_identifier.s_addr;
-                               iface->dhcp_server.s_addr =
-                                   imsg_req_request.dhcp_server.s_addr;
-                               send_request(iface);
-                       }
+                       memcpy(&imsg_req_dhcp, imsg.data,
+                           sizeof(imsg_req_dhcp));
+
+                       iface = get_iface_by_id(imsg_req_dhcp.if_index);
+
+                       if (iface == NULL)
+                               break;
+
+                       iface_data_from_imsg(iface, &imsg_req_dhcp);
+                       send_packet(DHCPREQUEST, iface);
                        break;
                }
                default:
@@ -885,10 +886,20 @@ bpf_receive(int fd, short events, void *arg)
        }
 }
 
+void
+iface_data_from_imsg(struct iface* iface, struct imsg_req_dhcp *imsg)
+{
+       iface->xid = imsg->xid;
+       iface->ciaddr.s_addr = imsg->ciaddr.s_addr;
+       iface->requested_ip.s_addr = imsg->requested_ip.s_addr;
+       iface->server_identifier.s_addr = imsg->server_identifier.s_addr;
+       iface->dhcp_server.s_addr = imsg->dhcp_server.s_addr;
+}
+
 ssize_t
 build_packet(uint8_t message_type, char *if_name, uint32_t xid,
-    struct ether_addr *hw_address, struct in_addr *requested_ip,
-    struct in_addr *server_identifier)
+    struct ether_addr *hw_address, struct in_addr *ciaddr, struct in_addr
+    *requested_ip, struct in_addr *server_identifier)
 {
        static uint8_t   dhcp_cookie[] = DHCP_COOKIE;
        static uint8_t   dhcp_message_type[] = {DHO_DHCP_MESSAGE_TYPE, 1,
@@ -926,6 +937,7 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid,
        hdr->hops = 0;
        hdr->xid = xid;
        hdr->secs = 0;
+       hdr->ciaddr.s_addr = ciaddr->s_addr;
        memcpy(hdr->chaddr, hw_address, sizeof(*hw_address));
        p += sizeof(struct dhcp_hdr);
        memcpy(p, dhcp_cookie, sizeof(dhcp_cookie));
@@ -966,20 +978,20 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid,
        memcpy(p, dhcp_req_list, sizeof(dhcp_req_list));
        p += sizeof(dhcp_req_list);
 
-       if (message_type == DHCPREQUEST) {
+       if (requested_ip->s_addr != INADDR_ANY) {
                memcpy(dhcp_requested_address + 2, requested_ip,
                    sizeof(*requested_ip));
                memcpy(p, dhcp_requested_address,
                    sizeof(dhcp_requested_address));
                p += sizeof(dhcp_requested_address);
+       }
 
-               if (server_identifier->s_addr != INADDR_ANY) {
-                       memcpy(dhcp_server_identifier + 2, server_identifier,
-                           sizeof(*server_identifier));
-                       memcpy(p, dhcp_server_identifier,
-                           sizeof(dhcp_server_identifier));
-                       p += sizeof(dhcp_server_identifier);
-               }
+       if (server_identifier->s_addr != INADDR_ANY) {
+               memcpy(dhcp_server_identifier + 2, server_identifier,
+                   sizeof(*server_identifier));
+               memcpy(p, dhcp_server_identifier,
+                   sizeof(dhcp_server_identifier));
+               p += sizeof(dhcp_server_identifier);
        }
 
        *p = DHO_END;
@@ -995,7 +1007,7 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid,
 }
 
 void
-send_discover(struct iface *iface)
+send_packet(uint8_t message_type, struct iface *iface)
 {
        ssize_t                  pkt_len;
        char                     ifnamebuf[IF_NAMESIZE], *if_name;
@@ -1004,27 +1016,17 @@ send_discover(struct iface *iface)
                iface->send_discover = 1;
                return;
        }
-       iface->send_discover = 0;
-
-       if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
-       log_debug("DHCPDISCOVER on %s", if_name == NULL ? "?" : if_name);
 
-       pkt_len = build_packet(DHCPDISCOVER, if_name, iface->xid,
-           &iface->ifinfo.hw_address, &iface->requested_ip, NULL);
-       bpf_send_packet(iface, dhcp_packet, pkt_len);
-}
+       iface->send_discover = 0;
 
-void
-send_request(struct iface *iface)
-{
-       ssize_t                  pkt_len;
-       char                     ifnamebuf[IF_NAMESIZE], *if_name;
+       if ((if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf)) == NULL)
+               return; /* iface went away, nothing to do */
 
-       if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf);
-       log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name);
+       log_debug("%s on %s", message_type == DHCPDISCOVER ? "DHCPDISCOVER" :
+           "DHCPREQUEST", if_name);
 
-       pkt_len = build_packet(DHCPREQUEST, if_name, iface->xid,
-           &iface->ifinfo.hw_address, &iface->requested_ip,
+       pkt_len = build_packet(message_type, if_name, iface->xid,
+           &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip,
            &iface->server_identifier);
        if (iface->dhcp_server.s_addr != INADDR_ANY) {
                if (udp_send_packet(iface, dhcp_packet, pkt_len) == -1)
@@ -1162,7 +1164,7 @@ set_bpfsock(int bpfsock, uint32_t if_index)
                    EV_PERSIST, bpf_receive, iface);
                event_add(&iface->bpfev.ev, NULL);
                if (iface->send_discover)
-                       send_discover(iface);
+                       send_packet(DHCPDISCOVER, iface);
        }
 }