Improve sing-tun cleanup lifecycle

Signed-off-by: Loren Eteval <loren.eteval@proton.me>

tests/test_sing_tun_linux_host.py

@@ -76,17 +76,20 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         scripts = []
         process = mock.Mock()
         process.wait.return_value = 0
-        process.poll.return_value = 0
+        process.poll.return_value = None
 
         def launch(arguments, **kwargs):
             self.assertEqual(arguments[:2], ['pkexec', 'bash'])
-            self.assertEqual(kwargs['stdin'], subprocess.DEVNULL)
+            self.assertEqual(kwargs['stdin'], subprocess.PIPE)
             self.assertTrue(
                 Path(arguments[2]).name.startswith(tempfile.gettempprefix())
             )
             self.assertEqual(Path(arguments[2]).suffix, '.sh')
 
             scripts.append(Path(arguments[2]).read_text(encoding='utf-8'))
+            Path(arguments[2]).parent.joinpath('ready').write_text(
+                '77', encoding='utf-8'
+            )
             return process
 
         link = {'ifname': 'utun101', 'ifindex': 77}
@@ -101,6 +104,8 @@ class SingTUNLinuxHostTest(unittest.TestCase):
             self.module, '_command', return_value=json.dumps([link])
         ):
             self.host.prepare()
+            with self.assertRaisesRegex(RuntimeError, 'already active'):
+                self.host.execute([])
             self.host.applyDNS('1.1.1.1')
 
         self.assertEqual(self.host.index, 77)
@@ -113,9 +118,22 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         self.assertIn('trap ', scripts[0])
         self.assertNotIn('ifalias', ''.join(scripts))
         self.assertNotIn('furious-sing-tun-', ''.join(scripts))
-        self.assertIn('/sys/class/net/utun101/ifindex', scripts[1])
-        self.assertIn('resolvectl dns utun101 1.1.1.1', scripts[1])
+        self.assertEqual(len(scripts), 1)
+        self.assertIn('/sys/class/net/utun101/ifindex', scripts[0])
+        self.assertIn('resolvectl dns utun101 1.1.1.1', scripts[0])
         self.assertNotIn('socks5', ''.join(scripts))
+        self.assertIs(self.host._process, process)
+        self.assertIsNotNone(self.host._directory)
+
+        # Forwarding stops first; releasing this same helper needs no pkexec.
+        with mock.patch.object(
+            self.module, '_command', return_value='[]'
+        ), mock.patch.object(self.module.subprocess, 'Popen') as launch:
+            self.host.cleanup()
+            self.host.cleanup()
+
+        launch.assert_not_called()
+        process.stdin.close.assert_called_once()
         self.assertIsNone(self.host._process)
         self.assertIsNone(self.host._directory)
 
@@ -130,37 +148,151 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                     checked.returncode, 0, checked.stderr.decode(errors='replace')
                 )
 
-    def testAuthorizationTimeoutRetainsExactProcessAndCancellationFile(self):
+    def testRecoveryWaitsForAuthorizationWithoutADeadline(self):
         process = mock.Mock()
-        process.wait.side_effect = subprocess.TimeoutExpired('pkexec', 60)
         process.poll.return_value = None
+        entered, authorized, complete = (
+            threading.Event(),
+            threading.Event(),
+            threading.Event(),
+        )
+        errors = []
+
+        def wait(timeout=None):
+            self.assertIsNone(timeout)
+            entered.set()
+            if not authorized.wait(5):
+                raise RuntimeError('fixture authorization was not released')
+            process.poll.return_value = 0
+            return 0
+
+        process.wait.side_effect = wait
+
+        def recover():
+            try:
+                self.host.execute([['ip', 'link', 'delete', 'dev', 'utun101']])
+            except Exception as ex:
+                # Any non-exit exceptions
+                errors.append(ex)
+            finally:
+                complete.set()
 
         with mock.patch.object(
             self.module.os, 'geteuid', return_value=1000, create=True
         ), mock.patch.object(self.module.subprocess, 'Popen', return_value=process):
