Refactor how we extract tarballs #44

* Introduce a new ExtractEvent that extract_tar() returns and
  contains error and warning messages.
* Extract symlinks correctly.
* do not try to strip leading slash in paths
* do not try to treat broken links specially when extracting links

Replacing: #42
Referenced-by: #44
Reported-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Author: pombredanne

CHANGELOG.rst

@@ -1,6 +1,18 @@
 Changelog
 =========
 
+v32.0.0 
+--------
+
+This is a minor release with bug fixes and an output change.
+
+- "utils.extract_tar" function now behaves correctly with links and return
+  ExtractEvent to track extraction errors and warnings.
+  This replaces the simpler list of error messages.
+- in all places where extract is callable (Image, Layer) there is a new
+  skip_symlinks argument defaulting to True. If True, we skip symlinks and links.
+
+
 v31.1.0
 --------
 

src/container_inspector/image.py

@@ -356,14 +356,22 @@ def bottom_layer(self):
         """
         return self.layers[0]
 
-    def extract_layers(self, extracted_location):
+    def extract_layers(self, extracted_location, skip_symlinks=True):
         """
         Extract all layer archives to the `extracted_location` directory.
         Each layer is extracted to its own directory named after its `layer_id`.
+        Skip symlinks and links if ``skip_symlinks`` is True.
+        Return a list of ExtractEvent.
         """
+        all_events = []
         for layer in self.layers:
             exloc = os.path.join(extracted_location, layer.layer_id)
-            layer.extract(extracted_location=exloc)
+            events = layer.extract(
+                extracted_location=exloc,
+                skip_symlinks=skip_symlinks,
+            )
+            all_events.extend(events)
+        return events
 
     def get_layers_resources(self, with_dir=False):
         """
@@ -453,9 +461,11 @@ def get_installed_packages(self, packages_getter):
     def extract(archive_location, extracted_location, skip_symlinks=False):
         """
         Extract the image archive tarball at ``archive_location`` to
-        ``extracted_location``. Skip symlinks and links if ``skip_symlinks`` is True.
+        ``extracted_location``.
+        Skip symlinks and links if ``skip_symlinks`` is True.
+        Return a list of ExtractEvent.
         """
-        utils.extract_tar(
+        return utils.extract_tar(
             location=archive_location,
             target_dir=extracted_location,
             skip_symlinks=skip_symlinks,
@@ -466,24 +476,29 @@ def get_images_from_tarball(
         archive_location,
         extracted_location,
         verify=True,
+        skip_symlinks=False,
     ):
         """
-        Return a list of Images found in the tarball at `archive_location` that
-        will be extracted to `extracted_location`. The tarball must be in the
+        Return a list of Images found in the tarball at ``archive_location`` that
+        will be extracted to ``extracted_location``. The tarball must be in the
         format of a "docker save" command tarball.
 
-        If `verify` is True, perform extra checks on the config data and layers
+        If ``verify`` is True, perform extra checks on the config data and layers
         checksums.
+        Skip symlinks and links if ``skip_symlinks`` is True.
+        Ignore the extract events from extraction.
         """
         if TRACE:
             logger.debug(
-                f'get_images_from_tarball: {archive_location} , '
+                f'get_images_from_tarball: {archive_location} '
                 f'extracting to: {extracted_location}'
             )
 
-        Image.extract(
+        # TODO: do not ignore extract events
+        _events = Image.extract(
             archive_location=archive_location,
             extracted_location=extracted_location,
+            skip_symlinks=skip_symlinks,
         )
 
         return Image.get_images_from_dir(
@@ -1071,13 +1086,15 @@ def __attrs_post_init__(self, *args, **kwargs):
         if not self.size:
             self.size = os.path.getsize(self.archive_location)
 
-    def extract(self, extracted_location, skip_symlinks=True):
+    def extract(self, extracted_location, skip_symlinks=False):
         """
         Extract this layer archive in the `extracted_location` directory and set
         this Layer ``extracted_location`` attribute to ``extracted_location``.
+        Skip symlinks and links if ``skip_symlinks`` is True.
+        Return a list of ExtractEvent.
         """
         self.extracted_location = extracted_location
-        utils.extract_tar(
+        return utils.extract_tar(
             location=self.archive_location,
             target_dir=extracted_location,
             skip_symlinks=skip_symlinks,

src/container_inspector/rootfs.py

@@ -31,14 +31,16 @@ class InconsistentLayersError(Exception):
     pass
 
 
-def rebuild_rootfs(img, target_dir):
+def rebuild_rootfs(img, target_dir, skip_symlinks=True):
     """
     Extract and merge or "squash" all layers of the `image` Image in a single
     rootfs in `target_dir`. Extraction is done in sequence from the bottom (root
     or initial) layer to the top (or latest) layer and the "whiteouts"
     unionfs/overlayfs procedure is applied at each step as per the OCI spec:
     https://github.com/opencontainers/image-spec/blob/master/layer.md#whiteouts
 
+    Skip symlinks and links if ``skip_symlinks`` is True.
+
     Return a list of deleted "whiteout" files.
     Raise an Exception on errrors.
 
@@ -73,8 +75,15 @@ def rebuild_rootfs(img, target_dir):
         # 1. extract a layer to temp.
         # Note that we are not preserving any special file and any file permission
         extracted_loc = tempfile.mkdtemp('container_inspector-docker')
-        layer.extract(extracted_location=extracted_loc)
-        if TRACE: logger.debug(f'  Extracted layer to: {extracted_loc}')
+        # TODO: do not ignore extract events
+        _extract_events = layer.extract(
+            extracted_location=extracted_loc,
+            skip_symlinks=skip_symlinks,
+        )
+        if TRACE:
+            logger.debug(f'  Extracted layer to: {extracted_loc} with skip_symlinks: {skip_symlinks}')
+            for ev in _extract_events:
+                logger.debug(f'  {ev}')
 
         # 2. find whiteouts in that layer.
         whiteouts = list(find_whiteouts(extracted_loc))

src/container_inspector/utils.py

@@ -10,10 +10,13 @@
 import logging
 import hashlib
 import os
+import traceback
+from typing import NamedTuple
 
 from commoncode import fileutils
 
 TRACE = False
+
 logger = logging.getLogger(__name__)
 if TRACE:
     import sys
@@ -80,67 +83,88 @@ def get_labels(config, container_config):
     return dict(sorted(labels.items()))
 
 
-def extract_tar(location, target_dir, skip_symlinks=True):
+class ExtractEvent(NamedTuple):
+    """
+    Represent an extraction event of interest. These are returned when running
+    extract_tar
+    """
+
+    INFO = "info"
+    WARNING = "warning"
+    ERROR = "error"
+    # type of event: one of error, warning or info
+    type: str
+    # source path in the archive
+    source: str
+    # even message
+    message: str
+
+
+def extract_tar(location, target_dir, skip_symlinks=True, trace=TRACE):
     """
-    Extract a tar archive at `location` in the `target_dir` directory.
-    Ignore special device files. Skip symlinks and hardlinks if skip_symlinks is True.
+    Extract a tar archive at ``location`` in the ``target_dir`` directory and
+    return a list of ExtractEvent possibly empty.
+    Skip symlinks and hardlinks if skip_symlinks is True.
+
+    Ignore special device files.
     Do not preserve the permissions and owners.
-    Raise exceptions on possible problematic relative paths.
-    Issue a warning if skip_symlinks is True and links target are missing.
     """
     import tarfile
-    tarfile.TarInfo
-    if TRACE: logger.debug(f'_extract_tar: {location} to {target_dir} skip_symlinks: {skip_symlinks}')
+    if trace:
+        logger.debug(f'_extract_tar: {location} to {target_dir} skip_symlinks: {skip_symlinks}')
 
     fileutils.create_dir(target_dir)
 
+    events = []
     with tarfile.open(location) as tarball:
-        # never extract character device, block and fifo files:
-        # we extract dirs, files and links only
-        error_messages = []
+
         for tarinfo in tarball:
-            if TRACE: logger.debug(f'_extract_tar: {tarinfo}')
+            if trace:
+                logger.debug(f'extract_tar: {location!r}: {tarinfo}')
 
             if tarinfo.isdev() or tarinfo.ischr() or tarinfo.isblk() or tarinfo.isfifo() or tarinfo.sparse:
-                msg = f'_extract_tar: skipping unsupported {tarinfo} file type: block, chr, dev or sparse file'
-                error_messages.append(msg)
-                if TRACE:
-                    logger.debug(msg)
+                msg = f'skipping unsupported {tarinfo.name} file type: block, chr, dev or sparse file'
+                events.append(ExtractEvent(type=ExtractEvent.INFO, source=tarinfo.name, message=msg))
+                if trace:
+                    logger.debug(f'extract_tar: {msg}')
                 continue
 
             if '..' in tarinfo.name:
-                msg = f'_extract_tar: skipping unsupported {tarinfo} with relative path'
-                error_messages.append(msg)
-                if TRACE:
-                    logger.debug(msg)
+                msg = f'{location}: skipping unsupported {tarinfo.name} with relative path.'
+                events.append(ExtractEvent(type=ExtractEvent.WARNING, source=tarinfo.name, message=msg))
+                if trace:
+                    logger.debug(f'extract_tar: {msg}')
+                continue
+
+            if skip_symlinks and (tarinfo.islnk() or tarinfo.issym()):
+                msg = f'{location}: skipping link with skip_symlinks: {skip_symlinks}: {tarinfo.name} -> {tarinfo.linkname}'
+                if trace:
+                    logger.debug(f'extract_tar: {msg}')
                 continue
 
-            if tarinfo.islnk() or tarinfo.issym():
-                try:
-                    target = tarball._find_link_target(tarinfo)
-                    if not target:
-                        msg = f'_extract_tar: skipping link with missing target: {tarinfo}'
-                        error_messages.append(msg)
-                        if TRACE:
-                            logger.debug(msg)
-                        continue
-
-                except Exception:
-                    import traceback
-                    msg = f'_extract_tar: skipping link with missing target: {tarinfo}: {traceback.format_exc()}'
-                    error_messages.append(msg)
-                    if TRACE:
-                        logger.debug(msg)
-                    continue
+            if tarinfo.name.startswith('/'):
+                msg = f'{location}: absolute path name: {tarinfo.name} transformed in relative path.'
+                events.append(ExtractEvent(type=ExtractEvent.WARNING, source=tarinfo.name, message=msg))
+                tarinfo.name = tarinfo.name.lstrip('/')
+                if trace:
+                    logger.debug(f'extract_tar: {msg}')
 
+            # finally extract proper
             tarinfo.mode = 0o755
-            tarinfo.name = tarinfo.name.lstrip('/')
-            tarball.extract(member=tarinfo, path=target_dir, set_attrs=False,)
-        return error_messages
+
+            try:
+                tarball.extract(member=tarinfo, path=target_dir, set_attrs=False,)
+            except Exception:
+                msg = f'{location}: failed to extract: {tarinfo.name}: {traceback.format_exc()}'
+                events.append(ExtractEvent(type=ExtractEvent.ERROR, source=tarinfo.name, message=msg))
+                if trace:
+                    logger.debug(f'extract_tar: {msg}')
+
+    return events
 
 
 def extract_tar_with_symlinks(location, target_dir):
-    return extract_tar(location, target_dir, skip_symlinks=False)
+    return extract_tar(location=location, target_dir=target_dir, skip_symlinks=False)
 
 
 def lower_keys(mapping):

tests/data/utils/layer_with_links.tar

@@ -0,0 +1 @@
+[]

tests/data/utils/layer_with_links.tar.expected-events-skipping.json

@@ -0,0 +1 @@
+[]

tests/data/utils/layer_with_links.tar.expected-events.json

@@ -0,0 +1,12 @@
+[
+  "/lib",
+  "/lib/libcrypto.so.1.0.0",
+  "/lib/libssl.so.1.0.0",
+  "/usr",
+  "/usr/lib",
+  "/usr/lib/icu",
+  "/usr/lib/icu/60.2",
+  "/usr/lib/icu/60.2/Makefile.inc",
+  "/usr/lib/icu/60.2/pkgdata.inc",
+  "/usr/lib/libffi.so.6.0.4"
+]

tests/data/utils/layer_with_links.tar.expected-skipping.json

@@ -0,0 +1,20 @@
+[
+  "/lib",
+  "/lib/libcrypto.so.1.0.0",
+  "/lib/libssl.so.1.0.0",
+  "/usr",
+  "/usr/lib",
+  "/usr/lib/icu",
+  "/usr/lib/icu/60.2",
+  "/usr/lib/icu/60.2/Makefile.inc",
+  "/usr/lib/icu/60.2/pkgdata.inc",
+  "/usr/lib/icu/Makefile.inc",
+  "/usr/lib/icu/current",
+  "/usr/lib/icu/current/Makefile.inc",
+  "/usr/lib/icu/current/pkgdata.inc",
+  "/usr/lib/icu/pkgdata.inc",
+  "/usr/lib/libcrypto.so.1.0.0",
+  "/usr/lib/libffi.so.6",
+  "/usr/lib/libffi.so.6.0.4",
+  "/usr/lib/libssl.so.1.0.0"
+]