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

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

On Wed, Jan 22, 2025 at 1:29 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Wed, 22 Jan 2025 13:09:07 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Thu, Jan 16, 2025 at 1:31 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > Thanks for sticking with this. The general approach and almost the
> > > whole patch looks good to me now, but there are still a couple of
> > > issues with the options:
> > >
> > > On Wed, 15 Jan 2025 09:18:44 +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           | 20 +++++++++--
> > > >  dhcp.c           | 60 ++++++++++++++++++++++++++-----
> > > >  dhcpv6.c         | 93 ++++++++++++++++++++++++++++++++++++++++--------
> > > >  passt.1          | 10 ++++++
> > > >  passt.h          |  5 +++
> > > >  pasta.c          | 17 ++++++---
> > > >  test/lib/setup   | 10 +++---
> > > >  test/passt.mbuto |  6 ++--
> > > >  test/passt/dhcp  | 15 +++++++-
> > > >  util.c           | 22 ++++++++++++
> > > >  util.h           |  6 ++++
> > > >  11 files changed, 226 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/conf.c b/conf.c
> > > > index df2b016..0cbd625 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,11 @@ 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))
> > > > +                             die("Invalid FQDN: %s", optarg);
> > > > +                     break;
> > > >               case 'd':
> > > >                       c->debug = 1;
> > > >                       c->quiet = 0;
> > > > @@ -1727,6 +1736,11 @@ 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))
> > > > +                             die("Invalid hostname: %s", optarg);
> > > > +                     break;
> > > >               case '4':
> > > >                       v4_only = true;
> > > >                       v6_only = false;
> > > > diff --git a/dhcp.c b/dhcp.c
> > > > index d8515aa..ba66f47 100644
> > > > --- a/dhcp.c
> > > > +++ b/dhcp.c
> > > > @@ -63,6 +63,11 @@ static struct opt opts[255];
> > > >
> > > >  #define OPT_MIN              60 /* RFC 951 */
> > > >
> > > > +/* Total option size (excluding end option) is 576 (RFC 2131), minus
> > > > + * offset of options (268), minus end option and its length (2).
> > > > + */
> > > > +#define OPT_MAX              306
> > > > +
> > > >  /**
> > > >   * dhcp_init() - Initialise DHCP options
> > > >   */
> > > > @@ -122,7 +127,7 @@ struct msg {
> > > >       uint8_t sname[64];
> > > >       uint8_t file[128];
> > > >       uint32_t magic;
> > > > -     uint8_t o[308];
> > > > +     uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
> > > >  } __attribute__((__packed__));
> > > >
> > > >  /**
> > > > @@ -130,15 +135,28 @@ struct msg {
> > > >   * @m:               Message to fill
> > > >   * @o:               Option number
> > > >   * @offset:  Current offset within options field, updated on insertion
> > > > + *
> > > > + * Return: false if m has space to write the option, true otherwise
> > > >   */
> > > > -static void fill_one(struct msg *m, int o, int *offset)
> > > > +static bool fill_one(struct msg *m, int o, int *offset)
> > > >  {
> > > > +     size_t slen = opts[o].slen;
> > > > +
> > > > +     /* If we don't have space to write the option, then just skip */
> > > > +     if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> > > > +             return true;
> > > > +
> > > >       m->o[*offset] = o;
> > > > -     m->o[*offset + 1] = opts[o].slen;
> > > > -     memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > > +     m->o[*offset + 1] = slen;
> > > > +
> > > > +     /* Move to option */
> > > > +     *offset += 2;
> > > > +
> > > > +     memcpy(&m->o[*offset], opts[o].s, slen);
> > > >
> > > >       opts[o].sent = 1;
> > > > -     *offset += 2 + opts[o].slen;
> > > > +     *offset += slen;
> > > > +     return false;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -162,17 +180,20 @@ static int fill(struct msg *m)
> > > >        * Put it there explicitly, unless requested via option 55.
> > > >        */
> > > >       if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> > > > -             fill_one(m, 53, &offset);
> > > > +             if (fill_one(m, 53, &offset))
> > > > +                      debug("DHCP: skipping option 55");
> > >
> > > We're skipping option 53 in this case, not 55.
> > >
> > > >
> > > >       for (i = 0; i < opts[55].clen; i++) {
> > > >               o = opts[55].c[i];
> > > >               if (opts[o].slen != -1)
> > > > -                     fill_one(m, o, &offset);
> > > > +                     if (fill_one(m, o, &offset))
> > > > +                             debug("DHCP: skipping option %i", o);
> > > >       }
> > > >
> > > >       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))
> > > > +                             debug("DHCP: skipping option %i", o);
> > > >       }
> > > >
> > > >       m->o[offset++] = 255;
> > > > @@ -398,6 +419,29 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > >       if (!opts[6].slen)
> > > >               opts[6].slen = -1;
> > > >
> > > > +     opt_len = strlen(c->hostname);
> > > > +     if (opt_len > 0) {
> > > > +             opts[12].slen = opt_len;
> > > > +             memcpy(opts[12].s, &c->hostname, opt_len);
> > > > +     }
> > >
> > > So I tried (note the comma):
> > >
> > > $ ./passt -d -f --hostname thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap
> > >
> > > $ ./passt -d -f --hostname , -p fqdn.pcap
> > >
> > > $ ./passt -d -f --hostname "" -p fqdn.pcap
> > >
> > > the hostname and the DHCP reply are fine, but we get an empty FQDN
> > > option in the DHCPv6 reply (only).
> > >
> > > Note that RFC 2132 (3.14) says, about option 12:
> > >
> > >   See RFC 1035 for character set restrictions.
> > >
> > > but I'm not sure if we should validate the user input any further. I
> > > wouldn't. One might legitimately use passt to test the robustness of a
> > > DHCP client.
> > >
> >
> > I understand that we are talking about two different things:
> > - The fact that adding "," to option 12 is not ok
>
> No, I think it's actually fine, I just wanted to make you aware of that.
>
> > - And DHCPv6 is broken if we don't pass --fqdn because it adds a "."
> > field that breaks it.
>
> Right, yes.
>
> > I will fix the DHCPv6 FQDN client option, not sure about validation
> > input, but it's true that more or less
> > we are going to send virtual machine names at hostnames and by
> > kubernetes standard those should be a fqdn,
> > so they are implicitly validated.
>
> Well, this is completely independent of what Kubernetes might do with
> it one day. But still, I would *not* validate it because passt is also
> quite useful (and used) to test this kind of stuff.
>
> > > > +
> > > > +     opt_len = strlen(c->fqdn);
> > > > +     if (opt_len > 0) {
> > > > +             opt_len += 3 /* flags */
> > > > +                     + 2; /* Length byte for first label, and terminator */
> > > > +
> > > > +             if (sizeof(opts[81].s) < opt_len)
> > > > +                     debug("DHCP: client FQDN option doesn't fit, skipping");
> > >
> > > For this one, I think you actually need to maintain the check in the
> > > caller of encode_domain_name() (that is, here), because of the three
> > > bytes for the flags.
> >
> > Right, I am not "skipping", I will fix that.
> >
> > > Otherwise, with this:
> > >
> > >   $ ./passt -d -f --fqdn
> > >   thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap
> > >
> > > this message is printed, but we end up with an option 81 with length
> > > 2, and the FQDN ends up in the padding (try and see capture).
> > >
> > > With this:
> > >
> > >   $ ./passt -d -f --fqdn
> > >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one -p fqdn.pcap
> > >
> > > length becomes 1, and the FQDN is still in the padding.
> > >
> > > With this:
> > >
> > >   $ ./passt -d -f --fqdn
> > >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.make_that_251_with_this_one -p fqdn.pcap
> > >
> > > length becomes 0, end option is missing, and we have an option 4 (time
> > > server) instead.
> >
> > After the fix I see just proper padding with 0 at the end, is this fine ?
>
> Probably. We just shouldn't have unnecessary padding.
>
> By the way, this change should depend on another change *not* reusing
> the original message as buffer for the reply, as discussed.
>

Totally forgot about that, should I make a patchset or different changes ?

> > > With this:
> > >
> > >   $ ./passt -d -f --fqdn
> > >  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_its_250_with_this_one -p fqdn.pcap
> > >
> > > things are almost fine. We get one byte of padding, which is probably
> > > not needed, but I didn't look into it.
> >
> > And at this case I see proper option 81 configured with length 255
> > (250 + 3 /* flags */ + 2 /* first length and terminator */)
> >
> > So with the fix now it looks good, I am super worried that we don't
> > have this at the tests, but we will go back to it when the proper
> > testing framework lands.
>
> I'm not, in the sense that this isn't really security relevant *unless
> we leak host memory into the guest*. The rest is a functional issue,
> and it's not easy to break this "by mistake".
>
> Sure, ideally we should have tests for those, but... they don't break
> by themselves. This code is pretty much isolated and stand-alone.
>

Fair enough.

> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat