* [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
@ 2024-12-17 6:50 Enrique Llorente
2024-12-17 23:34 ` Stefano Brivio
0 siblings, 1 reply; 6+ messages in thread
From: Enrique Llorente @ 2024-12-17 6:50 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 | 23 ++++++++++++++--
dhcp.c | 39 +++++++++++++++++++++++---
dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++---------
passt.1 | 11 ++++++++
passt.h | 6 ++++
pasta.c | 15 ++++++++--
test/lib/setup | 10 +++----
test/passt.mbuto | 6 ++--
test/passt/dhcp | 15 +++++++++-
util.c | 22 +++++++++++++++
util.h | 3 ++
11 files changed, 191 insertions(+), 31 deletions(-)
diff --git a/conf.c b/conf.c
index df2b016..29e4bed 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,12 @@ 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 +1737,12 @@ 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;
@@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
c->one_off = true;
break;
+
case 't':
case 'u':
case 'T':
diff --git a/dhcp.c b/dhcp.c
index d8515aa..a04462b 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -62,6 +62,7 @@ static struct opt opts[255];
#define DHCPFORCERENEW 9
#define OPT_MIN 60 /* RFC 951 */
+#define OPT_MAX 302 /* UDP packet limit - dhcp headers */
/**
* dhcp_init() - Initialise DHCP options
@@ -122,7 +123,7 @@ struct msg {
uint8_t sname[64];
uint8_t file[128];
uint32_t magic;
- uint8_t o[308];
+ uint8_t o[OPT_MAX+2];
} __attribute__((__packed__));
/**
@@ -131,14 +132,28 @@ struct msg {
* @o: Option number
* @offset: Current offset within options field, updated on insertion
*/
-static void fill_one(struct msg *m, int o, int *offset)
+static int fill_one(struct msg *m, int o, int *offset)
{
+ size_t idx, slen = 0;
+
+ // We cannot write even enum + len + one byte then just skip
+ if (*offset + 2 > OPT_MAX) {
+ return OPT_MAX;
+ }
m->o[*offset] = o;
m->o[*offset + 1] = opts[o].slen;
- memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
+ idx=*offset + 2;
+ slen=opts[o].slen;
+
+ // Truncate if it goes beyond OPT_MAX
+ if (idx + slen > OPT_MAX) {
+ slen = OPT_MAX - idx;
+ }
+ memcpy(&m->o[*offset + 2], opts[o].s, slen);
opts[o].sent = 1;
*offset += 2 + opts[o].slen;
+ return *offset;
}
/**
@@ -172,7 +187,9 @@ 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 (fill_one(m, o, &offset) == OPT_MAX) {
+ break;
+ }
}
m->o[offset++] = 255;
@@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
if (!opts[6].slen)
opts[6].slen = -1;
+ size_t hostname_len = strlen(c->hostname);
+ if (hostname_len > 0) {
+ opts[12].slen = hostname_len;
+ memcpy(opts[12].s, &c->hostname, hostname_len);
+ }
+
+ size_t fqdn_len = strlen(c->fqdn);
+ if (fqdn_len > 0) {
+ opts[81].s[0] = 0x4; // flags (E)
+ size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
+ (char *) opts[81].s + 3);
+ opts[81].slen = encoded_fqdn_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..b61a41d 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;
@@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)
+ * @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 +206,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 +217,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 +243,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 +365,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)
@@ -388,29 +406,56 @@ 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;
+ size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);
+ srch->hdr.l += encoded_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,
- ".");
- }
- }
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 ctx *c, char *buf, int offset)
+{
+ size_t fqdn_len, opt_hdr_len = 0;
+
+ fqdn_len = strlen(c->fqdn);
+ opt_hdr_len = sizeof(struct opt_hdr);
+
+ if (offset + opt_hdr_len + fqdn_len + 1 > 508) {
+ fqdn_len = 508 - (offset + opt_hdr_len + 1);
+ }
+
+ if (fqdn_len == 0) {
+ return offset;
+ }
+
+
+ struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset);
+ size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
+ o->domain_name);
+ size_t opt_len = encoded_fqdn_len + 1;
+
+ o->hdr.t = OPT_CLIENT_FQDN;
+ o->hdr.l = htons(opt_len);
+ o->flags = 0x00;
+
+ return offset + opt_hdr_len + opt_len;
+}
+
/**
* dhcpv6() - Check if this is a DHCPv6 message, reply as needed
* @c: Execution context
@@ -544,6 +589,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(c, (char*)&resp, n);
resp.hdr.xid = mh->xid;
diff --git a/passt.1 b/passt.1
index d9cd33e..27e70a7 100644
--- a/passt.1
+++ b/passt.1
@@ -401,6 +401,17 @@ 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 client with.
+For dhcp client aware encode \fIname as DHCPv4 option 12 (hostname).
+
+.TP
+.BR \-\-fqdn " " \fIname
+FQDN to configure client with.
+For dhcp client aware encode \fIname as dhcp client fqdn option, for DHCPv4 option 81 and for
+DHCPv6 option 39.
+
.SS \fBpasst\fR-only options
.TP
diff --git a/passt.h b/passt.h
index 0dd4efa..577e734 100644
--- a/passt.h
+++ b/passt.h
@@ -91,6 +91,7 @@ struct fqdn {
char n[NS_MAXDNAME];
};
+
#include <net/if.h>
#include <linux/un.h>
@@ -209,6 +210,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
@@ -268,6 +271,9 @@ struct ctx {
struct ip4_ctx ip4;
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..cdc9aa6 100644
--- a/pasta.c
+++ b/pasta.c
@@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
struct pasta_spawn_cmd_arg {
const char *exe;
char *const *argv;
+ struct ctx *c;
};
/**
@@ -187,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;
sigset_t set;
+ size_t conf_hostname_len = 0;
/* We run in a detached PID and mount namespace: mount /proc over */
if (mount("", "/proc", "proc", 0, NULL))
@@ -194,8 +196,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,
+
+ 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';
@@ -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..f3cf7b1 100644
--- a/util.c
+++ b/util.c
@@ -837,3 +837,25 @@ 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
+ * @domain_name: Input domain name to encode
+ * @len: Domain name length
+ * @buf: Buffer to fill in with encoded domain name
+ *
+ * Return: encoded domain name length
+ */
+size_t encode_domain_name(const char *domain_name, size_t len, char *buf){
+ char *p = NULL;
+ size_t i = 0;
+
+ buf[0] = strcspn(domain_name, ".");
+ p = buf + 1;
+ for (i = 0; i < len; i++) {
+ if (domain_name[i] == '.')
+ p[i] = strcspn(domain_name + i + 1, ".");
+ else
+ p[i] = domain_name[i];
+ }
+ return len + 2;
+}
diff --git a/util.h b/util.h
index 3fa1d12..d7751a8 100644
--- a/util.h
+++ b/util.h
@@ -346,4 +346,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 */
+size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
+
#endif /* UTIL_H */
--
@@ -346,4 +346,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 */
+size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
+
#endif /* UTIL_H */
--
2.47.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-17 6:50 [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
@ 2024-12-17 23:34 ` Stefano Brivio
2024-12-18 11:57 ` Enrique Llorente Pastora
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2024-12-17 23:34 UTC (permalink / raw)
To: Enrique Llorente; +Cc: passt-dev
The truncation logic looks good to me (as a first step). Some comments
below:
On Tue, 17 Dec 2024 07:50:28 +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 | 23 ++++++++++++++--
> dhcp.c | 39 +++++++++++++++++++++++---
> dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++---------
> passt.1 | 11 ++++++++
> passt.h | 6 ++++
> pasta.c | 15 ++++++++--
> test/lib/setup | 10 +++----
> test/passt.mbuto | 6 ++--
> test/passt/dhcp | 15 +++++++++-
> util.c | 22 +++++++++++++++
> util.h | 3 ++
> 11 files changed, 191 insertions(+), 31 deletions(-)
>
> diff --git a/conf.c b/conf.c
> index df2b016..29e4bed 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,12 @@ 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)) {
Nit: arguments should be visually aligned to the function you're
calling, that is, here:
if (snprintf_check(c->fqdn, PASST_MAXDNAME,
"%s", optarg)) {
> + die("Invalid fqdn: %s", optarg);
FQDN
> + }
> + break;
> case 'd':
> c->debug = 1;
> c->quiet = 0;
> @@ -1727,6 +1737,12 @@ 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)) {
Same as above.
> + die("Invalid hostname: %s", optarg);
> + }
> + break;
> case '4':
> v4_only = true;
> v6_only = false;
> @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
>
> c->one_off = true;
> break;
> +
Spurious change.
> case 't':
> case 'u':
> case 'T':
> diff --git a/dhcp.c b/dhcp.c
> index d8515aa..a04462b 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -62,6 +62,7 @@ static struct opt opts[255];
> #define DHCPFORCERENEW 9
>
> #define OPT_MIN 60 /* RFC 951 */
> +#define OPT_MAX 302 /* UDP packet limit - dhcp headers */
The original value for o[] below, 308, is 576 (see RFC 2131, page 10)
minus 268 (offset of options within an *IP* datagram).
How did you calculate 302 (+2)? I can't find out.
By the way, this reminds me that, as I mentioned previously, we should
really stop recycling the memory from the original message and simply
build a new one instead. It's not so much about security (a DHCP client
can anyway wreck the networking of its guest at will), but about
robustness. This becomes more relevant as we fill up replies more and
more.
> /**
> * dhcp_init() - Initialise DHCP options
> @@ -122,7 +123,7 @@ struct msg {
> uint8_t sname[64];
> uint8_t file[128];
> uint32_t magic;
> - uint8_t o[308];
> + uint8_t o[OPT_MAX+2];
Spaces around operator for consistency (OPT_MAX + 2). Should it really
be + 2, though?
> } __attribute__((__packed__));
>
> /**
> @@ -131,14 +132,28 @@ struct msg {
> * @o: Option number
> * @offset: Current offset within options field, updated on insertion
> */
> -static void fill_one(struct msg *m, int o, int *offset)
> +static int fill_one(struct msg *m, int o, int *offset)
You should update the function comment to say what it returns, now that
it returns something (try: 'grep Return: *.c').
> {
> + size_t idx, slen = 0;
> +
> + // We cannot write even enum + len + one byte then just skip
For consistency, no "C++ style" comments.
I would make it clear it's a conditional: "If we can't ..., then ..."
> + if (*offset + 2 > OPT_MAX) {
> + return OPT_MAX;
> + }
No need for curly brackets here.
> m->o[*offset] = o;
> m->o[*offset + 1] = opts[o].slen;
> - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> + idx=*offset + 2;
> + slen=opts[o].slen;
Spaces around assignment operator (idx = *offset ...).
> +
> + // Truncate if it goes beyond OPT_MAX
No C++ style comments.
> + if (idx + slen > OPT_MAX) {
> + slen = OPT_MAX - idx;
> + }
No need for curly brackets.
> + memcpy(&m->o[*offset + 2], opts[o].s, slen);
>
> opts[o].sent = 1;
> *offset += 2 + opts[o].slen;
> + return *offset;
> }
>
> /**
> @@ -172,7 +187,9 @@ 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 (fill_one(m, o, &offset) == OPT_MAX) {
> + break;
> + }
No need for curly brackets. We should log a debug message, here (a
warning would be more appropriate but I'm a bit wary of a malicious DHCP
client exploiting this to make log files blow up). Something like:
debug("DHCP: truncating after option %i", o);
> }
>
> m->o[offset++] = 255;
> @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
> if (!opts[6].slen)
> opts[6].slen = -1;
>
> + size_t hostname_len = strlen(c->hostname);
No mixed declarations and code (move the hostname_len declaration at
the beginning of the function).
> + if (hostname_len > 0) {
> + opts[12].slen = hostname_len;
> + memcpy(opts[12].s, &c->hostname, hostname_len);
> + }
> +
> + size_t fqdn_len = strlen(c->fqdn);
Same as above.
> + if (fqdn_len > 0) {
> + opts[81].s[0] = 0x4; // flags (E)
No C++ style comments.
> + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> + (char *) opts[81].s + 3);
Don't mix declarations and code. I think that 'encoded_len' would be
clear enough here.
> + opts[81].slen = encoded_fqdn_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..b61a41d 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;
> @@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)
"described by RFC 4704" (see also comments to v3).
> + * @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 +206,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 +217,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 +243,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 +365,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)
> @@ -388,29 +406,56 @@ 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;
> + size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);
No mixed declarations and code. Wrap at 80 columns if possible.
> + srch->hdr.l += encoded_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,
> - ".");
> - }
> - }
> srch->hdr.l = htons(srch->hdr.l);
Hah, thanks, that looks much better now.
> }
>
> 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 ctx *c, char *buf, int offset)
> +{
> + size_t fqdn_len, opt_hdr_len = 0;
> +
> + fqdn_len = strlen(c->fqdn);
> + opt_hdr_len = sizeof(struct opt_hdr);
> +
> + if (offset + opt_hdr_len + fqdn_len + 1 > 508) {
I'm a bit lost: why 508? Would it be possible to have this as a define
using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?
> + fqdn_len = 508 - (offset + opt_hdr_len + 1);
> + }
> +
> + if (fqdn_len == 0) {
> + return offset;
> + }
No need for curly brackets.
> +
> +
> + struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset);
> + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> + o->domain_name);
> + size_t opt_len = encoded_fqdn_len + 1;
No mixed declarations and code.
> +
> + o->hdr.t = OPT_CLIENT_FQDN;
> + o->hdr.l = htons(opt_len);
> + o->flags = 0x00;
> +
> + return offset + opt_hdr_len + opt_len;
> +}
> +
> /**
> * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> * @c: Execution context
> @@ -544,6 +589,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(c, (char*)&resp, n);
(char *) (for consistency, see line just above)
>
> resp.hdr.xid = mh->xid;
>
> diff --git a/passt.1 b/passt.1
> index d9cd33e..27e70a7 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -401,6 +401,17 @@ 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 client with.
> +For dhcp client aware encode \fIname as DHCPv4 option 12 (hostname).
DHCP: there's no such thing as DHCPv4.
...who is aware of... what? I would just say "Send \fIname\fR as
hostname via DHCP (Option 12)". I don't think we reason much about what
the client expects or not, right?
> +
> +.TP
> +.BR \-\-fqdn " " \fIname
> +FQDN to configure client with.
> +For dhcp client aware encode \fIname as dhcp client fqdn option, for DHCPv4 option 81 and for
Same here, I'm not sure who is aware of what: I would just say we send
\fIname\fR as FQDN (option 81 ...). Same as above about DHCP, FQDN, and
DHCPv4.
> +DHCPv6 option 39.
> +
> .SS \fBpasst\fR-only options
>
> .TP
> diff --git a/passt.h b/passt.h
> index 0dd4efa..577e734 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -91,6 +91,7 @@ struct fqdn {
> char n[NS_MAXDNAME];
> };
>
> +
Spurious change.
> #include <net/if.h>
> #include <linux/un.h>
>
> @@ -209,6 +210,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
> @@ -268,6 +271,9 @@ struct ctx {
> struct ip4_ctx ip4;
>
> 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..cdc9aa6 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> struct pasta_spawn_cmd_arg {
> const char *exe;
> char *const *argv;
> + struct ctx *c;
> };
>
> /**
> @@ -187,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;
> sigset_t set;
> + size_t conf_hostname_len = 0;
Align from longest to shortest:
const struct pasta_spawn_cmd_arg *a;
size_t conf_hostname_len = 0;
sigset_t set;
and there's no need to initialise conf_hostname_len (it makes the
reader think that it might be used without the strlen() assignment in
some cases, but it's not true).
>
> /* We run in a detached PID and mount namespace: mount /proc over */
> if (mount("", "/proc", "proc", 0, NULL))
> @@ -194,8 +196,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,
> +
> + 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,
} else if
> HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> errno == ENAMETOOLONG) {
These should move to be aligned with the arguments of gethostname() and with
the if clause:
} else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
errno == ENAMETOOLONG) {
> hostname[HOST_NAME_MAX] = '\0';
> @@ -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..f3cf7b1 100644
> --- a/util.c
> +++ b/util.c
> @@ -837,3 +837,25 @@ 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
> + * @domain_name: Input domain name to encode
> + * @len: Domain name length
> + * @buf: Buffer to fill in with encoded domain name
> + *
> + * Return: encoded domain name length
> + */
> +size_t encode_domain_name(const char *domain_name, size_t len, char *buf){
Opening curly bracket on a newline, for consistency.
> + char *p = NULL;
No need to initialise this, it's assigned unconditionally below.
> + size_t i = 0;
No need to initialise this, you only use it in the loop below.
> +
> + buf[0] = strcspn(domain_name, ".");
> + p = buf + 1;
> + for (i = 0; i < len; i++) {
> + if (domain_name[i] == '.')
> + p[i] = strcspn(domain_name + i + 1, ".");
> + else
> + p[i] = domain_name[i];
> + }
> + return len + 2;
> +}
> diff --git a/util.h b/util.h
> index 3fa1d12..d7751a8 100644
> --- a/util.h
> +++ b/util.h
> @@ -346,4 +346,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 */
> +size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
> +
> #endif /* UTIL_H */
--
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-17 23:34 ` Stefano Brivio
@ 2024-12-18 11:57 ` Enrique Llorente Pastora
2024-12-19 0:19 ` Stefano Brivio
0 siblings, 1 reply; 6+ messages in thread
From: Enrique Llorente Pastora @ 2024-12-18 11:57 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
On Wed, Dec 18, 2024 at 12:35 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> The truncation logic looks good to me (as a first step). Some comments
> below:
>
> On Tue, 17 Dec 2024 07:50:28 +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 | 23 ++++++++++++++--
> > dhcp.c | 39 +++++++++++++++++++++++---
> > dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++---------
> > passt.1 | 11 ++++++++
> > passt.h | 6 ++++
> > pasta.c | 15 ++++++++--
> > test/lib/setup | 10 +++----
> > test/passt.mbuto | 6 ++--
> > test/passt/dhcp | 15 +++++++++-
> > util.c | 22 +++++++++++++++
> > util.h | 3 ++
> > 11 files changed, 191 insertions(+), 31 deletions(-)
> >
> > diff --git a/conf.c b/conf.c
> > index df2b016..29e4bed 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,12 @@ 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)) {
>
> Nit: arguments should be visually aligned to the function you're
> calling, that is, here:
>
> if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> "%s", optarg)) {
Done.
>
>
> > + die("Invalid fqdn: %s", optarg);
>
> FQDN
Done.
>
> > + }
> > + break;
> > case 'd':
> > c->debug = 1;
> > c->quiet = 0;
> > @@ -1727,6 +1737,12 @@ 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)) {
>
> Same as above.
>
Done.
> > + die("Invalid hostname: %s", optarg);
> > + }
> > + break;
> > case '4':
> > v4_only = true;
> > v6_only = false;
> > @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >
> > c->one_off = true;
> > break;
> > +
>
> Spurious change.
Done.
>
> > case 't':
> > case 'u':
> > case 'T':
> > diff --git a/dhcp.c b/dhcp.c
> > index d8515aa..a04462b 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -62,6 +62,7 @@ static struct opt opts[255];
> > #define DHCPFORCERENEW 9
> >
> > #define OPT_MIN 60 /* RFC 951 */
> > +#define OPT_MAX 302 /* UDP packet limit - dhcp headers */
>
> The original value for o[] below, 308, is 576 (see RFC 2131, page 10)
> minus 268 (offset of options within an *IP* datagram).
>
> How did you calculate 302 (+2)? I can't find out.
>
My bad it's 306, I found that we are adding a pair of extra bytes
after the options so
that's why I am using 306 instead of 308 for checkings:
m->o[offset++] = 255;
m->o[offset++] = 0;
> By the way, this reminds me that, as I mentioned previously, we should
> really stop recycling the memory from the original message and simply
> build a new one instead. It's not so much about security (a DHCP client
> can anyway wreck the networking of its guest at will), but about
> robustness. This becomes more relevant as we fill up replies more and
> more.
>
Do we do this at this change or at a follow up one ?
> > /**
> > * dhcp_init() - Initialise DHCP options
> > @@ -122,7 +123,7 @@ struct msg {
> > uint8_t sname[64];
> > uint8_t file[128];
> > uint32_t magic;
> > - uint8_t o[308];
> > + uint8_t o[OPT_MAX+2];
>
> Spaces around operator for consistency (OPT_MAX + 2). Should it really
> be + 2, though?
Yes, "fill(..)" function is adding a pair of extra ones at the end so
we should limit the option to make space for them.
m->o[offset++] = 255;
m->o[offset++] = 0;
>
> > } __attribute__((__packed__));
> >
> > /**
> > @@ -131,14 +132,28 @@ struct msg {
> > * @o: Option number
> > * @offset: Current offset within options field, updated on insertion
> > */
> > -static void fill_one(struct msg *m, int o, int *offset)
> > +static int fill_one(struct msg *m, int o, int *offset)
>
> You should update the function comment to say what it returns, now that
> it returns something (try: 'grep Return: *.c').
>
Done.
> > {
> > + size_t idx, slen = 0;
> > +
> > + // We cannot write even enum + len + one byte then just skip
>
> For consistency, no "C++ style" comments.
>
> I would make it clear it's a conditional: "If we can't ..., then ..."
>
Done
> > + if (*offset + 2 > OPT_MAX) {
> > + return OPT_MAX;
> > + }
>
> No need for curly brackets here.
Done.
>
> > m->o[*offset] = o;
> > m->o[*offset + 1] = opts[o].slen;
> > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > + idx=*offset + 2;
> > + slen=opts[o].slen;
>
> Spaces around assignment operator (idx = *offset ...).
Done.
>
> > +
> > + // Truncate if it goes beyond OPT_MAX
>
> No C++ style comments.
Done.
>
> > + if (idx + slen > OPT_MAX) {
> > + slen = OPT_MAX - idx;
> > + }
>
> No need for curly brackets.
Done.
>
> > + memcpy(&m->o[*offset + 2], opts[o].s, slen);
> >
> > opts[o].sent = 1;
> > *offset += 2 + opts[o].slen;
> > + return *offset;
> > }
> >
> > /**
> > @@ -172,7 +187,9 @@ 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 (fill_one(m, o, &offset) == OPT_MAX) {
> > + break;
> > + }
>
> No need for curly brackets. We should log a debug message, here (a
> warning would be more appropriate but I'm a bit wary of a malicious DHCP
> client exploiting this to make log files blow up). Something like:
>
> debug("DHCP: truncating after option %i", o);
>
Done.
> > }
> >
> > m->o[offset++] = 255;
> > @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > if (!opts[6].slen)
> > opts[6].slen = -1;
> >
> > + size_t hostname_len = strlen(c->hostname);
>
> No mixed declarations and code (move the hostname_len declaration at
> the beginning of the function).
Done.
>
> > + if (hostname_len > 0) {
> > + opts[12].slen = hostname_len;
> > + memcpy(opts[12].s, &c->hostname, hostname_len);
> > + }
> > +
> > + size_t fqdn_len = strlen(c->fqdn);
>
> Same as above.
Done.
>
> > + if (fqdn_len > 0) {
> > + opts[81].s[0] = 0x4; // flags (E)
>
> No C++ style comments.
Done.
>
> > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > + (char *) opts[81].s + 3);
>
> Don't mix declarations and code. I think that 'encoded_len' would be
> clear enough here.
Done.
>
> > + opts[81].slen = encoded_fqdn_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..b61a41d 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;
> > @@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)
>
> "described by RFC 4704" (see also comments to v3).
>
Done.
> > + * @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 +206,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 +217,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 +243,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 +365,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)
> > @@ -388,29 +406,56 @@ 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;
> > + size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);
>
> No mixed declarations and code. Wrap at 80 columns if possible.
Done.
>
> > + srch->hdr.l += encoded_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,
> > - ".");
> > - }
> > - }
> > srch->hdr.l = htons(srch->hdr.l);
>
> Hah, thanks, that looks much better now.
>
> > }
> >
> > 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 ctx *c, char *buf, int offset)
> > +{
> > + size_t fqdn_len, opt_hdr_len = 0;
> > +
> > + fqdn_len = strlen(c->fqdn);
> > + opt_hdr_len = sizeof(struct opt_hdr);
> > +
> > + if (offset + opt_hdr_len + fqdn_len + 1 > 508) {
>
> I'm a bit lost: why 508? Would it be possible to have this as a define
> using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?
>
That's the max UDP packet payload size, since "offset" is referring to
the payload not the whole UDP
maybe we can do we can do max_len = 576 - (sizeof(struct ipv6hdr) +
sizeof(udphdr)), don't know if it's worth it.
" * @dlen: UDP payload length (not including UDP header)"
> > + fqdn_len = 508 - (offset + opt_hdr_len + 1);
> > + }
> > +
> > + if (fqdn_len == 0) {
> > + return offset;
> > + }
>
> No need for curly brackets.
Done.
> > +
> > +
> > + struct opt_client_fqdn *o = (struct opt_client_fqdn *)(buf + offset);
> > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > + o->domain_name);
> > + size_t opt_len = encoded_fqdn_len + 1;
>
> No mixed declarations and code.
Done.
>
> > +
> > + o->hdr.t = OPT_CLIENT_FQDN;
> > + o->hdr.l = htons(opt_len);
> > + o->flags = 0x00;
> > +
> > + return offset + opt_hdr_len + opt_len;
> > +}
> > +
> > /**
> > * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
> > * @c: Execution context
> > @@ -544,6 +589,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(c, (char*)&resp, n);
>
> (char *) (for consistency, see line just above)
>
Done.
> >
> > resp.hdr.xid = mh->xid;
> >
> > diff --git a/passt.1 b/passt.1
> > index d9cd33e..27e70a7 100644
> > --- a/passt.1
> > +++ b/passt.1
> > @@ -401,6 +401,17 @@ 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 client with.
> > +For dhcp client aware encode \fIname as DHCPv4 option 12 (hostname).
>
> DHCP: there's no such thing as DHCPv4.
>
Done
> ...who is aware of... what? I would just say "Send \fIname\fR as
> hostname via DHCP (Option 12)". I don't think we reason much about what
> the client expects or not, right?
>
> > +
> > +.TP
> > +.BR \-\-fqdn " " \fIname
> > +FQDN to configure client with.
> > +For dhcp client aware encode \fIname as dhcp client fqdn option, for DHCPv4 option 81 and for
>
> Same here, I'm not sure who is aware of what: I would just say we send
> \fIname\fR as FQDN (option 81 ...). Same as above about DHCP, FQDN, and
> DHCPv4.
Done.
>
> > +DHCPv6 option 39.
> > +
> > .SS \fBpasst\fR-only options
> >
> > .TP
> > diff --git a/passt.h b/passt.h
> > index 0dd4efa..577e734 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -91,6 +91,7 @@ struct fqdn {
> > char n[NS_MAXDNAME];
> > };
> >
> > +
>
> Spurious change.
>
Done.
> > #include <net/if.h>
> > #include <linux/un.h>
> >
> > @@ -209,6 +210,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
> > @@ -268,6 +271,9 @@ struct ctx {
> > struct ip4_ctx ip4;
> >
> > 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..cdc9aa6 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -173,6 +173,7 @@ void pasta_open_ns(struct ctx *c, const char *netns)
> > struct pasta_spawn_cmd_arg {
> > const char *exe;
> > char *const *argv;
> > + struct ctx *c;
> > };
> >
> > /**
> > @@ -187,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;
> > sigset_t set;
> > + size_t conf_hostname_len = 0;
>
> Align from longest to shortest:
>
> const struct pasta_spawn_cmd_arg *a;
> size_t conf_hostname_len = 0;
> sigset_t set;
>
> and there's no need to initialise conf_hostname_len (it makes the
> reader think that it might be used without the strlen() assignment in
> some cases, but it's not true).
>
Done.
> >
> > /* We run in a detached PID and mount namespace: mount /proc over */
> > if (mount("", "/proc", "proc", 0, NULL))
> > @@ -194,8 +196,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,
> > +
> > + 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,
>
> } else if
>
> > HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> > errno == ENAMETOOLONG) {
>
> These should move to be aligned with the arguments of gethostname() and with
> the if clause:
>
> } else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> errno == ENAMETOOLONG) {
>
>
Done.
> > hostname[HOST_NAME_MAX] = '\0';
> > @@ -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..f3cf7b1 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -837,3 +837,25 @@ 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
> > + * @domain_name: Input domain name to encode
> > + * @len: Domain name length
> > + * @buf: Buffer to fill in with encoded domain name
> > + *
> > + * Return: encoded domain name length
> > + */
> > +size_t encode_domain_name(const char *domain_name, size_t len, char *buf){
>
> Opening curly bracket on a newline, for consistency.
>
Done.
> > + char *p = NULL;
>
> No need to initialise this, it's assigned unconditionally below.
>
Done.
> > + size_t i = 0;
>
> No need to initialise this, you only use it in the loop below.
>
Done.
> > +
> > + buf[0] = strcspn(domain_name, ".");
> > + p = buf + 1;
> > + for (i = 0; i < len; i++) {
> > + if (domain_name[i] == '.')
> > + p[i] = strcspn(domain_name + i + 1, ".");
> > + else
> > + p[i] = domain_name[i];
> > + }
> > + return len + 2;
> > +}
> > diff --git a/util.h b/util.h
> > index 3fa1d12..d7751a8 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -346,4 +346,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 */
> > +size_t encode_domain_name(const char* domain_name, size_t len, char* buf);
> > +
> > #endif /* UTIL_H */
>
> --
> Stefano
>
--
Quique Llorente
CNV networking Senior Software Engineer
Red Hat EMEA
ellorent@redhat.com
@RedHat Red Hat Red Hat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-18 11:57 ` Enrique Llorente Pastora
@ 2024-12-19 0:19 ` Stefano Brivio
2024-12-19 11:51 ` Enrique Llorente Pastora
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Brivio @ 2024-12-19 0:19 UTC (permalink / raw)
To: Enrique Llorente Pastora; +Cc: passt-dev
On Wed, 18 Dec 2024 12:57:24 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> On Wed, Dec 18, 2024 at 12:35 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > The truncation logic looks good to me (as a first step). Some comments
> > below:
> >
> > On Tue, 17 Dec 2024 07:50:28 +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 | 23 ++++++++++++++--
> > > dhcp.c | 39 +++++++++++++++++++++++---
> > > dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++---------
> > > passt.1 | 11 ++++++++
> > > passt.h | 6 ++++
> > > pasta.c | 15 ++++++++--
> > > test/lib/setup | 10 +++----
> > > test/passt.mbuto | 6 ++--
> > > test/passt/dhcp | 15 +++++++++-
> > > util.c | 22 +++++++++++++++
> > > util.h | 3 ++
> > > 11 files changed, 191 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/conf.c b/conf.c
> > > index df2b016..29e4bed 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,12 @@ 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)) {
> >
> > Nit: arguments should be visually aligned to the function you're
> > calling, that is, here:
> >
> > if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > "%s", optarg)) {
>
> Done.
>
> >
> >
> > > + die("Invalid fqdn: %s", optarg);
> >
> > FQDN
>
> Done.
>
> >
> > > + }
> > > + break;
> > > case 'd':
> > > c->debug = 1;
> > > c->quiet = 0;
> > > @@ -1727,6 +1737,12 @@ 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)) {
> >
> > Same as above.
> >
>
> Done.
>
> > > + die("Invalid hostname: %s", optarg);
> > > + }
> > > + break;
> > > case '4':
> > > v4_only = true;
> > > v6_only = false;
> > > @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > >
> > > c->one_off = true;
> > > break;
> > > +
> >
> > Spurious change.
>
> Done.
>
> >
> > > case 't':
> > > case 'u':
> > > case 'T':
> > > diff --git a/dhcp.c b/dhcp.c
> > > index d8515aa..a04462b 100644
> > > --- a/dhcp.c
> > > +++ b/dhcp.c
> > > @@ -62,6 +62,7 @@ static struct opt opts[255];
> > > #define DHCPFORCERENEW 9
> > >
> > > #define OPT_MIN 60 /* RFC 951 */
> > > +#define OPT_MAX 302 /* UDP packet limit - dhcp headers */
> >
> > The original value for o[] below, 308, is 576 (see RFC 2131, page 10)
> > minus 268 (offset of options within an *IP* datagram).
> >
> > How did you calculate 302 (+2)? I can't find out.
>
> My bad it's 306, I found that we are adding a pair of extra bytes
> after the options so
> that's why I am using 306 instead of 308 for checkings:
>
> m->o[offset++] = 255;
> m->o[offset++] = 0;
Right, that's option 255 ("End") and its length (0). Can we have a
comment here actually explaining that? Say:
/* 576 (RFC 2131), minus offset
* of options (268), minus end
* option and its length (2)
*/
> > By the way, this reminds me that, as I mentioned previously, we should
> > really stop recycling the memory from the original message and simply
> > build a new one instead. It's not so much about security (a DHCP client
> > can anyway wreck the networking of its guest at will), but about
> > robustness. This becomes more relevant as we fill up replies more and
> > more.
>
> Do we do this at this change or at a follow up one ?
It's a logically separated change, so it can/should be a different
patch but I think that we really risk breaking things, so I would only
push them together. It can be before or after this change.
It should be kind of simple: instead of filling up (and sending) 'm',
we should declare and initialise a separate struct msg, say:
struct msg r = { 0 };
and use that. We'll need to copy a few fields from the request
('htype', 'hlen', 'xid', 'flags', 'giaddr', 'chaddr', see page 28 of
RFC 2131), but that's about it.
If you're lost I can also submit a patch on top of this one later, let
me know.
> > > /**
> > > * dhcp_init() - Initialise DHCP options
> > > @@ -122,7 +123,7 @@ struct msg {
> > > uint8_t sname[64];
> > > uint8_t file[128];
> > > uint32_t magic;
> > > - uint8_t o[308];
> > > + uint8_t o[OPT_MAX+2];
> >
> > Spaces around operator for consistency (OPT_MAX + 2). Should it really
> > be + 2, though?
>
> Yes, "fill(..)" function is adding a pair of extra ones at the end so
> we should limit the option to make space for them.
>
> m->o[offset++] = 255;
> m->o[offset++] = 0;
Then we should make it clear: OPT_MAX + 2 /* End option and length */
> > > } __attribute__((__packed__));
> > >
> > > /**
> > > @@ -131,14 +132,28 @@ struct msg {
> > > * @o: Option number
> > > * @offset: Current offset within options field, updated on insertion
> > > */
> > > -static void fill_one(struct msg *m, int o, int *offset)
> > > +static int fill_one(struct msg *m, int o, int *offset)
> >
> > You should update the function comment to say what it returns, now that
> > it returns something (try: 'grep Return: *.c').
> >
>
> Done.
>
> > > {
> > > + size_t idx, slen = 0;
> > > +
> > > + // We cannot write even enum + len + one byte then just skip
> >
> > For consistency, no "C++ style" comments.
> >
> > I would make it clear it's a conditional: "If we can't ..., then ..."
> >
>
> Done
>
> > > + if (*offset + 2 > OPT_MAX) {
> > > + return OPT_MAX;
> > > + }
> >
> > No need for curly brackets here.
>
> Done.
>
> >
> > > m->o[*offset] = o;
> > > m->o[*offset + 1] = opts[o].slen;
> > > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > + idx=*offset + 2;
> > > + slen=opts[o].slen;
> >
> > Spaces around assignment operator (idx = *offset ...).
>
> Done.
>
> >
> > > +
> > > + // Truncate if it goes beyond OPT_MAX
> >
> > No C++ style comments.
>
> Done.
>
> >
> > > + if (idx + slen > OPT_MAX) {
> > > + slen = OPT_MAX - idx;
> > > + }
> >
> > No need for curly brackets.
>
> Done.
>
> >
> > > + memcpy(&m->o[*offset + 2], opts[o].s, slen);
> > >
> > > opts[o].sent = 1;
> > > *offset += 2 + opts[o].slen;
> > > + return *offset;
> > > }
> > >
> > > /**
> > > @@ -172,7 +187,9 @@ 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 (fill_one(m, o, &offset) == OPT_MAX) {
> > > + break;
> > > + }
> >
> > No need for curly brackets. We should log a debug message, here (a
> > warning would be more appropriate but I'm a bit wary of a malicious DHCP
> > client exploiting this to make log files blow up). Something like:
> >
> > debug("DHCP: truncating after option %i", o);
> >
>
> Done.
>
> > > }
> > >
> > > m->o[offset++] = 255;
> > > @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > if (!opts[6].slen)
> > > opts[6].slen = -1;
> > >
> > > + size_t hostname_len = strlen(c->hostname);
> >
> > No mixed declarations and code (move the hostname_len declaration at
> > the beginning of the function).
>
> Done.
>
> >
> > > + if (hostname_len > 0) {
> > > + opts[12].slen = hostname_len;
> > > + memcpy(opts[12].s, &c->hostname, hostname_len);
> > > + }
> > > +
> > > + size_t fqdn_len = strlen(c->fqdn);
> >
> > Same as above.
>
> Done.
>
> >
> > > + if (fqdn_len > 0) {
> > > + opts[81].s[0] = 0x4; // flags (E)
> >
> > No C++ style comments.
>
> Done.
>
> >
> > > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > > + (char *) opts[81].s + 3);
> >
> > Don't mix declarations and code. I think that 'encoded_len' would be
> > clear enough here.
>
> Done.
>
> >
> > > + opts[81].slen = encoded_fqdn_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..b61a41d 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;
> > > @@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)
> >
> > "described by RFC 4704" (see also comments to v3).
> >
>
> Done.
>
> > > + * @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 +206,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 +217,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 +243,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 +365,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)
> > > @@ -388,29 +406,56 @@ 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;
> > > + size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);
> >
> > No mixed declarations and code. Wrap at 80 columns if possible.
>
> Done.
>
> >
> > > + srch->hdr.l += encoded_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,
> > > - ".");
> > > - }
> > > - }
> > > srch->hdr.l = htons(srch->hdr.l);
> >
> > Hah, thanks, that looks much better now.
> >
> > > }
> > >
> > > 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 ctx *c, char *buf, int offset)
> > > +{
> > > + size_t fqdn_len, opt_hdr_len = 0;
> > > +
> > > + fqdn_len = strlen(c->fqdn);
> > > + opt_hdr_len = sizeof(struct opt_hdr);
> > > +
> > > + if (offset + opt_hdr_len + fqdn_len + 1 > 508) {
> >
> > I'm a bit lost: why 508? Would it be possible to have this as a define
> > using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?
>
> That's the max UDP packet payload size, since "offset" is referring to
> the payload not the whole UDP
> maybe we can do we can do max_len = 576 - (sizeof(struct ipv6hdr) +
> sizeof(udphdr)), don't know if it's worth it.
Well, that would be 528... so I think it's worth it. :)
But in any case, this is not IPv4. From RFC 8415:
The server must be aware of the recommendations on packet sizes and
the use of fragmentation as discussed in Section 5 of [RFC8200].
...so we should start from 1280 (IPV6_MIN_MTU) bytes here, subtract the
size of an IPv6 header (40), of a UDP header (8), and of the message
header (sizeof(struct msg_hdr), 4).
I think we should really use defined constants and sizeof() otherwise
it gets confusing quite fast.
> " * @dlen: UDP payload length (not including UDP header)"
I guess we could have it as a define (OPT_MAX_SIZE?), no need to have
it in any struct.
By the way, note that we have MAX() defined in util.h, so you don't
need a condition and a separate statement, you can just do:
> > > + fqdn_len = 508 - (offset + opt_hdr_len + 1);
fqdn_len = MAX(fqdn_len, offset + ...);
--
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-19 0:19 ` Stefano Brivio
@ 2024-12-19 11:51 ` Enrique Llorente Pastora
2024-12-19 12:06 ` Stefano Brivio
0 siblings, 1 reply; 6+ messages in thread
From: Enrique Llorente Pastora @ 2024-12-19 11:51 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
On Thu, Dec 19, 2024 at 1:19 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Wed, 18 Dec 2024 12:57:24 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Wed, Dec 18, 2024 at 12:35 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > The truncation logic looks good to me (as a first step). Some comments
> > > below:
> > >
> > > On Tue, 17 Dec 2024 07:50:28 +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 | 23 ++++++++++++++--
> > > > dhcp.c | 39 +++++++++++++++++++++++---
> > > > dhcpv6.c | 72 +++++++++++++++++++++++++++++++++++++++---------
> > > > passt.1 | 11 ++++++++
> > > > passt.h | 6 ++++
> > > > pasta.c | 15 ++++++++--
> > > > test/lib/setup | 10 +++----
> > > > test/passt.mbuto | 6 ++--
> > > > test/passt/dhcp | 15 +++++++++-
> > > > util.c | 22 +++++++++++++++
> > > > util.h | 3 ++
> > > > 11 files changed, 191 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/conf.c b/conf.c
> > > > index df2b016..29e4bed 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,12 @@ 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)) {
> > >
> > > Nit: arguments should be visually aligned to the function you're
> > > calling, that is, here:
> > >
> > > if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> > > "%s", optarg)) {
> >
> > Done.
> >
> > >
> > >
> > > > + die("Invalid fqdn: %s", optarg);
> > >
> > > FQDN
> >
> > Done.
> >
> > >
> > > > + }
> > > > + break;
> > > > case 'd':
> > > > c->debug = 1;
> > > > c->quiet = 0;
> > > > @@ -1727,6 +1737,12 @@ 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)) {
> > >
> > > Same as above.
> > >
> >
> > Done.
> >
> > > > + die("Invalid hostname: %s", optarg);
> > > > + }
> > > > + break;
> > > > case '4':
> > > > v4_only = true;
> > > > v6_only = false;
> > > > @@ -1741,6 +1757,7 @@ void conf(struct ctx *c, int argc, char **argv)
> > > >
> > > > c->one_off = true;
> > > > break;
> > > > +
> > >
> > > Spurious change.
> >
> > Done.
> >
> > >
> > > > case 't':
> > > > case 'u':
> > > > case 'T':
> > > > diff --git a/dhcp.c b/dhcp.c
> > > > index d8515aa..a04462b 100644
> > > > --- a/dhcp.c
> > > > +++ b/dhcp.c
> > > > @@ -62,6 +62,7 @@ static struct opt opts[255];
> > > > #define DHCPFORCERENEW 9
> > > >
> > > > #define OPT_MIN 60 /* RFC 951 */
> > > > +#define OPT_MAX 302 /* UDP packet limit - dhcp headers */
> > >
> > > The original value for o[] below, 308, is 576 (see RFC 2131, page 10)
> > > minus 268 (offset of options within an *IP* datagram).
> > >
> > > How did you calculate 302 (+2)? I can't find out.
> >
> > My bad it's 306, I found that we are adding a pair of extra bytes
> > after the options so
> > that's why I am using 306 instead of 308 for checkings:
> >
> > m->o[offset++] = 255;
> > m->o[offset++] = 0;
>
> Right, that's option 255 ("End") and its length (0). Can we have a
> comment here actually explaining that? Say:
>
> /* 576 (RFC 2131), minus offset
> * of options (268), minus end
> * option and its length (2)
> */
Sure, will add the comment.
>
> > > By the way, this reminds me that, as I mentioned previously, we should
> > > really stop recycling the memory from the original message and simply
> > > build a new one instead. It's not so much about security (a DHCP client
> > > can anyway wreck the networking of its guest at will), but about
> > > robustness. This becomes more relevant as we fill up replies more and
> > > more.
> >
> > Do we do this at this change or at a follow up one ?
>
> It's a logically separated change, so it can/should be a different
> patch but I think that we really risk breaking things, so I would only
> push them together. It can be before or after this change.
>
> It should be kind of simple: instead of filling up (and sending) 'm',
> we should declare and initialise a separate struct msg, say:
>
> struct msg r = { 0 };
>
> and use that. We'll need to copy a few fields from the request
> ('htype', 'hlen', 'xid', 'flags', 'giaddr', 'chaddr', see page 28 of
> RFC 2131), but that's about it.
>
> If you're lost I can also submit a patch on top of this one later, let
> me know.
I will do a new patch after this one to cover that.
>
> > > > /**
> > > > * dhcp_init() - Initialise DHCP options
> > > > @@ -122,7 +123,7 @@ struct msg {
> > > > uint8_t sname[64];
> > > > uint8_t file[128];
> > > > uint32_t magic;
> > > > - uint8_t o[308];
> > > > + uint8_t o[OPT_MAX+2];
> > >
> > > Spaces around operator for consistency (OPT_MAX + 2). Should it really
> > > be + 2, though?
> >
> > Yes, "fill(..)" function is adding a pair of extra ones at the end so
> > we should limit the option to make space for them.
> >
> > m->o[offset++] = 255;
> > m->o[offset++] = 0;
>
> Then we should make it clear: OPT_MAX + 2 /* End option and length */
>
Make sense adding that too.
> > > > } __attribute__((__packed__));
> > > >
> > > > /**
> > > > @@ -131,14 +132,28 @@ struct msg {
> > > > * @o: Option number
> > > > * @offset: Current offset within options field, updated on insertion
> > > > */
> > > > -static void fill_one(struct msg *m, int o, int *offset)
> > > > +static int fill_one(struct msg *m, int o, int *offset)
> > >
> > > You should update the function comment to say what it returns, now that
> > > it returns something (try: 'grep Return: *.c').
> > >
> >
> > Done.
> >
> > > > {
> > > > + size_t idx, slen = 0;
> > > > +
> > > > + // We cannot write even enum + len + one byte then just skip
> > >
> > > For consistency, no "C++ style" comments.
> > >
> > > I would make it clear it's a conditional: "If we can't ..., then ..."
> > >
> >
> > Done
> >
> > > > + if (*offset + 2 > OPT_MAX) {
> > > > + return OPT_MAX;
> > > > + }
> > >
> > > No need for curly brackets here.
> >
> > Done.
> >
> > >
> > > > m->o[*offset] = o;
> > > > m->o[*offset + 1] = opts[o].slen;
> > > > - memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> > > > + idx=*offset + 2;
> > > > + slen=opts[o].slen;
> > >
> > > Spaces around assignment operator (idx = *offset ...).
> >
> > Done.
> >
> > >
> > > > +
> > > > + // Truncate if it goes beyond OPT_MAX
> > >
> > > No C++ style comments.
> >
> > Done.
> >
> > >
> > > > + if (idx + slen > OPT_MAX) {
> > > > + slen = OPT_MAX - idx;
> > > > + }
> > >
> > > No need for curly brackets.
> >
> > Done.
> >
> > >
> > > > + memcpy(&m->o[*offset + 2], opts[o].s, slen);
> > > >
> > > > opts[o].sent = 1;
> > > > *offset += 2 + opts[o].slen;
> > > > + return *offset;
> > > > }
> > > >
> > > > /**
> > > > @@ -172,7 +187,9 @@ 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 (fill_one(m, o, &offset) == OPT_MAX) {
> > > > + break;
> > > > + }
> > >
> > > No need for curly brackets. We should log a debug message, here (a
> > > warning would be more appropriate but I'm a bit wary of a malicious DHCP
> > > client exploiting this to make log files blow up). Something like:
> > >
> > > debug("DHCP: truncating after option %i", o);
> > >
> >
> > Done.
> >
> > > > }
> > > >
> > > > m->o[offset++] = 255;
> > > > @@ -398,6 +415,20 @@ int dhcp(const struct ctx *c, const struct pool *p)
> > > > if (!opts[6].slen)
> > > > opts[6].slen = -1;
> > > >
> > > > + size_t hostname_len = strlen(c->hostname);
> > >
> > > No mixed declarations and code (move the hostname_len declaration at
> > > the beginning of the function).
> >
> > Done.
> >
> > >
> > > > + if (hostname_len > 0) {
> > > > + opts[12].slen = hostname_len;
> > > > + memcpy(opts[12].s, &c->hostname, hostname_len);
> > > > + }
> > > > +
> > > > + size_t fqdn_len = strlen(c->fqdn);
> > >
> > > Same as above.
> >
> > Done.
> >
> > >
> > > > + if (fqdn_len > 0) {
> > > > + opts[81].s[0] = 0x4; // flags (E)
> > >
> > > No C++ style comments.
> >
> > Done.
> >
> > >
> > > > + size_t encoded_fqdn_len = encode_domain_name(c->fqdn, fqdn_len,
> > > > + (char *) opts[81].s + 3);
> > >
> > > Don't mix declarations and code. I think that 'encoded_len' would be
> > > clear enough here.
> >
> > Done.
> >
> > >
> > > > + opts[81].slen = encoded_fqdn_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..b61a41d 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;
> > > > @@ -163,6 +164,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 as stated at RFC 4704 (always zero for us)
> > >
> > > "described by RFC 4704" (see also comments to v3).
> > >
> >
> > Done.
> >
> > > > + * @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 +206,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 +217,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 +243,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 +365,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)
> > > > @@ -388,29 +406,56 @@ 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;
> > > > + size_t encoded_name_len = encode_domain_name(c->dns_search[i].n, name_len, srch->list);
> > >
> > > No mixed declarations and code. Wrap at 80 columns if possible.
> >
> > Done.
> >
> > >
> > > > + srch->hdr.l += encoded_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,
> > > > - ".");
> > > > - }
> > > > - }
> > > > srch->hdr.l = htons(srch->hdr.l);
> > >
> > > Hah, thanks, that looks much better now.
> > >
> > > > }
> > > >
> > > > 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 ctx *c, char *buf, int offset)
> > > > +{
> > > > + size_t fqdn_len, opt_hdr_len = 0;
> > > > +
> > > > + fqdn_len = strlen(c->fqdn);
> > > > + opt_hdr_len = sizeof(struct opt_hdr);
> > > > +
> > > > + if (offset + opt_hdr_len + fqdn_len + 1 > 508) {
> > >
> > > I'm a bit lost: why 508? Would it be possible to have this as a define
> > > using sizeof() on the relevant structs (struct udphdr, struct ipv6hdr)?
> >
> > That's the max UDP packet payload size, since "offset" is referring to
> > the payload not the whole UDP
> > maybe we can do we can do max_len = 576 - (sizeof(struct ipv6hdr) +
> > sizeof(udphdr)), don't know if it's worth it.
>
> Well, that would be 528... so I think it's worth it. :)
>
> But in any case, this is not IPv4. From RFC 8415:
>
> The server must be aware of the recommendations on packet sizes and
> the use of fragmentation as discussed in Section 5 of [RFC8200].
>
> ...so we should start from 1280 (IPV6_MIN_MTU) bytes here, subtract the
> size of an IPv6 header (40), of a UDP header (8), and of the message
> header (sizeof(struct msg_hdr), 4).
>
> I think we should really use defined constants and sizeof() otherwise
> it gets confusing quite fast.
>
> > " * @dlen: UDP payload length (not including UDP header)"
>
> I guess we could have it as a define (OPT_MAX_SIZE?), no need to have
> it in any struct.
>
> By the way, note that we have MAX() defined in util.h, so you don't
> need a condition and a separate statement, you can just do:
I am going to create a new OPT_MAX_SIZE with the computation and also a new
IPV6_MIN_MTU to be part of it and use that later on with MIN.
>
> > > > + fqdn_len = 508 - (offset + opt_hdr_len + 1);
>
> fqdn_len = MAX(fqdn_len, offset + ...);
>
> --
> Stefano
>
--
Quique Llorente
CNV networking Senior Software Engineer
Red Hat EMEA
ellorent@redhat.com
@RedHat Red Hat Red Hat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops
2024-12-19 11:51 ` Enrique Llorente Pastora
@ 2024-12-19 12:06 ` Stefano Brivio
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Brivio @ 2024-12-19 12:06 UTC (permalink / raw)
To: Enrique Llorente Pastora; +Cc: passt-dev
On Thu, 19 Dec 2024 12:51:27 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:
> On Thu, Dec 19, 2024 at 1:19 AM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> > I guess we could have it as a define (OPT_MAX_SIZE?), no need to have
> > it in any struct.
> >
> > By the way, note that we have MAX() defined in util.h, so you don't
> > need a condition and a separate statement, you can just do:
>
> I am going to create a new OPT_MAX_SIZE with the computation and also a new
> IPV6_MIN_MTU to be part of it and use that later on with MIN.
Sorry, MIN, of course, not MAX!
--
Stefano
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-19 14:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-17 6:50 [PATCH v4] dhcp, dhcpv6: Add hostname and client fqdn ops Enrique Llorente
2024-12-17 23:34 ` Stefano Brivio
2024-12-18 11:57 ` Enrique Llorente Pastora
2024-12-19 0:19 ` Stefano Brivio
2024-12-19 11:51 ` Enrique Llorente Pastora
2024-12-19 12:06 ` 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).