Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usersession: add systemd user instance service control to user session agent #7238

Open
wants to merge 8 commits into
base: master
from

Conversation

@jhenstridge
Copy link
Contributor

commented Aug 13, 2019

This PR adds an API to the user session agent to control user instance systemd services. It only aims to provide the features needed to manage user daemons, so covers daemon-reload, start, and stop (enabling and disabling services is done through the "global" /etc/systemd/user directory, so doesn't need access to the user instance).

There is a single /v1/services end point that accepts POST requests with a JSON body. The action member in the body specifies which action should be performed, and the services member provides the list of services for start/stop. As a restriction, the service names must start with snap., to prevent control of non-snap services.

At present, this doesn't have an option to force kill services for e.g. daemons using weird kill modes. That could probably be added later as an additional argument in the post body.

@jhenstridge jhenstridge force-pushed the jhenstridge:session-agent-systemd-control branch from ba6e076 to a5b4a49 Aug 15, 2019

@jhenstridge jhenstridge marked this pull request as ready for review Aug 16, 2019

if err != nil && systemd.IsTimeout(err) {
// ignore errors for kill; nothing we'd do differently at this point
sysd.Kill(service, "TERM", "")
time.Sleep(killWait)

This comment has been minimized.

Copy link
@zyga

zyga Aug 19, 2019

Contributor

Should we wait unconditionally? If the wait time is a bit larger then this will just wait even though the service may have reacted faster. I also wonder if we really need this. Systemd models behaviour for stopping services, along with any timeouts and cleanup necessary.

What we might do instead is kill stopping fails.

This comment has been minimized.

Copy link
@jhenstridge

jhenstridge Aug 20, 2019

Author Contributor

I adapted this code from stopService in wrappers/services.go. Looking at the documentation in systemd.kill(5), it isn't obvious to me that this is needed either.

usersession/agent/rest_api.go Outdated Show resolved Hide resolved

servicesCmd = &Command{
Path: "/v1/services",
POST: postServices,

This comment has been minimized.

Copy link
@zyga

zyga Aug 19, 2019

Contributor

Do we need any kind of authentication here? Do we need to enforce that the user making the request is the same as the user running the user session daemon?

This comment has been minimized.

Copy link
@jhenstridge

jhenstridge Aug 19, 2019

Author Contributor

The intended audience for this API is root. Other users should not be able to access the the session agent due to the permissions on $XDG_RUNTIME_DIR.

On top of this, PR #7197 adds a second layer of protection by performing an SO_PEERCRED check on new connections. If we're only performing actions on behalf of the same user or root, we don't need the complication of snapd's per-user authentication checks.

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

I agree with @jhenstridge but will add that snaps may run as root or the session user, but this socket isn't allowed in the policy. IME, we may want to add this to add another line of defense for snaps running as root/the session user so they can't use the API (though perhaps add only at a future date). See my comments wrt additional tests.

if err := decoder.Decode(&inst); err != nil {
return BadRequest("cannot decode request body into service instruction: %v", err)
}
impl := serviceInstructionDispTable[inst.Action]

This comment has been minimized.

Copy link
@zyga

zyga Aug 19, 2019

Contributor

Do we need to grab a lock for this? If several instructions are posted concurrently I think we should lock around either globally (per user) or per snap (and refuse to issue commands that would affect multiple snaps in one go).

This comment has been minimized.

Copy link
@jhenstridge

jhenstridge Aug 20, 2019

Author Contributor

That seems reasonable. I've added a global lock to serialise the final final portion of postServices.

@zyga
Copy link
Contributor

left a comment

I did a quick pass around the more essential parts. I support this change entirely and will do deeper pass after initial responses.

@pedronis pedronis requested review from pedronis and chipaca Aug 20, 2019

@pedronis pedronis changed the title usersession: Add systemd user instance service control to user session agent usersession: add systemd user instance service control to user session agent Aug 29, 2019

@jdstrand
Copy link
Contributor

left a comment

Since this has received reviews from others, I focused on isolation and in in general, this looks good, but would like to see a couple of additional test cases (thus, request changes). Beyond that, I think there is an opportunity for a future hardening improvement. See inline for a few other small things.


environment:
TEST_UID: $(id -u test)
USER_RUNTIME_DIR: /run/user/${TEST_UID}

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

Most tests simply hardcode the test user id as 12345. So perhaps just:

USER_RUNTIME_DIR: /run/user/12345
if snap list test-snapd-curl; then
snap remove test-snapd-curl
fi
systemctl stop "user@${TEST_UID}.service"

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

Since this is the restore step, should this and the below systemctl command have an || true to make sure that other cleanup items are still run? (cc @zyga)

-d '{"action": "stop", "services": ["snap.test-service.service"]}' \
http://localhost/v1/services | MATCH "HTTP/1.1 200 OK"
systemctl_user show --property=ActiveState snap.test-service.service |
MATCH ActiveState=inactive

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

"${USER_RUNTIME_DIR}/snapd-session-agent.socket" is not allowed to snaps in the default template or any interfaces. Since this socket is sensitive, can you add a test that the socket is blocked from snaps (when the system is in strict confinement mode)? Probably can install test-snapd-curl unconditionally and use it for this. Please test as both root and the test user since the PEERCRED check in PR 7197 only checks for a uid that isn't root or the session user.

A future improvement in a followup PR could be to see if the PEERCRED's pid is in a freezer cgroup for any snaps and deny it (this should be fine with pid reuse races since the connecting snap can't fake being in another cgroup).


servicesCmd = &Command{
Path: "/v1/services",
POST: postServices,

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

I agree with @jhenstridge but will add that snaps may run as root or the session user, but this socket isn't allowed in the policy. IME, we may want to add this to add another line of defense for snaps running as root/the session user so they can't use the API (though perhaps add only at a future date). See my comments wrt additional tests.

}

func serviceStop(inst *serviceInstruction, sysd systemd.Systemd) Response {
// Refuse to start non-snap services

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

s/start/stop/

killWait = 5 * time.Second
)

func stopOneService(service string, sysd systemd.Systemd) error {

This comment has been minimized.

Copy link
@jdstrand

jdstrand Sep 11, 2019

Contributor

This is only used internally and not exposed via serviceInstructionDispTable, but a reasonable future-proofing hardening measure seems to be to perform this here as well:

	if !strings.HasPrefix(service, "snap.") {
		return InternalError("cannot stop non-snap service %v", service)
	}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.