- Notifications
You must be signed in to change notification settings - Fork928
feat: add support for X11 forwarding#7205
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The dir shouldn't be made until an X11 forwarding requests comes through since most people won't use X11 forwarding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This never gets cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
x11.SingleConnection is not honored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is cool, but could we just shell out toxauth instead to write this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we trust that the xauth command is always present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we should rely onxauth. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would be nice if this also checked that reads and writes were piped properly like other listener tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed. Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't give this a try yet but this is awesome, nice job!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure how this works either actually... I suppose we should flock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added a flock!
| return | ||
| } | ||
| go Bicopy(ctx, conn, channel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we trust that the xauth command is always present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Small stuff but otherwise LGTM!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Adds support for
ssh -X!Fixes#4572