interfaces/cups: add cups-socket-directory attr, use to specify mount rules in backend #10427

anonymouse64 · 2021-06-18T00:29:53Z

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:

name: cups
slots:
  cups:
    cups-socket-directory:  $SNAP_COMMON # or wherever is the best location, but this needs to be in the snap's mount namespace and not in somewhere that is shared with the host like /run or /etc
...

and then the consuming snap that wants to print:

name: cups-consumer # or whatever snap you want to test with
plugs:
  - cups

...

and install both of those snaps and connect the two sides of the connection:

$ snap install ...
$ snap connect cups-consumer:cups cups:cups

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.

tillkamppeter · 2021-06-18T07:50:57Z

In order to test this out, please download the snapd snap file from the GitHub Actions
for this PR

@anonymouse64, @jhenstridge, @pedronis, I do not find this GitHub Action you mean here. How do I find the snapd Snap you mean for testing?

anonymouse64 · 2021-06-18T12:08:42Z

@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

tillkamppeter · 2021-06-18T12:42:18Z

@anonymouse64 thank you very much. I got it now and I am setting up for testing ...

tillkamppeter · 2021-06-18T13:08:53Z

I tested now and my client, connected via

$ sudo snap connect cups-admin-test-no-control:cups cups:cups
$ snap connections | grep cups-admin-test-no-control
cups                      cups-admin-test-no-control:cups           cups:cups                        manual
home                      cups-admin-test-no-control:home           :home                            -
network                   cups-admin-test-no-control:network        :network                         -
$

and the client connects to my system's CUPS:

$ cups-admin-test-no-control.lpstat -v
device for commandtops-test: usb://HP/OfficeJet%20Pro%208730?serial=CN783F60W1&interface=1
device for Printer: ipps://HP%20OfficeJet%20Pro%208730%20%5B08C229%5D._ipps._tcp.local/
$

and not to the snapped CUPS.

Can it be that there is no CUPS_SERVER env variable automatically set by the interface? Do I have to set it manually? If yes. to which file/path?

Here is my client's snapcraft.yaml (does not need any additional files to build):

name: cups-admin-test-no-control
base: core18 # The base snap is the execution environment for this snap
version: 0.1.0
summary: CUPS admin and non-admin tasks out of a Snap 
description: Testing interfaces for the CUPS Snap (no cups-control plugging, so admin tasks should fail)
grade: stable
confinement: strict

apps:
  lpinfo:
    command: usr/sbin/lpinfo
    plugs: [network, cups]
  lpadmin:
    command: usr/sbin/lpadmin
    plugs: [network, avahi-observe, home, cups]
  lpstat:
    command: usr/bin/lpstat
    plugs: [network, avahi-observe, cups]
  lpoptions:
    command: usr/bin/lpoptions
    plugs: [network, home, cups]
  lp:
    command: usr/bin/lp
    plugs: [network, home, cups]
  cancel:
    command: usr/bin/cancel
    plugs: [network, cups]
  lpmove:
    command: usr/sbin/lpmove
    plugs: [network, cups]
  cupsenable:
    command: usr/sbin/cupsenable
    plugs: [network, cups]
  cupsdisable:
    command: usr/sbin/cupsdisable
    plugs: [network, cups]
  cupsaccept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups]
  cupsreject:
    command: usr/sbin/cupsreject
    plugs: [network, cups]
  accept:
    command: usr/sbin/accept
    plugs: [network, cups]
  reject:
    command: usr/sbin/reject
    plugs: [network, cups]
  cupsctl:
    command: usr/sbin/cupsctl
    plugs: [network, cups]

