interfaces/cups: add cups-socket-directory attr, use to specify mount rules in backend #10427
Conversation
anonymouse64
force-pushed
the
feature/cups-mount-backend
branch
from
8d39b6c to
a8d46c4
Compare
pedronis
added
the
Needs Samuele review
@anonymouse64, @jhenstridge, @pedronis, I do not find this GitHub Action you mean here. How do I find the snapd Snap you mean for testing? |
|
@tillkamppeter go to https://github.com/snapcore/snapd/actions/runs/948262876, then scroll to the bottom where it says artifacts, there you can download the snapd snap from this branch |
|
@anonymouse64 thank you very much. I got it now and I am setting up for testing ... |
|
I tested now and my client, connected via and the client connects to my system's CUPS: and not to the snapped CUPS. Can it be that there is no Here is my client's |
|
Also upgrading |
|
@tillkamppeter yes you need to set CUPS_SERVER to |
|
I have added to the client's I think we can go with `/var/cups/cups.sock, especially as it does not perfectly conform with the FHS we can expect that no package's files will conflict with it. What would be perfect is if client Snaps could auto-set The only change to the GIT master of the CUPS Snaps is: Please tell when I should apply any changes to the CUPS Snap on GIT and which ones. Thanks. Here is my current client's |
|
@tillkamppeter that environment variable setting could probably be done with a snapcraft extension, which is not added to snapd, but rather to snapcraft itself. Regarding what changes you should make to cups upstream to account for this, please hold off on doing any such changes until we have agreement on this PR, we have had disagreement before and I'm hoping we can get agreement on this path forward now, but I may be wrong and there may still be disagreements about this implementation. |
|
@anonymouse64 OK, thanks. So when this gets settled in snapd, could you then post a PR for the snapcraft extension to auto-set the variable? |
|
@tillkamppeter it depends a bit on how we want folks to consume this, probably as I have mentioned before a new snapcraft extension called plugs:
cups:
default-provider: cups
environment:
CUPS_SERVER: /var/cups/cups.sockto the yaml automatically if a snapcraft developer just adds: extensions:
- cupsto their snapcraft.yaml. Also, lastly the final thing after this PR that we would want is to enable default-provider for the cups snap. Given the explanation I gave above about why we can now auto-connect any snap to the cups snap specifically on every distro, that should be sufficient justification for enabling default-provider for the cups interface I think, but that discussion will be held in another PR, not on this one. |
cmd/snap-update-ns/change.go
Outdated
| // this | ||
| if !(fi.Mode()&os.ModeSocket == os.ModeSocket && c.Entry.AllowSocketFile()) { | ||
| err = fmt.Errorf("cannot use %q as mount point: not a regular file", path) | ||
| } |
There was a problem hiding this comment.
Would it make sense to always allow mounting over sockets? In a quick test, it seems the kernel has no problem mounting regular files over the top of sockets, so we should just be erroring out if the target is a directory.
There was a problem hiding this comment.
I think the original intention of this code was to be as conservative and explicit as possible to reduce the chance of bugs which I think is fair, and considering that this was the first use case that we actually have for this situation of mounting a socket (which may actually go away if we end up going with what you suggested and just bind mount the directory), I think it's okay to leave as-is
|
if we need to have CUPS_SERVER set I don't think it should be done by an extension, instead we should have an environment backend at least conceptually that enables interfaces to specify environment variables that should be set when running applications from a snap |
There was a problem hiding this comment.
reinforced some points made by @jhenstridge and also a consideration about the environment variable
|
Now already a month has passed without anything moving here. What is still missing to get the "cups" interface for snapd out of Draft and into snapd? |
|
So I looked at this again, and actually we will need the AppArmor rule I mentioned as being necessary earlier, even if we just mount the directory and not specifically the socket. This is due to a bug that is known and is already handled in the content interface, as per this PR: #2462 and this comment in the content interface: So I think it is fine to add this, and again it doesn't end up sharing any additional files, since we are mounting it anyways so allowing to access via the original source does not provide anything that accessing via the target, except that it allows clients like this to actually work. |
…backend Make the cups interface slot have an optional attribute, cups-socket-dir, which is used to specify the directory where the cups socket that the slotting snap is going to share with client snaps. This directory then is mounted into client snap's mount namespaces via the mount backend at /var/cups/, such that client applications wishing to print need to adjust the location of where they expect the cups socket to be at all, but then will always end up sending print requests to the cups snap, which will mediate requests to print based on whether or not the client snap has a connected cups plug or not. The primary advantage of this is that it means we don't need to make the cups interface implictly slotted by the system snap, and it can always be provided by the cups snap, and we can then make any snap with a cups interface plug auto-connect to the slot from the cups snap unambiguously, providing all snap clients the ability to print without any extra connections necessary, regardless of their distro or what the client snap is. Signed-off-by: Ian Johnson <[email protected]>
anonymouse64
force-pushed
the
feature/cups-mount-backend
branch
from
a8d46c4 to
5701b08
Compare
anonymouse64
changed the title
|
Alright I updated this as per some of the recommendations and here's a high-level summary of the changes:
Things not in this PR right now:
|
Codecov Report
@@ Coverage Diff @@
## master #10427 +/- ##
==========================================
+ Coverage 78.23% 78.24% +0.01%
==========================================
Files 928 929 +1
Lines 106103 106200 +97
==========================================
+ Hits 83010 83098 +88
- Misses 17930 17934 +4
- Partials 5163 5168 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Second real-life test succeeded! Taken snapd from the "Artifacts" at the bottom of https://github.com/snapcore/snapd/actions/runs/1409198152 (snap-files.zip, the page is reachable by clicking the "Checks" tab right over the text of the initial description of this PR, then click "Tests" on the left side of the page which appears). Installed the snapd Snap ( Completely rebuilt it: Made it using parallel mode: Make sure you have also a classically installed CUPS of the latest version (2.4b1 or later, ideally current GIT snapshot from upstream, important is that this fix is applied). Now install the CUPS Snap: Make sure that all interfaces are connected, and connect them manually if needed, especially if you never had installed a CUPS Snap from the Snap Store on your system before. Classic CUPS is running on port 631 and the Snap on port 10631 now. Make sure that both have a print queue, ideally with different names. Use the web interfaces of the CUPS daemons, with the mentioned ports. Normal (classically installed) command line tools access the classic CUPS: The tools of the CUPS Snap access the Snap's CUPS: Check with these commands whether this is actually the case: Now create 2 client Snaps. These are the Snaps which I mentioned here for my first real-life test back in June. You only need a The first Snap is an application which prints but does not do any administrative tasks on CUPS. The snapper wants to upload his app to the Snap Store without hassle, especially without needing to ask for permission of auto-connecting interfaces. So he uses only the "cups" plug and not "cups-control". Here is the Note especially that all the tools under The second Snap is an application which is supposed to also do administrative tasks on CUPS. All the tools under Now build and install the two. Put the two Now we get If you get something like and the queue did not get removed, CUPS has crashed on you. Make sure you have the newest GIT snapshot from upstream or this patch applied. This is a summary of what I have tested and all succeeded as described here with the current snapd Snap as of this PR, the current CUPS Snap (current GIT snapshot with mentioned patch) and the current CUPS (Current GIT snapshot for both the Snap and the classic installation). Note that everything makes more sense if the CUPS Snap is running in proxy mode or stand-alone mode. Parallel mode is only for development and testing! A Snap plugging "cups" is supposed to force-install the CUPS Snap if the CUPS Snap is not already installed. The CUPS Snap starts in proxy mode if there is already a classic CUPS and in stand-alone mode otherwise. In any case the client plugging "cups" ONLY talks to the snapped CUPS, NEVER to the classic one. The CUPS Snap ALWAYS blocks admin tasks through the "cups" interface. Printing tasks get forwarded through the mirrored queues in the CUPS Snap running in proxy mode to the classic CUPS. If the CUPS Snap is the only CUPS and therefore is runningin stand-alone mode it prints the jobs by itself. A Snap plugging "cups-control" ALWAYS talks to the standard socket, the one which unsnapped apps also use. So it ignores a running CUPS Snap in proxy mode and so never messes with the mirrored queues there, as in this case the classic CUPS is on the standard socket. If there is no classic CUPS, the CUPS Snap (in stand-alone mode) listens on the standard socket in addition to its own socket and therefore the jobs and admin tasks of the client Snap plugging "cups-control" go to the CUPS Snap. So everything should work as intended now. |
…the store The new and improved version of these test snaps actually does real unix socket communication to make sure that it works. Signed-off-by: Ian Johnson <[email protected]>
…stractions This is because by design we want snaps using the cups interface to _only_ have access to the snapped version of cupsd's socket at /var/cups/cups.sock since we know that is always safe for snap apps to talk to, as it will always implement mediation. We do however grant access to ~/.cups/lpoptions since this is where things like default printer, etc. are configured and we do want client snaps to continue to be able to read this. Signed-off-by: Ian Johnson <[email protected]>
Signed-off-by: Ian Johnson <[email protected]>
|
Ok, spread tests are now passing on ubuntu at least, I pushed it up. I also changed the AppArmor policy slightly since I was inadvertently still allowing access to /run/cups/cups.sock when we don't want that. |
Signed-off-by: Ian Johnson <[email protected]>
|
Fifth test, like the forth, but with today's snapshot, to see whether the changes of today, especially removal of AppArmor permission rules, have no negative influence on the functionality. Today's snapshot is snapd 2.54.2+gitf720134.f720134, verified with |
anonymouse64
added
the
Needs security review
| // add a bind mount of the cups-socket-dir to /var/cups of the plugging snap | ||
| return spec.AddMountEntry(osutil.MountEntry{ | ||
| Name: cupsdSocketSourceDir, | ||
| Dir: "/var/cups/", |
There was a problem hiding this comment.
do we have a plan now to add those empty dirs to the base snaps?
These are already checked in other unit tests so we don't need to check them again here. Signed-off-by: Ian Johnson <[email protected]>
Test the mount specification that is generated for connected plugs, add more unhappy validation cases and also check the snap-update-ns snippets too. Thanks to Samuele for noticing these and suggesting the new attribute name. Signed-off-by: Ian Johnson <[email protected]>
anonymouse64
changed the title
Signed-off-by: Ian Johnson <[email protected]>
|
I have done my 6th test now, like the 5th, but with snapd 2.54.2+gita04dc60.a04dc60 (snapshot from today, after all your changes of today) and withe the CUPS Snap from GIT master but with following patch: I installed the new snapd Snap first, then the new CUPS Snap. After that the 3 commands with Still did not work. Now the three commands work again. Another question: I get Is it correct that the first command does not work? Should the first command not do exactly the same as the second does? snapd is snapd 2.54.2+gita04dc60.a04dc60 (the snapshot of today, see above). |
|
|
@tillkamppeter I can't reproduce the issue you mentioned, to be clear I did the following set of steps:
I don't know what went wrong with your test, but I think the fact that our spread tests are passing and that the cups-admin-test-no-control snap worked for me in the upgrade scenario (albeit from an intermediary commit that no one will ever have installed), and that after disconnecting and reconnecting your test works that we can safely ignore that failure. If you want to test again to be sure, I would ask that you test the following sequence of steps:
I am fairly confident that the tests will work if you do this sequence of events |
|
@anonymouse64 I have done the test you suggested now and it works all as expected. So seems to be all OK. |
|
@tillkamppeter that is great news thanks for testing again. To be clear, this PR is now only waiting on a security review. I don't think we need to wait for @jhenstridge's review, though it is of course appreciated to get an extra look. |
alexmurray
removed
the
Needs security review
anonymouse64
dismissed
jhenstridge’s stale review
quoting changes implemented as requested
This could be a check in the review-tools. Perhaps @alexmurray could pass that along? |
|
Thanks for the hint @jdstrand - I've taken a note to add something to review-tools for this. |
The new cups and cups-control plugs are now mutually exclusive so add a check to ensure snaps are not plugging both at the same time. See canonical/snapd#10427 (comment) Signed-off-by: Alex Murray <[email protected]>
|
For posterity, I filed PR's adding the /var/cups directories to all the base snaps:
|
With this change, the following commands are still required to enable printing: sudo snap install --edge cups snap connect prospect-mail:cups cups:cups It looks like the second command will not be required when running a version of snapd with the following change: canonical/snapd#10427 Co-authored-by: Keebler408 <[email protected]> Co-authored-by: Julian Alarcon <[email protected]>
With this change, the following commands are still required to enable printing: sudo snap install --edge cups snap connect prospect-mail:cups cups:cups It looks like the second command will not be required when running a version of snapd with the following change: canonical/snapd#10427 Co-authored-by: Keebler408 <[email protected]> Co-authored-by: Julian Alarcon <[email protected]>
Make the cups interface slot have an optional attribute, cups-socket-directory, which
is used to specify the directory where the cups socket that the slotting snap
is going to share with client snaps.
This directory then is mounted into client snap's mount namespaces via the
mount backend at /var/cups/, such that client applications wishing to
print need to adjust the location of where they expect the cups socket to
be at all, but then will always end up sending print requests to the cups snap,
which will mediate requests to print based on whether or not the client snap
has a connected cups plug or not.
The primary advantage of this is that it means we don't need to make the cups
interface implictly slotted by the system snap, and it can always be provided
by the cups snap, and we can then make any snap with a cups interface plug
auto-connect to the slot from the cups snap unambiguously, providing all snap
clients the ability to print without any extra connections necessary,
regardless of their distro or what the client snap is.
cc @tillkamppeter and @jhenstridge
In order to test this out, please download the snapd snap file from the GitHub Actions
for this PR, then install it with
snap install --dangerous, then setup your cups snap like so:and then the consuming snap that wants to print:
and install both of those snaps and connect the two sides of the connection:
and see if printing works and if everything works end-to-end, this worked with my test snaps that are not cups, and just shared a normal socket file so I'm 99% sure this should work, but of course there may be bugs as always. You can find the examples I used at https://github.com/anonymouse64/test-snapd-cups-provider and https://github.com/anonymouse64/test-snapd-cups-consumer.