I'm making a fine mess off this :(
On 2021-11-25 10:23 -07, Joel Knight <knight.j...@gmail.com> wrote:
> On Wed, Nov 24, 2021 at 4:46 AM Florian Obser <flor...@openbsd.org> wrote:
>
>> Thanks, I had indeed missed this. I went through the RFC and found that
>> we MUST NOT send the server identifier in rebooting state. While here I
>> also made it explicit that we are not sending server identifier in
>> rebinding state. This was already the case since we only transition from
>> renewing to rebinding, but this is clearer.
>
> The behavior for this option looks consistent with prior behavior.
> However, the RFC confuses me wrt this option and when it should/should
> not be set so I'm only relying on the fact that broadcast and unicast
> requests are working fine in testing.
yeah, I don't get it either
>
>> @@ -1486,9 +1488,17 @@ request_dhcp_request(struct dhcpleased_iface *iface)
>> iface->xid = arc4random();
>> imsg_req_request.if_index = iface->if_index;
>> imsg_req_request.xid = iface->xid;
>> + if (iface->state == IF_RENEWING || iface->state == IF_REBINDING)
>> + imsg_req_request.ciaddr.s_addr = iface->requested_ip.s_addr;
>> + else
>> + imsg_req_request.ciaddr.s_addr = 0;
>> imsg_req_request.server_identifier.s_addr =
>> iface->server_identifier.s_addr;
>> - imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr;
>> + if (iface->state == IF_RENEWING || iface->state == IF_REBINDING)
>> + imsg_req_request.requested_ip.s_addr = 0;
>> + else
>> + imsg_req_request.requested_ip.s_addr =
>> + iface->requested_ip.s_addr;
>
> These two if/else statements could be merged.
>
>> 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 +929,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));
>
> The problem with build_packet() now is that it will unconditionally
> insert opt 50 (requested IP address) even if ciaddr is being set. The
> request packet ends up with ciaddr as the correct client IP and opt 50
> as "0.0.0.0".
>
Thanks for keeping me on my toes.
I have decided to refactor the whole thing. I think this makes it easier
to understand and the logic is contained in request_dhcp_discover() and
request_dhcp_request() in engine.c and not all over the place.
I have now also tested all the cases and carefuly compared them with RFC
2131 4.3.6, Table 4. This might now be correct.
diff --git dhcpleased.h dhcpleased.h
index b3b4938ad3c..6df38be6f5b 100644
--- dhcpleased.h
+++ dhcpleased.h
@@ -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 engine.c engine.c
index 17e65fbe789..87cee256600 100644
--- engine.c
+++ engine.c
@@ -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;
@@ -1354,11 +1355,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 +1375,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 +1386,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 +1463,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 frontend.c frontend.c
index bd1e37fe952..e2c626ce5d2 100644
--- frontend.c
+++ frontend.c
@@ -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);
}
}
--
I'm not entirely sure you are real.
