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

x11 forwarding #9897

Merged
merged 31 commits into from
Feb 4, 2022
Merged

x11 forwarding #9897

merged 31 commits into from
Feb 4, 2022

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Jan 22, 2022 •
edited
Loading

This PR implements RFD

Closes #7799

@github-actions github-actions bot requested review from jakule and zmb3 January 22, 2022 04:03
@github-actions github-actions bot added audit-log Issues related to Teleports Audit Log tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Jan 22, 2022
zmb3
zmb3 reviewed Jan 24, 2022
View reviewed changes
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good but very few tests for the amount of code being added. Can we improve coverage a bit?

lib/client/api.go Outdated Show resolved Hide resolved
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/session.go Outdated Show resolved Hide resolved
lib/client/session.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Show resolved Hide resolved
tool/tsh/options.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
jakule
jakule reviewed Jan 26, 2022
View reviewed changes
lib/client/api.go Outdated Show resolved Hide resolved
lib/client/session.go Outdated Show resolved Hide resolved
lib/srv/reexec.go Show resolved Hide resolved
lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
lib/sshutils/forward.go Outdated Show resolved Hide resolved
lib/sshutils/x11/auth.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Outdated Show resolved Hide resolved
lib/sshutils/x11/forward.go Outdated Show resolved Hide resolved
@Joerger Joerger mentioned this pull request Jan 28, 2022
Merged
@Joerger Joerger force-pushed the joerger/x11-forwarding branch from 029e8b9 to 4e6cb6d Compare January 28, 2022 20:33
@Joerger Joerger requested review from zmb3 and jakule February 1, 2022 00:50
@Joerger
Copy link
Contributor Author

Joerger commented Feb 1, 2022 •
edited
Loading

@zmb3 @jakule PTAL, I addressed all of the comments except for the one about non-graceful shutdowns. I'm going to open another Issue for that bug fix since it's a separate issue.

zmb3
zmb3 approved these changes Feb 2, 2022
View reviewed changes
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A couple minor points, but nothing that should hold up a merge.

lib/srv/ctx.go Outdated Show resolved Hide resolved
lib/srv/reexec.go Outdated Show resolved Hide resolved
lib/sshutils/x11/conn.go Outdated Show resolved Hide resolved
lib/sshutils/x11/display.go Outdated Show resolved Hide resolved
lib/sshutils/x11/display.go Show resolved Hide resolved
jakule
jakule reviewed Feb 3, 2022
View reviewed changes
lib/srv/regular/sshserver_test.go Outdated Show resolved Hide resolved
lib/srv/regular/sshserver_test.go Outdated Show resolved Hide resolved
lib/srv/regular/sshserver_test.go Outdated Show resolved Hide resolved
lib/sshutils/x11/conn.go Outdated Show resolved Hide resolved
tool/tsh/options.go Outdated Show resolved Hide resolved
@jakule
Copy link
Contributor

jakule commented Feb 3, 2022

Looks like one X11 test is currently falling in CI:

Step #1 - "run-tests": FAIL: github.com/gravitational/teleport/lib/srv/app
Step #1 - "run-tests": FAIL: github.com/gravitational/teleport/lib/srv/app.TestHandleConnection
Step #1 - "run-tests": FAIL: github.com/gravitational/teleport/lib/srv/regular
Step #1 - "run-tests": FAIL: github.com/gravitational/teleport/lib/srv/regular.TestX11Forward

@Joerger Joerger force-pushed the joerger/x11-forwarding branch 9 times, most recently from 9a70827 to 52c3d3c Compare February 4, 2022 19:01
@Joerger Joerger mentioned this pull request Feb 4, 2022
Merged
@Joerger Joerger force-pushed the joerger/x11-forwarding branch 2 times, most recently from d08c0d3 to 3fbfcdf Compare February 4, 2022 21:10
@Joerger Joerger requested a review from jakule February 4, 2022 21:24
@Joerger Joerger enabled auto-merge (squash) February 4, 2022 22:28
Joerger added 18 commits February 4, 2022 14:37
@Joerger
Clean up.
fc88b02
@Joerger
Resolve PR comments and apply suggestions;
4dc4f33 
Add XConn and XDisplay abstractions;

Add tests in x11 package and sshserver;

Cleanup/fix forwarding logic;

Cleanup comments, fix X capitalization.
@Joerger
Fix ForwardX11Timeout functionality;
93ff2cb 
Simplify X11 forwarding cli flags and options;

Make XServer proxy display max configurable;

Move and improve SSH options tests.
@Joerger
Add x11-untrusted-timeout flag.
e01d0b3
@Joerger
Update UX to match updated RFD.
1c12e95
@Joerger
Fix re-exec chown syscall; Fix sshserver test.
07453b7
@Joerger
Handle X Server listener limit exceeded error.
dba63c8
@Joerger
Remove todo.
564ae1b
@Joerger
Cleanup.
a1a7678
@Joerger
Fix linter and fileconf tests.
6146215
@Joerger
Check for EADDRINUSE when opening XServer unix socket.
8e97f18
@Joerger
Address comments.
32a6eb3
@Joerger
Fix CI error.
ed31f03
@Joerger
Make suggested changes.
c028a5f
@Joerger
Continue session when forwarded x11 request denied; Print log to user…
2a53fa2 
… when x11-req is denied.
@Joerger
Add debug log.
2fcafb8 
Fix broken test.

Check for xauth in tests.

Add xauth to buildbox.
@Joerger
Add xauth tests; Disable xauth tests unless requested.
23cf2e0
@Joerger
Add todo for x11 test improvments.
39be324
@Joerger Joerger force-pushed the joerger/x11-forwarding branch from 6d9e026 to 39be324 Compare February 4, 2022 22:38
@Joerger Joerger merged commit d33f51d into master Feb 4, 2022
@Joerger Joerger deleted the joerger/x11-forwarding branch February 4, 2022 23:47
@Joerger Joerger mentioned this pull request Feb 4, 2022
Merged
Joerger added a commit that referenced this pull request Feb 5, 2022
@Joerger
x11 forwarding (#9897)
07a9be9
russjones pushed a commit that referenced this pull request Feb 9, 2022
@Joerger @russjones
x11 forwarding (#9897)
5db49b5
@Richard-Barrett Richard-Barrett mentioned this pull request Feb 14, 2022
Closed
@webvictim webvictim mentioned this pull request Mar 4, 2022
Closed
@alexatcanva alexatcanva mentioned this pull request Oct 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmb3 zmb3 zmb3 approved these changes

@jakule jakule jakule approved these changes

@russjones russjones russjones approved these changes

Assignees
No one assigned
Labels
audit-log Issues related to Teleports Audit Log tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

X11 Forwarding support for Teleport nodes
4 participants
@Joerger @jakule @russjones @zmb3