networkd: fallback to chaddr for static lease lookup when not found

[RobertMe]

DHCP static leases are looked up by the client identifier as send by the client, while configured based on MAC. As RFC 2131 states the client identifier is an opaque key and must not be interpreted by the server this means that DHCP clients can (/will) also use a client identifier which is not a MAC address. One of these clients actually is systemd-networkd which uses an RFC 4361 by default to generate the client identifier. For these kind of DHCP clients static leases thus don't work because of this mismatch between configuring a MAC address but the server matching based on client identifier. This adds a fallback to try to look up a configured static lease based on the "chaddr" of the DHCP message as this will always contain the MAC address of the client.

Fixes #21368

[RobertMe]

This is my first PR, and I'm a C noob, so please bear with me :)

Some remarks:

1. Code is "reused" from existing code where the configuration is converted into a client id:

   systemd/src/network/networkd-dhcp-server-static-lease.c

   Lines 197 to 203 in 360c9cd

   	c = new(uint8_t, ETH_ALEN + 1);
   	if (!c)
   	return log_oom();
   	/* set client id type to 1: Ethernet Link-Layer (RFC 2132) */
   	c[0] = 0x01;
   	memcpy(c + 1, &hwaddr, ETH_ALEN);

2. I couldn't find that there are any unit tests covering the DHCP server so I couldn't / didn't add any for this change. If there are unit tests covering this, than please point me to where these are
3. I've looked into "rewriting" the current implementation to drop "client id" fully and replacing it with just mac address as personally I believe that, in it's current form, it doesn't really make sense (no other DHCP server seems to work based on client id; And the configuration calling it MACAddress but actual implementation using it the "client identifier" and not the "chaddr"). But this seems to involve some shared libraries / headers which I believe would require rewiring the current methods to a "dumbed down" version which would be odd and unfamiliar to me. Furthermore later on there might still be the possibility to extend the configuration with a ClientIdentifier= based match (so either on MACAddress / chaddr or on client identifier).
4. I thought about adding some "logic" to the condition (if (!static_lease)) but as the spec says that the client id is an opaque value I believe that adding some checks like "only do this fallback lookup if the received client id doesn't look like a MAC address" would "violate" that rule as it wouldn't apply the fallback logic when the client id happens to be 7 bytes long and start with 0x01.

[yuwata]

Thank you! The basic logic sounds good to me. Several minor requests.

[RobertMe]

Thanks for the review @yuwata, despite my apparent failure to run the tests locally and all failing. I will try to fix your remarks in today or in the next couple of days.

Could you confirm that the CI error is due to a missing free(c.data) (after using c for the hashmap lookup)? And might you also be able to direct me to where these tests are actually located so I can add some? As I'm failing miserably to find anything based on the error. I do obviously find the LLVMFuzzerTestOneInput ../src/libsystemd-network/fuzz-dhcp-server.c:76, but missing on what this list of errors is based:

453/1183 systemd:libsystemd-network / test-dhcp-server FAIL 0.54s exit status 1
808/1183 systemd:fuzzers / fuzz-dhcp-server_buffer-overflow-2 FAIL 0.08s exit status 1
809/1183 systemd:fuzzers / fuzz-dhcp-server_discover-existing FAIL 0.08s exit status 1
810/1183 systemd:fuzzers / fuzz-dhcp-server_discover-new FAIL 0.09s exit status 1
811/1183 systemd:fuzzers / fuzz-dhcp-server_duplicate-input-data FAIL 0.08s exit status 1
812/1183 systemd:fuzzers / fuzz-dhcp-server_release FAIL 0.08s exit status 1
813/1183 systemd:fuzzers / fuzz-dhcp-server_request-existing FAIL 0.08s exit status 1
814/1183 systemd:fuzzers / fuzz-dhcp-server_request-new FAIL 0.09s exit status 1
815/1183 systemd:fuzzers / fuzz-dhcp-server_request-reboot FAIL 0.09s exit status 1
816/1183 systemd:fuzzers / fuzz-dhcp-server_request-renew FAIL 0.09s exit status 1

So the "buffer-overflow-2", "discover-existing", .... Are these tests? Or like data sets and LLVMFuzzerTestOneInput being the only test method (being executed multiple times)?
And a pointer in how I can run the tests locally would be appreciated as well :) I earlier quickly copied what I believed to be the (shell) commands from the CI run but still wasn't getting those errors as reported by the CI.

[RobertMe]

Could you confirm that the CI error is due to a missing free(c.data) (after using c for the hashmap lookup)

At the moment (some) of the builds passed. So I think this was corrent. Fixed by extracting a variabele for the data and using _cleanup_free_ macro on it.

And might you also be able to direct me to where these tests are actually located so I can add some?

I think these are based on files in test/fuzz/fuzz-dhcp-server/, for which I presume the file contents is the raw DHCP request which is passed to LLVMFuzzerTestOneInput? And the test is then just whether the server "works" (/doesn't crash or leak memory etc)? So there would be no actual validation. So best I can do is probably creating a request which doesn't match a configured static lease on client id (which at the moment most likely already occurs) and then have one with a non-matching and one with a matching chaddr? So will look into getting the tests running locally and setting up those tests.

[yuwata]

Looks mostly OK. Several minor comments.

[yuwata]

@RobertMe I cannot see the revised commit. Maybe you pushed an older version?

[RobertMe]

@yuwata whoops, that was stupid. Did a commit --amend without -a / staging the changes. So it's committed & pushed now.

[yuwata]

LGTM. Thank you!

[mbiebl]

@yuwata we had a downstream bug report against v252 in Debian:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050098

Do you think it would be feasible/reasonable to backport this patch to v252-stable?

[yuwata]

@yuwata we had a downstream bug report against v252 in Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050098

Do you think it would be feasible/reasonable to backport this patch to v252-stable?

Yeah, not sure we can cleanly backport it (that is, git cherry-pick -x work or not), but this PR should fix the issue.

[RobertMe]

That would be awesome. As I actually discovered this issue while running Debian (Bookworm) myself. So if it could be backported I don't have to wait ~2 years to actually use my own bugfix 😃

And if I can be of any help please let me know.

[mbiebl]

git cherry-pick -x works cleanly.
@RobertMe could you test/verify if the patch applied on top of v252-stable fixes your issue?