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

tests: add prompting client integration tests #14387

Conversation

olivercalder
Copy link
Member

@olivercalder olivercalder commented Aug 19, 2024

Add a spread test to run the integration tests from https://github.com/canonical/prompting-client

This is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-30440

This PR is blocked until https://github.com/canonical/prompting-client is made public and the integration-tests binary and aa-prompting-test snap are published and accessible from spread. For now, use unlisted binaries on a private webserver.

This PR is based on #14448, only the commits from "add prompting client integration tests" on are relevant.

@olivercalder
Copy link
Member Author

Restore is failing, claiming the system is corrupted... I suspect this may have something to do with using lxd for snapcraft to build the testing snap, but I'm not sure. Perhaps @sergiocazzolato could have a look? The relevant run will be ubuntu-noble.

@olivercalder olivercalder force-pushed the prompting-add-client-integration-tests branch from 1f150f1 to 53f9e5e Compare August 26, 2024 19:13
Copy link
Collaborator

@sergiocazzolato sergiocazzolato left a comment

Choose a reason for hiding this comment

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

Some comments inline

lxd init --auto
snap install snapcraft --classic

git clone https://github.com/canonical/prompting-client /home/ubuntu/prompting-client
Copy link
Collaborator

Choose a reason for hiding this comment

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

about this, is it possible to upload the snap to the snapt and avoid using snapcraft in the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally we'd build the snap and integration tests binary whenever a new release of the prompting-client occurs, but this is not being done at the moment. I'll bring it up with the Desktop team again, as these do take a very long time in spread.


restore: |
rm -rf /home/ubuntu/prompting-client
snap remove --purge lxd
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't be required, it is done automatically

cargo test --no-run
FNAME=$(find . -regextype grep -regex './target/debug/deps/integration.*[^.][^d]' | head -n1)
cp "$FNAME" /home/ubuntu/integration-tests
chown ubuntu:ubuntu /home/ubuntu/integration-tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

this dir needs to be cleaned

Copy link
Contributor

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

Thank you!

some small comments about test setup, nothing major.

lxd init --auto
snap install snapcraft --classic

git clone https://github.com/canonical/prompting-client /home/ubuntu/prompting-client
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to use relative paths when building If this test is ever extended to more distros.

cargo test --no-run
FNAME=$(find . -regextype grep -regex './target/debug/deps/integration.*[^.][^d]' | head -n1)
cp "$FNAME" /home/ubuntu/integration-tests
chown ubuntu:ubuntu /home/ubuntu/integration-tests
Copy link
Contributor

Choose a reason for hiding this comment

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

A new user session using tests.session -u test prepare in the prepare script would be distro agnostic.

snap set system experimental.apparmor-prompting=true

echo "Run prompting-client integration tests as non-root user"
sudo -iu ubuntu /home/ubuntu/integration-tests
Copy link
Contributor

Choose a reason for hiding this comment

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

then this would be

Suggested change
sudo -iu ubuntu /home/ubuntu/integration-tests
tests.session -u test exec /home/ubuntu/integration-tests

when run against snapd.

systems:
- ubuntu-2*
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be a good idea to check what happens in a few more distros.

@olivercalder olivercalder force-pushed the prompting-add-client-integration-tests branch from 9933012 to e33f87b Compare September 5, 2024 17:56
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Sep 5, 2024
@olivercalder
Copy link
Member Author

Looks like prompting client integration tests pass on 22.04 with whatever kernel the cloud image uses. Interesting...

@olivercalder
Copy link
Member Author

Can confirm, prompting client integration tests did run and pass on 22.04:

https://productionresultssa9.blob.core.windows.net/actions-results/5b5d1e61-c6a8-4842-aa04-809ccbfaf72e/workflow-job-run-14f95ab2-db4b-5972-1fce-a3a9fc33b542/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-09-05T21%3A40%3A39Z&sig=Fpbdpov0t0pQVKfKc128i9BJ%2BAukgUEgMqcqCdFh6Po%3D&ske=2024-09-06T09%3A21%3A38Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2024-09-05T21%3A21%3A38Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2024-05-04&sp=r&spr=https&sr=b&st=2024-09-05T21%3A30%3A34Z&sv=2024-05-04

2024-09-05T19:36:03.2666358Z 2024-09-05 19:35:43 WARNING: sep051829-816842 (google:ubuntu-22.04-64:tests/main/apparmor-prompting-client-integration-tests) running late. Current output:
2024-09-05T19:36:03.2667842Z -----
2024-09-05T19:36:03.2668384Z (... 365 lines above ...)
2024-09-05T19:36:03.2670886Z Compiling serde v1.0.204
2024-09-05T19:36:03.2671694Z Compiling bitflags v2.5.0
2024-09-05T19:36:03.2672527Z Compiling regex-automata v0.4.6
2024-09-05T19:36:03.2673125Z Compiling want v0.3.1
2024-09-05T19:36:03.2673743Z Compiling h2 v0.4.5
2024-09-05T19:36:03.2674399Z Compiling prost v0.13.1
2024-09-05T19:36:03.2675221Z Compiling tower v0.4.13
2024-09-05T19:36:03.2676203Z Compiling axum-core v0.4.3
2024-09-05T19:36:03.2676822Z Compiling async-trait v0.1.81
2024-09-05T19:36:03.2677590Z Compiling serde_derive v1.0.204
2024-09-05T19:36:03.2678302Z -----
2024-09-05T19:36:03.2678737Z .

We only compile the integration tests on systems where they'll be run, so we know they did run, and the spread test itself passed, so we know the integration tests passed.

@olivercalder olivercalder removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Sep 5, 2024
Prompting cannot be supported if the kernel notification socket does not
exist. Thus, add a check for its presence to PromptingSupportedByFeatures.

However, this alone is not sufficient. The notification socket may be
used for other purposes as well. When this becomes the case, a directory
will be added at `/sys/kernel/security/apparmor/features/policy/notify`.
If that directory exists, and it contains a file called `user`, which
contains the list of prompting mediation classes, then prompting is
supported on the system. If no file called `user` exists in that
directory, but the directory itself exists, then we know prompting is
not supported.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…support test

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…itu test

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…le on debian-sid"

This reverts commit 66eb862.

Debian Sid reports prompting support in its AppArmor kernel and parser
features, the former because the upstream kernel has extended
permissions merged (but non-functional), and the latter because we use
vendored AppArmor with snapd. But the kernel does not actually support
AppArmor prompting, as this requires patches from the Ubuntu kernel
which have not yet been upstreamed.

Now that we check for the presence of the kernel notification socket as
well as the AppArmor kernel and parser features, we can correctly
identify that prompting is not supported on Debian Sid.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…ored apparmor on 22.04"

This reverts commit 1ec4132.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…nal apparmor"

This reverts commit a4e46bb.

On Ubuntu LTS releases < 24.04, prompting is still not supported by the
standard (non-hwe) kernels. This is reflected in tests, now that
AppArmor prompting support checks test for the presence of the kernel
notification socket.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Ubuntu systems older than 24.04 are capable of supporting AppArmor
prompting if they are running a new enough Ubuntu kernel.

In particular, the Ubuntu 22.04 image on GCP runs a 6.5 kernel which
supports prompting. Thus, if run on GCP, Jammy supports prompting, but
if run on lxd, it does not.

Therefore, checks for Ubuntu version are insufficient for spread to
identify whether prompting should be supported. Instead, check for the
presence of the `prompt` directive in the permissions listed in
`/sys/kernel/security/apparmor/features/policy/permstable32`.

Any Ubuntu system which has `prompt` in that file should support
prompting. This can't be the entirety of the check, because those
permissions have been upstreamed to the Linux kernel, but other pieces
which are required for prompting to function have not yet been
upstreamed. Thus, Debian Sid reports supporting the prompt directive,
even though prompting is not supported.

Therefore, we must check that the system is Ubuntu, that it's not core,
and that the `prompt` directive is supported.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…s't include file

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@ZeyadYasser
Copy link
Contributor

Looks like prompting client integration tests pass on 22.04 with whatever kernel the cloud image uses. Interesting...

I checked, gcp is using a recent 6.5 custom kernel which has all the features needed by prompting.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…ests

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
… system

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder olivercalder force-pushed the prompting-add-client-integration-tests branch from e33f87b to d135758 Compare September 10, 2024 22:49
@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Sep 10, 2024
@ernestl ernestl added this to the 2.67 milestone Sep 19, 2024
@olivercalder
Copy link
Member Author

Rather than having a dependency cycle between prompting client integration tests and snapd, I re-implemented most of those integration tests as dedicated spread test cases in #14518. Closing this in favor of #14518.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Documentation -auto- Label automatically added which indicates the change needs documentation Squash-merge Please squash this PR when merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants