pkgme service

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

  1. lzma-support
  2. Merge into trunk
Proposed by Jonathan Lange
Status: Merged
Approved by: James Westby
Approved revision: 133
Merged at revision: 121
Proposed branch: lp:~jml/pkgme-service/lzma-support
Merge into: lp:pkgme-service
Prerequisite: lp:~jml/pkgme-service/move-utils-from-tasks
Diff against target: 177 lines (+93/-21)
2 files modified
src/djpkgme/osutils.py (+33/-21)
src/djpkgme/tests/test_osutils.py (+60/-0)
To merge this branch: bzr merge lp:~jml/pkgme-service/lzma-support
Reviewer Review Type Date Requested Status
James Westby (community) Approve
Review via email: mp+124930@code.launchpad.net

Commit message

Support .tar.lzma files.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
James Westby (james-w) wrote :

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 updated
133. By Jonathan Lange

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

Add some options to prevent certain security exploits.

Revision history for this message
Jonathan Lange (jml) wrote :

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.

Revision history for this message
James Westby (james-w) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/djpkgme/osutils.py'
2--- src/djpkgme/osutils.py 2012-09-18 12:19:18 +0000
3+++ src/djpkgme/osutils.py 2012-09-18 14:46:18 +0000
4@@ -12,25 +12,10 @@
5 import tarfile
6 import zipfile
7
8-
9-def _try_extract_tarfile(path, target_dir):
10- """Try extracting `path` to `target_dir` as a tarfile.
11-
12- :return: True if successful, False if the file at `path` isn't
13- a `tarfile`.
14- """
15- # XXX: untested.
16- try:
17- t = tarfile.open(path)
18- except tarfile.ReadError:
19- return False
20- else:
21- try:
22- t.extractall(target_dir)
23- finally:
24- t.close()
25- os.unlink(path)
26- return True
27+from pkgme.run_script import (
28+ run_subprocess,
29+ ScriptFailed,
30+ )
31
32
33 def _try_extract_zipfile(path, target_dir):
34@@ -53,10 +38,37 @@
35 return True
36
37
38+def try_extract_with_tar_command(tarball, target_dir):
39+ # The other try_extract dudes assume target_dir doesn't exist, create it,
40+ # and have it not exist when they fail. We implement the same interface.
41+ if not os.path.exists(target_dir):
42+ os.makedirs(target_dir)
43+ cmd = ['tar',
44+ '--force-local',
45+ '--no-same-owner',
46+ '--no-same-permissions',
47+ '-xf', tarball,
48+ '-C', target_dir]
49+ try:
50+ run_subprocess(cmd)
51+ except ScriptFailed, e:
52+ os.rmdir(target_dir)
53+ if e.returncode == 2:
54+ return False
55+ raise
56+ return True
57+
58+
59 # The name of the directory that we create inside of the temporary build
60 # directory that pkgme does all of its work on.
61 DEFAULT_WORKING_DIRECTORY_NAME = 'working'
62
63+_EXTRACTORS = (
64+ _try_extract_zipfile,
65+ try_extract_with_tar_command,
66+ )
67+
68+
69 def prepare_file(path, output_dir,
70 working_dir_name=DEFAULT_WORKING_DIRECTORY_NAME):
71 """Get 'path' ready for automated packaging inside 'output_dir'.
72@@ -77,8 +89,8 @@
73 :return: A path to a newly-created directory.
74 """
75 working_path = os.path.join(output_dir, working_dir_name)
76- for method in [_try_extract_tarfile, _try_extract_zipfile]:
77- if method(path, working_path):
78+ for extract in _EXTRACTORS:
79+ if extract(path, working_path):
80 break
81 else:
82 os.makedirs(working_path)
83
84=== modified file 'src/djpkgme/tests/test_osutils.py'
85--- src/djpkgme/tests/test_osutils.py 2012-09-18 12:19:18 +0000
86+++ src/djpkgme/tests/test_osutils.py 2012-09-18 14:46:18 +0000
87@@ -1,5 +1,6 @@
88 import errno
89 import os
90+import subprocess
91 import tarfile
92 import zipfile
93
94@@ -9,17 +10,60 @@
95 DirExists,
96 FileContains,
97 HasPermissions,
98+ Not,
99 TarballContains,
100 )
101+from treeshape import (
102+ from_rough_spec,
103+ HasFileTree,
104+ FileTree,
105+ MatchesFileTree,
106+ )
107
108 from ..osutils import (
109 DEFAULT_WORKING_DIRECTORY_NAME,
110 make_tarball,
111 prepare_file,
112+ try_extract_with_tar_command,
113 )
114 from .factory import PkgmeObjectFactory
115
116
117+class TestExtractWithTarCommand(TestCase):
118+
119+ def test_extracts_tarball(self):
120+ factory = self.useFixture(PkgmeObjectFactory())
121+ path = factory.make_tarball(self.useFixture(TempDir()).path)
122+ tarball = tarfile.open(path)
123+ expected_top_level = [tarball.getnames()[0] + '/']
124+ tempdir = self.useFixture(TempDir()).path
125+ success = try_extract_with_tar_command(path, tempdir)
126+ self.assertEqual(True, success)
127+ self.assertThat(
128+ tempdir, HasFileTree(from_rough_spec(expected_top_level)))
129+
130+ def test_non_existent_target_dir(self):
131+ factory = self.useFixture(PkgmeObjectFactory())
132+ path = factory.make_tarball(self.useFixture(TempDir()).path)
133+ tarball = tarfile.open(path)
134+ expected_top_level = [tarball.getnames()[0] + '/']
135+ tempdir = self.useFixture(FileTree({}))
136+ success = try_extract_with_tar_command(
137+ path, tempdir.join('nonexistent'))
138+ self.assertEqual(True, success)
139+ self.assertThat(
140+ tempdir.join('nonexistent'),
141+ HasFileTree(from_rough_spec(expected_top_level)))
142+
143+ def test_does_not_extract_non_tarball(self):
144+ filename = 'afile'
145+ tree = self.useFixture(FileTree({filename: {}}))
146+ tempdir = self.useFixture(TempDir()).path
147+ success = try_extract_with_tar_command(tree.join(filename), tempdir)
148+ self.assertEqual(False, success)
149+ self.assertThat(tempdir, Not(DirExists()))
150+
151+
152 class TestPrepareFile(TestCase):
153
154 def test_extracts_tarball(self):
155@@ -53,6 +97,22 @@
156 self.assertThat(new_path, DirExists())
157 self.assertThat(os.path.join(new_path, top_level_dir), DirExists())
158
159+ def test_extracts_lzma(self):
160+ tempdir = self.useFixture(TempDir()).path
161+ lzma_path = os.path.join(tempdir, 'application.tar.lzma')
162+ tree = {'topdir/afile': {}, 'topdir/somedir/morefiles': {}}
163+ orig_data = self.useFixture(FileTree(tree)).path
164+ popen = subprocess.Popen(
165+ ['tar', '--lzma', '-cf', lzma_path, '-C', orig_data, 'topdir'],
166+ stdout=subprocess.PIPE, stderr=subprocess.PIPE)
167+ out, err = popen.communicate()
168+ self.assertEqual((0, '', ''), (popen.returncode, out, err))
169+ new_path = prepare_file(lzma_path, tempdir)
170+ self.assertEqual(
171+ os.path.join(tempdir, DEFAULT_WORKING_DIRECTORY_NAME),
172+ new_path)
173+ self.assertThat(new_path, MatchesFileTree(tree))
174+
175 def test_copies_non_tarball(self):
176 # If the file downloaded is not a tarball then it is copied
177 source_dir = self.useFixture(TempDir()).path

Subscribers

People subscribed via source and target branches