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

revert: "interfaces/lxd-support: add support for allow all in AppArmr (#14251)" #14381

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

sergiocazzolato
Copy link
Collaborator

This reverts commit 4a0c15d.

The revert is required because this change produced the following issue when uninstalling snapcraft:
https://bugs.launchpad.net/snapd/+bug/2077101

@sergiocazzolato sergiocazzolato added the Simple 😃 A small PR which can be reviewed quickly label Aug 16, 2024
@sergiocazzolato sergiocazzolato changed the title Revert "interfaces/lxd-support: add support for allow all in AppArmr (#14251)" revert: "interfaces/lxd-support: add support for allow all in AppArmr (#14251)" Aug 16, 2024
Copy link
Contributor

@alexmurray alexmurray 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, apologies for the regression.

Copy link
Collaborator

@ernestl ernestl left a comment

Choose a reason for hiding this comment

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

Thanks

@ernestl ernestl merged commit 2cbcac5 into canonical:master Aug 16, 2024
15 of 17 checks passed
@tomponline
Copy link
Member

@alexmurray does this mean we wont need to switch to lxd-support-with-unconfined-mode from canonical/lxd-pkg-snap#277

@alexmurray
Copy link
Contributor

@tomponline well this got reverted (since I hadn't realised we would need some equivalent changes on the lxd snap side to handle this new type of confinement) - so we still need to use the unconfined-mode until I can rework this into a more complete solution.

@tomponline
Copy link
Member

@alexmurray thanks, what additional changes are needed?

@alexmurray
Copy link
Contributor

@tomponline since lxd tries to escape confinement, similar to the case where it is currently looking for either the unconfined AppArmor label or the (unconfined) profile mode, it would then need to recognise this allow_all mode and try to not escape confinement in this case (since it should already have all the required permissions from AppArmor). But given the experience with this PR here for snapd, I think it is clear I originally underestimated the integration work required between lxd and snapd, so this requires more careful consideration. However, since the current approach for LXD works well (and the new (unconfined) profile mode essentially acts as a substitute for the allow_all mode), there is no rush to implement this and I think it would end up as an item for the 25.04 roadmap if anything.

@tomponline
Copy link
Member

there is no rush to implement this and I think it would end up as an item for the 25.04 roadmap if anything.

The only downside to the current approach is that the LXD snap needs to keep disabling restricted unprivileged namespaces system-wide right?

@alexmurray
Copy link
Contributor

The only downside to the current approach is that the LXD snap needs to keep disabling restricted unprivileged namespaces system-wide right?

Yes, this is true. Given the current issues with the (unconfined) mode approach (ie since lxd now has an AppArmor label, it is not treated as label=unconfined anymore and hence its signals get blocked by AppArmor etc) I think we need to go back to the drawing board and figure out a better solution overall for LXD and its interaction with AppArmor, particularly when the userns restrictions are in place, as the current approach of disabling the restriction globally is not optimal.

@tomponline
Copy link
Member

I think we need to go back to the drawing board and figure out a better solution overall for LXD and its interaction with AppArmor, particularly when the userns restrictions are in place, as the current approach of disabling the restriction globally is not optimal.

Agreed, thanks. Let us know when you are ready to discuss other approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants