Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wezterm uses the login shell in /etc/passwd instead of the SHELL environment variable #4098

Closed
ShellCode33 opened this issue Aug 5, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@ShellCode33
Copy link

ShellCode33 commented Aug 5, 2023

Nothing in that post is relevant, this is not an ssh issue, see the comments down below.

What Operating System(s) are you seeing this problem on?

Linux Wayland

Which Wayland compositor or X11 Window manager(s) are you using?

sway version 1.8.1

WezTerm version

20230802-190652-a7c6ea8c

Did you try the latest nightly build to see if the issue is better (or worse!) than your current version?

Yes, and I updated the version box above to show the version of the nightly that I tried

Describe the bug

Fresh boot, after opening wezterm :

$ ps aux | grep ssh-agent
shellco+    1951  0.0  0.0   8436  1720 ?        Ss   15:07   0:00 ssh-agent
shellco+    2310  0.0  0.0   8568  1720 ?        Ss   15:08   0:00 ssh-agent

As you can see, 2 ssh-agents are running.

I spawn my own ssh agent in the config of my login shell. I have the following in ~/.profile :

# Run ssh-agent and export its variables
eval $(ssh-agent) > /dev/null

# Import all the environment variables into both dbus and systemd
dbus-update-activation-environment --systemd --all

# Start graphical server on user's current tty if not already running.
if [ "$(tty)" = "/dev/tty1" ] && ! pidof -s "$XDG_CURRENT_DESKTOP" >/dev/null 2>&1
then
    exec "$XDG_CURRENT_DESKTOP" >"$XDG_STATE_HOME/$XDG_CURRENT_DESKTOP.log" 2>&1
else
    export TMOUT=600
    exec zsh
fi

Moreover, everytime I spawn a new terminal, a new ssh agent is spawned, resulting in many instances being run at the same time:

$ ps aux | grep ssh-agent
shellco+    1951  0.0  0.0   8436  1720 ?        Ss   15:07   0:00 ssh-agent
shellco+    2310  0.0  0.0   8568  1720 ?        Ss   15:08   0:00 ssh-agent
shellco+    5245  0.0  0.0   8436  1720 ?        Ss   15:20   0:00 ssh-agent
shellco+    5309  0.0  0.0   8436  1716 ?        Ss   15:20   0:00 ssh-agent

To Reproduce

  • Add eval $(ssh-agent) > /dev/null to your login shell. For me it's ~/.profile because I'm using dash, but for you it might be ~/.bash_profile is you're using bash.
  • Logout and log back in
  • Open wezterm
  • Run ps aux | grep ssh-agent
  • Notice multiple ssh agents are running

Configuration

no config

Expected Behavior

wezterm should use the existing ssh agent instead of spawning new ones

Logs

15:20:23.569  ERROR  wezterm_mux_server_impl::local > writing pdu data buffer: Broken pipe (os error 32)
15:20:28.977  ERROR  wezterm_mux_server_impl::local > writing pdu data buffer: Broken pipe (os error 32)

Anything else?

I use KeePassXC's SSH agent integration to manage my SSH keys. It is in charge of inserting/removing keys from memory when the database is being unlocked/locked. It relies on SSH_AUTH_SOCK to know which ssh agent to use. Because wezterm spawns its own ssh agent and set SSH_AUTH_SOCK accordingly, I'm unable to connect to my servers by using SSH keys already available in the ssh-agent I run.

I'm pretty sure it's irrelevant, but just in case:

$ uname -a
Linux laptop 6.4.7-hardened1-2-hardened #1 SMP PREEMPT_DYNAMIC Wed, 02 Aug 2023 11:05:52 +0000 x86_64 GNU/Linux
@ShellCode33 ShellCode33 added the bug Something isn't working label Aug 5, 2023
@wez
Copy link
Owner

wez commented Aug 5, 2023

wezterm isn't spawning any ssh-agents; your login shell is spawning them.

I would suggest making your shell logic conditional; for example, checking to see if SSH_AUTH_SOCK is already set in the environment before spawning a new agent.

Alternatively, you can set your default_prog to spawn your shell as a non-login shell. eg: config.default_prog = {"dash"}

Note that wezterm's own built-in ssh client would prefer it if the agent is already running before wezterm is started, so that it can itself use your ssh agent.

@wez wez added the waiting-on-op Waiting for more information from the original poster label Aug 5, 2023
@ShellCode33
Copy link
Author

ShellCode33 commented Aug 5, 2023

Ok thanks, now I understand where the problem comes from. I will have to look into it but if I'm not mistaken, wezterm should not be using my login shell. Usually terminal emulators flow is:

  • Check the config file for explicit settings (default_prog in that case), if set, use it
  • Otherwise check if the SHELL variable is set, if so, use it (additional checks should be done, the path might be invalid or the binary not executable)
  • Otherwise fallback to the login shell in /etc/passwd

My SHELL variable is set system-wide by my login shell, in ~/.profile I have:

export SHELL=/usr/bin/zsh

There's no reason for wezterm to use my /bin/sh login shell when I explicitly tell him to use zsh through the SHELL variable.

@github-actions github-actions bot removed the waiting-on-op Waiting for more information from the original poster label Aug 5, 2023
@ShellCode33 ShellCode33 changed the title wezterm spawns multiple SSH agents wezterm uses the login shell in /etc/passwd instead of the SHELL environment variable Aug 5, 2023
@wez
Copy link
Owner

wez commented Aug 5, 2023

