dhcpv6: implement RFC9686

[systemcrash]

Registering Self-Generated IPv6 Addresses Using DHCPv6

This functionality registers a client SLAAC or static address in
the DHCPv6 pool, primarily for diagnostic purposes as outlined in §1.

Most of the machinery required for this functionality already exists,
so we perform a few additional sets and checks in the IA code.

The address registration mechanism overview:

+--------+        +------------------+       +---------------+
| CLIENT |        | FIRST-HOP ROUTER |       | DHCPv6 SERVER |
+--------+        +---------+--------+       +-------+-------+
    |      SLAAC            |                        |
    |<--------------------> |                        |
    |                       |                        |
    |                                                |
    |  src: link-local address                       |
    | -------------------------------------------->  |
    |    INFORMATION-REQUEST or SOLICIT/...          |
    |       - OPTION REQUEST OPTION                  |
    |          -- OPTION_ADDR_REG_ENABLE             |
    |                                                |
    |    ...                                         |
    |                                                |
    |                                                |
    |<---------------------------------------------  |
    |     REPLY or ADVERTISE MESSAGE                 |
    |       - OPTION_ADDR_REG_ENABLE                 |
    |                                                |
    |                                                |
    |  src: address being registered                 |
    | -------------------------------------------->  |
    |    ADDR-REG-INFORM MESSAGE                     |Register/
    |                                                |log addresses
    |                                                |
    |                                                |
    | <--------------------------------------------  |
    |        ADDR-REG-REPLY MESSAGE                  |
    |                                                |

Closes #349

Might require a few tweaks. This code is untested, but simple enough to merge and see production when this option appears (or we add it in odhcp6c and test that way - ULA are supposed to be registered).

@Alphix and @Noltari at your leisure.

[systemcrash]

Note to self: do we want a configure tuneable? Or do we just set client_addr_reg_enable to true only if M or O flags are set. The client SHOULD NOT send the ADDR-REG-INFORM message unless it has received a Router Advertisement (RA) message with either the M or O flags set to 1.

[Alphix]

There's more cases to consider? If a is true, then that address is already taken? Also, there might be a static lease configured (even though it might not be in use), we should check for that as well....in both cases, we should log and drop (4.2.1, second last paragraph)

[systemcrash]

This case is taken care of higher up - a is set, as part of normal lease operations, and the lease is updated. These 4 message types just lodge a new lease. Static leases are checked earlier in the code (irrespective of message type).

The spec is clear about 'refreshing timers' and updating leases - that's all implicitly handled (testing will establish this), so we should not drop those - but use those to update our leases.

[Alphix]

Good

[Alphix]

And some more random points that don't fit some specific line(s) in the diff:

• I think that there's no check that the to-be-registered address is of global scope? (GUA/ULA)
• I think you should add a bool to struct dhcpv6_lease to indicate that this is a self-registered address and not a "normal" lease
• That status should probably be included in the ubus flags for the DHCPv6 lease

(personal opinion) The RFC comments are slowly getting a bit out of hand, while a quick:

/* See RFC527, second paragraph */
if (something_unexpected)
        do_something_drastic();

Is helpful, citing ever longer passages from RFCs just blows up the size of the code, and anyone who is interested should really go read the referred-to paragraph/section of the relevant RFC anyway to make sure that they get the whole context.

[Alphix]

Note to self: do we want a configure tuneable?

I don't think it's necessary...if we're running a DHCPv6 server, it doesn't seem to serve any real purpose to not allow clients to inform us of their autoconfigured addresses...?

Or do we just set client_addr_reg_enable to true only if M or O flags are set.

If the M and O flags aren't set, we shouldn't expect to get any DHCPv6 requests in the first place? But another interesting twist would be if A isn't set and a client address registration msg comes in...we've told clients not to do autoconf...

[systemcrash]

• I think that there's no check that the to-be-registered address is of global scope? (GUA/ULA)

You're right. There is not (yet). The spec defines no server behaviour for this in §4.2.1.

It's mandated as part of §4.2 - what the client must do. ( It does no harm if the client erroneously lodges a LL with us. It's nothing that we would assign anyway from our available prefixes. IOW, it's also harmless for us to check this somewhere. And the implied behaviour is... that we discard it. )

static inline bool IN6_IS_ADDR_GLOBAL(const struct in6_addr *a)
{
	return !(
		IN6_IS_ADDR_UNSPECIFIED(a) ||
		IN6_IS_ADDR_LOOPBACK(a) ||
		IN6_IS_ADDR_MULTICAST(a) ||
		IN6_IS_ADDR_LINKLOCAL(a) ||
		IN6_IS_ADDR_ULA(a)
	);
}

?

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html defines others.

Edit: I think this is largely handled by if (ia_addr_present && !dhcpv6_ia_on_link(ia, a, iface)) ... else ...

• I think you should add a bool to struct dhcpv6_lease to indicate that this is a self-registered address and not a "normal" lease

Not a bad idea.

(personal opinion) The RFC comments are slowly getting a bit out of hand, while a quick:

Hmm - understand. I think 2-3 lines for a code-block is fine, with exceptions for complex bits. Because this RFC basically uses existing functionality with some extra ifs, it's much clearer that we cover all of the MUST parts in the server behaviour (e.g. via a code search) since most machinery is already implicit.

In short, it'd be nice to get this tested before making more changes, or... you could write out your implementation and we compare notes :)

[systemcrash]

OK @Alphix last commit addresses 'exactly one' and:

• I think you should add a bool to struct dhcpv6_lease to indicate that this is a self-registered address and not a "normal" lease

• That status should probably be included in the ubus flags for the DHCPv6 lease

[Alphix]

• I think that there's no check that the to-be-registered address is of global scope? (GUA/ULA)

You're right. There is not (yet). The spec defines no server behaviour for this in §4.2.1.

It's mandated as part of §4.2 - what the client must do. ( It does no harm if the client erroneously lodges a LL with us. It's nothing that we would assign anyway from our available prefixes. IOW, it's also harmless for us to check this somewhere. And the implied behaviour is... that we discard it. )
...
Edit: I think this is largely handled by if (ia_addr_present && !dhcpv6_ia_on_link(ia, a, iface)) ... else ...

