-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow reverse tunnel join without exposing the web API #13598
Conversation
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.
Looks good.
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.
Looks reasonable to me.
I'm not super familiar with the init functions and the complexity over at some funcs, like setupProxyListeners, is pretty high (not that this is your fault!). Because of that I think we should ask someone like @r0mant to take a look as well.
Apologies for the delay 🙏
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.
LGTM, barring that one small comment.
@ptgott Mind looking at the docs again when you have a moment? |
Service through the Proxy. | ||
to talk to port `3024` on the Proxy Service, which is used to both fetch the | ||
credentials (SSH and TLS certificates) and establish a connection to the Auth | ||
Service through the Proxy. Alternatively, port `3080` can be used to fetch |
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.
Also, if there's no difference between using port 3024 and 3080, we could probably remove the last sentence of this paragraph and say, "...the Node needs to be able to talk to either port 3024
or 3080
on the Proxy Service...".
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.
3024 is used in both cases to establish the reverse tunnel connection; it's just a question of whether the node fetches credentials over 3024 or 3080. It would have to be something like "either port 3024
or 3024
AND 3080
", and I'm not sure how to word that elegantly.
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.
How does the new behavior work with TLS multiplexing? This paragraph (and the ones after it) seem to assume that TLS multiplexing is disabled by default, while in all supported Teleport versions now (v8 and above), this is not the case.
In any case, I think this paragraph would be much clearer by:
-
Replacing specific port numbers with references to Proxy Service configuration settings. Otherwise, the text gets a bit muddled, since we have to go back and forth between port numbers and their usage. If readers need to know the exact ports to use, we could include a command to hit the
/webapi/ping
endpoint. -
Rephrasing "can be used" in the last sentence of the paragraph to something more specific. I'm not sure if 3080 only needs to be open on the Proxy Service or if there's some specific config setting/flag/env variable the user needs to change from something else to 3080.
@atburke - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes. |
@codingllama @strideynet I've fixed the last bugs. Feel free to make another review pass when you can. |
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.
Just a few thoughts, once these are addressed/discussed I'll approve.
@codingllama @strideynet Can I get a review on 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.
@atburke, please sync with master and solve the conflicts.
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 happy to approve this as the majority of this looks good, would be nice to iron out a decision on returning vs warning on errors for the listeners/muxers though. I think that we probably aren't handling this very well in the existing codebase so it might be nice to make a decision and then apply it retrospectively to all the handlers in another PR.
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.
Please address the remaining comment, otherwise LGTM.
This change allows agents to join over a reverse tunnel (port 3024 by default) only, instead of also requiring access to the web API (port 3080).
This PR allows agents to join over a reverse tunnel (port 3024 by default) only, instead of also requiring access to the web API (port 3080). This is accomplished by multiplexing a small subset of the web API (specifically,
/webapi/find
and/webapi/host/credentials
) over port 3024 when the web API and reverse tunnel aren't already being multiplexed.Resolves #12730.