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

snapcraft.yaml: enable unconfined mode in lxd-support interface #277

Conversation

alexmurray
Copy link
Contributor

@alexmurray alexmurray commented Jan 24, 2024

The associated PR for snapd is still open in canonical/snapd#13514 but opening this one against the lxd snap so the two can be reviewed in tandem.

This should not be merged until at least the snapd one has been AND the correct assumes syntax has been decided upon.

--

The old lxd-support interface allowed lxd to set itself to be unconfined - ie to not have any apparmor profile associated with it - but when the userns restrictions are enabled it would then still be blocked from using userns - so we then instead added the unconfined mode which allows a profile to essentially be the equivalent of the unconfined profile (ie no profile) - but then to also add in extra permissions (like the use of userns).

ie. the only way to use userns when the restrictions are enabled is to have an apparmor profile which grants the userns permission in apparmor - AND for the case of lxd which wants to be unconfined we then use the new unconfined profile mode as well

Signed-off-by: Alex Murray alex.murray@canonical.com

@tomponline
Copy link
Member

assumes syntax

What is this @alexmurray ?

snapcraft.yaml Outdated Show resolved Hide resolved
Signed-off-by: Alex Murray <alex.murray@canonical.com>
@alexmurray alexmurray force-pushed the enable-apparmor-unconfined-mode-via-interface-attribute branch from cac7a68 to 2f0fcab Compare March 14, 2024 11:54
@alexmurray
Copy link
Contributor Author

Rebased this onto current latest-edge and squashed the two commits into one.

@alexmurray
Copy link
Contributor Author

Hi folks, the snapd support for this feature landed in the 2.62 release of snapd - any chance you could merge this into the lxd snap? cc @tomponline @simondeziel

@tomponline
Copy link
Member

tomponline commented Mar 25, 2024

Hi @alexmurray

Before I can merge this we need to understand what happens if this is installed on a system without snapd2.62 (ie one without lxd-support-with-unconfined-mode support)?

The LXD snap needs operate on older Ubuntu releases that don't necessarily have this snapd version.

Please can you advise about the backwards compatibility of this change?

@alexmurray
Copy link
Contributor Author

The assumes: snapd2.62 means it can not be installed on a system without snapd 2.62 - so there is no risk here.

@tomponline
Copy link
Member

The assumes: snapd2.62 means it can not be installed on a system without snapd 2.62 - so there is no risk here.

Hrm, that sounds like it could be quite risky though - we would need to understand what happens on systems that do not yet have snapd 2.62 or where its pinned to a previous version.

@tomponline
Copy link
Member

There are multiple series of LXD:

5.0 - Previous LTS that needs to be able to run on >= Jammy (and ideally before that to allow upgrades).
5.21 - Current LTS, at minimal Noble, but ideally before that to allow upgrades.
latest - Currently the same as 5.21 but we can have higher release restrictions here - but we need to understand them well so we don't accidentally backport something to the LTS series that we shouldn't do.

So starting off we should consider the 5.0 series and what needs to be included in it to allow it to run with these restrictions and whether that will work on Jammy onwards. Also considering that LXD is preseeded into the Jammy images and so the snapd refresh may not have run on first LXD use.

@tomponline
Copy link
Member

@alexmurray shall we have a meeting about this to discuss?

@alexmurray
Copy link
Contributor Author

snapd 2.62 just migrated into the updates pocket for all the stable releases of Ubuntu and has been in the snapd snap stable channel for a while now too. So it should be quite safe to merge this PR now.

@tomponline
Copy link
Member

Thanks! We can try this now on various systems and check it doesn't cause a regression.

@tomponline
Copy link
Member

@mihalicyn this pr can help with your roadmap item to support restricted user namespaces.

@mihalicyn
Copy link
Member

I think we can merge this today, so we can test if there are any problems with it.

For me it looks perfectly safe and valid.

We have merged two other PRs related to this:
canonical/lxd#12713
#189

which are preparation to having this change if I understand correctly.

cc @tomponline

@tomponline tomponline merged commit fc5284c into canonical:latest-edge Jul 1, 2024
5 checks passed
@tomponline
Copy link
Member

Lets give it a go - this will need to be tested on bionic onwards please.

@tomponline
Copy link
Member

@alexmurray @mihalicyn this change is causing lxd snaps to be rejected by the snap store, so they are building ok in launchpad, but not being accepted for upload. Effectively this is now blocking latest/edge and will need to be reverted.

https://dashboard.snapcraft.io/snaps/lxd/revisions/29132/

unknown attribute 'enable-unconfined-mode' for interface 'lxd-support' (plugs) lint-snap-v2_plugs_attributes (lxd-support-with-unconfined-mode, enable-unconfined-mode)

@alexmurray please can you advise what the next steps are here?

@mihalicyn
Copy link
Member

Theoretically, this should allow using this option in snap store https://git.launchpad.net/review-tools/commit/reviewtools/sr_common.py?id=08e83bf8d0b36eb8ff82ae74e774f00d56493d5f

@alexmurray
Copy link
Contributor Author

@tomponline we are waiting on the store team to deploy the latest review-tools revision to the store dashboard - in the meantime we can manually approve revisions which I will do now - and I will poke the store team to get the latest review-tools deployed.

@tomponline
Copy link
Member

in the meantime we can manually approve revisions which I will do now

How do we do that?

@alexmurray
Copy link
Contributor Author

Unfortunately it's a manual process to approve each revision, apologies for the delay. They are now published.

@tomponline
Copy link
Member

Do you know how long it will take to get the snapstore updated? We may need to revert this change if its going to take a while.

@roosterfish
Copy link
Contributor

Hi, I am seeing some permission errors in the snap. Maybe related to those changes here?

When trying to force stop an instance in error state using lxc stop -f v1 I get:
Error: Failed to stop VM process 227032: permission denied

Kernel log shows this apparmor denied message:
[33170.368181] audit: type=1400 audit(1719850282.401:24245): apparmor="DENIED" operation="signal" class="signal" profile="lxd-v1_</var/snap/lxd/common/lxd>" pid=227614 comm="lxd" requested_mask="receive" denied_mask="receive" signal=kill peer="snap.lxd.daemon"

It's reproducible by repeating this https://github.com/canonical/lxd-ci/blob/main/tests/storage-vm#L226

@mihalicyn
Copy link
Member

@roosterfish thanks for reporting this, Julian!

Related to:
https://bugs.launchpad.net/apparmor/+bug/2067900

mihalicyn added a commit to mihalicyn/lxd that referenced this pull request Jul 17, 2024
Right now this patch does not change anything, because
user namespaces are always allowed. But after we merge
canonical/lxd-pkg-snap#277
user namespaces become restricted by default and we need
to explicitly allow it when needed.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
tomponline added a commit to canonical/lxd that referenced this pull request Jul 18, 2024
Right now this patch does not change anything, because user namespaces
are always allowed. But after we merge
canonical/lxd-pkg-snap#277 user namespaces
become restricted by default and we need to explicitly allow it when
needed.
tomponline pushed a commit to tomponline/lxd that referenced this pull request Jul 22, 2024
Right now this patch does not change anything, because
user namespaces are always allowed. But after we merge
canonical/lxd-pkg-snap#277
user namespaces become restricted by default and we need
to explicitly allow it when needed.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
hamistao pushed a commit to hamistao/lxd that referenced this pull request Jul 23, 2024
Right now this patch does not change anything, because
user namespaces are always allowed. But after we merge
canonical/lxd-pkg-snap#277
user namespaces become restricted by default and we need
to explicitly allow it when needed.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
tomponline pushed a commit to tomponline/lxd that referenced this pull request Sep 13, 2024
Right now this patch does not change anything, because
user namespaces are always allowed. But after we merge
canonical/lxd-pkg-snap#277
user namespaces become restricted by default and we need
to explicitly allow it when needed.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
(cherry picked from commit 20b397e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants