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

Add a dedicated client to communicate with the Proxy SSH servers #22629

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Add a dedicated client to communicate with the Proxy SSH servers #22629

merged 1 commit into from
Mar 15, 2023

Conversation

rosstimothy
Copy link
Contributor

A new api/client/proxy/Client has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server - this ensures backwards
compatibility.

Much of the SSH functionality mimics the existing behavior of the
ProxyClient in lib/client. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates lib/client to make use of the new client introduced
here.

Part of #19812

Note: The first commit moves api/client/proxy/proxy.go to api/utils.proxy.go in order to avoid circular dependencies.

@rosstimothy rosstimothy force-pushed the tross/api_proxy_client branch 5 times, most recently from cf77fa6 to 9e091f4 Compare March 7, 2023 14:11
@rosstimothy rosstimothy changed the title Add a dedicated client to communicate with the Proxy SSH server Add a dedicated client to communicate with the Proxy SSH servers Mar 7, 2023
@rosstimothy rosstimothy marked this pull request as ready for review March 7, 2023 14:52
@github-actions github-actions bot added size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Mar 7, 2023
@github-actions github-actions bot requested review from gzdunek and timothyb89 March 7, 2023 14:52
gzdunek
gzdunek approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Approved, but I'm not familiar with this part of the codebase, so you may want to request a review from someone else from group 2.

@rosstimothy rosstimothy force-pushed the tross/api_proxy_client branch from 9e091f4 to 1a47200 Compare March 10, 2023 20:02
@rosstimothy
Copy link
Contributor Author

Friendly ping @timothyb89

@rosstimothy
Copy link
Contributor Author

@timothyb89 PTAL?

timothyb89
timothyb89 approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@timothyb89 timothyb89 left a comment

Choose a reason for hiding this comment

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

This looks reasonable. I'm excited to see this in tsh ssh!

api/client/proxy/client.go
Comment on lines +471 to +477
if err != nil && !strings.Contains(err.Error(), "agent: already have handler for") {
return nil, trace.Wrap(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this error is safe to ignore? Might be worth a quick comment since we usually bubble it up (aside from client.ConnectToNode() at least)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The agent: already have handler for errors are safe to ignore as they indicate we're already handling the auth-agent@openssh.com channel so the agent forwarding should still function. I'll add a comment with some details.

@rosstimothy
Add a dedicated client to communicate with the Proxy SSH server
f97c312 
A new `api/client/proxy/Client` has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server.

Much of the SSH functionality mimics the existing behavior of the
`ProxyClient` in `lib/client`. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates `lib/client` to make use of the new client instead.

Part of #19812
@rosstimothy rosstimothy force-pushed the tross/api_proxy_client branch from d37f2eb to f97c312 Compare March 15, 2023 13:16
@rosstimothy rosstimothy enabled auto-merge March 15, 2023 13:17
@rosstimothy rosstimothy added this pull request to the merge queue Mar 15, 2023
Merged via the queue into master with commit 1c3188a Mar 15, 2023
@rosstimothy rosstimothy deleted the tross/api_proxy_client branch March 20, 2023 16:58
@rosstimothy rosstimothy mentioned this pull request Apr 18, 2023
Merged
@rosstimothy rosstimothy mentioned this pull request Apr 18, 2023
Merged
rosstimothy added a commit that referenced this pull request Apr 18, 2023
@rosstimothy
Add a dedicated client to communicate with the Proxy SSH server (#22629)
9f64812 
A new `api/client/proxy/Client` has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server.

Much of the SSH functionality mimics the existing behavior of the
`ProxyClient` in `lib/client`. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates `lib/client` to make use of the new client instead.

Part of #19812
rosstimothy added a commit that referenced this pull request Apr 18, 2023
@rosstimothy
Add a dedicated client to communicate with the Proxy SSH server (#22629)
eaa4ffc 
A new `api/client/proxy/Client` has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server.

Much of the SSH functionality mimics the existing behavior of the
`ProxyClient` in `lib/client`. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates `lib/client` to make use of the new client instead.

Part of #19812
rosstimothy added a commit that referenced this pull request Apr 21, 2023
@rosstimothy
[v11] Proxy Client (#24737)
92a9ab1 
* Add a dedicated client to communicate with the Proxy SSH server (#22629)

A new `api/client/proxy/Client` has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server.

Much of the SSH functionality mimics the existing behavior of the
`ProxyClient` in `lib/client`. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates `lib/client` to make use of the new client instead.

Part of #19812

* Make `proxy.Client` infer the cluster name from Proxy (#23644)

Instead of relying on users to provide the cluster name, the client
now determines the cluster name by inspecting the certificate
presented by the Proxy during the TLS or SSH handshake. This is
required when connecting to a Proxy via a jump host since the
name of the cluster may not match the currently logged in cluster.

This is achieved by leveraging a custom `credentials.TransportCredentials`
when connecting via gRPC and a custom `ssh.HostKeyCallback` when
connecting SSH.
rosstimothy added a commit that referenced this pull request Apr 21, 2023
@rosstimothy
[v12] Proxy Client (#24734)
935c2aa 
* Add a dedicated client to communicate with the Proxy SSH server (#22629)

A new `api/client/proxy/Client` has been added to interact with
the SSH and gRPC servers that the Proxy serves on its SSH port.
The client will first try connecting to the gRPC server and if
that fails it will fall back to the SSH server.

Much of the SSH functionality mimics the existing behavior of the
`ProxyClient` in `lib/client`. This is the first part of phasing
out that client in favor of the new client. There will be a follow
up PR that migrates `lib/client` to make use of the new client instead.

Part of #19812

* Make `proxy.Client` infer the cluster name from Proxy (#23644)

Instead of relying on users to provide the cluster name, the client
now determines the cluster name by inspecting the certificate
presented by the Proxy during the TLS or SSH handshake. This is
required when connecting to a Proxy via a jump host since the
name of the cluster may not match the currently logged in cluster.

This is achieved by leveraging a custom `credentials.TransportCredentials`
when connecting via gRPC and a custom `ssh.HostKeyCallback` when
connecting SSH.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothyb89 timothyb89 timothyb89 approved these changes

@gzdunek gzdunek gzdunek approved these changes

Assignees
No one assigned
Labels
size/lg 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.
3 participants
@rosstimothy @timothyb89 @gzdunek