From 54ccb747cd7d83b8af92841abbb1e59f32fd2779 Mon Sep 17 00:00:00 2001
From: Franco Fichtner <franco@opnsense.org>
Date: Fri, 30 Aug 2024 10:29:40 +0200
Subject: [PATCH] system: handle stale "pfsyncinterfaces" and improve workflow

PR: https://forum.opnsense.org/index.php?topic=42549.0
---
 plist                                         |  1 +
 src/etc/inc/interfaces.inc                    | 23 ++++++--
 .../OPNsense/Core/forms/hasyncSettings.xml    | 10 +---
 .../mvc/app/models/OPNsense/Core/Hasync.xml   |  9 +--
 .../OPNsense/Core/Migrations/MHA1_0_0.php     |  7 +++
 .../OPNsense/Core/Migrations/MHA1_0_1.php     | 59 +++++++++++++++++++
 6 files changed, 88 insertions(+), 21 deletions(-)
 create mode 100644 src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_1.php

diff --git a/plist b/plist
index 483544bf2..d7409ac5f 100644
--- a/plist
+++ b/plist
@@ -638,6 +638,7 @@
 /usr/local/opnsense/mvc/app/models/OPNsense/Core/Migrations/M1_0_0.php
 /usr/local/opnsense/mvc/app/models/OPNsense/Core/Migrations/M1_0_1.php
 /usr/local/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_0.php
+/usr/local/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_1.php
 /usr/local/opnsense/mvc/app/models/OPNsense/Core/repositories/opnsense.xml
 /usr/local/opnsense/mvc/app/models/OPNsense/Cron/ACL/ACL.xml
 /usr/local/opnsense/mvc/app/models/OPNsense/Cron/Cron.php
diff --git a/src/etc/inc/interfaces.inc b/src/etc/inc/interfaces.inc
index 23cd6cf41..7da4031d3 100644
--- a/src/etc/inc/interfaces.inc
+++ b/src/etc/inc/interfaces.inc
@@ -1379,10 +1379,21 @@ function interfaces_pfsync_configure()
     global $config;
 
     if (!empty($config['hasync']['pfsyncinterface'])) {
-        $carp_sync_int = get_real_interface($config['hasync']['pfsyncinterface']);
+        /*
+         * We are just checking the actual attached interface here as get_real_interface()
+         * was not dependable when the selected interface does not exist for any reason.
+         *
+         * What the current method tells us is that we are going to ignore whether this
+         * interface is currently enabled or not.  To avoid breakage we will keep it so
+         * although in reality disabling your pfsync interface should cause it to stop
+         * syncing.
+         */
+        if (!empty($config['interfaces'][$config['hasync']['pfsyncinterface']]['if'])) {
+            $syncdev = $config['interfaces'][$config['hasync']['pfsyncinterface']]['if'];
+        }
     }
 
-    if (!empty($carp_sync_int) && !empty($config['hasync']['pfsyncenabled'])) {
+    if (!empty($syncdev)) {
         if (!empty($config['hasync']['pfsyncpeerip']) && is_ipaddrv4($config['hasync']['pfsyncpeerip'])) {
             $syncpeer = "syncpeer " . escapeshellarg($config['hasync']['pfsyncpeerip']);
         } else {
@@ -1394,12 +1405,12 @@ function interfaces_pfsync_configure()
             $version = 'version ' . escapeshellarg($config['hasync']['pfsyncversion']);
         }
 
-        $intf_stats = legacy_interfaces_details();
+        $intf_stats = legacy_interfaces_details(); /* XXX could require passing this down */
 
-        mwexec("/sbin/ifconfig pfsync0 syncdev {$carp_sync_int} {$syncpeer} {$version} up");
+        mwexec("/sbin/ifconfig pfsync0 syncdev {$syncdev} {$syncpeer} {$version} up");
 
-        if (!empty($intf_stats[$carp_sync_int]['mtu'])) {
-            mwexec("/sbin/ifconfig pfsync0 mtu " . escapeshellarg($intf_stats[$carp_sync_int]['mtu']));
+        if (!empty($intf_stats[$syncdev]['mtu'])) {
+            mwexecf('/sbin/ifconfig pfsync0 mtu %s', [$intf_stats[$syncdev]['mtu']]);
         }
     } else {
         mwexec('/sbin/ifconfig pfsync0 -syncdev -syncpeer down');
diff --git a/src/opnsense/mvc/app/controllers/OPNsense/Core/forms/hasyncSettings.xml b/src/opnsense/mvc/app/controllers/OPNsense/Core/forms/hasyncSettings.xml
index b4d0c8e4c..5cbb2463a 100644
--- a/src/opnsense/mvc/app/controllers/OPNsense/Core/forms/hasyncSettings.xml
+++ b/src/opnsense/mvc/app/controllers/OPNsense/Core/forms/hasyncSettings.xml
@@ -15,17 +15,11 @@
         <type>checkbox</type>
         <help>When this device is configured as CARP backup it will disconnect all PPP type interfaces and try to reconnect them when becoming master again.</help>
     </field>
-    <field>
-        <id>hasync.pfsyncenabled</id>
-        <label>Synchronize States</label>
-        <type>checkbox</type>
-        <help>pfsync transfers state insertion, update, and deletion messages between firewalls.</help>
-    </field>
     <field>
         <id>hasync.pfsyncinterface</id>
-        <label>Synchronize Interface</label>
+        <label>Synchronize all states via</label>
         <type>dropdown</type>
-        <help>If Synchronize States is enabled, it will utilize this interface for communication. Best choose a dedicated interface for this type of communication to prevent manipulation of states causing security issues.</help>
+        <help>This enables state insertion, update, and deletion messages between firewalls by utilizing the selected interface for communication. Best choose a dedicated interface for this type of communication to prevent manipulation of states causing security issues.</help>
     </field>
     <field>
         <id>hasync.pfsyncversion</id>
diff --git a/src/opnsense/mvc/app/models/OPNsense/Core/Hasync.xml b/src/opnsense/mvc/app/models/OPNsense/Core/Hasync.xml
index 537bb9fb8..0fd524a83 100644
--- a/src/opnsense/mvc/app/models/OPNsense/Core/Hasync.xml
+++ b/src/opnsense/mvc/app/models/OPNsense/Core/Hasync.xml
@@ -1,7 +1,7 @@
 <model>
     <mount>//hasync</mount>
     <migration_prefix>MHA</migration_prefix>
-    <version>1.0.0</version>
+    <version>1.0.1</version>
     <description>HA sync</description>
     <items>
         <disablepreempt type="BooleanField">
@@ -12,14 +12,9 @@
             <Default>0</Default>
             <Required>Y</Required>
         </disconnectppps>
-        <pfsyncenabled type="BooleanField">
-            <Default>0</Default>
-            <Required>Y</Required>
-        </pfsyncenabled>
         <pfsyncinterface type="InterfaceField">
-            <Required>Y</Required>
-            <Default>lan</Default>
             <AllowDynamic>Y</AllowDynamic>
+            <BlankDesc>Disabled</BlankDesc>
             <filters>
                 <virtual>/^(?!1).*$/</virtual>
             </filters>
diff --git a/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_0.php b/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_0.php
index b8528f8dd..af836bd06 100644
--- a/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_0.php
+++ b/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_0.php
@@ -57,6 +57,13 @@ class MHA1_0_0 extends BaseModelMigration
             if (!empty((string)$model->pfsyncenabled)) {
                 $model->pfsyncversion = '1301'; // on upgrade keep legacy pfsync version
             }
+            if (empty($src->pfsyncenabled)) {
+                /* disabe via pfsyncinterface if not set */
+                $model->pfsyncinterface = null;
+            } else {
+                /* may need to disable if previous value is no longer available */
+                $model->pfsyncinterface->normalizeValue();
+            }
         } else {
             throw new \Exception('Missing (configd) ha options list');
         }
diff --git a/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_1.php b/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_1.php
new file mode 100644
index 000000000..cdbdb04e2
--- /dev/null
+++ b/src/opnsense/mvc/app/models/OPNsense/Core/Migrations/MHA1_0_1.php
@@ -0,0 +1,59 @@
+<?php
+
+/*
+ * Copyright (C) 2024 Deciso B.V.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY
+ * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
+ * OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+namespace OPNsense\Core\Migrations;
+
+use OPNsense\Base\BaseModelMigration;
+use OPNsense\Core\Hasync;
+use OPNsense\Core\Backend;
+use OPNsense\Core\Config;
+
+class MHA1_0_1 extends BaseModelMigration
+{
+    /**
+     * Remove pfsyncenabled by folding it into the pfsyncinterface setting
+     * @param $model
+     */
+    public function run($model)
+    {
+        if (!($model instanceof Hasync)) {
+            return;
+        }
+
+        $src = Config::getInstance()->object()->hasync;
+
+        /* duplicated effort from 1.0.0 since that was functional on early 24.7.x */
+        if (empty($src->pfsyncenabled)) {
+            /* disabe via pfsyncinterface if not set */
+            $model->pfsyncinterface = null;
+        } else {
+            /* may need to disable if previous value is no longer available */
+            $model->pfsyncinterface->normalizeValue();
+        }
+    }
+}