Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PRO security isolation: apt_news #2794

Merged
merged 22 commits into from
Jan 4, 2024
Merged

Conversation

panlinux
Copy link
Contributor

@panlinux panlinux commented Oct 16, 2023

Why is this needed?

This is part of JIRA SD-1277 which aims to enable security features to existing Pro systemd services. The goal is to provide reasonable AppAmor and systemd security isolation configuration for the Pro systemd services.

There are multiple services to be tackled, and this PR is about apt-news.service, and tracked via JIRA SD-1450

Test Steps

This change relies mostly on the integration tests, but it can also be superficially tested manually.

Install ubuntu-advantage-tools, and run apt-get update.. Observe dmesg -wT and confirm there are no apparmor DENIED messages tied to the profile ubuntu_advantage_apt_news, and that the apt-news service performs normally.

Checklist

  • I have updated or added any unit tests accordingly
  • I have updated or added any integration tests accordingly
  • Changes here need to be documented, and this was done in: Docs apparmor update #2906

Does this PR require extra reviews?

  • Yes
  • No

Unsure, thinking.

@github-actions
Copy link

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: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

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

👍 this comment to confirm that this is correct.

@panlinux panlinux force-pushed the ua-security-isolation-upstream branch 6 times, most recently from 539fc07 to b1ee21c Compare October 17, 2023 17:02
@panlinux panlinux force-pushed the ua-security-isolation-upstream branch 12 times, most recently from fb1c46f to f9befe3 Compare November 23, 2023 12:59
@panlinux panlinux force-pushed the ua-security-isolation-upstream branch 5 times, most recently from 77caeda to 676ab8e Compare December 3, 2023 18:40
@panlinux panlinux force-pushed the ua-security-isolation-upstream branch 6 times, most recently from 1362117 to 53fc62c Compare December 7, 2023 12:19
@panlinux
Copy link
Contributor Author

panlinux commented Jan 1, 2024

So, the test_collect_logs() test is mocking a lot of stuff, but builtins.open together with os.path.isfile always returning true, that utterly breaks shutil.copy() in bionic and xenial, generating an OOM. This simple file reproduces it:

#!/usr/bin/python3
import mock
import shutil
@mock.patch("os.path.isfile", return_value=True)
@mock.patch("builtins.open")
def mycopy(_fopen, m_isfile):
    shutil.copy("/doesnotexist", "/tmp")

mycopy()

@panlinux panlinux force-pushed the ua-security-isolation-upstream branch from 431faf9 to 433afa4 Compare January 2, 2024 18:56
@panlinux panlinux force-pushed the ua-security-isolation-upstream branch from 433afa4 to 4869695 Compare January 2, 2024 18:57
@panlinux
Copy link
Contributor Author

panlinux commented Jan 2, 2024

I updated pro collect-logs and the apparmor hook regarding the new apparmor logs and profile. I also updated the unit tests more or less following the same mock pattern that test_collect_logs was already using, but I think at some point these tests should get rid of mock.

@panlinux panlinux mentioned this pull request Jan 3, 2024
5 tasks
@panlinux
Copy link
Contributor Author

panlinux commented Jan 3, 2024

Documentation PR: #2906

uaclient/actions.py Outdated Show resolved Hide resolved
uaclient/actions.py Outdated Show resolved Hide resolved
@panlinux
Copy link
Contributor Author

panlinux commented Jan 4, 2024

bionic started failing out of the blue. Did something in the bionic esm environment around this test change?

2024-01-04T01:19:55.7518165Z   Scenario: Fix command on a machine without security/updates source lists                                                                                                            # features/fix.feature:828
2024-01-04T01:19:55.7520635Z     Given a `bionic` `lxd-container` machine with ubuntu-advantage-tools installed                                                                                                    # features/steps/machines.py:157
2024-01-04T01:20:08.1412381Z     When I run `sed -i "/bionic-updates/d" /etc/apt/sources.list` with sudo                                                                                                           # features/steps/shell.py:40
2024-01-04T01:20:08.1561893Z     And I run `sed -i "/bionic-security/d" /etc/apt/sources.list` with sudo                                                                                                           # features/steps/shell.py:40
2024-01-04T01:20:08.2130721Z     And I run `apt-get update` with sudo                                                                                                                                              # features/steps/shell.py:40
2024-01-04T01:20:12.1434933Z     And I run `wget -O pkg.deb https://launchpad.net/ubuntu/+source/openssl/1.1.1-1ubuntu2.1~18.04.14/+build/22454675/+files/openssl_1.1.1-1ubuntu2.1~18.04.14_amd64.deb` as non-root # features/steps/shell.py:40
2024-01-04T01:20:13.5576722Z     And I run `dpkg -i pkg.deb` with sudo                                                                                                                                             # features/steps/shell.py:40
2024-01-04T01:20:14.0312620Z     And I verify that running `pro fix CVE-2023-0286` `as non-root` exits `1`                                                                                                         # features/steps/shell.py:130
2024-01-04T01:20:16.8225661Z     Then stdout matches regexp                                                                                                                                                        # features/steps/output.py:64
2024-01-04T01:20:16.8227631Z       """
2024-01-04T01:20:16.8229118Z       CVE-2023-0286: OpenSSL vulnerabilities
2024-01-04T01:20:16.8230785Z        - https://ubuntu.com/security/CVE-2023-0286
2024-01-04T01:20:16.8232171Z       
2024-01-04T01:20:16.8233806Z       2 affected source packages are installed: openssl, openssl1.0
2024-01-04T01:20:16.8235485Z       \(1/2, 2/2\) openssl, openssl1.0:
2024-01-04T01:20:16.8237023Z       A fix is available in Ubuntu standard updates.
2024-01-04T01:20:16.8239454Z       - Cannot install package openssl version 1.1.1-1ubuntu2.1~18.04.21
2024-01-04T01:20:16.8241057Z       
2024-01-04T01:20:16.8241746Z       1 package is still affected: openssl
2024-01-04T01:20:16.8242722Z       .*✘.* CVE-2023-0286 is not resolved.
2024-01-04T01:20:16.8243487Z       """
2024-01-04T01:20:18.5561813Z       Assertion Failed: Expected to match regexp:
2024-01-04T01:20:18.5563708Z         CVE-2023-0286: OpenSSL vulnerabilities
2024-01-04T01:20:18.5565344Z          - https://ubuntu.com/security/CVE-2023-0286
2024-01-04T01:20:18.5566737Z       
2024-01-04T01:20:18.5570555Z         2 affected source packages are installed: openssl, openssl1.0
2024-01-04T01:20:18.5572297Z         \(1/2, 2/2\) openssl, openssl1.0:
2024-01-04T01:20:18.5574264Z         A fix is available in Ubuntu standard updates.
2024-01-04T01:20:18.5576200Z         - Cannot install package openssl version 1.1.1-1ubuntu2.1~18.04.21
2024-01-04T01:20:18.5577791Z       
2024-01-04T01:20:18.5578930Z         1 package is still affected: openssl
2024-01-04T01:20:18.5580475Z         .*✘.* CVE-2023-0286 is not resolved.
2024-01-04T01:20:18.5581794Z       But got:
2024-01-04T01:20:18.5582970Z         CVE-2023-0286: Node.js vulnerabilities
2024-01-04T01:20:18.5583964Z          - https://ubuntu.com/security/CVE-2023-0286
2024-01-04T01:20:18.5584813Z       
2024-01-04T01:20:18.5585628Z         2 affected source packages are installed: openssl, openssl1.0
2024-01-04T01:20:18.5586627Z         (1/2, 2/2) openssl, openssl1.0:
2024-01-04T01:20:18.5587525Z         A fix is available in Ubuntu standard updates.
2024-01-04T01:20:18.5588691Z         - Cannot install package openssl version 1.1.1-1ubuntu2.1~18.04.21
2024-01-04T01:20:18.5589661Z       
2024-01-04T01:20:18.5590342Z         1 package is still affected: openssl
2024-01-04T01:20:18.5591302Z         �[91m✘�[0m CVE-2023-0286 is not resolved.

Most or all of the settings are defined there, and since that module
doesn't import anything, this also avoids potential future dependencies.
@lucasmoura
Copy link
Contributor

@panlinux regarding the test error that you mentioned, it will be fixed by this PR here: #2897
And I can confirm it has no relations to your PR

@panlinux panlinux force-pushed the ua-security-isolation-upstream branch from 5590710 to 77b4d75 Compare January 4, 2024 14:38
@panlinux
Copy link
Contributor Author

panlinux commented Jan 4, 2024

Mantic failure:

2024-01-04T15:11:48.3060589Z   Scenario Outline: services fail gracefully when yaml is broken/absent -- @1.2 ubuntu release        # features/unattached_commands.feature:418
2024-01-04T15:11:48.3063382Z     Given a `mantic` `lxd-container` machine with ubuntu-advantage-tools installed                    # features/steps/machines.py:157
2024-01-04T15:12:03.9848203Z     When I run `apt update` with sudo                                                                 # features/steps/shell.py:40
2024-01-04T15:12:11.5977607Z     And I run `rm -rf /usr/lib/python3/dist-packages/yaml` with sudo                                  # features/steps/shell.py:40
2024-01-04T15:12:11.6557738Z     And I verify that running `pro status` `with sudo` exits `1`                                      # features/steps/shell.py:130
2024-01-04T15:12:11.7871834Z     Then stderr matches regexp                                                                        # features/steps/output.py:64
2024-01-04T15:12:11.7873754Z       """
2024-01-04T15:12:11.7875105Z       Couldn't import the YAML module.
2024-01-04T15:12:11.7876952Z       Make sure the 'python3-yaml' package is installed correctly
2024-01-04T15:12:11.7878827Z       and \/usr\/lib\/python3\/dist-packages is in your PYTHONPATH\.
2024-01-04T15:12:11.7880175Z       """
2024-01-04T15:12:11.7881597Z     When I verify that running `python3 /usr/lib/ubuntu-advantage/esm_cache.py` `with sudo` exits `1` # features/steps/shell.py:130
2024-01-04T15:12:11.8678579Z     Then stderr matches regexp                                                                        # features/steps/output.py:64
2024-01-04T15:12:11.8680527Z       """
2024-01-04T15:12:11.8681959Z       Couldn't import the YAML module.
2024-01-04T15:12:11.8683748Z       Make sure the 'python3-yaml' package is installed correctly
2024-01-04T15:12:11.8686113Z       and \/usr\/lib\/python3\/dist-packages is in your PYTHONPATH\.
2024-01-04T15:12:11.8687177Z       """
2024-01-04T15:12:11.8688585Z     When I verify that running `systemctl start apt-news.service` `with sudo` exits `1`               # features/steps/shell.py:130
2024-01-04T15:12:13.2490930Z       Assertion Failed: Expected exit code in: ['1'] but got 0

Can't diagnose because we have no logs, which is because of #2902

@panlinux
Copy link
Contributor Author

panlinux commented Jan 4, 2024

bionic is the same failure as #2794 (comment), which is supposed to be fixed by #2897 according to #2794 (comment)

@panlinux
Copy link
Contributor Author

panlinux commented Jan 4, 2024

@lucasmoura I added a new function to gather the apparmor logs, which also creates an -error file if there is something wrong. The error situation is not unit tested (frankly, that test_collect_logs() function is far com being a unittest), but I tried it manually, injecting an error, and the file was created.

Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this great effort @panlinux!

The bionic fail will be fixed when #2897 is merged.

I recognize the mantic fail as a flaky test and am unable to reproduce the failure when running the test locally.

Merging

@orndorffgrant orndorffgrant merged commit b849ffe into main Jan 4, 2024
29 of 31 checks passed
@lucasmoura lucasmoura deleted the ua-security-isolation-upstream branch May 17, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants