[v18] avoid join fallback attempts on legitimate join failure

Backport #60673 to branch/v18

Author: nklaassen

lib/join/join_test.go

@@ -225,6 +225,134 @@ func TestJoin(t *testing.T) {
 	})
 }
 
+// TestJoinError asserts that attempts to join with an invalid token return an
+// AccessDenied error and do not fall back to joining via the legacy join
+// service.
+func TestJoinError(t *testing.T) {
+	t.Parallel()
+
+	token, err := types.NewProvisionTokenFromSpec("token1", time.Now().Add(time.Minute), types.ProvisionTokenSpecV2{
+		Roles: []types.SystemRole{
+			types.RoleNode,
+			types.RoleProxy,
+		},
+	})
+	require.NoError(t, err)
+
+	authService := newFakeAuthService(t)
+	require.NoError(t, authService.Auth().UpsertToken(t.Context(), token))
+
+	proxy := newFakeProxy(authService)
+	proxy.join(t)
+	proxyListener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	t.Cleanup(func() { proxyListener.Close() })
+	proxy.runGRPCServer(t, proxyListener)
+
+	// List on a free port just to guarantee an address that will reject/close
+	// all connection attempts.
+	badListener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	testutils.RunTestBackgroundTask(t.Context(), t, &testutils.TestBackgroundTask{
+		Name: "bad listener",
+		Task: func(ctx context.Context) error {
+			for {
+				conn, err := badListener.Accept()
+				if err != nil {
+					if ctx.Err() != nil {
+						return ctx.Err()
+					}
+					return err
+				}
+				conn.Close()
+			}
+		},
+		Terminate: badListener.Close,
+	})
+
+	// Assert that the real AccessDenied error is returned with various
+	// configurations joining via an auth or proxy address.
+	for _, tc := range []struct {
+		desc       string
+		joinParams joinclient.JoinParams
+		assertErr  assert.ErrorAssertionFunc
+	}{
+		{
+			desc: "auth direct",
+			joinParams: joinclient.JoinParams{
+				AuthServers: []utils.NetAddr{utils.FromAddr(authService.TLS.Listener.Addr())},
+			},
+			assertErr: func(t assert.TestingT, err error, msgAndArgs ...any) bool {
+				// Should get AccessDenied and should not fall back to joining
+				// via the legacy service.
+				return assert.ErrorAs(t, err, new(*trace.AccessDeniedError)) &&
+					assert.NotErrorAs(t, err, new(*joinclient.LegacyJoinError))
+			},
+		},
+		{
+			// With teleport config v2 or certain bot configurations a proxy
+			// address is passed in AuthServers, which supports both auth and
+			// proxy addresses.
+			desc: "proxy as auth",
+			joinParams: joinclient.JoinParams{
+				AuthServers: []utils.NetAddr{utils.FromAddr(proxyListener.Addr())},
+				Insecure:    true,
+			},
+			assertErr: func(t assert.TestingT, err error, msgAndArgs ...any) bool {
+				// Should get AccessDenied and should not fall back to joining
+				// via the legacy service.
+				return assert.ErrorAs(t, err, new(*trace.AccessDeniedError)) &&
+					assert.NotErrorAs(t, err, new(*joinclient.LegacyJoinError))
+			},
+		},
+		{
+			desc: "proxy direct",
+			joinParams: joinclient.JoinParams{
+				ProxyServer: utils.FromAddr(proxyListener.Addr()),
+				Insecure:    true,
+			},
+			assertErr: func(t assert.TestingT, err error, msgAndArgs ...any) bool {
+				// Should get AccessDenied and should not fall back to joining
+				// via the legacy service.
+				return assert.ErrorAs(t, err, new(*trace.AccessDeniedError)) &&
+					assert.NotErrorAs(t, err, new(*joinclient.LegacyJoinError))
+			},
+		},
+		{
+			desc: "bad auth address",
+			joinParams: joinclient.JoinParams{
+				AuthServers: []utils.NetAddr{utils.FromAddr(badListener.Addr())},
+			},
+			assertErr: func(t assert.TestingT, err error, msgAndArgs ...any) bool {
+				// Should fall back to a legacy join attempt before failing.
+				return assert.ErrorAs(t, err, new(*joinclient.LegacyJoinError))
+			},
+		},
+		{
+			desc: "bad proxy address",
+			joinParams: joinclient.JoinParams{
+				ProxyServer: utils.FromAddr(badListener.Addr()),
+			},
+			assertErr: func(t assert.TestingT, err error, msgAndArgs ...any) bool {
+				// Should fall back to a legacy join attempt before failing.
+				return assert.ErrorAs(t, err, new(*joinclient.LegacyJoinError))
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			joinParams := tc.joinParams
+			joinParams.ID = state.IdentityID{
+				Role:     types.RoleInstance,
+				NodeName: "test",
+			}
+
+			joinParams.Token = "invalid"
+			_, err = joinclient.Join(t.Context(), joinParams)
+			tc.assertErr(t, err)
+		})
+	}
+}
+
 type fakeAuthService struct {
 	*authtest.Server
 }

