On Thu, Jan 9, 2025 at 9:16 AM Stefano Brivio wrote:

Just coding style comments, plus one functional issue, but it should be relatively contained:

On Wed, 8 Jan 2025 15:58:11 +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 | 54 ++++++++++++++++++++++++++---- dhcpv6.c | 85 +++++++++++++++++++++++++++++++++++++++--------- passt.1 | 10 ++++++ passt.h | 5 +++ pasta.c | 17 +++++++--- test/lib/setup | 10 +++--- test/passt.mbuto | 6 ++-- test/passt/dhcp | 15 ++++++++- util.c | 31 ++++++++++++++++++ util.h | 6 ++++ 11 files changed, 222 insertions(+), 37 deletions(-)

diff --git a/conf.c b/conf.c index df2b016..554e5c3 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))

Why? This was fine in v6. I'm not sure what happened here. In case your editor/email client doesn't show this, now it's:

tab tab tab tab tab tab tab tab tab tab tab tab space "%s", optarg))

that is, twelve tabs and one space. See also:

https://archives.passt.top/passt-dev/20250108145811.833308-1-ellorent@redhat...

It should be:

if (snprintf_check(c->hostname, PASST_MAXDNAME, "%s", optarg))

tab tab tab tab tab space space space "%s", optarg))

that is, five tabs and three spaces.

Fixed, no clue how I de-formated it.

...

+ die("Invalid hostname: %s", optarg); + break; case '4': v4_only = true; v6_only = false; diff --git a/dhcp.c b/dhcp.c index d8515aa..50c220d 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*/];

Same as my comment to v6:

"Spaces around /* ... */ wouldn't be bad for readability."

Right, I understood the opposite, fixing.

...

} __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: offset for the next option field or -1 if option do not fit

Same as my comment to v6:

"doesn't fit"

It's a bit weird, but harmless, that this function updates 'offset', and also returns it (unless there's an error). Maybe it saves a bit of typing in callers, though (I would have just returned offset or -1 at this point, without passing a pointer).

Make sense to not pass a pointer to offset, I will change it.

...

*/ -static void fill_one(struct msg *m, int o, int *offset) +static ssize_t 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 -1; + 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 *offset; }

/** @@ -171,8 +189,11 @@ 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 (opts[o].slen != -1 && !opts[o].sent) { + if (fill_one(m, o, &offset) == -1) {

Curly brackets not needed.

Done.

...

+ debug("DHCP: skipping option %i", o); + } + } }

m->o[offset++] = 255; @@ -398,6 +419,25 @@ 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); + } + + opt_len = strlen(c->fqdn); + if (opt_len > 0) { + size_t encoded_len; + encoded_len = encode_domain_name((char *)opts[81].s + 3, sizeof(opts[81].s) - 3, + c->fqdn, opt_len);

Arguments are aligned properly, but it exceeds 80 columns for no compelling reason. It should be:

encoded_len = encode_domain_name((char *)opts[81].s + 3, sizeof(opts[81].s) - 3, c->fqdn, opt_len);

Done.

...

+ if (encoded_len > 0 ) {

Excess whitespace between 0 and ).

Done.

...

+ opts[81].s[0] = 0x4; /* flags (E) */ + opts[81].s[1] = 0xff; /* RCODE1 */ + opts[81].s[2] = 0xff; /* RCODE2 */ + opts[81].slen = encoded_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..07ce768 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; @@ -58,6 +59,9 @@ struct opt_hdr { sizeof(struct opt_hdr)) #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \ sizeof(struct opt_hdr)) +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \ + sizeof(struct udphdr) + \ + sizeof(struct msg_hdr))

/** * struct opt_client_id - DHCPv6 Client Identifier option @@ -163,6 +167,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 described by RFC 4704 + * @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 +209,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 +220,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 +246,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 +368,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) @@ -373,6 +394,7 @@ search: return offset;

for (i = 0; *c->dns_search[i].n; i++) { + size_t encoded_name_len; size_t name_len = strlen(c->dns_search[i].n);

/* We already append separators, don't duplicate if present */ @@ -388,29 +410,61 @@ 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; + encoded_name_len = encode_domain_name(srch->list, NS_MAXDNAME, + c->dns_search[i].n, + name_len); + srch->hdr.l += encoded_name_len;

...not if encoded_name_len is -1. In that case, you could also have an integer overflow, other than broken functionality.

Right, I will check -1 and skip on that case. Also I have to move the list buffer or I will be storing it at the first element. encoded_name_len = encode_domain_name(srch->list + srch->hdr.l, NS_MAXDNAME, c->dns_search[i].n, 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, - "."); - } - } + if (srch) srch->hdr.l = htons(srch->hdr.l); - }

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 pool *p, const struct ctx *c, + char *buf, int offset) + +{ + ssize_t opt_len; + struct opt_client_fqdn *o; + struct opt_client_fqdn const *req_opt;

Same as my comment to v6:

"These should go from longest to shortest."

Done.

...

+ + opt_len = MIN(PASST_MAXDNAME, OPT_MAX_SIZE - (offset + sizeof(struct opt_hdr) + 1/*flags*/));

This should be PASST_MAXDNAME + 1, otherwise:

./passt -d -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one

19.4621: DHCPv6: received REQUEST/RENEW/CONFIRM, sending REPLY 19.4621: DHCPv6: client FQDN option does not fit, skipping

Make sense I will change it

...but that domain name is 252 characters, and it's accepted (longer domains are not accepted, which makes me think: should PASST_MAXDNAME be 254 instead, as it includes the terminator?).

We are also using that const for validation (and on that case we don't count the terminator) so we cannot modify it if (snprintf_check(c->fqdn, PASST_MAXDNAME, "%s", optarg)) die("Invalid FQDN: %s", optarg);

If it's one byte shorter, then it works (254 bytes option size, 253 bytes of encoded domain, 251 of original domain).

Once this is fixed, this reveals an issue in encode_domain_name(), more on that below.

By the way, this exceeds 80 columns for no particular reason, and "+ 1 /* flags */" is probably a bit more readable.

...

+ + o = (struct opt_client_fqdn *)(buf + offset); + opt_len = encode_domain_name(o->domain_name, opt_len, c->fqdn, strlen(c->fqdn));

This now exceeds 80 columns.

Done.

...

+ if (opt_len == -1) { + debug("DHCPv6: client FQDN option does not fit, skipping"); + return offset; + } + + req_opt = (struct opt_client_fqdn *)dhcpv6_opt(p, &(size_t){ 0 }, + OPT_CLIENT_FQDN); + if (req_opt && req_opt->flags & 0x01 /* S flag */) + o->flags = 0x02 /* O flag */; + else + o->flags = 0x00; + + opt_len++; + + o->hdr.t = OPT_CLIENT_FQDN; + o->hdr.l = htons(opt_len);

It's much easier to follow now, I think.

...

+ + return offset + sizeof(struct opt_hdr) + opt_len; +} + /** * dhcpv6() - Check if this is a DHCPv6 message, reply as needed * @c: Execution context @@ -544,6 +598,7 @@ int dhcpv6(struct ctx *c, const struct pool *p, n = offsetof(struct resp_t, client_id) + sizeof(struct opt_hdr) + ntohs(client_id->l); n = dhcpv6_dns_fill(c, (char *)&resp, n); + n = dhcpv6_client_fqdn_fill(p, c, (char *)&resp, n);

resp.hdr.xid = mh->xid;

diff --git a/passt.1 b/passt.1 index d9cd33e..7051fc4 100644 --- a/passt.1 +++ b/passt.1 @@ -401,6 +401,16 @@ Enable IPv6-only operation. IPv4 traffic will be ignored. By default, IPv4 operation is enabled as long as at least an IPv4 route and an interface address are configured on a given host interface.

+.TP +.BR \-H ", " \-\-hostname " " \fIname +Hostname to configure the client with. +Send \fIname\fR as DHCP option 12 (hostname). + +.TP +.BR \-\-fqdn " " \fIname +FQDN to configure the client with. +Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39. + .SS \fBpasst\fR-only options

.TP diff --git a/passt.h b/passt.h index 0dd4efa..f3151f0 100644 --- a/passt.h +++ b/passt.h @@ -209,6 +209,8 @@ struct ip6_ctx { * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled * @ip: IPv4 configuration * @dns_search: DNS search list + * @hostname: Guest hostname + * @fqdn: Guest FQDN * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled * @ip6: IPv6 configuration * @pasta_ifn: Name of namespace interface for pasta @@ -269,6 +271,9 @@ struct ctx {

struct fqdn dns_search[MAXDNSRCH];

+ char hostname[PASST_MAXDNAME]; + char fqdn[PASST_MAXDNAME]; + int ifi6; struct ip6_ctx ip6;

diff --git a/pasta.c b/pasta.c index ff41c95..f3b4cae 100644 --- a/pasta.c +++ b/pasta.c @@ -169,10 +169,12 @@ void pasta_open_ns(struct ctx *c, const char *netns) * struct pasta_spawn_cmd_arg - Argument for pasta_spawn_cmd() * @exe: Executable to run * @argv: Command and arguments to run + * @ctx: Context to read config from */ struct pasta_spawn_cmd_arg { const char *exe; char *const *argv; + struct ctx *c; };

/** @@ -186,6 +188,7 @@ static int pasta_spawn_cmd(void *arg) { char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX; const struct pasta_spawn_cmd_arg *a; + size_t conf_hostname_len; sigset_t set;

/* We run in a detached PID and mount namespace: mount /proc over */ @@ -195,9 +198,15 @@ static int pasta_spawn_cmd(void *arg) if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) warn("Cannot set ping_group_range, ICMP requests might fail");

- if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || - errno == ENAMETOOLONG) { + a = (const struct pasta_spawn_cmd_arg *)arg; + + conf_hostname_len = strlen(a->c->hostname); + if (conf_hostname_len > 0) { + if (sethostname(a->c->hostname, conf_hostname_len)) + warn("Unable to set configured hostname"); + } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || + errno == ENAMETOOLONG) {

Same as my comment to v6, which was the same as my comment to v5...:

"Same as for v5: this is an argument to gethostname(), so it should be aligned accordingly. ! is a unary operator, it doesn't take two operands.

That is:

} else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||

see also the version without your patch."

...

hostname[HOST_NAME_MAX] = '\0'; if (sethostname(hostname, strlen(hostname))) warn("Unable to set pasta-prefixed hostname");

Done.

...

@@ -208,7 +217,6 @@ static int pasta_spawn_cmd(void *arg) sigaddset(&set, SIGUSR1); sigwaitinfo(&set, NULL);

- a = (const struct pasta_spawn_cmd_arg *)arg; execvp(a->exe, a->argv);

die_perror("Failed to start command or shell"); @@ -230,6 +238,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, struct pasta_spawn_cmd_arg arg = { .exe = argv[0], .argv = argv, + .c = c, }; char uidmap[BUFSIZ], gidmap[BUFSIZ]; char *sh_argv[] = { NULL, NULL }; diff --git a/test/lib/setup b/test/lib/setup index 580825f..ee67152 100755 --- a/test/lib/setup +++ b/test/lib/setup @@ -49,7 +49,7 @@ setup_passt() {

context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"

# pidfile isn't created until passt is listening wait_for [ -f "${STATESETUP}/passt.pid" ] @@ -160,11 +160,11 @@ setup_passt_in_ns() { if [ ${VALGRIND} -eq 1 ]; then context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" else context_run passt "make clean" context_run passt "make" - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" fi wait_for [ -f "${STATESETUP}/passt.pid" ]

@@ -243,7 +243,7 @@ setup_two_guests() { [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"

- context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001" + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001" wait_for [ -f "${STATESETUP}/passt_1.pid" ]

__opts= @@ -252,7 +252,7 @@ setup_two_guests() { [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"

- context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004" + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004" wait_for [ -f "${STATESETUP}/passt_2.pid" ]

__vmem="$((${MEM_KIB} / 1024 / 4))" diff --git a/test/passt.mbuto b/test/passt.mbuto index 138d365..1e07693 100755 --- a/test/passt.mbuto +++ b/test/passt.mbuto @@ -13,7 +13,7 @@ PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}" + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"

# OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and # sshd-session the per-session program. We need the latter as well, and the path @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"' #!/bin/sh LOG=/var/log/dhclient-script.log echo \${reason} \${interface} >> \$LOG +env >> \$LOG set >> \$LOG

[ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu} @@ -54,7 +55,8 @@ set >> \$LOG [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface} [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}" +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn exit 0 EOF chmod 755 /sbin/dhclient-script diff --git a/test/passt/dhcp b/test/passt/dhcp index 9925ab9..145f1ba 100644 --- a/test/passt/dhcp +++ b/test/passt/dhcp @@ -11,7 +11,7 @@ # Copyright (c) 2021 Red Hat GmbH # Author: Stefano Brivio

-gtools ip jq dhclient sed tr +gtools ip jq dhclient sed tr hostname htools ip jq sed tr head

test Interface name @@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH__" = "__HOST_SEARCH__" ]

+test DHCP: Hostname +gout NEW_HOST_NAME cat /tmp/new_host_name +check [ "__NEW_HOST_NAME__" = "hostname1" ] + +test DHCP: Client FQDN +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ] + test DHCPv6: address +guest rm /tmp/new_fqdn_fqdn guest /sbin/dhclient -6 __IFNAME__ # Wait for DAD to complete guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done @@ -70,3 +79,7 @@ test DHCPv6: search list gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH6__" = "__HOST_SEARCH6__" ] + +test DHCPv6: Hostname +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ] diff --git a/util.c b/util.c index 11973c4..ba876ea 100644 --- a/util.c +++ b/util.c @@ -837,3 +837,34 @@ void raw_random(void *buf, size_t buflen) if (random_read < buflen) die("Unexpected EOF on random data source"); } +/** + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1 + * @buf: Buffer to fill in with encoded domain name + * @len Buffer length

@len:

Done.

...

+ * @domain_name: Input domain name + * @domain_name_len Domain name length

@domain_name_len:

Done.

...

+ * + * Return: encoded domain name length or -1 if it do not fit at buffer

"doesn't"

"fit in the buffer" (or simply "doesn't fit")

Done.

...

+ */ +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len)

This now exceeds 80 columns for no compelling reason.

Done.

...

+{ + char *p; + size_t i;

These should go from longest to shortest.

Done.

...

+ + if (domain_name_len + 2 > len) + return -1; + + buf[0] = strcspn(domain_name, "."); + p = buf + 1; + for (i = 0; i < len; i++) { + if (domain_name[i] == '.')

This overflows the *input* buffer, if len > domain_name_len (which is the intended usage).

I understand that if you limit len in the caller (<= PASST_MAXDNAME), then you avoid the overflow, but this looks extremely fragile (the restriction in the caller is buggy anyway), and it would leak host memory into the guest.

I still think that v6 plus my suggestion (ensure adequate buffer size in the caller) would be a bit simpler.

If you really want to add a check on the output buffer size here, I would suggest that you iterate over the *input* bytes anyway, and then at every byte check that you're not exceeding the output buffer.

So your suggestion is at the caller limiting|check buffer size and not pass domain_name len, so function will be: size_t encode_domain_name(char* buf, const char *domain_name) That means that the caller has to know that the encoded len is len (without terminator) + 2, is that fine ? The idea for v7 was encapsulating the size check at the function so we don't have to repeat len + 2 check at all the callers, but maybe is plain overengineering stuff.

...

+ p[i] = strcspn(domain_name + i + 1, "."); + else + p[i] = domain_name[i]; + } + + /* domain name is terminated by a length byte of zero */ + p[len + 1] = 0x00; + + return domain_name_len + 2; +} diff --git a/util.h b/util.h index 3fa1d12..b7d5b91 100644 --- a/util.h +++ b/util.h @@ -40,6 +40,9 @@ #ifndef IP_MAX_MTU #define IP_MAX_MTU USHRT_MAX #endif +#ifndef IPV6_MIN_MTU +#define IPV6_MIN_MTU 1280

This would need tabs instead of spaces (even better: align it with the other values above, so that it becomes some kind of table).

Done.

...

+#endif

#ifndef MIN #define MIN(x, y) (((x) < (y)) ? (x) : (y)) @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, #define accept4(s, addr, addrlen, flags) \ wrap_accept4((s), (addr), (addrlen), (flags))

+#define PASST_MAXDNAME 253 /* RFC 1035 */ +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len);

This exceeds 80 columns and can conveniently be wrapped.

...

+ #endif /* UTIL_H */

-- Stefano

-- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat

On Thu, Jan 9, 2025 at 2:34 PM Stefano Brivio wrote:

On Thu, 9 Jan 2025 13:57:38 +0100 Enrique Llorente Pastora wrote:

...

On Thu, Jan 9, 2025 at 9:16 AM Stefano Brivio wrote:

...

Just coding style comments, plus one functional issue, but it should be relatively contained:

On Wed, 8 Jan 2025 15:58:11 +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 | 54 ++++++++++++++++++++++++++---- dhcpv6.c | 85 +++++++++++++++++++++++++++++++++++++++--------- passt.1 | 10 ++++++ passt.h | 5 +++ pasta.c | 17 +++++++--- test/lib/setup | 10 +++--- test/passt.mbuto | 6 ++-- test/passt/dhcp | 15 ++++++++- util.c | 31 ++++++++++++++++++ util.h | 6 ++++ 11 files changed, 222 insertions(+), 37 deletions(-)

diff --git a/conf.c b/conf.c index df2b016..554e5c3 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))

