Merge ~alexmurray/qa-regression-testing:update-test-samba-to-run-under-apparmor into qa-regression-testing:master

[Alex Murray]

Includes the following changes:

scripts/test-samba.py: robustify handling of command-line args

Won't bork if run without -v now and should handle things more gracefully in
general

scripts/test-samba.py: test with apparmor profiles installed

To do this we then need to add some support for manually patching the apparmor
profile to allow the required path accesses by smbd.

scripts/test-samba.py: add local fix for upstream issue #386

To fix https://gitlab.com/apparmor/apparmor/-/issues/386 in noble manually add
some rules based on those in
https://gitlab.com/apparmor/apparmor/-/merge_requests/1219

.launchpad.yaml: add samba to lpci

Also skip a few tests in the lpci environment since we don't have permission to
mount there

[Alex Murray (alexmurray)]

The primary motivation for the main change here (to run samba with apparmor-profiles installed and hence under confinement) is to catch any future instances of https://gitlab.com/apparmor/apparmor/-/issues/386

[Alex Murray (alexmurray)]

Bump - any chance to get a review on this one?

[Marc Deslauriers (mdeslaur)]

LGTM, though is your plan to fix the actual profiles and remove the workarounds in this script? It kind of defeats the purpose to have the workarounds here...

review: Approve

[Steve Beattie (sbeattie)]

Note that the tests are failing in focal and bionic because pre-usrmerge the apparmor_parser binary lived in /sbin/ as well as an issue with attempting to umount; see https://launchpadlibrarian.net/727074670/buildlog_ci_qa-regression-testing_1ffc82734f3224575281f37fed586bb559ef63d7_BUILDING.txt.gz

(Also, rather than having a seprate commit for merging changes from master, in this case to get the fixes to .launchpad.yaml, rebasing on top of master gives a cleaner history.)

Inline comment as well.

review: Needs Fixing

da83729... by Alex Murray on 2024-05-02

scripts/test-samba.py: use assertShellExitSuccess() as suggest by sbeattie

Signed-off-by: Alex Murray <email address hidden>

5efb927... by Alex Murray on 2024-05-02

scripts/test-samba.py: skip more tests in lpci that use mount

Signed-off-by: Alex Murray <email address hidden>

[Alex Murray (alexmurray)]

@mdeslaur - yep the plan is to remove these workarounds once apparmor 4.0.1 lands in noble via https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2064672 (or some other such upload to get the issue fixed directly in apparmor-profiles)

@sbeattie - thanks for the review - issues should be fixed in the two subsequent commits. Also I chose to merge to keep the original commits ids to try and preserve any previous comments against them but I realise now that was redundant as there were none :) - will try rebase and drop the merge commit and hopefully it doesn't break too much stuff.

Unmerged commits

5efb927... by Alex Murray on 2024-05-02

scripts/test-samba.py: skip more tests in lpci that use mount

Signed-off-by: Alex Murray <email address hidden>

	imagemagick:0 (build)
	imagemagick:1 (build)
	imagemagick:2 (build)
	gcc-security:0 (build)
	gcc-security:1 (build)
	gcc-security:2 (build)
	glibc:0 (build)
	glibc:1 (build)
	glibc:2 (build)
	glibc-security:0 (build)
	glibc-security:1 (build)
	glibc-security:2 (build)
	gnupg:0 (build)
	gnupg:1 (build)
	gnupg:2 (build)
	sudo:0 (build)
	sudo:1 (build)
	sudo:2 (build)
	git:0 (build)
	git:1 (build)
	git:2 (build)
	ghostscript:0 (build)
	ghostscript:1 (build)
	ghostscript:2 (build)
	busybox:0 (build)
	busybox:1 (build)
	busybox:2 (build)
	coreutils:0 (build)
	coreutils:1 (build)
	coreutils:2 (build)
	util-linux:0 (build)
	util-linux:1 (build)
	util-linux:2 (build)
	ecdsautils:0 (build)
	ecdsautils:1 (build)
	ecdsautils:2 (build)
	python-urllib3:0 (build)
	python-urllib3:1 (build)
	python-urllib3:2 (build)
	amanda:0 (build)
	amanda:1 (build)
	samba:0 (build)
	samba:1 (build)
	samba:2 (build)

1 → 44 of 44 results

First • Previous • Next • Last

da83729... by Alex Murray on 2024-05-02

scripts/test-samba.py: use assertShellExitSuccess() as suggest by sbeattie

Signed-off-by: Alex Murray <email address hidden>

c7edc74... by Alex Murray on 2024-04-24

.launchpad.yaml: add samba to lpci

Also skip a few tests in the lpci environment since we don't have permission to
mount there

Signed-off-by: Alex Murray <email address hidden>

71f83c1... by Alex Murray on 2024-04-24

scripts/test-samba.py: add local fix for upstream issue #386

To fix https://gitlab.com/apparmor/apparmor/-/issues/386 in noble manually add
some rules based on those in
https://gitlab.com/apparmor/apparmor/-/merge_requests/1219

Signed-off-by: Alex Murray <email address hidden>

96b9f5b... by Alex Murray on 2024-04-23

scripts/test-samba.py: test with apparmor profiles installed

To do this we then need to add some support for manually patching the apparmor
profile to allow the required path accesses by smbd.

Signed-off-by: Alex Murray <email address hidden>

9a73a0c... by Alex Murray on 2024-04-23

scripts/test-samba.py: robustify handling of command-line args

Won't bork if run without -v now and should handle things more gracefully in
general

Signed-off-by: Alex Murray <email address hidden>

diff --git a/.launchpad.yaml b/.launchpad.yaml
index a32b91d..c498f70 100644
--- a/.launchpad.yaml
+++ b/.launchpad.yaml
@@ -14,6 +14,7 @@ pipeline:
     - ecdsautils
     - python-urllib3
     - amanda
+    - samba
 
 jobs:
   imagemagick:
@@ -226,3 +227,18 @@ jobs:
       DEBIAN_FRONTEND=noninteractive apt upgrade --assume-yes
     run: |
       ./lpcraft-runner amanda
+
+  samba:
+    matrix:
+      - series: jammy
+        architectures: amd64
+      - series: focal
+        architectures: amd64
+      - series: bionic
+        architectures: amd64
+    packages:
+      - sudo
+    run-before: |
+      DEBIAN_FRONTEND=noninteractive apt upgrade --assume-yes
+    run: |
+      ./lpcraft-runner samba
diff --git a/scripts/test-samba.py b/scripts/test-samba.py
index df2e6b5..d2adfe0 100755
--- a/scripts/test-samba.py
+++ b/scripts/test-samba.py
@@ -19,7 +19,7 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-# QRT-Packages: samba smbclient bind9-host sudo
+# QRT-Packages: samba smbclient bind9-host sudo apparmor-profiles linux-generic
 # QRT-Alternates: smbfs cifs-utils
 
 '''
@@ -81,6 +81,8 @@ class SambaCommon(testlib.TestlibCase):
         rc, report = testlib.cmd(["/sbin/ip", "-4", "route", "list", "0/0"])
         interface = report.split("\n")[0].split()[4]
 
+        # ensure real_ip is defined
+        self.real_ip = self.ip
         rc, report = testlib.cmd(["/sbin/ip", "-4", "address", "show", "dev", interface])
         for line in report.split("\n"):
             if line.strip().startswith("inet"):
@@ -267,6 +269,29 @@ class SambaCommon(testlib.TestlibCase):
         self._testparm()
         self._restart()
 
+    def _add_apparmor_rule(self, rule, path):
+        contents = None
+        if os.path.exists(path):
+            with open(path, "r") as f:
+                contents = f.read()
+            new_contents = ""
+            for line in contents.splitlines():
+                # append rule before the end of the profile
+                if line == "}":
+                    new_contents = new_contents + "  " + rule + "\n"
+                new_contents = new_contents + line + "\n"
+            with open(path, "w") as f:
+                f.write(new_contents)
+            self.assertShellExitSuccess(['apparmor_parser', '-r', path])
+        return (contents, path)
+
+    def _restore_apparmor_profile(self, profile):
+        contents, path = profile
+        if contents is not None:
+            with open(path, "w") as f:
+                f.write(contents)
+            self.assertShellExitSuccess(['apparmor_parser', '-r', path])
+
     def _word_find(self, report, name):
         '''Check for a specific string'''
         warning = 'Could not find "%s"\n' % name
@@ -444,9 +469,26 @@ class SambaTmp(SambaCommon):
 ''', append=True)
         self._testparm()
         self._restart()
+        # ensure that apparmor will allow access to /tmp - NOTE this should happen
+        # automatically in noble+ when restarting smbd once LP: #2063079 is
+        # resolved
+        self.orig_profile = self._add_apparmor_rule("/tmp{,/**} rw,", "/etc/apparmor.d/usr.sbin.smbd")
+        # add some rules for samba-rpcd-classic as well until this is fixed in
+        # the apparmor-profiles package (see
+        # https://gitlab.com/apparmor/apparmor/-/issues/386 for more details)
+        self.orig_rpcd_classic = self._add_apparmor_rule("""
+@{run}/samba/ncalrpc/np/srvsvc wr,
+@{run}/samba/ncalrpc/np/winreg wr,
+/dev/urandom rw,
+/usr/lib*/samba/{,samba/}samba-dcerpcd Px -> samba-dcerpcd,
+""",
+                                path="/etc/apparmor.d/samba-rpcd-classic")
 
     def tearDown(self):
         '''Tear down method'''
