Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 18 Dec 2024 12:57:24 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Wed, Dec 18, 2024 at 12:35 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > The truncation logic looks good to me (as a first step). Some comments
> > below:
> >
> > On Tue, 17 Dec 2024 07:50:28 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> > > (client fqdn), for some virt deployments like kubevirt is expected to
> > > have the VirtualMachine name as the guest hostname.
> > >
> > > This change add the following arguments:
> > >  - -H --hostname NAME to configure the hostname DHCPv4 option(12)
> > >  - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
> > >    DHCPv6(39)
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > >  conf.c           | 23 ++++++++++++++--
> > >  dhcp.c           | 39 +++++++++++++++++++++++---
> > >  dhcpv6.c         | 72 +++++++++++++++++++++++++++++++++++++++---------
> > >  passt.1          | 11 ++++++++
> > >  passt.h          |  6 ++++
> > >  pasta.c          | 15 ++++++++--
> > >  test/lib/setup   | 10 +++----
> > >  test/passt.mbuto |  6 ++--
> > >  test/passt/dhcp  | 15 +++++++++-
> > >  util.c           | 22 +++++++++++++++
> > >  util.h           |  3 ++
> > >  11 files changed, 191 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/conf.c b/conf.c
> > > index df2b016..29e4bed 100644
> > > --- a/conf.c
> > > +++ b/conf.c
> > > @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
> > >               FPRINTF(f, "    default: use addresses from /etc/resolv.conf\n");
> > >       FPRINTF(f,
> > >               "  -S, --search LIST    Space-separated list, search domains\n"
> > > -             "    a single, empty option disables the DNS search list\n");
> > > +             "    a single, empty option disables the DNS search list\n"
> > > +             "  -H, --hostname NAME  Hostname to configure client with\n"
> > > +             "  --fqdn NAME          FQDN to configure client with\n");
> > >       if (strstr(name, "pasta"))
> > >               FPRINTF(f, "    default: don't use any search list\n");
> > >       else
> > > @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               {"outbound",    required_argument,      NULL,           'o' },
> > >               {"dns",         required_argument,      NULL,           'D' },
> > >               {"search",      required_argument,      NULL,           'S' },
> > > +             {"hostname",    required_argument,      NULL,           'H' },
> > >               {"no-tcp",      no_argument,            &c->no_tcp,     1 },
> > >               {"no-udp",      no_argument,            &c->no_udp,     1 },
> > >               {"no-icmp",     no_argument,            &c->no_icmp,    1 },
> > > @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               /* vhost-user backend program convention */
> > >               {"print-capabilities", no_argument,     NULL,           26 },
> > >               {"socket-path", required_argument,      NULL,           's' },
> > > +             {"fqdn",        required_argument,      NULL,           27 },
> > >               { 0 },
> > >       };
> > >       const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> > > @@ -1379,9 +1383,9 @@ void conf(struct ctx *c, int argc, char **argv)
> > >       if (c->mode == MODE_PASTA) {
> > >               c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> > >               fwd_default = FWD_AUTO;
> > > -             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> > > +             optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
> > >       } else {
> > > -             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> > > +             optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
> > >       }
> > >
> > >       c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> > > @@ -1558,6 +1562,12 @@ void conf(struct ctx *c, int argc, char **argv)
> > >               case 26:
> > >                       vu_print_capabilities();
> > >                       break;
> > > +             case 27:
> > > +                     if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > > +                                     "%s", optarg)) {  
> >
> > Nit: arguments should be visually aligned to the function you're
> > calling, that is, here:
> >
> >                         if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> >                                            "%s", optarg)) {  
> 
> Done.
> 
> >
> >  
> > > +                             die("Invalid fqdn: %s", optarg);  
> >
> > FQDN  
> 
> Done.
> 
> >  
> > > +                     }
> > > +                     break;
> > >               case 'd':
> > >                       c->debug = 1;
> > >                       c->quiet = 0;
> > > @@ -1727,6 +1737,12 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > >                       die("Cannot use DNS search domain %s", optarg);
> > >                       break;
> > > +             case 'H':
> > > +                     if (snprintf_check(c->hostname, PASST_MAXDNAME,
> > > +                                     "%s", optarg)) {  
> >
> > Same as above.
> >  
> 
> Done.
> 
> > > +                             die("Invalid hostname: %s", optarg);
> > > +                     }
> > > +                     break;
> > >               case '4':
> > >                       v4_only = true;
> > >                       v6_only = false;
> > > @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > >                       c->one_off = true;
> > >                       break;
> > > +  
> >
> > Spurious change.  
> 
> Done.
> 
> >  
> > >               case 't':
> > >               case 'u':
> > >               case 'T':
> > > diff --git a/dhcp.c b/dhcp.c
> > > index d8515aa..a04462b 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -62,6 +62,7 @@ static struct opt opts[255];
> > >  #define DHCPFORCERENEW       9
> > >
> > >  #define OPT_MIN              60 /* RFC 951 */
> > > +#define OPT_MAX              302 /* UDP packet limit - dhcp headers */  
> >
> > The original value for o[] below, 308, is 576 (see RFC 2131, page 10)
> > minus 268 (offset of options within an *IP* datagram).
> >
> > How did you calculate 302 (+2)? I can't find out.
> 
> My bad it's 306, I found that we are adding a pair of extra bytes
> after the options so
> that's why I am using 306 instead of 308 for checkings:
> 
>         m->o[offset++] = 255;
>         m->o[offset++] = 0;

Right, that's option 255 ("End") and its length (0). Can we have a
comment here actually explaining that? Say:

					/* 576 (RFC 2131), minus offset
					 * of options (268), minus end
					 * option and its length (2)
					 */

> > By the way, this reminds me that, as I mentioned previously, we should
> > really stop recycling the memory from the original message and simply
> > build a new one instead. It's not so much about security (a DHCP client
> > can anyway wreck the networking of its guest at will), but about
> > robustness. This becomes more relevant as we fill up replies more and
> > more.
> 
> Do we do this at this change or at a follow up one ?

It's a logically separated change, so it can/should be a different
patch but I think that we really risk breaking things, so I would only
push them together. It can be before or after this change.

It should be kind of simple: instead of filling up (and sending) 'm',
we should declare and initialise a separate struct msg, say:

	struct msg r = { 0 };

and use that. We'll need to copy a few fields from the request
('htype', 'hlen', 'xid', 'flags', 'giaddr', 'chaddr', see page 28 of
RFC 2131), but that's about it.

If you're lost I can also submit a patch on top of this one later, let
me know.

> > >  /**
> > >   * dhcp_init() - Initialise DHCP options
> > > @@ -122,7 +123,7 @@ struct msg {
> > >       uint8_t sname[64];
> > >       uint8_t file[128];
> > >       uint32_t magic;
> > > -     uint8_t o[308];
> > > +     uint8_t o[OPT_MAX+2];  
> >
> > Spaces around operator for consistency (OPT_MAX + 2). Should it really
> > be + 2, though?  
> 
> Yes, "fill(..)" function is adding a pair of extra ones at the end so
> we should limit the option to make space for them.
> 
>         m->o[offset++] = 255;
>         m->o[offset++] = 0;

Then we should make it clear: OPT_MAX + 2 /* End option and length */

> > >  } __attribute__((__packed__));
> > >
> > >  /**
> > > @@ -131,14 +132,28 @@ struct msg {
> > >   * @o:               Option number
> > >   * @offset:  Current offset within options field, updated on insertion
> > >   */
> > > -static void fill_one(struct msg *m, int o, int *offset)
> > > +static int fill_one(struct msg *m, int o, int *offset)  
> >
> > You should update the function comment to say what it returns, now that
> > it returns something (try: 'grep Return: *.c').
> >  
> 
> Done.
> 
> > >  {
> > > +     size_t idx, slen = 0;
> > > +
> > > +     // We cannot write even enum + len + one byte then just skip  
> >
> > For consistency, no "C++ style" comments.
> >
> > I would make it clear it's a conditional: "If we can't ..., then ..."
> >  
> 
> Done
> 
> > > +     if (*offset + 2 > OPT_MAX) {
> > > +             return OPT_MAX;
> > > +     }  
> >
> > No need for curly brackets here.  
> 
> Done.
> 
> >  
> > >       m->o[*offset] = o;
> > >       m->o[*offset + 1] = opts[o].slen;
> > > -     memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > +     idx=*offset + 2;
> > > +     slen=opts[o].slen;  
> >
> > Spaces around assignment operator (idx = *offset ...).  
> 
> Done.
> 
> >  
> > > +
> > > +     // Truncate if it goes beyond OPT_MAX  
> >
> > No C++ style comments.  
> 
> Done.
> 
> >  
> > > +     if (idx + slen > OPT_MAX) {
> > > +             slen = OPT_MAX - idx;
> > > +     }  
> >
> > No need for curly brackets.  
> 
> Done.
> 
> >  
> > > +     memcpy(&m->o[*offset + 2], opts[o].s, slen);
> > >
> > >       opts[o].sent = 1;
> > >       *offset += 2 + opts[o].slen;
> > > +     return *offset;
> > >  }
> > >
> > >  /**
> > > @@ -172,7 +187,9 @@ static int fill(struct msg *m)
> > >
> > >       for (o = 0; o < 255; o++) {
> > >               if (opts[o].slen != -1 && !opts[o].sent)
> > > -                     fill_one(m, o, &offset);
> > > +                     if (fill_one(m, o, &offset) == OPT_MAX) {
> > > +                             break;
> > > +                     }  
> >
> > No need for curly brackets. We should log a debug message, here (a
> > warning would be more appropriate but I'm a bit wary of a malicious DHCP
> > client exploiting this to make log files blow up). Something like:
> >
> >                                 debug("DHCP: truncating after option %i", o);
> >  
> 
> Done.
> 
> > >       }
> > >
> > >       m->o[offset++] = 255;
> > > @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > >       if (!opts[6].slen)
> > >               opts[6].slen = -1;
> > >
> > > +     size_t hostname_len = strlen(c->hostname);  
> >
> > No mixed declarations and code (move the hostname_len declaration at
> > the beginning of the function).  
> 
> Done.
> 
> >  
> > > +     if (hostname_len > 0) {
> > > +             opts[12].slen = hostname_len;
> > > +             memcpy(opts[12].s, &c->hostname, hostname_len);
> > > +     }
> > > +
> > > +     size_t fqdn_len = strlen(c->fqdn);  
> >
> > Same as above.  
> 
> Done.
> 
> >  
> > > +     if (fqdn_len > 0) {
> > > +             opts[81].s[0] = 0x4; // flags (E)  
> >
> > No C++ style comments.  
> 
> Done.
> 
> >  
> > > +             size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > > +                                                     (char *) opts[81].s + 3);  
> >
> > Don't mix declarations and code. I think that 'encoded_len' would be
> > clear enough here.  
> 
> Done.
> 
> >  
> > > +             opts[81].slen = encoded_fqdn_len + 3;
> > > +     }
> > > +
> > >       if (!c->no_dhcp_dns_search)
> > >               opt_set_dns_search(c, sizeof(m->o));
> > >
> > > diff --git a/dhcpv6.c b/dhcpv6.c
> > > index 0523bba..b61a41d 100644
> > > --- a/dhcpv6.c
> > > +++ b/dhcpv6.c
> > > @@ -48,6 +48,7 @@ struct opt_hdr {
> > >  # define  STATUS_NOTONLINK   htons_constant(4)
> > >  # define OPT_DNS_SERVERS     htons_constant(23)
> > >  # define OPT_DNS_SEARCH              htons_constant(24)
> > > +# define OPT_CLIENT_FQDN     htons_constant(39)
> > >  #define   STR_NOTONLINK              "Prefix not appropriate for link."
> > >
> > >       uint16_t l;
> > > @@ -163,6 +164,18 @@ struct opt_dns_search {
> > >       char list[MAXDNSRCH * NS_MAXDNAME];
> > >  } __attribute__((packed));
> > >
> > > +/**
> > > + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> > > + * @hdr:             Option header
> > > + * @flags:           Flags as stated at RFC 4704 (always zero for us)  
> >
> > "described by RFC 4704" (see also comments to v3).
> >  
> 
> Done.
> 
> > > + * @domain_name:     Client FQDN
> > > + */
> > > +struct opt_client_fqdn{
> > > +     struct opt_hdr hdr;
> > > +     uint8_t flags;
> > > +     char domain_name[PASST_MAXDNAME];
> > > +} __attribute__((packed));
> > > +
> > >  /**
> > >   * struct msg_hdr - DHCPv6 client/server message header
> > >   * @type:            DHCP message type
> > > @@ -193,6 +206,7 @@ struct msg_hdr {
> > >   * @client_id:               Client Identifier, variable length
> > >   * @dns_servers:     DNS Recursive Name Server, here just for storage size
> > >   * @dns_search:              Domain Search List, here just for storage size
> > > + * @client_fqdn:     Client FQDN, variable length
> > >   */
> > >  static struct resp_t {
> > >       struct msg_hdr hdr;
> > > @@ -203,6 +217,7 @@ static struct resp_t {
> > >       struct opt_client_id client_id;
> > >       struct opt_dns_servers dns_servers;
> > >       struct opt_dns_search dns_search;
> > > +     struct opt_client_fqdn client_fqdn;
> > >  } __attribute__((__packed__)) resp = {
> > >       { 0 },
> > >       SERVER_ID,
> > > @@ -228,6 +243,10 @@ static struct resp_t {
> > >       { { OPT_DNS_SEARCH,     0, },
> > >         { 0 },
> > >       },
> > > +
> > > +     { { OPT_CLIENT_FQDN, 0, },
> > > +       0, { 0 },
> > > +     },
> > >  };
> > >
> > >  static const struct opt_status_code sc_not_on_link = {
> > > @@ -346,7 +365,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
> > >  {
> > >       struct opt_dns_servers *srv = NULL;
> > >       struct opt_dns_search *srch = NULL;
> > > -     char *p = NULL;
> > >       int i;
> > >
> > >       if (c->no_dhcp_dns)
> > > @@ -388,29 +406,56 @@ search:
> > >                       offset += sizeof(struct opt_hdr);
> > >                       srch->hdr.t = OPT_DNS_SEARCH;
> > >                       srch->hdr.l = 0;
> > > -                     p = srch->list;
> > >               }
> > >
> > > -             *p = '.';
> > > -             p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> > > -             p++;
> > > -             srch->hdr.l += name_len + 2;
> > > -             offset += name_len + 2;
> > > +             size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);  
> >
> > No mixed declarations and code. Wrap at 80 columns if possible.  
> 
> Done.
> 
> >  
> > > +             srch->hdr.l += encoded_name_len;
> > > +             offset += encoded_name_len;
> > >       }
> > >
> > >       if (srch) {
> > > -             for (i = 0; i < srch->hdr.l; i++) {
> > > -                     if (srch->list[i] == '.') {
> > > -                             srch->list[i] = strcspn(srch->list + i + 1,
> > > -                                                     ".");
> > > -                     }
> > > -             }
> > >               srch->hdr.l = htons(srch->hdr.l);  
> >
> > Hah, thanks, that looks much better now.
> >  
> > >       }
> > >
> > >       return offset;
> > >  }
> > >
> > > +/**
> > > + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> > > + * @c:               Execution context
> > > + * @buf:     Response message buffer where options will be appended
> > > + * @offset:  Offset in message buffer for new options
> > > + *
> > > + * Return: updated length of response message buffer.
> > > + */
> > > +static size_t dhcpv6_client_fqdn_fill(const struct ctx *c, char *buf, int offset)
> > > +{
> > > +     size_t fqdn_len, opt_hdr_len = 0;
> > > +
> > > +     fqdn_len = strlen(c->fqdn);
> > > +     opt_hdr_len = sizeof(struct opt_hdr);
> > > +
> > > +     if (offset + opt_hdr_len + fqdn_len + 1 > 508) {  
> >
> > I'm a bit lost: why 508? Would it be possible to have this as a define
> > using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?
> 
> That's the max UDP packet payload size, since "offset" is referring to
> the payload not the whole UDP
> maybe we can do we can do max_len = 576 - (sizeof(struct ipv6hdr) +
> sizeof(udphdr)), don't know if it's worth it.

Well, that would be 528... so I think it's worth it. :)

But in any case, this is not IPv4. From RFC 8415:

   The server must be aware of the recommendations on packet sizes and
   the use of fragmentation as discussed in Section 5 of [RFC8200].

...so we should start from 1280 (IPV6_MIN_MTU) bytes here, subtract the
size of an IPv6 header (40), of a UDP header (8), and of the message
header (sizeof(struct msg_hdr), 4).

I think we should really use defined constants and sizeof() otherwise
it gets confusing quite fast.

> " * @dlen:       UDP payload length (not including UDP header)"

I guess we could have it as a define (OPT_MAX_SIZE?), no need to have
it in any struct.

By the way, note that we have MAX() defined in util.h, so you don't
need a condition and a separate statement, you can just do:

> > > +             fqdn_len = 508 - (offset + opt_hdr_len + 1);

	fqdn_len = MAX(fqdn_len, offset + ...);

-- 
Stefano