Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

odhcpd: add DNR (RFC 9463) support #213

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Alphix
Copy link

@Alphix Alphix commented Feb 10, 2024

Discovery of Network-designated Resolvers (DNR) allows devices on the network
to discover encrypted DNS resolvers, which has so far required either manual
configuration or other approaches (like systemd-resolved's "opportunistic"
mode).

To enable DNR, a new uci parameter has been added, which needs to contain at
the very least, the priority (1-65535, lower = higher priority) and the server
hostname (Authentication Domain Name, ADN, to use the wording of RFC9463):

config dhcp 'lan'
        …
        list dnr '100 foobar.example.com'

Optionally (and preferably), a comma-separated list of IP addresses and SvcParams can also be specified, like this (line wrapping added):

config dhcp 'lan'
        …
        list dnr '100 resolver1.example.com
                    fda7:ab54:69fb::1,fda7:ab54:69fb::2,10.0.0.1
                    alpn=dot port=853'
        list dnr '200 resolver2.example.com
                    fda7:ab54:69fb::2,10.0.1.1,10.0.1.2
                    alpn=dot port=853'

Client support is on it's way (e.g. in systemd PR #30952 and in Windows).

Draft for now, needs more testing (I've only tested the RA alternative so far) and some small changes to how the options are handled.

@Alphix
Copy link
Author

Alphix commented Feb 10, 2024

@rpigott this might interest you, given this PR: systemd/systemd#30952

@rpigott
Copy link

rpigott commented Feb 10, 2024

Yes! Thanks for the ping.

Storing request options as chars means that it's difficult to handle option
codes > 127 due to the signedness of char (and options aren't chars, they're
binary data).

Signed-off-by: David Härdeman <[email protected]>
Discovery of Network-designated Resolvers (DNR) allows devices on the network
to discover encrypted DNS resolvers, which has so far required either manual
configuration or other approaches (like systemd-resolved's "opportunistic"
mode).

To enable DNR, a new uci parameter has been added, which needs to contain at
the very least, the priority (1-65535, lower = higher priority) and the server
hostname (Authentication Domain Name, ADN, to use the wording of RFC9463):

config dhcp 'lan'
        …
        list dnr '100 foobar.example.com'

Optionally (and preferably), a comma-separated list of IP addresses and
SvcParams can also be specified, like this (line wrapping added):

config dhcp 'lan'
        …
        list dnr '100 resolver1.example.com
                    fda7:ab54:69fb::1,fda7:ab54:69fb::2,10.0.0.1
                    alpn=dot port=853'
        list dnr '200 resolver2.example.com
                    fda7:ab54:69fb::2,10.0.1.1,10.0.1.2
                    alpn=dot port=853'

Client support is on it's way (e.g. in systemd PR #30952 or in the Windows
Insiders program).

Signed-off-by: David Härdeman <[email protected]>
@Alphix
Copy link
Author

Alphix commented Feb 11, 2024

Ok, changed the options handling a bit (it's now <prio> <ADN> <IPs> <SVC>) and added some fixes/changes (e.g. for unaligned struct access).

@Alphix
Copy link
Author

Alphix commented Feb 12, 2024

And now I've done more testing, both on x86/qemu and on real hardware (Turris Omnia, running stock OpenWRT 23.05.2). Using a patched version of rdisc6 (patches sent upstream already) and eyeballing the generated DHCPv4/DHCPv6 packets using tcpdump. Looks good to me. Going to convert this to a PR for more review.

One thing I'm less certain about is the configuration format. I've just started looking at luci, and these multivalue strings look like they could make life difficult....

@Alphix Alphix marked this pull request as ready for review February 12, 2024 09:36
@systemcrash
Copy link
Contributor

One thing I'm less certain about is the configuration format. I've just started looking at luci, and these multivalue strings look like they could make life difficult....

Hard choices need to be made. A single string is the cleanest copy/paste way to get data in, while chopping fields up makes error checking generally simpler. I recommend staying with long strings.

@systemcrash
Copy link
Contributor

@Alphix There is a bit in the RFC about lifetime:
https://www.rfc-editor.org/rfc/rfc9463.html#name-option-format-3

A value of zero means that this ADN MUST no longer be used.

Is it possible (does it make sense to?) to add lifetime as a parameter or flag, for those cases where it can be set to zero and force invalidate a specific resolver? (i.e. have a config entry with lifetime 0 as 'reminder' that some older resolver must no longer be used). I'm debating a bit with myself here since some other settings co-opt zero to mean 0xffffffff i.e. infinity.

It might not be immediately convenient to add. My last reading of the source is that lifetime is internally managed.

Also, does this handle the ND(P) DNR?

@Alphix
Copy link
Author

Alphix commented Apr 8, 2024

Is it possible (does it make sense to?) to add lifetime as a parameter or flag, for those cases where it can be set to zero and force invalidate a specific resolver? (i.e. have a config entry with lifetime 0 as 'reminder' that some older resolver must no longer be used). I'm debating a bit with myself here since some other settings co-opt zero to mean 0xffffffff i.e. infinity.

It might not be immediately convenient to add. My last reading of the source is that lifetime is internally managed.

@systemcrash It's certainly possible...and you're right that it's currently internally managed (i.e. hardcoded in src/router.c, line 807 to use lifetime which corresponds to 3 * MaxRtrAdvInterval, as per §6.1 of RFC9463).

The config string could be extended to also include a lifetime. If we use a fake SVC parameter (e.g. _lifetime=XYZ), it would be easy to make it optional (so that the automagic 3 * MaxRtrAdvInterval can be used when the lifetime is not explicitly configured).

Also, does this handle the ND(P) DNR?

Yeah, all possibilities (sections 4, 5 and 6 of RFC9463) are covered:

  • DHCPv4 (though only in odhcp, which is of course not traditionally used as the DHCPv4 server in OpenWRT)
  • DHCPv6
  • IPv6 RA (NDP)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants