From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=iedYxZ7P; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id 59F895A061E for ; Wed, 22 Jan 2025 14:20:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737552038; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=43vxxaybWfTrPeeMdeJRIm2SjdsBiWA8wzL7Kr18ioY=; b=iedYxZ7PinntHBy+XHR6MbBh9OQwLk+KeI2Dc/HmfXYazYmcVATd+3NKxHSUgjLYz3bQ4K 4Kqi9Tku3ySJ8V5chd4rH1mD1FT/UMWBbAN5IUE2i1uYayaH1ffRitvE42NdeZYrlUCtie aiN3ebm2eURiiv57ZBZoUx7/v2yBZxk= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-186-mB7trNkFNbu3uFhCMKKpmQ-1; Wed, 22 Jan 2025 08:20:37 -0500 X-MC-Unique: mB7trNkFNbu3uFhCMKKpmQ-1 X-Mimecast-MFC-AGG-ID: mB7trNkFNbu3uFhCMKKpmQ Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-ab367e406b3so606237666b.0 for ; Wed, 22 Jan 2025 05:20:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737552035; x=1738156835; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=43vxxaybWfTrPeeMdeJRIm2SjdsBiWA8wzL7Kr18ioY=; b=NmSxlbiYG5PH+UwvLuBKrv64DC/iBVm/mbFYsDmNa0+K4asZmiPfq+tV0corKWZ+0p QOclgCktYIOyRG4rqRd4UU7wdssoTh5d8g2Rq3qlVHyp3PMsaue7dI62QCuzZHN7v3VO vV6rjXrA5CIY0Ni0SGjXori7EPcKkYMwvIRODFCxJVmXz/E7JDXl5Eg0CzC410LnBLf4 uZqFsKRwifxrSer5UCn7uvrEScnp40bIvlaO5b1el8xNzx3cLbnDuT5mDv0J7F80pPNa 5iTjSa/xO27WWktlcq4d9oVkgiSzkdBlHA1tzSyZoVSuQOFZBXHHTK5hK0U0gpBj+azJ w7Zw== X-Gm-Message-State: AOJu0Yz97q6eVbdNN09qPf3jnkfM90EkjXCLqh56X2IFw/BEJyQKECjs BpaEbAJX0iR6G1JGtqBxEJp9r6Ckxkwr4KIJRus2PVUYr6QZSB5AYOAgxuxPaZjRvwa0yD3qiDY G3Ds0CtgGgrxm9ScfvZXdHMNwNFB3HJJy5x2j52CFAnP7eHTUElaBFu79V9ZwofyLvqypPSDgAW EczMmjx2ASM4bjXCXkLG6m/L5po9ArRwdC X-Gm-Gg: ASbGncu/n6U6GwEH3gHBv4k0MmGQ1OniatQsXeaH6gaV7+oqnJDltppSW/vaDt4+Gj1 lmZhVTcYG+4t9ggnQiO1mwCAUhZDESNfd1YHlyZ5w1kkXHvA46/v4BI5COkLCio9QEV7v7RIw/3 OijsT6w5odiw== X-Received: by 2002:a17:907:d0f:b0:aa6:a9fe:46dd with SMTP id a640c23a62f3a-ab38b3aff9fmr2258884866b.38.1737552034521; Wed, 22 Jan 2025 05:20:34 -0800 (PST) X-Google-Smtp-Source: AGHT+IE2QUdmEFw7yuVEopBL1HaAV6G5dIgii9nNSDcUwjzyjwc4H6aOQGUTBO0U+D7RwVdgVgA+E4jP841BRHQvpEI= X-Received: by 2002:a17:907:d0f:b0:aa6:a9fe:46dd with SMTP id a640c23a62f3a-ab38b3aff9fmr2258880166b.38.1737552033945; Wed, 22 Jan 2025 05:20:33 -0800 (PST) MIME-Version: 1.0 References: <20250115081844.27020-1-ellorent@redhat.com> <20250116013058.0871d04d@elisabeth> <20250122132932.21f30b7c@elisabeth> In-Reply-To: <20250122132932.21f30b7c@elisabeth> From: Enrique Llorente Pastora Date: Wed, 22 Jan 2025 14:20:22 +0100 X-Gm-Features: AbW1kva9mFyF0tc0g9-0F4hxDhW4YzhoycpRYuhmq8e_9vWVglUz94WLNdWaVyI Message-ID: Subject: Re: [PATCH v9] dhcp, dhcpv6: Add hostname and client fqdn ops To: Stefano Brivio X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: Lh_mCxJB1aCtzre0zZrKRbDwIUYcR0TazJAk1sNLhCU_1737552036 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: A3PNWGH7PQQOR6MXXRQNOMBLW4WV7QV2 X-Message-ID-Hash: A3PNWGH7PQQOR6MXXRQNOMBLW4WV7QV2 X-MailFrom: ellorent@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, Jan 22, 2025 at 1:29=E2=80=AFPM Stefano Brivio = wrote: > > On Wed, 22 Jan 2025 13:09:07 +0100 > Enrique Llorente Pastora wrote: > > > On Thu, Jan 16, 2025 at 1:31=E2=80=AFAM Stefano Brivio 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 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 > > > > --- > > > > 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, in= t status) > > > > FPRINTF(f, " default: use addresses from /etc/reso= lv.conf\n"); > > > > FPRINTF(f, > > > > " -S, --search LIST Space-separated list, search = domains\n" > > > > - " a single, empty option disables the DNS search l= ist\n"); > > > > + " a single, empty option disables the DNS search l= ist\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 **arg= v) > > > > {"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 **arg= v) > > > > /* 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 =3D (c->mode =3D=3D MODE_PASTA) ? "pasta"= : "passt"; > > > > @@ -1379,9 +1383,9 @@ void conf(struct ctx *c, int argc, char **arg= v) > > > > if (c->mode =3D=3D MODE_PASTA) { > > > > c->no_dhcp_dns =3D c->no_dhcp_dns_search =3D 1; > > > > fwd_default =3D FWD_AUTO; > > > > - optstring =3D "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:= u:T:U:"; > > > > + optstring =3D "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46= t:u:T:U:"; > > > > } else { > > > > - optstring =3D "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t= :u:"; > > > > + optstring =3D "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:46= 1t:u:"; > > > > } > > > > > > > > c->tcp.fwd_in.mode =3D c->tcp.fwd_out.mode =3D FWD_UNSET; > > > > @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **ar= gv) > > > > 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 =3D 1; > > > > c->quiet =3D 0; > > > > @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **ar= gv) > > > > > > > > die("Cannot use DNS search domain %s", optarg= ); > > > > break; > > > > + case 'H': > > > > + if (snprintf_check(c->hostname, PASST_MAXDNAM= E, > > > > + "%s", optarg)) > > > > + die("Invalid hostname: %s", optarg); > > > > + break; > > > > case '4': > > > > v4_only =3D true; > > > > v6_only =3D 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), min= us > > > > + * 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 inser= tion > > > > + * > > > > + * Return: false if m has space to write the option, true otherwis= e > > > > */ > > > > -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 =3D 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] =3D o; > > > > - m->o[*offset + 1] =3D opts[o].slen; > > > > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen); > > > > + m->o[*offset + 1] =3D slen; > > > > + > > > > + /* Move to option */ > > > > + *offset +=3D 2; > > > > + > > > > + memcpy(&m->o[*offset], opts[o].s, slen); > > > > > > > > opts[o].sent =3D 1; > > > > - *offset +=3D 2 + opts[o].slen; > > > > + *offset +=3D 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].cle= n)) > > > > - 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 =3D 0; i < opts[55].clen; i++) { > > > > o =3D opts[55].c[i]; > > > > if (opts[o].slen !=3D -1) > > > > - fill_one(m, o, &offset); > > > > + if (fill_one(m, o, &offset)) > > > > + debug("DHCP: skipping option %i", o); > > > > } > > > > > > > > for (o =3D 0; o < 255; o++) { > > > > if (opts[o].slen !=3D -1 && !opts[o].sent) > > > > - fill_one(m, o, &offset); > > > > + if (fill_one(m, o, &offset)) > > > > + debug("DHCP: skipping option %i", o); > > > > } > > > > > > > > m->o[offset++] =3D 255; > > > > @@ -398,6 +419,29 @@ int dhcp(const struct ctx *c, const struct poo= l *p) > > > > if (!opts[6].slen) > > > > opts[6].slen =3D -1; > > > > > > > > + opt_len =3D strlen(c->hostname); > > > > + if (opt_len > 0) { > > > > + opts[12].slen =3D opt_len; > > > > + memcpy(opts[12].s, &c->hostname, opt_len); > > > > + } > > > > > > So I tried (note the comma): > > > > > > $ ./passt -d -f --hostname thirtytwocharactersforeachlabel.thirtytwoc= haractersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersfo= reachlabel.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 =3D strlen(c->fqdn); > > > > + if (opt_len > 0) { > > > > + opt_len +=3D 3 /* flags */ > > > > + + 2; /* Length byte for first label, and term= inator */ > > > > + > > > > + 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.thi= rtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwochara= ctersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeac= hlabel.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.thir= tytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharac= tersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeach= label.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.thir= tytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharac= tersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeach= label.make_that_251_with_this_one -p fqdn.pcap > > > > > > length becomes 0, end option is missing, and we have an option 4 (tim= e > > > 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.thir= tytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharac= tersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeach= label.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 > --=20 Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat