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

feat: Support more addresses in RDNSS section #245

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

robbat2
Copy link
Member

@robbat2 robbat2 commented Dec 28, 2024

This is based on PR#193, with significant cleanups & safety checks added.

  • RFC 6106 section 5.3.1 recommended that the number of RDNSS addresses should be limited to three, however that was for clients, not servers.
  • RFC 8106 section 5.3.1 clarified the recommendation to at least 3 from multiple sources.

Based on this, set the limit for a single RDNSS section to 127 servers in radvd, based on the size of the length field. Note that it may cause fragmentation as the single option exceeds usable RA option space (MTU less headers); and server configurations should generally use fewer.

See also the RFC6106 errata & RFC6980 section 2 for behaviors of very large RA options.

Reference: https://www.rfc-editor.org/errata/rfc6106
Reference: https://datatracker.ietf.org/doc/html/rfc6980#section-2
Reference: https://datatracker.ietf.org/doc/html/rfc6106#section-5.3.1
Reference: https://datatracker.ietf.org/doc/html/rfc8106#section-5.3.1
Based-on: #193
Based-on-work-by: WenChao1Hou [email protected]

This is based on PR#193, with significant cleanups & safety checks
added.

- RFC 6106 section 5.3.1 recommended that the number of RDNSS addresses
  should be limited to three, however that was for clients, not servers.
- RFC 8106 section 5.3.1 clarified the recommendation to at least 3 from
  multiple sources.

Based on this, set the limit for a single RDNSS section to 127 servers
in radvd, based on the size of the length field. Note that it may cause
fragmentation as the single option exceeds usable RA option space (MTU
less headers); and server configurations should generally use fewer.

See also the RFC6106 errata & RFC6980 section 2 for behaviors of very
large RA options.

Reference: https://www.rfc-editor.org/errata/rfc6106
Reference: https://datatracker.ietf.org/doc/html/rfc6980#section-2
Reference: https://datatracker.ietf.org/doc/html/rfc6106#section-5.3.1
Reference: https://datatracker.ietf.org/doc/html/rfc8106#section-5.3.1
Based-on: radvd-project#193
Based-on-work-by: WenChao1Hou <[email protected]>
Signed-off-by: Robin H. Johnson <[email protected]>
@robbat2 robbat2 force-pushed the robbat2/rdnss-20241217 branch from 4cc9ad6 to 74f5412 Compare December 29, 2024 05:14
@stappersg
Copy link
Member

What I missed:

--- a/radvd.conf.5.man
+++ b/radvd.conf.5.man
@@ -83,11 +83,24 @@ advertise more specific routes to the hosts.
 RDNSS (Recursive DNS server) definitions are of the form:
 
 .nf
-.BR "RDNSS " "ip [ip] [ip] " {
+.BR "RDNSS " "ip [ip] [ip] ..." {
 	list of rdnss specific options
 .B };
 .fi
 
+For your consideration:
+
+* RFC 6106 section 5.3.1 recommended that the number of RDNSS addresses
+  should be limited to three, however that was for clients, not servers.
+* RFC 8106 section 5.3.1 clarified the recommendation to at least 3 from
+  multiple sources.
+
+Based on this, set the limit for a single RDNSS section to 127 servers
+in radvd, based on the size of the length field. Note that it may cause
+fragmentation as the single option exceeds usable RA option space (MTU
+less headers); and server configurations should generally use fewer.
+
+
 DNSSL (DNS Search List) definitions are of the form:
 
 .nf

Please make such minimalistic change the manual page of the configuration file. The word minimalistic is for expressing "it is okay to make a non perfect formatted change, better formatting can be done in a follow-up change".

Copy link
Contributor

@mpontillo mpontillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. (Just one minor nit.)

// too many DNS servers could exceed it
// https://datatracker.ietf.org/doc/html/rfc8106#section-5.1
if (bytes > (255 * 8)) {
flog(LOG_ERR, "RDNSS too long for RA option, must be < 2032 bytes. Skipping option.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as the other PR - shouldn't this be <= 2040 bytes? Or am I not understanding the math?

@robbat2
Copy link
Member Author

robbat2 commented Dec 30, 2024

Will improve the manpage & consistent error messages after both #245 and #244 are merged

@robbat2 robbat2 merged commit 049fd2a into radvd-project:master Dec 30, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants