From nobody Thu Sep 19 00:54:39 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1715969055815950.3069380817454; Fri, 17 May 2024 11:04:15 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 7B4481D3D; Fri, 17 May 2024 14:04:14 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id 33C281E44; Fri, 17 May 2024 13:32:07 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 062BA1780; Fri, 17 May 2024 13:30:23 -0400 (EDT) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 7417B1A19 for ; Fri, 17 May 2024 13:30:13 -0400 (EDT) Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-56-Zz_j9l0JOreoeDNcDtSYQw-1; Fri, 17 May 2024 13:30:11 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DA92B8008A4 for ; Fri, 17 May 2024 17:30:10 +0000 (UTC) Received: from vhost3.router.laine.org (unknown [10.22.16.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id C311C40C6CB4 for ; Fri, 17 May 2024 17:30:10 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1715967013; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QzsfNk1O1YkvTDtfXQqtJ6q1f3pTRoEFy4SOL9jnSB4=; b=Pzfr8zeB5u8gXDQd5SM/PZE+bEnpWx8fZ98rFnol38QUgwkxqtYNNyABStZxQ3RmFjgGDr tGPwjdyjcMDWEonUIm/Ms4S6adwtBIzwgHdM/d9jpy25QVvJhrklT+jjTR3YV1XdRkWCIs OA91niU77CJquZJ6R3KEfQHL0MGuaBo= X-MC-Unique: Zz_j9l0JOreoeDNcDtSYQw-1 From: Laine Stump To: devel@lists.libvirt.org Subject: [PATCH v5 21/30] network: use previously saved list of firewall removal commands Date: Fri, 17 May 2024 13:29:58 -0400 Message-ID: <20240517173007.8125-22-laine@redhat.com> In-Reply-To: <20240517173007.8125-1-laine@redhat.com> References: <20240517173007.8125-1-laine@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Message-ID-Hash: XMGKLFWSZ4U37REAJ3DM53UHAS423RWQ X-Message-ID-Hash: XMGKLFWSZ4U37REAJ3DM53UHAS423RWQ X-MailFrom: laine@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; suspicious-header X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1715969056400100001 Content-Type: text/plain; charset="utf-8" When destroying a network, the network driver has always assumed that it knew what firewall rules had been added as the network was started. This was usually correct - I only recall one time in the past that the firewall rules added by libvirt were changed. But if the exact rules used for a network *were* ever changed from one build/version of libvirt to another, then we would end up attempting to remove rules that hadn't been added, and could possibly *not* remove rules that had been added. The solution to this to not make such brash assumptions about the past, but instead to save (in the network status object at network start time) a list of all the rules needed to remove the rules that were added for the network, and then use that saved list during network destroy to remove exactly what was previous added. Beyond making net-destroy more precise, there are other benefits: 1) We can change the details of the rules we add for networks from one build/release of libvirt to another and painlessly upgrade. 2) The user can switch from one firewall backend to another by simply changing the setting in network.conf and restarting libvirtd/virtnetworkd. In both cases, the restarted libvirtd/virtnetworkd will remove all the rules that had been previously added (based on the network status), and then add new rules (saving the new removal commands back into the network status) Signed-off-by: Laine Stump Reviewed-by: Daniel P. Berrang=C3=A9 --- src/network/bridge_driver.c | 35 ++++++++++++++-------- src/network/bridge_driver_linux.c | 26 +++++++++++++---- src/network/bridge_driver_nop.c | 6 ++-- src/network/bridge_driver_platform.h | 6 ++-- src/network/network_iptables.c | 43 +++++++++++++++++++++++++--- src/network/network_iptables.h | 2 +- tests/networkxml2firewalltest.c | 2 +- 7 files changed, 91 insertions(+), 29 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 82e1052978..1b4c5aedf2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1685,6 +1685,7 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj, g_autoptr(virNetworkDriverConfig) cfg =3D virNetworkDriverGetConfig(ne= tworkGetDriver()); VIR_LOCK_GUARD lock =3D virObjectLockGuard(obj); virNetworkDef *def =3D virNetworkObjGetDef(obj); + virFirewall *fwRemoval =3D NULL; =20 if (virNetworkObjIsActive(obj)) { switch ((virNetworkForwardType) def->forward.type) { @@ -1696,8 +1697,9 @@ networkReloadFirewallRulesHelper(virNetworkObj *obj, * network type, forward=3D'open', doesn't need this because it * has no iptables rules. */ - networkRemoveFirewallRules(def, cfg->firewallBackend); - ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= )); + networkRemoveFirewallRules(obj); + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= , &fwRemoval)); + virNetworkObjSetFwRemoval(obj, fwRemoval); break; =20 case VIR_NETWORK_FORWARD_OPEN: @@ -1904,6 +1906,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, bool devOnline =3D false; bool firewalRulesAdded =3D false; virSocketAddr *dnsServer =3D NULL; + virFirewall *fwRemoval =3D NULL; =20 /* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(def) < 0) @@ -1949,9 +1952,11 @@ networkStartNetworkVirtual(virNetworkDriverState *dr= iver, =20 /* Add "once per network" rules */ if (def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN && - networkAddFirewallRules(def, cfg->firewallBackend) < 0) + networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0= ) { goto error; + } =20 + virNetworkObjSetFwRemoval(obj, fwRemoval); firewalRulesAdded =3D true; =20 for (i =3D 0; (ipdef =3D virNetworkDefGetIPByIndex(def, AF_UNSPEC, i))= ; i++) { @@ -2067,7 +2072,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, =20 if (firewalRulesAdded && def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN) - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); =20 virNetworkObjUnrefMacMap(obj); =20 @@ -2079,8 +2084,7 @@ networkStartNetworkVirtual(virNetworkDriverState *dri= ver, =20 =20 static int -networkShutdownNetworkVirtual(virNetworkObj *obj, - virNetworkDriverConfig *cfg) +networkShutdownNetworkVirtual(virNetworkObj *obj) { virNetworkDef *def =3D virNetworkObjGetDef(obj); pid_t dnsmasqPid; @@ -2106,7 +2110,7 @@ networkShutdownNetworkVirtual(virNetworkObj *obj, ignore_value(virNetDevSetOnline(def->bridge, false)); =20 if (def->forward.type !=3D VIR_NETWORK_FORWARD_OPEN) - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); =20 ignore_value(virNetDevBridgeDelete(def->bridge)); =20 @@ -2410,7 +2414,7 @@ networkShutdownNetwork(virNetworkDriverState *driver, case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_OPEN: - ret =3D networkShutdownNetworkVirtual(obj, cfg); + ret =3D networkShutdownNetworkVirtual(obj); break; =20 case VIR_NETWORK_FORWARD_BRIDGE: @@ -3215,6 +3219,8 @@ networkUpdate(virNetworkPtr net, virNetworkIPDef *ipdef; bool oldDhcpActive =3D false; bool needFirewallRefresh =3D false; + virFirewall *fwRemoval =3D NULL; + =20 =20 virCheckFlags(VIR_NETWORK_UPDATE_AFFECT_LIVE | @@ -3261,7 +3267,7 @@ networkUpdate(virNetworkPtr net, * old rules (and remember to load new ones after the * update). */ - networkRemoveFirewallRules(def, cfg->firewallBackend); + networkRemoveFirewallRules(obj); needFirewallRefresh =3D true; break; default: @@ -3288,16 +3294,21 @@ networkUpdate(virNetworkPtr net, if (virNetworkObjUpdate(obj, command, section, parentIndex, xml, network_driver->xmlopt, flags) < 0) { - if (needFirewallRefresh) - ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= )); + if (needFirewallRefresh) { + ignore_value(networkAddFirewallRules(def, cfg->firewallBackend= , &fwRemoval)); + virNetworkObjSetFwRemoval(obj, fwRemoval); + } goto cleanup; } =20 /* @def is replaced */ def =3D virNetworkObjGetDef(obj); =20 - if (needFirewallRefresh && networkAddFirewallRules(def, cfg->firewallB= ackend) < 0) + if (needFirewallRefresh && + networkAddFirewallRules(def, cfg->firewallBackend, &fwRemoval) < 0= ) { goto cleanup; + } + virNetworkObjSetFwRemoval(obj, fwRemoval); =20 if (flags & VIR_NETWORK_UPDATE_AFFECT_CONFIG) { /* save updated persistent config to disk */ diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_= linux.c index 988362abef..b488600989 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -325,7 +325,8 @@ int networkCheckRouteCollision(virNetworkDef *def) =20 int networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend) + virFirewallBackend firewallBackend, + virFirewall **fwRemoval) { =20 networkSetupPrivateChains(firewallBackend, false); @@ -411,13 +412,28 @@ networkAddFirewallRules(virNetworkDef *def, } } =20 - return iptablesAddFirewallRules(def); + return iptablesAddFirewallRules(def, fwRemoval); } =20 =20 void -networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend G_GNUC_UNUSE= D) +networkRemoveFirewallRules(virNetworkObj *obj) { - iptablesRemoveFirewallRules(def); + virFirewall *fw; + + if ((fw =3D virNetworkObjGetFwRemoval(obj)) =3D=3D NULL) { + /* No information about firewall rules in the network status, + * so we assume the old iptables-based rules from 10.2.0 and + * earlier. + */ + VIR_DEBUG("No firewall info in network status, assuming old-style = iptables"); + iptablesRemoveFirewallRules(virNetworkObjGetDef(obj)); + return; + } + + /* fwRemoval info was stored in the network status, so use that to + * remove the firewall + */ + VIR_DEBUG("Removing firewall rules with commands saved in network stat= us"); + virFirewallApply(fw); } diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_no= p.c index 7d9a061e50..537b9234f8 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -37,12 +37,12 @@ int networkCheckRouteCollision(virNetworkDef *def G_GNU= C_UNUSED) } =20 int networkAddFirewallRules(virNetworkDef *def G_GNUC_UNUSED, - virFirewallBackend firewallBackend G_GNUC_UNUS= ED) + virFirewallBackend firewallBackend G_GNUC_UNUS= ED, + virFirewall **fwRemoval G_GNUC_UNUSED) { return 0; } =20 -void networkRemoveFirewallRules(virNetworkDef *def G_GNUC_UNUSED, - virFirewallBackend firewallBackend G_GNUC_U= NUSED) +void networkRemoveFirewallRules(virNetworkObj *obj G_GNUC_UNUSED) { } diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driv= er_platform.h index 7443c3129f..cd2e3fa7b5 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -33,7 +33,7 @@ void networkPostReloadFirewallRules(bool startup); int networkCheckRouteCollision(virNetworkDef *def); =20 int networkAddFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend); + virFirewallBackend firewallBackend, + virFirewall **fwRemoval); =20 -void networkRemoveFirewallRules(virNetworkDef *def, - virFirewallBackend firewallBackend); +void networkRemoveFirewallRules(virNetworkObj *obj); diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index 467d43c1e9..f774176b3d 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -1591,9 +1591,19 @@ iptablesRemoveIPSpecificFirewallRules(virFirewall *f= w, } =20 =20 -/* Add all rules for all ip addresses (and general rules) on a network */ +/* iptablesAddFirewallrules: + * + * @def - the network that needs an iptables firewall added + * @fwRemoval - if this is not NULL, it points to a pointer + * that should be filled in with a virFirewall object containing + * all the commands needed to remove this firewall at a later time. + * + * Add all rules for all ip addresses (and general rules) on a + * network, and optionally return a virFirewall object containing all + * the rules needed to later remove the firewall that has been added. +*/ int -iptablesAddFirewallRules(virNetworkDef *def) +iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval) { size_t i; virNetworkIPDef *ipdef; @@ -1614,10 +1624,35 @@ iptablesAddFirewallRules(virNetworkDef *def) VIR_FIREWALL_TRANSACTION_AUTO_ROLLBAC= K)); iptablesAddChecksumFirewallRules(fw, def); =20 - return virFirewallApply(fw); + if (virFirewallApply(fw) < 0) + return -1; + + if (fwRemoval) { + /* caller wants us to create a virFirewall object that can be + * applied to undo everything that was just done by * virFirewallA= pply() + */ + + if (virFirewallNewFromRollback(fw, fwRemoval) < 0) + return -1; + } + + return 0; } =20 -/* Remove all rules for all ip addresses (and general rules) on a network = */ +/* iptablesRemoveFirewallRules: + * + * @def - the network that needs its iptables firewall rules removed + * + * Remove all rules for all ip addresses (and general rules) on a + * network that is being shut down. + * + * This function assumes the set of iptables rules that were added by + * all versions of libvirt prior to 10.4.0; any libvirt of that + * release or newer may or may not have this same set of rules, and + * should be using the list of commands saved in NetworkObj::fwRemoval + * ( element in the network status XML) to remove the + * network's firewall rules. + */ void iptablesRemoveFirewallRules(virNetworkDef *def) { diff --git a/src/network/network_iptables.h b/src/network/network_iptables.h index cdc143f154..d6ffc15bb0 100644 --- a/src/network/network_iptables.h +++ b/src/network/network_iptables.h @@ -23,7 +23,7 @@ #include "virfirewall.h" #include "network_conf.h" =20 -int iptablesAddFirewallRules(virNetworkDef *def); +int iptablesAddFirewallRules(virNetworkDef *def, virFirewall **fwRemoval); =20 void iptablesRemoveFirewallRules(virNetworkDef *def); =20 diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltes= t.c index 3a9f409e2a..082979e5dc 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -98,7 +98,7 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(def =3D virNetworkDefParse(NULL, xml, NULL, false))) return -1; =20 - if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES) < 0) + if (networkAddFirewallRules(def, VIR_FIREWALL_BACKEND_IPTABLES, NULL) = < 0) return -1; =20 actual =3D actualargv =3D virBufferContentAndReset(&buf); --=20 2.45.0