+        # revert apparmor profiles
+        self._restore_apparmor_profile(self.orig_profile)
+        self._restore_apparmor_profile(self.orig_rpcd_classic)
         self._tearDown()
 
     def test_tmp_browse(self):
@@ -704,6 +746,11 @@ class SambaSmbfs(SambaCommon):
         self.mountpoint = tempfile.mkdtemp(prefix='testlib', dir='/mnt')
         os.chmod(self.mountpoint, 0o755)
 
+        # ensure that apparmor will allow access to /tmp - NOTE this should happen
+        # automatically in noble+ when restarting smbd once LP: #2063079 is
+        # resolved
+        self.orig_profile = self._add_apparmor_rule("/tmp{,/**} rw,", "/etc/apparmor.d/usr.sbin.smbd")
+
         self.testdir = tempfile.mkdtemp(prefix='testlib', dir='/tmp')
         os.chmod(self.testdir, 0o1777)
 
@@ -715,6 +762,8 @@ class SambaSmbfs(SambaCommon):
         if os.path.exists(self.testdir):
             testlib.recursive_rm(self.testdir)
         self.user = None
+        # revert apparmor profile
+        self._restore_apparmor_profile(self.orig_profile)
         self._tearDown()
 
     def test_guest(self):
@@ -830,6 +879,10 @@ class SambaUnixext(SambaCommon):
         self._testparm()
         self._restart()
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_default(self):
         '''(SambaUnixext) Default options'''
 
@@ -951,6 +1004,10 @@ class SambaUnixext(SambaCommon):
         self.assertEqual(expected, rc, result + report)
 
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_unixextandwldis(self):
         '''(SambaUnixext) Test with unix extensions and wide links disabled'''
 
@@ -1029,6 +1086,11 @@ class SambaCifs(SambaCommon):
         self.testdir = tempfile.mkdtemp(prefix='testlib', dir='/tmp')
         os.chmod(self.testdir, 0o1777)
 
+        # ensure that apparmor will allow access to /tmp - NOTE this should happen
+        # automatically in noble+ when restarting smbd once LP: #2063079 is
+        # resolved
+        self.orig_profile = self._add_apparmor_rule("/tmp{,/**} rw,", "/etc/apparmor.d/usr.sbin.smbd")
+
     def tearDown(self):
         '''Tear down method'''
         subprocess.call(['umount', self.mountpoint])
@@ -1041,8 +1103,14 @@ class SambaCifs(SambaCommon):
         if os.path.exists(self.creds):
             os.unlink(self.creds)
         self.user = None
+        # revert apparmor profile
+        self._restore_apparmor_profile(self.orig_profile)
         self._tearDown()
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_guest(self):
         '''(SambaCifs) Guest mount (read-only)'''
 
@@ -1059,6 +1127,10 @@ class SambaCifs(SambaCommon):
 
         SambaCommon._test_dir_list(self, self.mountpoint)
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_user(self):
         '''(SambaCifs) User mount (read/write)'''
         self.user = self._adduser()
@@ -1152,6 +1224,10 @@ class SambaCifs(SambaCommon):
         result = 'Found user password in report\n'
         self.assertFalse(self.user.password in report, result + report)
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_number_files(self):
         '''(SambaCifs) Test number of files'''
 
@@ -1203,6 +1279,10 @@ class SambaHomes(SambaCommon):
         self.nohome_user = None
         self._tearDown()
 
+    @unittest.skipIf(
+        'LPCRAFT_QRT_RUNNER' in os.environ,
+        'unable to mount in lpci environment'
+    )
     def test_homedir_user(self):
         '''(SambaHomes) Regular User mount'''
 
@@ -1392,11 +1472,13 @@ if __name__ == '__main__':
     ubuntu_version = testlib.manager.lsb_release["Release"]
 
     tmpserve = False
-    if (len(sys.argv) == 1 or sys.argv[1] != '-v'):
-        if sys.argv[1] == "tmpserve":
-            tmpserve = True
-        else:
-            test_client = sys.argv[1]
+    if "tmpserve" in sys.argv:
+        tmpserve = True
+    elif len(sys.argv) > 1:
+        # infer test_client as the first non -v argument
+        args = [arg for arg in sys.argv[1:] if arg != "-v"]
+        if args:
+            test_client = args[0]
 
     if tmpserve:
         # useful for testing apps that use samba