Hm, yes, dhcpv6_ia_on_link might actually be enough. The only thing it could get "wrong" is if the list of addresses that dhcpv6_ia_on_link checks against includes our own linklocal address (I haven't checked if it does, but the list comes from netlink, so my guess is that it does).

In short, it'd be nice to get this tested before making more changes, or... you could write out your implementation and we compare notes :)

Yeah, feel free to not do any changes until you've done more tests...odhcp6c support would probably be a good step. I always prefer developing bits and pieces like this with a suitable counterpart to test with...

...and no, I'm not going to write an implementation right now :)

[systemcrash]

Thanks for the comprehensive review @Alphix. This already feels like a better result. If those last few changes look good - then I'll squash.

This is the on_link check

odhcpd/src/dhcpv6-ia.c

Lines 902 to 934 in 03c1468

	struct odhcpd_ipaddr *addrs = iface->addr6;
	size_t addrlen = iface->addr6_len;
	time_t now = odhcpd_time();
	uint8_t *odata, *end = ((uint8_t*)ia) + htons(ia->len) + 4;
	uint16_t otype, olen;
	bool onlink = true;
	dhcpv6_for_each_option((uint8_t*)&ia[1], end, otype, olen, odata) {
	struct dhcpv6_ia_prefix *p = (struct dhcpv6_ia_prefix *)&odata[-4];
	struct dhcpv6_ia_addr *n = (struct dhcpv6_ia_addr *)&odata[-4];
	if ((otype != DHCPV6_OPT_IA_PREFIX || olen < sizeof(*p) - 4) &&
	(otype != DHCPV6_OPT_IA_ADDR || olen < sizeof(*n) - 4))
	continue;
	onlink = false;
	for (size_t i = 0; i < addrlen; ++i) {
	if (!valid_addr(&addrs[i], now))
	continue;
	if (ADDR_MATCH_PIO_FILTER(&addrs[i], iface))
	continue;
	if (ia->type == htons(DHCPV6_OPT_IA_PD)) {
	if (p->prefix_len < addrs[i].prefix_len ||
	odhcpd_bmemcmp(&p->addr, &addrs[i].addr.in6, addrs[i].prefix_len))
	continue;
	} else if (odhcpd_bmemcmp(&n->addr, &addrs[i].addr.in6, addrs[i].prefix_len))
	continue;
	onlink = true;
	}

where

valid_addr is

odhcpd/src/dhcpv6-ia.h

Lines 33 to 36 in 03c1468

	static inline bool valid_addr(const struct odhcpd_ipaddr *addr, time_t now)
	{
	return (addr->prefix_len <= 96 && addr->valid_lt > (uint32_t)now && addr->preferred_lt > (uint32_t)now);
	}

That could probably be better named as something like addr_valid_lifetime but for the prefix bit.

[Alphix]

Thanks for the comprehensive review @Alphix. This already feels like a better result. If those last few changes look good - then I'll squash.

I think I'll just keep my mouth shut now until you have the chance to test this code. Feel free to squash/not squash as/when you prefer...

[systemcrash]

The good news is that most of this is structurally correct, and almost worked on the first try (I got the replies but IA Addr was missing from response because I used type 3 and not 5) :)

Basically, assign_na() stuffs IA_NA(type 3)->IA Addr addresses in our linked list (we have T1, T2, constructed host_id), but RFC9686 sends one naked IA Addr (type 5), while odhcpd IA is set up to handle (non-naked) IA_NA->IA Addr. I think we need a new function specific for these IA Addr that come in (since we also receive ULA which have identical host_id portions).

I'll refresh with what I get working so we can chisel out a good way to handle these records.

DHCPv6
    Message type: Address Registration Inform (36)
    Transaction ID: 0x7ede1d
    Client Identifier
        Option: Client Identifier (1)
        Length: 18
        DUID: 0004xx
        DUID Type: Universally Unique IDentifier (UUID) (4)
        UUID: xx
    IA Address
        Option: IA Address (5)
        Length: 24
        IPv6 address: 2001:db8:10:1100:1200:27ff:fe59:96ef
        Preferred lifetime: 2147
        Valid lifetime: 2147

...
DHCPv6
    Message type: Address Registration Reply (37)
    Transaction ID: 0x7ede1d
    Server Identifier
        Option: Server Identifier (2)
        Length: 10
        DUID: 0003000xx
        DUID Type: link-layer address (3)
        Hardware type: Ethernet (1)
        Link-layer address: xx
        Link-layer address (Ethernet): xx
    Client Identifier
        Option: Client Identifier (1)
        Length: 18
        DUID: 0004xx
        DUID Type: Universally Unique IDentifier (UUID) (4)
        UUID: xx
    SOL_MAX_RT
        Option: SOL_MAX_RT (82)
        Length: 4
        Data: 0000003c
    DNS recursive name server
        Option: DNS recursive name server (23)
        Length: 16
         1 DNS server address: fd51:1c2a:8909::1
    Domain Search List
        Option: Domain Search List (24)
        Length: 5
        Domain name suffix search list
    IA Address
        Option: IA Address (5)
        Length: 24
        IPv6 address: 2001:db8:10:1100:1200:27ff:fe59:96ef
        Preferred lifetime: 2147
        Valid lifetime: 2147

[Alphix]

Basically, assign_na() stuffs IA_NA(type 3)->IA Addr addresses in our linked list (we have T1, T2, constructed host_id), but RFC9686 sends one naked IA Addr (type 5), while odhcpd IA is set up to handle (non-naked) IA_NA->IA Addr. I think we need a new function specific for these IA Addr that come in (since we also receive ULA which have identical host_id portions).

What a coincidence, I've been fiddling with the same stuff because of my work with creating better statefiles that can be used to persist leases across a restart/reboot. I think I might have something that can benefit both of us....basically I want to add an array of real IPv6 addresses to struct dhcpv6_lease, so that we can know exactly which addresses have been handed out (the assigned_host_id is not enough for that...consider a situation where an iface gets another prefix after it has already handed out a lease, for example)...

[systemcrash]

Yeah, my solution at present is just to jam the peer IP in there (if checks pass), since this RFC mandates the IP Addr == sender addr. Now the ubus lease handling stuff needs tweaking 😅

[systemcrash]

OK - the exchanges work with this, although the lease handling is still not yet ready.

[Alphix]

OK - the exchanges work with this, although the lease handling is still not yet ready.

Did you implement it in odhcp6c as well, or how are you testing the functionality?

Edit: and if you give me some time, I think I'll have something for you wrt. the lease handling...

[systemcrash]

Implemented in 6c. Easiest way to test.

[systemcrash]

OK - I've been testing this one out against 6c, and it works fine:

odhcp6c request

DHCPv6
    Message type: Address Registration Inform (36)
    Transaction ID: 0x793fe6
    Client Identifier
        Option: Client Identifier (1)
        Length: 18
        DUID: 0004xxx
        DUID Type: Universally Unique IDentifier (UUID) (4)
        UUID: xxx
    IA Address
        Option: IA Address (5)
        Length: 24
        IPv6 address: 2001:db8:20:1100:a00:27ff:fe59:96ef
        Preferred lifetime: 1917
        Valid lifetime: 1917

odhcpd reply:

DHCPv6
    Message type: Address Registration Reply (37)
    Transaction ID: 0x793fe6
    Server Identifier
        Option: Server Identifier (2)
        Length: 10
        DUID: 0003xx
        DUID Type: link-layer address (3)
        Hardware type: Ethernet (1)
        Link-layer address: xx
        Link-layer address (Ethernet): xx
    Client Identifier
        Option: Client Identifier (1)
        Length: 18
        DUID: 0004xxx
        DUID Type: Universally Unique IDentifier (UUID) (4)
        UUID: xxx
    IA Address
        Option: IA Address (5)
        Length: 24
        IPv6 address: 2001:db8:20:1100:a00:27ff:fe59:96ef
        Preferred lifetime: 1917
        Valid lifetime: 1917

and ubus call dhcp ipv6leases produces:

{
	"duid": "0004xx",
	"iaid": 0,
	"hostname": "",
	"accept-reconf": false,
	"flags": [
		"bound",
		"self-generated"
	],
	"ipv6-addr": [
		{
			"address": "fd51:1c2a:8909:0:a00:27ff:fe59:96ef",
			"preferred-lifetime": 1325,
			"valid-lifetime": 1325
		}
	],
	"valid": 4297
},
{
	"duid": "0004xx",
	"iaid": 0,
	"hostname": "",
	"accept-reconf": false,
	"flags": [
		"bound",
		"self-generated"
	],
	"ipv6-addr": [
		{
			"address": "2001:db8:20:1100:a00:27ff:fe59:96ef",
			"preferred-lifetime": 1325,
			"valid-lifetime": 1325
		}
	],
	"valid": 1325
}

[Alphix]

Awesome, I'll post a draft of the thing I'm working on....hopefully this evening (CET) or tomorrow...I hope you'll find that both of our projects will dovetail nicely...

[systemcrash]

Let's see - I'm happy with where things are handling leases at present here - but you typically reveal something I hadn't thought of.

[Alphix]

Awesome, I'll post a draft of the thing I'm working on....hopefully this evening (CET) or tomorrow...I hope you'll find that both of our projects will dovetail nicely...

@systemcrash see #364

[systemcrash]

I think this is ready for general consumption now. @Alphix you seem to dislike passing loads of parameters, but it feels like we can pass addrbuf once or twice to avoid doing inet_ntop at different steps.

The 6c half needs some more time - cleanup and hooking in to stats. Currently it triggers on RA, which doesn't seem optimal... but it works. So I can throw it out there so at least others can test this.