NAS-134584 / 25.04-RC.1 / Make sure VNC password is not visible in ps…

… (by Qubad786) (#15933)

## Problem

For VM based virt instances, if any had vnc password configured - that
was visible in ps command output as it was being passed as command line
argument to qemu.

## Solution

Make sure VNC password is not exposed in ps output.

Original PR: #15930
Jira URL: https://ixsystems.atlassian.net/browse/NAS-134584

Co-authored-by: M. Rehan <[email protected]>

src/middlewared/middlewared/plugins/virt/global.py

@@ -1,4 +1,5 @@
 from typing import TYPE_CHECKING
+import shutil
 import subprocess
 import middlewared.sqlalchemy as sa
 
@@ -17,7 +18,7 @@
 from middlewared.utils import run
 from middlewared.plugins.boot import BOOT_POOL_NAME_VALID
 
-from .utils import Status, incus_call
+from .utils import Status, incus_call, VNC_PASSWORD_DIR
 if TYPE_CHECKING:
     from middlewared.main import Middleware
 
@@ -226,6 +227,7 @@ async def setup(self, job):
             await self.middleware.call('cache.put', 'VIRT_STATE', Status.INITIALIZED.value)
         finally:
             self.middleware.send_event('virt.global.config', 'CHANGED', fields=await self.config())
+            await self.auto_start_instances()
 
     async def _setup_impl(self):
         config = await self.config()
@@ -434,6 +436,22 @@ async def reset(self, job, start: bool = False, config: dict | None = None):
         if start and not await self.middleware.call('service.start', 'incus'):
             raise CallError('Failed to start virtualization service')
 
+        if not start:
+            await self.middleware.run_in_thread(shutil.rmtree, VNC_PASSWORD_DIR, True)
+
+    @private
+    async def auto_start_instances(self):
+        await self.middleware.call(
+            'core.bulk', 'virt.instance.start', [
+                [instance['name']] for instance in await self.middleware.call(
+                    'virt.instance.query', [['autostart', '=', True], ['status', '=', 'STOPPED']]
+                )
+                # We have an explicit filter for STOPPED because old virt instances would still have
+                # incus autostart enabled and we don't want to attempt to start them again.
+                # We can remove this in FT release perhaps.
+            ]
+        )
+
 
 async def _event_system_ready(middleware: 'Middleware', event_type, args):
     if not await middleware.call('failover.licensed'):

src/middlewared/middlewared/plugins/virt/instance.py

@@ -22,7 +22,8 @@
     VirtInstanceImageChoicesArgs, VirtInstanceImageChoicesResult,
 )
 from .utils import (
-    get_vnc_info_from_config, get_root_device_dict, Status, incus_call, incus_call_and_wait, VNC_BASE_PORT
+    create_vnc_password_file, get_vnc_info_from_config, get_root_device_dict, get_vnc_password_file_path,
+    Status, incus_call, incus_call_and_wait, VNC_BASE_PORT,
 )
 
 
@@ -60,6 +61,8 @@ async def query(self, filters, options):
                 status = 'UNKNOWN'
             else:
                 status = i['state']['status'].upper()
+
+            autostart = i['config'].get('boot.autostart')
             secureboot = None
             if i['config'].get('image.requirements.secureboot') == 'true':
                 secureboot = True
@@ -71,7 +74,7 @@ async def query(self, filters, options):
                 'type': 'CONTAINER' if i['type'] == 'container' else 'VM',
                 'status': status,
                 'cpu': i['config'].get('limits.cpu'),
