[Spice-devel] [vdagent-linux v3 3/4] session-info: check if session belongs to user
Victor Toso
lists at victortoso.com
Sat Apr 30 06:44:33 UTC 2016
Hi,
On Fri, Apr 29, 2016 at 01:46:52PM -0500, Jonathon Jongsma wrote:
> On Thu, 2016-04-28 at 15:37 +0200, Victor Toso wrote:
> > session-info back-ends such as console-kit and systemd-login have the
> > concept of session's class which informs if session belongs to user or
> > not [0]. We can disable features based on the session class.
> >
> > [0] Display-Managers are 'Greeter' and Display lock screens are
> > 'lock-screen'
> >
> > This patch introduces session_info_is_user() and disable file-xfer in
> > case agent's session does not belong to the 'user' class. As the
> > session-info data is hold by vdagentd, this patch also introduces
> > VDAGENTD_FILE_XFER_DISABLE message to disable file-xfer that is done
> > in vdagent.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1328761
> > ---
> > src/console-kit.c | 6 ++++++
> > src/dummy-session-info.c | 5 +++++
> > src/session-info.h | 1 +
> > src/systemd-login.c | 26 ++++++++++++++++++++++++++
> > src/vdagent.c | 9 +++++++++
> > src/vdagentd-proto-strings.h | 1 +
> > src/vdagentd-proto.h | 1 +
> > src/vdagentd.c | 9 +++++++++
> > 8 files changed, 58 insertions(+)
> >
> > diff --git a/src/console-kit.c b/src/console-kit.c
> > index b21df6f..036234e 100644
> > --- a/src/console-kit.c
> > +++ b/src/console-kit.c
> > @@ -502,3 +502,9 @@ gboolean session_info_session_is_locked(struct
> > session_info *info)
> > }
> > return (info->session_is_locked || info->session_idle_hint);
> > }
> > +
> > +gboolean session_info_is_user(struct session_info *info)
> > +{
> > + /* TODO */
> > + return TRUE;
> > +}
> > diff --git a/src/dummy-session-info.c b/src/dummy-session-info.c
> > index e7a716f..8ffe5e4 100644
> > --- a/src/dummy-session-info.c
> > +++ b/src/dummy-session-info.c
> > @@ -49,3 +49,8 @@ gboolean session_is_locked(struct session_info *ck)
> > {
> > return FALSE;
> > }
> > +
> > +gboolean session_info_is_user(struct session_info *si)
> > +{
> > + return TRUE;
> > +}
> > diff --git a/src/session-info.h b/src/session-info.h
> > index f395dbf..c8edb86 100644
> > --- a/src/session-info.h
> > +++ b/src/session-info.h
> > @@ -38,5 +38,6 @@ const char *session_info_get_active_session(struct
> > session_info *ck);
> > char *session_info_session_for_pid(struct session_info *ck, uint32_t pid);
> >
> > gboolean session_info_session_is_locked(struct session_info *si);
> > +gboolean session_info_is_user(struct session_info *si);
> >
> > #endif
> > diff --git a/src/systemd-login.c b/src/systemd-login.c
> > index 470a9ff..342a9b1 100644
> > --- a/src/systemd-login.c
> > +++ b/src/systemd-login.c
> > @@ -232,3 +232,29 @@ gboolean session_info_session_is_locked(struct
> > session_info *si)
> > si_dbus_read_signals(si);
> > return si->session_is_locked;
> > }
> > +
> > +/* This function should only be called after session_info_get_active_session
> > + * in order to verify if active session belongs to user (non greeter) */
> > +gboolean session_info_is_user(struct session_info *si)
> > +{
> > + gchar *class = NULL;
> > + gboolean ret;
> > +
> > + if (si == NULL || si->session == NULL)
> > + return TRUE;
>
> Like a comment on a previous patch, I wonder if it would be better to default to
> FALSE to fall back to a 'safe' state. comments?
I put my comment in the locked-screen default behavior. This one has slightly
different rationale.
The reason for me to return that all sessions's owners is 'user' by default is
that only 'gdm' is the non 'user' that should have spice-vdagent running. So, in
case of unforeseen problems (most likely with dbus), the gdm would be consider
as 'user' and we would have a few features enabled to him that should be disable
which seems an okayish problem;
If we return 'non-user' by default, in case of problems, we would disable
features for the whole session for the real user.
>
> > +
> > + if (sd_session_get_class(si->session, &class) != 0) {
> > + syslog(LOG_WARNING, "Unable to get class from session: %s",
> > + si->session);
> > + return TRUE;
> > + }
>
> same here.
>
> > +
> > + if (si->verbose)
> > + syslog(LOG_DEBUG, "(systemd-login) class for %s is %s",
> > + si->session, class);
> > +
> > + ret = (g_strcmp0(class, "user") == 0);
> > + g_free(class);
> > +
> > + return ret;
> > +}
> > diff --git a/src/vdagent.c b/src/vdagent.c
> > index 9c619ef..a3cdd9b 100644
> > --- a/src/vdagent.c
> > +++ b/src/vdagent.c
> > @@ -110,6 +110,15 @@ static void daemon_read_complete(struct udscs_connection
> > **connp,
> > }
> > free(data);
> > break;
> > + case VDAGENTD_FILE_XFER_DISABLE:
> > + if (debug)
> > + syslog(LOG_DEBUG, "Disabling file-xfers");
> > +
> > + if (vdagent_file_xfers != NULL) {
> > + vdagent_file_xfers_destroy(vdagent_file_xfers);
> > + vdagent_file_xfers = NULL;
>
> So, as far as I can tell, if there are any file-transfers currently ongoing,
> this will close the file descriptor for each transfer task, delete the
> corresponding file, but will not send an XFER_STATUS_ERROR message back to the
> client. Although, presumably the next time we receive a XFER_DATA message, we
> won't find a corresponding task for that message and will return an error at
> that point.
>
> But perhaps a bigger question is: do we want to abort ongoing file transfers as
> soon as a session becomes locked? Or do we want to simply disallow new file
> transfers from being initiated? Or am I misunderstanding the scenario here?
This is should not be about the locked-screen but the login-screen.
If I got everything right, after spice-vdagent is created we will meet the
requirements to check if vdagent belongs to a user or not; As soon as we
update_active_session_connection() for the first time, we can send
VDAGENTD_FILE_XFER_DISABLE to vdagent.
With vdagent_file_xfers set to NULL after Christophe's patch [0], we will return
an error to any request drag-and-drop request.
[0] https://cgit.freedesktop.org/spice/linux/vd_agent/commit/?id=9ea18740b8c
>
> > + }
> > + break;
> > case VDAGENTD_AUDIO_VOLUME_SYNC: {
> > VDAgentAudioVolumeSync *avs = (VDAgentAudioVolumeSync *)data;
> > if (avs->is_playback) {
> > diff --git a/src/vdagentd-proto-strings.h b/src/vdagentd-proto-strings.h
> > index 86332f9..6e7bcee 100644
> > --- a/src/vdagentd-proto-strings.h
> > +++ b/src/vdagentd-proto-strings.h
> > @@ -34,6 +34,7 @@ static const char * const vdagentd_messages[] = {
> > "file xfer start",
> > "file xfer status",
> > "file xfer data",
> > + "file xfer disable",
> > "client disconnected",
> > };
> >
> > diff --git a/src/vdagentd-proto.h b/src/vdagentd-proto.h
> > index b181105..9815488 100644
> > --- a/src/vdagentd-proto.h
> > +++ b/src/vdagentd-proto.h
> > @@ -40,6 +40,7 @@ enum {
> > VDAGENTD_FILE_XFER_START,
> > VDAGENTD_FILE_XFER_STATUS,
> > VDAGENTD_FILE_XFER_DATA,
> > + VDAGENTD_FILE_XFER_DISABLE,
> > VDAGENTD_CLIENT_DISCONNECTED, /* daemon -> client */
> > VDAGENTD_NO_MESSAGES /* Must always be last */
> > };
> > diff --git a/src/vdagentd.c b/src/vdagentd.c
> > index efcf951..42ce7a5 100644
> > --- a/src/vdagentd.c
> > +++ b/src/vdagentd.c
> > @@ -642,6 +642,15 @@ static void update_active_session_connection(struct
> > udscs_connection *new_conn)
> > active_session_conn = new_conn;
> > if (debug)
> > syslog(LOG_DEBUG, "%p is now the active session", new_conn);
> > +
> > + if (active_session_conn && !session_info_is_user(session_info)) {
> > + if (debug)
> > + syslog(LOG_DEBUG, "New session agent does not belong to user: "
> > + "disabling file-xfer");
> > + udscs_write(active_session_conn, VDAGENTD_FILE_XFER_DISABLE, 0, 0,
> > + NULL, 0);
> > + }
> > +
> > if (active_session_conn && mon_config)
> > udscs_write(active_session_conn, VDAGENTD_MONITORS_CONFIG, 0, 0,
> > (uint8_t *)mon_config, sizeof(VDAgentMonitorsConfig) +
>
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list