GNOME Bugzilla – Bug 793957
Inconsistency in DHCPv4 client identifiers
Last modified: 2018-03-15 16:36:42 UTC
There is an incompatibility between how different NM DHCP clients interpret the user-specified DHCPv4 client identifier. They seem to have opposite requirements: For textual (that is, non hexadecimal) identifiers: * With dhcp=internal, the client ID automatically gets a type prefix 0x00. The user *cannot* include the type prefix on their own. * With dhcp=dhclient, the user *must* include the type prefix as part of the NM setting (e.g. "\x00foo" which gets unescaped) – trying to use just "foo" would result in sending unknown type 0x66. Hexadecimal identifiers have the opposite problem: * With dhcp=internal, the 1st byte of hex client ID is sent as the type prefix. E.g. "00:66:6f:6f" correctly sends the identifier 'foo' of type 0x00. * With dhcp=dhclient, NM detects and throws away the leading 0x00 byte, so "00:66:6f:6f" gets written as send dhcp-client-identifier "foo"; # added by NetworkManager and sends identifier 'oo' of type 0x66. Examples: * This works with dhclient, but results in an unwanted extra 0x00 byte with internal: nmcli con modify "Ethernet" ipv4.dhcp-client-id '\x00frost' * This works with internal, but results in unknown client-ID type (0x66) with dhclient: nmcli con modify "Ethernet" ipv4.dhcp-client-id 'frost' * This works with internal, but *still* results in unknown client-ID type (0x66) with dhclient, because NM detects and discards the leading 00: nmcli con modify "Ethernet" ipv4.dhcp-client-id '00:66:72:6f:73:74' networkmanager 1.10.5dev+3+g5159c34ea (that's what Arch packaged in stable) networkmanager 1.11.1dev+762+g608dfacb0 (git master)
Created attachment 369318 [details] [review] [PATCH] dhcp: dhclient: set type 0 for printable client IDs Maybe we should do this... the only problem is that the client-id would change when users upgrade to a newer version of NM. But I think sooner or later we have to fix this.
seems this was introduced by https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e43174f368f4cb319897626207afea30f5896147 This bug is very painful, because (as you say), fixing it will have visible changes for users out there. But I still think we have to fix it the painful way. We also read the client-id when parsing the file. Can you make sure that the parsing code properly handles backslash escapes? also, when writing a client ID as string, let's make sure that there are no escape sequences that result in wrong configuration. E.g. just detect \\ and ", and fallback to hex notation.
Pushed branch bg/dhcp-client-id-bgo793957.
lgtm. But it's quite a bugfix... we should try to document this change in behavior properly. Maybe add a clear NEWS file entry... I wonder about backporting this... but probably, not. Already released versions should continue to have the broken behavior.
ah, and could the commit message better document which commit introduced the bug, which released versions have the broken behavior, and elaborate a bit more on the effects. Thanks :)
In commit "dhcp: dhclient: set type 0 for printable client IDs" + + /* Otherwise, try to read a hexadecimal sequence */ + s = g_strdup (str + 1); g_strchomp (s); why we do skip first char? In hex shouldn't we read the type too? Rest lgtm
(In reply to Francesco Giudici from comment #6) > In commit "dhcp: dhclient: set type 0 for printable client IDs" > > + > + /* Otherwise, try to read a hexadecimal sequence */ > + s = g_strdup (str + 1); > g_strchomp (s); > > why we do skip first char? In hex shouldn't we read the type too? Good catch, fixed.
(In reply to Thomas Haller from comment #5) > ah, and could the commit message better document which commit introduced the > bug, which released versions have the broken behavior, and elaborate a bit > more on the effects. Thanks :) How about now?
(In reply to Beniamino Galvani from comment #8) > How about now? The remark: "Looking through git history, the dhclient plugin has always behaved this way" seems wrong to me. I think this was introduced some while ago. I think the commit message should mention "Fixes:" https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e43174f368f4cb319897626207afea30f5896147 . Otherwise, lgtm
(In reply to Thomas Haller from comment #9) > (In reply to Beniamino Galvani from comment #8) > > How about now? > > The remark: > "Looking through git history, the dhclient plugin has always behaved this > way" seems wrong to me. > I think this was introduced some while ago. I think the commit message > should mention "Fixes:" > https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > ?id=e43174f368f4cb319897626207afea30f5896147 . Before that commit the client-id string from the connection was added to the configuration file without changes and so I think the problem was already present.
(In reply to Beniamino Galvani from comment #10) > (In reply to Thomas Haller from comment #9) > > (In reply to Beniamino Galvani from comment #8) > > > How about now? > > > > The remark: > > "Looking through git history, the dhclient plugin has always behaved this > > way" seems wrong to me. > > I think this was introduced some while ago. I think the commit message > > should mention "Fixes:" > > https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > > ?id=e43174f368f4cb319897626207afea30f5896147 . > > Before that commit the client-id string from the connection was added to the > configuration file without changes and so I think the problem was already > present. you are correct. branch lgtm
Merged: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=32a279ea5f0ba7e1a5dcce2b76bd0775f295cc88