Bug 5099 - sessions are always started through bash, not the user's shell
Summary: sessions are always started through bash, not the user's shell
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: VSM Agent (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.7.0
Assignee: Pierre Ossman
URL:
Keywords: hean01_tester, relnotes
Depends on:
Blocks: 5411
  Show dependency treegraph
 
Reported: 2014-04-16 11:43 CEST by Pierre Ossman
Modified: 2016-09-29 14:26 CEST (History)
1 user (show)

See Also:
Acceptance Criteria:


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

Description Pierre Ossman cendio 2014-04-16 11:43:09 CEST
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 Pierre Ossman cendio 2015-02-03 13:33:56 CET
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 Pierre Ossman cendio 2016-07-11 16:29:55 CEST
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 Pierre Ossman cendio 2016-07-12 16:51:59 CEST
Main part of this is in r31562.
Comment 9 Pierre Ossman cendio 2016-07-13 11:37:29 CEST
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 Peter Åstrand cendio 2016-08-09 09:59:42 CEST
Reopening for discussion about alternate solution.
Comment 11 Peter Åstrand cendio 2016-08-24 17:05:44 CEST
Created attachment 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 Pierre Ossman cendio 2016-08-26 09:19:05 CEST
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 Peter Åstrand cendio 2016-08-29 09:55:03 CEST
(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 Peter Åstrand cendio 2016-08-29 10:49:49 CEST
(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 Peter Åstrand cendio 2016-08-29 11:26:51 CEST
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 Karl Mikaelsson cendio 2016-09-05 13:52:56 CEST
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 Pierre Ossman cendio 2016-09-06 14:07:56 CEST
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 Karl Mikaelsson cendio 2016-09-07 10:37:49 CEST
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 Karl Mikaelsson cendio 2016-09-08 15:08:36 CEST
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 Karl Mikaelsson cendio 2016-09-08 15:11:21 CEST
(In reply to comment #31)
>

This means that we can't rely on the shell being set in a getpwnam answer.
Comment 38 Pierre Ossman cendio 2016-09-09 09:44:47 CEST
Discovered issues should be fixed now.
Comment 39 Henrik Andersson cendio 2016-09-12 15:15:15 CEST
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 Henrik Andersson cendio 2016-09-12 15:27:10 CEST
(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 Henrik Andersson cendio 2016-09-12 16:22:42 CEST
(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 Henrik Andersson cendio 2016-09-12 16:27:25 CEST
Works as expected.
Comment 44 Pierre Ossman cendio 2016-09-26 13:23:56 CEST
Actually, the file was changed. But dpkg still doesn't correct the permissions for some reason.
Comment 46 Henrik Andersson cendio 2016-09-27 14:20:33 CEST
Verified an upgrade of 4.6.0 to 4.7.0 and the permissions are changed on xsession file. Tested on debian 8.

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