Allow tunnel nodes to enable the ssh listener#53473
Merged
rosstimothy merged 1 commit intomasterfrom Apr 22, 2025
Merged
Conversation
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
2 times, most recently
from
8add08a to
7fa96e7
Compare
rosstimothy
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
from
7fa96e7 to
6d23b17
Compare
Contributor
|
Amplify deployment status
|
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
from
6d23b17 to
224b603
Compare
rosstimothy
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
github-actions
Bot
requested review from
Joerger,
mmcallister,
ptgott,
r0mant,
strideynet and
zmb3
fspmarshall
approved these changes
Apr 15, 2025
Comment on lines
+3223
to
+3210
| if !useLocalListener { | ||
| // Start the SSH server. This kicks off updating labels and starting the | ||
| // heartbeat. | ||
| if err := s.Start(); err != nil { |
Contributor
There was a problem hiding this comment.
nit: this conditional start reads a bit funny when scanning this logic. Might be worth an explanatory comment. Ex:
// if local listener setup wasn't run, the server still needs to be started
Contributor
There was a problem hiding this comment.
I'd consider making it even more explicit in the code and setting a flag for serverStarted to true right before the go s.Serve(listener) and checking for conn.UseTunnel() && !serverStarted here.
Contributor
Author
There was a problem hiding this comment.
I moved this back into the else clause to reduce confusion.
Joerger
approved these changes
Apr 15, 2025
public-teleport-github-review-bot
Bot
removed request for
espadolini,
mmcallister,
ptgott,
r0mant,
strideynet and
zmb3
espadolini
reviewed
Apr 15, 2025
Comment on lines
+3222
to
+3207
| if conn.UseTunnel() { | ||
| if !useLocalListener { |
Contributor
There was a problem hiding this comment.
Suggested change
| if conn.UseTunnel() { | |
| if !useLocalListener { | |
| if conn.UseTunnel() && !useLocalListener { |
Comment on lines
+9203
to
+9206
| func testForceListenerInTunnelMode(t *testing.T, suite *integrationTestSuite) { | ||
| // InsecureDevMode needed for IoT node handshake | ||
| lib.SetInsecureDevMode(true) | ||
| defer lib.SetInsecureDevMode(false) |
Contributor
There was a problem hiding this comment.
I'm always scared of global state manipulation in tests.
Suggested change
| func testForceListenerInTunnelMode(t *testing.T, suite *integrationTestSuite) { | |
| // InsecureDevMode needed for IoT node handshake | |
| lib.SetInsecureDevMode(true) | |
| defer lib.SetInsecureDevMode(false) | |
| func testForceListenerInTunnelMode(t *testing.T, suite *integrationTestSuite) { | |
| // InsecureDevMode needed for IoT node handshake | |
| defer lib.SetInsecureDevMode(lib.IsInsecureDevMode()) | |
| lib.SetInsecureDevMode(true) | |
| // fail the test if it's accidentally made parallel | |
| t.SetEnv("_testForceListenerInTunnelMode_SetInsecureDevMode", "1") |
Comment on lines
+3223
to
+3210
| if !useLocalListener { | ||
| // Start the SSH server. This kicks off updating labels and starting the | ||
| // heartbeat. | ||
| if err := s.Start(); err != nil { |
Contributor
There was a problem hiding this comment.
I'd consider making it even more explicit in the code and setting a flag for serverStarted to true right before the go s.Serve(listener) and checking for conn.UseTunnel() && !serverStarted here.
espadolini
approved these changes
Apr 15, 2025
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
from
224b603 to
540d53f
Compare
rosstimothy
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
from
540d53f to
9fb28d3
Compare
rosstimothy
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
This allows the service to be connectable by users with direct network access. All connections still require a valid user certificate to be presented and will not permit any extra access. This is intended to provide an optional connection path to hosts that may provide reduced latency if the Proxy is not co-located with the user and service.
rosstimothy
force-pushed
the
tross/tunnel_direct_node
branch
from
9fb28d3 to
e6bf732
Compare
rosstimothy
temporarily deployed
to
docs-amplify
GitHub Actions
Inactive
Contributor
|
@rosstimothy See the table below for backport results.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 allows the service to be connectable by users with direct network access. All connections still require a valid user
certificate to be presented and will not permit any extra access. This is intended to provide an optional connection path to hosts that may provide reduced latency if the Proxy is not co-located with the user and service.
Changelog: Allow the
ssh_service.listen_addrto forcibly be enabled when operating in reverse tunnel mode to provide an optional direct access path to hosts.