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

[Barry Warsaw]

snappy build needs to generate default apparmor profile automatically

[Jamie Strandboge (jdstrand)]

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)]

Do you have debs available for testing?

[Barry Warsaw (barry)]

> 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)]

> Do you have debs available for testing?

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

[Michael Vogt (mvo)]

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)]

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)]

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!

=== modified file 'click/build.py'
--- click/build.py	2014-12-03 15:46:34 +0000
+++ click/build.py	2014-12-03 23:21:56 +0000
@@ -62,6 +62,18 @@
 )
 
 
+APPARMOR_DEFAULT_TEMPLATE = """\
+{
+    "template": "default",
+    "policy_groups": [
+        "networking"
+    ],
+    "policy_vendor": "ubuntu-snappy",
+    "policy_version": 1.3
+}
+"""
+
+
 @contextlib.contextmanager
 def make_temp_dir():
     temp_dir = tempfile.mkdtemp(prefix="click")
@@ -89,6 +101,7 @@
 class ClickBuilderBase:
     def __init__(self):
         self.file_map = {}
+        self.autogen_apparmor = []
 
     def add_file(self, source_path, dest_path):
         self.file_map[source_path] = dest_path
@@ -136,18 +149,28 @@
         # deal with new "services" field
         hooks = self.manifest.get("hooks", {})
         for service in self.manifest.get("services", []):
-            current_hook = hooks.get(service["name"], {})
-            snappy_systemd = "%s.snappy-systemd" % service["name"]
+            name = service["name"]
+            current_hook = hooks.get(name, {})
+            snappy_systemd = "{}.snappy-systemd".format(name)
             description = service.get(
                 "description",
                 self.manifest.get("description", "no description"))
-            with open(os.path.join(
-                    os.path.dirname(manifest_path), snappy_systemd), "w") as fp:
+            systemd_path = os.path.join(
+                os.path.dirname(manifest_path), snappy_systemd)
+            with open(systemd_path, "w") as fp:
                 yaml.dump({"description": description,
                            "start": service["start"],
                           }, fp)
             current_hook["snappy-systemd"] = "meta/"+snappy_systemd
-            hooks[service["name"]] = current_hook
+            hooks[name] = current_hook
+            # Is there an explicit apparmor hook?  If so, use it otherwise
+            # generate a default one.
+            if (    'apparmor' not in current_hook and
+                    'apparmor-profile' not in current_hook):
+                template_path = os.path.join(
+                    'meta', '{}.apparmor'.format(name))
+                current_hook['apparmor'] = template_path
+                self.autogen_apparmor.append(template_path)
         self.manifest["hooks"] = hooks
         # cleanup
         if "services" in self.manifest:
@@ -316,6 +339,11 @@
             if "framework" in self.manifest:
                 self._validate_framework(self.manifest["framework"])
 
+            for template_path in self.autogen_apparmor:
+                path = os.path.join(root_path, template_path)
+                with io.open(path, 'w', encoding='utf-8') as fp:
+                    fp.write(APPARMOR_DEFAULT_TEMPLATE)
+
             du_output = subprocess.check_output(
                 ["du", "-k", "-s", "--apparent-size", "."],
                 cwd=temp_dir, universal_newlines=True).rstrip("\n")
 
=== modified file 'click/tests/test_build.py'
--- click/tests/test_build.py	2014-12-02 15:34:43 +0000
+++ click/tests/test_build.py	2014-12-03 23:21:56 +0000
@@ -342,31 +342,82 @@
             self.assertTrue(tar.getmember("./bin/foo").isfile())
 
 
