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: Add a caveat for setpriv #93

Open
akcano opened this issue Jun 14, 2024 · 9 comments
Open

Snapcraft: Add a caveat for setpriv #93

akcano opened this issue Jun 14, 2024 · 9 comments
Assignees

Comments

@akcano
Copy link
Collaborator

akcano commented Jun 14, 2024

Update the following guides:

Background

The documentation suggests using the cleanup part, but does not fully warn about the potential problems of using it. For instance, when using setpriv, setpriv may end up being removed from a snap's prime even though it's a part of the core20 image. A similar issue was reported in the Snapcraft forum.

Prerequisites

Familiarity with Snapcraft or snaps is needed to understand the content and test any conjectures. The documentation is Discourse-hosted, so some experience with Discourse is also a plus.

@akcano akcano changed the title Snapcraft: Clarify dump plugin reference Snapcraft: Add a caveat for setpriv Jun 14, 2024
@userMaximilian
Copy link
Contributor

Hello, @akcano! I would like to help with this issue. I have already taken a look at the background and I was able to reproduce the removal of setpriv from a core22-based snap after adding the cleanup part.

I wondered whether it would be worth providing readers with a possible workaround, for example an additional part that runs after cleanup to re-introduce setpriv (perhaps by manually extracting the setpriv binary from the util-linux deb package using override-prime).

Another option would be to update the cleanup part itself, so that snap developers could specify files - like the setpriv binary - that shouldn't be removed. I appreciate that updating the cleanup part is probably out-of-scope of this issue, but I wanted to raise the possibility of doing so here, as it would affect how we update the documentation, and it could be useful in other cases (like the forum post that you linked to). As a proof of concept, the override-prime step could start off with something like this:

    override-prime: |
      set -eux

      # Set SNAPS to a space separated list comprising the name of the base snap
      # and the name of each content snap that your snap is connected to
      # (e.g. "core22 gtk-common-themes gnome-42-2204")
      SNAPS="core22"

      # Set WANTED to a space separated list comprising the name (without path) of 
      # each file that needs to be kept in the snap (e.g. "setpriv whoami")
      WANTED="setpriv"

      wanted_opts=""
      for item in $WANTED; do wanted_opts+=" -not -name $item"; done
      for snap in $SNAPS; do
        cd "/snap/$snap/current" && find -L . -type f,l $wanted_opts -exec rm -f "$CRAFT_PRIME/{}" \;
      done

I have a few other changes in mind (e.g. removing empty directories, broken symlinks and unnecessary files from $CRAFT_PRIME/usr/share) but I have left these out for now.

What do you think? If you agree that it would be worth pursing, then I assume that it would be best for me to raise this on the Snapcraft forum - perhaps as a new thread for greater visibility - and allow some time for comments. I'm happy to keep working on this as needed.

On a separate point, would also it be worth updating the System usernames page to refer to _daemon_ as the system user throughout, and only mention the deprecated snap_daemon user in a note (i.e. the inverse of what is currently being done)? I appreciate that some users might be limited to an old version of snapd, and so may not be able to run snaps that rely on the _daemon_ user. I'm happy to make this change as part of my pull request, unless you think that it isn't needed, or that it should be done as a separate academy issue.

@akcano
Copy link
Collaborator Author

akcano commented Jul 1, 2024

Hi @userMaximilian,

Thank you for your participation! Step by step:

Workaround for setpriv removal: sounds promising, and I suggest you run it by the broader Snapcraft forum community first for feedback. Same for the cleanup part, if you can keep the explanation of what's going on to a necessary minimum.

System usernames page: I would suggest opening a new CODA issue ticket for that in this repo, otherwise it's a great idea.

Looking forward to your contributions!

@userMaximilian
Copy link
Contributor

Thanks, @akcano! I will make a start on this shortly.

@userMaximilian
Copy link
Contributor

Hi @akcano,

As a quick update:

  • I posted a thread on the Snapcraft forum earlier today to prompt a discussion about the cleanup+setpriv issue, possible workarounds, and potential improvements to the cleanup part. Once I have some feedback, I'll work on updating both the Reducing the size of desktop snaps and System usernames pages as needed. If you think it would be worth posting an interim workaround please let me know as I have something in mind.

  • In the forum post, I also raised the idea of making further amendments to the cleanup part that go beyond addressing the setpriv issue - if these progress to anything, then I'll raise a further CODA issue here to separate those changes out.

  • On a separate (but somewhat related) note, I have filed a bug report asking for the snapd AppArmor rules to be changed, to enable developers to run setpriv from the base snap on core20 onwards. This would sidestep the cleanup+setpriv issue in most situations. It's not going to affect the documentation any time soon, given that it'll take a while to propagate to a snapd release (if it's even an acceptable change) but I thought I would mention it here for completeness.

  • I still need to post the spin-out CODA issue regarding the System usernames page. Partly for my own future reference, there's a post on the Snapcraft forum about the Ubuntu HPC team experiencing an issue with the Snap Store rejecting snaps that use the _daemon_ username - hopefully this is resolved soon.

@akcano
Copy link
Collaborator Author

akcano commented Jul 29, 2024

Hi, @userMaximilian ! Thanks for getting back with an update, much appreciated. I think there's no need for interim postings to avoid propagating potentially suboptimal solutions.

@userMaximilian
Copy link
Contributor

Thanks, @akcano! I'll report again once I have any further updates - hopefully in the next week or two.

@userMaximilian
Copy link
Contributor

Hi @akcano,

This is just another brief update about the setpriv+cleanup issue. Unfortunately, I don't have much to report at the moment.

I haven't received any community feedback that specifically addresses the proposed workarounds/solutions mentioned in my forum post. The thread has been inactive for five weeks, so I doubt that I'm going to receive any further comments unless someone specifically searches for it.

I also haven't received any responses to my bug report about enabling access to setpriv as bundled with the base snap. As this should resolve the issue quite neatly, without the need for a caveat or for any amendments to the cleanup part, I have tried to progress this further by filing a PR (fortunately it only involves a one line change to the AppArmor template): see canonical/snapd#14362.

Given your earlier comment about wanting to "avoid propagating potentially suboptimal solutions", I think it might be worth waiting a bit longer to see what happens with the PR. What do you think?

@dilyn-corner
Copy link

dilyn-corner commented Sep 13, 2024

I like the cleanup suggestion, although:

for item in $WANTED; do wanted_opts+=" -not -name $item"; done

Probably won't work; the scriptlet in this override is run through dash, and += isn't supported; change to for item in $WANTED; do wanted_opts="$wanted_opts -not -name $item"; done.

ETA: I suspect I am actually mistaken... I'll need to consult with Callahan but I guess overrides now set bash in the shebang? 🤔

I like your suggestion to extend the base apparmor profile; this would save some of my snaps a lot of extra effort getting setpriv :) hopefully snapd team responds to it...

@akcano
Copy link
Collaborator Author

akcano commented Sep 26, 2024

hi @userMaximilian ! sorry, took me a while. I'll ask around about the PR, thanks for bringing this to my attention.

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

No branches or pull requests

3 participants