Why? This was fine in v6. I'm not sure what happened here. In case your editor/email client doesn't show this, now it's:

tab tab tab tab tab tab tab tab tab tab tab tab space "%s", optarg))

that is, twelve tabs and one space. See also:

https://archives.passt.top/passt-dev/20250108145811.833308-1-ellorent@redhat...

It should be:

if (snprintf_check(c->hostname, PASST_MAXDNAME, "%s", optarg))

tab tab tab tab tab space space space "%s", optarg))

that is, five tabs and three spaces.

Fixed, no clue how I de-formated it.

...

...

+ die("Invalid hostname: %s", optarg); + break; case '4': v4_only = true; v6_only = false; diff --git a/dhcp.c b/dhcp.c index d8515aa..50c220d 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*/];

Same as my comment to v6:

"Spaces around /* ... */ wouldn't be bad for readability."

Right, I understood the opposite, fixing.

...

...

} __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: offset for the next option field or -1 if option do not fit

Same as my comment to v6:

"doesn't fit"

It's a bit weird, but harmless, that this function updates 'offset', and also returns it (unless there's an error). Maybe it saves a bit of typing in callers, though (I would have just returned offset or -1 at this point, without passing a pointer).

Make sense to not pass a pointer to offset, I will change it.

...

...

*/ -static void fill_one(struct msg *m, int o, int *offset) +static ssize_t 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 -1; + 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 *offset; }

/** @@ -171,8 +189,11 @@ 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 (opts[o].slen != -1 && !opts[o].sent) { + if (fill_one(m, o, &offset) == -1) {

Curly brackets not needed.

Done.

...

...

+ debug("DHCP: skipping option %i", o); + } + } }

m->o[offset++] = 255; @@ -398,6 +419,25 @@ 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); + } + + opt_len = strlen(c->fqdn); + if (opt_len > 0) { + size_t encoded_len; + encoded_len = encode_domain_name((char *)opts[81].s + 3, sizeof(opts[81].s) - 3, + c->fqdn, opt_len);

Arguments are aligned properly, but it exceeds 80 columns for no compelling reason. It should be:

encoded_len = encode_domain_name((char *)opts[81].s + 3, sizeof(opts[81].s) - 3, c->fqdn, opt_len);

Done.

...

...

+ if (encoded_len > 0 ) {

Excess whitespace between 0 and ).

Done.

...

...

+ opts[81].s[0] = 0x4; /* flags (E) */ + opts[81].s[1] = 0xff; /* RCODE1 */ + opts[81].s[2] = 0xff; /* RCODE2 */ + opts[81].slen = encoded_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..07ce768 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; @@ -58,6 +59,9 @@ struct opt_hdr { sizeof(struct opt_hdr)) #define OPT_VSIZE(x) (sizeof(struct opt_##x) - \ sizeof(struct opt_hdr)) +#define OPT_MAX_SIZE IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \ + sizeof(struct udphdr) + \ + sizeof(struct msg_hdr))

/** * struct opt_client_id - DHCPv6 Client Identifier option @@ -163,6 +167,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 described by RFC 4704 + * @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 +209,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 +220,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 +246,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 +368,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) @@ -373,6 +394,7 @@ search: return offset;

for (i = 0; *c->dns_search[i].n; i++) { + size_t encoded_name_len; size_t name_len = strlen(c->dns_search[i].n);

/* We already append separators, don't duplicate if present */ @@ -388,29 +410,61 @@ 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; + encoded_name_len = encode_domain_name(srch->list, NS_MAXDNAME, + c->dns_search[i].n, + name_len); + srch->hdr.l += encoded_name_len;

...not if encoded_name_len is -1. In that case, you could also have an integer overflow, other than broken functionality.

Right, I will check -1 and skip on that case.

Also I have to move the list buffer or I will be storing it at the first element. encoded_name_len = encode_domain_name(srch->list + srch->hdr.l, NS_MAXDNAME, c->dns_search[i].n, name_len);

Oh, right. I didn't test this. Can't you just follow along where you are with 'offset'? I'm not sure, I didn't look into it.

I cannot use offset since it's initialize with + header offset += sizeof(struct opt_hdr);

...

...

...

+ 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, - "."); - } - } + if (srch) srch->hdr.l = htons(srch->hdr.l); - }

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 pool *p, const struct ctx *c, + char *buf, int offset) + +{ + ssize_t opt_len; + struct opt_client_fqdn *o; + struct opt_client_fqdn const *req_opt;

Same as my comment to v6:

"These should go from longest to shortest."

Done.

...

...

+ + opt_len = MIN(PASST_MAXDNAME, OPT_MAX_SIZE - (offset + sizeof(struct opt_hdr) + 1/*flags*/));

This should be PASST_MAXDNAME + 1, otherwise:

./passt -d -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one

19.4621: DHCPv6: received REQUEST/RENEW/CONFIRM, sending REPLY 19.4621: DHCPv6: client FQDN option does not fit, skipping

Make sense I will change it

...

...but that domain name is 252 characters, and it's accepted (longer domains are not accepted, which makes me think: should PASST_MAXDNAME be 254 instead, as it includes the terminator?).

We are also using that const for validation (and on that case we don't count the terminator) so we cannot modify it

if (snprintf_check(c->fqdn, PASST_MAXDNAME, "%s", optarg)) die("Invalid FQDN: %s", optarg);

Well, no, that was exactly my point. We do count the terminator there:

$ ./passt -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_get_to_253_with_this_one Invalid FQDN: thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_get_to_253_with_this_one

because snprintf_check() is snprintf()-like, see snprintf(3):

The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.

Back to RFC 1035 (3.1):

To simplify implementations, the total length of a domain name (i.e., label octets and label length octets) is restricted to 255 octets or less.

which means 253 characters before encoding, because we have two labels, at start and end. That's why we used 253.

But that actually includes the terminator, so we should probably size everything to 254 bytes (either use PASST_MAXDNAME + 1 or define PASST_MAXDNAME as 254... the first one is more consistent with similar constants, the second one might be more practical, I'm not sure).

Ahh cool, I will go with defining PASST_MAXDNAME as 254 and explain that it includes the terminator at the const description, so we don't have to "+ 1" around and explain every time.

...

...

If it's one byte shorter, then it works (254 bytes option size, 253 bytes of encoded domain, 251 of original domain).

Once this is fixed, this reveals an issue in encode_domain_name(), more on that below.

By the way, this exceeds 80 columns for no particular reason, and "+ 1 /* flags */" is probably a bit more readable.

...

+ + o = (struct opt_client_fqdn *)(buf + offset); + opt_len = encode_domain_name(o->domain_name, opt_len, c->fqdn, strlen(c->fqdn));

This now exceeds 80 columns.

Done.

...

...

+ if (opt_len == -1) { + debug("DHCPv6: client FQDN option does not fit, skipping"); + return offset; + } + + req_opt = (struct opt_client_fqdn *)dhcpv6_opt(p, &(size_t){ 0 }, + OPT_CLIENT_FQDN); + if (req_opt && req_opt->flags & 0x01 /* S flag */) + o->flags = 0x02 /* O flag */; + else + o->flags = 0x00; + + opt_len++; + + o->hdr.t = OPT_CLIENT_FQDN; + o->hdr.l = htons(opt_len);

It's much easier to follow now, I think.

...

+ + return offset + sizeof(struct opt_hdr) + opt_len; +} + /** * dhcpv6() - Check if this is a DHCPv6 message, reply as needed * @c: Execution context @@ -544,6 +598,7 @@ int dhcpv6(struct ctx *c, const struct pool *p, n = offsetof(struct resp_t, client_id) + sizeof(struct opt_hdr) + ntohs(client_id->l); n = dhcpv6_dns_fill(c, (char *)&resp, n); + n = dhcpv6_client_fqdn_fill(p, c, (char *)&resp, n);

resp.hdr.xid = mh->xid;

diff --git a/passt.1 b/passt.1 index d9cd33e..7051fc4 100644 --- a/passt.1 +++ b/passt.1 @@ -401,6 +401,16 @@ Enable IPv6-only operation. IPv4 traffic will be ignored. By default, IPv4 operation is enabled as long as at least an IPv4 route and an interface address are configured on a given host interface.

+.TP +.BR \-H ", " \-\-hostname " " \fIname +Hostname to configure the client with. +Send \fIname\fR as DHCP option 12 (hostname). + +.TP +.BR \-\-fqdn " " \fIname +FQDN to configure the client with. +Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39. + .SS \fBpasst\fR-only options

.TP diff --git a/passt.h b/passt.h index 0dd4efa..f3151f0 100644 --- a/passt.h +++ b/passt.h @@ -209,6 +209,8 @@ struct ip6_ctx { * @ifi4: Template interface for IPv4, -1: none, 0: IPv4 disabled * @ip: IPv4 configuration * @dns_search: DNS search list + * @hostname: Guest hostname + * @fqdn: Guest FQDN * @ifi6: Template interface for IPv6, -1: none, 0: IPv6 disabled * @ip6: IPv6 configuration * @pasta_ifn: Name of namespace interface for pasta @@ -269,6 +271,9 @@ struct ctx {

struct fqdn dns_search[MAXDNSRCH];

+ char hostname[PASST_MAXDNAME]; + char fqdn[PASST_MAXDNAME]; + int ifi6; struct ip6_ctx ip6;

diff --git a/pasta.c b/pasta.c index ff41c95..f3b4cae 100644 --- a/pasta.c +++ b/pasta.c @@ -169,10 +169,12 @@ void pasta_open_ns(struct ctx *c, const char *netns) * struct pasta_spawn_cmd_arg - Argument for pasta_spawn_cmd() * @exe: Executable to run * @argv: Command and arguments to run + * @ctx: Context to read config from */ struct pasta_spawn_cmd_arg { const char *exe; char *const *argv; + struct ctx *c; };

/** @@ -186,6 +188,7 @@ static int pasta_spawn_cmd(void *arg) { char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX; const struct pasta_spawn_cmd_arg *a; + size_t conf_hostname_len; sigset_t set;

/* We run in a detached PID and mount namespace: mount /proc over */ @@ -195,9 +198,15 @@ static int pasta_spawn_cmd(void *arg) if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) warn("Cannot set ping_group_range, ICMP requests might fail");

- if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, - HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || - errno == ENAMETOOLONG) { + a = (const struct pasta_spawn_cmd_arg *)arg; + + conf_hostname_len = strlen(a->c->hostname); + if (conf_hostname_len > 0) { + if (sethostname(a->c->hostname, conf_hostname_len)) + warn("Unable to set configured hostname"); + } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, + HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) || + errno == ENAMETOOLONG) {

Same as my comment to v6, which was the same as my comment to v5...:

"Same as for v5: this is an argument to gethostname(), so it should be aligned accordingly. ! is a unary operator, it doesn't take two operands.

That is:

} else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||

see also the version without your patch."

...

hostname[HOST_NAME_MAX] = '\0'; if (sethostname(hostname, strlen(hostname))) warn("Unable to set pasta-prefixed hostname");

Done.

...

...

@@ -208,7 +217,6 @@ static int pasta_spawn_cmd(void *arg) sigaddset(&set, SIGUSR1); sigwaitinfo(&set, NULL);

- a = (const struct pasta_spawn_cmd_arg *)arg; execvp(a->exe, a->argv);

die_perror("Failed to start command or shell"); @@ -230,6 +238,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, struct pasta_spawn_cmd_arg arg = { .exe = argv[0], .argv = argv, + .c = c, }; char uidmap[BUFSIZ], gidmap[BUFSIZ]; char *sh_argv[] = { NULL, NULL }; diff --git a/test/lib/setup b/test/lib/setup index 580825f..ee67152 100755 --- a/test/lib/setup +++ b/test/lib/setup @@ -49,7 +49,7 @@ setup_passt() {

context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"

# pidfile isn't created until passt is listening wait_for [ -f "${STATESETUP}/passt.pid" ] @@ -160,11 +160,11 @@ setup_passt_in_ns() { if [ ${VALGRIND} -eq 1 ]; then context_run passt "make clean" context_run passt "make valgrind" - context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" else context_run passt "make clean" context_run passt "make" - context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}" fi wait_for [ -f "${STATESETUP}/passt.pid" ]

@@ -243,7 +243,7 @@ setup_two_guests() { [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"

- context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001" + context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001" wait_for [ -f "${STATESETUP}/passt_1.pid" ]

__opts= @@ -252,7 +252,7 @@ setup_two_guests() { [ ${TRACE} -eq 1 ] && __opts="${__opts} --trace" [ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"

- context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004" + context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004" wait_for [ -f "${STATESETUP}/passt_2.pid" ]

__vmem="$((${MEM_KIB} / 1024 / 4))" diff --git a/test/passt.mbuto b/test/passt.mbuto index 138d365..1e07693 100755 --- a/test/passt.mbuto +++ b/test/passt.mbuto @@ -13,7 +13,7 @@ PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl - nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}" + nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"

# OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and # sshd-session the per-session program. We need the latter as well, and the path @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"' #!/bin/sh LOG=/var/log/dhclient-script.log echo \${reason} \${interface} >> \$LOG +env >> \$LOG set >> \$LOG

[ -n "\${new_interface_mtu}" ] && ip link set dev \${interface} mtu \${new_interface_mtu} @@ -54,7 +55,8 @@ set >> \$LOG [ -n "\${new_ip6_address}" ] && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface} [ -n "\${new_dhcp6_name_servers}" ] && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf -[ -n "\${new_host_name}" ] && hostname "\${new_host_name}" +[ -n "\${new_host_name}" ] && echo "\${new_host_name}" > /tmp/new_host_name +[ -n "\${new_fqdn_fqdn}" ] && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn exit 0 EOF chmod 755 /sbin/dhclient-script diff --git a/test/passt/dhcp b/test/passt/dhcp index 9925ab9..145f1ba 100644 --- a/test/passt/dhcp +++ b/test/passt/dhcp @@ -11,7 +11,7 @@ # Copyright (c) 2021 Red Hat GmbH # Author: Stefano Brivio

-gtools ip jq dhclient sed tr +gtools ip jq dhclient sed tr hostname htools ip jq sed tr head

test Interface name @@ -47,7 +47,16 @@ gout SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc hout HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH__" = "__HOST_SEARCH__" ]

+test DHCP: Hostname +gout NEW_HOST_NAME cat /tmp/new_host_name +check [ "__NEW_HOST_NAME__" = "hostname1" ] + +test DHCP: Client FQDN +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ] + test DHCPv6: address +guest rm /tmp/new_fqdn_fqdn guest /sbin/dhclient -6 __IFNAME__ # Wait for DAD to complete guest while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done @@ -70,3 +79,7 @@ test DHCPv6: search list gout SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' hout HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/' check [ "__SEARCH6__" = "__HOST_SEARCH6__" ] + +test DHCPv6: Hostname +gout NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn +check [ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ] diff --git a/util.c b/util.c index 11973c4..ba876ea 100644 --- a/util.c +++ b/util.c @@ -837,3 +837,34 @@ void raw_random(void *buf, size_t buflen) if (random_read < buflen) die("Unexpected EOF on random data source"); } +/** + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1 + * @buf: Buffer to fill in with encoded domain name + * @len Buffer length

@len:

Done.

...

...

+ * @domain_name: Input domain name + * @domain_name_len Domain name length

@domain_name_len:

Done.

...

...

+ * + * Return: encoded domain name length or -1 if it do not fit at buffer

"doesn't"

"fit in the buffer" (or simply "doesn't fit")

Done.

...

...

+ */ +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len)

This now exceeds 80 columns for no compelling reason.

Done.

...

...

+{ + char *p; + size_t i;

These should go from longest to shortest.

Done.

...

...

+ + if (domain_name_len + 2 > len) + return -1; + + buf[0] = strcspn(domain_name, "."); + p = buf + 1; + for (i = 0; i < len; i++) { + if (domain_name[i] == '.')

This overflows the *input* buffer, if len > domain_name_len (which is the intended usage).

I understand that if you limit len in the caller (<= PASST_MAXDNAME), then you avoid the overflow, but this looks extremely fragile (the restriction in the caller is buggy anyway), and it would leak host memory into the guest.

I still think that v6 plus my suggestion (ensure adequate buffer size in the caller) would be a bit simpler.

If you really want to add a check on the output buffer size here, I would suggest that you iterate over the *input* bytes anyway, and then at every byte check that you're not exceeding the output buffer.

So your suggestion is at the caller limiting|check buffer size and not pass domain_name len, so function will be: size_t encode_domain_name(char* buf, const char *domain_name)

Yes, just like you had.

...

That means that the caller has to know that the encoded len is len (without terminator) + 2, is that fine ? The idea for v7 was encapsulating the size check at the function so we don't have to repeat len + 2 check at all the callers, but maybe is plain overengineering stuff.

Well, the check is so simple that it's probably the same lines of code as checking if the return code is -1.

Sure, it's nice in general to avoid duplication, but at the moment you have three bugs, which I don't think you introduced on purpose:

- dhcp() uses encoded_len just as it is, even if it's -1 (I'm not sure if I reported this)

- same for dhcpv6_dns_fill()

- encode_domain_name() itself might overrun the *input*

so perhaps it's actually less bug prone to have a simple check in the callers?

Again, that's not the only way, you can still check the buffer size here, but it should be used as *check*, not to iterate on the input.

Maybe implement both (it's a few lines) and see what looks simpler or more robust?

Let's go with doing the check at the caller, and if in the future we see a lot of duplication there we can refactor and do the check inside at the callee.

...

...

...

+ p[i] = strcspn(domain_name + i + 1, "."); + else + p[i] = domain_name[i]; + } + + /* domain name is terminated by a length byte of zero */ + p[len + 1] = 0x00; + + return domain_name_len + 2; +} diff --git a/util.h b/util.h index 3fa1d12..b7d5b91 100644 --- a/util.h +++ b/util.h @@ -40,6 +40,9 @@ #ifndef IP_MAX_MTU #define IP_MAX_MTU USHRT_MAX #endif +#ifndef IPV6_MIN_MTU +#define IPV6_MIN_MTU 1280

This would need tabs instead of spaces (even better: align it with the other values above, so that it becomes some kind of table).

Done.

...

...

+#endif

#ifndef MIN #define MIN(x, y) (((x) < (y)) ? (x) : (y)) @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr, #define accept4(s, addr, addrlen, flags) \ wrap_accept4((s), (addr), (addrlen), (flags))

+#define PASST_MAXDNAME 253 /* RFC 1035 */ +ssize_t encode_domain_name(char *buf, size_t len, const char *domain_name, size_t domain_name_len);

This exceeds 80 columns and can conveniently be wrapped.

...

+ #endif /* UTIL_H */

-- Stefano

-- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat