Assist - Execution web endpoint by jakule · Pull Request #25955 · gravitational/teleport

jakule · 2023-05-09T20:12:48Z

This PR implements the execution endpoint used by Teleport Assist. Command passed in the request is being executed on multiple nodes. Nodes are picked based on the query also passed in the request. The command is being executed as non-interactive SSH command, and the result is being recorded, which is a different behavior rather than calling tsh ssh command where the output is not saved.

zmb3

This one is a bit scary to me just because it's not as isolated as the previous PRs, and touches a bunch of critical code (starting sessions, streaming output, etc)

How much testing was done (outside of Assist) to ensure that this does not introduce regressions in other parts of the product?

zmb3 · 2023-05-10T14:26:26Z

Was this always missing or is this new because of the assist changes?

I was never needed as non interactive session was never recorded. This is also related to #21267, which shows that it's not fully reliable.

zmb3 · 2023-05-10T14:40:54Z

Suggested change

func NewWStream(ws WSConn) *WsStream {

func NewWStream(ws WSConn) *WSStream {

If we're going to use WS to mean "websocket" then lets consistently capitalize it.

rosstimothy · 2023-05-10T16:14:38Z

If you use GetAllResources you can drop the type assertions

Suggested change

resources, err := apiclient.GetResourcesWithFilters(ctx, clt, proto.ListResourcesRequest{

resources, err := apiclient.GetAllResources[types.Server](ctx, clt, &proto.ListResourcesRequest{

rosstimothy · 2023-05-10T16:22:36Z

There looks to be some duplicate/overlapping code between this and the TerminalHandler. Can we refactor things so that it only needs to be implemented a single time in lib/web?

I extracted more shared code. Let me know it you're ok with the current state.

r0mant

Don't have too many comments besides what others have already pointed out. If there's a way to reuse more code between command executor and existing web terminal, let's do it.

r0mant · 2023-05-10T17:12:21Z

It's a little strange that this is not exported while everything else is. Are callers outside of this package not supposed to be able to set this?

Callers outside of this package should use AddEnv() method https://github.com/gravitational/teleport/pull/25955/files#diff-9fedd696f2d0af6a43723bf16cadcc57adc8f30ca98833baaf8b560f8f8b8f80R1669

r0mant · 2023-05-10T17:14:51Z

Suggested change

return session.Session{}, err

return session.Session{}, trace.Wrap(err)

r0mant · 2023-05-10T17:37:42Z

Can query be empty? It's not validated here.

The idea behind it was that an empty query returns all nodes, so the client passing an empty query wants to execute a command on all nodes (that they have access to). I'm not aware of how you can create a query asking to do that, so that was kind of a workaround, but the more I think about it more I lean to block this edge case, as someone may, by mistake execute a command on all nodes.

r0mant · 2023-05-10T17:38:31Z

Is this needed?

Suggested change

h.log.WithError(err).Debug("Unable to get auth access point.")

Related to https://github.com/gravitational/teleport/pull/25955/files#r1190324593

r0mant · 2023-05-10T17:38:44Z

Is this needed? The caller should log the error?

Suggested change

h.log.WithError(err).Debug("Unable to fetch cluster networking config.")

Related to https://github.com/gravitational/teleport/pull/25955/files#r1190324593

r0mant · 2023-05-10T17:40:17Z

Why not return the actual error in this case? Otherwise may be confusing why you get "no servers found" without looking in the logs.

Fixed.. kind of.

r0mant · 2023-05-10T17:43:13Z

Do we need to keep logging every error additionally? Caller should log it, no?

The caller does not log it. Our server handler assumes that the caller is an HTTP client, and then the error is returned. Here we've already upgraded the connection to WebSocket, so the client doesn't care about the HTTP responses.

I can remove the log messages, but then there is no trace in the logs of what went wrong. I think we have two options here:

Leave as it is as it matches the implementation of TerminalHandler, for example, https://github.com/gravitational/teleport/blob/6db605fbb854d67aea96398b4e12abccdf61e4f8/lib/web/apiserver.go#L2619

I can clean it up a bit by:

returning error as HTTP response - the client should be able to read those

after the WS connection is created, return any error as a WS message. The client should be able to handle those.
As a side effect, we will have no indication in Teleport logs of what happens, which can be a good and bad thing.

WDYGT @r0mant @zmb3

I vote for #1 for now.

There are a few other related issues where we fail to respond with errors after the WS upgrade completes, I think we should fix them all at once and not make you do it now.

Clean up code a bit

…lers

Add comments

r0mant · 2023-05-11T14:47:28Z

+	"github.com/gravitational/trace"
+)
+
+// WSConn is a gorilla/websocket minimal interface used by our web implementation.


Can you explain in the godoc why do we want to override Close method?

Fix linter

rosstimothy · 2023-05-11T19:04:11Z

 		env[sshutils.SessionEnvVar] = tc.SessionID
 	}
+
+	for key, val := range tc.extraEnvs {


Each of these additional environment variables we set on the session will result in an additional roundtrip between client and target before the session is available.

rosstimothy · 2023-05-11T19:04:32Z

 	SSHSessionID = "SSH_SESSION_ID"
+
+	// EnableNonInteractiveSessionRecording can be used to record non-interactive SSH session.
+	EnableNonInteractiveSessionRecording = "SSH_RECORD_NON_INTERACTIVE"


Should we prefix this with TELEPORT_?

Renamed to SSH_TELEPORT_RECORD_NON_INTERACTIVE to match our other names.

* Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable.

* Assist - Execution web endpoint (#25955) * Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable. * Fixes after merge

* Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable.

* Assist - API implementation (#25810) * Assist - API implementation * Add Assistant CRUD test * Move assistant API to a separate file. * Rename GRPC methods Fix comments * Address PR comments * Update GRPC API - addressing code review comments * Move assist API to a new GRPC service * Add Assistant RBAC rules to this backport. * Add missing license * Update comment. * Move username and ID out of AssistantMessage (#25964) * Move username and ID out of AssistantMessage #25810 added username and conversation ID to AssistantMessage. They aren't needed inside the message and can be moved out of it. This PR also fixes ` grpc: error while marshaling: proto: Marshal called with nil` in `CreateAssistantMessage`. Note: It's safe to change protobuf numbers as this code hasn't been released or backported yet. * Fix test * make grpc * Assist - Configuration and usage (#25953) * Assist - Configuration and usage * Add network config test * Add config test * Run GCI * Address review comments * Assist - OpenAI library port (#25948) * Assist - OpenAI library port * Add tests * Address code review comments * Added comment * Partial backport of #26058 * Move AI messages to a new file * Prevent blocking on error * Add comments. Fix typo * Assist - Execution web endpoint (#25955) * Assist - Execution web endpoint * Add test Clean up code a bit * Add missing username * Address review comments * Make more implementation shared between Terminal and Command Web Handlers * Address review comments * Address review comments * Fixes after rebase Add comments * Add comments Fix linter * Add TELEPORT to Teleport related environment variable. * Assist - Web endpoints (#26046) * Assist - Web endpoints * GCI * proto rpc * enforce endpoint checks * disable in ui if disallowed by auth * add comment * unwrap stack * Refactor code --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> * [Assist] Fix random user selection (#26183) Currently, after selecting a user for command execution in Teleport Assist the user can randomly change. This PR fixes this behavior. * Add rate limiting to Assist (#26011) * Add rate limiting to Assist * Only rate limit Assist in Cloud * Add a comment to assistantLimiter * Fixes after rebase * Add 'rate-limited' test case to assistant_test * Handle CHAT_MESSAGE_UI in Assist web UI * Add godoc * CHAT_MESSAGE_UI -> CHAT_MESSAGE_ERROR * Run assistant test cases in parallel * Rename `ChatGPT` to `GPT-4` (#26272) * Rename `ChatGPT` to `GPT4` ChatGPT is a user friendly name, but is technically inaccurate. * Apply suggestions from code review Co-authored-by: Reed Loden <reed@goteleport.com> * Lint fix * Additional ChatGPT --> OpenAI GPT-4 fixes --------- Co-authored-by: Reed Loden <reed@goteleport.com> * Fix Assist rate-limiting in Cloud (#26342) When Proxy is separate from Auth, Proxy 'modules' will not contain meaningful data. Instead, one must use ClusterFeatures fetched from the Auth server * Assist UI improvements (#26365) * Update assist warning wording, add link to ToS (#26396) * Always render the portal for the assist title to go into (#26733) * Stop using TokenPath for API key in Assist (#26671) * [Assist] MFA support (#26719) * Initial support for MFA in Assist * UI webauth handler * WebUI - WIP * Run prettier * Perform MFA ceremony only once. * Cleanup JS * Remove hacky WS logic * Add cancel MFA logic * [Assist] Prevent creating messages without conversation (#26797) * Add parallel execution to assist. (#26563) * Add parallel execution to assist. * Extract execution logic to a new function. * Add test * Switch uber to std * Address code review comments * Fix display of Assist command executions with empty output (#27010) * Fix display of Assist executions with empty output * Lint * Nit * Improve display of commands that failed with code * Address lint * [Assist] Allow removing assist conversations (#26788) * [Assist] Allow removing assist conversations * Display landing page after the conversion is removed * Improve styling and add a confirmation dialog * Change the icon opacity to copy the main navigation * Remove unused minus icon * Add missing trace.wrap --------- Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> * Backport fixes * Regenerate go.sum to make the linter happy. * Sync files after rebase * After rebase fixes --------- Co-authored-by: Joel Wejdenstål <jwejdenstal@icloud.com> Co-authored-by: Justinas Stankevičius <justinas@users.noreply.github.com> Co-authored-by: Mike Jensen <jentfoo@users.noreply.github.com> Co-authored-by: Reed Loden <reed@goteleport.com> Co-authored-by: Ryan Clark <ryan.clark@goteleport.com>

jakule force-pushed the jakule/ai-execute-port branch 2 times, most recently from c4865ff to 7739c27 Compare May 9, 2023 22:13

jakule marked this pull request as ready for review May 9, 2023 23:22

jakule added the assist label May 9, 2023

github-actions Bot requested review from nklaassen and rudream May 9, 2023 23:23

github-actions Bot added the size/lg label May 9, 2023

jakule requested review from r0mant and zmb3 May 9, 2023 23:23

jakule self-assigned this May 10, 2023

zmb3 reviewed May 10, 2023

View reviewed changes

rosstimothy reviewed May 10, 2023

View reviewed changes

r0mant reviewed May 10, 2023

View reviewed changes

jakule added 8 commits May 10, 2023 23:59

Assist - Execution web endpoint

075d118

Add test

93efabe

Clean up code a bit

Add missing username

a103e25

Address review comments

b1e8a02

Make more implementation shared between Terminal and Command Web Hand…

0294f3d

…lers

Address review comments

7ce9911

Address review comments

5da08f2

Fixes after rebase

08cac88

Add comments

jakule force-pushed the jakule/ai-execute-port branch from 323fb10 to 08cac88 Compare May 11, 2023 04:05

jakule requested review from r0mant, rosstimothy and zmb3 May 11, 2023 04:05

r0mant approved these changes May 11, 2023

View reviewed changes

zmb3 approved these changes May 11, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed request for nklaassen and rudream May 11, 2023 17:38

Add comments

df1fcb3

Fix linter

jakule added this pull request to the merge queue May 11, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 11, 2023

rosstimothy reviewed May 11, 2023

View reviewed changes

Add TELEPORT to Teleport related environment variable.

0bc173d

jakule enabled auto-merge May 11, 2023 20:09

jakule added this pull request to the merge queue May 11, 2023

Merged via the queue into master with commit 9a89404 May 11, 2023

jakule deleted the jakule/ai-execute-port branch May 11, 2023 20:37

jakule mentioned this pull request May 11, 2023

[v12] Assist - Execution web endpoint #26111

Merged

	func NewWStream(ws WSConn) *WsStream {
	func NewWStream(ws WSConn) *WSStream {

	resources, err := apiclient.GetResourcesWithFilters(ctx, clt, proto.ListResourcesRequest{
	resources, err := apiclient.GetAllResources[types.Server](ctx, clt, &proto.ListResourcesRequest{

	return session.Session{}, err
	return session.Session{}, trace.Wrap(err)

Conversation

jakule commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakule commented May 9, 2023 •

edited

Loading