From f894aa369c2e4918a40d6b91b3f4dcf230059e51 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 18 Apr 2023 17:23:21 -0700 Subject: [PATCH 1/2] * Use Stderr during headless login to fix rsync compatibility * Improve error messages when attempting to login without a terminal * Skip MOTD acknowledement when logging in without a terminal (headless, sso) * When attempting re-login without a terminal, make a debug log and return the original error * Return access denied error on unexpected handshake failures in tsh proxy ssh --- lib/client/api.go | 40 +++++++++++++++++++++--------- lib/utils/prompt/context_reader.go | 5 ++++ lib/utils/prompt/mock.go | 4 +++ lib/utils/prompt/stdin.go | 1 + tool/tsh/proxy.go | 7 ++++++ tool/tsh/proxy_test.go | 3 +-- 6 files changed, 46 insertions(+), 14 deletions(-) diff --git a/lib/client/api.go b/lib/client/api.go index 267fc968741f5..10e229a81a4b8 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -551,6 +551,10 @@ func RetryWithRelogin(ctx context.Context, tc *TeleportClient, fn func() error) key, err := tc.Login(ctx) if err != nil { + if errors.Is(err, prompt.ErrNotTerminal) { + log.WithError(err).Debugf("Relogin is not available in this environment") + return trace.Wrap(fnErr) + } if trace.IsTrustError(err) { return trace.Wrap(err, "refusing to connect to untrusted proxy %v without --insecure flag\n", tc.SSHProxyAddr) } @@ -3697,7 +3701,7 @@ func (tc *TeleportClient) headlessLogin(ctx context.Context, priv *keys.PrivateK tshApprove := fmt.Sprintf("tsh headless approve --user=%v --proxy=%v %v", tc.Username, tc.WebProxyAddr, headlessAuthenticationID) - fmt.Fprintf(tc.Stdout, "Complete headless authentication in your local web browser:\n\n%s\n"+ + fmt.Fprintf(tc.Stderr, "Complete headless authentication in your local web browser:\n\n%s\n"+ "\nor execute this command in your local terminal:\n\n%s\n", webUILink, tshApprove) response, err := SSHAgentHeadlessLogin(ctx, SSHLoginHeadless{ @@ -3819,7 +3823,7 @@ func (tc *TeleportClient) Ping(ctx context.Context) (*webclient.PingResponse, er // If version checking was requested and the server advertises a minimum version. if tc.CheckVersions && pr.MinClientVersion != "" { if err := utils.CheckVersion(teleport.Version, pr.MinClientVersion); err != nil && trace.IsBadParameter(err) { - fmt.Fprintf(tc.Config.Stderr, ` + fmt.Fprintf(tc.Stderr, ` WARNING Detected potentially incompatible client and server versions. Minimum client version supported by the server is %v but you are using %v. @@ -3863,15 +3867,19 @@ func (tc *TeleportClient) ShowMOTD(ctx context.Context) error { } if motd.Text != "" { - fmt.Fprintf(tc.Config.Stderr, "%s\nPress [ENTER] to continue.\n", motd.Text) - // We're re-using the password reader for user acknowledgment for - // aesthetic purposes, because we want to hide any garbage the - // use might enter at the prompt. Whatever the user enters will - // be simply discarded, and the user can still CTRL+C out if they - // disagree. - _, err := prompt.Stdin().ReadPassword(context.Background()) - if err != nil { - return trace.Wrap(err) + fmt.Fprintln(tc.Stderr, motd.Text) + + // If possible, prompt the user for acknowledement before continuing. + if stdin := prompt.Stdin(); stdin.IsTerminal() { + // We're re-using the password reader for user acknowledgment for + // aesthetic purposes, because we want to hide any garbage the + // user might enter at the prompt. Whatever the user enters will + // be simply discarded, and the user can still CTRL+C out if they + // disagree. + fmt.Fprintln(tc.Stderr, "Press [ENTER] to continue.") + if _, err := stdin.ReadPassword(ctx); err != nil { + return trace.Wrap(err) + } } } @@ -4228,13 +4236,21 @@ func Username() (string, error) { // AskOTP prompts the user to enter the OTP token. func (tc *TeleportClient) AskOTP(ctx context.Context) (token string, err error) { + stdin := prompt.Stdin() + if !stdin.IsTerminal() { + return "", trace.Wrap(prompt.ErrNotTerminal, "cannot perform OTP login without a terminal") + } return prompt.Password(ctx, tc.Stderr, prompt.Stdin(), "Enter your OTP token") } // AskPassword prompts the user to enter the password func (tc *TeleportClient) AskPassword(ctx context.Context) (pwd string, err error) { + stdin := prompt.Stdin() + if !stdin.IsTerminal() { + return "", trace.Wrap(prompt.ErrNotTerminal, "cannot perform password login without a terminal") + } return prompt.Password( - ctx, tc.Stderr, prompt.Stdin(), fmt.Sprintf("Enter password for Teleport user %v", tc.Config.Username)) + ctx, tc.Stderr, stdin, fmt.Sprintf("Enter password for Teleport user %v", tc.Config.Username)) } // LoadTLSConfig returns the user's TLS configuration, either from static diff --git a/lib/utils/prompt/context_reader.go b/lib/utils/prompt/context_reader.go index 005ea0277d1b3..7c81e76aac1a2 100644 --- a/lib/utils/prompt/context_reader.go +++ b/lib/utils/prompt/context_reader.go @@ -184,6 +184,11 @@ func (cr *ContextReader) processReads() { } } +// IsTerminal returns whether the given reader is a terminal. +func (cr *ContextReader) IsTerminal() bool { + return cr.term.IsTerminal(cr.fd) +} + // handleInterrupt restores terminal state on interrupts. // Called only on global ContextReaders, such as Stdin. func (cr *ContextReader) handleInterrupt() { diff --git a/lib/utils/prompt/mock.go b/lib/utils/prompt/mock.go index 455a700da3d7d..de707584dcad3 100644 --- a/lib/utils/prompt/mock.go +++ b/lib/utils/prompt/mock.go @@ -34,6 +34,10 @@ func NewFakeReader() *FakeReader { return &FakeReader{} } +func (r *FakeReader) IsTerminal() bool { + return true +} + func (r *FakeReader) AddReply(fn FakeReplyFunc) *FakeReader { r.mu.Lock() defer r.mu.Unlock() diff --git a/lib/utils/prompt/stdin.go b/lib/utils/prompt/stdin.go index 49a35d55f7da9..452dd15f9a0f5 100644 --- a/lib/utils/prompt/stdin.go +++ b/lib/utils/prompt/stdin.go @@ -28,6 +28,7 @@ var ( // StdinReader contains ContextReader methods applicable to stdin. type StdinReader interface { + IsTerminal() bool Reader SecureReader } diff --git a/tool/tsh/proxy.go b/tool/tsh/proxy.go index 0089e10ca70d3..8c94a8afbee45 100644 --- a/tool/tsh/proxy.go +++ b/tool/tsh/proxy.go @@ -197,6 +197,13 @@ func sshProxy(ctx context.Context, tc *libclient.TeleportClient, sp sshProxyPara HostKeyCallback: tc.HostKeyCallback, }) if err != nil { + if utils.IsHandshakeFailedError(err) { + // TODO(codingllama): Improve error message below for device trust. + // An alternative we have here is querying the cluster to check if device + // trust is required, a check similar to `IsMFARequired`. + log.Infof("Access denied to %v connecting to %v: %v", tc.HostLogin, remoteProxyAddr, err) + return trace.AccessDenied(`access denied to %v connecting to %v`, tc.HostLogin, remoteProxyAddr) + } return trace.Wrap(err) } defer client.Close() diff --git a/tool/tsh/proxy_test.go b/tool/tsh/proxy_test.go index de03ab62ad24c..c943f557de371 100644 --- a/tool/tsh/proxy_test.go +++ b/tool/tsh/proxy_test.go @@ -47,7 +47,6 @@ import ( "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/service/servicecfg" "github.com/gravitational/teleport/lib/teleagent" - "github.com/gravitational/teleport/lib/utils" ) // TestSSH verifies "tsh ssh" command. @@ -338,7 +337,7 @@ func TestProxySSH(t *testing.T) { err := runProxySSH(invalidLoginRequest, setHomePath(homePath), setKubeConfigPath(kubeConfigPath), setMockSSOLogin(t, s)) require.Error(t, err) - require.True(t, utils.IsHandshakeFailedError(err), "expected handshake error, got %v", err) + require.True(t, trace.IsAccessDenied(err), "expected access denied, got %v", err) }) }) } From bde733a43f9dc98e09586a25c05e5f970d892fa2 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 26 Apr 2023 11:33:50 -0700 Subject: [PATCH 2/2] Fix TestProxySSH test and add it to the flaky test detector testsToSkip list. --- build.assets/tooling/cmd/difftest/main.go | 4 +++ tool/tsh/proxy.go | 14 ++++++--- tool/tsh/proxy_test.go | 37 +++++++++++------------ 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/build.assets/tooling/cmd/difftest/main.go b/build.assets/tooling/cmd/difftest/main.go index 9e54bb540fdbb..fcd38cead5e83 100644 --- a/build.assets/tooling/cmd/difftest/main.go +++ b/build.assets/tooling/cmd/difftest/main.go @@ -55,6 +55,10 @@ var ( // TestSSHOnMultipleNodes and its successor TestSSHWithMFA take ~10-15s to run which prevents // it from ever completing the 100 runs successfully. "TestSSHOnMultipleNodes", "TestSSHWithMFA", + + // TestProxySSH takes around 10-15s to run, largely due to the 7-10 seconds it takes to create a + // tsh test suite. This prevents it from ever completing the 100 runs successfully. + "TestProxySSH", } ) diff --git a/tool/tsh/proxy.go b/tool/tsh/proxy.go index 8c94a8afbee45..d85674b4b4377 100644 --- a/tool/tsh/proxy.go +++ b/tool/tsh/proxy.go @@ -188,12 +188,18 @@ func sshProxy(ctx context.Context, tc *libclient.TeleportClient, sp sshProxyPara } defer upstreamConn.Close() + signers, err := tc.LocalAgent().Signers() + if err != nil { + return trace.Wrap(err) + } + if len(signers) == 0 { + return trace.BadParameter("no SSH auth methods loaded, are you logged in?") + } + remoteProxyAddr := net.JoinHostPort(sp.proxyHost, sp.proxyPort) client, err := makeSSHClient(ctx, upstreamConn, remoteProxyAddr, &ssh.ClientConfig{ - User: tc.HostLogin, - Auth: []ssh.AuthMethod{ - ssh.PublicKeysCallback(tc.LocalAgent().Signers), - }, + User: tc.HostLogin, + Auth: []ssh.AuthMethod{ssh.PublicKeys(signers...)}, HostKeyCallback: tc.HostKeyCallback, }) if err != nil { diff --git a/tool/tsh/proxy_test.go b/tool/tsh/proxy_test.go index c943f557de371..c297bb0800cac 100644 --- a/tool/tsh/proxy_test.go +++ b/tool/tsh/proxy_test.go @@ -252,7 +252,9 @@ func TestSSHLoadAllCAs(t *testing.T) { // TestProxySSH verifies "tsh proxy ssh" functionality func TestProxySSH(t *testing.T) { - createAgent(t) + // ssh agent can cause race conditions in parallel tests. + disableAgent(t) + ctx := context.Background() tests := []struct { name string @@ -279,7 +281,9 @@ func TestProxySSH(t *testing.T) { } for _, tc := range tests { + tc := tc t.Run(tc.name, func(t *testing.T) { + t.Parallel() s := newTestSuite(t, tc.opts...) proxyRequest := fmt.Sprintf("%s.%s:%d", @@ -288,35 +292,27 @@ func TestProxySSH(t *testing.T) { s.root.Config.SSH.Addr.Port(defaults.SSHServerListenPort)) runProxySSH := func(proxyRequest string, opts ...cliOption) error { - return Run(context.Background(), []string{ + return Run(ctx, []string{ "--insecure", "--proxy", s.root.Config.Proxy.WebAddr.Addr, "proxy", "ssh", proxyRequest, }, opts...) } - t.Run("login", func(t *testing.T) { - t.Parallel() - - // Should fail without login - err := runProxySSH(proxyRequest, setHomePath(t.TempDir())) - require.Error(t, err) + // login to Teleport + homePath, kubeConfigPath := mustLogin(t, s) - // login into Teleport - homePath, kubeConfigPath := mustLogin(t, s) + t.Run("logged in", func(t *testing.T) { + t.Parallel() - // Should succeed with login - err = runProxySSH(proxyRequest, setHomePath(homePath), setKubeConfigPath(kubeConfigPath)) + err := runProxySSH(proxyRequest, setHomePath(homePath), setKubeConfigPath(kubeConfigPath)) require.NoError(t, err) }) t.Run("re-login", func(t *testing.T) { t.Parallel() - // login into Teleport - homePath, kubeConfigPath := mustLogin(t, s) - - err := runProxySSH(proxyRequest, setHomePath(homePath), setKubeConfigPath(kubeConfigPath), setMockSSOLogin(t, s)) + err := runProxySSH(proxyRequest, setHomePath(t.TempDir()), setKubeConfigPath(filepath.Join(t.TempDir(), teleport.KubeConfigFile)), setMockSSOLogin(t, s)) require.NoError(t, err) }) @@ -331,10 +327,6 @@ func TestProxySSH(t *testing.T) { t.Parallel() invalidLoginRequest := fmt.Sprintf("%s@%s", "invalidUser", proxyRequest) - - // login into Teleport - homePath, kubeConfigPath := mustLogin(t, s) - err := runProxySSH(invalidLoginRequest, setHomePath(homePath), setKubeConfigPath(kubeConfigPath), setMockSSOLogin(t, s)) require.Error(t, err) require.True(t, trace.IsAccessDenied(err), "expected access denied, got %v", err) @@ -703,6 +695,11 @@ func createAgent(t *testing.T) string { return teleAgent.Path } +func disableAgent(t *testing.T) { + t.Helper() + t.Setenv(teleport.SSHAuthSock, "") +} + func setMockSSOLogin(t *testing.T, s *suite) cliOption { return func(cf *CLIConf) error { cf.mockSSOLogin = mockSSOLogin(t, s.root.GetAuthServer(), s.user)