From 461878f0f657a2127adbe4f7a024ee6697cc34c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Andr=C3=A9=20Dinis?= Date: Tue, 31 May 2022 16:24:57 +0100 Subject: [PATCH] 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 | 34 +++++++------- api/client/client_test.go | 7 +-- integration/client_test.go | 76 +++++++++++++++++++++++++++++++ integration/proxy_helpers_test.go | 4 +- integration/proxy_test.go | 2 +- 5 files changed, 100 insertions(+), 23 deletions(-) create mode 100644 integration/client_test.go diff --git a/api/client/client.go b/api/client/client.go index 457d298752ac3..96042bdb339a5 100644 --- a/api/client/client.go +++ b/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 ( diff --git a/api/client/client_test.go b/api/client/client_test.go index b67aceb0c65c4..2b563aff3881a 100644 --- a/api/client/client_test.go +++ b/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") }, }} diff --git a/integration/client_test.go b/integration/client_test.go new file mode 100644 index 0000000000000..49b8475493367 --- /dev/null +++ b/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") +} diff --git a/integration/proxy_helpers_test.go b/integration/proxy_helpers_test.go index f2836ae7e2ae0..9a112de2e6609 100644 --- a/integration/proxy_helpers_test.go +++ b/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, ) diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 833c95ad7ce5b..2593f66fe46f1 100644 --- a/integration/proxy_test.go +++ b/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)