+SIMPLE_TEMPLATE = """\
+name: test-yaml
+icon: data/icon.svg
+version: 1.0
+maintainer: Foo Bar <[e-mail]>
+architecture: all
+framework: ubuntu-core-15.04
+integration:
+  app1:
+    hookname: hook-file-name
+services:
+  - name: app1
+    description: some description
+    start: start-something
+    stop: stop-something
+binaries:
+  - name: path/to/cli1
+"""
+
+APPARMOR_1_TEMPLATE = """\
+name: test-yaml
+icon: data/icon.svg
+version: 1.0
+maintainer: Foo Bar <[e-mail]>
+architecture: all
+framework: ubuntu-core-15.04
+integration:
+  app1:
+    hookname: hook-file-name
+    apparmor: path/to/app1.apparmor
+services:
+  - name: app1
+    description: some description
+    start: start-something
+    stop: stop-something
+  - name: app2
+    description: some description
+    start: start-something
+    stop: stop-something
+binaries:
+  - name: path/to/cli1
+"""
+
+APPARMOR_2_TEMPLATE = """\
+name: test-yaml
+icon: data/icon.svg
+version: 1.0
+maintainer: Foo Bar <[e-mail]>
+architecture: all
+framework: ubuntu-core-15.04
+integration:
+  app1:
+    hookname: hook-file-name
+    apparmor-profile: path/to/app1.apparmor-profile
+services:
+  - name: app1
+    description: some description
+    start: start-something
+    stop: stop-something
+  - name: app2
+    description: some description
+    start: start-something
+    stop: stop-something
+binaries:
+  - name: path/to/cli1
+"""
+
+
 class TestClickBuildYaml(TestCase):
     def setUp(self):
         super(TestClickBuildYaml, self).setUp()
         self.builder = ClickBuilder()
 
-    def _make_metadata(self, scratch):
+    def _make_metadata(self, scratch, template=SIMPLE_TEMPLATE):
         with mkfile(os.path.join(scratch, "meta", "package.yaml")) as f:
-            f.write(dedent("""\
-            name: test-yaml
-            icon: data/icon.svg
-            version: 1.0
-            maintainer: Foo Bar <[e-mail]>
-            architecture: all
-            framework: ubuntu-core-15.04
-            integration:
-              app1:
-                hookname: hook-file-name
-            services:
-              - name: app1
-                description: some description
-                start: start-something
-                stop: stop-something
-            binaries:
-              - name: path/to/cli1
-            """))
+            f.write(dedent(template))
         with mkfile(os.path.join(scratch, "meta", "readme.md")) as f:
             f.write(dedent("""test title
 
@@ -401,7 +452,8 @@
         click_contents = subprocess.check_output(
             ["dpkg-deb", "-c", actual_path], universal_newlines=True)
         for file_path in ("./data/icon.svg", "./meta/package.yaml",
-                     "./meta/readme.md", "./bin/foo"):
+                          "./meta/readme.md", "./bin/foo",
+                          "./meta/app1.apparmor"):
             self.assertIn(file_path, click_contents)
 
     def test_read_manifest(self):
@@ -422,3 +474,152 @@
         self.assertEqual(
             "path/to/cli1",
             self.builder.manifest["hooks"]["cli1"]["bin-path"])
+        self.assertEqual(
+            "meta/app1.apparmor",
+            self.builder.manifest["hooks"]["app1"]["apparmor"])
+
+    def test_one_explicit_apparmor(self):
+        self.use_temp_dir()
+        self._make_metadata(self.temp_dir, APPARMOR_1_TEMPLATE)
+        self.builder.read_manifest(self.temp_dir + "/meta/package.yaml")
+        self.assertEqual(
+            "path/to/app1.apparmor",
+            self.builder.manifest["hooks"]["app1"]["apparmor"])
+        self.assertEqual(
+            "meta/app2.apparmor",
+            self.builder.manifest["hooks"]["app2"]["apparmor"])
+
+    def test_one_explicit_apparmor_profile(self):
+        self.use_temp_dir()
+        self._make_metadata(self.temp_dir, APPARMOR_2_TEMPLATE)
+        self.builder.read_manifest(self.temp_dir + "/meta/package.yaml")
+        self.assertEqual(
+            "path/to/app1.apparmor-profile",
+            self.builder.manifest["hooks"]["app1"]["apparmor-profile"])
+        self.assertIsNone(
+            self.builder.manifest["hooks"]["app1"].get('apparmor'))
+        self.assertEqual(
+            "meta/app2.apparmor",
+            self.builder.manifest["hooks"]["app2"]["apparmor"])
+
+    def test_one_explicit_apparmor_file(self):
+        self.use_temp_dir()
+        scratch = os.path.join(self.temp_dir, "scratch")
+        touch(os.path.join(scratch, "bin", "foo"))
+        touch(os.path.join(scratch, "data", "icon.svg"))
+        apparmor_path = os.path.join(scratch, 'path', 'to', 'app1.apparmor')
+        os.makedirs(os.path.dirname(apparmor_path))
+        with open(apparmor_path, 'w', encoding='utf-8') as fp:
+            fp.write(dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 99.99
+            }
+            """))
+        self._make_metadata(scratch, APPARMOR_1_TEMPLATE)
+        self.builder.add_file(scratch, "./")
+        # FIXME: make it a .snap
+        expected_path = os.path.join(self.temp_dir, "test-yaml_1.0_all.click")
+        actual_path = self.builder.build(self.temp_dir, "meta/package.yaml")
+        self.assertEqual(expected_path, actual_path)
+        self.assertTrue(os.path.exists(actual_path))
+        contents = os.path.join(self.temp_dir, 'contents')
+        subprocess.check_output(
+            ["dpkg-deb", "-R", actual_path, contents], universal_newlines=True)
+        self.assertEqual(sorted(os.listdir(contents)),
+                         ['DEBIAN', 'bin', 'data', 'meta', 'path'])
+        self.assertEqual(
+            sorted(os.listdir(os.path.join(contents, 'path', 'to'))),
+            ['app1.apparmor'])
+        with open(os.path.join(contents, 'path', 'to', 'app1.apparmor'),
+                  encoding='utf-8') as fp:
+            data = fp.read()
+        self.assertEqual(data, dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 99.99
+            }
+            """))
+        with open(os.path.join(contents, 'meta', 'app2.apparmor'),
+                  encoding='utf-8') as fp:
+            data = fp.read()
+        self.assertEqual(data, dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 1.3
+            }
+            """))
+
+    def test_one_explicit_apparmor_profile_file(self):
+        self.use_temp_dir()
+        scratch = os.path.join(self.temp_dir, "scratch")
+        touch(os.path.join(scratch, "bin", "foo"))
+        touch(os.path.join(scratch, "data", "icon.svg"))
+        apparmor_path = os.path.join(
+            scratch, 'path', 'to', 'app1.apparmor-profile')
+        os.makedirs(os.path.dirname(apparmor_path))
+        with open(apparmor_path, 'w', encoding='utf-8') as fp:
+            fp.write(dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 88.88
+            }
+            """))
+        self._make_metadata(scratch, APPARMOR_2_TEMPLATE)
+        self.builder.add_file(scratch, "./")
+        # FIXME: make it a .snap
+        expected_path = os.path.join(self.temp_dir, "test-yaml_1.0_all.click")
+        actual_path = self.builder.build(self.temp_dir, "meta/package.yaml")
+        self.assertEqual(expected_path, actual_path)
+        self.assertTrue(os.path.exists(actual_path))
+        contents = os.path.join(self.temp_dir, 'contents')
+        subprocess.check_output(
+            ["dpkg-deb", "-R", actual_path, contents], universal_newlines=True)
+        self.assertEqual(sorted(os.listdir(contents)),
+                         ['DEBIAN', 'bin', 'data', 'meta', 'path'])
+        self.assertEqual(
+            sorted(os.listdir(os.path.join(contents, 'path', 'to'))),
+            ['app1.apparmor-profile'])
+        with open(
+                os.path.join(contents, 'path', 'to', 'app1.apparmor-profile'),
+                encoding='utf-8') as fp:
+            data = fp.read()
+        self.assertEqual(data, dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 88.88
+            }
+            """))
+        with open(os.path.join(contents, 'meta', 'app2.apparmor'),
+                  encoding='utf-8') as fp:
+            data = fp.read()
+        self.assertEqual(data, dedent("""\
+            {
+                "template": "default",
+                "policy_groups": [
+                    "networking"
+                ],
+                "policy_vendor": "ubuntu-snappy",
+                "policy_version": 1.3
+            }
+            """))