GNOME Bugzilla – Bug 745243
[enh] use DHCPv6 DUID to construct DHCPv4 Client ID
Last modified: 2020-11-12 14:29:57 UTC
For new leases, we should use the DUID to create the DHCPv4 client ID according to http://www.ietf.org/rfc/rfc4361.txt so that it is consistent between v4 and v6. This is apparently required for some DHCP servers to update the DDNS name of the client when sending the hostname to the DHCP server, since they compare the DHCPv6 and DHCPv4 IDs and expect them to be compatible.
We have to take into account all three dhcp modes NM support: "dhclient", "dhcpcd" and "internal". [dhcpcd]: in NM no support for IPv6. Nothing to do here. [dhclient]: at present, if no client-id is specified, NM will rely on the dhclient daemon to provide a client-id. It will construct a DUID-LLT format, following rfc4361, but will generate one DUID for each connection, so changing the DUID among different connections (and so also for IPv4 and IPv6 ones). [internal]: we should support construction of client-id for DHCPv4 request from DUID. Pushed a patch to address initial changes and solution for dhclient dhcp mode only. The DUID will be constructed using RFC 6355 (DUID-UUID), the client-id will be filled with a value constructed by DUID + IAID as described in RFC4361. Next patch will be for the "internal" dhcp case. branch fg/DUID-IPv4-bgo745243
Updated branch fg/DUID-IPv4-bgo745243 with systemd (internal dhcp client) patches.
>> dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (dhclient) Could you split this commit in two, first only move the function and then do the changes? (and having a subject like "dhcp/trivial: move code" for the first commit). >> squash! dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (dhclient) Just to go into the details about logging ... :) + if ((nm_logging_enabled (LOGL_DEBUG, LOGD_DHCP6)) || + (nm_logging_enabled (LOGL_DEBUG, LOGD_DHCP4))) { This could be: nm_logging_enabled (LOGL_DEBUG, LOGD_DHCP6 | LOGD_DHCP4) or even better (the equal form): nm_logging_enabled (LOGL_DEBUG, LOGD_DHCP) Usually, the logging macros don't need to be wrapped by if(nm_logging_enabled) because they are already lazy in evaluation their arguments. See at "nm_log()" macro. In this case, it could make sense, to avoid an extra "nm_dhcp_utils_duid_to_string (duid)". But it seems better to do: if (G_UNLIKELY (duid == NULL)) { gs_free char *tmp = NULL; duid = generate_duid_from_machine_id (); g_assert (duid); _LOGD ("generated DUID %s", (tmp = nm_dhcp_utils_duid_to_string (duid))); } Sidenote: it's wrong to do: if (nm_logging_enabled (LOGL_DEBUG, LOGD_DOMAIN)) _LOGD (...) it should be: if (_LOGD_ENABLED (...)) _LOGD (...) If you log via _LOGx(). +ip4_start (NMDhcpClient *client, + const char *dhcp_anycast_addr, indention. + if (dhcp_client_id) { + nm_dhcp_client_set_client_id (self, + nm_dhcp_utils_client_id_string_to_bytes (dhcp_client_id)); leaks a GBytes. The leak is there already before. Can we do a first commit, that only fixes the leak? >> dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (internal) [1/2] usually, when updating systemd code, we do a complete new re-import. See "systemd" branch and `gitk e1e428b21e56cad3c50419e3fe2806dbbb21c976`. There are exceptions to this, like https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e94329f0929a46cb84b692af44285bbb856e2b86 , where the patch is not yet merged to systemd and I didn't want to wait for [1]. If all the code is there in systemd, it is usually better to do a complete reimport and let git do the merging. By that I mean, update systemd branch and merge that to your dev-branch. Anyway, these API changes in systemd are IMO wrong and should be first fixed before we reimport code from there. [1] https://github.com/systemd/systemd/pull/2818#issuecomment-195265635
(In reply to Thomas Haller from comment #3) > >> dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (dhclient) > > Could you split this commit in two, first only move the function and then do > the changes? (and having a subject like "dhcp/trivial: move code" for the > first commit). just changed the comment.. It was already just a trivial "move code" commit, I thought to merge it after review with the commit that followed ("squash!"). Fine than, we'll keep these apart! > Just to go into the details about logging ... :) <snip> Thanks... with all the options there I think I took the worse one... ;-) > But it seems better to do: > > > if (G_UNLIKELY (duid == NULL)) { > gs_free char *tmp = NULL; > > duid = generate_duid_from_machine_id (); > g_assert (duid); > _LOGD ("generated DUID %s", > (tmp = nm_dhcp_utils_duid_to_string (duid))); > } sure, much better thanks, code updated > +ip4_start (NMDhcpClient *client, > + const char *dhcp_anycast_addr, > > indention. Missed it! Thanks, fixed > > + if (dhcp_client_id) { > + nm_dhcp_client_set_client_id (self, > + > nm_dhcp_utils_client_id_string_to_bytes (dhcp_client_id)); > > leaks a GBytes. The leak is there already before. Can we do a first commit, > that only fixes the leak? Fixed and merged, thanks. > > >> dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (internal) [1/2] > > usually, when updating systemd code, we do a complete new re-import. See > "systemd" branch and `gitk e1e428b21e56cad3c50419e3fe2806dbbb21c976`. > There are exceptions to this, like > https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > ?id=e94329f0929a46cb84b692af44285bbb856e2b86 , where the patch is not yet > merged to systemd and I didn't want to wait for [1]. > If all the code is there in systemd, it is usually better to do a complete > reimport and let git do the merging. By that I mean, update systemd branch > and merge that to your dev-branch. > > > Anyway, these API changes in systemd are IMO wrong and should be first fixed > before we reimport code from there. > > [1] https://github.com/systemd/systemd/pull/2818#issuecomment-195265635 You are right. I think the definitive solution would be to use the systemd APIs (when they will be ready and we will align the NM code). In the meanwhile we can just set the client_id generating by our own the DUID+IAID identifier as per RFC4361. I have included the patch for this, but I have to admit that I am not very happy with this, as exposes some raw low level packet construction that would be much better if dealt by the dhcp APIs. Maybe we could just take the dhclient patch, leaving internal dhcp unpatched till systemd APIs will be ready (so, skipping last commit). Updated fg/DUID-IPv4-bgo745243
+ if (nm_logging_enabled (LOGL_DEBUG, LOGD_DHCP4)) { + str = nm_dhcp_utils_duid_to_string (priv->duid); + _LOGD ("DUID is '%s'", str); + } no need for if(nm_logging_enabled) -- pushed a fixup. - dhcp_identifier_set_iaid() We don't want to use internal systemd API. Can you do without it? So, priv->duid is always constructed like: »···if (!priv->duid) »···»···priv->duid = NM_DHCP_CLIENT_GET_CLASS (self)->get_duid (self); immediately before using it. If so, then this initialization should be done by nm_dhcp_client_get_duid() -- including the logging. Actually not true: nm_dhcp_client_start_ip4() doesn't set priv->duid in case dhcp_client_id is provided. I guess that is a bug, because later nm-dhcp-systemd.c:ip4_start() asserts: g_return_val_if_fail (duid != NULL, FALSE); If we always pass on priv->duid to the virtual functions, there is no point in passing them as arguments to stop()/ip4_start()/etc. In that case, they should call nm_dhcp_client_get_duid() as needed.
> dhcp: use DHCPv6 DUID to construct DHCPv4 Client ID (internal) + client_id = g_byte_array_free (iaid_duid, FALSE); + } + + sd_dhcp_client_set_client_id (priv->client4, + client_id[0], + client_id + 1, + client_id_len -1); Isn't client_id leaked here? The DUID is present in both the parent object and as argument to ip4_start(). Maybe the argument can be dropped in favor of calling nm_dhcp_client_get_duid() (as we do for client_id)?
Thanks Thomas, thanks Ben... I'll try to recap all points: 1) no need for if(nm_logging_enabled) -- pushed a fixup. Thanks. You already warned me on nm_logging_enabled... sorry. --- 2) dhcp_identifier_set_iaid() We don't want to use internal systemd API. Can you do without it? I can... anyway I would consider leveraging on set_iaid() from systemd as we do the same for DHCPv6 code (implicitly): sd_dhcp6_client_start() /* NM code */ --> client_ensure_iaid() /* systemd */ --> dhcp_identifier_set_iaid() More important, when https://github.com/systemd/systemd/pull/2818#issuecomment-195265635 will be fixed, we could leverage on the new sd_dhcp_client_set_iaid_duid() systemd API specific for DHCPv4 requests, which will share DUID and IAID setup code with the DHCPv6 part. Should we avoid this? As a side note, I see also that in DHCPv6 NM code we use the sd_dhcp6_client_set_duid() systemd API for filling the DUID structure. I have a more "structural" question here: should we manage DUID+IAID inside NM or let systemd / dhclient take care of it? Currently we enforce DUID, but leave IAID management and construction of DUID based client id to dhcp clients. Should we take full control of this? --- 3) nm-dhcp-systemd.c:ip4_start() asserts: > g_return_val_if_fail (duid != NULL, FALSE); You are right, it should be removed. --- 4) DUID parameter can be dropped in favor of nm_dhcp_client_get_duid() Yes, seems the right thing to do. I followed ip6_start, which do the same... maybe I could remove the parameter from there too. 5) client id leaked when setting DUID Right, should be fixed. So, concluding, just not sure what would be better for point 2): Should I copy IAID generation from systemd in NM code? I don't have any big complaint for this at present, but when the sd_dhcp_client_set_iaid_duid() API from systemd would be available I would leverage on it. If not, I wonder if would be than better to take care of IAID management in NM... in this case would be worth to do it now?
The approach in the patches looks OK to me. But backing up a bit, I wonder about the privacy implications of using a stable machine identifier for all connections. It's been discussed also in https://tools.ietf.org/html/draft-ietf-dhc-dhcpv6-privacy-04 (though that's not just about DUID). At least the DUID is only sent when the user has chosen to allow connection to a specific network. But with this change, for IPv4 now we'll be sending a stable client ID that trackers could use to track the client across networks.
Yeah, nice point. I didn't think at all about it... I agree, privacy concerns must be taken into account. I think we should consider the overall NM policy we want to put in place for DUID / IAID / client id management. * Current NM DUID + IAID policy for DHCPv6 NM currently specifies the DUID as a DUID-UUID, following RFC 6355, generated from the machine id (and so having a stable DUID). The IAID is not filled, so will be the dhcp client we rely on that will take care of it. Both dhclient and libsystemd will pick the last four octects of the MAC address of the interface belonging to the connection. So, we will have per interface static DUID and IAID. * Current NM DUID + IAID policy for DHCPv4 If the user does not explicit a client id, NM leaves it to the dhcp client. Then the client id will be saved in the lease file and re-used. With dhclient, the client id will be constructed as a IAID+DUID-LLT, as suggested in RFC 4361. NM passes a unique, per connection lease file to dhclient, so that each DHCPv4 connection has its own unique DUID. But this alone does not save us from privacy issues as the generated DUID is a DUID-LLT: 0 1 2 3 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | 1 | hardware type (16 bits) | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | time (32 bits) | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ . . . link-layer address (variable length) . . . +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ the only changing part in practice will be the time field, as the link-layer address will be common to all connections sharing the same interface. The IAID will be composed by the last 4 octects of the same address. In order to preserve privacy mac address randomization should be put in place. * RFC 4361 - last note The only RFC 4361 mandatory requirement is: "A DHCPv4 client that generates a DUID and that has stable storage MUST retain this DUID for use in subsequent DHCPv4 messages, even after an operating system reboot." So we have conflicting goals: same DUID as requested by RFC 4361 (that could allow enabled servers to recognize interfaces belonging to the same device) or different DUID for privacy preservation. I can see two main paths here: 1) We could implement multiple policies, at least something like static vs randomized, and allow user to choose between them 2) We could keep away at all from DUID and IAID management, leaving the dhcp clients to deal with it (as they already have to)
bugzilla.gnome.org is being shut down in favor of a GitLab instance. We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time. If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/ Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).