many: add the cups interface #1160

Merged
merged 9 commits into from May 30, 2016

Conversation

Projects
None yet
5 participants
Contributor

zyga commented May 11, 2016

This branch adds a cups interface that allows apps to print on classic. It was tested with snapped lpr.

zyga added some commits May 11, 2016

interfaces/builtin: add cups interface for classic
This patch adds a new interface that allows snaps running on classic to
print using the CUPS interface.

Tested-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: add cups as an implicit slot on classic
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
integration-tests: add a manual test case for cups
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Member

chipaca commented May 11, 2016

👍 💯

+ lpr:
+ plugin: nil
+ stage-packages: [cups-bsd]
+2. Ensure that the 'cups' interface is connected to lpr
@elopio

elopio May 11, 2016

Member

Please give the commands to ensure this.

+4. Ensure that it was added to the queue of the default CUPS printer. This can
+ be checked in the ubuntu-control-center under the printers applet. Right
+ click on the default printer and look at the queue. Ensure it contains the
+ new item.
@elopio

elopio May 11, 2016

Member

Thank you!

Member

elopio commented May 11, 2016

I think this new interface needs to be documented somewhere. Ideally, in a nice user-friendly document. Failing that, at least mention it in https://github.com/ubuntu-core/snappy/blob/master/docs/interfaces.md :)

interfaces/builtin/cups.go
+const cupsConnectedPlugAppArmor = `
+# Description: Can access cups daemon.
+# Usage: reserved
+#include <abstractions/cups-client>
@jdstrand

jdstrand May 11, 2016

Contributor

This policy looks ok but note that on Ubuntu the default user is in the lpadmin group. Currently this policy does not allow access to the org.opensuse.CupsPkHelper.Mechanism dbus interface (which is good), but a user in the lpadmin group can access /run/cups/cups.sock and do things, such as adjust /etc/cups/cupsd.conf:

$ sudo grep Browsing /etc/cups/cupsd.conf # file perms need root
Browsing Off

$ cupsctl Browsing=On  # access to the socket if you are in the lpadmin group does not

$ sudo grep Browsing /etc/cups/cupsd.conf # file perms need root
Browsing On

If you are not in the lpadmin group, you can not do this:

sudo -H -u clickpkg cupsctl Browsing=On
cupsctl: Forbidden

Note: running the above will remove comments from /etc/cups/cupsd.conf even if you revert with Browsing=off.

As such, this interface should probably not be autoconnected.

@tyhicks - I wonder if the launcher should drop supplemental groups when launching commends. Granted, that doesn't help for a malicious service that is running as root....

@jdstrand

jdstrand May 11, 2016

Contributor

"As such, this interface should probably not be autoconnected."

Ideally, cups would in addition to checking group membership check the security label and ask snapd if it should allow printing, configuration, etc.

@zyga

zyga May 23, 2016

Contributor

Disabled autoconnect

Contributor

jdstrand commented May 12, 2016

In addition to setting admin users, auth, limits, policy, etc, configuring cupsd via cupsctl allows for a trivial full breakout using (at least) SetEnv to be used to manipulate a child process to execute arbitrary code (that would normally be confined by the system cups AppArmor policy, but with cups filters you get more....).

It is probably best to call this "cups-control", mark it reserved and to not autoconnect. Clients should use IPP ipp://localhost:631 to print just as if they were connecting to a remote printer. While WebInterface is set to On in cupsd.conf, when connecting via http:localhost:631 a BasicAuth prompt is presented for administration functions and clients (root or not) would not be able to configure cupsd.conf, etc.

CC: @tyhicks

Contributor

niemeyer commented May 19, 2016

+1 on cups-control. @zyga, this needs some love.

@niemeyer niemeyer added the Reviewed label May 19, 2016

Contributor

zyga commented May 23, 2016

Renamed to cups-control, disabled autoconnect and de-conflicted.

interfaces/builtin/cups_control.go
+import "github.com/ubuntu-core/snappy/interfaces"
+
+const cupsConnectedPlugAppArmor = `
+# Description: Can access cups daemon.
@jdstrand

jdstrand May 23, 2016

Contributor

Please adjust to be:

# Description: Can access cups control socket. This is restricted because it provides
# privileged access to configure printing.
@zyga

zyga May 30, 2016

Contributor

Done

snap/implicit.go
@@ -43,6 +43,7 @@ var implicitClassicSlots = []string{
"opengl",
"network-manager",
"pulseaudio",
+ "cups-control",
@jdstrand

jdstrand May 23, 2016

Contributor

I know they aren't now and it doesn't have to be in this PR, but it would be nice if these were alphabetized.

@zyga

zyga May 30, 2016

Contributor

Done

Contributor

jdstrand commented May 23, 2016

The security policy looks ok if you make the 'Description' change. Please also add documentation for this interface to docs/interfaces.md.

Contributor

zyga commented May 23, 2016

Ack, will do shortly

Contributor

niemeyer commented May 25, 2016

zyga added some commits May 30, 2016

docs: mention the cups-control interface
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: tweak description of cups-control
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: tweak private variable names in cups-control
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
snap: sort implicit slot lists
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented May 30, 2016

Applied all feedback, merged master to get repo rename changes. I will merge this once tests go green.

@zyga zyga merged commit 601c7de into snapcore:master May 30, 2016

3 checks passed

Integration tests Success
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zyga zyga deleted the zyga:ifaces-cups branch May 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment