daemon: allow polkit authorisation to install/remove snaps #3797

jhenstridge · Aug 24, 2017

This branch extends polkit authorisation support to cover managing snaps (install, remove, refresh, enable, disable, etc).

The intention is to allow gnome-software to install and remove snaps without having to create a store account, similar to what is possible on the command line once the user has proven that they have administrative access with sudo.

I needed to alter the canAccess method to only perform the polkit authorisation check for non-GET requests, since the same API paths are used for non-privileged operations via GET (e.g. snap list, snap info, etc) and we don't want to prompt in those cases.

Here are some ways to manually test this:

the following operations should succeed without any prompts:
```
snap list
snap info core
```

the following commands should result in a prompt from polkit

snap install some_package
snap install some_package some_other_package
snap remove some_package
snap refresh some_package
snap refresh
snap enable some_package
snap disable some_package

jhenstridge · Aug 25, 2017

With this patch alone, I can install and remove packages via gnome-software without providing Ubuntu One account credentials. This is because gnome-software currently only performs a store login if it gets an access denied error from snapd. Assuming snapd gives a similar access denied error if we attempt to install a paid snap, this should give us a way to seemlessly upgrade the user to macaroon auth when they need it.

If this branch is used in conjunction with #3795, then it will also be necessary to apply snapcore/snapd-glib#14 so that gnome-software indicates that user interaction is allowed.

codecov-io · Aug 25, 2017

Codecov Report

Merging #3797 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3797      +/-   ##
==========================================
+ Coverage   75.71%   75.89%   +0.17%     
==========================================
  Files         413      416       +3     
  Lines       35546    35988     +442     
==========================================
+ Hits        26914    27313     +399     
- Misses       6729     6755      +26     
- Partials     1903     1920      +17

Impacted Files	Coverage Δ
daemon/api.go	`72.44% <ø> (-0.01%)`	⬇️
daemon/daemon.go	`64.31% <100%> (ø)`	⬆️
overlord/configstate/config/transaction.go	`80.4% <0%> (-4.78%)`	⬇️
cmd/snap-repair/runner.go	`83.59% <0%> (-1.35%)`	⬇️
errtracker/errtracker.go	`71.42% <0%> (-0.99%)`	⬇️
cmd/snap/cmd_run.go	`61.79% <0%> (-0.71%)`	⬇️
image/image.go	`67.06% <0%> (-0.3%)`	⬇️
overlord/assertstate/assertstate.go	`78.63% <0%> (-0.1%)`	⬇️
overlord/snapstate/storehelpers.go	`80% <0%> (ø)`	⬆️
interfaces/builtin/hardware_observe.go	`100% <0%> (ø)`	⬆️
... and 26 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 ade31f1...ad3ea74. Read the comment docs.

mvo5 · Aug 28, 2017

mvo5 reviewed Aug 28, 2017

View changes

Code wise this looks fine (and it is amazingly small). We need a policy/architectural review from @niemeyer here.

zyga · Aug 30, 2017

zyga approved these changes Aug 30, 2017

View changes

One comment about GET requests. Perhaps I'm missing some context but it feels like not something that needs special casing. It should be a per-endpoint decision IMO.

Otherwise LGTM

Hmmm, kind of unsure if all GETs should skip policykit check.

Without this change, commands like "snap list" may pop up an interactive dialog requesting authorisation. The command will still work if the user dismisses the dialog, but we don't want to unnecessarily prompt the user.

Part of the problem here is that there is only a single security policy per resource exported via the API, but in reality we're applying different policies to different request methods sent to that resource.

For example, while /v2/snaps entry has the UserOK flag set, the real policy is that GET is available to regular users while POST requires admin access. We want to avoid the polkit check completely for the GET request. I could probably tighten things up with something like !(r.Method == "GET" && (c.UserOK || c.GuestOK)), but that's encoding even more details about the rest of the policy.

Perhaps things would be clearer if the permissions were explicitly specified on a per request method basis?

jhenstridge · Sep 1, 2017

I've reorganised canAccess() a bit based on @zyga's question about special casing GET. It now grants access for guest and user commands first (provided it is a GET request, to match the existing policy), and handles admin access last. This ensures we only hit the polkit code path if the request actually needs admin privileges.

niemeyer · Sep 4, 2017

niemeyer requested changes Sep 4, 2017

View changes

Looks great, thanks!

Only question here is about the strategy for the permission IDs.

How fine grained do we plan this to be? Gut feeling is that "manage-snaps" might pretty much cover the entirety of snapd's functionality, and as such simply "manage" might be enough. Alternatively, do we want to distinguish installations/removals from configuration? If so, we probably need more specific names.

So, either something more specific, or if we plan for it to be widely scoped (which sounds fine in practice), perhaps simply "manage" might be enough.

Perhaps going wide is the sanest route for now, actually, as many of the actions are in fact different parameters on the request, so we'll need to change this model a bit if we want to be more fine grained.

If you want, I can change the name of the polkit action to something more specific (any particular preference?), and we can look at splitting the other operations off into other action IDs if needed.

I guess the main question is whether admins would want to grant some of these permissions to users but not others. For the policy defaults, I don't think it matters much though.

Can we please fix the change performed in the prior PR just before merging? The point was to use the default behavior (false) when the header was missing entirely, but keep the original logging if an error happens due to an invalid header.

/cc @mvo5

I've pushed a fix for this to add back the logging on garbage values, while not logging when the header is missing.

mvo5 · Sep 5, 2017

mvo5 reviewed Sep 5, 2017

View changes

Code looks very nice. Two nitpicks inside but feel free to ignore.

Not sure we want this logger.Noticef() here, we could just silently ignore if the client sends us silly data to avoid spamming syslog to much.

Above, @niemeyer specifically asked for the logging to be added back (although only if the client actually sends the request header: not if it is missing entirely). I've pushed an update to the test to verify that we're only logging on a garbage value now.

This isn't the only way clients can cause us to write log messages, and it is probably better to know that there is a misbehaving client.

I missed the message from Gustavo earlier, thanks for adding it back!

(nitpick) the comment is slightly misleading. AFAICT it does not test that the value is actually logged(?).

mvo5 · Sep 8, 2017

mvo5 approved these changes Sep 8, 2017

View changes

mvo5 · Sep 8, 2017

We need to squash merge this when it lands so that we can cherry pick it to 2.28.

pedronis requested a review from niemeyer Aug 24, 2017

mvo5 added this to the 2.28 milestone Sep 8, 2017

@@ -100,7 +100,7 @@ func (c *Command) canAccess(r *http.Request, user *auth.UserState) bool {
          			return true
          		}
-        -		if c.PolkitOK != "" {
+        +		if r.Method != "GET" && c.PolkitOK != "" {

-        -		POST:   postSnaps,
+        +		Path:     "/v2/snaps",
+        +		UserOK:   true,
+        +		PolkitOK: "io.snapcraft.snapd.manage-snaps",

-        -		return true
+        +	if c.PolkitOK != "" {
+        +		allow, err := strconv.ParseBool(r.Header.Get(client.AllowInteractionHeader))
+        +		if err != nil {

+        +		allowHeader := r.Header.Get(client.AllowInteractionHeader)
+        +		if allowHeader != "" {
+        +			if allow, err := strconv.ParseBool(allowHeader); err != nil {
+        +				logger.Noticef("error parsing %s header: %s", client.AllowInteractionHeader, err)

@@ -248,6 +264,11 @@ func (s *daemonSuite) TestPolkitInteractivity(c *check.C) {
          	put.Header.Set(client.AllowInteractionHeader, "true")
          	c.Check(cmd.canAccess(put, nil), check.Equals, true)
          	c.Check(s.lastPolkitFlags, check.Equals, polkit.CheckAllowInteraction)
+        +
+        +	// bad values are logged and treated as false

snapcore/snapd

daemon: allow polkit authorisation to install/remove snaps #3797

jhenstridge commented Aug 24, 2017 •

Edited 1 time

jhenstridge
Aug 24, 2017

pedronis requested a review from niemeyer Aug 24, 2017

jhenstridge commented Aug 25, 2017

codecov-io commented Aug 25, 2017 •

Edited 1 time

codecov-io
Sep 1, 2017

mvo5 reviewed Aug 28, 2017

View changes

zyga approved these changes Aug 30, 2017

View changes

jhenstridge added some commits Sep 1, 2017

jhenstridge commented Sep 1, 2017

niemeyer requested changes Sep 4, 2017

View changes

mvo5 reviewed Sep 5, 2017

View changes

jhenstridge added some commits Sep 5, 2017

mvo5 added this to the 2.28 milestone Sep 8, 2017

mvo5 approved these changes Sep 8, 2017

View changes

mvo5 commented Sep 8, 2017

mvo5 merged commit `4461114` into snapcore:master Sep 8, 2017
4 of 7 checks passed

4 of 7 checks passed

snapcore/snapd

Join GitHub today

daemon: allow polkit authorisation to install/remove snaps #3797

Conversation

jhenstridge commented Aug 24, 2017 • edited Edited 1 time jhenstridge Aug 24, 2017

All checks have passed

pedronis requested a review from niemeyer Aug 24, 2017

jhenstridge commented Aug 25, 2017

codecov-io commented Aug 25, 2017 • edited Edited 1 time codecov-io Sep 1, 2017

Codecov Report

mvo5 reviewed Aug 28, 2017 View changes

zyga approved these changes Aug 30, 2017 View changes

jhenstridge added some commits Sep 1, 2017

Some checks were not successful

All checks have passed

jhenstridge commented Sep 1, 2017

All checks have passed

niemeyer requested changes Sep 4, 2017 View changes

Some checks were not successful

mvo5 reviewed Sep 5, 2017 View changes

jhenstridge added some commits Sep 5, 2017

Some checks were not successful

Some checks were not successful

mvo5 added this to the 2.28 milestone Sep 8, 2017

mvo5 approved these changes Sep 8, 2017 View changes

mvo5 commented Sep 8, 2017

Hide details View details mvo5 merged commit 4461114 into snapcore:master Sep 8, 2017 4 of 7 checks passed

4 of 7 checks passed

jhenstridge commented Aug 24, 2017 •

Edited 1 time

jhenstridge
Aug 24, 2017

codecov-io commented Aug 25, 2017 •

Edited 1 time

codecov-io
Sep 1, 2017

mvo5 reviewed Aug 28, 2017

View changes

zyga approved these changes Aug 30, 2017

View changes

niemeyer requested changes Sep 4, 2017

View changes

mvo5 reviewed Sep 5, 2017

View changes

mvo5 approved these changes Sep 8, 2017

View changes

mvo5 merged commit `4461114` into snapcore:master Sep 8, 2017
4 of 7 checks passed