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

[Enrique Llorente Pastora <ellorent@redhat.com>]

On Thu, Dec 19, 2024 at 1:19 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> 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)
>                                          */

Sure, will add the comment.

>
> > > 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.

I will do a new patch after this one to cover that.

>
> > > >  /**
> > > >   * 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 */
>

Make sense adding that too.

> > > >  } __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:

I am going to create a new OPT_MAX_SIZE with the computation and also a new
IPV6_MIN_MTU to be part of it and use that later on with MIN.

>
> > > > +             fqdn_len = 508 - (offset + opt_hdr_len + 1);
>
>         fqdn_len = MAX(fqdn_len, offset + ...);
>
> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat