From 72a1bd23fd243e51c3d5e18c37e044c75f963f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Fri, 29 Mar 2024 16:35:52 +0100 Subject: [PATCH 1/2] Use dbcmd.WithPrintFormat only for preview of teleterm gateway command --- .../go/teleport/lib/teleterm/v1/gateway.pb.go | 13 +++-- integration/proxy/teleterm_test.go | 2 +- lib/client/db/dbcmd/dbcmd.go | 2 +- lib/client/db/dbcmd/dbcmd_test.go | 2 + lib/teleterm/cmd/cmds/cmds.go | 33 ++++++++++++ lib/teleterm/cmd/db.go | 36 +++++++++---- lib/teleterm/cmd/db_test.go | 52 ++++++++++++++++--- lib/teleterm/cmd/kube.go | 7 +-- lib/teleterm/cmd/kube_test.go | 2 +- lib/teleterm/gateway/base.go | 24 ++++----- lib/teleterm/gateway/base_test.go | 7 +-- proto/teleport/lib/teleterm/v1/gateway.proto | 13 +++-- 12 files changed, 151 insertions(+), 42 deletions(-) create mode 100644 lib/teleterm/cmd/cmds/cmds.go diff --git a/gen/proto/go/teleport/lib/teleterm/v1/gateway.pb.go b/gen/proto/go/teleport/lib/teleterm/v1/gateway.pb.go index 2e2c767e9155e..ec969bbc5e140 100644 --- a/gen/proto/go/teleport/lib/teleterm/v1/gateway.pb.go +++ b/gen/proto/go/teleport/lib/teleterm/v1/gateway.pb.go @@ -168,8 +168,9 @@ func (x *Gateway) GetGatewayCliCommand() *GatewayCLICommand { return nil } -// GatewayCLICommand represents a command that the user can execute to connect to the gateway -// resource. It is a direct translation of os.exec.Cmd. +// GatewayCLICommand represents a command that the user can execute to connect to a gateway +// resource. It is a combination of two different os/exec.Cmd structs, where path, args and env are +// directly taken from one Cmd and the preview field is constructed from another Cmd. type GatewayCLICommand struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache @@ -179,10 +180,16 @@ type GatewayCLICommand struct { Args []string `protobuf:"bytes,2,rep,name=args,proto3" json:"args,omitempty"` Env []string `protobuf:"bytes,3,rep,name=env,proto3" json:"env,omitempty"` // preview is used to show the user what command will be executed before they decide to run it. - // It's like os.exec.Cmd.String with two exceptions: + // It can also be copied and then pasted into a terminal. + // It's like os/exec.Cmd.String with two exceptions: // // 1) It is prepended with Cmd.Env. // 2) The command name is relative and not absolute. + // 3) It is taken from a different Cmd than the other fields in this message. This Cmd uses a + // special print format which makes the args suitable to be entered into a terminal, but not to + // directly spawn a process. + // + // Should not be used to execute the command in the shell. Instead, use path, args, and env. Preview string `protobuf:"bytes,4,opt,name=preview,proto3" json:"preview,omitempty"` } diff --git a/integration/proxy/teleterm_test.go b/integration/proxy/teleterm_test.go index 7f72bbbaf13fc..ea357ff3db7fd 100644 --- a/integration/proxy/teleterm_test.go +++ b/integration/proxy/teleterm_test.go @@ -372,5 +372,5 @@ func checkKubeconfigPathInCommandEnv(t *testing.T, gw gateway.Gateway, wantKubec cmd, err := gw.CLICommand() require.NoError(t, err) - require.Equal(t, cmd.Env, []string{"KUBECONFIG=" + wantKubeconfigPath}) + require.Equal(t, []string{"KUBECONFIG=" + wantKubeconfigPath}, cmd.Env) } diff --git a/lib/client/db/dbcmd/dbcmd.go b/lib/client/db/dbcmd/dbcmd.go index 08f1811d19885..5b1d913601762 100644 --- a/lib/client/db/dbcmd/dbcmd.go +++ b/lib/client/db/dbcmd/dbcmd.go @@ -861,7 +861,7 @@ func WithPassword(pass string) ConnectCommandFunc { // WithPrintFormat is known to be used for the following situations: // - tsh db config --format cmd // - tsh proxy db --tunnel -// - Teleport Connect where the command is put into a terminal. +// - Teleport Connect where the gateway command is shown in the UI. // // WithPrintFormat should NOT be used when the exec.Cmd gets executed by the // client application. diff --git a/lib/client/db/dbcmd/dbcmd_test.go b/lib/client/db/dbcmd/dbcmd_test.go index 5024664558357..2cf57ef95fc8d 100644 --- a/lib/client/db/dbcmd/dbcmd_test.go +++ b/lib/client/db/dbcmd/dbcmd_test.go @@ -570,6 +570,8 @@ func TestCLICommandBuilderGetConnectCommand(t *testing.T) { cmd: nil, wantErr: true, }, + // If you find yourself changing this test so that generating a command for DynamoDB _doesn't_ + // fail if WithPrintFormat() is not provided, please remember to update lib/teleterm/cmd/db.go. { name: "dynamodb for exec is an error", dbProtocol: defaults.ProtocolDynamoDB, diff --git a/lib/teleterm/cmd/cmds/cmds.go b/lib/teleterm/cmd/cmds/cmds.go new file mode 100644 index 0000000000000..d20800f921fee --- /dev/null +++ b/lib/teleterm/cmd/cmds/cmds.go @@ -0,0 +1,33 @@ +// Copyright 2024 Gravitational, Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package cmds + +import ( + "os/exec" +) + +// Cmds represents a single command in two variants – one that can be used to spawn a process and +// one that can be copied and pasted into a terminal. +// +// Defined in a separate package to avoid cyclic imports. CLI commands got refactored in v15+ anyway. +type Cmds struct { + // Exec is the command that should be used when directly executing a command for the given + // gateway. + Exec *exec.Cmd + // Preview is the command that should be used to display the command in the UI. Typically this + // means that Preview includes quotes around special characters, so that the command gets executed + // properly when the user copies and then pastes it into a terminal. + Preview *exec.Cmd +} + diff --git a/lib/teleterm/cmd/db.go b/lib/teleterm/cmd/db.go index 9614d13821ab6..257cdb043925e 100644 --- a/lib/teleterm/cmd/db.go +++ b/lib/teleterm/cmd/db.go @@ -15,14 +15,14 @@ package cmd import ( - "os/exec" - "github.com/gravitational/trace" "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/client/db/dbcmd" + "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/teleterm/api/uri" "github.com/gravitational/teleport/lib/teleterm/clusters" + "github.com/gravitational/teleport/lib/teleterm/cmd/cmds" "github.com/gravitational/teleport/lib/teleterm/gateway" "github.com/gravitational/teleport/lib/tlsca" ) @@ -45,10 +45,10 @@ func NewDBCLICommandProvider(storage StorageByResourceURI, execer dbcmd.Execer) } } -func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (*exec.Cmd, error) { +func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (cmds.Cmds, error) { cluster, _, err := d.storage.GetByResourceURI(gateway.TargetURI()) if err != nil { - return nil, trace.Wrap(err) + return cmds.Cmds{}, trace.Wrap(err) } routeToDb := tlsca.RouteToDatabase{ @@ -58,17 +58,35 @@ func (d DBCLICommandProvider) GetCommand(gateway gateway.Gateway) (*exec.Cmd, er Database: gateway.TargetSubresourceName(), } - cmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, + opts := []dbcmd.ConnectCommandFunc{ dbcmd.WithLogger(gateway.Log()), dbcmd.WithLocalProxy(gateway.LocalAddress(), gateway.LocalPortInt(), ""), dbcmd.WithNoTLS(), - dbcmd.WithPrintFormat(), dbcmd.WithTolerateMissingCLIClient(), dbcmd.WithExecer(d.execer), - ).GetConnectCommand() + } + + // DynamoDB doesn't support non-print-format use. + if gateway.Protocol() == defaults.ProtocolDynamoDB { + opts = append(opts, dbcmd.WithPrintFormat()) + } + + previewOpts := append(opts, dbcmd.WithPrintFormat()) + + execCmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, opts...).GetConnectCommand() if err != nil { - return nil, trace.Wrap(err) + return cmds.Cmds{}, trace.Wrap(err) + } + + previewCmd, err := clusters.NewDBCLICmdBuilder(cluster, routeToDb, previewOpts...).GetConnectCommand() + if err != nil { + return cmds.Cmds{}, trace.Wrap(err) + } + + cmds := cmds.Cmds{ + Exec: execCmd, + Preview: previewCmd, } - return cmd, nil + return cmds, nil } diff --git a/lib/teleterm/cmd/db_test.go b/lib/teleterm/cmd/db_test.go index c6cbdbdff9d9f..4722a50089f54 100644 --- a/lib/teleterm/cmd/db_test.go +++ b/lib/teleterm/cmd/db_test.go @@ -15,6 +15,7 @@ package cmd import ( + "fmt" "os/exec" "path/filepath" "strings" @@ -28,6 +29,7 @@ import ( "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/teleterm/api/uri" "github.com/gravitational/teleport/lib/teleterm/clusters" + "github.com/gravitational/teleport/lib/teleterm/cmd/cmds" "github.com/gravitational/teleport/lib/teleterm/gateway" "github.com/gravitational/teleport/lib/teleterm/gatewaytest" "github.com/gravitational/teleport/lib/tlsca" @@ -77,13 +79,14 @@ type fakeDatabaseGateway struct { gateway.Database targetURI uri.ResourceURI subresourceName string + protocol string } func (m fakeDatabaseGateway) TargetURI() uri.ResourceURI { return m.targetURI } func (m fakeDatabaseGateway) TargetName() string { return m.targetURI.GetDbName() } func (m fakeDatabaseGateway) TargetUser() string { return "alice" } func (m fakeDatabaseGateway) TargetSubresourceName() string { return m.subresourceName } -func (m fakeDatabaseGateway) Protocol() string { return defaults.ProtocolMongoDB } +func (m fakeDatabaseGateway) Protocol() string { return m.protocol } func (m fakeDatabaseGateway) Log() *logrus.Entry { return nil } func (m fakeDatabaseGateway) LocalAddress() string { return "localhost" } func (m fakeDatabaseGateway) LocalPortInt() int { return 8888 } @@ -93,14 +96,27 @@ func TestDbcmdCLICommandProviderGetCommand(t *testing.T) { testCases := []struct { name string targetSubresourceName string + argsCount int + protocol string + checkCmds func(*testing.T, fakeDatabaseGateway, cmds.Cmds) }{ { name: "empty name", + protocol: defaults.ProtocolMongoDB, targetSubresourceName: "", + checkCmds: checkMongoCmds, }, { name: "with name", + protocol: defaults.ProtocolMongoDB, targetSubresourceName: "bar", + checkCmds: checkMongoCmds, + }, + { + name: "custom handling of DynamoDB does not blow up", + targetSubresourceName: "bar", + protocol: defaults.ProtocolDynamoDB, + checkCmds: checkArgsNotEmpty, }, } @@ -116,15 +132,14 @@ func TestDbcmdCLICommandProviderGetCommand(t *testing.T) { mockGateway := fakeDatabaseGateway{ targetURI: cluster.URI.AppendDB("foo"), subresourceName: tc.targetSubresourceName, + protocol: tc.protocol, } dbcmdCLICommandProvider := NewDBCLICommandProvider(fakeStorage, fakeExec{}) - command, err := dbcmdCLICommandProvider.GetCommand(mockGateway) - + cmds, err := dbcmdCLICommandProvider.GetCommand(mockGateway) require.NoError(t, err) - require.NotEmpty(t, command.Args) - require.Contains(t, command.Args[1], tc.targetSubresourceName) - require.Contains(t, command.Args[1], mockGateway.LocalPort()) + + tc.checkCmds(t, mockGateway, cmds) }) } } @@ -168,3 +183,28 @@ func TestDbcmdCLICommandProviderGetCommand_ReturnsErrorIfClusterIsNotFound(t *te require.Error(t, err) require.True(t, trace.IsNotFound(err), "err is not trace.NotFound") } + +func checkMongoCmds(t *testing.T, gw fakeDatabaseGateway, cmds cmds.Cmds) { + t.Helper() + require.Len(t, cmds.Exec.Args, 2) + require.Len(t, cmds.Preview.Args, 2) + + execConnString := cmds.Exec.Args[1] + previewConnString := cmds.Preview.Args[1] + + require.Contains(t, execConnString, gw.TargetSubresourceName()) + require.Contains(t, previewConnString, gw.TargetSubresourceName()) + require.Contains(t, execConnString, gw.LocalPort()) + require.Contains(t, previewConnString, gw.LocalPort()) + + // Verify that the preview cmd has exec cmd conn string wrapped in quotes. + require.NotContains(t, execConnString, "\"") + expectedPreviewConnString := fmt.Sprintf("%q", execConnString) + require.Equal(t, expectedPreviewConnString, previewConnString) +} + +func checkArgsNotEmpty(t *testing.T, gw fakeDatabaseGateway, cmds cmds.Cmds) { + t.Helper() + require.NotEmpty(t, cmds.Exec.Args) + require.NotEmpty(t, cmds.Preview.Args) +} diff --git a/lib/teleterm/cmd/kube.go b/lib/teleterm/cmd/kube.go index c035ece3654d7..1976f571c67d2 100644 --- a/lib/teleterm/cmd/kube.go +++ b/lib/teleterm/cmd/kube.go @@ -23,6 +23,7 @@ import ( "github.com/gravitational/trace" "github.com/gravitational/teleport" + "github.com/gravitational/teleport/lib/teleterm/cmd/cmds" "github.com/gravitational/teleport/lib/teleterm/gateway" ) @@ -37,14 +38,14 @@ func NewKubeCLICommandProvider() KubeCLICommandProvider { // GetCommand returns a exec.Cmd with KUBECONFIG environment variable that can // be used by kube clients to connect to the kube gateway. -func (p KubeCLICommandProvider) GetCommand(g gateway.Gateway) (*exec.Cmd, error) { +func (p KubeCLICommandProvider) GetCommand(g gateway.Gateway) (cmds.Cmds, error) { kube, err := gateway.AsKube(g) if err != nil { - return nil, trace.Wrap(err) + return cmds.Cmds{}, trace.Wrap(err) } // Use kubectl version as placeholders. Only env should be used. cmd := exec.Command("kubectl", "version") cmd.Env = []string{fmt.Sprintf("%v=%v", teleport.EnvKubeConfig, kube.KubeconfigPath())} - return cmd, nil + return cmds.Cmds{Exec: cmd, Preview: cmd}, nil } diff --git a/lib/teleterm/cmd/kube_test.go b/lib/teleterm/cmd/kube_test.go index e3b3b2653f3d5..9ee812b03958a 100644 --- a/lib/teleterm/cmd/kube_test.go +++ b/lib/teleterm/cmd/kube_test.go @@ -33,5 +33,5 @@ func (m fakeKubeGateway) KubeconfigPath() string { return "test.kubeconfig" } func TestKubeCLICommandProvider(t *testing.T) { cmd, err := NewKubeCLICommandProvider().GetCommand(fakeKubeGateway{}) require.NoError(t, err) - require.Equal(t, []string{"KUBECONFIG=test.kubeconfig"}, cmd.Env) + require.Equal(t, []string{"KUBECONFIG=test.kubeconfig"}, cmd.Exec.Env) } diff --git a/lib/teleterm/gateway/base.go b/lib/teleterm/gateway/base.go index 9e274e5eb8931..0f4511e6a6c28 100644 --- a/lib/teleterm/gateway/base.go +++ b/lib/teleterm/gateway/base.go @@ -21,7 +21,6 @@ import ( "crypto/tls" "fmt" "net" - "os/exec" "strconv" "strings" @@ -32,6 +31,7 @@ import ( api "github.com/gravitational/teleport/gen/proto/go/teleport/lib/teleterm/v1" alpn "github.com/gravitational/teleport/lib/srv/alpnproxy" "github.com/gravitational/teleport/lib/teleterm/api/uri" + "github.com/gravitational/teleport/lib/teleterm/cmd/cmds" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" ) @@ -189,24 +189,24 @@ func (b *base) LocalPortInt() int { // CLICommand returns a command which launches a CLI client pointed at the gateway. func (b *base) CLICommand() (*api.GatewayCLICommand, error) { - cmd, err := b.cfg.CLICommandProvider.GetCommand(b) + cmds, err := b.cfg.CLICommandProvider.GetCommand(b) if err != nil { return nil, trace.Wrap(err) } - return makeCLICommand(cmd), nil + return makeCLICommand(cmds), nil } -func makeCLICommand(cmd *exec.Cmd) *api.GatewayCLICommand { - cmdString := strings.TrimSpace( +func makeCLICommand(cmds cmds.Cmds) *api.GatewayCLICommand { + preview := strings.TrimSpace( fmt.Sprintf("%s %s", - strings.Join(cmd.Env, " "), - strings.Join(cmd.Args, " "))) + strings.Join(cmds.Preview.Env, " "), + strings.Join(cmds.Preview.Args, " "))) return &api.GatewayCLICommand{ - Path: cmd.Path, - Args: cmd.Args, - Env: cmd.Env, - Preview: cmdString, + Path: cmds.Exec.Path, + Args: cmds.Exec.Args, + Env: cmds.Exec.Env, + Preview: preview, } } @@ -277,7 +277,7 @@ type base struct { // CLICommandProvider provides a CLI command for gateways which support CLI clients. type CLICommandProvider interface { - GetCommand(gateway Gateway) (*exec.Cmd, error) + GetCommand(gateway Gateway) (cmds.Cmds, error) } type TCPPortAllocator interface { diff --git a/lib/teleterm/gateway/base_test.go b/lib/teleterm/gateway/base_test.go index 9b430fb33b1b9..e5e7e8a041726 100644 --- a/lib/teleterm/gateway/base_test.go +++ b/lib/teleterm/gateway/base_test.go @@ -29,6 +29,7 @@ import ( "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/teleterm/api/uri" + "github.com/gravitational/teleport/lib/teleterm/cmd/cmds" "github.com/gravitational/teleport/lib/teleterm/gatewaytest" "github.com/gravitational/teleport/lib/tlsca" ) @@ -148,10 +149,10 @@ func TestNewWithLocalPortReturnsErrorIfNewPortEqualsOldPort(t *testing.T) { type mockCLICommandProvider struct{} -func (m mockCLICommandProvider) GetCommand(gateway Gateway) (*exec.Cmd, error) { +func (m mockCLICommandProvider) GetCommand(gateway Gateway) (cmds.Cmds, error) { absPath, err := filepath.Abs(gateway.Protocol()) if err != nil { - return nil, err + return cmds.Cmds{}, err } arg := fmt.Sprintf("%s/%s", gateway.TargetName(), gateway.TargetSubresourceName()) // Call exec.Command with a relative path so that cmd.Args[0] is a relative path. @@ -164,7 +165,7 @@ func (m mockCLICommandProvider) GetCommand(gateway Gateway) (*exec.Cmd, error) { cmd := exec.Command(gateway.Protocol(), arg) cmd.Path = absPath cmd.Env = []string{"FOO=bar"} - return cmd, nil + return cmds.Cmds{Exec: cmd, Preview: cmd}, nil } func createAndServeGateway(t *testing.T, tcpPortAllocator TCPPortAllocator) Gateway { diff --git a/proto/teleport/lib/teleterm/v1/gateway.proto b/proto/teleport/lib/teleterm/v1/gateway.proto index 32d1856435b34..605575dab1f6b 100644 --- a/proto/teleport/lib/teleterm/v1/gateway.proto +++ b/proto/teleport/lib/teleterm/v1/gateway.proto @@ -55,16 +55,23 @@ message Gateway { GatewayCLICommand gateway_cli_command = 10; } -// GatewayCLICommand represents a command that the user can execute to connect to the gateway -// resource. It is a direct translation of os.exec.Cmd. +// GatewayCLICommand represents a command that the user can execute to connect to a gateway +// resource. It is a combination of two different os/exec.Cmd structs, where path, args and env are +// directly taken from one Cmd and the preview field is constructed from another Cmd. message GatewayCLICommand { string path = 1; repeated string args = 2; repeated string env = 3; // preview is used to show the user what command will be executed before they decide to run it. - // It's like os.exec.Cmd.String with two exceptions: + // It can also be copied and then pasted into a terminal. + // It's like os/exec.Cmd.String with two exceptions: // // 1) It is prepended with Cmd.Env. // 2) The command name is relative and not absolute. + // 3) It is taken from a different Cmd than the other fields in this message. This Cmd uses a + // special print format which makes the args suitable to be entered into a terminal, but not to + // directly spawn a process. + // + // Should not be used to execute the command in the shell. Instead, use path, args, and env. string preview = 4; } From 895e1bff9c2e53ee8ef7c0cb23de10b0a023447e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 2 Apr 2024 13:12:19 +0200 Subject: [PATCH 2/2] Fix lint --- lib/teleterm/cmd/cmds/cmds.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/teleterm/cmd/cmds/cmds.go b/lib/teleterm/cmd/cmds/cmds.go index d20800f921fee..4bd1aca10a2ea 100644 --- a/lib/teleterm/cmd/cmds/cmds.go +++ b/lib/teleterm/cmd/cmds/cmds.go @@ -11,6 +11,7 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + package cmds import ( @@ -30,4 +31,3 @@ type Cmds struct { // properly when the user copies and then pastes it into a terminal. Preview *exec.Cmd } -