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.

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

Adds concurrent default-port detection to tsh #6374

Merged
merged 14 commits into from
May 14, 2021
Merged

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented Apr 9, 2021

Addresses issue #4924

If a default Web Proxy port is not specified by the user, either via
config or on the command line, tsh defaults to 3080. Unfortunately
3080 is often blocked by firewalls, leading to an unacceptably long
timeout for the user.

This change adds an RFC8305-like default-port selection algorithm,
that will try multiple ports on the supplied host concurrently and
select the most reponsive address to use for Web Proxy traffic. I
have included the standard HTTPS port (443) in the defaulut set,
and this can be easily expanded if other good candidates come along.

If the port selection fails for any reason, tsh reverts to the
legacy behaviour of picking 3080 automatically.

@russjones
Copy link
Contributor

@fspmarshall @xacrimon Can you two review?

lib/client/api.go Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

lib/client/api.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr_test.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
lib/defaults/defaults.go Outdated Show resolved Hide resolved
@russjones russjones self-requested a review April 20, 2021 01:24
tool/tsh/tsh.go Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr_test.go Outdated Show resolved Hide resolved
lib/client/api.go Outdated Show resolved Hide resolved
tool/tsh/tsh.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/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
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
lib/client/api.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.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/api.go Outdated Show resolved Hide resolved
@tcsc tcsc requested review from r0mant and nklaassen April 21, 2021 02:28
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
lib/client/api.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

A few more comments but should be good to go once they're addressed.

lib/client/api.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
tcsc added 7 commits May 4, 2021 18:02
Addresses issue #4924

If a default Web Proxy port is not specified by the user, either via
config or on the command line, `tsh` defaults to `3080`. Unfortunately
`3080` is often blocked by firewalls, leading to an unacceptably long
timeout for the user.

This change adds an RFC8305-like default-port selection algorithm,
that will try multiple ports on the supplied host concurrently and
select the most reponsive address to use for Web Proxy traffic. I
have included the standard HTTPS port (443) in the defaulut set,
and this can be easily expanded if other good candidates come along.

If the port selection fails for any reason, `tsh` reverts to the
legacy behaviour of picking `3080` automatically.
func raceRequest(ctx context.Context, cli *http.Client, addr string, waitgroup *sync.WaitGroup, results chan<- raceResult) {
defer waitgroup.Done()

target := fmt.Sprintf("https://%s/webapi/ping", addr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noticing this just now - proxy can run in insecure mode so https:// won't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the solution to that? I could spawn separate http & https racers I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... although the webclient is just going to plug whtever we find straight into an https URL anyway. See:

func Ping(ctx context.Context, proxyAddr string, insecure bool, pool *x509.CertPool, connectorName string) (*PingResponse, error) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm this is a good point. I'm only superficially familiar with --insecure-no-tls flag but I wonder if it exists solely for scenarios where proxy runs behind a TLS-terminating load balancer so the actual public proxy endpoint clients connect to is still TLS (i.e. proxy in "insecure no TLS" mode is not supposed to be connected to directly). If that's the case, then it's ok to just use https. @webvictim or @russjones maybe, do you folks know?

Copy link
Contributor

Choose a reason for hiding this comment

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

Your reasoning is correct - --insecure-no-tls disables the TLS part of Teleport's web listener and just makes it speak plain HTTP, but is only intended to be used for backend -> Teleport connections. tsh and Teleport agents joining the cluster will still be expecting to communicate over HTTPS - it's just that the HTTPS will be terminated in front of Teleport rather than inside it.

While we're on the topic, though, it's worth checking how the --insecure flag to tsh and Teleport agents works as that skips all TLS verification, but I'm unsure whether it also allows the use of an HTTP connection rather than HTTPS when combined with --insecure-no-tls on the web listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as tsh is concerned, it blithely assumes https everywhere that I can see. There are at least half a dozen places in the webclient that have constants that start "https://".

tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Show resolved Hide resolved
tool/tsh/resolve_default_addr.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

LGTM once the https question is resolved

lib/client/api.go Outdated Show resolved Hide resolved
@russjones
Copy link
Contributor

@tcsc Is this ready to merge or are the test failures real?

@russjones russjones merged commit 1d0dd97 into master May 14, 2021
@russjones russjones deleted the trent/happy-eyes branch May 14, 2021 23:09
@russjones russjones mentioned this pull request May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants