* [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops
@ 2025-01-10 10:26 Enrique Llorente
2025-01-10 23:52 ` Stefano Brivio
0 siblings, 1 reply; 4+ messages in thread
From: Enrique Llorente @ 2025-01-10 10:26 UTC (permalink / raw)
To: passt-dev; +Cc: Enrique Llorente
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 | 63 ++++++++++++++++++++++++++------
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 | 24 +++++++++++++
util.h | 6 ++++
11 files changed, 229 insertions(+), 40 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..8491075 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: offset for the next option field or -1 if option doesn't fit
*/
-static void fill_one(struct msg *m, int o, int *offset)
+static ssize_t fill_one(struct msg *m, int o, int offset)
{
- m->o[*offset] = o;
- m->o[*offset + 1] = opts[o].slen;
- memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
+ 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] = 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;
}
/**
@@ -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);
+ offset = fill_one(m, 53, offset);
for (i = 0; i < opts[55].clen; i++) {
o = opts[55].c[i];
if (opts[o].slen != -1)
- fill_one(m, o, &offset);
+ offset = fill_one(m, o, offset);
}
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) {
+ offset = fill_one(m, o, offset);
+ if (offset == -1)
+ debug("DHCP: skipping option %i", o);
+ }
}
m->o[offset++] = 255;
@@ -398,6 +419,28 @@ 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) {
+ opt_len += 3/* flags */ + 2/* extra for encoded fqdn */;
+
+ if (sizeof(opts[81].s) < opt_len)
+ debug("DHCP: client FQDN option do not fit, skipping");
+
+ opts[81].s[0] = 0x4; /* flags (E) */
+ opts[81].s[1] = 0xff; /* RCODE1 */
+ opts[81].s[2] = 0xff; /* RCODE2 */
+
+ encode_domain_name((char *)opts[81].s + 3, c->fqdn);
+
+ opts[81].slen = opt_len;
+ }
+
if (!c->no_dhcp_dns_search)
opt_set_dns_search(c, sizeof(m->o));
diff --git a/dhcpv6.c b/dhcpv6.c
index 0523bba..29e8b02 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)
@@ -383,34 +404,75 @@ search:
if (!name_len)
continue;
+ name_len += 2/* encoded domain name extra bytes */;
+ if (name_len > NS_MAXDNAME) {
+ debug("DHCP: DNS search name '%s' too big, skipping",
+ c->dns_search[i].n);
+ continue;
+ }
+
if (!srch) {
srch = (struct opt_dns_search *)(buf + offset);
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;
+ encode_domain_name(buf + offset, c->dns_search[i].n);
+
+ srch->hdr.l += name_len;
+ offset += 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)
+
+{
+ struct opt_client_fqdn const *req_opt;
+ struct opt_client_fqdn *o;
+ size_t opt_len;
+
+ opt_len = strlen(c->fqdn) + 2/* encoded domain name extra bytes */;
+ if (opt_len > MIN(PASST_MAXDNAME,
+ OPT_MAX_SIZE - (offset +
+ sizeof(struct opt_hdr) +
+ 1/* flags */))){
+ debug("DHCPv6: client FQDN option doesn't fit, skipping");
+ return offset;
+ }
+
+ o = (struct opt_client_fqdn *)(buf + offset);
+ encode_domain_name(o->domain_name, c->fqdn);
+ 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);
+
+ return offset + sizeof(struct opt_hdr) + opt_len;
+}
+
/**
* dhcpv6() - Check if this is a DHCPv6 message, reply as needed
* @c: Execution context
@@ -544,6 +606,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..922aa10 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) {
hostname[HOST_NAME_MAX] = '\0';
if (sethostname(hostname, strlen(hostname)))
warn("Unable to set pasta-prefixed hostname");
@@ -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 <sbrivio@redhat.com>
-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..40d95bf 100644
--- a/util.c
+++ b/util.c
@@ -837,3 +837,27 @@ 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
+ * @domain_name: Input domain name string with terminator
+ *
+ * The buffer's 'buf' size has to be >= strlen(domain_name) + 2
+ */
+void encode_domain_name(char *buf, const char *domain_name)
+{
+ size_t i;
+ char *p;
+
+ buf[0] = strcspn(domain_name, ".");
+ p = buf + 1;
+ for (i = 0; ; i++) {
+ if (domain_name[i] == '.')
+ p[i] = strcspn(domain_name + i + 1, ".");
+ else {
+ p[i] = domain_name[i];
+ if (p[i] == 0L)
+ break;
+ }
+ }
+}
diff --git a/util.h b/util.h
index 3fa1d12..0744276 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
+#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 254 /* 253 (RFC 1035) + 1 (the terminator) */
+void encode_domain_name(char *buf, const char *domain_name);
+
#endif /* 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
+#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 254 /* 253 (RFC 1035) + 1 (the terminator) */
+void encode_domain_name(char *buf, const char *domain_name);
+
#endif /* UTIL_H */
--
2.47.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops
2025-01-10 10:26 [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
@ 2025-01-10 23:52 ` Stefano Brivio
2025-01-14 14:01 ` Enrique Llorente Pastora
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Brivio @ 2025-01-10 23:52 UTC (permalink / raw)
To: Enrique Llorente; +Cc: passt-dev
On Fri, 10 Jan 2025 11:26:26 +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 | 63 ++++++++++++++++++++++++++------
> 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 | 24 +++++++++++++
> util.h | 6 ++++
> 11 files changed, 229 insertions(+), 40 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..8491075 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, same as my comment to v7:
"Spaces around /* ... */ wouldn't be bad for readability."
> } __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 doesn't fit
> */
> -static void fill_one(struct msg *m, int o, int *offset)
> +static ssize_t fill_one(struct msg *m, int o, int offset)
> {
> - m->o[*offset] = o;
> - m->o[*offset + 1] = opts[o].slen;
> - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> + 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] = 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;
> }
>
> /**
> @@ -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);
> + offset = fill_one(m, 53, offset);
Now suppose that somebody adds a "long" option before this block, or...
>
> for (i = 0; i < opts[55].clen; i++) {
> o = opts[55].c[i];
> if (opts[o].slen != -1)
> - fill_one(m, o, &offset);
> + offset = fill_one(m, o, offset);
> }
before this one, and fill_one() returns -1. Then...
> 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) {
> + offset = fill_one(m, o, offset);
you use offset as -1 here, and boom. If not, see directly below).
> + if (offset == -1)
Or maybe the domain name is actually too long, and offset is -1 here...
> + debug("DHCP: skipping option %i", o);
> + }
> }
>
> m->o[offset++] = 255;
...and boom. This sets the last byte of "magic" (depending on the
architecture) to 0xff, making the whole message invalid.
Two solutions:
- the obvious one, which adds a couple of lines but it doesn't look
that bad: check if fill_one() returns -1 before setting offset
- the short but maybe hacky one (I would personally go with this, but
it's a matter of taste): make fill_one() return bool, true means
failure, and keep passing offset as a pointer, which is *not* set on
failure. Then you get, simply:
if (fill_one(m, o, &offset))
debug("DHCP: skipping option %i", o);
> @@ -398,6 +419,28 @@ 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) {
> + opt_len += 3/* flags */ + 2/* extra for encoded fqdn */;
Same as several comments on several revisions:
3 /* flags */ + 2 /* ... */
It's FQDN, not fqdn.
> +
> + if (sizeof(opts[81].s) < opt_len)
> + debug("DHCP: client FQDN option do not fit, skipping");
Same as several comments on several revisions: "doesn't"
> +
> + opts[81].s[0] = 0x4; /* flags (E) */
> + opts[81].s[1] = 0xff; /* RCODE1 */
> + opts[81].s[2] = 0xff; /* RCODE2 */
> +
> + encode_domain_name((char *)opts[81].s + 3, c->fqdn);
> +
> + opts[81].slen = opt_len;
> + }
> +
> if (!c->no_dhcp_dns_search)
> opt_set_dns_search(c, sizeof(m->o));
>
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 0523bba..29e8b02 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)
> @@ -383,34 +404,75 @@ search:
> if (!name_len)
> continue;
>
> + name_len += 2/* encoded domain name extra bytes */;
Same as several comments on several revisions: leave spaces between
comments and code. There's no need to add this before the ;, it could
simply be:
name_len += 2; /* Length byte for first label, and terminator */
> + if (name_len > NS_MAXDNAME) {
With this, if I do:
$ ./passt -d -f -S thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one,a.b -p search.pcap
I get (try and see capture):
- a single domain name that's 258 characters long (terminator is
missing)
- a 3-byte FQDN option (there should be none)
> + debug("DHCP: DNS search name '%s' too big, skipping",
I'd say it's too long, rather than big.
> + c->dns_search[i].n);
> + continue;
> + }
> +
> if (!srch) {
> srch = (struct opt_dns_search *)(buf + offset);
> 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;
> + encode_domain_name(buf + offset, c->dns_search[i].n);
> +
> + srch->hdr.l += name_len;
> + offset += 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)
> +
> +{
> + struct opt_client_fqdn const *req_opt;
> + struct opt_client_fqdn *o;
> + size_t opt_len;
> +
> + opt_len = strlen(c->fqdn) + 2/* encoded domain name extra bytes */;
Same as above.
> + if (opt_len > MIN(PASST_MAXDNAME,
This doesn't quite work:
$ ./passt -d -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one -p fqdn.pcap
[...]
9.0202: DHCP: client FQDN option do not fit, skipping
and the check for the domain search list would have the same problem:
now you add 2 to opt_len (length of first label, and terminator), but
it's only the terminator accounted for in PASST_MAXDNAME (which is
correct: that represents the storage size).
However, you can actually send 255 bytes (including those two extra
bytes).
That is, the failure should be on strlen(c->fqdn) + 1 > PASST_MAXDNAME,
or, equivalently, opt_len > PASST_MAXDNAME + 1.
By the way, don't you already check that in conf()? I guess the check
here could simply be on the remaining space in the options area? I'm
not sure, I didn't really think this through.
> + OPT_MAX_SIZE - (offset +
> + sizeof(struct opt_hdr) +
> + 1/* flags */))){
Same as several comments on several revisions: leave spaces around
comments. Missing whitespace between ) and {.
> + debug("DHCPv6: client FQDN option doesn't fit, skipping");
> + return offset;
> + }
> +
> + o = (struct opt_client_fqdn *)(buf + offset);
> + encode_domain_name(o->domain_name, c->fqdn);
> + 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);
> +
> + return offset + sizeof(struct opt_hdr) + opt_len;
> +}
> +
> /**
> * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> * @c: Execution context
> @@ -544,6 +606,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..922aa10 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) {
> hostname[HOST_NAME_MAX] = '\0';
> if (sethostname(hostname, strlen(hostname)))
> warn("Unable to set pasta-prefixed hostname");
> @@ -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 <sbrivio@redhat.com>
>
> -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..40d95bf 100644
> --- a/util.c
> +++ b/util.c
> @@ -837,3 +837,27 @@ 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
> + * @domain_name: Input domain name string with terminator
> + *
> + * The buffer's 'buf' size has to be >= strlen(domain_name) + 2
> + */
> +void encode_domain_name(char *buf, const char *domain_name)
> +{
> + size_t i;
> + char *p;
> +
> + buf[0] = strcspn(domain_name, ".");
> + p = buf + 1;
> + for (i = 0; ; i++) {
That's equivalent to having domain_name[i] as loop condition I suppose,
which would look a bit clearer.
> + if (domain_name[i] == '.')
I don't think you'll actually need the extra condition in the else
block below, but for consistency (same as kernel coding style), if one
conditional block has curly brackets, we'll use them for all the others
too, even if they are a single line.
> + p[i] = strcspn(domain_name + i + 1, ".");
> + else {
> + p[i] = domain_name[i];
> + if (p[i] == 0L)
> + break;
> + }
> + }
> +}
> diff --git a/util.h b/util.h
> index 3fa1d12..0744276 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
> +#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 254 /* 253 (RFC 1035) + 1 (the terminator) */
> +void encode_domain_name(char *buf, const char *domain_name);
> +
> #endif /* UTIL_H */
The rest looks good to me, but I didn't try really hard to break it.
--
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops
2025-01-10 23:52 ` Stefano Brivio
@ 2025-01-14 14:01 ` Enrique Llorente Pastora
2025-01-14 19:32 ` Stefano Brivio
0 siblings, 1 reply; 4+ messages in thread
From: Enrique Llorente Pastora @ 2025-01-14 14:01 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
On Sat, Jan 11, 2025 at 12:53 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Fri, 10 Jan 2025 11:26:26 +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 | 63 ++++++++++++++++++++++++++------
> > 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 | 24 +++++++++++++
> > util.h | 6 ++++
> > 11 files changed, 229 insertions(+), 40 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..8491075 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, same as my comment to v7:
>
> "Spaces around /* ... */ wouldn't be bad for readability."
>
> > } __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 doesn't fit
> > */
> > -static void fill_one(struct msg *m, int o, int *offset)
> > +static ssize_t fill_one(struct msg *m, int o, int offset)
> > {
> > - m->o[*offset] = o;
> > - m->o[*offset + 1] = opts[o].slen;
> > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > + 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] = 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;
> > }
> >
> > /**
> > @@ -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);
> > + offset = fill_one(m, 53, offset);
>
> Now suppose that somebody adds a "long" option before this block, or...
>
> >
> > for (i = 0; i < opts[55].clen; i++) {
> > o = opts[55].c[i];
> > if (opts[o].slen != -1)
> > - fill_one(m, o, &offset);
> > + offset = fill_one(m, o, offset);
> > }
>
> before this one, and fill_one() returns -1. Then...
>
> > 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) {
> > + offset = fill_one(m, o, offset);
>
> you use offset as -1 here, and boom. If not, see directly below).
>
> > + if (offset == -1)
>
> Or maybe the domain name is actually too long, and offset is -1 here...
>
> > + debug("DHCP: skipping option %i", o);
> > + }
> > }
> >
> > m->o[offset++] = 255;
>
> ...and boom. This sets the last byte of "magic" (depending on the
> architecture) to 0xff, making the whole message invalid.
>
well, I was expecting the only problematic options being at > 55
(didn't check the standard)
but I agree that we have to be consistent on handling the fill_in result.
> Two solutions:
>
> - the obvious one, which adds a couple of lines but it doesn't look
> that bad: check if fill_one() returns -1 before setting offset
>
> - the short but maybe hacky one (I would personally go with this, but
> it's a matter of taste): make fill_one() return bool, true means
> failure, and keep passing offset as a pointer, which is *not* set on
> failure. Then you get, simply:
>
> if (fill_one(m, o, &offset))
> debug("DHCP: skipping option %i", o);
>
I will go with the second one since it's more passt idiomatic and will
add the check at all the
three blocks.
> > @@ -398,6 +419,28 @@ 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) {
> > + opt_len += 3/* flags */ + 2/* extra for encoded fqdn */;
>
> Same as several comments on several revisions:
>
> 3 /* flags */ + 2 /* ... */
>
> It's FQDN, not fqdn.
>
> > +
> > + if (sizeof(opts[81].s) < opt_len)
> > + debug("DHCP: client FQDN option do not fit, skipping");
>
> Same as several comments on several revisions: "doesn't"
>
> > +
> > + opts[81].s[0] = 0x4; /* flags (E) */
> > + opts[81].s[1] = 0xff; /* RCODE1 */
> > + opts[81].s[2] = 0xff; /* RCODE2 */
> > +
> > + encode_domain_name((char *)opts[81].s + 3, c->fqdn);
> > +
> > + opts[81].slen = opt_len;
> > + }
> > +
> > if (!c->no_dhcp_dns_search)
> > opt_set_dns_search(c, sizeof(m->o));
> >
> > diff --git a/dhcpv6.c b/dhcpv6.c
> > index 0523bba..29e8b02 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)
> > @@ -383,34 +404,75 @@ search:
> > if (!name_len)
> > continue;
> >
> > + name_len += 2/* encoded domain name extra bytes */;
>
> Same as several comments on several revisions: leave spaces between
> comments and code. There's no need to add this before the ;, it could
> simply be:
>
> name_len += 2; /* Length byte for first label, and terminator */
>
> > + if (name_len > NS_MAXDNAME) {
>
> With this, if I do:
>
> $ ./passt -d -f -S thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one,a.b -p search.pcap
>
> I get (try and see capture):
>
> - a single domain name that's 258 characters long (terminator is
> missing)
>
> - a 3-byte FQDN option (there should be none)
>
Right, we should add the first byte to the check
if (name_len > NS_MAXDNAME + 1) {
> > + debug("DHCP: DNS search name '%s' too big, skipping",
>
> I'd say it's too long, rather than big.
>
> > + c->dns_search[i].n);
> > + continue;
> > + }
> > +
> > if (!srch) {
> > srch = (struct opt_dns_search *)(buf + offset);
> > 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;
> > + encode_domain_name(buf + offset, c->dns_search[i].n);
> > +
> > + srch->hdr.l += name_len;
> > + offset += 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)
> > +
> > +{
> > + struct opt_client_fqdn const *req_opt;
> > + struct opt_client_fqdn *o;
> > + size_t opt_len;
> > +
> > + opt_len = strlen(c->fqdn) + 2/* encoded domain name extra bytes */;
>
> Same as above.
>
> > + if (opt_len > MIN(PASST_MAXDNAME,
>
> This doesn't quite work:
>
> $ ./passt -d -f --fqdn thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one -p fqdn.pcap
>
> [...]
>
> 9.0202: DHCP: client FQDN option do not fit, skipping
>
> and the check for the domain search list would have the same problem:
> now you add 2 to opt_len (length of first label, and terminator), but
> it's only the terminator accounted for in PASST_MAXDNAME (which is
> correct: that represents the storage size).
>
> However, you can actually send 255 bytes (including those two extra
> bytes).
>
> That is, the failure should be on strlen(c->fqdn) + 1 > PASST_MAXDNAME,
> or, equivalently, opt_len > PASST_MAXDNAME + 1.
>
> By the way, don't you already check that in conf()? I guess the check
> here could simply be on the remaining space in the options area? I'm
> not sure, I didn't really think this through.
>
Right, --fqdn config value is already checked at starting time, no
need for redundant checks,
we just need to check the limits with the packet max size:
opt_len = strlen(c->fqdn) + 2; /* Length byte for first label,
and terminator */
if (opt_len > OPT_MAX_SIZE - (offset +
sizeof(struct opt_hdr) +
1 /* flags */ )) {
debug("DHCPv6: client FQDN option doesn't fit, skipping");
return offset;
}
Also fix the search domain check to take into account the first label
length byte.
name_len += 2; /* Length byte for first label, and terminator */
if (name_len > NS_MAXDNAME +
1 /* Length byte for first label */) {
debug("DHCP: DNS search name '%s' too long, skipping",
c->dns_search[i].n);
continue;
}
> > + OPT_MAX_SIZE - (offset +
> > + sizeof(struct opt_hdr) +
> > + 1/* flags */))){
>
> Same as several comments on several revisions: leave spaces around
> comments. Missing whitespace between ) and {.
>
> > + debug("DHCPv6: client FQDN option doesn't fit, skipping");
> > + return offset;
> > + }
> > +
> > + o = (struct opt_client_fqdn *)(buf + offset);
> > + encode_domain_name(o->domain_name, c->fqdn);
> > + 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);
> > +
> > + return offset + sizeof(struct opt_hdr) + opt_len;
> > +}
> > +
> > /**
> > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> > * @c: Execution context
> > @@ -544,6 +606,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..922aa10 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) {
> > hostname[HOST_NAME_MAX] = '\0';
> > if (sethostname(hostname, strlen(hostname)))
> > warn("Unable to set pasta-prefixed hostname");
> > @@ -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 <sbrivio@redhat.com>
> >
> > -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..40d95bf 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -837,3 +837,27 @@ 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
> > + * @domain_name: Input domain name string with terminator
> > + *
> > + * The buffer's 'buf' size has to be >= strlen(domain_name) + 2
> > + */
> > +void encode_domain_name(char *buf, const char *domain_name)
> > +{
> > + size_t i;
> > + char *p;
> > +
> > + buf[0] = strcspn(domain_name, ".");
> > + p = buf + 1;
> > + for (i = 0; ; i++) {
>
> That's equivalent to having domain_name[i] as loop condition I suppose,
> which would look a bit clearer.
>
> > + if (domain_name[i] == '.')
>
The only difference is that if I do:
for (i = 0; domain_name[9]; i++)
then we will have to add the terminator after the loop like we were
doing before, that's not that bad.
But the end result is easier to follow than what I have now:
for (i = 0; domain_name[i]; i++) {
if (domain_name[i] == '.')
p[i] = strcspn(domain_name + i + 1, ".");
else
p[i] = domain_name[i];
}
p[i] = 0L;
> I don't think you'll actually need the extra condition in the else
> block below, but for consistency (same as kernel coding style), if one
> conditional block has curly brackets, we'll use them for all the others
> too, even if they are a single line.
>
I am going to check doimain_name as the for loop so no need for that.
> > + p[i] = strcspn(domain_name + i + 1, ".");
> > + else {
> > + p[i] = domain_name[i];
> > + if (p[i] == 0L)
> > + break;
> > + }
> > + }
> > +}
> > diff --git a/util.h b/util.h
> > index 3fa1d12..0744276 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
> > +#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 254 /* 253 (RFC 1035) + 1 (the terminator) */
> > +void encode_domain_name(char *buf, const char *domain_name);
> > +
> > #endif /* UTIL_H */
>
> The rest looks good to me, but I didn't try really hard to break it.
>
> --
> Stefano
>
--
Quique Llorente
CNV networking Senior Software Engineer
Red Hat EMEA
ellorent@redhat.com
@RedHat Red Hat Red Hat
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops
2025-01-14 14:01 ` Enrique Llorente Pastora
@ 2025-01-14 19:32 ` Stefano Brivio
0 siblings, 0 replies; 4+ messages in thread
From: Stefano Brivio @ 2025-01-14 19:32 UTC (permalink / raw)
To: Enrique Llorente Pastora; +Cc: passt-dev
On Tue, 14 Jan 2025 15:01:33 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> On Sat, Jan 11, 2025 at 12:53 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 10 Jan 2025 11:26:26 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >
> > > @@ -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);
> > > + offset = fill_one(m, 53, offset);
> >
> > Now suppose that somebody adds a "long" option before this block, or...
> >
> > >
> > > for (i = 0; i < opts[55].clen; i++) {
> > > o = opts[55].c[i];
> > > if (opts[o].slen != -1)
> > > - fill_one(m, o, &offset);
> > > + offset = fill_one(m, o, offset);
> > > }
> >
> > before this one, and fill_one() returns -1. Then...
> >
> > > 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) {
> > > + offset = fill_one(m, o, offset);
> >
> > you use offset as -1 here, and boom. If not, see directly below).
> >
> > > + if (offset == -1)
> >
> > Or maybe the domain name is actually too long, and offset is -1 here...
> >
> > > + debug("DHCP: skipping option %i", o);
> > > + }
> > > }
> > >
> > > m->o[offset++] = 255;
> >
> > ...and boom. This sets the last byte of "magic" (depending on the
> > architecture) to 0xff, making the whole message invalid.
>
> well, I was expecting the only problematic options being at > 55
> (didn't check the standard)
> but I agree that we have to be consistent on handling the fill_in result.
Sure, that's the (reasonable) expectation, but note that you would have
the same problem even with a domain name that's too long for fill_one().
--
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-01-14 19:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-10 10:26 [PATCH v8] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
2025-01-10 23:52 ` Stefano Brivio
2025-01-14 14:01 ` Enrique Llorente Pastora
2025-01-14 19:32 ` Stefano Brivio
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).