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.
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.
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.
Main part of this is in r31562.
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
Reopening for discussion about alternate solution.
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
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
(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.
(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).
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.
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.
thinlinc-login needs to be updated to allow xsession as a permitted command when thinlinc-login is used as the user's shell.
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.
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.
(In reply to comment #31) > This means that we can't rely on the shell being set in a getpwnam answer.
Discovered issues should be fixed now.
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.
(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
(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.
Works as expected.
Actually, the file was changed. But dpkg still doesn't correct the permissions for some reason.
Verified an upgrade of 4.6.0 to 4.7.0 and the permissions are changed on xsession file. Tested on debian 8.