-            with self.assertRaisesRegex(RuntimeError, 'timed out'):
-                self.host.execute(
-                    [['ip', 'link', 'set', 'dev', 'utun101', 'up']], preparing=True
-                )
+            worker = threading.Thread(target=recover)
+            worker.start()
+            try:
+                self.assertTrue(entered.wait(2))
+                self.assertFalse(complete.is_set())
+                self.assertIs(self.host._process, process)
+                self.assertIsNotNone(self.host._directory)
+                process.terminate.assert_not_called()
+                process.kill.assert_not_called()
+            finally:
+                authorized.set()
+                worker.join(5)
+
+        self.assertFalse(worker.is_alive())
+        self.assertEqual(errors, [])
+        process.wait.assert_called_once_with()
+        self.assertIsNone(self.host._process)
+        self.assertIsNone(self.host._directory)
+
+    def testDelayedAuthorizationIsUsedForSetupDnsAndDisconnect(self):
+        """A slow password entry does not create extra authorization requests."""
+        process = mock.Mock()
+        process.wait.return_value = 0
+        process.poll.return_value = None
+        clock = [0.0]
+        script = []
+
+        def launch(arguments, **kwargs):
+            script.append(Path(arguments[-1]))
+            return process
+
+        def advance(_seconds):
+            clock[0] += 3600
+            if clock[0] >= 86400:
+                script[0].parent.joinpath('ready').write_text('77', encoding='utf-8')
+
+        with mock.patch.object(
+            self.module.os, 'geteuid', return_value=1000, create=True
+        ), mock.patch.object(
+            self.module.os, 'getuid', return_value=1000, create=True
+        ), mock.patch.object(
+            self.module.subprocess, 'Popen', side_effect=launch
+        ) as launchHelper, mock.patch.object(
+            self.module.time, 'monotonic', side_effect=lambda: clock[0]
+        ), mock.patch.object(
+            self.module.time, 'sleep', side_effect=advance
+        ), mock.patch.object(
+            self.module,
+            '_command',
+            return_value=json.dumps([{'ifname': 'utun101', 'ifindex': 77}]),
+        ):
+            self.host.prepare('9.9.9.9')
+            self.host.applyDNS('9.9.9.9')
+            self.host.cancel()
+            self.host.drain()
+            process.stdin.close.assert_not_called()
+
+            with mock.patch.object(self.module, '_command', return_value='[]'):
+                self.host.cleanup()
+
+        self.assertEqual(clock[0], 86400)
+        launchHelper.assert_called_once()
+        process.stdin.close.assert_called_once()
+        self.assertFalse(self.host.dnsTouched)
+
+    def testPendingAuthorizationCancellationRetainsHelperUntilItExits(self):
+        process = mock.Mock()
+        process.poll.return_value = None
+        process.wait.side_effect = subprocess.TimeoutExpired('pkexec', 5)
+
+        def launch(arguments, **kwargs):
+            self.host.cancelled.set()
+            return process
+
+        with mock.patch.object(
+            self.module.os, 'geteuid', return_value=1000, create=True
+        ), mock.patch.object(self.module.subprocess, 'Popen', side_effect=launch):
+            with self.assertRaisesRegex(RuntimeError, 'preparation was cancelled'):
+                self.host.execute([], preparing=True, holding=True)
 
         directory = Path(self.host._directory.name)
-
-        self.assertIs(self.host._process, process)
-        self.assertEqual(len(list(directory.glob('*.sh'))), 1)
         self.assertTrue((directory / 'cancelled').exists())
+        process.stdin.close.assert_called_once()
 
         with self.assertRaisesRegex(RuntimeError, 'still running'):
             self.host.drain()
 
         self.assertIs(self.host._process, process)
-        process.terminate.assert_not_called()
-        process.kill.assert_not_called()
-
-        process.poll.return_value = 0
+        self.assertTrue(directory.exists())
+        process.poll.return_value = 126
         self.host.drain()
-
         self.assertFalse(directory.exists())
-        self.assertIsNone(self.host._process)
+
+    def testLiveForwarderDnsCannotBeReportedAsRestored(self):
+        """A refused child reap retains DNS and the authorized helper together."""
+        plan = SingTUNHostPlan({}, platform='Linux')
+        plan._linux = self.host
+        self.host._process = process = mock.Mock()
+        process.poll.return_value = None
+        self.host._directory = tempfile.TemporaryDirectory()
+        self.host._holding = self.host._ready = self.host.dnsTouched = True
+
+        with self.assertRaisesRegex(
+            RuntimeError, 'requires stopping the forwarding child'
+        ):
+            while not plan.finishDNSCleanup():
+                self.assertTrue(plan._done.wait(2))
+                plan._worker.join(2)
+
+        self.assertTrue(self.host.dnsTouched)
+        self.assertIs(self.host._process, process)
+        process.stdin.close.assert_not_called()
+        self.host._releaseHelper()
 
     def testScriptRetainsSuccessfulSetupAndRollsBackFailure(self):
         bash = _bash()
@@ -171,9 +303,9 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         realPopen = subprocess.Popen
 
         failures = (
-            ('mtu', 'cancel', 'abandon', 'none')
+            ('mtu', 'rule', 'dns', 'cancel', 'abandon', 'none', 'eof')
             if os.name != 'nt'
-            else ('mtu', 'cancel', 'none')
+            else ('mtu', 'rule', 'dns', 'cancel', 'none', 'eof')
         )
 
         for failure in failures:
@@ -209,7 +341,16 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                         '"link set dev")\n'
                         '  if [ "$FAKE_FAILURE" = mtu ] && [ "$5" = mtu ]; then exit 1; fi\n'
                         '  ;;\n'
-                        '"-4 addr add"|"-6 addr add"|"-4 route add"|"-6 route add"|"-4 rule add"|"-6 rule add") ;;\n'
+                        '"-4 rule add"|"-6 rule add")\n'
+                        '  count=$(cat "$FAKE_NET/rules" 2>/dev/null || echo 0)\n'
+                        '  if [ "$FAKE_FAILURE" = rule ] && [ "$count" = 2 ]; then exit 1; fi\n'
+                        '  echo $((count + 1)) > "$FAKE_NET/rules"\n'
+                        '  ;;\n'
+                        '"-4 rule del"|"-6 rule del")\n'
+                        '  count=$(cat "$FAKE_NET/rules")\n'
+                        '  echo $((count - 1)) > "$FAKE_NET/rules"\n'
+                        '  ;;\n'
+                        '"-4 addr add"|"-6 addr add"|"-4 route add"|"-6 route add") ;;\n'
                         '"link delete dev")\n'
                         '  rm "$device/ifindex"\n'
                         '  rmdir "$device"\n'
@@ -219,6 +360,14 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                     )
 
                 ip.chmod(0o755)
+                resolver = binDirectory / 'resolvectl'
+                resolver.write_text(
+                    '#!/usr/bin/env bash\n'
+                    'echo "$1" >> "$FAKE_NET/dns"\n'
+                    'if [ "$FAKE_FAILURE" = dns ] && [ "$1" = domain ]; then exit 1; fi\n',
+                    encoding='utf-8',
+                )
+                resolver.chmod(0o755)
 
                 def launch(arguments, **kwargs):
                     self.assertEqual(arguments[:2], ['pkexec', 'bash'])
@@ -257,21 +406,40 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                 ), mock.patch.object(
                     self.module,
                     '_command',
-                    return_value=json.dumps([{'ifname': 'utun101', 'ifindex': 77}]),
+                    side_effect=lambda args: json.dumps(
+                        [{'ifname': 'utun101', 'ifindex': 77}]
+                        if args == ['ip', '-N', '-j', 'link', 'show']
+                        and (net / 'utun101').exists()
+                        else []
+                    ),
                 ):
-                    if failure == 'none':
+                    if failure in ('none', 'eof'):
                         self.host.prepare()
 
                         self.assertEqual(self.host.index, 77)
+                        self.assertTrue((net / 'utun101').exists())
+                        self.assertIsNone(self.host._process.poll())
+                        if failure == 'eof':
+                            # Closing the parent pipe also covers an application
+                            # exit without calling the normal cleanup method.
+                            self.host._releaseHelper()
+                            self.assertEqual(self.host._process.wait(timeout=5), 0)
+                        self.host.cleanup()
                     else:
                         with self.assertRaisesRegex(
                             RuntimeError, 'host operation failed'
                         ):
                             self.host.prepare()
 
-                self.assertEqual((net / 'utun101').exists(), failure == 'none')
+                self.assertFalse((net / 'utun101').exists())
                 self.assertIsNone(self.host._process)
                 self.assertIsNone(self.host._directory)
+                if (net / 'rules').exists():
+                    self.assertEqual((net / 'rules').read_text().strip(), '0')
+                if (net / 'dns').exists():
+                    self.assertEqual(
+                        (net / 'dns').read_text().splitlines()[-1], 'revert'
+                    )
 
     def testCancellationBeforeAuthorizationCannotAcquireDevice(self):
         scripts = []
@@ -461,6 +629,28 @@ class SingTUNLinuxHostTest(unittest.TestCase):
                 'utun101',
             )
 
+    def testKernelSuppressionFieldMatchesExactRule(self):
+        """Use iproute2's JSON spelling, which differs from its CLI spelling."""
+        expected = {_linuxRuleSelectors(command) for command in self.host.rules}
+        rule = {
+            'priority': 2001,
+            'src': 'all',
+            'table': 123456,
+            'suppress_prefixlen': 0,
+            'protocol': 0,
+        }
+        command = _linuxRuleDelete('-4', rule, 123456, 2000, 'utun101')
+        self.assertIn(_linuxRuleSelectors(command), expected)
+        self.assertIn('suppress_prefixlength', command)
+
+        for value in (1, -1, '0'):
+            with self.subTest(value=value), self.assertRaisesRegex(
+                RuntimeError, 'Conflicting'
+            ):
+                _linuxRuleDelete(
+                    '-4', {**rule, 'suppress_prefixlen': value}, 123456, 2000, 'utun101'
+                )
+
     def testCleanupUsesExactSelectorsAndRetainsRefusedCleanupForRetry(self):
         plan = SingTUNHostPlan({}, platform='Linux')
         plan._linux = self.host
@@ -468,7 +658,13 @@ class SingTUNLinuxHostTest(unittest.TestCase):
         link = {'ifname': 'utun101', 'ifindex': 77}
         self.host.index = 77
         present = True
-        rule = {'priority': 2000, 'src': 'all', 'dst': '198.18.0.0/15', 'table': 123456}
+        rule = {
+            'priority': 2001,
+            'src': 'all',
+            'table': 123456,
+            'suppress_prefixlen': 0,
+            'protocol': 0,
+        }
 
         def command(arguments):
             if arguments == ['ip', '-N', '-j', 'link', 'show']:
@@ -492,8 +688,8 @@ class SingTUNLinuxHostTest(unittest.TestCase):
             nonlocal present
             self.assertTrue(kwargs['guard'])
             self.assertEqual(commands[-1], ['ip', 'link', 'delete', 'dev', 'utun101'])
-            self.assertIn('to', commands[0])
-            self.assertIn('198.18.0.0/15', commands[0])
+            self.assertIn('suppress_prefixlength', commands[0])
+            self.assertIn('123456', commands[0])
             self.assertNotIn('flush', str(commands))
             present = False