lib/join/joinclient/join.go

@@ -23,9 +23,11 @@ import (
 	"encoding/pem"
 	"errors"
 	"log/slog"
+	"strings"
 
 	"github.com/gravitational/trace"
 	"golang.org/x/crypto/ssh"
+	"google.golang.org/grpc/status"
 
 	"github.com/gravitational/teleport/api/client/proto"
 	"github.com/gravitational/teleport/api/types"
@@ -61,9 +63,9 @@ func Join(ctx context.Context, params JoinParams) (*JoinResult, error) {
 	}
 	slog.InfoContext(ctx, "Trying to join with the new join service")
 	result, err := joinNew(ctx, params)
-	if trace.IsNotImplemented(err) || errors.As(err, new(*connectionError)) {
+	if trace.IsNotImplemented(err) || isConnectionError(err) {
 		// Fall back to joining via legacy service.
-		slog.InfoContext(ctx, "Falling back to joining via the legacy join service", "error", err)
+		slog.InfoContext(ctx, "Joining via new join service failed, falling back to joining via the legacy join service", "error", err)
 		// Non-bots must provide their own host UUID when joining via legacy service.
 		if params.ID.HostUUID == "" && params.ID.Role != types.RoleBot {
 			hostID, err := hostid.Generate(ctx, params.JoinMethod)
@@ -74,7 +76,10 @@ func Join(ctx context.Context, params JoinParams) (*JoinResult, error) {
 			params.ID.HostUUID = hostID
 		}
 		result, err := LegacyJoin(ctx, params)
-		return result, trace.Wrap(err)
+		if err != nil {
+			return nil, trace.Wrap(&LegacyJoinError{err})
+		}
+		return result, nil
 	}
 	return result, trace.Wrap(err)
 }
@@ -91,34 +96,63 @@ func LegacyJoin(ctx context.Context, params JoinParams) (*JoinResult, error) {
 
 func joinNew(ctx context.Context, params JoinParams) (*JoinResult, error) {
 	if params.AuthClient != nil {
+		slog.InfoContext(ctx, "Attempting to join cluster with existing Auth client")
 		return joinViaAuthClient(ctx, params, params.AuthClient)
 	}
 	if !params.ProxyServer.IsEmpty() {
+		slog.InfoContext(ctx, "Attempting to join cluster via Proxy")
 		return joinViaProxy(ctx, params, params.ProxyServer.String())
 	}
+
 	// params.AuthServers could contain auth or proxy addresses, try both.
 	// params.CheckAndSetDefaults() asserts that this list is not empty when
 	// AuthClient and ProxyServer are both unset.
+	addr := params.AuthServers[0].String()
+	slog := slog.With("addr", addr)
+
+	type strategy struct {
+		name string
+		fn   func() (*JoinResult, error)
+	}
+	proxyStrategy := strategy{
+		name: "proxy",
+		fn: func() (*JoinResult, error) {
+			return joinViaProxy(ctx, params, addr)
+		},
+	}
+	authStrategy := strategy{
+		name: "auth",
+		fn: func() (*JoinResult, error) {
+			return joinViaAuth(ctx, params)
+		},
+	}
+	var strategies []strategy
 	if authjoin.LooksLikeProxy(params.AuthServers) {
-		proxyAddr := params.AuthServers[0].String()
-		slog.InfoContext(ctx, "Attempting to join cluster, address looks like a Proxy", "addr", proxyAddr)
-		result, proxyJoinErr := joinViaProxy(ctx, params, proxyAddr)
-		if proxyJoinErr == nil {
+		slog.InfoContext(ctx, "Attempting to join cluster, address looks like a Proxy")
+		strategies = []strategy{proxyStrategy, authStrategy}
+	} else {
+		slog.InfoContext(ctx, "Attempting to join cluster, address looks like an Auth server")
+		strategies = []strategy{authStrategy, proxyStrategy}
+	}
+
+	var errs []error
+	for i, strat := range strategies { //nolint:misspell // strat is an intentional abbreviation of strategy
+		result, err := strat.fn()
+		switch {
+		case err == nil:
 			return result, nil
+		case !isConnectionError(err):
+			// Non-connection errors are hard failures: return immediately.
+			return nil, trace.Wrap(err, "joining via %s", strat.name)
+		}
+		// Connection error: keep for aggregate and try next strategy (if any).
+		errs = append(errs, trace.Wrap(err, "joining via %s", strat.name))
+		if i+1 < len(strategies) {
+			slog.InfoContext(ctx, "Failed to join cluster with a connection error, will try next method",
+				"method", strat.name, "next_method", strategies[i+1].name)
 		}
-		slog.InfoContext(ctx, "Joining via proxy failed, will try to join via Auth", "error", proxyJoinErr)
-		result, authJoinErr := joinViaAuth(ctx, params)
-		return result, trace.Wrap(authJoinErr)
-	}
-	addr := params.AuthServers[0].String()
-	slog.InfoContext(ctx, "Attempting to join cluster, address looks like an Auth server", "addr", addr)
-	result, authJoinErr := joinViaAuth(ctx, params)
-	if authJoinErr == nil {
-		return result, nil
 	}
-	slog.InfoContext(ctx, "Joining via auth failed, will try to join via Proxy", "error", authJoinErr)
-	result, proxyJoinErr := joinViaProxy(ctx, params, addr)
-	return result, trace.Wrap(proxyJoinErr)
+	return nil, trace.NewAggregate(errs...)
 }
 
 func joinViaProxy(ctx context.Context, params JoinParams, proxyAddr string) (*JoinResult, error) {
@@ -387,3 +421,29 @@ func (e *connectionError) Error() string {
 func (e *connectionError) Unwrap() error {
 	return e.wrapped
 }
+
+func isConnectionError(err error) bool {
+	var ce *connectionError
+	if errors.As(err, &ce) {
+		return true
+	}
+	// It's possible to hit a gRPC status error like this when reading the
+	// first response from the stream if connecting in insecure mode to a proxy
+	// address provided as an auth address.
+	statusErr, ok := status.FromError(err)
+	return ok && strings.Contains(statusErr.Message(), "unexpected HTTP status code")
+}
+
+// LegacyJoinError is returned when the join attempt failed while attempting to
+// join via the legacy join service.
+type LegacyJoinError struct {
+	wrapped error
+}
+
+func (e *LegacyJoinError) Error() string {
+	return e.wrapped.Error()
+}
+
+func (e *LegacyJoinError) Unwrap() error {
+	return e.wrapped
+}