From 51d6bf55f155a982903da44737b0c86d9aacc320 Mon Sep 17 00:00:00 2001 From: Samuel Angebault Date: Fri, 28 May 2021 14:26:01 -0700 Subject: [PATCH] Fix Aboot breakage in sonic package manager in sonic-installer (#1625) > Failure cause The `get_rootfs_path` contextmanager was repurposed to implement `get_file_in_image` and later used as a function by leveraging some python complexity to bypass the restrictions coming with the contextmanager which were added purposefuly. It was then called multiple times to compute paths though a simple path join using `new_image_dir` could have been performed. The `get_rootfs_path` implementation for Aboot behaved differently when a rootfs was extracted or kept within the SWI image. It also behaved differently on secureboot systems. The updated method was then called on non-existing files for which the method was never meant to process. > Context around the failure Over time, the installation and boot process has slightly diverged from the ONIE one. There are 3 scenarios to consider. 1) Regular boot similar to ONIE This one should just work the same as the filesystem layout is unchanged. 2) docker_inram where dockerfs.tar.gz is extracted in tmpfs at boot In this scenario the boot is similar to the regular one beside that dockerfs.tar.gz is preserved intact on the flash and not extracted. By design this does not fit the sonic-package-manager requierements and the migration should be skipped which is what I did in this review. In the coming month this boot mode will look closer to 3) below. 3) Secureboot on Arista devices In this scenario the SWI image is kept intact and nothing extracted from it. By ensuring the integrity of the SWI we can guarantee that no code/data has been tampered with. This mode also relies on `docker_inram` at the moment. It could be enhanced when sonic-package-manager can guarantee the integrity of code and data that is both installed and migrated. > Solution provided The method `get_file_in_image` was reverted to its original meaning `get_rootfs_path` as there is no point in keeping the new one. It doesn't have the necessary logic to handle more than the rootfs and the logic can be easily be achieved by the following line. `os.path.join(bootloader.get_image_path(binary_image_name), 'something')` A new Bootloader method called `support_package_migration` is introduced to allow the bootloader to skip the package migration based on the image (docker_inram) or its own configuration (secureboot). By default all bootloaders enable the package migration. That change leads to 1) above running package migration while 2) and 3) skip it. --- sonic_installer/bootloader/aboot.py | 60 +++++++++++++++++++----- sonic_installer/bootloader/bootloader.py | 9 +++- sonic_installer/main.py | 37 +++++++-------- 3 files changed, 73 insertions(+), 33 deletions(-) diff --git a/sonic_installer/bootloader/aboot.py b/sonic_installer/bootloader/aboot.py index a2ef2acf4fcd..47e9c5db5fde 100644 --- a/sonic_installer/bootloader/aboot.py +++ b/sonic_installer/bootloader/aboot.py @@ -19,6 +19,7 @@ HOST_PATH, IMAGE_DIR_PREFIX, IMAGE_PREFIX, + ROOTFS_NAME, run_command, run_command_or_raise, ) @@ -26,18 +27,36 @@ _secureboot = None DEFAULT_SWI_IMAGE = 'sonic.swi' +KERNEL_CMDLINE_NAME = 'kernel-cmdline' # For the signature format, see: https://github.com/aristanetworks/swi-tools/tree/master/switools SWI_SIG_FILE_NAME = 'swi-signature' SWIX_SIG_FILE_NAME = 'swix-signature' ISSUERCERT = 'IssuerCert' -def isSecureboot(): +def parse_cmdline(cmdline=None): + if cmdline is None: + with open('/proc/cmdline') as f: + cmdline = f.read() + + data = {} + for entry in cmdline.split(): + idx = entry.find('=') + if idx == -1: + data[entry] = None + else: + data[entry[:idx]] = entry[idx+1:] + return data + +def docker_inram(cmdline=None): + cmdline = parse_cmdline(cmdline) + return cmdline.get('docker_inram') == 'on' + +def is_secureboot(): global _secureboot if _secureboot is None: - with open('/proc/cmdline') as f: - m = re.search(r"secure_boot_enable=[y1]", f.read()) - _secureboot = bool(m) + cmdline = parse_cmdline() + _secureboot = cmdline.get('secure_boot_enable') in ['y', '1'] return _secureboot class AbootBootloader(Bootloader): @@ -70,7 +89,7 @@ def _boot_config_set(self, **kwargs): def _swi_image_path(self, image): image_dir = image.replace(IMAGE_PREFIX, IMAGE_DIR_PREFIX) - if isSecureboot(): + if is_secureboot(): return 'flash:%s/sonic.swi' % image_dir return 'flash:%s/.sonic-boot.swi' % image_dir @@ -118,6 +137,25 @@ def remove_image(self, image): subprocess.call(['rm','-rf', image_path]) click.echo('Image removed') + def _get_image_cmdline(self, image): + image_path = self.get_image_path(image) + with open(os.path.join(image_path, KERNEL_CMDLINE_NAME)) as f: + return f.read() + + def supports_package_migration(self, image): + if is_secureboot(): + # NOTE: unsafe until migration can guarantee migration safety + # packages need to be signed and verified at boot time. + return False + cmdline = self._get_image_cmdline(image) + if docker_inram(cmdline): + # NOTE: the docker_inram feature extracts builtin containers at boot + # time in memory. the use of package manager under these + # circumpstances is not possible without a boot time package + # installation mechanism. + return False + return True + def get_binary_image_version(self, image_path): try: version = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.imagehash'], text=True) @@ -140,7 +178,7 @@ def verify_next_image(self): return self._verify_secureboot_image(image_path) def _verify_secureboot_image(self, image_path): - if isSecureboot(): + if is_secureboot(): cert = self.getCert(image_path) return cert is not None return True @@ -188,14 +226,14 @@ def _get_swi_file_offset(self, swipath, filename): return f._fileobj.tell() # pylint: disable=protected-access @contextmanager - def get_path_in_image(self, image_path, path): - path_in_image = os.path.join(image_path, path) - if os.path.exists(path_in_image) and not isSecureboot(): - yield path_in_image + def get_rootfs_path(self, image_path): + path = os.path.join(image_path, ROOTFS_NAME) + if os.path.exists(path) and not is_secureboot(): + yield path return swipath = os.path.join(image_path, DEFAULT_SWI_IMAGE) - offset = self._get_swi_file_offset(swipath, path) + offset = self._get_swi_file_offset(swipath, ROOTFS_NAME) loopdev = subprocess.check_output(['losetup', '-f']).decode('utf8').rstrip() try: diff --git a/sonic_installer/bootloader/bootloader.py b/sonic_installer/bootloader/bootloader.py index a6694977ae3b..b953dd2715ec 100644 --- a/sonic_installer/bootloader/bootloader.py +++ b/sonic_installer/bootloader/bootloader.py @@ -9,6 +9,7 @@ HOST_PATH, IMAGE_DIR_PREFIX, IMAGE_PREFIX, + ROOTFS_NAME, ) class Bootloader(object): @@ -58,6 +59,10 @@ def verify_next_image(self): image_path = self.get_image_path(image) return path.exists(image_path) + def supports_package_migration(self, image): + """tells if the image supports package migration""" + return True + @classmethod def detect(cls): """returns True if the bootloader is in use""" @@ -70,6 +75,6 @@ def get_image_path(cls, image): return image.replace(IMAGE_PREFIX, prefix) @contextmanager - def get_path_in_image(self, image_path, path_in_image): + def get_rootfs_path(self, image_path): """returns the path to the squashfs""" - yield path.join(image_path, path_in_image) + yield path.join(image_path, ROOTFS_NAME) diff --git a/sonic_installer/main.py b/sonic_installer/main.py index 57cd5adda899..bab1166422c9 100644 --- a/sonic_installer/main.py +++ b/sonic_installer/main.py @@ -1,5 +1,4 @@ import configparser -import contextlib import os import re import subprocess @@ -12,11 +11,9 @@ from swsscommon.swsscommon import SonicV2Connector from .bootloader import get_bootloader -from .bootloader.aboot import AbootBootloader from .common import ( run_command, run_command_or_raise, IMAGE_PREFIX, - ROOTFS_NAME, UPPERDIR_NAME, WORKDIR_NAME, DOCKERDIR_NAME, @@ -279,7 +276,7 @@ def update_sonic_environment(bootloader, binary_image_version): env_dir = os.path.join(new_image_dir, "sonic-config") env_file = os.path.join(env_dir, "sonic-environment") - with bootloader.get_path_in_image(new_image_dir, ROOTFS_NAME) as new_image_squashfs_path: + with bootloader.get_rootfs_path(new_image_dir) as new_image_squashfs_path: try: mount_squash_fs(new_image_squashfs_path, new_image_mount) @@ -321,21 +318,21 @@ def migrate_sonic_packages(bootloader, binary_image_version): packages_path = os.path.join(PACKAGE_MANAGER_DIR, packages_file) sonic_version = re.sub(IMAGE_PREFIX, '', binary_image_version) new_image_dir = bootloader.get_image_path(binary_image_version) + new_image_upper_dir = os.path.join(new_image_dir, UPPERDIR_NAME) + new_image_work_dir = os.path.join(new_image_dir, WORKDIR_NAME) + new_image_docker_dir = os.path.join(new_image_dir, DOCKERDIR_NAME) + new_image_mount = os.path.join("/", tmp_dir, "image-{0}-fs".format(sonic_version)) + new_image_docker_mount = os.path.join(new_image_mount, "var", "lib", "docker") + + if not os.path.isdir(new_image_docker_dir): + # NOTE: This codepath can be reached if the installation process did not + # extract the default dockerfs. This can happen with docker_inram + # though the bootloader class should have disabled the package + # migration which is why this message is a non fatal error message. + echo_and_log("Error: SONiC package migration cannot proceed due to missing docker folder", LOG_ERR, fg="red") + return - with contextlib.ExitStack() as stack: - def get_path(path): - """ Closure to get path by entering - a context manager of bootloader.get_path_in_image """ - - return stack.enter_context(bootloader.get_path_in_image(new_image_dir, path)) - - new_image_squashfs_path = get_path(ROOTFS_NAME) - new_image_upper_dir = get_path(UPPERDIR_NAME) - new_image_work_dir = get_path(WORKDIR_NAME) - new_image_docker_dir = get_path(DOCKERDIR_NAME) - new_image_mount = os.path.join("/", tmp_dir, "image-{0}-fs".format(sonic_version)) - new_image_docker_mount = os.path.join(new_image_mount, "var", "lib", "docker") - + with bootloader.get_rootfs_path(new_image_dir) as new_image_squashfs_path: try: mount_squash_fs(new_image_squashfs_path, new_image_mount) # make sure upper dir and work dir exist @@ -434,8 +431,8 @@ def install(url, force, skip_migration=False, skip_package_migration=False): update_sonic_environment(bootloader, binary_image_version) - if isinstance(bootloader, AbootBootloader) and not skip_package_migration: - echo_and_log("Warning: SONiC package migration is not supported currenty on aboot platform due to https://github.com/Azure/sonic-buildimage/issues/7566.", LOG_ERR, fg="red") + if not bootloader.supports_package_migration(binary_image_version) and not skip_package_migration: + echo_and_log("Warning: SONiC package migration is not supported for this bootloader/image", fg="yellow") skip_package_migration = True if not skip_package_migration: