Merge lp:~jml/pkgme-service/lzma-support into lp:pkgme-service

[Jonathan Lange]

Support .tar.lzma files.

Adds a thing to try to use the 'tar' command to extract what we're given. This is only used as a fallback after the existing tarfile and zipfile extractors, since I'm assuming that spawning subprocesses is more expensive.

This gives us lzma support, as tar automatically detects compression to use and as lzma support is available on any precise machine, due to apt depending on xz-utils, which provides it.

There are direct tests, as well as a test for prepare_file.

[James Westby (james-w)]

Hi,

In execing tar there are a few things we have to be careful of to prevent
attacks.

--force-local will mean that if someone uploads a tarball with a colon in the name
it won't try and connect to a remote machine to find the file.

We want --no-same-owner and --no-same-permissions, but they are default for
non-root users, so I don't know if we want to be explicit or not.

In addition, I think this will fix some issues we see in scoreboard with
e.g. unable to process tarfiles, or trying to extract to absolute paths.

Rather than fixing those, we could just replace the existing tarfile extractor
with this one. You are right that it is more expensive though, so maybe we
don't want to.

Thanks,

James

review: Needs Fixing

[lp:~jml/pkgme-service/lzma-support]

133. By Jonathan Lange on 2012-09-18

Only use 'tar' command, rather than Python's tarfile to try to extract
tarballs.

Add some options to prevent certain security exploits.

[Jonathan Lange (jml)]

We haven't demonstrated that the greater expense matters. We have demonstrated that the errors do. Have deleted the old _try_extract_tarfile. We now rely only on try_extract_with_tar_command.

Have added the extra options for defense-in-depth.

[James Westby (james-w)]

review: Approve

=== modified file 'src/djpkgme/osutils.py'
--- src/djpkgme/osutils.py	2012-09-18 12:19:18 +0000
+++ src/djpkgme/osutils.py	2012-09-18 14:46:18 +0000
@@ -12,25 +12,10 @@
 import tarfile
 import zipfile
 
-
-def _try_extract_tarfile(path, target_dir):
-    """Try extracting `path` to `target_dir` as a tarfile.
-
-    :return: True if successful, False if the file at `path` isn't
-             a `tarfile`.
-    """
-    # XXX: untested.
-    try:
-        t = tarfile.open(path)
-    except tarfile.ReadError:
-        return False
-    else:
-        try:
-            t.extractall(target_dir)
-        finally:
-            t.close()
-        os.unlink(path)
-    return True
+from pkgme.run_script import (
+    run_subprocess,
+    ScriptFailed,
+    )
 
 
 def _try_extract_zipfile(path, target_dir):
@@ -53,10 +38,37 @@
     return True
 
 
+def try_extract_with_tar_command(tarball, target_dir):
+    # The other try_extract dudes assume target_dir doesn't exist, create it,
+    # and have it not exist when they fail.  We implement the same interface.
+    if not os.path.exists(target_dir):
+        os.makedirs(target_dir)
+    cmd = ['tar',
+           '--force-local',
+           '--no-same-owner',
+           '--no-same-permissions',
+           '-xf', tarball,
+           '-C', target_dir]
+    try:
+        run_subprocess(cmd)
+    except ScriptFailed, e:
+        os.rmdir(target_dir)
+        if e.returncode == 2:
+            return False
+        raise
+    return True
+
+
 # The name of the directory that we create inside of the temporary build
 # directory that pkgme does all of its work on.
 DEFAULT_WORKING_DIRECTORY_NAME = 'working'
 
+_EXTRACTORS = (
+    _try_extract_zipfile,
+    try_extract_with_tar_command,
+    )
+
+
 def prepare_file(path, output_dir,
                  working_dir_name=DEFAULT_WORKING_DIRECTORY_NAME):
     """Get 'path' ready for automated packaging inside 'output_dir'.
@@ -77,8 +89,8 @@
     :return: A path to a newly-created directory.
     """
     working_path = os.path.join(output_dir, working_dir_name)
-    for method in [_try_extract_tarfile, _try_extract_zipfile]:
-        if method(path, working_path):
+    for extract in _EXTRACTORS:
+        if extract(path, working_path):
             break
     else:
         os.makedirs(working_path)
 
=== modified file 'src/djpkgme/tests/test_osutils.py'
--- src/djpkgme/tests/test_osutils.py	2012-09-18 12:19:18 +0000
+++ src/djpkgme/tests/test_osutils.py	2012-09-18 14:46:18 +0000
@@ -1,5 +1,6 @@
 import errno
 import os
+import subprocess
 import tarfile
 import zipfile
 
@@ -9,17 +10,60 @@
     DirExists,
     FileContains,
     HasPermissions,
+    Not,
     TarballContains,
     )
+from treeshape import (
+    from_rough_spec,
+    HasFileTree,
+    FileTree,
+    MatchesFileTree,
+    )
 
 from ..osutils import (
     DEFAULT_WORKING_DIRECTORY_NAME,
     make_tarball,
     prepare_file,
+    try_extract_with_tar_command,
     )
 from .factory import PkgmeObjectFactory
 
 
+class TestExtractWithTarCommand(TestCase):
+
+    def test_extracts_tarball(self):
+        factory = self.useFixture(PkgmeObjectFactory())
+        path = factory.make_tarball(self.useFixture(TempDir()).path)
+        tarball = tarfile.open(path)
+        expected_top_level = [tarball.getnames()[0] + '/']
+        tempdir = self.useFixture(TempDir()).path
+        success = try_extract_with_tar_command(path, tempdir)
+        self.assertEqual(True, success)
+        self.assertThat(
+            tempdir, HasFileTree(from_rough_spec(expected_top_level)))
+
+    def test_non_existent_target_dir(self):
+        factory = self.useFixture(PkgmeObjectFactory())
+        path = factory.make_tarball(self.useFixture(TempDir()).path)
+        tarball = tarfile.open(path)
+        expected_top_level = [tarball.getnames()[0] + '/']
+        tempdir = self.useFixture(FileTree({}))
+        success = try_extract_with_tar_command(
+            path, tempdir.join('nonexistent'))
+        self.assertEqual(True, success)
+        self.assertThat(
+            tempdir.join('nonexistent'),
+            HasFileTree(from_rough_spec(expected_top_level)))
+
+    def test_does_not_extract_non_tarball(self):
+        filename = 'afile'
+        tree = self.useFixture(FileTree({filename: {}}))
+        tempdir = self.useFixture(TempDir()).path
+        success = try_extract_with_tar_command(tree.join(filename), tempdir)
+        self.assertEqual(False, success)
+        self.assertThat(tempdir, Not(DirExists()))
+
+
 class TestPrepareFile(TestCase):
 
     def test_extracts_tarball(self):
@@ -53,6 +97,22 @@
         self.assertThat(new_path, DirExists())
         self.assertThat(os.path.join(new_path, top_level_dir), DirExists())
 
+    def test_extracts_lzma(self):
+        tempdir = self.useFixture(TempDir()).path
+        lzma_path = os.path.join(tempdir, 'application.tar.lzma')
+        tree = {'topdir/afile': {}, 'topdir/somedir/morefiles': {}}
+        orig_data = self.useFixture(FileTree(tree)).path
+        popen = subprocess.Popen(
+            ['tar', '--lzma', '-cf', lzma_path, '-C', orig_data, 'topdir'],
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        out, err = popen.communicate()
+        self.assertEqual((0, '', ''), (popen.returncode, out, err))
+        new_path = prepare_file(lzma_path, tempdir)
+        self.assertEqual(
+            os.path.join(tempdir, DEFAULT_WORKING_DIRECTORY_NAME),
+            new_path)
+        self.assertThat(new_path, MatchesFileTree(tree))
+
     def test_copies_non_tarball(self):
         # If the file downloaded is not a tarball then it is copied
         source_dir = self.useFixture(TempDir()).path