Improve error msg when client fails to auth in Teleport (#12677)

When the client connects to teleport with invalid credentials (eg
expired ones) it will retry multiple times until the context deadline is
reached.
When it happens, we receive the generic error: context deadline
exceeded.
However, we can ask for the latest connection error, one which will give
us more information on why it happened.
To ask for this extra error we need to add the following
grpc.DialOption: grpc.WithReturnConnectionError()

After doing this, we will get the errors that happenned when trying to
connect to the grpc Server.

This should help us debug possible connection problems.

We had to refactor a little bit the way we handle the parallel
connection attempts to receive all the connection errors from the
multiple flows.

api/client/client.go

@@ -153,10 +153,7 @@ func connect(ctx context.Context, cfg Config) (*Client, error) {
 	// sendError is used to send errors to errChan with context.
 	errChan := make(chan error)
 	sendError := func(err error) {
-		select {
-		case <-ctx.Done():
-		case errChan <- trace.Wrap(err):
-		}
+		errChan <- trace.Wrap(err)
 	}
 
 	// syncConnect is used to concurrently create multiple clients
@@ -248,7 +245,7 @@ func connect(ctx context.Context, cfg Config) (*Client, error) {
 	}()
 
 	var errs []error
-	for {
+	for errChan != nil {
 		select {
 		// Use the first client to successfully connect in syncConnect.
 		case clt := <-cltChan:
@@ -259,20 +256,23 @@ func connect(ctx context.Context, cfg Config) (*Client, error) {
 				errs = append(errs, trace.Wrap(err, ""))
 				continue
 			}
-			// errChan is closed, return errors.
-			if len(errs) == 0 {
-				if len(cfg.Addrs) == 0 && cfg.Dialer == nil {
-					// Some credentials don't require these fields. If no errors propagate, then they need to provide these fields.
-					return nil, trace.BadParameter("no connection methods found, try providing Dialer or Addrs in config")
-				}
-				// This case should never be reached with config validation and above case.
-				return nil, trace.Errorf("no connection methods found")
-			}
-			return nil, trace.Wrap(trace.NewAggregate(errs...), "all connection methods failed")
-		case <-ctx.Done():
-			return nil, trace.Wrap(ctx.Err())
+			errChan = nil
 		}
 	}
+
+	// errChan is closed, return errors.
+	if len(errs) == 0 {
+		if len(cfg.Addrs) == 0 && cfg.Dialer == nil {
+			// Some credentials don't require these fields. If no errors propagate, then they need to provide these fields.
+			return nil, trace.BadParameter("no connection methods found, try providing Dialer or Addrs in config")
+		}
+		// This case should never be reached with config validation and above case.
+		return nil, trace.Errorf("no connection methods found")
+	}
+	if ctx.Err() != nil {
+		errs = append(errs, trace.Wrap(ctx.Err()))
+	}
+	return nil, trace.Wrap(trace.NewAggregate(errs...), "all connection methods failed")
 }
 
 type (

api/client/client_test.go

@@ -21,7 +21,6 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net"
-	"strings"
 	"testing"
 	"time"
 
@@ -338,7 +337,8 @@ func TestNew(t *testing.T) {
 			},
 		},
 		assertErr: func(t require.TestingT, err error, _ ...interface{}) {
-			require.True(t, strings.Contains(err.Error(), "all connection methods failed"))
+			require.Error(t, err)
+			require.Contains(t, err.Error(), "all connection methods failed")
 		},
 	}, {
 		desc: "fail to dial with no address or dialer.",
@@ -352,7 +352,8 @@ func TestNew(t *testing.T) {
 			},
 		},
 		assertErr: func(t require.TestingT, err error, _ ...interface{}) {
-			require.True(t, strings.Contains(err.Error(), "no connection methods found, try providing Dialer or Addrs in config"))
+			require.Error(t, err)
+			require.Contains(t, err.Error(), "no connection methods found, try providing Dialer or Addrs in config")
 		},
 	}}
 

integration/client_test.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2020-2022 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 integration
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/gravitational/teleport/api/client"
+	"github.com/gravitational/teleport/lib/service"
+	"github.com/gravitational/teleport/lib/utils"
+	"github.com/stretchr/testify/require"
+	"google.golang.org/grpc"
+)
+
+// TestClientWithExpiredCredentialsAndDetailedErrorMessage creates and connects to the Auth service
+// using an expired user identity
+// We should receive an error message which contains the real cause (ssh: handshake)
+func TestClientWithExpiredCredentialsAndDetailedErrorMessage(t *testing.T) {
+	rc := NewInstance(InstanceConfig{
+		ClusterName: "root.example.com",
+		HostID:      uuid.New().String(),
+		NodeName:    Loopback,
+		log:         utils.NewLoggerForTests(),
+		Ports:       singleProxyPortSetup(),
+	})
+
+	rcConf := service.MakeDefaultConfig()
+	rcConf.DataDir = t.TempDir()
+	rcConf.Auth.Enabled = true
+	rcConf.Proxy.Enabled = true
+	rcConf.Proxy.DisableWebInterface = true
+	rcConf.SSH.Enabled = true
+	rcConf.Version = "v2"
+
+	username := mustGetCurrentUser(t).Username
+	rc.AddUser(username, []string{username})
+
+	err := rc.CreateEx(t, nil, rcConf)
+	require.NoError(t, err)
+	err = rc.Start()
+	require.NoError(t, err)
+	defer rc.StopAll()
+
+	// Create an expired identity file: ttl is 1 second in the past
+	identityFilePath := mustCreateUserIdentityFile(t, rc, username, -time.Second)
+
+	ctx, cancelFunc := context.WithTimeout(context.Background(), time.Second)
+	defer cancelFunc()
+	_, err = client.New(ctx, client.Config{
+		Addrs:       []string{rc.GetAuthAddr()},
+		Credentials: []client.Credentials{client.LoadIdentityFile(identityFilePath)},
+		DialOpts: []grpc.DialOption{
+			// ask for underlying errors
+			grpc.WithReturnConnectionError(),
+		},
+	})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "ssh: handshake failed")
+}

integration/proxy_helpers_test.go

@@ -511,13 +511,13 @@ func makeNodeConfig(nodeName, authAddr string) *service.Config {
 	return nodeConfig
 }
 
-func mustCreateUserIdentityFile(t *testing.T, tc *TeleInstance, username string) string {
+func mustCreateUserIdentityFile(t *testing.T, tc *TeleInstance, username string, ttl time.Duration) string {
 	key, err := libclient.NewKey()
 	require.NoError(t, err)
 	key.ClusterName = tc.Secrets.SiteName
 
 	sshCert, tlsCert, err := tc.Process.GetAuthServer().GenerateUserTestCerts(
-		key.Pub, username, time.Hour,
+		key.Pub, username, ttl,
 		constants.CertificateFormatStandard,
 		tc.Secrets.SiteName,
 	)

integration/proxy_test.go

@@ -641,7 +641,7 @@ func TestALPNProxyAuthClientConnectWithUserIdentity(t *testing.T) {
 	require.NoError(t, err)
 	defer rc.StopAll()
 
-	identityFilePath := mustCreateUserIdentityFile(t, rc, username)
+	identityFilePath := mustCreateUserIdentityFile(t, rc, username, time.Hour)
 
 	identity := client.LoadIdentityFile(identityFilePath)
 	require.NoError(t, err)