Re: [PATCH] dhcpv6: Properly separate domain names in search list

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]

On Thu, Sep 21, 2023 at 05:08:22PM +0200, Stefano Brivio wrote:
> On Thu, 21 Sep 2023 09:55:11 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Sep 20, 2023 at 05:05:06PM +0200, Stefano Brivio wrote:
> > > If we go over the flattened list of search domains and just replace
> > > dots and zero bytes with the length of the next label to implement
> > > the encoding specified by section 3.1 of RFC 1035, if there are
> > > multiple domains in the search list, we'll also replace separators
> > > between two domain names with the length of the first label of the
> > > second domain, plus one.  
> > 
> > That is... an impressively long sentence.  Any chance you could reword
> > that in shorter ones that are easier to follow ;).
> 
> Oops. :) What about:
> 
>   To prepare the DHCPv6 domain search list option, we go over the
>   flattened list of domains, and replace both dots and zero bytes with
>   a counter of bytes in the next label, implementing the encoding specified
>   by section 3.1 of RFC 1035.
> 
>   If there are multiple domains in the list, however, zero bytes serve
>   as markers for the end of a domain name, and we'll replace them with
>   the length of the first label of the next domain, plus one. This is
>   wrong. We should only convert the dots before the labels.
> 
> ?

Better..  I think my main confusion is that I don't remember enough
about DNS to recall what the distinction is between domains and
labels.

> > > Those should remain as zero bytes to
> > > separate domains, though.
> > > 
> > > To distinguish between label separators and domain names separators,
> > > for simplicity, introduce a dot before the first label of every
> > > domain we copy to form the list. All dots are then replaced by label
> > > lengths, and separators (zero bytes) remain as they are.
> > > 
> > > As we do this, we need to make sure we don't replace the trailing
> > > dot, if present: that's already a separator. Skip copying it, and
> > > just add separators as needed.
> > > 
> > > Now that we don't copy those, though, we might end up with
> > > zero-length domains: skip them, as they're meaningless anyway.
> > > 
> > > And as we might skip domains, we can't use the index 'i' to check if
> > > we're at the beginning of the option -- use 'srch' instead.
> > > 
> > > This is very similar to how we prepare the list for NDP option 31,
> > > except that we don't need padding (RFC 8106, 5.2) here, and we should
> > > refactor this into common functions, but it probably makes sense to
> > > rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21)
> > > first.
> > > 
> > > Reported-by: Sebastian Mitterle <smitterl@redhat.com>
> > > Link: https://bugs.passt.top/show_bug.cgi?id=75
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  dhcpv6.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > index fc42a84..58171bb 100644
> > > --- a/dhcpv6.c
> > > +++ b/dhcpv6.c
> > > @@ -376,24 +376,34 @@ search:
> > >  		return offset;
> > >  
> > >  	for (i = 0; *c->dns_search[i].n; i++) {
> > > -		if (!i) {
> > > +		size_t name_len = strlen(c->dns_search[i].n);
> > > +
> > > +		/* We already append separators, don't duplicate if present */
> > > +		if (c->dns_search[i].n[name_len - 1] == '.')
> > > +			name_len--;
> > > +
> > > +		/* Skip root-only search domains */
> > > +		if (!name_len)
> > > +			continue;  
> > 
> > Should we consider doing this normalisation when we build
> > c->dns_search, rather than here?
> 
> I quickly looked at that, but it complicates the (insane) compression
> scheme from RFC 1035 4.1.4 implemented by the DHCP server -- and also
> we would need to convert them back before printing them.

Ok, fair enough.

> I don't think it's necessarily a bad idea though. I guess we should
> have a few explicit functions to convert between different encodings and
> then stick to the storage format that turns out to be the most
> convenient. But that's beyond the scope of this fix.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]