wezterm doesn't use $SHELL any more. It's not very clearly shown on https://wezfurlong.org/wezterm/config/launch.html but it is there; we use the shell that is configured for the user via the passwd database so that long running wezterm processes will spawn the correct shell if the user changes it, without having to restart wezterm and lose all their state in their various tabs and panes.

If you don't want to set the correct shell in the password database, then the resolution is for you to change the default_prog.

@wez
Copy link
Owner

wez commented Aug 5, 2023

also: why is your /etc/passwd shell set to /bin/sh? That seems unusual and a bit restrictive.

@wez wez added the waiting-on-op Waiting for more information from the original poster label Aug 5, 2023
@ShellCode33
Copy link
Author

ShellCode33 commented Aug 5, 2023

I was unable to find any specification regarding the SHELL variable except the following:

This variable shall represent a pathname of the user's preferred command language interpreter. If this interpreter does not conform to the Shell Command Language in XCU Shell Command Language, utilities may behave differently from those described in POSIX.1-2017.

I was also unable to find anything on vt100.net, so I guess it's not really a requirement for terminal emulators to use it.


If you don't want to set the correct shell in the password database

This is the correct shell ! What do you want me to use ? The login shell should be a POSIX compliant shell, people using shells such as zsh in their passwd db are prone to a lot of trouble (even if the doc says there's an "emulated POSIX compliance"). I've experienced it in the past, some stuff broke randomly, it was very hard to debug and understand that the apparently-not-really-posix-shell was the culprit.

I could use bash but I'd rather use dash which is faster and smaller (less prone to bugs).

Having /bin/sh in my passwd db doesn't mean that's the one I use on a daily basis. It's just the shell being run by /usr/bin/login that I use to setup system-wide environment variables and start my graphical session (I don't have a display manager).

But default_prog definitely works for me, thanks 👍

Though I still think you should be using the SHELL environment variable, I don't see any case where users would change the SHELL variable on the fly, it's usually something set by /usr/bin/login (see man login(1)) or by the profile of the login shell, users could also decide to change their default login shell using chsh, what should you do then? It should be users' responsibility to know that what they're doing can have side-effects

EDIT: I think the likelihood of users running chsh is higher than users changing their SHELL variable manually. Considering that, I think wezterm would be more robust if it relied on SHELL instead of the passwd db

EDIT2: FYI on modern GNU/Linux distributions /bin/sh is usually a symlink to either bash or dash

@github-actions github-actions bot removed the waiting-on-op Waiting for more information from the original poster label Aug 5, 2023
@ShellCode33
Copy link
Author

ShellCode33 commented Aug 5, 2023

Well, I just noticed that even if I use default_prog to start zsh, wezterm still sets the SHELL to my login shell (/bin/sh). It seems to me that it's a bit invasive, it should be users' responsibility to do that. Programs that I start using wezterm will get the wrong SHELL variable (and most of them will rely on it instead of the passwd db for sure). I could set the SHELL variable in my zshrc to workaround that, but it would be an ugly hack

@wez
Copy link
Owner

wez commented Aug 5, 2023

It seems to me that your usage of SHELL is a bit unusual.
The classic unix approach for shells is:

  • The passwd entry is set to the preferred shell for that user
  • The login/session initiation machinery reads that and exports it as SHELL, then spawns the shell in login mode (by spawning it as -$SHELL)

(wezterm operates along those same principles: it is initiating your sessions)

The login shell is not implicitly or explicitly required to be /bin/sh. That just happens to be the default posix interpreter on old unix systems.

There are no rules against having other shells as the default, and in fact most modern unix systems use either bash or zsh by default.

If you intend to use zsh, then there should be no problems making zsh your shell, and avoiding the complication of setting an alternative shell from ~/.profile and then the overhead of subsequently spawning that shell.

Some people fear setting the shell of the root user to anything other than a basic /bin/sh. I can understand the reasoning there, but in the context of wezterm: those folks probably should not be logging into a graphical environment as root to spawn wezterm as root, and only then being worried about using zsh instead of sh.

For users that follow the standard approach, wezterm's use of the passwd entry to respect the latest changes made via chsh is perfectly fine and aligns with their expectations. I know this because of previous issues filed here to change the behavior away from using $SHELL.

For users that are doing something unusual, then the combination of default_prog and set_environment_variables config options are sufficient to encode setting things up differently.

@wez wez closed this as completed Aug 5, 2023
@ShellCode33
Copy link
Author

ShellCode33 commented Aug 5, 2023

wezterm operates along those same principles: it is initiating your sessions

Alright I can see the reasoning here. Even though I don't think it's a good idea to assume that the login shell and the "regular" shell are always the same, your "is initiating sessions" reasoning makes sense to me, it also explains why you set the SHELL variable. I have to say however that it's the first time I use a terminal emulator that doesn't comply with my SHELL variable, others behave as I would expect, and I don't think it's that "unusual" to quote you.

Thanks for your explanations

wez added a commit that referenced this issue Aug 20, 2023
* Allow set_environment_variables to set the SHELL.
  Previously, we'd always set it to the shell from the password
  database.  Now we do that by default, but if set_environment_variables
  has been used, we'll preserve that value for the environment
  of the to-be-spawned process

* Clarify the docs:
  * Remove the confusing version dependent sections that started
    with old behavior and rewrite in terms of the behavior that
    has been true for the past year
  * I've heard from a few people that they tried to change COMSPEC
    on Windows and pain ensued.  Try to nudge them to read the
    next paragraph that tells them what they are actually supposed
    to change.

refs: #4098
refs: #4168
closes: #3816
refs: #3317
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants