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.

Sign up for GitHub

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

Jump to bottom

Minor tweaks to ssh kitten and shell integration #4794

Merged
merged 7 commits into from Mar 7, 2022
Merged

Minor tweaks to ssh kitten and shell integration #4794

merged 7 commits into from Mar 7, 2022

Conversation

page-down
Copy link
Contributor

@page-down page-down commented Mar 7, 2022 •
edited

  • Avoid unnecessary which and fix typos.
  • Adjust the order of exec function definitions: zsh, fish, bash.
  • Improve SSH detection without affecting daily performance.
  • Remove the fish integration B prompt marking. (The same as zsh, bash.)

For SSH detection, first exclude the case that KITTY_PID exists.
Then check the existence of KITTY_WINDOW_ID.
Call who only if sudo is running (environment variables are cleared) and shell integration is manually installed for the switched user.

  • When run locally, only one environment variable, KITTY_PID, will be checked.
  • In most SSH connections, only KITTY_PID, SSH_TTY will be checked.
  • Only when the integration is installed manually and sudo is run, four environment variables will be checked and who will be called once.

Users can add KITTY_WINDOW_ID to sudo env_keep to avoid calling who.
SSH_TTY is not suitable for adding to env_keep, because it is possible that the switched user does not have permission to access it and affecting other programs.

Please review, thank you.

I'm a little concerned about whether tar x --no-same-owner will work properly on OpenBSD and am going to give it a try.

@page-down
Explicitly use builtin and command
df9e893
@page-down
Remove trailing semicolons
3095e7a
@page-down
Avoid unnecessary which and fix typos
397fbe7
@page-down
Unify exec function definition order
43fd8cb
@page-down
Detect SSH sessions without affecting performance
817ac82 
Exclude local runs by KITTY_PID.
Check KITTY_WINDOW_ID to detect connections via ssh kitten.
Check SSH via who -m with the integration manually installed and sudo.
@page-down
Remove the fish integration prompt end B prompt marking
1b68e41 
Currently kitty does not use the B prompt marking.
This is consistent with the zsh and bash implementations.
Improve compatibility with most user configurations.
@page-down
...
5f3d90e
@kovidgoyal kovidgoyal merged commit d4d4e00 into kovidgoyal:master Mar 7, 2022
@page-down page-down deleted the ksi branch March 7, 2022 06:00
@kovidgoyal
Copy link
Owner

According to the man page there is no --no-same-owner in openbsd so I doubt it will. https://man.openbsd.org/tar

@page-down
Copy link
Contributor Author

I saw the latest commit which should fix the tar issue.

I installed an openbsd vm and the first time I connected via ssh kitten it disconnected immediately after enter password.
Try to connect directly using ssh again:

tset: unknown terminal type xterm-kitty
Terminal type?

Try env TERM

kitty +kitten ssh --kitten="env TERM=xterm-256color" root@openbsd
# ... password: ...
# Connection to openbsd closed.

I wonder if other systems may also have this issue.

Regarding the hostname configuration, is there a special consideration that does not support matching the hostname set by the user? (Host configured in .ssh/config)
Could you share some thoughts on this?

A service component in a different project may use the same hostname of a common word. The hostname can be changed on the remote. etc. ...
It would be nice to additionally provide a way to specify the local name,
so that the user can explicitly use the specified configuration to send the files to the host when connecting.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 7, 2022 via email

@page-down
Copy link
Contributor Author

... Use the debug function to see where the script is failing. ...

sh: base64: not found

I was wondering if we could check the commands that will be used first, then output the warning log earlier and abort.
This doesn't even compare to the tools that come with the busybox which is a few MB in size.
I don't care about this system for now.

... The remote hostname is one simple point of configuration. ...

Ok, I understand that this is the acceptable way to configure it.

@page-down
Copy link
Contributor Author

patches for openbsd:

elif command -v uuencode > /dev/null 2> /dev/null; then
    base64_encode() { command uuencode -m -o /dev/stdout /dev/stdin | command sed '1d;$d' | command tr -d \\n\\r; }
    base64_decode() { printf 'begin-base64 644 -\n%s' $(</dev/stdin) | command uudecode -m -o /dev/stdout 2> /dev/null; }

if [ -n "$ZSH_VERSION" ] && builtin zmodload 

# dcs_to_kitty() { printf "\033P@kitty-$1|%s\033\\\\"
dcs_to_kitty() { printf "\033P@kitty-$1|%s\033\134"

Should the following head -c fall back to dd?
Is it possible to ask for gz?

head: unknown option -- c
tar: could not exec bzip2: No such file or directory

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 7, 2022 via email

@page-down
Copy link
Contributor Author

I went on to try FreeBSD.

# kitty +kitten ssh root@freebsd
/bin/sh: Event not found.
Missing name for redirect.
saved_tty_settings=: Command not found.
Badly placed ()'s.
...

Try modifying bootstrap.sh to see what is happening.

<Here is an empty line.>
ps -f
exit 0

Unmatched '''.
 PID TT  STAT    TIME COMMAND
# ...
1493  0  Ss+  0:00.00 csh -c sh -c '\nps -f\nexit 0'
1495  0  R+   0:00.00 ps -f
Unmatched '''.

chsh -s /bin/sh

# bootstrap.sh: failed at the last line:
# exec $login_shell "-l"

Illegal option -l

Is there any other option?

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 8, 2022 via email

@page-down
Copy link
Contributor Author

Which shell doesnt support -l?

/bin/sh
https://www.freebsd.org/cgi/man.cgi?sh

The root user uses /bin/csh (tcsh) by default, which is supported to run with -l, but I haven't been able to run it yet.
It seems to be a quoting problem, and the code is being run with (t)csh.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 8, 2022 via email

@page-down
Copy link
Contributor Author

Ah, well no I don't see an alternative. ...

I see that openbsd, freebsd all come with clang by default, which seems very possible ... It's just ridiculous.
Users can use the default csh or configure login_shell tcsh in ssh.conf to work properly.

csh is not POSIX compatible ...

We explicitly use sh to run it, so the code should work.
See my ps -f above.

  • default root login shell: /bin/csh
  • ssh sh -c "<bootstrap.sh codes>"
  • freebsd: csh -c sh -c ...
    • Unmatched '''. ...

It's just that under freebsd csh seems to treat the -c after sh as its own parameter.

@kovidgoyal
Copy link
Owner

Weird are you saying that on freebsd running sh -c "whatever" actually runs csh -c sh -c "whatever"?? If os that's a bug in FreeBSD.

@page-down
Copy link
Contributor Author

Linux

# ssh linux sh -c '"t(){ echo ok; }; t; sleep 10; exit 0"'
ok

# ps
\_ sshd:
   \_ sh -c t(){ echo ok; }; t; sleep 10; exit 0
       \_ sleep 10

FreeBSD with login shell /bin/csh

# user login shell: tcsh
# ssh freebsd sh -c '"t(){ echo ok; }; t; sleep 10; exit 0"'
ok

# ps
tcsh -c sh -c "t(){ echo ok; }; t; sleep 10; exit 0"
sh -c t(){ echo ok; }; t; sleep 10; exit 0

# ssh freebsd sh -c "t(){ echo ok; }; t; sleep 10; exit 0"
Badly placed ()'s.

FreeBSD with login shell /bin/sh

# user login shell: sh
# ssh freebsd sh -c '"t(){ echo ok; }; t; sleep 10; exit 0"'
ok

# ps
sh -c t(){ echo ok; }; t; sleep 10; exit 0

@page-down
Copy link
Contributor Author

For the above reason, setting it to python does not work either.

/usr/bin/env: Event not found.
Missing name for redirect.
import: Command not found.
...

I have fish installed on Linux and I would like to confirm that:
The code is actually executed by login_shell, not the explicitly specified sh.

# Linux, login shell: fish
# ssh sh -c '"t(){ echo ok; }; t; sleep 10; exit 0"'
\_ sshd:
    \_ fish -c sh -c "t(){ echo ok; }; t; sleep 10; exit 0"
        \_ sh -c t(){ echo ok; }; t; sleep 10; exit 0
            \_ sleep 10

@page-down
Copy link
Contributor Author

To summarize:
The current quoting style, only compatible with the login shell set in the remote system is POSIX compatible.
For example, on Linux the user's login shell is fish, which will not work properly. (kitty +kitten ssh --kitten="interpreter=python3" user@host)

I can think of one solution.
The sh -c code in the argument sent to ssh without any special quotes,
finds the available base64 decoder in its simplest form, and then decodes the bootstrap script to run.

ssh host sh -c "echo <BOOTSTRAP_SH_BASE64> | base64 -d | sh"
ssh host python3 -c "eval(compile(standard_b64decode(..."

Since the actual code is run by interpreter, it should work on all login shells as long as they don't have any special quotes.

@kovidgoyal
Copy link
Owner

But that will break if base64 is not available.

@page-down
Copy link
Contributor Author

But that will break if base64 is not available.

Replace the quotes with AAA, BBB, (not sure if Unicode will work) and then replace them back using simple tools like tr, do you think it will work?

@kovidgoyal
Copy link
Owner

yes that will likely work, note that awk is part of POSIX so we are not limited to tr

@kovidgoyal
Copy link
Owner

As is sed: https://pubs.opengroup.org/onlinepubs/009695299/utilities/sed.html

https://pubs.opengroup.org/onlinepubs/009604499/utilities/awk.html

@page-down
Copy link
Contributor Author

I don't see string expansion in the bootstrap code, and it looks like it could also be used where needed.

https://pubs.opengroup.org/onlinepubs/009604499/utilities/xcu_chap02.html#tag_02_06_02

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 8, 2022 via email

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 10, 2022 via email

@page-down
Copy link
Contributor Author

page-down commented Mar 10, 2022 •
edited

I tried the latest commit and unfortunately, it still needs to limit the path length.

unix_listener: path "/Users/test/Library/Caches/kitty/run/kssh-9023-a5bc21fdef2a7e26de907f92602636d6e3c57fff.baqiDKDaIHdFO4Ci" too long for Unix domain socket

I think the maximum length is 108 on Linux and 104 on macOS and FreeBSD.

EDIT:
~/.ssh/config

Host debug
    ControlPath ~/Library/Caches/kitty/ssh/kssh-12345-%C 
    ControlMaster auto

ssh debug
# unix_listener: path "<104 chars>" too long for Unix domain socket

@kovidgoyal
Copy link
Owner

why is the hash so long on your system, on mine its 40 chars why is it
67 on yours?

@kovidgoyal
Copy link
Owner

ssh -G -o ControlPath=%C test | grep -i path

on my macOS system gives a 40 char hash as expected

@page-down
Copy link
Contributor Author 

# which ssh
/usr/bin/ssh
# ssh -V
OpenSSH_8.6p1, LibreSSL 2.8.3
# ssh -G -o ControlPath=%C test | grep -i path
controlpath c586a5b9927432f0397bed623ab88f818128f848

@kovidgoyal
Copy link
Owner

So what command line is generating a hash of

a5bc21fdef2a7e26de907f92602636d6e3c57fff.baqiDKDaIHdFO4Ci

It seems extremely odd that the has format is not constant.

@page-down
Copy link
Contributor Author 

# ssh -G -o controlpath=$HOME/Library/Caches/kitty/ssh/kssh-12345-%C -o controlmaster=auto debug | grep path
controlpath /Users/test/Library/Caches/kitty/ssh/kssh-12345-c586a5b9927432f0397bed623ab88f818128f848

# ssh -o controlpath=$HOME/Library/Caches/kitty/ssh/kssh-12345-%C -o controlmaster=auto debug
unix_listener: path "/Users/test/Library/Caches/kitty/ssh/kssh-12345-cf75bcb79413dfc5a049e5dd48c071dbe28a704d.1AuEGEs2JjSHTQlg" too long for Unix domain socket

It's obviously not kitty's problem, hmm...

@kovidgoyal
Copy link
Owner

I looked at openssh code, it basically creates a temp socket file and
then renames it. And that temporary file is where the .suffix is coming
from. Sigh.

@page-down
Copy link
Contributor Author

I assume you are looking at the latest code, I upgraded to OpenSSH 8.9p1 and the problem still exists.
If that is the case, that %C is unusable.

@page-down
Copy link
Contributor Author

I tried the latest commit and now the ssh kitten works fine.
That's a brilliant workaround for the idiotic design.
I looked at the script for clearing tmp, the symlink we created (and the sockets) wouldn't be matched at all.

/etc/periodic/daily/110.clean-tmps

# ...
find -dx . -fstype local -type f $args -delete $print
find -dx . -fstype local ! -name . -type d $dargs -delete $print

@page-down
Copy link
Contributor Author

I updated to the latest commit.

There is a problem with using the --kitten option to override the configuration.
kittens/ssh/config.py

def options_for_host(hostname: str, username: str, per_host_opts: Dict[str, SSHOptions], cli_hostname: str = '', cli_uname: str = '') -> SSHOptions:
    # ...
        for name in option_names:
            for opts in rest:
                # ...
                    setattr(ans, name, val)

~/.config/kitty/ssh.conf

shell_integration disabled

hostname debug
shell_integration enabled

matches: '*', 'debug', 'user@debug'

The login shell and shell integration configured by the following command does not take effect.
kitty +kitten ssh --kitten='hostname debug' --kitten='login_shell zsh' debug

hostname -> * -> debug -> user@debug
login_shell -> "" -> "" -> ""
shell_integration -> disabled -> enabled -> inherited

The login shell configured by the following command takes effect, but the shell integration is set to the default value from SSHOptions.
kitty +kitten ssh --kitten='login_shell zsh' debug

hostname -> * -> debug -> user@debug
login_shell -> "" -> "" -> zsh
shell_integration -> disabled -> enabled -> inherited

Now the following warning appears, is it possible to suppress it? E.g. > /dev/null
I found this a bit annoying, not too concerned about the time difference within one second.

tar: data.sh: time stamp xxx is 0.xxx s in the future

Is it worth providing a subcommand to disconnect all shared ssh connections? I found it might be useful when I was trying password login.

Is it appropriate to use the POSIX SHELL environment variable to get the login shell?
(Except that it did not take effect immediately after chsh with shared ssh.)
Is it efficient in most cases?

@page-down
Copy link
Contributor Author

When connecting to a non-existent host, the data is written to the local shell after ssh exits.

# non-existent host
kitty +kitten ssh test-not-exists

# ssh not yet started or port not listened to
kitty +kitten ssh user@debug:2222

ssh: Could not resolve hostname test-not-exists: nodename nor servname provided, or not known
# executed as commands
[127]> QZhtQZhtQZhtQZhtQZhtQZhtQZhtQZhtQZht...

I guess this also happens when ssh quits in the middle of transmission for any reason. Not yet tested for interruptions due to network connection failure.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 15, 2022 via email

@page-down
Copy link
Contributor Author

... check drain_potential_tty_garbage() ...

After running kitty the first connection works fine and eventually reads KITTY_DATA_END.
Connecting again is problematic, and the following condition is not valid.

while ... and select([tf], [], [], 0.075)[0]:

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 15, 2022 via email

@page-down
Copy link
Contributor Author

does not spew garbage the first time it is run, but does the second

Yes, or run kitty +kitten ssh localhost (never connected), even if it's the first time you run it after kitty starts.

macOS, OpenSSH 8.9p1

# I think it's at alt screen.
Are you sure you want to continue connecting (yes/no/[fingerprint])? 
> no
Host key verification failed.
XXXXX base64 ...

Linux, OpenSSH 8.2p1

The authenticity of host ...
ECDSA key fingerprint is ...
Are you sure you want to ... ?
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint:

I haven't tested the latest openssh under Linux yet.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 15, 2022 via email

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 15, 2022 via email

@page-down
Copy link
Contributor Author

Thanks, I updated to the latest commit and it's working fine.
Regarding the last issue I mentioned above,
here's what happened on the first and second run on Linux after I cleared known_hosts.

$ kitty +kitten ssh debug
The authenticity of host ... can't be established.
ECDSA key fingerprint is ...
Are you sure you want to continue connecting (yes/no/[fingerprint])? 
Host key verification failed.

$ kitty +kitten ssh debug
The authenticity of host ... can't be established.
ECDSA key fingerprint is ...
Are you sure you want to continue connecting (yes/no/[fingerprint])? 
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint: 
Please type 'yes', 'no' or the fingerprint:

Is this a compatibility issue?

@kovidgoyal
Copy link
Owner

That's an impossible issue :)

  1. ssh is asking for confirmation in the terminal, which means you have
    disabled askpass in ssh.conf?

  2. The kitten is sending data over the terminal without waiting for
    ssh to finish connecting, which can only happen if the kittne thinks ssh
    will use askpass, which means you havent changed it in ssh.conf

So why is ssh not using kitty's askpass on your Linux system?

@kovidgoyal
Copy link
Owner

Oh and btw to tets this you dont need to clear th eknown hosts file, you can just do

kitty +kitten ssh -o UserKnownHostsFile=/dev/null -o StrictHostKeyChecking=ask whatever

@page-down
Copy link
Contributor Author

Thanks for the tip.
OpenSSH 8.2p1 from Ubuntu LTS 20.04 does not support SSH_ASKPASS_REQUIRE, so it does not work.
If I remove os.environ['SSH_ASKPASS_REQUIRE'] = 'force' then I can reproduce the above issue under Arch Linux (with the latest OpenSSH).
Maybe a fallback is needed.

@kovidgoyal
Copy link
Owner

Sigh, endlessly annoying Linux distros with their ancient software.

It was introduced in openssh 8.4 so one just needs to check the version
with ssh -V.

@page-down
Copy link
Contributor Author

I tried the latest commit and found that the following doesn't work.

if os.environ.get('DISPLAY') or ssh_version() >= (8, 4):

Since the environment variable DISPLAY is always present, it is equivalent to not checking the version.
If there must be a fallback by default, perhaps the only way to skip this check is to configure askpass explicitly in ssh.conf.

... do the copy without any roundtrips ... it wont be possible to do even this.
So lets table this for now and get back to it later. ...

It seems that this has landed.
Since we are using a shared connection, I wonder if after the first connection is established, can we assume that the terminfo, shell_integration files, etc. have been updated to the current version and subsequent connections don't need to send these again, and there is no need to tar data.sh and extract.

These are files that are mandatory before the login shell runs, and after this is resolved, other files might be able to be synchronized asynchronously after the connection (e.g. explicitly configured to use librsync).

@kovidgoyal
Copy link
Owner

kovidgoyal commented Mar 15, 2022 via email

@kovidgoyal
Copy link
Owner

I just checked, it compresses to 15K.

@page-down
Copy link
Contributor Author

... it's a very marginal saving for a fair bit of complication ...
Cant do that, other files might be needed to ...
I just checked, it compresses to 15K.

OK, It is not possible to cover everything, and users with special needs will need to write their own tools. This obviously cannot be done with minimal cost to know the status of the target host.

Also this reminds me of a scenario where a user (e.g., root) on a device can log in and run programs or write files, but there is no $HOME.
I think in this environment the user needs to specify the extract base dir and the TERMINFO path themselves.

@page-down
Copy link
Contributor Author

page-down commented Mar 28, 2022 •
edited

@kovidgoyal
I tested one more SSH server: TinySSH.
https://github.com/janmojzis/tinyssh

When connecting with OpenSSH client version v8.9p1, it outputs KITTY_DATA_START ... KITTY_DATA_END.
Is there a way to work around this?
For example by specifying need_to_request_data in the host configuration. (stty "-echo" and stty "echo")

@kovidgoyal
Copy link
Owner

Sounds like a bug in that server, its not cloning the echo state of the
local terminal on the remote side. You can force it by turning off
connection sharing and kitty askpass for that host. There isn't a
dedicated setting for it. Is this server widely used?

@page-down
Copy link
Contributor Author

... its not cloning the echo state ...

OK, then this better be fixed in that project.

... turning off connection sharing and kitty askpass ...

These two options are sufficient, and the latency is acceptable.

Is this server widely used?

No. There are actually not many open source SSH servers, and this is the last one I want to try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@page-down @kovidgoyal