parts:
  cups-client:
    plugin: dump
    source: .
    stage-packages:
        - cups-client
        - libcups2
    prime:
        - usr/bin/*
        - usr/sbin/*
        - usr/lib/*

tillkamppeter · 2021-06-18T13:25:33Z

Also upgrading cups-admin-test-no-control to core20 does not help.

anonymouse64 · 2021-06-18T13:48:13Z

@tillkamppeter yes you need to set CUPS_SERVER to /var/cups/cups.sock, it is not set automatically. That path is just what I chose for the PR for you to test this as a concept, we can adjust that in this PR before it is released

tillkamppeter · 2021-06-18T14:32:48Z

I have added

environment:
  CUPS_SERVER: /var/cups/cups.sock

to the client's snapcraft.yaml now, rebuilt, and re-installed and it works now as expected. The CUPS Snap is running in proxy mode and jobs (lp command) are correctly forwarded to the system's CUPS whereas administrative commands (lpadmin for example) get denied.

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 CUPS_SERVER=/var/cups/cups.sock when they plug the cups interface. Then snappers could make the Snaps printing with ease.

The only change to the GIT master of the CUPS Snaps is:

diff --git a/snapcraft.yaml b/snapcraft.yaml
index 7ea197e..bc39f00 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -66,6 +66,7 @@ slots:
     interface: cups-control
   cups:
     interface: cups
+    cups-socket:  $SNAP_COMMON/run/cups.sock
 
 apps:
   cupsd:

Please tell when I should apply any changes to the CUPS Snap on GIT and which ones. Thanks.

Here is my current client's snapcraft.yaml with which my client works:

name: cups-admin-test-no-control
base: core20 # The base snap is the execution environment for this snap
version: 0.1.0
summary: CUPS admin and non-admin tasks out of a Snap 
description: Testing interfaces for the CUPS Snap (no cups-control plugging, so admin tasks should fail)
grade: stable
confinement: strict

environment:
  CUPS_SERVER: /var/cups/cups.sock

apps:
  lpinfo:
    command: usr/sbin/lpinfo
    plugs: [network, cups]
  lpadmin:
    command: usr/sbin/lpadmin
    plugs: [network, avahi-observe, home, cups]
  lpstat:
    command: usr/bin/lpstat
    plugs: [network, avahi-observe, cups]
  lpoptions:
    command: usr/bin/lpoptions
    plugs: [network, home, cups]
  lp:
    command: usr/bin/lp
    plugs: [network, home, cups]
  cancel:
    command: usr/bin/cancel
    plugs: [network, cups]
  lpmove:
    command: usr/sbin/lpmove
    plugs: [network, cups]
  cupsenable:
    command: usr/sbin/cupsenable
    plugs: [network, cups]
  cupsdisable:
    command: usr/sbin/cupsdisable
    plugs: [network, cups]
  cupsaccept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups]
  cupsreject:
    command: usr/sbin/cupsreject
    plugs: [network, cups]
  accept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups]
  reject:
    command: usr/sbin/cupsreject
    plugs: [network, cups]
  cupsctl:
    command: usr/sbin/cupsctl
    plugs: [network, cups]

parts:
  cups-client:
    plugin: dump
    source: .
    stage-packages:
        - cups-client
        - libcups2
    prime:
        - usr/bin/*
        - usr/sbin/*
        - usr/lib/*

anonymouse64 · 2021-06-18T15:30:55Z

@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.

tillkamppeter · 2021-06-20T23:05:47Z

@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?

anonymouse64 · 2021-06-21T12:06:19Z

@tillkamppeter it depends a bit on how we want folks to consume this, probably as I have mentioned before a new snapcraft extension called cups or something like this is easiest, that can then add the

plugs:
  cups:
    default-provider: cups

environment:
  CUPS_SERVER: /var/cups/cups.sock

to the yaml automatically if a snapcraft developer just adds:

extensions:
  - cups

to 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.

jhenstridge

This looks pretty good overall. I've left a few suggestions at some of the TODO markers below.

I suspect things will be simpler if you try to bind mount the directory containing the socket rather than the socket itself.

interfaces/builtin/cups.go

jhenstridge · 2021-07-06T08:56:30Z

cmd/snap-update-ns/change.go

+				// 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)
+				}


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.

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

interfaces/builtin/cups.go

pedronis · 2021-08-04T12:09:14Z

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

pedronis

reinforced some points made by @jhenstridge and also a consideration about the environment variable

interfaces/builtin/cups.go

tillkamppeter · 2021-09-15T15:40:46Z

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?
@jhenstridge @anonymouse64 @pedronis What is missing to bring this PR forward?
We need this interface to allow all-Snap Ubuntu and also to allow application snappers to easily upload applications with print capabilities (preferably before Snap gets replaced by something else).

anonymouse64 · 2021-10-31T22:43:53Z

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:

snapd/interfaces/builtin/content.go

Lines 221 to 230 in b59f7b3

    
           		fmt.Fprintf(contentSnippet, ` 
        
           # In addition to the bind mount, add any AppArmor rules so that 
        
           # snaps may directly access the slot implementation's files. Due 
        
           # to a limitation in the kernel's LSM hooks for AF_UNIX, these 
        
           # are needed for using named sockets within the exported 
        
           # directory. 
        
           `) 
        
           		for i, w := range writePaths { 
        
           			fmt.Fprintf(contentSnippet, "%s/** mrwklix,\n", 
        
           				resolveSpecialVariable(w, slot.Snap()))

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 <ian.johnson@canonical.com>

anonymouse64 · 2021-11-01T18:45:12Z

Alright I updated this as per some of the recommendations and here's a high-level summary of the changes:

The attribute is now a directory where we expect the cups.sock file to be in, not the specific cups socket file, since as James pointed out, doing it with the file alone is a bit racy
The example snaps I have in GitHub are updated to use the new cups-socket-dir attribute (well really just the provider since the consumer doesn't need to be changed at all), see the README there for more details
As such, the snap-update-ns changes are now dropped, and I don't plan on reintroducing them until such a day we need to be able to mount sockets from snap-update-ns for some other reason

Things not in this PR right now:

Some spread testing, not sure if we want spread tests around this, if we do then I can add them, but let's agree on the code here before writing any spread tests
More unit tests around validating the path provided via cups-socket-dir, we should do this eventually but I didn't do that yet
the environment backend as suggested by @pedronis, I left this out since while yes snaps will almost certainly need to use CUPS_SERVER=/var/cups/cups.sock in order to use this interface and the cups snap appropriately, this seems like a bit of work and can be worked around by just documenting that snaps should set this environment variable manually in their wrappers / snap{,craft}.yaml files.

codecov-commenter · 2021-11-01T19:03:32Z

Codecov Report

Merging #10427 (65a97c4) into master (5227f66) will increase coverage by 0.01%.
The diff coverage is 85.10%.

❗ Current head 65a97c4 differs from pull request most recent head 513c7d0. Consider uploading reports for the commit 513c7d0 to get more accurate results

@@            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

Flag	Coverage Δ
unittests	`78.24% <85.10%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
interfaces/builtin/cups.go	`84.94% <84.61%> (-15.06%)`	⬇️
cmd/snap-exec/main.go	`75.00% <100.00%> (+0.44%)`	⬆️
store/cache.go	`69.23% <0.00%> (-1.93%)`	⬇️
asserts/ifacedecls.go	`93.09% <0.00%> (-1.40%)`	⬇️
overlord/hookstate/hookmgr.go	`74.67% <0.00%> (-0.65%)`	⬇️
gadget/update.go	`91.30% <0.00%> (ø)`
overlord/snapstate/flags.go	`100.00% <0.00%> (ø)`
asserts/constraint.go	`100.00% <0.00%> (ø)`
overlord/snapstate/snapstate.go	`83.30% <0.00%> (+0.01%)`	⬆️
dirs/dirs.go	`94.04% <0.00%> (+0.02%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5227f66...513c7d0. Read the comment docs.

mardy

LGTM, but even though we trust the cups snap, it's better to take extra precautions with the socket path.

interfaces/builtin/cups.go

tillkamppeter · 2021-11-03T00:17:38Z

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 (sudo snap install --dangerous snapd_2.53.1+git63c1ab0.63c1ab0_amd64.snap), after that taken the current GIT snapshot of the CUPS Snap and applied the following patch:

--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -66,6 +66,7 @@ slots:
     interface: cups-control
   cups:
     interface: cups
+    cups-socket-dir: $SNAP_COMMON/run
 
 apps:
   cupsd:

Completely rebuilt it:

snapcraft clean
snapcraft snap

Made it using parallel mode:

sudo mkdir -p /var/snap/cups/common/
sudo touch /var/snap/cups/common/no-proxy

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:

sudo snap install --dangerous cups_0.1.0_amd64.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:

lpstat -H
lpstat -v
lpinfo -v

The tools of the CUPS Snap access the Snap's CUPS:

cups.lpstat -H
cups.lpstat -v
cups.lpinfo -v

Check with these commands whether this is actually the case:

$ lpstat -H
/var/run/cups/cups.sock
$ cups.lpstat -H
/var/snap/cups/common/run/cups.sock
$

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 snapcraft.yaml for each client, nothing more. They package files from Ubuntu packages pulled in as "stage-packages" and only differ in used plugs and environment variables. They are also an example how developers/snappers should snap real-life applications.

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 snapcraft.yaml:

name: cups-admin-test-no-control
base: core20 # The base snap is the execution environment for this snap
version: 0.1.0
summary: CUPS admin and non-admin tasks out of a Snap 
description: Testing interfaces for the CUPS Snap (no cups-control plugging, so admin tasks should fail)
grade: stable
confinement: strict

environment:
  CUPS_SERVER: /var/cups/cups.sock

apps:
  lpinfo:
    command: usr/sbin/lpinfo
    plugs: [network, cups]
  lpadmin:
    command: usr/sbin/lpadmin
    plugs: [network, avahi-observe, home, cups]
  lpstat:
    command: usr/bin/lpstat
    plugs: [network, avahi-observe, cups]
  lpoptions:
    command: usr/bin/lpoptions
    plugs: [network, home, cups]
  lp:
    command: usr/bin/lp
    plugs: [network, home, cups]
  cancel:
    command: usr/bin/cancel
    plugs: [network, cups]
  lpmove:
    command: usr/sbin/lpmove
    plugs: [network, cups]
  cupsenable:
    command: usr/sbin/cupsenable
    plugs: [network, cups]
  cupsdisable:
    command: usr/sbin/cupsdisable
    plugs: [network, cups]
  cupsaccept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups]
  cupsreject:
    command: usr/sbin/cupsreject
    plugs: [network, cups]
  accept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups]
  reject:
    command: usr/sbin/cupsreject
    plugs: [network, cups]
  cupsctl:
    command: usr/sbin/cupsctl
    plugs: [network, cups]

parts:
  cups-client:
    plugin: dump
    source: .
    stage-packages:
        - cups-client
        - libcups2
    prime:
        - usr/bin/*
        - usr/sbin/*
        - usr/lib/*

Note especially that all the tools under apps: (they are the command line tools which come with CUPS for this simple example) plug the "cups" interface, not "cups-control". Also important for the "cups" interface as it is now actually to work, the CUPS_SERVER environment variable has to be set to /var/cups/cups.sock. This is where inside the client Snap the CUPS Snap's socket is mounted/linked to. Note that it is **/var/**cups/cups.sock, not **/run/**cups/cups.sock. We will see later that the administrative CUPS tools (lpadmin, lpinfo, ...) will not work in this Snap.

The second Snap is an application which is supposed to also do administrative tasks on CUPS. All the tools under apps: plug "cups-control". Note especially that also the non-admin tools plug "cups-control". One can also print through "cups-control" not only manage CUPS and NEVER use both "cups" and "cups-control" in the same client Snap. This makes a mess which will confuse users. Also do not set the CUPS_SERVER environment variable in a Snap which uses "cups-control". Note also that when uploading a Snap to the Snap Store, auto-connection of "cups-control" needs permission by the Snap Store team. Here is the snapcraft.yaml:

name: cups-admin-test
base: core18 # The base snap is the execution environment for this snap
version: 0.1.0
summary: CUPS admin and non-admin tasks out of a Snap 
description: Testing interfaces for the CUPS Snap
grade: stable
confinement: strict

apps:
  lpinfo:
    command: usr/sbin/lpinfo
    plugs: [network, cups-control]
  lpadmin:
    command: usr/sbin/lpadmin
    plugs: [network, avahi-observe, home, cups-control]
  lpstat:
    command: usr/bin/lpstat
    plugs: [network, cups-control]
  lpoptions:
    command: usr/bin/lpoptions
    plugs: [network, home, cups-control]
  lp:
    command: usr/bin/lp
    plugs: [network, home, cups-control]
  cancel:
    command: usr/bin/cancel
    plugs: [network, cups-control]
  lpmove:
    command: usr/sbin/lpmove
    plugs: [network, cups-control]
  cupsenable:
    command: usr/sbin/cupsenable
    plugs: [network, cups-control]
  cupsdisable:
    command: usr/sbin/cupsdisable
    plugs: [network, cups-control]
  cupsaccept:
    command: usr/sbin/cupsaccept
    plugs: [network, cups-control]
  cupsreject:
    command: usr/sbin/cupsreject
    plugs: [network, cups-control]
  accept:
    command: usr/sbin/accept
    plugs: [network, cups-control]
  reject:
    command: usr/sbin/reject
    plugs: [network, cups-control]
  cupsctl:
    command: usr/sbin/cupsctl
    plugs: [network, cups-control]

parts:
  cups-client:
    plugin: dump
    source: .
    stage-packages:
        - cups-client
        - libcups2
    prime:
        - usr/bin/*
        - usr/sbin/*
        - usr/lib/*

Now build and install the two. Put the two snapcraft.yaml files into two empty directories and build the Snaps the usual way and install them.

Now we get

$ cups-admin-test-no-control.lpstat -H
/var/cups/cups.sock
$ cups-admin-test-no-control.lpstat -v
device for printer_on_snap: ...
$ cups-admin-test-no-control.lpinfo -v
lpinfo: Forbidden
$ cups-admin-test-no-control.lpadmin -x printer_on_snap
lpinfo: Forbidden
$ cups-admin-test-no-control.lpstat -v
device for printer_on_snap: ...
$
$ cups-admin-test.lpstat -H
/run/cups/cups.sock
$ cups-admin-test.lpstat -v
device for printer_on_system
$ cups-admin-test.lpinfo -v
network ipps
network beh
...
$ cups-admin-test-no-control.lpadmin -x printer_on_system
$ cups-admin-test.lpstat -v
No destinations found
$

If you get something like

$ cups-admin-test.lpinfo -v
lpinfo: Success
$ cups-admin-test-no-control.lpadmin -x printer_on_system
lpadmin: Success
$

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 <ian.johnson@canonical.com>

…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 <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 · 2022-01-26T00:04:53Z

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 <ian.johnson@canonical.com>

tillkamppeter · 2022-01-26T02:37:30Z

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 snap version, then ran the three commands, which worked and then, to assure that they did not work as they ran on an old instance of snapd, I ran sudo /usr/lib/snapd/snap-discard-ns cups-admin-test-no-control, ran the commands again, and they worked again. Seems to be all OK now.

pedronis

did another pass, there seem to be missing some unit tests. I'm also wondering about the attribute name, see comment about that.

cmd/snap-exec/main_test.go

pedronis · 2022-01-27T17:34:53Z

interfaces/builtin/cups.go

+	// 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/",


do we have a plan now to add those empty dirs to the base snaps?

interfaces/builtin/cups.go

These are already checked in other unit tests so we don't need to check them again here. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

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 <ian.johnson@canonical.com>

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

tillkamppeter · 2022-01-28T01:01:05Z

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:

--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -66,6 +66,7 @@ slots:
     interface: cups-control
   cups:
     interface: cups
+    cups-socket-directory: $SNAP_COMMON/run
 
 apps:
   cupsd:

I installed the new snapd Snap first, then the new CUPS Snap. After that the 3 commands with cups-admin-test-no-control did all not work. So I tried

sudo /usr/lib/snapd/snap-discard-ns cups-admin-test-no-control

Still did not work.
So I actually disconnected and re-connected:

sudo snap disconnect cups-admin-test-no-control:cups
sudo snap connect cups-admin-test-no-control:cups cups

Now the three commands work again.
I did not modify nor rebuild the cups-admin-test-no-control Snap.

Another question:

I get

$ sudo snap connect cups-admin-test-no-control:cups
error: snap "snapd" has no "cups" interface slots
$ sudo snap connect cups-admin-test-no-control:cups cups
$

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).

pedronis

thanks

anonymouse64 · 2022-01-31T16:31:56Z

@tillkamppeter

it's unexpected that this stopped working for you to update, I'm not sure what happened on your machine, but I will re-run my tests to test the upgrade scenario you mentioned.
running snap connect cups-admin-test-no-control:cups is indeed expected to fail - you need to specify the slot since it is not an implicit system slot, so the full command is actually snap connect cups-admin-test-no-control:cups cups:cups

anonymouse64 · 2022-01-31T20:06:44Z

@tillkamppeter I can't reproduce the issue you mentioned, to be clear I did the following set of steps:

installed snapd from stable
disconnected cups-admin-test-no-control:cups
installed snapd "before" I pushed the most recent changes (commit af5832b specifically)
installed updated cups snap with the new attribute name
re-connected cups-admin-test-no-control:cups so that the connection is as if you already had the snap installed
installed the most recent version of snapd from this branch (commit 513c7d0 specifically)
ran cups-admin-test-no-control.lpstat -H and -r and -v and got the expected outputs

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:

Ensure that you have a version of the cups snap installed with the new attribute cups-socket-directory
Refresh snapd back to stable with snap refresh snapd --stable --amend
Refresh snapd to the most recent build in this PR
Discard the snap namespace for the snap with sudo /usr/lib/snapd/snap-discard-ns cups-admin-test-no-control (this is due to the snap-exec bug from the stale snapd tooling mount bug I described to you on MM)
Re-run your tests of the cups-admin-test-no-control snap again and see if they work

I am fairly confident that the tests will work if you do this sequence of events

tillkamppeter · 2022-01-31T20:19:59Z

@anonymouse64 I have done the test you suggested now and it works all as expected. So seems to be all OK.

anonymouse64 · 2022-01-31T20:40:32Z

@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

LGTM - thanks for the strict validation of cups-socket-directory in validateCupsSocketDirSlotAttr

quoting changes implemented as requested

jdstrand · 2022-02-08T00:36:44Z

Note especially that also the non-admin tools plug "cups-control". One can also print through "cups-control" not only manage CUPS and NEVER use both "cups" and "cups-control" in the same client Snap. This makes a mess which will confuse users.

This could be a check in the review-tools. Perhaps @alexmurray could pass that along?

alexmurray · 2022-02-08T01:26:59Z

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 snapcore/snapd#10427 (comment) Signed-off-by: Alex Murray <alex.murray@canonical.com>

anonymouse64 · 2022-03-08T21:12:39Z

For posterity, I filed PR's adding the /var/cups directories to all the base snaps:

hooks: mkdir /var/cups to avoid writable mimic creation for snaps using cups core#129
hooks: mkdir /var/cups to avoid writable mimic creation for snaps using cups core18#187
hooks: mkdir /var/cups to avoid writable mimic creation for snaps using cups core20#133
hooks: mkdir /var/cups to avoid writable mimic creation for snaps using cups core-base#33

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: snapcore/snapd#10427 Co-authored-by: Keebler408 <85420839+Keebler408@users.noreply.github.com> Co-authored-by: Julian Alarcon <alarconj@gmail.com>

anonymouse64 force-pushed the feature/cups-mount-backend branch from 8d39b6c to a8d46c4 Compare June 18, 2021 00:30

anonymouse64 mentioned this pull request Jun 18, 2021

interfaces: make cups interface implicit on classic #10023

Closed

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Jun 18, 2021

anonymouse64 requested a review from jhenstridge June 21, 2021 14:19

pedronis self-requested a review June 26, 2021 09:39

jhenstridge requested changes Jul 6, 2021

View reviewed changes

pedronis reviewed Aug 4, 2021

View reviewed changes

interfaces/builtin/cups.go Show resolved Hide resolved

interfaces/builtin/cups.go Outdated Show resolved Hide resolved

anonymouse64 force-pushed the feature/cups-mount-backend branch from a8d46c4 to 5701b08 Compare November 1, 2021 18:36

anonymouse64 changed the title ~~[RFC] interfaces/cups: add cups-socket attr, use to specify mount rules in backend~~ interfaces/cups: add cups-socket attr, use to specify mount rules in backend Nov 1, 2021

anonymouse64 requested review from jhenstridge and pedronis November 1, 2021 18:37

anonymouse64 marked this pull request as ready for review November 1, 2021 18:37

mardy approved these changes Nov 2, 2021

View reviewed changes

interfaces/builtin/cups.go Show resolved Hide resolved

interfaces/builtin/cups.go Outdated Show resolved Hide resolved

interfaces/builtin/cups.go Outdated Show resolved Hide resolved

anonymouse64 added 3 commits January 25, 2022 15:57

tests/main/interfaces-cups: adjust for new interface, use snaps from …

0d6a1c4

…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 <ian.johnson@canonical.com>

Merge branch 'master' into feature/cups-mount-backend

af5832b

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

tests/main/interfaces-cups: fix shellcheck issues

82f3d67

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 added the Needs security review Can only be merged once security gave a :+1: label Jan 27, 2022

pedronis reviewed Jan 27, 2022

View reviewed changes

anonymouse64 added 2 commits January 27, 2022 11:56

tests/cmd/snap-exec: rm unnecessary duplicated checks

b9514af

These are already checked in other unit tests so we don't need to check them again here. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

anonymouse64 requested a review from pedronis January 27, 2022 19:01

anonymouse64 changed the title ~~interfaces/cups: add cups-socket-dir attr, use to specify mount rules in backend~~ interfaces/cups: add cups-socket-directory attr, use to specify mount rules in backend Jan 27, 2022

tests/main/interfaces-cups: add notes about test snap sources

513c7d0

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis approved these changes Jan 28, 2022

View reviewed changes

alexmurray approved these changes Feb 7, 2022

View reviewed changes

alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Feb 7, 2022

anonymouse64 merged commit a2169a4 into snapcore:master Feb 7, 2022

anonymouse64 deleted the feature/cups-mount-backend branch February 7, 2022 15:13

interfaces/cups: add cups-socket-directory attr, use to specify mount rules in backend #10427

interfaces/cups: add cups-socket-directory attr, use to specify mount rules in backend #10427

Conversation

anonymouse64 commented Jun 18, 2021 • edited

tillkamppeter commented Jun 18, 2021

anonymouse64 commented Jun 18, 2021

tillkamppeter commented Jun 18, 2021

tillkamppeter commented Jun 18, 2021

tillkamppeter commented Jun 18, 2021

anonymouse64 commented Jun 18, 2021

tillkamppeter commented Jun 18, 2021

anonymouse64 commented Jun 18, 2021 • edited

tillkamppeter commented Jun 20, 2021

anonymouse64 commented Jun 21, 2021

jhenstridge left a comment

Choose a reason for hiding this comment

jhenstridge Jul 6, 2021

Choose a reason for hiding this comment

anonymouse64 Jul 6, 2021

Choose a reason for hiding this comment

pedronis commented Aug 4, 2021

pedronis left a comment

Choose a reason for hiding this comment

tillkamppeter commented Sep 15, 2021

anonymouse64 commented Oct 31, 2021

anonymouse64 commented Nov 1, 2021

codecov-commenter commented Nov 1, 2021 • edited

Codecov Report

mardy left a comment

Choose a reason for hiding this comment

tillkamppeter commented Nov 3, 2021

anonymouse64 commented Jan 26, 2022

tillkamppeter commented Jan 26, 2022

pedronis left a comment

Choose a reason for hiding this comment

pedronis Jan 27, 2022

Choose a reason for hiding this comment

tillkamppeter commented Jan 28, 2022

pedronis left a comment

Choose a reason for hiding this comment

anonymouse64 commented Jan 31, 2022

anonymouse64 commented Jan 31, 2022

tillkamppeter commented Jan 31, 2022

anonymouse64 commented Jan 31, 2022

alexmurray left a comment

Choose a reason for hiding this comment

jdstrand commented Feb 8, 2022

alexmurray commented Feb 8, 2022

anonymouse64 commented Mar 8, 2022

anonymouse64 commented Jun 18, 2021 •

edited

anonymouse64 commented Jun 18, 2021 •

edited

codecov-commenter commented Nov 1, 2021 •

edited