feat: add support for X11 forwarding #7205
Conversation
agent/agentssh/agentssh.go
Outdated
| if x11SocketDir == "" { | ||
| x11SocketDir = filepath.Join(os.TempDir(), ".X11-unix") | ||
| } | ||
| err = fs.MkdirAll(x11SocketDir, 0o700) |
There was a problem hiding this comment.
The dir shouldn't be made until an X11 forwarding requests comes through since most people won't use X11 forwarding.
agent/agentssh/x11.go
Outdated
| return false | ||
| } | ||
|
|
||
| err = addXauthEntry(fs, hostname, strconv.Itoa(int(x11.ScreenNumber)), x11.AuthProtocol, x11.AuthCookie) |
There was a problem hiding this comment.
This never gets cleaned up
There was a problem hiding this comment.
It doesn't with normal X either, so I don't think we should
| defer s.trackListener(listener, false) | ||
|
|
||
| for { | ||
| conn, err := listener.Accept() |
There was a problem hiding this comment.
x11.SingleConnection is not honored
agent/agentssh/x11.go
Outdated
|
|
||
| // addXauthEntry adds an Xauthority entry to the Xauthority file. | ||
| // The Xauthority file is located at ~/.Xauthority. | ||
| func addXauthEntry(fs afero.Fs, host string, display string, authProtocol string, authCookie string) error { |
There was a problem hiding this comment.
This is cool, but could we just shell out to xauth instead to write this?
There was a problem hiding this comment.
Can we trust that the xauth command is always present?
There was a problem hiding this comment.
I don't think we should rely on xauth. The spec is extremely stable and seems unlikely to change anytime soon.
| }, testutil.WaitShort, testutil.IntervalFast) | ||
|
|
||
| x11 := <-x11Chans | ||
| ch, reqs, err := x11.Accept() |
There was a problem hiding this comment.
It would be nice if this also checked that reads and writes were piped properly like other listener tests.
agent/agentssh/x11.go
Outdated
| return false | ||
| } | ||
|
|
||
| err = addXauthEntry(fs, hostname, strconv.Itoa(int(x11.ScreenNumber)), x11.AuthProtocol, x11.AuthCookie) |
There was a problem hiding this comment.
From the RFC:
It is RECOMMENDED that the 'x11 authentication cookie' that is sent
be a fake, random cookie, and that the cookie be checked and replaced
by the real cookie when a connection request is received.
Is this something we should do?
There was a problem hiding this comment.
I considered commenting on this but the amount of complexity this adds is pretty immense, since you'd have to intercept the stream, validate the cookie and then reauthenticate the stream before you start copying the streams.
There was a problem hiding this comment.
I think it's not worth the complexity right now. The auth cookie doesn't do much these days, because we get encryption with SSH.
| xauthPath := filepath.Join(homeDir, ".Xauthority") | ||
|
|
||
| // Open or create the Xauthority file | ||
| file, err := fs.OpenFile(xauthPath, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0o600) |
There was a problem hiding this comment.
Should we consider flock-ing the file so we're the only one writing to it? Not sure if xauth respects that though.
How about multiple SSH connections with X11 forwarding?
There was a problem hiding this comment.
I'm not sure how this works either actually... I suppose we should flock?
| return | ||
| } | ||
|
|
||
| go Bicopy(ctx, conn, channel) |
There was a problem hiding this comment.
We might want to track that conn and channel are actually closed by the time (*Server).Close completes. Or essentially, that bicopy has exited, but I think we can keep it like this unless we start seeing goleak errors in the tests.
agent/agentssh/x11.go
Outdated
|
|
||
| // addXauthEntry adds an Xauthority entry to the Xauthority file. | ||
| // The Xauthority file is located at ~/.Xauthority. | ||
| func addXauthEntry(fs afero.Fs, host string, display string, authProtocol string, authCookie string) error { |
There was a problem hiding this comment.
Can we trust that the xauth command is always present?
Adds support for
ssh -X!Fixes #4572