/snapd

daemon,debian: add PolicyKit support to snapd #409

Closed
wants to merge 14 commits into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@robert-ancell @snappy-m-o @zyga @niemeyer @elopio @mvo5 @sergiusens @chipaca
@robert-ancell
Contributor

robert-ancell commented Feb 2, 2016

Use PolicyKit to authorize write access on the the snapd interface.
@snappy-m-o
Contributor

snappy-m-o commented Feb 2, 2016

Can one of the admins verify this patch?
@robert-ancell
Contributor

robert-ancell commented Feb 2, 2016

The parts of this patch I'm not 100% sure about:

  • CheckAuthorization() will block while a dialog is shown until the user provides a response. I don't know the snapd/go threading/loop model so I'm not sure if this will block up the snapd process or not. We can do this async with libsystemd if necessary, but again not sure how that interacts with the snapd threading model.
  • I created a policy for each path - this may not be an appropriate granularity.
  • We could (should?) also use policy for read actions. However this would mean if PolicyKit is not present only root works - is that fine?
  • The way the credentials are converted into a string is... interesting. I hope I've done the right thing shoehorning a new variable in there.
  • The C code is just thrown in daemon.go. I'm guessing it should probably be in a separate policykit.go - please let me know if so and what form it should take.
  • Is using the C code for libsystemd correct or should I be using godbus instead?
daemon/api.go
+ UserOK: true,
+ GET: getSnapsInfo,
+ POST: sideloadSnap,
+ PolicyKitAction: "com.ubuntu.snappy.sideload-snap",
@zyga

zyga Feb 2, 2016

Contributor

Personally I'd prefer to have a few documented constants that are referenced from all the Command objects.

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

Fixed.

daemon/daemon.go
+ // 14207 (gnome-terminal-) S 3399 3741 3741 0 -1 4194304 95756 7917395 23 857 6850 942 29913 2486 20 0 4 0 545930 692899840 12642 18446744073709551615 4194304 4486204 140735647896976 140735647896432 139643272960061 0 0 4096 65536 0 0 0 17 3 0 0 1 0 0 6585664 6596840 22155264 140735647901892 140735647901938 140735647901938 140735647903690 0
+
+ // Find the end of the name field, search from the right in case the name contains a ')'.
+ for (i = strlen (line) - 1; i > 0 && line[i] != ')'; i--);
@zyga

zyga Feb 2, 2016

Contributor

I'd use i >= 0 although here it makes little difference.

@chipaca

chipaca Feb 2, 2016

Member

@zyga there's no valid line with i < 2 anyway.

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

It has to be i > 0 otherwise the following line will fail:
if (line[i] != ')')

daemon/daemon.go
+
+ // A line looks like this (the 22nd field is the start time - 545930 in this case):
+ // 14207 (gnome-terminal-) S 3399 3741 3741 0 -1 4194304 95756 7917395 23 857 6850 942 29913 2486 20 0 4 0 545930 692899840 12642 18446744073709551615 4194304 4486204 140735647896976 140735647896432 139643272960061 0 0 4096 65536 0 0 0 17 3 0 0 1 0 0 6585664 6596840 22155264 140735647901892 140735647901938 140735647901938 140735647903690 0
+
@zyga

zyga Feb 2, 2016

Contributor

Could all of this hand-crafted manipulation be replaced by strtok_r()?

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

strrchr? Yes. I think initially I was using that and it had too much pointer arithmetic but it looks better now.

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

Fixed.

daemon/daemon.go
@@ -79,8 +177,13 @@ func (c *Command) canAccess(r *http.Request) bool {
isUser = true
}
- // only superuser can modify
+ logger.Debugf("canAccess %s %d %d '%s'", r.Method, pid, uid, c.PolicyKitAction)
@zyga

zyga Feb 2, 2016

Contributor

"canAccess %s %d %d %q"

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

Fixed.

daemon/ucrednet.go
const ucrednetNobody = uint32((1 << 32) - 1)
-func ucrednetGetUID(remoteAddr string) (uint32, error) {
+func ucrednetGet(remoteAddr string) (uint64, uint32, error) {
@zyga

zyga Feb 2, 2016

Contributor

Personally I'd replace all of the parsing below with a simple generic (and easier to verify) code that splits elements on ';' and then parses each as a key=value. Then the code below could just look up the key uid or pid, convert it to a number and do what it has to do.

Specifically, I'd use: https://golang.org/pkg/strings/#Split

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

Fixed.

data/com.ubuntu.snappy.policy
+
+ <action id="com.ubuntu.snappy.service-snap">
+ <description gettext-domain="system-service">Service snaps</description>
+ <message gettext-domain="system-service">Authentication is required to service snaps</message>
@zyga

zyga Feb 2, 2016

Contributor

What does it mean to "service" a snap?

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

I don't know - I added policy for all the write operations. I don't know what service means exactly for a snap - ideas for better names welcome.

@zyga
Contributor

zyga commented Feb 2, 2016

A few comments below but nothing terrible. I would love to see a check that this still builds and works fine on architectures which build go with gccgo as we had a few quirks around C interactions there.

@niemeyer niemeyer changed the title from Add PolicyKit support to snapd to daemon,debian: add PolicyKit support to snapd Feb 2, 2016

@niemeyer
Contributor

niemeyer commented Feb 2, 2016

Before reviewing this, can we have some more context for the proposal? This is likely a good idea, but it's not yet clear if it is a good idea right now.
daemon/daemon.go
+ if (reply)
+ sd_bus_message_unref (reply);
+ if (bus)
+ sd_bus_close (bus);
@chipaca

chipaca Feb 2, 2016

Member

There doesn't seem to be a manpage for sd_bus_close, does it also do sd_bus_unref?

@zyga

zyga Feb 2, 2016

Contributor

There is sd_bus *sd_bus_unref(sd_bus *bus);

@robert-ancell

robert-ancell Feb 2, 2016

Contributor

Fixed.

@robert-ancell
Contributor

robert-ancell commented Feb 2, 2016

@niemeyer the purpose of this change is to allow non-root users to install and remove snaps if the system policy allows. For example, a GUI application.
@niemeyer
Contributor

niemeyer commented Feb 3, 2016

Okay, let's catch up live somewhere to discuss details of this work.
@elopio
Member

elopio commented Feb 3, 2016

@robert-ancell and we need a user test for this. Talk to me or @fgimenez if you need a hand with this.
daemon/api.go
@@ -66,6 +66,12 @@ var api = []*Command{
assertsFindManyCmd,
}
+// PolicyKit action names
+const sideloadSnapAction = "com.ubuntu.snappy.sideload-snap"
@sergiusens

sergiusens Feb 3, 2016

Contributor

sideload these days means install from somewhere different than the store and provide all the required assertions. I think you are referring to developer snaps here, right?

@robert-ancell

robert-ancell Feb 4, 2016

Contributor

I just copied the associated method name sideloadSnap() - if there's a more appropriate name then happy to switch to that.

@mvo5
Collaborator

mvo5 commented Feb 22, 2016

I close this pull-request temporarily because we discussed various options at the recent sprint in SC and its not clear that this branch is the one we need.

@mvo5 mvo5 closed this Feb 22, 2016

@jhenstridge jhenstridge referenced this pull request Jul 12, 2017

Merged

daemon: add polkit support to /v2/login #3581

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