Merge lp:~snappy-dev/click/default-apparmor into lp:~snappy-dev/click/snappy

Proposed by Barry Warsaw on 2014-12-03
Status: Needs review
Proposed branch: lp:~snappy-dev/click/default-apparmor
Merge into: lp:~snappy-dev/click/snappy
Diff against target: 345 lines (+254/-25)
2 files modified
click/build.py (+33/-5)
click/tests/test_build.py (+221/-20)
To merge this branch: bzr merge lp:~snappy-dev/click/default-apparmor
Reviewer Review Type Date Requested Status
Michael Vogt 2014-12-03 Approve on 2014-12-04
Jamie Strandboge 2014-12-03 Pending
Review via email: mp+243609@code.launchpad.net

Description of the change

snappy build needs to generate default apparmor profile automatically

To post a comment you must log in.
Jamie Strandboge (jdstrand) wrote :

Without testing the code, this looks reasonable. I can say that the hardcoding of the policy_version is something we won't want long term. We'll need to have click-apparmor expose that some how so it can be consumed by click.

Jamie Strandboge (jdstrand) wrote :

Do you have debs available for testing?

Barry Warsaw (barry) wrote :

> Without testing the code, this looks reasonable. I can say that the hardcoding
> of the policy_version is something we won't want long term. We'll need to have
> click-apparmor expose that some how so it can be consumed by click.

Will the rest of the file contents potentially change, or just the policy_version? If it's just the latter and we can determine the version at build-time, we can easily substitute that in. Actually, if the entire file contents is available somewhere we can pretty easily read that in at build time.

Barry Warsaw (barry) wrote :

> Do you have debs available for testing?

Not yet. I was hoping that once Michael reviews he can set that up.

Michael Vogt (mvo) wrote :

Looks great, thanks! And extra credits for the tests, yay! I merged it into the yaml-manifest branch and it will go to the PPA next, I did a tiny change to name APPARMOR_DEFAULT_TEMPLATE -> SNAPPY_APPARMOR_DEFAULT_TEMPLATE

review: Approve
Michael Vogt (mvo) wrote :

I just uploaded the new click to the ppa:snappy-dev/tools PPA so it should be ready for testing RSN. I did a manual test with my xkcd-webserver example and it worked there, I will do another one with the golang example next.

Jamie Strandboge (jdstrand) wrote :

On 12/03/2014 06:41 PM, Barry Warsaw wrote:
>> Without testing the code, this looks reasonable. I can say that the hardcoding
>> of the policy_version is something we won't want long term. We'll need to have
>> click-apparmor expose that some how so it can be consumed by click.
>
> Will the rest of the file contents potentially change, or just the policy_version?
> If it's just the latter and we can determine the version at build-time, we can
> easily substitute that in. Actually, if the entire file contents is available
> somewhere we can pretty easily read that in at build time.

Well,the contents of the file are up to us-- we can define the defaults. Dealing
with the policy_version requires further thought and discussion since it has
been changing based on the Ubuntu release (eg, 1.2 is 14.10, 1.1 14.04). That is
just convention though-- there is no reason why it has to be that way, but it
will definitely change if we want to remove policy groups or templates, or if
new policy groups and templates need to use new apparmor syntax.

--
Jamie Strandboge | http://www.ubuntu.com

Unmerged revisions

574. By Barry Warsaw on 2014-12-03

Create default apparmor profiles if the yaml doesn't explicitly name
integration:<app>:apparmor{,-profile}

