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=SyXWJKPI; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 6C1795A061E for ; Wed, 22 Jan 2025 13:29:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737548979; 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=PwUuiXxQRzMP3UA46aicQP1ETfMxRQK+6XttPb/HAQg=; b=SyXWJKPIeoezvs/k+nOuxV3Kfr0pu/4ai85q7S6WN2yOiXzg0aSc5r2KJXsTrdAHYMtfx0 QoJvT5EFEuYDrRWDKXOMCvqhG5s47fhJ37JhQ+gFx94bGKb4apMwLMKGj/67HSStpdLrkb ukikYANAByZpSgb0yV8dlenruOv8iiY= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-621-hpn2SQGNM0mtKTXZTzny3w-1; Wed, 22 Jan 2025 07:29:38 -0500 X-MC-Unique: hpn2SQGNM0mtKTXZTzny3w-1 X-Mimecast-MFC-AGG-ID: hpn2SQGNM0mtKTXZTzny3w Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-4361efc9dc6so37085965e9.3 for ; Wed, 22 Jan 2025 04:29:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737548975; x=1738153775; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=fd9vF5HsPgQUNH6tT0tB57PFGHs0I86UIrAT1Eytj0s=; b=gpOMoeCpvQV1146B/e9uHRhT5mpgP0gwXyNksLlGx4fSh9YQ69xdcvPTZNcwT+BlHB scKn6bI753lJIcXpXvxx0mQD/0HqXdatjsb12Nl9hCfG0NqNYTKrg/sr/rwDmjS9oJFV azJYbVRbOhISCS+Sc+uUGtLW/afV1aHxbmuG8I7+ksrTLKE4CiIFQwc3+brVSfXez9Bc APWIZkJ9bVf9PNX/Op3eNppBXTN8r53d7TtHsZVlUzfcSJSTOxCo1xROnk5SAH0gaIO8 urpVf7fwhcL0JLVVSwmwjEGUakhIbpZpTFU8c9pANatCmSAELfSrW1vurPoFwBVOfwh2 id8g== X-Gm-Message-State: AOJu0YwJcD1t602R8IQwbI9FwuD6fm7jTx8FaUnDjvjuOJDQ6DqYt0oH ftBbD9fCjUR8W4dmQLGZpjcOOOXj8rS+Yrqb37agQt0Y7esTIpZ4RAVWLcqcC3qHlNryGxg0f/d sIPIczOTsMUnEU1GqeiPG17bjKPVCCyfLlTjeUleYG8C3b76VPpCQf8eZbDrtm0UePJ34vsqvQS hI+dME6O9DPVu6HXtcYs3snkL4XfnS/fpK X-Gm-Gg: ASbGncseKLEP+xMpkoWbY9RYbD7trAG7BRk3lmN74k3MUc/fBG9jsvLTUM6HTmngIMv joGw4g/RvT3aLpshDamSpBEFqgWdVbuL3YrrU47AXERFA2h86mBBYwSeBefw15xohtrjA8rF1v6 v1IpTNJ/4nzi2N0g4cFBkVkT+PjGIFIejEmuxov/YU2zD4eM27S2IbzhSDWlkYEDKLW6xmOxEMY mCkxrEbJKyptuL4GuIi7vFIi7dttMFYz47LKhZ+O1vCRnmYmXcrMytTOJ+1bM/KeV0r89rtdpm9 sIYjwg== X-Received: by 2002:a05:600c:3542:b0:434:9e1d:7626 with SMTP id 5b1f17b1804b1-4389145137dmr168373025e9.25.1737548975378; Wed, 22 Jan 2025 04:29:35 -0800 (PST) X-Google-Smtp-Source: AGHT+IHRmyj2qvU60shn3FsgXIf+muKAIVAbaASKGNbLyWZTMbC6P6U309ppaa7M/DX3iUl4v9K+3g== X-Received: by 2002:a05:600c:3542:b0:434:9e1d:7626 with SMTP id 5b1f17b1804b1-4389145137dmr168372795e9.25.1737548974802; Wed, 22 Jan 2025 04:29:34 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-438b318beadsm23203755e9.8.2025.01.22.04.29.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 22 Jan 2025 04:29:34 -0800 (PST) Date: Wed, 22 Jan 2025 13:29:32 +0100 From: Stefano Brivio To: Enrique Llorente Pastora Subject: Re: [PATCH v9] dhcp, dhcpv6: Add hostname and client fqdn ops Message-ID: <20250122132932.21f30b7c@elisabeth> In-Reply-To: References: <20250115081844.27020-1-ellorent@redhat.com> <20250116013058.0871d04d@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 6sLnAlhUGQL9b1AEFpVmYZ-HEyq0Ge0QLeF_QRdL9f0_1737548977 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: L2EDUTKW5ODFBOW3YCGGMNJJF2H5II6L X-Message-ID-Hash: L2EDUTKW5ODFBOW3YCGGMNJJF2H5II6L X-MailFrom: sbrivio@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, 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: > > =20 > > > Both DHCPv4 and DHCPv6 has the capability to pass the hostname to > > > clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses o= ption 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) an= d > > > 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, int = status) > > > FPRINTF(f, " default: use addresses from /etc/resolv= .conf\n"); > > > FPRINTF(f, > > > " -S, --search LIST Space-separated list, search do= mains\n" > > > - " a single, empty option disables the DNS search lis= t\n"); > > > + " a single, empty option disables the DNS search lis= t\n" > > > + " -H, --hostname NAME Hostname to configure client wi= th\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 =3D (c->mode =3D=3D MODE_PASTA) ? "pasta" := "passt"; > > > @@ -1379,9 +1383,9 @@ void conf(struct ctx *c, int argc, char **argv) > > > 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:46t:= 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:461t= :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 **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 =3D 1; > > > c->quiet =3D 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 =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), 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 inserti= on > > > + * > > > + * 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 =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].clen)= ) > > > - fill_one(m, 53, &offset); > > > + if (fill_one(m, 53, &offset)) > > > + debug("DHCP: skipping option 55"); =20 > > > > We're skipping option 53 in this case, not 55. > > =20 > > > > > > 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 pool = *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); > > > + } =20 > > > > So I tried (note the comma): > > > > $ ./passt -d -f --hostname thirtytwocharactersforeachlabel.thirtytwocha= ractersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersfore= achlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.th= irtytwocharactersforeachlabel.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. > > =20 >=20 > 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 termin= ator */ > > > + > > > + if (sizeof(opts[81].s) < opt_len) > > > + debug("DHCP: client FQDN option doesn't fit, sk= ipping"); =20 > > > > 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. >=20 > Right, I am not "skipping", I will fix that. >=20 > > Otherwise, with this: > > > > $ ./passt -d -f --fqdn > > thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirt= ytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharact= ersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachl= abel.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.thirty= twocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharacte= rsforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachla= bel.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.thirty= twocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharacte= rsforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachla= bel.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. >=20 > 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. > > With this: > > > > $ ./passt -d -f --fqdn > > thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirty= twocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharacte= rsforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachla= bel.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. >=20 > And at this case I see proper option 81 configured with length 255 > (250 + 3 /* flags */ + 2 /* first length and terminator */) >=20 > 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. --=20 Stefano