search

canonical / ubuntu-pro-client

Ubuntu Pro Client for offerings from Canonical
https://canonical-ubuntu-pro-client.readthedocs-hosted.com/en/latest/
GNU General Public License v3.0
51 stars 69 forks source link

apparmor: deal with usr-merge consequences #3131

Closed panlinux closed 1 month ago

panlinux commented 1 month ago

Focal systems that were upgraded from bionic are experiencing a series of apparmor DENIED messages that have one thing in common: execution of binaries in /sbin or /bin.

This happens because of the usr-merge effort. On fresh focal systems, we have symlinks replacing top-level directories like /bin, /sbin, and others:

root@f-pristine:~# ls -la /{bin,lib,lib*,sbin} lrwxrwxrwx 1 root root 7 May 24 21:40 /bin -> usr/bin lrwxrwxrwx 1 root root 7 May 24 21:40 /lib -> usr/lib lrwxrwxrwx 1 root root 7 May 24 21:40 /lib -> usr/lib lrwxrwxrwx 1 root root 9 May 24 21:40 /lib32 -> usr/lib32 lrwxrwxrwx 1 root root 9 May 24 21:40 /lib64 -> usr/lib64 lrwxrwxrwx 1 root root 10 May 24 21:40 /libx32 -> usr/libx32 lrwxrwxrwx 1 root root 8 May 24 21:40 /sbin -> usr/sbin

And the apparmor profile for focal systems expects that directory layout.

In bionic, these are actual directories: root@b-pristine:~# ls -lad /{bin,lib,lib*,sbin} drwxr-xr-x 1 root root 2472 Jun 7 2023 /bin drwxr-xr-x 1 root root 438 Jun 7 2023 /lib drwxr-xr-x 1 root root 438 Jun 7 2023 /lib drwxr-xr-x 1 root root 40 Jun 7 2023 /lib64 drwxr-xr-x 1 root root 3694 Jun 7 2023 /sbin

In a focal system that was upgraded from bionic, the usr-merge is not done, and this focal system will retain the bionic top-level directories. Binaries which were previously allowed to be executed in /sbin and /bin now will be denied, because in this upgraded focal system they remain in /bin and /sbin, and the focal apparmor profile was expecting them to be in /usr/bin and /usr/sbin.

LP: #2067319

Why is this needed?

This PR solves all of our problems because...

Test Steps

There is also a PPA with this change, built for all ubuntu releases:

https://launchpad.net/~ahasenack/+archive/ubuntu/uat-test/+packages

Note: this ppa didn't change version.py, just the apparmor profiles.

Checklist

Does this PR require extra reviews?

github-actions[bot] commented 1 month ago

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references)

Launchpad Bugs:

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

panlinux commented 1 month ago

All lxd containers are failing to be created because their names is suddenly too long:

      RuntimeError: Failure (rc=1): Error: Failed instance creation: Invalid instance name: Name must be 1-63 characters long

And the name is indeed larger than 63 characters: upro-behave-mantic-builder-lxd-container-mantic-0527-210853039716

This might have come from commit a608a9ef837f7cba4114622fcaa830cefcfdf147:

diff --git a/features/steps/machines.py b/features/steps/machines.py
index fd57034a6..35a90f5f9 100644
--- a/features/steps/machines.py
+++ b/features/steps/machines.py
@@ -205,7 +205,7 @@ def given_a_sut_machine(context, series, machine_type):
             )
             sys.exit(1)

-    builder_name = BUILDER_NAME_PREFIX + machine_type
+    builder_name = BUILDER_NAME_PREFIX + machine_type + "-" + series

     if context.pro_config.snapshot_strategy:
         if builder_name not in context.snapshots:
renanrodrigo commented 1 month ago

The CI errors here are related to Security Endpoints being nonresponsive and have nothing to do with the change in place. Those can be safely ignored.

panlinux commented 1 month ago

I used this grep to check:

$ grep usr debian/apparmor/*|grep -v ,usr/

The results are all under an ifdef, or touching /usr/share which is not affected (there was never a toplevel /share directory).

BUT PLEASE RE-VERIFY

panlinux commented 1 month ago

jammy: internal server error

["2024-05-28T16:07:01.023", "ERROR", "ubuntupro.cli", "wrapper", 1351, "Error connecting to /v1/contracts/cAH6JVxGoVX25-fdk2MtgS8_dl7ZoRv3zmgAkociIyls/machine-activity/008f71f292534c20b5c399d3143ce53c: 500 <html>\r\n<head><title>500 Internal Server Error</title></head>\r\n<body>\r\n<center><h1>500 Internal Server Error</h1></center>\r\n<hr><center>nginx/1.18.0 (Ubuntu)</center>\r\n</body>\r\n</html>\r\n", {}]
panlinux commented 1 month ago

The changes LGTM. Thanks, Andreas.

The commit tagged for removal will remain in the commit history even after it is reverted. It would be nice to give it a better summary/title (e.g., revert foo due to bar) and file an issue here in GH to revert it later.

The reason for the revert is a few comments up, at https://github.com/canonical/ubuntu-pro-client/pull/3131#issuecomment-2134051940

panlinux commented 1 month ago

I don't know what that commit was fixing, hence the XXX remark, but it needs reverting for github CI, at least at the moment.

panlinux commented 1 month ago

bionic failure: internal server error

["2024-05-28T15:31:59.014", "DEBUG", "ubuntupro.http", "readurl", 385, "URL [POST] response: https://contracts.canonical.com/v1/contracts/cAH6JVxGoVX25-fdk2MtgS8_dl7ZoRv3zmgAkociIyls/machine-activity/0921c750bb544f8097b793c7b6aa0d0c, headers: {'connection': 'close', 'content-length': '186', 'content-type': 'text/html', 'date': 'Tue, 28 May 2024 15:31:58 GMT', 'server': 'nginx/1.18.0 (Ubuntu)'}, data: <html>\r\n<head><title>500 Internal Server Error</title></head>\r\n<body>\r\n<center><h1>500 Internal Server Error</h1></center>\r\n<hr><center>nginx/1.18.0 (Ubuntu)</center>\r\n</body>\r\n</html>\r\n", {}]
panlinux commented 1 month ago

focal: also internal server errors

panlinux commented 1 month ago

I'll just retry the failed ones :/

panlinux commented 1 month ago

@panlinux just please remove the XXX commit and forcepush without it so it does not need to be reverted

Done