aboutgitcodebugslistschat
path: root/conf.c
Commit message (Collapse)AuthorAgeFilesLines
* conf: Safer parsing of MAC addressesDavid Gibson2024-06-071-17/+36
| | | | | | | | | | | | | | | | | In conf() we parse a MAC address in two places, for the --ns-mac-addr and the -M options. As well as duplicating code, the logic for this parsing has several bugs: * The most serious is that if the given string is shorter than a MAC address should be, we'll access past the end of it. * We don't check the endptr supplied by strtol() which means we could ignore certain erroneous contents * We never check the separator characters between each octet * We ignore certain sorts of garbage that follow the MAC address Correct all these bugs in a new parse_mac() helper. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Don't print usage via the logging subsystemDavid Gibson2024-06-051-160/+166
| | | | | | | | | | | | | | | | | | | | | | The message from usage() when given invalid options, or the -h / --help option is currently printed by many calls to the info() function, also used for runtime logging of informational messages. That isn't useful: the usage message should always go to the terminal (stdout or stderr), never syslog or a logfile. It should never be filtered by priority. Really the only thing using the common logging functions does is give more opportunities for something to go wrong. Replace all the info() calls with direct fprintf() calls. This does mean manually adding "\n" to each message. A little messy, but worth it for the simplicity in other dimensions. While we're there make much heavier use of single strings containing multiple lines of output text. That reduces the number of fprintf calls, reducing visual clutter and making it easier to see what the output will look like from the source. Link: https://bugs.passt.top/show_bug.cgi?id=90 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Remove unhelpful usage() wrapperDavid Gibson2024-06-051-13/+4
| | | | | | | | | usage() does nothing but call print_usage() with EXIT_FAILURE as a parameter. It's no more complex to just give that parameter at the single call site. Eliminate it and rename print_usage() to just usage(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, passt.h: Rename pid_file in struct ctx to pidfileStefano Brivio2024-05-231-4/+4
| | | | | | | We have pidfile_fd now, pid_file_fd would be quite ugly. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
* conf, passt, tap: Open socket and PID files before switching UID/GIDStefano Brivio2024-05-231-1/+16
| | | | | | | | | | | | Otherwise, if the user runs us as root, and gives us paths that are only accessible by root, we'll fail to open them, which might in turn encourage users to change permissions or ownerships: definitely a bad idea in terms of security. Reported-by: Minxi Hou <mhou@redhat.com> Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com>
* conf: Don't lecture user about starting us as rootStefano Brivio2024-05-231-1/+1
| | | | | | | | | | | | | | | libguestfs tools have a good reason to run as root: if the guest image is owned by root, it would be counterproductive to encourage users to invoke them as non-root, as it would require changing permissions or ownership of the image file. And if they run as root, we'll start as root, too. Warn users we'll switch to 'nobody', but don't tell them what to do. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
* conf: Fix clang-tidy warning about using an undefined enum valueDavid Gibson2024-05-131-2/+2
| | | | | | | | | | In conf() we temporarily set the forwarding mode variables to 0 - an invalid value, so that we can check later if they've been set by the intervening logic. clang-tidy 18.1.1 in Fedora 40 now complains about this. Satisfy it by giving an name in the enum to the 0 value. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Don't fail if the template interface doesn't have a MAC addressStefano Brivio2024-04-191-4/+8
| | | | | | | | | | | | | | | | ...simply resort to using locally-administered address (LAA) as host-side source, instead. Pick 02:00:00:00:00:00, to make it clear that we don't actually care about that address, and also to match the 00 (Administratively Assigned Identifier) quadrant of SLAP (RFC 8948). Otherwise, pasta refuses to start if the template is a tun or Wireguard interface. Link: https://bugs.passt.top/show_bug.cgi?id=49 Link: https://github.com/containers/podman/issues/22320 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: We're interested in the MAC address, not in the MAC itselfStefano Brivio2024-04-191-2/+2
| | | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Fix selection of template interfaceDavid Gibson2024-03-201-2/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | Since f919dc7a4b1c ("conf, netlink: Don't require a default route to start"), if there is only one host interface with routes, we will pick that as the template interface, even if there are no default routes for an IP version. Unfortunately this selection had a serious flaw: in some cases it would 'return' in the middle of an nl_foreach() loop, meaning we wouldn't consume all the netlink responses for our query. This could cause later netlink operations to fail as we read leftover responses from the aborted query. Rewrite the interface detection to avoid this problem. While we're there: * Perform detection of both default and non-default routes in a single pass, avoiding an ugly goto * Give more detail on error and working but unusual paths about the situation (no suitable interface, multiple possible candidates, etc.). Fixes: f919dc7a4b1c ("conf, netlink: Don't require a default route to start") Link: https://bugs.passt.top/show_bug.cgi?id=83 Link: https://github.com/containers/podman/issues/22052 Link: https://bugzilla.redhat.com/show_bug.cgi?id=2270257 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Use info(), not warn() for somewhat expected cases where one IP version has no default routes, or no routes at all] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, netlink: Don't require a default route to startStefano Brivio2024-03-181-2/+2
| | | | | | | | | | | | | | | | | | | | There might be isolated testing environments where default routes and global connectivity are not needed, a single interface has all non-loopback addresses and routes, and still passt and pasta are expected to work. In this case, it's pretty obvious what our upstream interface should be, so go ahead and select the only interface with at least one route, disabling DHCP and implying --no-map-gw as the documentation already states. If there are multiple interfaces with routes, though, refuse to start, because at that point it's really not clear what we should do. Reported-by: Martin Pitt <mpitt@redhat.com> Link: https://github.com/containers/podman/issues/21896 Signed-off-by: Stefano brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Warn if we can't advertise any nameserver via DHCP, NDP, or DHCPv6Stefano Brivio2024-03-141-2/+15
| | | | | | | | | | | We might have read from resolv.conf, or from the command line, a resolver that's reachable via loopback address, but that doesn't mean we can offer that via DHCP, NDP or DHCPv6: warn if there are no resolvers we can offer for a given IP version. Suggested-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Handle addresses passed via --dns just like the ones from resolv.confStefano Brivio2024-03-141-6/+6
| | | | | | | | | | | | | | | | | ...that is, call add_dns4() and add_dns6() instead of simply adding those to the list of servers we advertise. Most importantly, this will set the 'dns_host' field for the matching IP version, so that, as mentioned in the man page, servers passed via --dns are used for DNS mapping as well, if used in combination with --dns-forward. Reported-by: Paul Holzinger <pholzing@redhat.com> Link: https://bugs.passt.top/show_bug.cgi?id=82 Fixes: 89678c515755 ("conf, udp: Introduce basic DNS forwarding") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Tested-by: Paul Holzinger <pholzing@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Don't warn if nameservers were found, but won't be advertisedStefano Brivio2024-03-121-3/+8
| | | | | | | | | | | | | | | | | | | | | | | | Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields"), we won't add to c->ip4.dns and c->ip6.dns nameservers that can't be used by the guest or container, and we won't advertise them. However, the fact that we don't advertise any nameserver doesn't mean that we didn't find any, and we should warn only if we couldn't find any. This is particularly relevant in case both --dns-forward and --no-map-gw are passed, and a single loopback address is listed in /etc/resolv.conf: we'll forward queries directed to the address specified by --dns-forward to the loopback address we found, we won't advertise that address, so we shouldn't warn: this is a perfectly legitimate usage. Reported-by: Paul Holzinger <pholzing@redhat.com> Link: https://github.com/containers/podman/issues/19213 Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Tested-by: Paul Holzinger <pholzing@redhat.com>
* util: move IP stuff from util.[ch] to ip.[ch]Laurent Vivier2024-03-061-0/+1
| | | | | | | | | | | | Introduce ip.[ch] file to encapsulate IP protocol handling functions and structures. Modify various files to include the new header ip.h when it's needed. Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Message-ID: <20240303135114.1023026-5-lvivier@redhat.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* fwd: Rename port_fwd.[ch] and their contentsDavid Gibson2024-02-291-4/+4
| | | | | | | | | | | | Currently port_fwd.[ch] contains helpers related to port forwarding, particular automatic port forwarding. We're planning to allow much more flexible sorts of forwarding, including both port translation and NAT based on the flow table. This will subsume the existing port forwarding logic, so rename port_fwd.[ch] to fwd.[ch] with matching updates to all the names within. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: If no interface with a default route was found, say itStefano Brivio2024-02-281-2/+2
| | | | | | | | | | | ...instead of implying that by stating that there's no routable interface for a given IP version. There might be interfaces with non-default routes. Suggested-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Paul Holzinger <pholzing@redhat.com>
* conf: set the log level much earlierPaul Holzinger2024-02-271-0/+10
| | | | | | | | | | | | | --quiet is supposed to silence the "No routable interface" message but it does not work because the log level was set long after conf_ip4/6() was called which means it uses the default level which logs everything. To address this move the log level logic directly after the option parsing in conf(). Signed-off-by: Paul Holzinger <pholzing@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: No routable interface for IPv4 or IPv6 is informational, not a warningStefano Brivio2024-02-161-2/+2
| | | | | | | | | | | | | | | | | | ...Podman users might get confused by the fact that if we can't find a default route for a given IP version, we'll report that as a warning message and possibly just before actual error messages. However, a lack of routable interface for IPv4 or IPv6 can be a normal circumstance: don't warn about it, just state that as informational message, if those are displayed (they're not in non-error paths in Podman, for example). While at it, make it clear that we're disabling IPv4 or IPv6 if there's no routable interface for the corresponding IP version. Reported-by: Paul Holzinger <pholzing@redhat.com> Link: https://github.com/containers/podman/pull/21563#issuecomment-1937024642 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, passt.1: Exit if we can't bind a forwarded port, except for -[tu] allStefano Brivio2024-02-161-25/+11
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | ...or similar, that is, if only excluded ranges are given (implying we'll forward any other available port). In that case, we'll usually forward large sets of ports, and it might be inconvenient for the user to skip excluding single ports that are already taken. The existing behaviour, that is, exiting only if we fail to bind all the ports for one given forwarding option, turns out to be problematic for several aspects raised by Paul: - Podman merges ranges anyway, so we might fail to bind all the ports from a specific range given by the user, but we'll not fail anyway because Podman merges it with another one where we succeed to bind at least one port. At the same time, there should be no semantic difference between multiple ranges given by a single option and multiple ranges given as multiple options: it's unexpected and not documented - the user might actually rely on a given port to be forwarded to a given container or a virtual machine, and if connections are forwarded to an unrelated process, this might raise security concerns - given that we can try and fail to bind multiple ports before exiting (in case we can't bind any), we don't have a specific error code we can return to the user, so we don't give the user helpful indication as to why we couldn't bind ports. Exit as soon as we fail to create or bind a socket for a given forwarded port, and report the actual error. Keep the current behaviour, however, in case the user wants to forward all the (available) ports for a given protocol, or all the ports with excluded ranges only. There, it's more reasonable that the user is expecting partial failures, and it's probably convenient that we continue with the ports we could forward. Update the manual page to reflect the new behaviour, and the old behaviour too in the cases where we keep it. Suggested-by: Paul Holzinger <pholzing@redhat.com> Link: https://github.com/containers/podman/pull/21563#issuecomment-1937024642 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Tested-by: Paul Holzinger <pholzing@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* treewide: Make a bunch of pointer variables pointers to constDavid Gibson2024-01-161-5/+7
| | | | | | | | | | Sufficiently recent cppcheck (I'm using 2.13.0) seems to have added another warning for pointer variables which could be pointer to const but aren't. Use this to make a bunch of variables const pointers where they previously weren't for no particular reason. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* udp: Consistently use -1 to indicate un-opened sockets in mapsDavid Gibson2023-11-071-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | udp uses the udp_tap_map, udp_splice_ns and udp_splice_init tables to keep track of already opened sockets bound to specific ports. We need a way to indicate entries where a socket hasn't been opened, but the code isn't consistent if this is indicated by a 0 or a -1: * udp_splice_sendfrom() and udp_tap_handler() assume that 0 indicates an unopened socket * udp_sock_init() fills in -1 for a failure to open a socket * udp_timer_one() is somewhere in between, treating only strictly positive fds as valid -1 (or, at least, negative) is really the correct choice here, since 0 is a theoretically valid fd value (if very unlikely in practice). Change to use that consistently throughout. The table does need to be initialised to all -1 values before any calls to udp_sock_init() which can happen from conf_ports(). Because C doesn't make it easy to statically initialise non zero values in large tables, this does require a somewhat awkward call to initialise the table from conf(). This is the best approach I could see for the short term, with any luck it will go away at some point when those socket tables are replaced by a unified flow table. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* log: Enable format warningsDavid Gibson2023-11-071-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | logmsg() takes printf like arguments, but because it's not a built in, the compiler won't generate warnings if the format string and parameters don't match. Enable those by using the format attribute. Strictly speaking this is a gcc extension, but I believe it is also supported by some other common compilers. We already use some other attributes in various places. For now, just use it and we can worry about compilers that don't support it if it comes up. This exposes some warnings from existing callers, both in gcc and in clang-tidy: - Some are straight out bugs, which we correct - It's occasionally useful to invoke the logging functions with an empty string, which gcc objects to, so disable that specific warning in the Makefile - Strictly speaking the C standard requires that the parameter for a %p be a (void *), not some other pointer type. That's only likely to cause problems in practice on weird architectures with different sized representations for pointers to different types. Nonetheless add the casts to make it happy. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* port_fwd: Move automatic port forwarding code to port_fwd.[ch]David Gibson2023-11-071-84/+1
| | | | | | | | | | | | | The implementation of scanning /proc files to do automatic port forwarding is a bit awkwardly split between procfs_scan_listen() in util.c, get_bound_ports() and related functions in conf.c and the initial setup code in conf(). Consolidate all of this into port_fwd.h, which already has some related definitions, and a new port_fwd.c. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Cleaner initialisation of default forwarding modesDavid Gibson2023-11-071-33/+27
| | | | | | | | | | | | | | | | | | Initialisation of the forwarding mode variables is complicated a bit by the fact that we have different defaults for passt and pasta modes. This leads to some debateably duplicated code between those two cases. More significantly, however, currently the final setting of the mode variable is interleaved with the code to actually start automatic scanning when that's selected. This essentially mingles "policy" code (setting the default mode), with implementation code (making that happen). That's a bit inflexible if we want to change policies in future. Disentangle these two pieces, and use a slightly different construction to make things briefer as well. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Remove overly cryptic selection of forward tableDavid Gibson2023-10-041-12/+8
| | | | | | | | | | | | | | | | In a couple of places in conf(), we use a local 'fwd' variable to reference one of our forwarding tables. The value depends on which command line option we're currently looking at, and is initialized rather cryptically from an assignment side-effect within the if condition checking that option. Newer versions of cppcheck complain about this assignment being an always true condition, but in any case it's both clearer and slightly shorter to use separate if branches for the two cases and set the forwarding parameter more directly. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Make many pointers constDavid Gibson2023-10-041-2/+3
| | | | | | | | | Newer versions of cppcheck (as of 2.12.0, at least) added a warning for pointers which could be declared to point at const data, but aren't. Based on that, make many pointers throughout the codebase const. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Demote overlapping port ranges error to a warningDavid Gibson2023-08-131-4/+3
| | | | | | | | | | | | | | | | | | | | We give a fatal error if the port ranges from any port forwarding specifiers overlap. This occurs even if those port ranges are specifically bound to different addresses, so there's not really any overlap. Right now, we can't 100% handle this case correctly, because our data structures don't have a way to represent per-address forwarding. However, there are a number of cases that will actually work just fine: e.g. mapping the same port to the same port on two different addresses (say :: and 127.0.0.1). We have long term plans to fix this properly, but that is still some time away. For the time being, demote this error to a warning so that the cases that already work will be allowed. Link: https://bugs.passt.top/show_bug.cgi?id=56 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Propagate errors for "dump" operationsDavid Gibson2023-08-041-14/+51
| | | | | | | | | | | | | | Currently if we receive any netlink errors while discovering network configuration from the host, we'll just ignore it and carry on. This might lead to cryptic error messages later on, or even silent misconfiguration. We now have the mechanisms to detect errors from get/dump netlink operations. Propgate these errors up to the callers and report them usefully. Link: https://bugs.passt.top/show_bug.cgi?id=60 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Explicitly pass netlink sockets to operationsDavid Gibson2023-08-041-7/+8
| | | | | | | | | | | | | | All the netlink operations currently implicitly use one of the two global netlink sockets, sometimes depending on an 'ns' parameter. Change them all to explicitly take the socket to use (or two sockets to use in the case of the *_dup() functions). As well as making these functions strictly more general, it makes the callers easier to follow because we're passing a socket variable with a name rather than an unexplained '0' or '1' for the ns parameter. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Minor formatting changes in pasta_ns_conf()] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Split nl_route() into separate operation functionsDavid Gibson2023-08-041-2/+2
| | | | | | | | | | nl_route() can perform 3 quite different operations based on the 'op' parameter. Split this into separate functions for each one. This requires more lines of code, but makes the internal logic of each operation much easier to follow. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Split nl_addr() into separate operation functionsDavid Gibson2023-08-041-7/+5
| | | | | | | | | | | | | | | nl_addr() can perform three quite different operations based on the 'op' parameter, each of which uses a different subset of the parameters. Split them up into a function for each operation. This does use more lines of code, but the overlap wasn't that great, and the separated logic is much easier to follow. It's also clearer in the callers what we expect the netlink operations to do, and what information it uses. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Minor formatting fixes in pasta_ns_conf()] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Split up functionality of nl_link()David Gibson2023-08-041-2/+2
| | | | | | | | | | | | | | | | | | | | | | | nl_link() performs a number of functions: it can bring links up, set MAC address and MTU and also retrieve the existing MAC. This makes for a small number of lines of code, but high conceptual complexity: it's quite hard to follow what's going on both in nl_link() itself and it's also not very obvious which function its callers are intending to use. Clarify this, by splitting nl_link() into nl_link_up(), nl_link_set_mac(), and nl_link_get_mac(). The first brings up a link, optionally setting the MTU, the others get or set the MAC address. This fixes an arguable bug in pasta_ns_conf(): it looks as though that was intended to retrieve the guest MAC whether or not c->pasta_conf_ns is set. However, it only actually does so in the !c->pasta_conf_ns case: the fact that we set up==1 means we would only ever set, never get, the MAC in the nl_link() call in the other path. We get away with this because the MAC will quickly be discovered once we receive packets on the tap interface. Still, it's neater to always get the MAC address here. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: Make ns_enter() a void function and report setns() errorsDavid Gibson2023-08-041-1/+2
| | | | | | | | | | | | | | | | ns_enter() returns an integer... but it's always zero. If we actually fail the function doesn't return. Therefore it makes more sense for this to be a function returning void, and we can remove the cases where we pointlessly checked its return value. In addition ns_enter() is usually called from an ephemeral thread created by NS_CALL(). That means that the exit(EXIT_FAILURE) there usually won't be reported (since NS_CALL() doesn't wait() for the thread). So, use die() instead to print out some information in the unlikely event that our setns() here does fail. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Correct length checking of interface names in conf_ports()David Gibson2023-06-281-3/+8
| | | | | | | | | | | | | | | | | | | | | | When interface names are specified in forwarding specs, we need to check the length of the given interface name against the limit of IFNAMSIZ - 1 (15) characters. However, we managed to have 3 separate off-by-one errors here meaning we only accepted interface names up to 12 characters. 1. At the point of the check 'ifname' was still on the '%' character, not the first character of the name, meaning we overestimated the length by one 2. At the point of the check 'spec' had been advanced one character past the '/' which terminates the interface name, meaning we overestimated the length by another one 3. We checked if the (miscalculated) length was >= IFNAMSIZ - 1, that is >= 15, whereas lengths equal to 15 should be accepted. Correct all 3 errors. Link: https://bugs.passt.top/show_bug.cgi?id=61 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Fix size checking of -I interface nameDavid Gibson2023-06-281-2/+2
| | | | | | | | | | | | | | | | | | Network interface names must fit in a buffer of IFNAMSIZ bytes, including the terminating \0. IFNAMSIZ is 16 on Linux, so interface names can be up to (and including) 15 characters long. We validate this for the -I option, but we have an off by one error. We pass (IFNAMSIZ - 1) as the buffer size to snprintf(), but that buffer size already includes the terminating \0, so this actually truncates the value to 14 characters. The return value returned from snprintf() however, is the number of characters that would have been printed *excluding* the terminating \0, so by comparing it >= IFNAMSIZ - 1 we are giving an error on names >= 15 characters rather than strictly > 15 characters. Link: https://bugs.passt.top/show_bug.cgi?id=61 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Accept -a and -g without --config-net in pasta modeStefano Brivio2023-06-251-6/+7
| | | | | | | | | | | | | While --no-copy-addrs and --no-copy-routes only make sense with --config-net, and they are implied on -g and -a, respectively, that doesn't mean we should refuse -a or -g without --config-net: they are still relevant for a number of things (including DHCP/DHCPv6/NDP configuration). Reported-by: Gianluca Stivan <me@yawnt.com> Fixes: cc9d16758be6 ("conf, pasta: With --config-net, copy all addresses by default") Fixes: da54641f140e ("conf, pasta: With --config-net, copy all routes by default") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Make -a/--address really imply --no-copy-addrsStefano Brivio2023-06-251-0/+3
| | | | | | | | | | | | | | | | | | | | I wrote it in commit message and man page, but not in conf()... Note that -g/--gateway correctly implies --no-copy-routes already. This fixes Podman's tests: podman networking with pasta(1) - IPv4 address assignment podman networking with pasta(1) - IPv4 default route assignment where we pass -a and -g to assign an address and a default gateway that's compatible with it, but -a doesn't disable the copy of addresses, so we ignore -a, and the default gateway is incompatible with the addresses we copy -- hence no routes in the container. Link: https://github.com/containers/podman/pull/18612 Fixes: cc9d16758be6 ("conf, pasta: With --config-net, copy all addresses by default") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, log: On -h / --help, print usage to stdout, not stderrStefano Brivio2023-06-231-5/+18
| | | | | | | | | | | | | | Erik suggests that this makes it easier to grep for options, and with --help we're anyway printing usage information as expected, not as part of an error report. While at it: on -h, we should exit with 0. Reported-by: Erik Sjölund <erik.sjolund@gmail.com> Link: https://bugs.passt.top/show_bug.cgi?id=52 Link: https://bugs.passt.top/show_bug.cgi?id=53 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Fix erroneous check of ip6->gw2023_06_03.429e1a7David Gibson2023-06-031-1/+1
| | | | | | | | | | | | | | | | | a7359f094898 ("conf: Don't exit if sourced default route has no gateway") was supposed to allow passt/pasta to run even if given a template interface which has no default gateway. However a mistake in the patch means it still requires the gateway, but doesn't require a global address for the guest which we really do need. This is one part (but not the only part) of the problem seen in https://bugs.passt.top/show_bug.cgi?id=50. Reported-by: Justin Jereza <justinjereza@gmail.com> Fixes: a7359f094898 ("conf: Don't exit if sourced default route has no gateway") Link: https://bugs.passt.top/show_bug.cgi?id=50 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, pasta: With --config-net, copy all addresses by defaultStefano Brivio2023-05-231-2/+16
| | | | | | | | | | | | | | | | | | | | | | | | Use the newly-introduced NL_DUP mode for nl_addr() to copy all the addresses associated to the template interface in the outer namespace, unless --no-copy-addrs (also implied by -a) is given. This option is introduced as deprecated right away: it's not expected to be of any use, but it's helpful to keep it around for a while to debug any suspected issue with this change. This is done mostly for consistency with routes. It might partially cover the issue at: https://bugs.passt.top/show_bug.cgi?id=47 Support multiple addresses per address family for some use cases, but not the originally intended one: we'll still use a single outbound address (unless the routing table specifies different preferred source addresses depending on the destination), regardless of the address used in the target namespace. Link: https://bugs.passt.top/show_bug.cgi?id=47 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Add functionality to copy addresses from outer namespaceStefano Brivio2023-05-231-3/+5
| | | | | | | | | | | | | | | | | | | | Similarly to what we've just done with routes, support NL_DUP for addresses (currently not exposed): nl_addr() can optionally copy mulitple addresses to the target namespace, by fixing up data from the dump with appropriate flags and interface index, and repeating it back to the kernel on the socket opened in the target namespace. Link-local addresses are not copied: the family is set to AF_UNSPEC, which means the kernel will ignore them. Same for addresses from a mismatching address (pre-4.19 kernels without support for NETLINK_GET_STRICT_CHK). Ignore IFA_LABEL attributes by changing their type to IFA_UNSPEC, because in general they will report mismatching names, and we don't really need to use labels as we already know the interface index. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Don't exit if sourced default route has no gatewayStefano Brivio2023-05-231-3/+7
| | | | | | | | | | | | | | | | | | | | | | | | If we use a template interface without a gateway on the default route, we can still offer almost complete functionality, except that, of course, we can't map the gateway address to the outer namespace or host, and that we have no obvious server address or identifier for use in DHCP's siaddr and option 54 (Server identifier, mandatory). Continue, if we have a default route but no default gateway, and imply --no-map-gw and --no-dhcp in that case. NDP responder and DHCPv6 should be able to work as usual because we require a link-local address to be present, and we'll fall back to that. Together with the previous commits implementing an actual copy of routes from the outer namespace, this should finally fix the operation of 'pasta --config-net' for cases where we have a default route on the host, but no default gateway, as it's the case for tap-style routes, including typical Wireguard endpoints. Reported-by: me@yawnt.com Link: https://bugs.passt.top/show_bug.cgi?id=49 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Revert "conf: Adjust netmask on mismatch between IPv4 address/netmask and ↵Stefano Brivio2023-05-231-24/+1
| | | | | | | | | | | | gateway" This reverts commit 7656a6f8888237b9e23d63666e921528b6aaf950: now, by default, we copy all the routes associated to the outbound interface into the routing table of the container, so there's no need for this horrible workaround anymore. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf, pasta: With --config-net, copy all routes by defaultStefano Brivio2023-05-231-0/+16
| | | | | | | | | | | | | | | | | | Use the newly-introduced NL_DUP mode for nl_route() to copy all the routes associated to the template interface in the outer namespace, unless --no-copy-routes (also implied by -g) is given. This option is introduced as deprecated right away: it's not expected to be of any use, but it's helpful to keep it around for a while to debug any suspected issue with this change. Otherwise, we can't use default gateways which are not, address-wise, on the same subnet as the container, as reported by Callum. Reported-by: Callum Parsey <callum@neoninteger.au> Link: https://github.com/containers/podman/issues/18539 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: --config-net option is for pasta mode onlyStefano Brivio2023-05-231-1/+7
| | | | | | Reported-by: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Add functionality to copy routes from outer namespaceStefano Brivio2023-05-231-2/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Instead of just fetching the default gateway and configuring a single equivalent route in the target namespace, on 'pasta --config-net', it might be desirable in some cases to copy the whole set of routes corresponding to a given output interface. For instance, in: https://github.com/containers/podman/issues/18539 IPv4 Default Route Does Not Propagate to Pasta Containers on Hetzner VPSes configuring the default gateway won't work without a gateway-less route (specifying the output interface only), because the default gateway is, somewhat dubiously, not on the same subnet as the container. This is a similar case to the one covered by commit 7656a6f88882 ("conf: Adjust netmask on mismatch between IPv4 address/netmask and gateway"), and I'm not exactly proud of that workaround. We also have: https://bugs.passt.top/show_bug.cgi?id=49 pasta does not work with tap-style interface for which, eventually, we should be able to configure a gateway-less route in the target namespace. Introduce different operation modes for nl_route(), including a new NL_DUP one, not exposed yet, which simply parrots back to the kernel the route dump for a given interface from the outer namespace, fixing up flags and interface indices on the way, and requesting to add the same routes in the target namespace, on the interface we manage. For n routes we want to duplicate, send n identical netlink requests including the full dump: routes might depend on each other and the kernel processes RTM_NEWROUTE messages sequentially, not atomically, and repeating the full dump naturally resolves dependencies without the need to actually calculate them. I'm not kidding, it actually works pretty well. Link: https://github.com/containers/podman/issues/18539 Link: https://bugs.passt.top/show_bug.cgi?id=49 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* util, conf: Add and use ns_is_init() helperStefano Brivio2023-05-231-15/+1
| | | | | | | | | | We'll need this in isolate_initial(). While at it, don't rely on BUFSIZ: the earlier issue we had with musl reminded me it's not a magic "everything will fit" value. Size the read buffer to what we actually need from uid_map, and check for the final newline too, because uid_map is organised in lines. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt: Relicense to GPL 2.0, or any later versionStefano Brivio2023-04-061-1/+1
| | | | | | | | | | | | | | | | | | | In practical terms, passt doesn't benefit from the additional protection offered by the AGPL over the GPL, because it's not suitable to be executed over a computer network. Further, restricting the distribution under the version 3 of the GPL wouldn't provide any practical advantage either, as long as the passt codebase is concerned, and might cause unnecessary compatibility dilemmas. Change licensing terms to the GNU General Public License Version 2, or any later version, with written permission from all current and past contributors, namely: myself, David Gibson, Laine Stump, Andrea Bolognani, Paul Holzinger, Richard W.M. Jones, Chris Kuhn, Florian Weimer, Giuseppe Scrivano, Stefan Hajnoczi, and Vasiliy Ulyanov. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Allow binding to ports on an interface without a specific addressStefano Brivio2023-03-291-1/+3
| | | | | | | | | | | | | | | | | Somebody might want to bind listening sockets to a specific interface, but not a specific address, and there isn't really a reason to prevent that. For example: -t %eth0/2022 Alternatively, we support options such as -t 0.0.0.0%eth0/2022 and -t ::%eth0/2022, but not together, for the same port. Enable this kind of syntax and add examples to the man page. Reported-by: Paul Holzinger <pholzing@redhat.com> Link: https://github.com/containers/podman/issues/14425#issuecomment-1485192195 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-07-08 11:13:54 +0000