Merge pull request #5264 from rnavarro/fix/vti-updown-db-flock-race

ipsec: T8975: lock vti-up-down state DB to prevent lost updates under concurrent rekey

4 files changed, 298 insertions, 39 deletions

diff --git a/python/vyos/ifconfig/vti.py b/python/vyos/ifconfig/vti.py
index 030aa1ed7..01d724593 100644
--- a/python/vyos/ifconfig/vti.py
+++ b/python/vyos/ifconfig/vti.py
@@ -15,7 +15,7 @@
 
 from vyos.ifconfig.interface import Interface
 from vyos.utils.dict import dict_search
-from vyos.utils.vti_updown_db import vti_updown_db_exists, open_vti_updown_db_readonly
+from vyos.utils.vti_updown_db import open_vti_updown_db_readonly
 
 @Interface.register
 class VTIIf(Interface):
@@ -64,14 +64,14 @@ class VTIIf(Interface):
         """
         Set interface administrative state to be 'up' or 'down'.
 
-        The interface will only be brought 'up' if ith is attached to an
+        The interface will only be brought 'up' if it is attached to an
         active ipsec site-to-site connection or remote access connection.
         """
         if state == 'down' or self.bypass_vti_updown_db:
             super().set_admin_state(state)
-        elif vti_updown_db_exists():
+        else:
             with open_vti_updown_db_readonly() as db:
-                if db.wantsInterfaceUp(self.ifname):
+                if db is not None and db.wantsInterfaceUp(self.ifname):
                     super().set_admin_state(state)
 
     def get_mac(self):
diff --git a/python/vyos/utils/vti_updown_db.py b/python/vyos/utils/vti_updown_db.py
index 2931df86c..390db5195 100644
--- a/python/vyos/utils/vti_updown_db.py
+++ b/python/vyos/utils/vti_updown_db.py
@@ -18,55 +18,67 @@ import os
 from contextlib import contextmanager
 from syslog import syslog
 
+from vyos.utils.locking import Lock
+
 VTI_WANT_UP_IFLIST = '/tmp/ipsec_vti_interfaces'
+VTI_UPDOWN_LOCK_NAME = 'ipsec_vti_updown'
 
 def vti_updown_db_exists():
     """ Returns true if the database exists """
     return os.path.exists(VTI_WANT_UP_IFLIST)
 
 @contextmanager
-def open_vti_updown_db_for_create_or_update():
-    """ Opens the database for reading and writing, creating the database if it does not exist """
-    if vti_updown_db_exists():
-        f = open(VTI_WANT_UP_IFLIST, 'r+')
-    else:
-        f = open(VTI_WANT_UP_IFLIST, 'x+')
+def _vti_updown_db_lock():
+    """Serialise access to the VTI up/down DB across the concurrent updown-hook
+    invocations (one per VTI) that strongSwan fires during a coordinated rekey,
+    which would otherwise lost-update the shared state file."""
+    lock = Lock(VTI_UPDOWN_LOCK_NAME)
+    lock.acquire()  # timeout=0 -> block until acquired
     try:
-        db = VTIUpDownDB(f)
-        yield db
+        yield
     finally:
-        f.close()
+        lock.release()
+
+
+@contextmanager
+def open_vti_updown_db_for_create_or_update():
+    """ Opens the database for reading and writing, creating the database if it does not exist """
+    with _vti_updown_db_lock():
+        mode = 'r+' if vti_updown_db_exists() else 'x+'
+        with open(VTI_WANT_UP_IFLIST, mode) as f:
+            yield VTIUpDownDB(f)
 
 @contextmanager
 def open_vti_updown_db_for_update():
     """ Opens the database for reading and writing, returning an error if it does not exist """
-    f = open(VTI_WANT_UP_IFLIST, 'r+')
-    try:
-        db = VTIUpDownDB(f)
-        yield db
-    finally:
-        f.close()
+    with _vti_updown_db_lock():
+        with open(VTI_WANT_UP_IFLIST, 'r+') as f:
+            yield VTIUpDownDB(f)
 
 @contextmanager
 def open_vti_updown_db_readonly():
-    """ Opens the database for reading, returning an error if it does not exist """
-    f = open(VTI_WANT_UP_IFLIST, 'r')
-    try:
-        db = VTIUpDownDB(f)
-        yield db
-    finally:
-        f.close()
+    """Opens the database for reading. Yields None if the database does not exist."""
+    with _vti_updown_db_lock():
+        if not vti_updown_db_exists():
+            yield None
+            return
+        with open(VTI_WANT_UP_IFLIST, 'r') as f:
+            yield VTIUpDownDB(f)
 
 def remove_vti_updown_db():
-    """ Brings down any interfaces referenced by the database and removes the database """
-    # We need to process the DB first to bring down any interfaces still up
-    with open_vti_updown_db_for_update() as db:
-        db.removeAllOtherInterfaces([])
-        # this usage of commit will only ever bring down interfaces,
-        # do not need to provide a functional interface dict supplier
-        db.commit(lambda _: None)
-
-    os.unlink(VTI_WANT_UP_IFLIST)
+    """Brings down any interfaces referenced by the database and removes the database, if it exists."""
+    with _vti_updown_db_lock():
+        if not vti_updown_db_exists():
+            return
+        # We hold the lock already; open the file directly rather than via the
+        # locking context manager to avoid re-acquiring (which would deadlock).
+        with open(VTI_WANT_UP_IFLIST, 'r+') as f:
+            db = VTIUpDownDB(f)
+            db.removeAllOtherInterfaces([])
+            # this usage of commit will only ever bring down interfaces,
+            # do not need to provide a functional interface dict supplier
+            db.commit(lambda _: None)
+        os.unlink(VTI_WANT_UP_IFLIST)
 
 class VTIUpDownDB:
     # The VTI Up-Down DB is a text-based database of space-separated "ifspecs".
diff --git a/src/conf_mode/vpn_ipsec.py b/src/conf_mode/vpn_ipsec.py
index 78d5cbe83..be8eeca13 100755
--- a/src/conf_mode/vpn_ipsec.py
+++ b/src/conf_mode/vpn_ipsec.py
@@ -50,7 +50,6 @@ from vyos.utils.network import interface_exists
 from vyos.utils.dict import dict_search
 from vyos.utils.dict import dict_search_args
 from vyos.utils.process import call
-from vyos.utils.vti_updown_db import vti_updown_db_exists
 from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
 from vyos.utils.vti_updown_db import remove_vti_updown_db
 from vyos import ConfigError
@@ -900,8 +899,7 @@ def apply(ipsec):
     systemd_service = 'strongswan.service'
     if not ipsec or 'deleted' in ipsec:
         call(f'systemctl stop {systemd_service}')
-        if vti_updown_db_exists():
-            remove_vti_updown_db()
+        remove_vti_updown_db()
     else:
         call(f'systemctl reload-or-restart {systemd_service}')
         if ipsec['enabled_vti_interfaces']:
@@ -909,7 +907,7 @@ def apply(ipsec):
                 db.removeAllOtherInterfaces(ipsec['enabled_vti_interfaces'])
                 db.setPersistentInterfaces(ipsec['persistent_vti_interfaces'])
                 db.commit(lambda interface: ipsec['vti_interface_dicts'][interface])
-        elif vti_updown_db_exists():
+        else:
             remove_vti_updown_db()
     if ipsec:
         if ipsec.get('nhrp_exists', False):
diff --git a/src/tests/test_vti_updown_db.py b/src/tests/test_vti_updown_db.py
new file mode 100644
index 000000000..b246f89eb
--- /dev/null
+++ b/src/tests/test_vti_updown_db.py
@@ -0,0 +1,249 @@
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 2 or later as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import tempfile
+import unittest
+
+from unittest.mock import MagicMock, patch
+
+import vyos.utils.vti_updown_db as _vti_mod
+from vyos.utils.vti_updown_db import VTIUpDownDB
+
+
+class TestVTIUpDownDBLogic(unittest.TestCase):
+    """Exercise the in-memory DB API without calling commit()."""
+
+    def setUp(self):
+        self.tmpfile = tempfile.NamedTemporaryFile(delete=False)
+        self.tmpfile.close()
+        self._path_patcher = patch.object(
+            _vti_mod, 'VTI_WANT_UP_IFLIST', self.tmpfile.name
+        )
+        self._path_patcher.start()
+
+    def tearDown(self):
+        self._path_patcher.stop()
+        if os.path.exists(self.tmpfile.name):
+            os.unlink(self.tmpfile.name)
+
+    def test_add_makes_interface_wanted_up(self):
+        """add() marks the interface as wanted-up."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_for_create_or_update() as db:
+                db.add('vti0', 'conn-a', 'IKEv2')
+                self.assertTrue(db.wantsInterfaceUp('vti0'))
+
+    def test_remove_clears_interface(self):
+        """remove() clears the entry; wantsInterfaceUp returns False when no entries remain."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_for_create_or_update() as db:
+                db.add('vti0', 'conn-a', 'IKEv2')
+                db.remove('vti0', 'conn-a', 'IKEv2')
+                self.assertFalse(db.wantsInterfaceUp('vti0'))
+
+    def test_multiple_connections_same_interface(self):
+        """Interface stays wanted-up until all its connection entries are removed."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_for_create_or_update() as db:
+                db.add('vti0', 'conn-a', 'IKEv2')
+                db.add('vti0', 'conn-b', 'IKEv2')
+                db.remove('vti0', 'conn-a', 'IKEv2')
+                # conn-b still present
+                self.assertTrue(db.wantsInterfaceUp('vti0'))
+                db.remove('vti0', 'conn-b', 'IKEv2')
+                self.assertFalse(db.wantsInterfaceUp('vti0'))
+
+    def test_remove_all_other_interfaces(self):
+        """removeAllOtherInterfaces() drops entries for interfaces not in the keep list."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_for_create_or_update() as db:
+                db.add('vti0', 'conn-a', 'IKEv2')
+                db.add('vti1', 'conn-b', 'IKEv2')
+                db.removeAllOtherInterfaces(['vti0'])
+                self.assertTrue(db.wantsInterfaceUp('vti0'))
+                self.assertFalse(db.wantsInterfaceUp('vti1'))
+
+    def test_set_persistent_interfaces(self):
+        """setPersistentInterfaces() adds bare-name (persistent) entries."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_for_create_or_update() as db:
+                db.setPersistentInterfaces(['vti0', 'vti1'])
+                # Persistent entries have no colon in the ifspec.
+                persistent = {s for s in db._ifspecs if ':' not in s}
+                self.assertEqual(persistent, {'vti0', 'vti1'})
+                self.assertTrue(db.wantsInterfaceUp('vti0'))
+                self.assertTrue(db.wantsInterfaceUp('vti1'))
+
+    def test_seed_from_file(self):
+        """VTIUpDownDB reads pre-existing entries from the file on open."""
+        with open(self.tmpfile.name, 'w') as f:
+            f.write('vti0:conn-a:IKEv2 vti1:conn-b:IKEv2')
+
+        from vyos.utils.vti_updown_db import open_vti_updown_db_readonly
+
+        with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+            with open_vti_updown_db_readonly() as db:
+                self.assertTrue(db.wantsInterfaceUp('vti0'))
+                self.assertTrue(db.wantsInterfaceUp('vti1'))
+
+    def test_readonly_yields_none_when_db_absent(self):
+        """open_vti_updown_db_readonly() yields None (no raise) when the DB file is absent."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_readonly
+
+        # Point at a path that does not exist.
+        missing = os.path.join(tempfile.gettempdir(), 'nonexistent_vti_updown_db_test')
+        if os.path.exists(missing):
+            os.unlink(missing)
+        with patch.object(_vti_mod, 'VTI_WANT_UP_IFLIST', missing):
+            with patch('vyos.utils.vti_updown_db.Lock', MagicMock()):
+                with open_vti_updown_db_readonly() as db:
+                    self.assertIsNone(db)
+
+
+class TestVTIUpDownDBLockWiring(unittest.TestCase):
+    """Verify Lock is acquired before yielding and released on context exit."""
+
+    def setUp(self):
+        self.tmpfile = tempfile.NamedTemporaryFile(delete=False)
+        self.tmpfile.close()
+        self._path_patcher = patch.object(
+            _vti_mod, 'VTI_WANT_UP_IFLIST', self.tmpfile.name
+        )
+        self._path_patcher.start()
+
+    def tearDown(self):
+        self._path_patcher.stop()
+        if os.path.exists(self.tmpfile.name):
+            os.unlink(self.tmpfile.name)
+
+    def _make_lock_mock(self):
+        lock_instance = MagicMock()
+        lock_cls = MagicMock(return_value=lock_instance)
+        return lock_cls, lock_instance
+
+    def test_create_or_update_acquires_and_releases(self):
+        """open_vti_updown_db_for_create_or_update() acquires the lock before yielding and releases it on exit."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_create_or_update
+
+        lock_cls, lock_inst = self._make_lock_mock()
+        with patch('vyos.utils.vti_updown_db.Lock', lock_cls):
+            with open_vti_updown_db_for_create_or_update() as _db:
+                lock_inst.acquire.assert_called_once()
+                lock_inst.release.assert_not_called()
+            lock_inst.release.assert_called_once()
+
+    def test_update_acquires_and_releases(self):
+        """open_vti_updown_db_for_update() acquires the lock before yielding and releases it on exit."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_for_update
+
+        lock_cls, lock_inst = self._make_lock_mock()
+        with patch('vyos.utils.vti_updown_db.Lock', lock_cls):
+            with open_vti_updown_db_for_update() as _db:
+                lock_inst.acquire.assert_called_once()
+                lock_inst.release.assert_not_called()
+            lock_inst.release.assert_called_once()
+
+    def test_readonly_acquires_and_releases(self):
+        """open_vti_updown_db_readonly() acquires the lock before yielding and releases it on exit."""
+        from vyos.utils.vti_updown_db import open_vti_updown_db_readonly
+
+        lock_cls, lock_inst = self._make_lock_mock()
+        with patch('vyos.utils.vti_updown_db.Lock', lock_cls):
+            with open_vti_updown_db_readonly() as _db:
+                lock_inst.acquire.assert_called_once()
+                lock_inst.release.assert_not_called()
+            lock_inst.release.assert_called_once()
+
+
+class TestRemoveVTIUpDownDBLockOrdering(unittest.TestCase):
+    """remove_vti_updown_db() must hold the lock across os.unlink."""
+
+    def setUp(self):
+        self.tmpfile = tempfile.NamedTemporaryFile(delete=False)
+        self.tmpfile.close()
+        self._path_patcher = patch.object(
+            _vti_mod, 'VTI_WANT_UP_IFLIST', self.tmpfile.name
+        )
+        self._path_patcher.start()
+
+    def tearDown(self):
+        self._path_patcher.stop()
+        # File may have been unlinked by the function under test.
+        if os.path.exists(self.tmpfile.name):
+            os.unlink(self.tmpfile.name)
+
+    def test_lock_held_across_unlink(self):
+        """os.unlink fires before lock.release(), proving the lock covers the unlink."""
+        from vyos.utils.vti_updown_db import remove_vti_updown_db
+
+        lock_inst = MagicMock()
+        lock_cls = MagicMock(return_value=lock_inst)
+
+        # Record lock.release call_count at the moment os.unlink fires.
+        release_count_at_unlink = []
+
+        def _observe_unlink(path):
+            release_count_at_unlink.append(lock_inst.release.call_count)
+
+        with patch('vyos.utils.vti_updown_db.Lock', lock_cls):
+            with patch.object(VTIUpDownDB, 'commit'):
+                with patch.object(_vti_mod.os, 'unlink', side_effect=_observe_unlink):
+                    remove_vti_updown_db()
+
+        lock_inst.acquire.assert_called_once()
+        lock_inst.release.assert_called_once()
+        self.assertTrue(release_count_at_unlink, 'os.unlink was never called')
+        self.assertEqual(
+            release_count_at_unlink[0],
+            0,
+            'lock.release() was called before os.unlink -- unlink is not protected by the lock',
+        )
+
+    def test_remove_noop_when_db_absent(self):
+        """remove_vti_updown_db() returns without raising and does not unlink when the DB is absent."""
+        from vyos.utils.vti_updown_db import remove_vti_updown_db
+
+        # No DB file present.
+        os.unlink(self.tmpfile.name)
+        self.assertFalse(os.path.exists(self.tmpfile.name))
+
+        lock_inst = MagicMock()
+        lock_cls = MagicMock(return_value=lock_inst)
+
+        with patch('vyos.utils.vti_updown_db.Lock', lock_cls):
+            with patch.object(
+                _vti_mod.os,
+                'unlink',
+                side_effect=AssertionError('os.unlink must not be called'),
+            ) as unlink_mock:
+                remove_vti_updown_db()
+                unlink_mock.assert_not_called()
+
+        lock_inst.acquire.assert_called_once()
+        lock_inst.release.assert_called_once()
+
+
+if __name__ == '__main__':
+    unittest.main()