From 94baceef87afa8929bdfbf2b86f526ef9b7f718d Mon Sep 17 00:00:00 2001 From: Andrew LeFevre Date: Thu, 13 Apr 2023 17:16:26 -0400 Subject: [PATCH] only call 'user.Current' when we really need to (#24156) Calling 'user.Current' can be extremely slow in some instances, so avoid it when possible. --- api/profile/profile.go | 4 +- api/utils/user.go | 64 +++++++++++++++++++++++++++ api/utils/user_test.go | 44 ++++++++++++++++++ lib/client/api.go | 19 ++++---- lib/client/db/mysql/optionfile.go | 4 +- lib/client/db/postgres/servicefile.go | 4 +- tool/tsh/tsh.go | 3 +- 7 files changed, 126 insertions(+), 16 deletions(-) create mode 100644 api/utils/user.go create mode 100644 api/utils/user_test.go diff --git a/api/profile/profile.go b/api/profile/profile.go index b06046e760d..a45a3cb3d3f 100644 --- a/api/profile/profile.go +++ b/api/profile/profile.go @@ -23,7 +23,6 @@ import ( "io/fs" "net" "os" - "os/user" "path/filepath" "strings" @@ -31,6 +30,7 @@ import ( "golang.org/x/crypto/ssh" "gopkg.in/yaml.v2" + "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/api/utils/keypaths" "github.com/gravitational/teleport/api/utils/keys" "github.com/gravitational/teleport/api/utils/sshutils" @@ -314,7 +314,7 @@ func FullProfilePath(dir string) string { // defaultProfilePath retrieves the default path of the TSH profile. func defaultProfilePath() string { home := os.TempDir() - if u, err := user.Current(); err == nil && u.HomeDir != "" { + if u, err := utils.CurrentUser(); err == nil && u.HomeDir != "" { home = u.HomeDir } return filepath.Join(home, profileDir) diff --git a/api/utils/user.go b/api/utils/user.go new file mode 100644 index 00000000000..f79aea1f5b7 --- /dev/null +++ b/api/utils/user.go @@ -0,0 +1,64 @@ +/* +Copyright 2023 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 utils + +import ( + "os/user" + "time" + + "github.com/gravitational/trace" +) + +const lookupTimeout = 10 * time.Second + +type userGetter func() (*user.User, error) + +type userResult struct { + user *user.User + err error +} + +// CurrentUser is just like [user.Current], except an error is returned +// if the user lookup exceeds 10 seconds. This is because if +// [user.Current] is called on a domain joined host, the user lookup +// will contact potentially multiple domain controllers which can hang. +func CurrentUser() (*user.User, error) { + return currentUser(user.Current, lookupTimeout) +} + +func currentUser(getUser userGetter, timeout time.Duration) (*user.User, error) { + result := make(chan userResult, 1) + go func() { + u, err := getUser() + result <- userResult{ + user: u, + err: err, + } + }() + + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case u := <-result: + return u.user, trace.Wrap(u.err) + case <-timer.C: + return nil, trace.LimitExceeded( + "looking up the current host user exceeded timeout, try explicitly specifying the Teleport user or host user with --user or --login", + ) + } +} diff --git a/api/utils/user_test.go b/api/utils/user_test.go new file mode 100644 index 00000000000..d8bfbe4e744 --- /dev/null +++ b/api/utils/user_test.go @@ -0,0 +1,44 @@ +/* +Copyright 2023 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 utils + +import ( + "os/user" + "testing" + "time" + + "github.com/gravitational/trace" + "github.com/stretchr/testify/require" +) + +func TestCurrentUser(t *testing.T) { + t.Parallel() + + u, err := currentUser(func() (*user.User, error) { + time.Sleep(100 * time.Millisecond) + return nil, nil + }, 10*time.Millisecond) + require.Nil(t, u) + require.NotNil(t, err) + require.True(t, trace.IsLimitExceeded(err)) + + u, err = currentUser(func() (*user.User, error) { + return nil, nil + }, lookupTimeout) + require.Nil(t, u) + require.NoError(t, err) +} diff --git a/lib/client/api.go b/lib/client/api.go index 4591199c858..464d08ef7c7 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -29,7 +29,6 @@ import ( "net/url" "os" "os/exec" - "os/user" "path/filepath" "runtime" "strconv" @@ -1727,7 +1726,7 @@ func (tc *TeleportClient) runShellOrCommandOnSingleNode(ctx context.Context, clt if len(tc.Config.LocalForwardPorts) == 0 { fmt.Println("Executing command locally without connecting to any servers. This makes no sense.") } - return runLocalCommand(command) + return runLocalCommand(tc.Config.HostLogin, command) } if len(command) > 0 { @@ -4152,7 +4151,7 @@ func connectToSSHAgent() agent.ExtendedAgent { // Username returns the current user's username func Username() (string, error) { - u, err := user.Current() + u, err := apiutils.CurrentUser() if err != nil { return "", trace.Wrap(err) } @@ -4325,18 +4324,22 @@ func ParseSearchKeywords(spec string, customDelimiter rune) []string { // Executes the given command on the client machine (localhost). If no command is given, // executes shell -func runLocalCommand(command []string) error { +func runLocalCommand(hostLogin string, command []string) error { if len(command) == 0 { - user, err := user.Current() - if err != nil { - return trace.Wrap(err) + if hostLogin == "" { + user, err := apiutils.CurrentUser() + if err != nil { + return trace.Wrap(err) + } + hostLogin = user.Username } - shell, err := shell.GetLoginShell(user.Username) + shell, err := shell.GetLoginShell(hostLogin) if err != nil { return trace.Wrap(err) } command = []string{shell} } + cmd := exec.Command(command[0], command[1:]...) cmd.Stderr = os.Stderr cmd.Stdin = os.Stdin diff --git a/lib/client/db/mysql/optionfile.go b/lib/client/db/mysql/optionfile.go index 576f66384aa..7c6a31b98fa 100644 --- a/lib/client/db/mysql/optionfile.go +++ b/lib/client/db/mysql/optionfile.go @@ -17,7 +17,6 @@ limitations under the License. package mysql import ( - "os/user" "path/filepath" "strconv" "strings" @@ -25,6 +24,7 @@ import ( "github.com/gravitational/trace" "gopkg.in/ini.v1" + "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/lib/client/db/profile" ) @@ -40,7 +40,7 @@ type OptionFile struct { func DefaultConfigPath() (string, error) { // Default location is .my.cnf file in the user's home directory. - usr, err := user.Current() + usr, err := utils.CurrentUser() if err != nil { return "", trace.ConvertSystemError(err) } diff --git a/lib/client/db/postgres/servicefile.go b/lib/client/db/postgres/servicefile.go index 2018d4d0363..67357f76d63 100644 --- a/lib/client/db/postgres/servicefile.go +++ b/lib/client/db/postgres/servicefile.go @@ -17,7 +17,6 @@ limitations under the License. package postgres import ( - "os/user" "path/filepath" "strconv" "strings" @@ -25,6 +24,7 @@ import ( "github.com/gravitational/trace" "gopkg.in/ini.v1" + "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/lib/client/db/profile" ) @@ -42,7 +42,7 @@ type ServiceFile struct { func Load() (*ServiceFile, error) { // Default location is .pg_service.conf file in the user's home directory. // TODO(r0mant): Check PGSERVICEFILE and PGSYSCONFDIR env vars as well. - user, err := user.Current() + user, err := utils.CurrentUser() if err != nil { return nil, trace.ConvertSystemError(err) } diff --git a/tool/tsh/tsh.go b/tool/tsh/tsh.go index 399f7aa9326..819b108b6ed 100644 --- a/tool/tsh/tsh.go +++ b/tool/tsh/tsh.go @@ -595,10 +595,9 @@ func Run(ctx context.Context, args []string, opts ...cliOption) error { app := utils.InitCLIParser("tsh", "Teleport Command Line Client").Interspersed(true) app.Flag("login", "Remote host login").Short('l').Envar(loginEnvVar).StringVar(&cf.NodeLogin) - localUser, _ := client.Username() app.Flag("proxy", "Teleport proxy address").Envar(proxyEnvVar).StringVar(&cf.Proxy) app.Flag("nocache", "do not cache cluster discovery locally").Hidden().BoolVar(&cf.NoCache) - app.Flag("user", fmt.Sprintf("Teleport user [%s]", localUser)).Envar(userEnvVar).StringVar(&cf.Username) + app.Flag("user", "Teleport user, defaults to current local user").Envar(userEnvVar).StringVar(&cf.Username) app.Flag("mem-profile", "Write memory profile to file").Hidden().StringVar(&memProfile) app.Flag("cpu-profile", "Write CPU profile to file").Hidden().StringVar(&cpuProfile) app.Flag("trace-profile", "Write trace profile to file").Hidden().StringVar(&traceProfile)