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

ssh/tailssh: set pty termios settings #4146

Closed
bradfitz opened this issue Mar 11, 2022 · 8 comments
Closed

ssh/tailssh: set pty termios settings #4146

bradfitz opened this issue Mar 11, 2022 · 8 comments
Labels
ssh Relating to Tailscale SSH https://tailscale.com/blog/tailscale-ssh/

Comments

@bradfitz
Copy link
Member

Our SSH server isn't yet setting the pty termio settings. Using stty -a, you can see the difference. This Go-based JSON-supporting stty makes it easier to see:

https://github.com/u-root/u-root/tree/main/cmds/core/stty

Diff from normal to ours:

bradfitz@tsdev:~$ diff -u stty-normal.json  stty-ts.json
--- stty-normal.json	2022-03-11 07:45:12.833294697 -0800
+++ stty-ts.json	2022-03-11 07:45:07.765250539 -0800
@@ -1,12 +1,12 @@
 {
 	"Ispeed": 0,
 	"Ospeed": 0,
-	"Row": 24,
-	"Col": 80,
+	"Row": 62,
+	"Col": 143,
 	"CC": {
 		"eof": 4,
-		"eol": 255,
-		"eol2": 255,
+		"eol": 0,
+		"eol2": 0,
 		"erase": 127,
 		"intr": 3,
 		"kill": 21,
@@ -27,7 +27,7 @@
 		"echo": true,
 		"echoctl": true,
 		"echoe": true,
-		"echok": false,
+		"echok": true,
 		"echoke": true,
 		"echonl": false,
 		"echoprt": false,
@@ -39,14 +39,14 @@
 		"ignbrk": false,
 		"igncr": false,
 		"ignpar": false,
-		"imaxbel": true,
+		"imaxbel": false,
 		"inlcr": false,
 		"inpck": false,
 		"isig": true,
 		"istrip": false,
 		"iuclc": false,
-		"iutf8": true,
-		"ixany": true,
+		"iutf8": false,
+		"ixany": false,
 		"ixoff": false,
 		"ixon": true,
 		"noflsh": false,
@@ -61,7 +61,7 @@
 		"parenb": false,
 		"parmrk": false,
 		"parodd": false,
-		"pendin": true,
+		"pendin": false,
 		"tostop": false,
 		"xcase": false
 	}

(Ignore the Row/Col differences, of course)

/cc @maisem

@bradfitz bradfitz added the ssh Relating to Tailscale SSH https://tailscale.com/blog/tailscale-ssh/ label Mar 11, 2022
@bradfitz
Copy link
Member Author

Even if I stty dump the JSON of a normal OpenSSH session's pty and stty load it in Tailscale session's pty, then start a new GNU Screen session and switch back and forth between two windows where one is Emacs in go-mode showing indented code in color, it gets all corrupted (loses indentation, etc) when switching back to Emacs.

So something beyond just the termios, it seems.

bradfitz added a commit that referenced this issue Mar 11, 2022
Still not sure the exact rules of how/when/who's supposed to set
these, but this works for now on making them match. Baby steps.
Will research more and adjust later.

Updates #4146 (but not enough to fix it, something's still wrong)
Updates #3802

Change-Id: I496d8cd7e31d45fe9ede88fc8894f35dc096de67
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Mar 11, 2022
Still not sure the exact rules of how/when/who's supposed to set
these, but this works for now on making them match. Baby steps.
Will research more and adjust later.

Updates #4146 (but not enough to fix it, something's still wrong)
Updates #3802

Change-Id: I496d8cd7e31d45fe9ede88fc8894f35dc096de67
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Mar 11, 2022
Still not sure the exact rules of how/when/who's supposed to set
these, but this works for now on making them match. Baby steps.
Will research more and adjust later.

Updates #4146 (but not enough to fix it, something's still wrong)
Updates #3802

Change-Id: I496d8cd7e31d45fe9ede88fc8894f35dc096de67
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz
Copy link
Member Author

Well, with #4151 my stty -a output is identical between a normal OpenSSH session and over Tailscale. And environment is near identical too, except in things that don't matter.

And yet....

If I run screen in a session over Tailscale SSH and have two screen Windows: one in bash and one in Emacs with indented code in go-mode in color and I switch back and forth between the windows, Emacs shows all corrupt until I hit Ctrl-L:

Screen Shot 2022-03-11 at 11 51 26 AM

And whenever I switch back & forth between screen windows, it goes corrupt again.

Terminals!!! termcaps!! ptys!!! termios!! rawwrggh!! </vent>

Any clues welcome.

bradfitz added a commit that referenced this issue Mar 11, 2022
Still not sure the exact rules of how/when/who's supposed to set
these, but this works for now on making them match. Baby steps.
Will research more and adjust later.

Updates #4146 (but not enough to fix it, something's still wrong)
Updates #3802

Change-Id: I496d8cd7e31d45fe9ede88fc8894f35dc096de67
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Mar 11, 2022
Still not sure the exact rules of how/when/who's supposed to set
these, but this works for now on making them match. Baby steps.
Will research more and adjust later.

Updates #4146 (but not enough to fix it, something's still wrong)
Updates #3802

Change-Id: I496d8cd7e31d45fe9ede88fc8894f35dc096de67
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz
Copy link
Member Author

@maisem has a change to do my dirty hacky properly: #4154 (it could reference this bug too)

But even with the hacky hard-coded tty settings it currently does, we still see corruption in emacs+vim (even without screen or tmux involved).

On a whim, I tried changing the io.Copy between the tty/pty halves to be ANSI-escape sequence aware to not split a sequence in half between writes, but that didn't help, even if it was doing one write per escape sequence.

@bradfitz
Copy link
Member Author

(Corruption happens in both macOS Terminal and iTerm2.)

@bradfitz
Copy link
Member Author

bradfitz commented Mar 12, 2022

Local hack, I set the termio Clag to 189 (it was 191) to unset bit 2 (whatever that is), and now my stty -g output is identical between OpenSSH and Tailscale:

diff --git a/ssh/tailssh/incubator.go b/ssh/tailssh/incubator.go
index 53ede705..0727190f 100644
--- a/ssh/tailssh/incubator.go
+++ b/ssh/tailssh/incubator.go
@@ -210,12 +210,12 @@ func startWithPTY(cmd *exec.Cmd, ptyReq ssh.Pty) (ptyFile *os.File, err error) {
                        tty.Close()
                }
        }()
-       ptyRawConn, err := ptyFile.SyscallConn()
+       ttyRawConn, err := tty.SyscallConn()
        if err != nil {
                return nil, fmt.Errorf("SyscallConn: %w", err)
        }
        var ctlErr error
-       if err := ptyRawConn.Control(func(fd uintptr) {
+       if err := ttyRawConn.Control(func(fd uintptr) {
                // Load existing PTY settings to modify them & save them back.
                tios, err := termios.GTTY(int(fd))
                if err != nil {
@@ -249,6 +249,18 @@ func startWithPTY(cmd *exec.Cmd, ptyReq ssh.Pty) (ptyFile *os.File, err error) {
                        ctlErr = fmt.Errorf("STTY: %w", err)
                        return
                }
+
+               term, err := unix.IoctlGetTermios(int(fd), unix.TCGETS)
+               if err != nil {
+                       ctlErr = fmt.Errorf("unix.IoctlGetTermios: %w", err)
+                       return
+               }
+               term.Cflag = 189 // *shrug* HACK HACK HACK
+               if err := unix.IoctlSetTermios(int(fd), unix.TCSETS, term); err != nil {
+                       ctlErr = fmt.Errorf("unix.IoctlSetTermios: %w", err)
+                       return
+               }
+
        }); err != nil {
                return nil, fmt.Errorf("ptyRawConn.Control: %w", err)
        }

And now:

$ stty -g
6d00:5:bd:ca1b:3:1c:7f:15:4:0:1:0:11:13:1a:ff:12:f:17:16:ff:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0
$ stty -a
speed 9600 baud; rows 71; columns 167; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = M-^?; eol2 = M-^?; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V;
discard = ^O; min = 1; time = 0;
-parenb -parodd -cmspar cs8 -hupcl -cstopb cread -clocal -crtscts
-ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl ixon -ixoff -iuclc ixany imaxbel iutf8
opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
isig icanon iexten echo echoe -echok -echonl -noflsh -xcase -tostop -echoprt echoctl echoke -flusho -extproc

... everything's identical (-g shows a bit more than -a) between both SSH servers, including most of the environment (no LOGNAME or SSH_TTY set yet; that's in @maisem's pending PR, but doesn't seem to matter for this)

But despite all that matching, Emacs shows corruption only over Tailscale SSH.

@bradfitz
Copy link
Member Author

Ah, @maisem figured it out... gliderlabs/ssh had this hack, which not only do we not need, but actively breaks us when we have the terminal properly configured:

func (sess *session) Write(p []byte) (n int, err error) {
	if sess.pty != nil {
		m := len(p)
		// normalize \n to \r\n when pty is accepted.
		// this is a hardcoded shortcut since we don't support terminal modes.
		p = bytes.Replace(p, []byte{'\n'}, []byte{'\r', '\n'}, -1)
		p = bytes.Replace(p, []byte{'\r', '\r', '\n'}, []byte{'\r', '\n'}, -1)
		n, err = sess.Channel.Write(p)
		if n > m {
			n = m
		}
		return
	}
	return sess.Channel.Write(p)
}

Deleting all that makes it work.

bradfitz added a commit that referenced this issue Mar 12, 2022
Maisem figured out the real problem but will take several commits
(e.g. tailscale/ssh#2) in different repos to get it fixed
properly. This is an interim hack.

Details of real fix:
#4146 (comment)

Updates #4146
Updates #3802

Change-Id: I7b7dc5713baa3e5de75b87b69e7179a6e7549b0b
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue Mar 12, 2022
Maisem figured out the real problem but will take several commits
(e.g. tailscale/ssh#2) in different repos to get it fixed
properly. This is an interim hack.

Details of real fix:
#4146 (comment)

Updates #4146
Updates #3802

Change-Id: I7b7dc5713baa3e5de75b87b69e7179a6e7549b0b
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz
Copy link
Member Author

Didn't mean to close.

@bradfitz bradfitz reopened this Mar 12, 2022
maisem added a commit to maisem/ssh that referenced this issue Mar 13, 2022
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit to maisem/ssh that referenced this issue Mar 13, 2022
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit to maisem/ssh that referenced this issue Mar 13, 2022
… disable it.

Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit to tailscale/ssh that referenced this issue Mar 13, 2022
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit to maisem/ssh that referenced this issue Mar 13, 2022
… disable it.

Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit to tailscale/ssh that referenced this issue Mar 13, 2022
… disable it.

Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
maisem added a commit that referenced this issue Mar 13, 2022
Updates #3802 #4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
@maisem
Copy link
Collaborator

maisem commented Mar 13, 2022

This should be fixed as of #4154

@maisem maisem closed this as completed Mar 13, 2022
mafredri pushed a commit to coder/ssh that referenced this issue Aug 11, 2022
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
mafredri pushed a commit to coder/ssh that referenced this issue Aug 11, 2022
… disable it.

Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
aymanbagabas pushed a commit to aymanbagabas/ssh that referenced this issue Jul 31, 2023
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
aymanbagabas pushed a commit to aymanbagabas/ssh that referenced this issue Jul 31, 2023
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
aymanbagabas added a commit to aymanbagabas/ssh that referenced this issue Jul 31, 2023
Updates tailscale/tailscale#4146

Signed-off-by: Maisem Ali <maisem@tailscale.com>
Co-Authored-By: Ayman Bagabas <ayman.bagabas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ssh Relating to Tailscale SSH https://tailscale.com/blog/tailscale-ssh/
Projects
None yet
Development

No branches or pull requests

2 participants