www.cendio.com
Bug 5099 - sessions are always started through bash, not the user's shell
: sessions are always started through bash, not the user's shell
Status: CLOSED FIXED
: ThinLinc
VSM Agent
: trunk
: PC Unknown
: P2 Normal
: 4.7.0
Assigned To:
:
:
:
: 5411
  Show dependency treegraph
 
Reported: 2014-04-16 11:43 by
Modified: 2016-09-29 14:26 (History)
Acceptance Criteria:


Attachments
Combined patch of this work (8.14 KB, patch)
2016-08-24 17:05, Peter Åstrand
Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From cendio 2014-04-16 11:43:09
We have hard coded bash as the startup shell for ThinLinc, overriding whatever
shell the user might have configured. This means that the user will not get the
same environment as if logged on locally.

Most users don't notice the difference, but some do and we've gotten a few
complaints so we might want to change things.

Currently, this is how we start the session:

  /bin/bash --login /opt/thinlinc/etc/xsession

(notice the lack of "-c")

This is how Fedora/Red Hat starts a graphical session, and might be a good
substitute:

  /bin/sh -c "exec -l $SHELL -c \"$1\""

$SHELL would have to be looked up by the agent and $1 would be replaced with 
/opt/thinlinc/etc/xsession in our case.


The difficult part is how much support we want to give to non-functional
shells. Currently we recommend customers to set the shell to
/usr/bin/thinlinc-login as setting it to /bin/false (as is otherwise common)
breaks ThinLinc.
------- Comment #1 From cendio 2015-02-03 13:33:56 -------
Note that this should also resolve bug 5411 as we should no longer have a need
to run our scripts with bash in login mode. System bash scripts don't need to
protect themselves against -u, so there shouldn't be a need for us either.
------- Comment #3 From cendio 2016-07-11 16:29:55 -------
We are also documenting that xstartup is being executed, but in reality it is
being sourced. This is probably another symptom of our bash centric world view.

I would suggest we fix this by starting to execute them. There is no big reason
why we need to source them, given the current code. This would also make
ThinLinc behave more like a local login, with these equivalents:

/etc/X11/xinit/Xsession <=> /opt/thinlinc/etc/xsession
~/.xsession <=> ~/.thinlinc/xstartup

It doesn't match perfectly though as the local login equivalent of xstartup.d
happens much earlier. That means that what we have in xstartup.default
currently maps partially to GDM, partially to various scripts in
/etc/X11/xinit.


We however also document that tl-run-xstartup.d is executed, even though it is
sourced (which it must be for it to work). The rest of the text elaborates
correctly, but we may need some adjustment to the wording.
------- Comment #5 From cendio 2016-07-12 16:51:59 -------
Main part of this is in r31562.
------- Comment #9 From cendio 2016-07-13 11:37:29 -------
Should be all done.

Tester should verify:

 - Documentation
 - Switching to different shell
 - That you get a login shell
 - That noshell still works
 - Custom xstartup
 - That set -u in .bashrc doesn't break anything
------- Comment #10 From cendio 2016-08-09 09:59:42 -------
Reopening for discussion about alternate solution.
------- Comment #11 From cendio 2016-08-24 17:05:44 -------
Created an attachment (id=734) [details]
Combined patch of this work

The attached patch is the combined work on this bug. Generated with:

/home/astrand/bin/svn-total-diff r31559 r31561 r31562 r31563 r31564 r31566
r31567
------- Comment #12 From cendio 2016-08-26 09:19:05 -------
Committed solution vs alternative:

Pro:

 - Better mimics how a local system behaves where our xsession corresponds to
/etc/X11/Xsession. Less risk for bugs and more familiar for admins.

 - More control for the admin how the session is invoked

 - User startup scripts can override the default environment set up by xsession

 - xstartup can now be written in any language

 - Better layering as the command line is now coded where it is also executed

Con:

 - Admins/users will need to port over any changes they've made to xsession and
xstartup

 - Users who relied on sourcing would need to add a shebang and set the execute
bit (could probably be handled with a fallback)

 - The white-list in thinlinc-login/noshell will need more entries and may stop
working if xsession invokes something non-default via the shell
------- Comment #13 From cendio 2016-08-29 09:55:03 -------
(In reply to comment #12)

> 
> Con:
> 
>  - Admins/users will need to port over any changes they've made to xsession and
> xstartup

Also note that this violates the general principle of "old config files should
work". For example, consider this case:

1. Running 4.6, making some change to xstartup.default, even minimal

2. Upgrading to 4.7. In tl-setup, selecting "params" or "old" when asking to
migrate the configuration. 

Now you will have a system with no SIGHUP handling at all in the active config
files. 

If xstartup.default had some other changes, such as removed exec-bits or
hashbang, you will have even more severe problems. 

If the new implementation should stay, I think we need to provide some
warning/information in TAG and/or tl-setup. 

Wrt the SIGHUP stuff, we could also check if this is still necessary.
------- Comment #16 From cendio 2016-08-29 10:49:49 -------
(In reply to comment #12)
> Committed solution vs alternative:
> 
> Pro:
> 
>  - Better mimics how a local system behaves where our xsession corresponds to
> /etc/X11/Xsession. Less risk for bugs and more familiar for admins.

I think this argument is weak, considering that a local login is so very
different. For example, with GDM etc you first select the desktop type, then
login, but with ThinLinc, the desktop type is selected via the profile
selector, which happens as a part of the session startup scripts. 

I think it's more value in "If it ain't broke, don't fix it". 


>  - User startup scripts can override the default environment set up by xsession

On the other hand, sometimes the admin wants to be in control instead... In
practice, I think this point is mainly for this code:

# Set language on Debian based systems
if [ -r /etc/default/locale ]; then
    source /etc/default/locale
    export LANG LANGUAGE LC_NUMERIC LC_TIME LC_MONETARY LC_PAPER 
    export LC_IDENTIFICATION LC_NAME LC_ADDRESS LC_TELEPHONE LC_MEASUREMENT
fi

It's actually a bit odd and probably belongs in vsmagent instead. With things
configured with /vsmagent/default_environment, user startup scrips can still be
overridden with the old/my implementation. 

Also, even if a majority of the users thinks its better that the environment
can be overridden by user files, this would still be a behavioural change,
which can be a problem for some customers.


>  - xstartup can now be written in any language

This is also possible with my proposal, if xsession is configured for this. 


In general, my main point is that xsession, xstartup.default et al are
configuration files, and we should strive for having them stable, and avoid
incompatible changes. If the customer wants to rewrite them in another
language, use "exec" instead of "source", they can do so, but we should not
(ab)use the configuration files for implementing new functionality (such as
starting to use $SHELL instead of Bash).
------- Comment #17 From cendio 2016-08-29 11:26:51 -------
Trying to summarize:

The core of Pierres idea is:

--- vsm/modules/thinlinc/vsm/sessionstart.py    (revision 31558)
+++ vsm/modules/thinlinc/vsm/sessionstart.py    (revision 31567)
@@ -179,7 +179,7 @@

         tlsession = os.path.join(self.session_env['TLPREFIX'], "libexec",
"tl-session")
-        args = [tlsession, "/bin/bash", "--login", xstartupfile,
+        args = [tlsession, xstartupfile,

--- vsm/xsession    (revision 31558)
+++ vsm/xsession    (revision 31567)
@@ -17,9 +14,10 @@
 # Log system/distribution information
 source ${TLPREFIX}/libexec/log_sysinfo.sh

-# Source xstartup script
+# Custom xstartup script has priority
 if [ -f ~/.thinlinc/xstartup ] ; then
-    source ~/.thinlinc/xstartup
-else
-    source "${TLPREFIX}/etc/xstartup.default"
+    exec -l $SHELL -c ~/.thinlinc/xstartup
 fi
+
+# Default xstartup script
+exec -l $SHELL -c "${TLPREFIX}/etc/xstartup.default"


The core of my idea is:

--- vsm/modules/thinlinc/vsm/sessionstart.py.31119    2016-08-29
11:25:11.802366455 +0200
+++ vsm/modules/thinlinc/vsm/sessionstart.py    2016-08-29 11:18:42.478280781
+0200
@@ -179,7 +179,8 @@
         vncpasswdfile = locale_encode(self.vncpasswdfile)

         tlsession = os.path.join(self.session_env['TLPREFIX'], "libexec",
"tl-session")
-        args = [tlsession, "/bin/bash", "--login", xstartupfile,
+        shell = self.session_env["SHELL"]
+        args = [tlsession, shell, "-c", xstartupfile,


Here's my thinking: if we use my minimal solution now, we can always do the
bigger structural changes, as proposed by Pierre, later.
------- Comment #18 From cendio 2016-09-05 13:52:56 -------
These commits were done as part of the first attempt to solve bug 5099.

Reverting:

 * r31567: xstartup scripts should now always be executable
 * r31566: xsession is now expected to be executable
 * r31562: Start session via the user's shell
 * r31560: Add copyright header to xstartup.default
 * r31559: exec xstartup instead of sourcing it

Keeping:

 * r31564: Fix xstartup file reference

   Documentation fix, still valid with old code.

 * r31563: Remove documentation on bash --login

   Needs adjusting but the general idea is valid for alternative solution as
well.

 * r31561: Don't let anyone execute tl-run-xstartup.d/logout.d

   Decided to keep this after discussions with Pierre. He created a new bug
(bug 5976) for it so the change can be documented properly in release notes.
------- Comment #27 From cendio 2016-09-06 14:07:56 -------
thinlinc-login needs to be updated to allow xsession as a permitted command
when thinlinc-login is used as the user's shell.
------- Comment #29 From cendio 2016-09-07 10:37:49 -------
Dash (/bin/sh) on Ubuntu 16.04 does not support the "-l" flag to exec.

> tl-xinit: Xserver ready for clients.
> /bin/sh: 1: exec: -l: not found
> tl-xinit: client terminated and returned 127
> tl-xinit: Session terminated. Exiting.
------- Comment #31 From cendio 2016-09-08 15:08:36 -------
Modifying sessionstart.py to use "/bin/bash" instead of "/bin/sh" to get around
the exec -l problems on Ubuntu in turns makes /opt/thinlinc/etc/xsession fail
because TLPREFIX isn't set.

> /opt/thinlinc/etc/xsession: line 18: /libexec/log_sysinfo.sh: No such file or directory
> /opt/thinlinc/etc/xsession: line 24: /etc/xstartup.default: No such file or directory
> tl-xinit: client terminated and returned 1
> tl-xinit: deleting ../1.1473236166.ended
> tl-xinit: Session terminated. Exiting.

Why this was happening took a while to find. When I created my users, Ubuntu
declined to set a shell for them. That led to this command line being created
by vsmagent:

> /opt/thinlinc/libexec/tl-xinit /bin/bash -c exec -l  -c /opt/thinlinc/etc/xsession -- [...]
> # There should be a shell here!                   ^^^^^

exec -c in bash means:
>      -c		execute COMMAND with an empty environment


If I log in to Unity through lightdm and open a terminal, /bin/bash gets
started for me.

If I log in through OpenSSH, I get a /bin/sh shell.
------- Comment #32 From cendio 2016-09-08 15:11:21 -------
(In reply to comment #31)
>

This means that we can't rely on the shell being set in a getpwnam answer.
------- Comment #38 From cendio 2016-09-09 09:44:47 -------
Discovered issues should be fixed now.
------- Comment #39 From cendio 2016-09-12 15:15:15 -------
Tested using ThinLinc 4.6.0 build 5230 with following shells bash, ksh, tcsh
and thinlinc-login

>  - Documentation

Looks good.

>  - Switching to different shell

Verified using ksh,tcsh and thinlinc-login. Works as expected.

>  - That you get a login shell

Verified with ksh, tcsh and bash. Works as expected.

>  - Custom xstartup

Works as expected.
------- Comment #40 From cendio 2016-09-12 15:27:10 -------
(In reply to comment #39)
> Tested using ThinLinc 4.6.0 build 5230 with following shells bash, ksh, tcsh
> and thinlinc-login
> 

These tests was performed on Ubuntu 16.04
------- Comment #41 From cendio 2016-09-12 16:22:42 -------
(In reply to comment #9)
> Should be all done.
> 
> Tester should verify:
> 
>  - That set -u in .bashrc doesn't break anything

Added set -u to user .profile and verified that a session could start as
expected.
------- Comment #42 From cendio 2016-09-12 16:27:25 -------
Works as expected.
------- Comment #44 From cendio 2016-09-26 13:23:56 -------
Actually, the file was changed. But dpkg still doesn't correct the permissions
for some reason.
------- Comment #46 From cendio 2016-09-27 14:20:33 -------
Verified an upgrade of 4.6.0 to 4.7.0 and the permissions are changed on
xsession file. Tested on debian 8.