Add remote port forwarding for Teleport nodes #38828
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Joerger
reviewed
Mar 4, 2024
atburke
force-pushed
the
atburke/rpf-node
branch
from
c828a08
to
269ac1c
Compare
Joerger
approved these changes
Mar 6, 2024
atburke
commented
Mar 7, 2024
espadolini
reviewed
Mar 8, 2024
Comment on lines
+105
to
+96
| if err != nil { | ||
| if !utils.IsOKNetworkError(err) { | ||
| log.WithError(err).Warn("failed to accept connection") | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
The net/http server will check if the error is temporary and if so, it'll continue accepting after a small delay; it's a pattern that I've been using, but I'm not sure if it's actually necessary.
lib/srv/regular/sshserver.go
Outdated
Comment on lines
1275
to
1285
| // Verify that reuse is not enabled on the socket | ||
| if reuseAddr, err := unix.GetsockoptInt(int(listenerFD.Fd()), unix.SOL_SOCKET, unix.SO_REUSEADDR); err != nil { | ||
| return trace.Wrap(err) | ||
| } else if reuseAddr != 0 { | ||
| return trace.AccessDenied("SO_REUSEADDR is enabled on the socket") | ||
| } | ||
| if reusePort, err := unix.GetsockoptInt(int(listenerFD.Fd()), unix.SOL_SOCKET, unix.SO_REUSEPORT); err != nil { | ||
| // Some systems may not support SO_REUSEPORT, so we ignore the error here | ||
| } else if reusePort != 0 { | ||
| return trace.AccessDenied("SO_REUSEPORT is enabled on the socket") | ||
| } |
There was a problem hiding this comment.
Can we check GOOS to see which of those are supposed to be supported?
espadolini
reviewed
Mar 11, 2024
atburke
force-pushed
the
atburke/rpf-node
branch
from
16ed426
to
fe65245
Compare
espadolini
reviewed
Mar 15, 2024
|
|
||
| // remoteForwardingMap holds the remote port forwarding listeners that need | ||
| // to be closed when forwarding finishes, keyed by listen addr. | ||
| remoteForwardingMap utils.SyncMap[string, io.Closer] |
There was a problem hiding this comment.
Is there some extra synchronization that prevents new additions to remoteForwardingMap while the *Server is being closed or is closed?
espadolini
approved these changes
Mar 15, 2024
lib/srv/regular/sshserver.go
Outdated
| _, fn, err := localConn.ReadWithFDs(nil, fbuf) | ||
| if err != nil || fn == 0 { | ||
| fileCh <- nil | ||
| } |
There was a problem hiding this comment.
I'm not entirely sure one can recvmsg() and get data and an error, but we shouldn't risk it:
Suggested change
| _, fn, err := localConn.ReadWithFDs(nil, fbuf) | |
| if err != nil || fn == 0 { | |
| fileCh <- nil | |
| } | |
| if _, fn, _ := localConn.ReadWithFDs(nil, fbuf); fn == 0 { | |
| fileCh <- nil | |
| } |
lib/srv/regular/sshserver.go
Outdated
| if err != nil || fn == 0 { | ||
| fileCh <- nil | ||
| } | ||
| fileCh <- fbuf[0] |
There was a problem hiding this comment.
This might get stuck if the reader is gone.
Suggested change
| fileCh <- fbuf[0] | |
| select { | |
| case fileCh <- fbuf[0]: | |
| case <-proc.Done: | |
| fbuf[0].Close() | |
| } |
public-teleport-github-review-bot
bot
removed request for
tigrato and
rosstimothy
atburke
force-pushed
the
atburke/rpf-node
branch
from
9969ecf
to
fd74e1c
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This change adds support for remote port forwarding (ssh -R) for Teleport nodes.
atburke
force-pushed
the
atburke/rpf-node
branch
from
ff335c6
to
86dc672
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
atburke
added a commit
that referenced
this pull request
Mar 15, 2024
* Add remote port forwarding for Teleport nodes This change adds support for remote port forwarding (ssh -R) for Teleport nodes. * Fix windows build
atburke
added a commit
that referenced
this pull request
Mar 16, 2024
* Add remote port forwarding for Teleport nodes This change adds support for remote port forwarding (ssh -R) for Teleport nodes. * Fix windows build
atburke
added a commit
that referenced
this pull request
Mar 16, 2024
* Add remote port forwarding for Teleport nodes This change adds support for remote port forwarding (ssh -R) for Teleport nodes. * Fix windows build
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds support for remote port forwarding (
ssh -R) for Teleport nodes.Changelog: Added remote port forwarding for Teleport nodes