wrappers: allow sockets under $XDG_RUNTIME_DIR #6327

kubiko · 2019-01-04T15:58:32Z

/run/user/0/$SNAP_NAME/ is permited path for sockets to be created, this is at the moment blocked
when socket is defined as part of daemon configuration

Signed-off-by: Ondrej Kubik ondrej.kubik@canonical.com

Thanks for helping us make a better snapd!
Have you signed the license agreement and read the contribution guide?

zyga

This looks okay but I don't remember the plan for /run/user/NNN vs /run/user/NNN/$SNAP_INSTANCE_NAME.

CC @jdstrand and @chipaca for comments.

bboozzoo · 2019-01-07T07:54:48Z

snap/validate.go

@@ -272,7 +272,7 @@ func validateSocketAddrPath(socket *SocketInfo, fieldName string, path string) e
 		return fmt.Errorf("invalid %q: %q should be written as %q", fieldName, path, clean)
 	}

-	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/")) {
+	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/") || strings.HasPrefix(path, "/run/user/0/$SNAP_NAME/")) {
 		return fmt.Errorf(
 			"invalid %q: must have a prefix of $SNAP_DATA or $SNAP_COMMON", fieldName)


If we're going this way, the message needs to be updated to include /run/user/0/...

I agree with @bboozzoo on this point.

I did updated message, though with XDG_RUNTIME_DIR as @jdstrand proposed

bboozzoo · 2019-01-07T07:58:24Z

snap/validate.go

@@ -272,7 +272,7 @@ func validateSocketAddrPath(socket *SocketInfo, fieldName string, path string) e
 		return fmt.Errorf("invalid %q: %q should be written as %q", fieldName, path, clean)
 	}

-	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/")) {
+	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/") || strings.HasPrefix(path, "/run/user/0/$SNAP_NAME/")) {


I think this should use $SNAP_INSTANCE_NAME

I change it to XDG_RUNTIME_DIR, so there is no more need or SNAP_INSTANCE_NAME now

kubiko · 2019-01-07T11:27:06Z

Updated to use $SNAP_INSTANCE_NAME

jdstrand

Clearly there is a bug since we allow /run/user/[0-9]*/snap.@{SNAP_INSTANCE_NAME}/** but it feels wrong to make the user have to specify /run/user/*0*/... since in the future we might change that and this is exactly what XDG_RUNTIME_DIR is for. Eg:

$ sudo hello-world.env|grep XDG
XDG_RUNTIME_DIR=/run/user/0/snap.hello-world

Perhaps instead use || strings.HasPrefix(path, "$XDG_RUNTIME_DIR/")? I'm not sure if you'll have to expand that in the systemd unit or not, but this seems much more user friendly and in line with the other two variables.

jdstrand · 2019-01-10T19:32:22Z

snap/validate.go

@@ -272,7 +272,7 @@ func validateSocketAddrPath(socket *SocketInfo, fieldName string, path string) e
 		return fmt.Errorf("invalid %q: %q should be written as %q", fieldName, path, clean)
 	}

-	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/")) {
+	if !(strings.HasPrefix(path, "$SNAP_DATA/") || strings.HasPrefix(path, "$SNAP_COMMON/") || strings.HasPrefix(path, "/run/user/0/$SNAP_NAME/")) {
 		return fmt.Errorf(
 			"invalid %q: must have a prefix of $SNAP_DATA or $SNAP_COMMON", fieldName)


I agree with @bboozzoo on this point.

zyga

Please adjust tests to update the validation routine.

kubiko · 2019-05-22T10:22:31Z

@jdstrand @zyga @bboozzoo implementation is now updated to use XDG_RUNTIME_DIR instead of hardcoded /run/path as @jdstrand proposed, this seems way more reliable and also easier to use.
I have also updated tests around to test this scenario. Thanks for good feedback :)

jdstrand

Approving, but please consider adding the suggested comment (see inline).

jdstrand · 2019-05-23T17:11:24Z

wrappers/services.go

@@ -600,6 +601,7 @@ func generateSnapSocketFiles(app *snap.AppInfo) (*map[string][]byte, error) {
 func renderListenStream(socket *snap.SocketInfo) string {
 	snap := socket.App.Snap
 	listenStream := strings.Replace(socket.ListenStream, "$SNAP_DATA", snap.DataDir(), -1)
+	listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", snap.UserXdgRuntimeDir(sys.Geteuid()), -1)


I was slightly uncomfortable with this change since sys.Geteuid() is going to be whatever snapd is running as, which works for both the testsuite and typical "daemons run as root" case but would not work whenever we introduce User= and Group= in the generated systemd units. In that case, we would want to adjust sys.Geteuid() to be a lookup of the specified user/group, which is ok.

As a future reminder, can you add the following comment:

// TODO: when we support User/Group in the generated systemd unit, // adjust this accordingly

@jdstrand I was thinking exactly same thing. But I was not sure if we have anything ready for this usecase. As I assume socket options will then need to expand to SNAP_USER_COMMON and SNAP_USER_DATA. But then we need way to identify what is user service should run under. And actually forbid use of SNAP_DATA/SNAP_COMMON if not running as root.
And indeed I used sys.Geteuid() trick to get right value in test and in actual run time
I will add comment, so make sure we keep this updated when introducing serviced for users.
I will add comment to make sure we capture this

I think I would prefer not to use sys.Geteuid() here. Its slightly misleading, when looking at this code it seems like the value in xdg-runtime-dir depends on the daemon uid. But in reality it depends on the setting of the snap.yaml for the given service. By default the service will run as root and once we have a way to express users and groups it will run as a user or group. So I think it would be slightly better to represent that here (especially since initially this will always be uid=0). Something like:

--- a/wrappers/services.go +++ b/wrappers/services.go @@ -603,7 +603,9 @@ func renderListenStream(socket *snap.SocketInfo) string { listenStream := strings.Replace(socket.ListenStream, "$SNAP_DATA", snap.DataDir(), -1) // TODO: when we support User/Group in the generated systemd unit, // adjust this accordingly - listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", snap.UserXdgRuntimeDir(sys.Geteuid()), -1) + serviceUserUid := sys.UserID(0) + runtimeDir := snap.UserXdgRuntimeDir(serviceUserUid) + listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", runtimeDir, -1) return strings.Replace(listenStream, "$SNAP_COMMON", snap.CommonDataDir(), -1) } diff --git a/wrappers/services_test.go b/wrappers/services_test.go index 87de9e55c5..29700bc577 100644 --- a/wrappers/services_test.go +++ b/wrappers/services_test.go @@ -25,7 +25,6 @@ import ( "path/filepath" "regexp" "sort" - "strconv" "strings" "time" @@ -33,7 +32,6 @@ import ( "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/osutil" - "github.com/snapcore/snapd/osutil/sys" "github.com/snapcore/snapd/progress" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" @@ -460,7 +458,7 @@ Service=snap.hello-snap.svc1.service FileDescriptorName=sock3 ListenStream=%s -`, filepath.Join(s.tempdir, "/run/user", strconv.FormatUint(uint64(sys.Geteuid()), 10), "snap.hello-snap/sock3.socket")) +`, filepath.Join(s.tempdir, "/run/user/0/snap.hello-snap/sock3.socket")) c.Check(sock3File, testutil.FileContains, expected) }

kubiko · 2019-06-06T12:52:54Z

Please adjust tests to update the validation routine.

@zyga can you please review updated tests as it should be fixed now

mvo5

Thanks, this looks good! +0.9 with one small suggestion, otherwise good to go.

mvo5 · 2019-06-17T06:58:36Z

wrappers/services.go

@@ -600,6 +601,7 @@ func generateSnapSocketFiles(app *snap.AppInfo) (*map[string][]byte, error) {
 func renderListenStream(socket *snap.SocketInfo) string {
 	snap := socket.App.Snap
 	listenStream := strings.Replace(socket.ListenStream, "$SNAP_DATA", snap.DataDir(), -1)
+	listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", snap.UserXdgRuntimeDir(sys.Geteuid()), -1)


I think I would prefer not to use sys.Geteuid() here. Its slightly misleading, when looking at this code it seems like the value in xdg-runtime-dir depends on the daemon uid. But in reality it depends on the setting of the snap.yaml for the given service. By default the service will run as root and once we have a way to express users and groups it will run as a user or group. So I think it would be slightly better to represent that here (especially since initially this will always be uid=0). Something like:

--- a/wrappers/services.go +++ b/wrappers/services.go @@ -603,7 +603,9 @@ func renderListenStream(socket *snap.SocketInfo) string { listenStream := strings.Replace(socket.ListenStream, "$SNAP_DATA", snap.DataDir(), -1) // TODO: when we support User/Group in the generated systemd unit, // adjust this accordingly - listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", snap.UserXdgRuntimeDir(sys.Geteuid()), -1) + serviceUserUid := sys.UserID(0) + runtimeDir := snap.UserXdgRuntimeDir(serviceUserUid) + listenStream = strings.Replace(listenStream, "$XDG_RUNTIME_DIR", runtimeDir, -1) return strings.Replace(listenStream, "$SNAP_COMMON", snap.CommonDataDir(), -1) } diff --git a/wrappers/services_test.go b/wrappers/services_test.go index 87de9e55c5..29700bc577 100644 --- a/wrappers/services_test.go +++ b/wrappers/services_test.go @@ -25,7 +25,6 @@ import ( "path/filepath" "regexp" "sort" - "strconv" "strings" "time" @@ -33,7 +32,6 @@ import ( "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/osutil" - "github.com/snapcore/snapd/osutil/sys" "github.com/snapcore/snapd/progress" "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/snaptest" @@ -460,7 +458,7 @@ Service=snap.hello-snap.svc1.service FileDescriptorName=sock3 ListenStream=%s -`, filepath.Join(s.tempdir, "/run/user", strconv.FormatUint(uint64(sys.Geteuid()), 10), "snap.hello-snap/sock3.socket")) +`, filepath.Join(s.tempdir, "/run/user/0/snap.hello-snap/sock3.socket")) c.Check(sock3File, testutil.FileContains, expected) }

mvo5 · 2019-06-17T07:00:47Z

wrappers/services_test.go

+FileDescriptorName=sock3
+ListenStream=%s
+
+`, filepath.Join(s.tempdir, "/run/user", strconv.FormatUint(uint64(sys.Geteuid()), 10), "snap.hello-snap/sock3.socket"))


This is similar to above (and the diff will change this part too): when reading this its slightly misleading as it may make the reader think the value is dependent on the runtime uid. But its strictly dependent on the user configured in the service yaml. So I would prefer here to not use sys.Geteuid() as well.

@mvo5 I updated code per proposal. Initially I was trying to avoid hardcoding uid but you are right that it actually made it worse and code was giving wrong impression.
And true, this will anyway need to be revisited when we land support for user services

XDG_RUNTIME_DIR (usually /run/user/<uid>/snap.$SNAP_INSTANCE_NAME/) is permited path for sockets to be created, this is at the moment blocked when socket is defined as part of daemon configuration Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>

…ation Expand XDG_RUNTIME_DIR if used for socket activated service Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>

…ME_DIR socket path Update socket-activation test snap with socket in XDG_RUNTIME_DIR and SNAP_DATA path Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>

zyga

I re-read the patch and the discussion, looks good to me.

pedronis · 2019-06-20T08:35:57Z

seems there are some actual SE Linux failures now, @bboozzoo maybe can help

kubiko · 2019-06-20T08:55:12Z

seems there are some actual SE Linux failures now, @bboozzoo maybe can help

@pedronis yep @bboozzoo is already looking into this, he confirmed there is need to update SE profile

…-fix

bboozzoo · 2019-06-24T05:42:53Z

----
type=AVC msg=audit(06/20/19 12:48:16.638:1057) : avc:  denied  { write } for  pid=12685 comm=snapd name=/ dev="tmpfs" ino=21744 scontext=system_u:system_r:snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 
----
type=AVC msg=audit(06/20/19 12:48:16.639:1058) : avc:  denied  { remove_name } for  pid=12685 comm=snapd name=snap.socket-activation dev="tmpfs" ino=146684 scontext=system_u:system_r:snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 
----
type=AVC msg=audit(06/20/19 12:48:16.639:1059) : avc:  denied  { rmdir } for  pid=12685 comm=snapd name=snap.socket-activation dev="tmpfs" ino=146684 scontext=system_u:system_r:snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 
----
type=AVC msg=audit(06/20/19 12:48:16.639:1060) : avc:  denied  { unlink } for  pid=12685 comm=snapd name=socket-xdg dev="tmpfs" ino=146685 scontext=system_u:system_r:snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=sock_file permissive=1

Yes, this is caused by snapd trying to remove a socket file under /run/. I'll look into adjusting the policy and will push a patch to this branch.

Adjust SELinux policy to allow snapd to remove socket files under /run/user (aka $XDG_RUNTIME_DIR). Fixes the following denials: ---- type=AVC msg=audit(06/24/19 07:52:12.092:324) : avc: denied { write } for pid=25331 comm=snapd name=/ dev="tmpfs" ino=21794 scontext=system_u:system_r:snappy_t:s0 tcontext =system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 ---- type=AVC msg=audit(06/24/19 07:52:12.092:325) : avc: denied { remove_name } for pid=25331 comm=snapd name=snap.socket-activation dev="tmpfs" ino=69344 scontext=system_u:sy stem_r:snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 ---- type=AVC msg=audit(06/24/19 07:52:12.092:326) : avc: denied { rmdir } for pid=25331 comm=snapd name=snap.socket-activation dev="tmpfs" ino=69344 scontext=system_u:system_r :snappy_t:s0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=dir permissive=1 ---- type=AVC msg=audit(06/24/19 07:52:12.093:327) : avc: denied { unlink } for pid=25331 comm=snapd name=socket-xdg dev="tmpfs" ino=69345 scontext=system_u:system_r:snappy_t:s 0 tcontext=system_u:object_r:user_tmp_t:s0 tclass=sock_file permissive=1 Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2019-06-24T08:36:09Z

Pushed a patch that extends the policy accordingly.

zyga reviewed Jan 7, 2019

View reviewed changes

bboozzoo reviewed Jan 7, 2019

View reviewed changes

kubiko force-pushed the socket-validation-fix branch from a55b558 to 96b8e1f Compare January 7, 2019 11:26

jdstrand suggested changes Jan 10, 2019

View reviewed changes

zyga requested changes Mar 1, 2019

View reviewed changes

kubiko force-pushed the socket-validation-fix branch 4 times, most recently from 3a7e59b to 7b2f8d6 Compare May 22, 2019 10:18

kubiko changed the title ~~Allowing sockets under /run/user/0/$SNAP_NAME~~ Allowing sockets under $XDG_RUNTIME_DIR May 22, 2019

kubiko force-pushed the socket-validation-fix branch from 7b2f8d6 to effeb09 Compare May 22, 2019 11:26

jdstrand approved these changes May 23, 2019

View reviewed changes

kubiko force-pushed the socket-validation-fix branch 2 times, most recently from 543eb6b to 87e4807 Compare May 23, 2019 22:28

mvo5 reviewed Jun 17, 2019

View reviewed changes

kubiko added 3 commits June 17, 2019 17:18

wrappers: services: add XDG_RUNTIME_DIR support to ListenStream gener…

142f7f5

…ation Expand XDG_RUNTIME_DIR if used for socket activated service Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>

tests: lib: snaps: socket-activation: update test snap with XDG_RUNTI…

3e0aa03

…ME_DIR socket path Update socket-activation test snap with socket in XDG_RUNTIME_DIR and SNAP_DATA path Signed-off-by: Ondrej Kubik <ondrej.kubik@canonical.com>

kubiko force-pushed the socket-validation-fix branch from 87e4807 to 3e0aa03 Compare June 17, 2019 16:19

kubiko changed the title ~~Allowing sockets under $XDG_RUNTIME_DIR~~ wrappers: allow sockets under $XDG_RUNTIME_DIR Jun 17, 2019

mvo5 approved these changes Jun 18, 2019

View reviewed changes

zyga reviewed Jun 18, 2019

View reviewed changes

zyga approved these changes Jun 18, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into socket-validation…

f742c71

…-fix

bboozzoo merged commit e8aef1c into snapcore:master Jun 24, 2019

kubiko deleted the socket-validation-fix branch July 6, 2019 22:53

jhenstridge mentioned this pull request Apr 22, 2020

wrappers: allow user mode systemd daemons #5822

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wrappers: allow sockets under $XDG_RUNTIME_DIR #6327

wrappers: allow sockets under $XDG_RUNTIME_DIR #6327

kubiko commented Jan 4, 2019

zyga left a comment

bboozzoo Jan 7, 2019

jdstrand Jan 10, 2019

kubiko May 22, 2019

bboozzoo Jan 7, 2019

kubiko May 22, 2019

kubiko commented Jan 7, 2019

jdstrand left a comment

jdstrand Jan 10, 2019

zyga left a comment

kubiko commented May 22, 2019

jdstrand left a comment

jdstrand May 23, 2019

kubiko May 23, 2019 •

edited

mvo5 Jun 17, 2019

kubiko commented Jun 6, 2019

mvo5 left a comment

mvo5 Jun 17, 2019

mvo5 Jun 17, 2019

kubiko Jun 17, 2019

zyga left a comment

pedronis commented Jun 20, 2019

kubiko commented Jun 20, 2019

bboozzoo commented Jun 24, 2019

bboozzoo commented Jun 24, 2019

wrappers: allow sockets under $XDG_RUNTIME_DIR #6327

wrappers: allow sockets under $XDG_RUNTIME_DIR #6327

Conversation

kubiko commented Jan 4, 2019

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiko commented Jan 7, 2019

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

kubiko commented May 22, 2019

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiko May 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiko commented Jun 6, 2019

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

pedronis commented Jun 20, 2019

kubiko commented Jun 20, 2019

bboozzoo commented Jun 24, 2019

bboozzoo commented Jun 24, 2019

kubiko May 23, 2019 •

edited