-                'autostart': True if i['config'].get('boot.autostart') == 'true' else False,
+                'autostart': True if i['config'].get('user.autostart', autostart) == 'true' else False,
                 'environment': {},
                 'aliases': [],
                 'image': {
@@ -237,7 +240,7 @@ async def validate(self, new, schema_name, verrors, old=None):
                     if any(new['vnc_port'] in v for v in port_mapping.values()):
                         verrors.add(f'{schema_name}.vnc_port', 'VNC port is already in use by another virt instance')
 
-    def __data_to_config(self, data: dict, raw: dict = None, instance_type=None):
+    def __data_to_config(self, instance_name: str, data: dict, raw: dict = None, instance_type=None):
         config = {}
         if 'environment' in data:
             # If we are updating environment we need to remove current values
@@ -257,8 +260,9 @@ def __data_to_config(self, data: dict, raw: dict = None, instance_type=None):
             else:
                 config['limits.memory'] = None
 
+        config['boot.autostart'] = 'false'
         if data.get('autostart') is not None:
-            config['boot.autostart'] = str(data['autostart']).lower()
+            config['user.autostart'] = str(data['autostart']).lower()
 
         if instance_type == 'VM':
             config.update({
@@ -274,7 +278,8 @@ def __data_to_config(self, data: dict, raw: dict = None, instance_type=None):
             if data.get('enable_vnc') and data.get('vnc_port'):
                 vnc_config = f'-vnc :{data["vnc_port"] - VNC_BASE_PORT}'
                 if data.get('vnc_password'):
-                    vnc_config = f'-object secret,id=vnc0,data={data["vnc_password"]} {vnc_config},password-secret=vnc0'
+                    vnc_config = (f'-object secret,id=vnc0,file={get_vnc_password_file_path(instance_name)} '
+                                  f'{vnc_config},password-secret=vnc0')
 
                 config['raw.qemu'] = vnc_config
             if data.get('enable_vnc') is False:
@@ -421,36 +426,20 @@ async def running_cb(data):
             await incus_call_and_wait('1.0/instances', 'post', {'json': {
                 'name': data['name'],
                 'ephemeral': False,
-                'config': self.__data_to_config(data, instance_type=data['instance_type']),
+                'config': self.__data_to_config(data['name'], data, instance_type=data['instance_type']),
                 'devices': devices,
                 'source': source,
                 'type': 'container' if data['instance_type'] == 'CONTAINER' else 'virtual-machine',
-                'start': False if data['instance_type'] == 'CONTAINER' else data['autostart'],
+                'start': False,
             }}, running_cb, timeout=15 * 60)
             # We will give 15 minutes to incus to download relevant image and then timeout
         except CallError as e:
             if await self.middleware.call('virt.instance.query', [['name', '=', data['name']]]):
                 await (await self.middleware.call('virt.instance.delete', data['name'])).wait()
             raise e
 
-        if data['instance_type'] == 'CONTAINER':
-            # apply idmap settings then apply autostart settings
-            await self.set_account_idmaps(data['name'])
-
-            if data['autostart']:
-                raw = (await self.middleware.call(
-                    'virt.instance.get_instance', data['name'], {'extra': {'raw': True}}
-                ))['raw']
-                raw['config']['boot.autostart'] = 'true'
-                try:
-                    await incus_call_and_wait(f'1.0/instances/{data["name"]}', 'put', {'json': raw})
-                except CallError as e:
-                    await (await self.middleware.call('virt.instance.delete', data['name'])).wait()
-                    raise e
-
-                await incus_call_and_wait(f'1.0/instances/{data["name"]}/state', 'put', {'json': {
-                    'action': 'start',
-                }})
+        if data['autostart']:
+            await self.start_impl(job, data['name'])
 
         return await self.middleware.call('virt.instance.get_instance', data['name'])
 
@@ -489,7 +478,7 @@ async def do_update(self, job, id, data):
 
         verrors.check()
 
-        instance['raw']['config'].update(self.__data_to_config(data, instance['raw']['config'], instance['type']))
+        instance['raw']['config'].update(self.__data_to_config(id, data, instance['raw']['config'], instance['type']))
         if data.get('root_disk_size') or data.get('root_disk_io_bus'):
             root_disk_size = data.get('root_disk_size') or int(instance['root_disk_size'] / (1024 ** 3))
             io_bus = data.get('root_disk_io_bus') or instance['root_disk_io_bus']
@@ -525,10 +514,18 @@ async def start(self, job, id):
         Start an instance.
         """
         await self.middleware.call('virt.global.check_initialized')
+        return await self.start_impl(job, id)
+
+    @private
+    async def start_impl(self, job, id):
         instance = await self.middleware.call('virt.instance.get_instance', id)
 
         # Apply any idmap changes
-        await self.set_account_idmaps(id)
+        if instance['status'] != 'RUNNING':
+            await self.set_account_idmaps(id)
+
+        if instance['vnc_password']:
+            await self.middleware.run_in_thread(create_vnc_password_file, id, instance['vnc_password'])
 
         try:
             await incus_call_and_wait(f'1.0/instances/{id}/state', 'put', {'json': {
@@ -592,6 +589,9 @@ async def restart(self, job, id, data):
         # Apply any idmap changes
         await self.set_account_idmaps(id)
 
+        if instance['vnc_password']:
+            await self.middleware.run_in_thread(create_vnc_password_file, id, instance['vnc_password'])
+
         await incus_call_and_wait(f'1.0/instances/{id}/state', 'put', {'json': {
             'action': 'start',
         }})

src/middlewared/middlewared/plugins/virt/utils.py

@@ -1,4 +1,5 @@
 import asyncio
+import os
 from dataclasses import dataclass
 
 import aiohttp
@@ -8,13 +9,15 @@
 from collections.abc import Callable
 
 from middlewared.service import CallError
+from middlewared.utils import MIDDLEWARE_RUN_DIR
 
 from .websocket import IncusWS
 
 
 SOCKET = '/var/lib/incus/unix.socket'
 HTTP_URI = 'http://unix.socket'
 VNC_BASE_PORT = 5900
+VNC_PASSWORD_DIR = os.path.join(MIDDLEWARE_RUN_DIR, 'incus/passwords')
 
 
 class Status(enum.Enum):
@@ -107,6 +110,21 @@ def get_vnc_info_from_config(config: dict):
     return json.loads(vnc_raw_config)
 
 
+def get_vnc_password_file_path(instance_id: str) -> str:
+    return os.path.join(VNC_PASSWORD_DIR, instance_id)
+
+
+def create_vnc_password_file(instance_id: str, password: str) -> str:
+    os.makedirs(VNC_PASSWORD_DIR, exist_ok=True)
+    pass_file_path = get_vnc_password_file_path(instance_id)
+    with open(pass_file_path, 'w') as w:
+        os.fchmod(w.fileno(), 0o600)
+        w.write(password)
+
+
+    return pass_file_path
+
+
 def get_root_device_dict(size: int, io_bus: str) -> dict:
     return {
         'path': '/',

tests/api2/test_virt_vm.py

@@ -12,7 +12,7 @@
 from middlewared.test.integration.assets.virt import (
     virt, import_iso_as_volume, volume, virt_device, virt_instance
 )
-from middlewared.test.integration.utils import call
+from middlewared.test.integration.utils import call, ssh
 from middlewared.service_exception import ValidationErrors as ClientValidationErrors
 
 from functions import POST, wait_on_job
@@ -169,8 +169,9 @@ def test_vm_props(vm):
     # Going to unset VNC
     call('virt.instance.update', VM_NAME, {'enable_vnc': False}, job=True)
     instance = call('virt.instance.get_instance', VM_NAME, {'extra': {'raw': True}})
-    assert instance['raw']['config']['user.ix_old_raw_qemu_config'] == f'-object secret,id=vnc0,data=test123 ' \
-                                                                       f'-vnc :{VNC_PORT - 5900},password-secret=vnc0'
+    assert instance['raw']['config']['user.ix_old_raw_qemu_config'] == (f'-object secret,id=vnc0,file=/var/run/'
+                                                                        f'middleware/incus/passwords/{VM_NAME} '
+                                                                        f'-vnc :{VNC_PORT - 5900},password-secret=vnc0')
     assert instance['vnc_enabled'] is False, instance
     assert instance['vnc_port'] is None, instance
 
@@ -185,15 +186,19 @@ def test_vm_props(vm):
     call('virt.instance.update', VM_NAME, {'vnc_password': 'update_test123', 'enable_vnc': True}, job=True)
     instance = call('virt.instance.get_instance', VM_NAME, {'extra': {'raw': True}})
     assert instance['raw']['config'].get('user.ix_old_raw_qemu_config') == f'-vnc :{1001}'
-    assert instance['raw']['config']['raw.qemu'] == f'-object secret,id=vnc0,data=update_test123' \
-                                                    f' -vnc :{1001},password-secret=vnc0'
+    assert instance['raw']['config']['raw.qemu'] == ('-object secret,id=vnc0,file=/var/run/middleware/incus/'
+                                                     f'passwords/{VM_NAME} -vnc :{1001},password-secret=vnc0')
     assert instance['vnc_port'] == 6901, instance
 
     # Changing nothing
     instance = call('virt.instance.update', VM_NAME, {}, job=True)
     assert instance['vnc_port'] == 6901, instance
     assert instance['vnc_password'] == 'update_test123', instance
 
+    call('virt.instance.start', VM_NAME, job=True)
+    assert ssh(f'cat /var/run/middleware/incus/passwords/{VM_NAME}') == 'update_test123'
+    call('virt.instance.stop', VM_NAME, {'force': True, 'timeout': -1}, job=True)
+
 
 def test_vm_iso_volume(vm, iso_volume):
     device_name = 'iso_device'