With tests!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'click/build.py'
2--- click/build.py 2014-12-03 15:46:34 +0000
3+++ click/build.py 2014-12-03 23:21:56 +0000
4@@ -62,6 +62,18 @@
5 )
6
7
8+APPARMOR_DEFAULT_TEMPLATE = """\
9+{
10+ "template": "default",
11+ "policy_groups": [
12+ "networking"
13+ ],
14+ "policy_vendor": "ubuntu-snappy",
15+ "policy_version": 1.3
16+}
17+"""
18+
19+
20 @contextlib.contextmanager
21 def make_temp_dir():
22 temp_dir = tempfile.mkdtemp(prefix="click")
23@@ -89,6 +101,7 @@
24 class ClickBuilderBase:
25 def __init__(self):
26 self.file_map = {}
27+ self.autogen_apparmor = []
28
29 def add_file(self, source_path, dest_path):
30 self.file_map[source_path] = dest_path
31@@ -136,18 +149,28 @@
32 # deal with new "services" field
33 hooks = self.manifest.get("hooks", {})
34 for service in self.manifest.get("services", []):
35- current_hook = hooks.get(service["name"], {})
36- snappy_systemd = "%s.snappy-systemd" % service["name"]
37+ name = service["name"]
38+ current_hook = hooks.get(name, {})
39+ snappy_systemd = "{}.snappy-systemd".format(name)
40 description = service.get(
41 "description",
42 self.manifest.get("description", "no description"))
43- with open(os.path.join(
44- os.path.dirname(manifest_path), snappy_systemd), "w") as fp:
45+ systemd_path = os.path.join(
46+ os.path.dirname(manifest_path), snappy_systemd)
47+ with open(systemd_path, "w") as fp:
48 yaml.dump({"description": description,
49 "start": service["start"],
50 }, fp)
51 current_hook["snappy-systemd"] = "meta/"+snappy_systemd
52- hooks[service["name"]] = current_hook
53+ hooks[name] = current_hook
54+ # Is there an explicit apparmor hook? If so, use it otherwise
55+ # generate a default one.
56+ if ( 'apparmor' not in current_hook and
57+ 'apparmor-profile' not in current_hook):
58+ template_path = os.path.join(
59+ 'meta', '{}.apparmor'.format(name))
60+ current_hook['apparmor'] = template_path
61+ self.autogen_apparmor.append(template_path)
62 self.manifest["hooks"] = hooks
63 # cleanup
64 if "services" in self.manifest:
65@@ -316,6 +339,11 @@
66 if "framework" in self.manifest:
67 self._validate_framework(self.manifest["framework"])
68
69+ for template_path in self.autogen_apparmor:
70+ path = os.path.join(root_path, template_path)
71+ with io.open(path, 'w', encoding='utf-8') as fp:
72+ fp.write(APPARMOR_DEFAULT_TEMPLATE)
73+
74 du_output = subprocess.check_output(
75 ["du", "-k", "-s", "--apparent-size", "."],
76 cwd=temp_dir, universal_newlines=True).rstrip("\n")
77
78=== modified file 'click/tests/test_build.py'
79--- click/tests/test_build.py 2014-12-02 15:34:43 +0000
80+++ click/tests/test_build.py 2014-12-03 23:21:56 +0000
81@@ -342,31 +342,82 @@
82 self.assertTrue(tar.getmember("./bin/foo").isfile())
83
84
85+SIMPLE_TEMPLATE = """\
86+name: test-yaml
87+icon: data/icon.svg
88+version: 1.0
89+maintainer: Foo Bar <foo@example.org>
90+architecture: all
91+framework: ubuntu-core-15.04
92+integration:
93+ app1:
94+ hookname: hook-file-name
95+services:
96+ - name: app1
97+ description: some description
98+ start: start-something
99+ stop: stop-something
100+binaries:
101+ - name: path/to/cli1
102+"""
103+
104+APPARMOR_1_TEMPLATE = """\
105+name: test-yaml
106+icon: data/icon.svg
107+version: 1.0
108+maintainer: Foo Bar <foo@example.org>
109+architecture: all
110+framework: ubuntu-core-15.04
111+integration:
112+ app1:
113+ hookname: hook-file-name
114+ apparmor: path/to/app1.apparmor
115+services:
116+ - name: app1
117+ description: some description
118+ start: start-something
119+ stop: stop-something
120+ - name: app2
121+ description: some description
122+ start: start-something
123+ stop: stop-something
124+binaries:
125+ - name: path/to/cli1
126+"""
127+
128+APPARMOR_2_TEMPLATE = """\
129+name: test-yaml
130+icon: data/icon.svg
131+version: 1.0
132+maintainer: Foo Bar <foo@example.org>
133+architecture: all
134+framework: ubuntu-core-15.04
135+integration:
136+ app1:
137+ hookname: hook-file-name
138+ apparmor-profile: path/to/app1.apparmor-profile
139+services:
140+ - name: app1
141+ description: some description
142+ start: start-something
143+ stop: stop-something
144+ - name: app2
145+ description: some description
146+ start: start-something
147+ stop: stop-something
148+binaries:
149+ - name: path/to/cli1
150+"""
151+
152+
153 class TestClickBuildYaml(TestCase):
154 def setUp(self):
155 super(TestClickBuildYaml, self).setUp()
156 self.builder = ClickBuilder()
157
158- def _make_metadata(self, scratch):
159+ def _make_metadata(self, scratch, template=SIMPLE_TEMPLATE):
160 with mkfile(os.path.join(scratch, "meta", "package.yaml")) as f:
161- f.write(dedent("""\
162- name: test-yaml
163- icon: data/icon.svg
164- version: 1.0
165- maintainer: Foo Bar <foo@example.org>
166- architecture: all
167- framework: ubuntu-core-15.04
168- integration:
169- app1:
170- hookname: hook-file-name
171- services:
172- - name: app1
173- description: some description
174- start: start-something
175- stop: stop-something
176- binaries:
177- - name: path/to/cli1
178- """))
179+ f.write(dedent(template))
180 with mkfile(os.path.join(scratch, "meta", "readme.md")) as f:
181 f.write(dedent("""test title
182
183@@ -401,7 +452,8 @@
184 click_contents = subprocess.check_output(
185 ["dpkg-deb", "-c", actual_path], universal_newlines=True)
186 for file_path in ("./data/icon.svg", "./meta/package.yaml",
187- "./meta/readme.md", "./bin/foo"):
188+ "./meta/readme.md", "./bin/foo",
189+ "./meta/app1.apparmor"):
190 self.assertIn(file_path, click_contents)
191
192 def test_read_manifest(self):
193@@ -422,3 +474,152 @@
194 self.assertEqual(
195 "path/to/cli1",
196 self.builder.manifest["hooks"]["cli1"]["bin-path"])
197+ self.assertEqual(
198+ "meta/app1.apparmor",
199+ self.builder.manifest["hooks"]["app1"]["apparmor"])
200+
201+ def test_one_explicit_apparmor(self):
202+ self.use_temp_dir()
203+ self._make_metadata(self.temp_dir, APPARMOR_1_TEMPLATE)
204+ self.builder.read_manifest(self.temp_dir + "/meta/package.yaml")
205+ self.assertEqual(
206+ "path/to/app1.apparmor",
207+ self.builder.manifest["hooks"]["app1"]["apparmor"])
208+ self.assertEqual(
209+ "meta/app2.apparmor",
210+ self.builder.manifest["hooks"]["app2"]["apparmor"])
211+
212+ def test_one_explicit_apparmor_profile(self):
213+ self.use_temp_dir()
214+ self._make_metadata(self.temp_dir, APPARMOR_2_TEMPLATE)
215+ self.builder.read_manifest(self.temp_dir + "/meta/package.yaml")
216+ self.assertEqual(
217+ "path/to/app1.apparmor-profile",
218+ self.builder.manifest["hooks"]["app1"]["apparmor-profile"])
219+ self.assertIsNone(
220+ self.builder.manifest["hooks"]["app1"].get('apparmor'))
221+ self.assertEqual(
222+ "meta/app2.apparmor",
223+ self.builder.manifest["hooks"]["app2"]["apparmor"])
224+
225+ def test_one_explicit_apparmor_file(self):
226+ self.use_temp_dir()
227+ scratch = os.path.join(self.temp_dir, "scratch")
228+ touch(os.path.join(scratch, "bin", "foo"))
229+ touch(os.path.join(scratch, "data", "icon.svg"))
230+ apparmor_path = os.path.join(scratch, 'path', 'to', 'app1.apparmor')
231+ os.makedirs(os.path.dirname(apparmor_path))
232+ with open(apparmor_path, 'w', encoding='utf-8') as fp:
233+ fp.write(dedent("""\
234+ {
235+ "template": "default",
236+ "policy_groups": [
237+ "networking"
238+ ],
239+ "policy_vendor": "ubuntu-snappy",
240+ "policy_version": 99.99
241+ }
242+ """))
243+ self._make_metadata(scratch, APPARMOR_1_TEMPLATE)
244+ self.builder.add_file(scratch, "./")
245+ # FIXME: make it a .snap
246+ expected_path = os.path.join(self.temp_dir, "test-yaml_1.0_all.click")
247+ actual_path = self.builder.build(self.temp_dir, "meta/package.yaml")
248+ self.assertEqual(expected_path, actual_path)
249+ self.assertTrue(os.path.exists(actual_path))
250+ contents = os.path.join(self.temp_dir, 'contents')
251+ subprocess.check_output(
252+ ["dpkg-deb", "-R", actual_path, contents], universal_newlines=True)
253+ self.assertEqual(sorted(os.listdir(contents)),
254+ ['DEBIAN', 'bin', 'data', 'meta', 'path'])
255+ self.assertEqual(
256+ sorted(os.listdir(os.path.join(contents, 'path', 'to'))),
257+ ['app1.apparmor'])
258+ with open(os.path.join(contents, 'path', 'to', 'app1.apparmor'),
259+ encoding='utf-8') as fp:
260+ data = fp.read()
261+ self.assertEqual(data, dedent("""\
262+ {
263+ "template": "default",
264+ "policy_groups": [
265+ "networking"
266+ ],
267+ "policy_vendor": "ubuntu-snappy",
268+ "policy_version": 99.99
269+ }
270+ """))
271+ with open(os.path.join(contents, 'meta', 'app2.apparmor'),
272+ encoding='utf-8') as fp:
273+ data = fp.read()
274+ self.assertEqual(data, dedent("""\
275+ {
276+ "template": "default",
277+ "policy_groups": [
278+ "networking"
279+ ],
280+ "policy_vendor": "ubuntu-snappy",
281+ "policy_version": 1.3
282+ }
283+ """))
284+
285+ def test_one_explicit_apparmor_profile_file(self):
286+ self.use_temp_dir()
287+ scratch = os.path.join(self.temp_dir, "scratch")
288+ touch(os.path.join(scratch, "bin", "foo"))
289+ touch(os.path.join(scratch, "data", "icon.svg"))
290+ apparmor_path = os.path.join(
291+ scratch, 'path', 'to', 'app1.apparmor-profile')
292+ os.makedirs(os.path.dirname(apparmor_path))
293+ with open(apparmor_path, 'w', encoding='utf-8') as fp:
294+ fp.write(dedent("""\
295+ {
296+ "template": "default",
297+ "policy_groups": [
298+ "networking"
299+ ],
300+ "policy_vendor": "ubuntu-snappy",
301+ "policy_version": 88.88
302+ }
303+ """))
304+ self._make_metadata(scratch, APPARMOR_2_TEMPLATE)
305+ self.builder.add_file(scratch, "./")
306+ # FIXME: make it a .snap
307+ expected_path = os.path.join(self.temp_dir, "test-yaml_1.0_all.click")
308+ actual_path = self.builder.build(self.temp_dir, "meta/package.yaml")
309+ self.assertEqual(expected_path, actual_path)
310+ self.assertTrue(os.path.exists(actual_path))
311+ contents = os.path.join(self.temp_dir, 'contents')
312+ subprocess.check_output(
313+ ["dpkg-deb", "-R", actual_path, contents], universal_newlines=True)
314+ self.assertEqual(sorted(os.listdir(contents)),
315+ ['DEBIAN', 'bin', 'data', 'meta', 'path'])
316+ self.assertEqual(
317+ sorted(os.listdir(os.path.join(contents, 'path', 'to'))),
318+ ['app1.apparmor-profile'])
319+ with open(
320+ os.path.join(contents, 'path', 'to', 'app1.apparmor-profile'),
321+ encoding='utf-8') as fp:
322+ data = fp.read()
323+ self.assertEqual(data, dedent("""\
324+ {
325+ "template": "default",
326+ "policy_groups": [
327+ "networking"
328+ ],
329+ "policy_vendor": "ubuntu-snappy",
330+ "policy_version": 88.88
331+ }
332+ """))
333+ with open(os.path.join(contents, 'meta', 'app2.apparmor'),
334+ encoding='utf-8') as fp:
335+ data = fp.read()
336+ self.assertEqual(data, dedent("""\
337+ {
338+ "template": "default",
339+ "policy_groups": [
340+ "networking"
341+ ],
342+ "policy_vendor": "ubuntu-snappy",
343+ "policy_version": 1.3
344+ }
345+ """))

Subscribers

People subscribed via source and target branches

to all changes: