[2/2] Support for adjusting socket access rights to allow group of users to connect to the socket.

Submitted by Imran Zaman on Oct. 16, 2014, 4:23 p.m.

Details

Message ID 1413476620-32213-2-git-send-email-imran.zaman@gmail.com
State Rejected
Headers show

Not browsing as part of any series.

Commit Message

Imran Zaman Oct. 16, 2014, 4:23 p.m.
This is used for nested compositor architectures.

Signed-off-by: Imran Zaman <imran.zaman@gmail.com>
---
 src/wayland-server.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/wayland-server.c b/src/wayland-server.c
index 09e8903..721fabe 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -39,6 +39,8 @@ 
 #include <fcntl.h>
 #include <sys/file.h>
 #include <sys/stat.h>
+#include <sys/types.h>
+#include <grp.h>
 #include <ffi.h>
 
 #include "wayland-private.h"
@@ -1117,6 +1119,10 @@  static int
 _wl_display_add_socket(struct wl_display *display, struct wl_socket *s)
 {
 	socklen_t size;
+	const char *socket_mode_str;
+	const char *socket_group_str;
+	const struct group *socket_group;
+	unsigned socket_mode;
 
 	s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
 	if (s->fd < 0) {
@@ -1134,6 +1140,27 @@  _wl_display_add_socket(struct wl_display *display, struct wl_socket *s)
 		return -1;
 	}
 
+	socket_group_str = getenv("WAYLAND_SERVER_GROUP");
+	if (socket_group_str != NULL) {
+		socket_group = getgrnam(socket_group_str);
+		if (socket_group != NULL) {
+			if (chown(s->addr.sun_path,
+				-1, socket_group->gr_gid) != 0)
+				wl_log("chown(\"%s\") failed: %s",
+					s->addr.sun_path,
+					strerror(errno));
+		}
+	}
+	socket_mode_str = getenv("WAYLAND_SERVER_MODE");
+	if (socket_mode_str != NULL) {
+		if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
+			if (chmod(s->addr.sun_path, socket_mode) != 0) {
+				wl_log("chmod(\"%s\") failed: %s",
+					s->addr.sun_path,
+					strerror(errno));
+			}
+	}
+
 	s->source = wl_event_loop_add_fd(display->loop, s->fd,
 					 WL_EVENT_READABLE,
 					 socket_data, display);

Comments

On Thu, Oct 16, 2014 at 9:23 AM, Imran Zaman <imran.zaman@gmail.com> wrote:

> This is used for nested compositor architectures.
>

Could you please provide a little more explanation than that.  What kind of
nesting are you doing?

Also, why are you doing this through environment variables and not
something explicit?  For instance, the compositor can easily grab the
socket and chmod it.  It has the privileges and knows what socket it is.

--Jason Ekstrand


>
> Signed-off-by: Imran Zaman <imran.zaman@gmail.com>
> ---
>  src/wayland-server.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 09e8903..721fabe 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -39,6 +39,8 @@
>  #include <fcntl.h>
>  #include <sys/file.h>
>  #include <sys/stat.h>
> +#include <sys/types.h>
> +#include <grp.h>
>  #include <ffi.h>
>
>  #include "wayland-private.h"
> @@ -1117,6 +1119,10 @@ static int
>  _wl_display_add_socket(struct wl_display *display, struct wl_socket *s)
>  {
>         socklen_t size;
> +       const char *socket_mode_str;
> +       const char *socket_group_str;
> +       const struct group *socket_group;
> +       unsigned socket_mode;
>
>         s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
>         if (s->fd < 0) {
> @@ -1134,6 +1140,27 @@ _wl_display_add_socket(struct wl_display *display,
> struct wl_socket *s)
>                 return -1;
>         }
>
> +       socket_group_str = getenv("WAYLAND_SERVER_GROUP");
> +       if (socket_group_str != NULL) {
> +               socket_group = getgrnam(socket_group_str);
> +               if (socket_group != NULL) {
> +                       if (chown(s->addr.sun_path,
> +                               -1, socket_group->gr_gid) != 0)
> +                               wl_log("chown(\"%s\") failed: %s",
> +                                       s->addr.sun_path,
> +                                       strerror(errno));
> +               }
> +       }
> +       socket_mode_str = getenv("WAYLAND_SERVER_MODE");
> +       if (socket_mode_str != NULL) {
> +               if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
> +                       if (chmod(s->addr.sun_path, socket_mode) != 0) {
> +                               wl_log("chmod(\"%s\") failed: %s",
> +                                       s->addr.sun_path,
> +                                       strerror(errno));
> +                       }
> +       }
> +
>         s->source = wl_event_loop_add_fd(display->loop, s->fd,
>                                          WL_EVENT_READABLE,
>                                          socket_data, display);
> --
> 1.9.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On 17.10.2014 20:00, Jason Ekstrand wrote:
> On Thu, Oct 16, 2014 at 9:23 AM, Imran Zaman <imran.zaman@gmail.com
> <mailto:imran.zaman@gmail.com>> wrote:
>
>     This is used for nested compositor architectures.
>
>
> Could you please provide a little more explanation than that.  What kind
> of nesting are you doing?

We have one system compositor using DRM backend and then nested 
compositors for each user using wayland backend.

This is in order to share single GPU with multiple display outputs among 
multiple users.

> Also, why are you doing this through environment variables and not
> something explicit?  For instance, the compositor can easily grab the
> socket and chmod it.  It has the privileges and knows what socket it is.

This is related to the other patch that allows modifying location of the 
server/client socket location. I thought that the access control is best 
being close to the place where socket is created. Otherwise it is hard 
to follow what is going on if the related code is scattered across modules.
Hi,

On 20 October 2014 15:19, Jussi Laako <jussi.laako@linux.intel.com> wrote:

> On 17.10.2014 20:00, Jason Ekstrand wrote:
>
>> Could you please provide a little more explanation than that.  What kind
>> of nesting are you doing?
>>
>
> We have one system compositor using DRM backend and then nested
> compositors for each user using wayland backend.
>
> This is in order to share single GPU with multiple display outputs among
> multiple users.


Makes sense, although you can already enforce isolation with a single
shared compositor ...


>  Also, why are you doing this through environment variables and not
>> something explicit?  For instance, the compositor can easily grab the
>> socket and chmod it.  It has the privileges and knows what socket it is.
>>
>
> This is related to the other patch that allows modifying location of the
> server/client socket location. I thought that the access control is best
> being close to the place where socket is created. Otherwise it is hard to
> follow what is going on if the related code is scattered across modules.


Doing it through environment variables is just plain nasty though; I really
don't like this patch. I'd much rather see an explicit call, or users
creating the appropriate fd and then just passing it to the lib.

Cheers,
Daniel
On 20.10.2014 17:26, Daniel Stone wrote:
> Makes sense, although you can already enforce isolation with a single
> shared compositor ...

Only weston can access the DRM compositor socket since it's the only one 
having group privileges for doing it. So each user has their own 
contained environment. DRM compositor is using fullscreen-shell and 
nested ones are using what ever other shell they wish.

We looked into input methods splitting with a shared compositor but it 
was getting a bit too complex. So now each nested compositor will manage 
their own set of input devices.

So we have a functional setup with two displays, two mice, two keyboards 
and a touch screen. With the current approach. Two users have two mouse 
pointers and can type on the keyboard same time and window focus 
management and such works without modifications.

> Doing it through environment variables is just plain nasty though; I
> really don't like this patch. I'd much rather see an explicit call, or
> users creating the appropriate fd and then just passing it to the lib.

OK, then I think it would be better to move the socket creation out from 
libwayland and just pass the socket descriptor to libwayland.
Hi,

On 20 October 2014 16:06, Jussi Laako <jussi.laako@linux.intel.com> wrote:

> On 20.10.2014 17:26, Daniel Stone wrote:
>
>> Makes sense, although you can already enforce isolation with a single
>> shared compositor ...
>>
>
> Only weston can access the DRM compositor socket since it's the only one
> having group privileges for doing it. So each user has their own contained
> environment. DRM compositor is using fullscreen-shell and nested ones are
> using what ever other shell they wish.
>

weston-launch should be taking care of the privileges for you anyway ...


> We looked into input methods splitting with a shared compositor but it was
> getting a bit too complex. So now each nested compositor will manage their
> own set of input devices.
>
> So we have a functional setup with two displays, two mice, two keyboards
> and a touch screen. With the current approach. Two users have two mouse
> pointers and can type on the keyboard same time and window focus management
> and such works without modifications.


Each to their own; I don't think it's necessarily any more complex than
split compositors, since at some point you have to deal with input
splitting anyway, and if you want both security (i.e. random users not
opening random devices) and performance (i.e. no unnecessary hops through
input compositor for events), you end up having to deal with input
splitting in your system compositor anyway.

Cheers,
Daniel
On 20.10.2014 18:13, Daniel Stone wrote:
> Each to their own; I don't think it's necessarily any more complex than
> split compositors, since at some point you have to deal with input
> splitting anyway, and if you want both security (i.e. random users not
> opening random devices) and performance (i.e. no unnecessary hops
> through input compositor for events), you end up having to deal with
> input splitting in your system compositor anyway.

Access control is no different whether you have 1 weston or 3 westons 
running.

It is about amount of changes needed, fairly small patch vs almost 
complete weston rewrite...
(Fixing CC lists, please use reply-to-all in the future...)

On Wed, 22 Oct 2014 10:55:25 +0300
Jussi Laako <jussi.laako@linux.intel.com> wrote:

> On 20.10.2014 18:13, Daniel Stone wrote:
> > Each to their own; I don't think it's necessarily any more complex than
> > split compositors, since at some point you have to deal with input
> > splitting anyway, and if you want both security (i.e. random users not
> > opening random devices) and performance (i.e. no unnecessary hops
> > through input compositor for events), you end up having to deal with
> > input splitting in your system compositor anyway.
> 
> Access control is no different whether you have 1 weston or 3 westons 
> running.
> 
> It is about amount of changes needed, fairly small patch vs almost 
> complete weston rewrite...

Yeah, we've had long discussions about this with Imran.

The problem here is doing multi-seat with a single GPU (multi-head), so
for output they need a central compositor.

Running one session compositor per seat, on top of a system compositor,
has some significant advantages:
- each session compositor can be from each user's favourite DE
- there is no need to add restrictions into the shared compositor, like
  preventing pointers or windows from traveling to other users'
  outputs, if you do evdev pass-through
- advertising globals in the protocol can be per-seat, rather than
  every app seeing every seat's output globals

I'm sure there are more, but yes, adding all that isolation into Weston
to share the same compositor instance between all physical seats would
be quite an ordeal. You can't really create, say, multiple
weston_compositor instances, because that means multiple
drm_compositors, which means the GPU sharing part would be hard. And if
you share weston_compositor, then adding the isolation is hard.


Like said, environment variables are not really good here. You can
already, without modifications to libwayland-server, create your custom
sockets like I explained in the earlier thread: just create the socket,
watch the fd in your event loop, accept connections and call
wl_client_create(). You duplicate a bit of code from libwayland-server,
but you have total control on where and how the socket is created, and
you know explicitly from which socket a client comes from.

This would be used by the system compositor to offer sockets for the
session compositors to connect to, right?

When a session compositor is started, you can already use
WAYLAND_SOCKET environment variable to pass an opened connection to it.
If your system compositor forks session compositors, no problem. If
something else starts your session compositors, you need a wrapper
program to pre-create a connection to the socket file in the shared
directory (that is not your XDG_RUNTIME_DIR) and then exec the session
compositor.

Would that solve all the problems you were trying to solve with these
two patches?


Thanks,
pq
On 19.11.2014 16:22, Pekka Paalanen wrote:
> When a session compositor is started, you can already use
> WAYLAND_SOCKET environment variable to pass an opened connection to it.
> If your system compositor forks session compositors, no problem. If
> something else starts your session compositors, you need a wrapper
> program to pre-create a connection to the socket file in the shared
> directory (that is not your XDG_RUNTIME_DIR) and then exec the session
> compositor.

We are running system compositor under one special user session, in this 
case user "genivi". This is normal session other than it is not visible 
to logind (pam_systemd is not called).

Then, in our login manager (TLM), other seats are set to wait for the 
system compositor lock file to appear before logging in default (guest) 
user sessions. These are normal sessions visible to logind and weston 
for each seat runs inside the user session. When user is logged out, the 
weston instance is terminated and restarted for the next user session.

Passing WAYLAND_SOCKET environment from a user session of user X to 
another newly created user sessions Y and Z is not very straightforward 
operation... Also lifetime of the Y and Z vary and new sessions will be 
spawned after one is terminated while the system compositor persists.
On Tue, 25 Nov 2014 09:22:47 +0200
Jussi Laako <jussi.laako@linux.intel.com> wrote:

> On 19.11.2014 16:22, Pekka Paalanen wrote:
> > When a session compositor is started, you can already use
> > WAYLAND_SOCKET environment variable to pass an opened connection to it.
> > If your system compositor forks session compositors, no problem. If
> > something else starts your session compositors, you need a wrapper
> > program to pre-create a connection to the socket file in the shared
> > directory (that is not your XDG_RUNTIME_DIR) and then exec the session
> > compositor.
> 
> We are running system compositor under one special user session, in this 
> case user "genivi". This is normal session other than it is not visible 
> to logind (pam_systemd is not called).
> 
> Then, in our login manager (TLM), other seats are set to wait for the 
> system compositor lock file to appear before logging in default (guest) 
> user sessions. These are normal sessions visible to logind and weston 
> for each seat runs inside the user session. When user is logged out, the 
> weston instance is terminated and restarted for the next user session.
> 
> Passing WAYLAND_SOCKET environment from a user session of user X to 
> another newly created user sessions Y and Z is not very straightforward 
> operation... Also lifetime of the Y and Z vary and new sessions will be 
> spawned after one is terminated while the system compositor persists.

But you don't need that if you write a tiny wrapper program to be
executed instead of weston for the user sessions: you can even hardcode
there the special socket path it needs to open and then exec weston
with WAYLAND_SOCKET set.

Sounds like all the server side issues you tried to solve by
environment variables are already solvable in your system compositor,
right?

Getting session compositor to connect to the system compositor is the
last open issue in this series, isn't it?

Now the remaining question is, should libwayland-client use an
environment variable, that would allow overriding the directory for
where to look for socket files?

It's not strictly necessary, because one can always do that wrapper
program trick. However, that is also an argument that says that there is
no harm adding such an environment variable, because one can already
achieve the same thing.

There is one case where the wrapper program trick does not work: if the
session compositor or any of its forked children needs to open a new
connection to the system compositor, it doesn't work. I'm not
completely sure if that is a bad thing or a good thing.

Opinions, people?

I think I am slightly leaning towards adding such an environment
variable for libwayland-client.


Thanks,
pq