From d8d0b50bd5f3da1d39a891516db6a106bab0bdb6 Mon Sep 17 00:00:00 2001 From: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Date: Fri, 5 Apr 2024 15:04:07 -0400 Subject: [PATCH] [v15] Fix flaky test `TestProxySSHJumpHost` (#40232) * Use testServer.MakeTestServer to fix issue with parallel runs using the same ports. * Add deprecation comments for test server helpers outside of tools/teleport/testenv. * Update tool/teleport/testenv/test_server.go Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * Disable SSH resumption Enable LoadAllCAs Update comments * Prevent context.Canceled from being wrapped across transport stream trail doesn't properly handle conversions to and from errors that are context.Canceled which results in code paths trying to evaluate errors.Is(err, context.Canceled) failing. The error recevied is an interceptors.RemoteError, which wraps a trace.TraceErr, which contains a status.Status with codes.Canceled. To work around this until trail is updated the server half of the stream was updated to return an io.EOF if if encouters a canceled error. * Fix mockSSOLogin. --------- Co-authored-by: joerger --- api/utils/grpc/stream/stream.go | 5 +- tool/teleport/testenv/test_server.go | 33 ++++- tool/tsh/common/app_aws_test.go | 1 + tool/tsh/common/proxy_test.go | 175 ++++++++++++++------------- tool/tsh/common/tsh_helper_test.go | 1 + tool/tsh/common/tsh_test.go | 2 + 6 files changed, 133 insertions(+), 84 deletions(-) diff --git a/api/utils/grpc/stream/stream.go b/api/utils/grpc/stream/stream.go index a74f411b585fd..b1af771379313 100644 --- a/api/utils/grpc/stream/stream.go +++ b/api/utils/grpc/stream/stream.go @@ -22,6 +22,8 @@ import ( "time" "github.com/gravitational/trace" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // MaxChunkSize is the maximum number of bytes to send in a single data message. @@ -70,9 +72,10 @@ func (c *ReadWriter) Read(b []byte) (n int, err error) { if len(c.rBytes) == 0 { data, err := c.source.Recv() - if errors.Is(err, io.EOF) { + if errors.Is(err, io.EOF) || status.Code(err) == codes.Canceled { return 0, io.EOF } + if err != nil { return 0, trace.ConnectionProblem(trace.Wrap(err), "failed to receive from source: %v", err) } diff --git a/tool/teleport/testenv/test_server.go b/tool/teleport/testenv/test_server.go index e6024f57c5990..fedb9a3dc3132 100644 --- a/tool/teleport/testenv/test_server.go +++ b/tool/teleport/testenv/test_server.go @@ -30,6 +30,7 @@ import ( "time" "github.com/gravitational/trace" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/breaker" @@ -276,7 +277,7 @@ type TestServerOptFunc func(o *TestServersOpts) func WithBootstrap(bootstrap ...types.Resource) TestServerOptFunc { return func(o *TestServersOpts) { - o.Bootstrap = bootstrap + o.Bootstrap = append(o.Bootstrap, bootstrap...) } } @@ -330,6 +331,36 @@ func WithAuthPreference(authPref types.AuthPreference) TestServerOptFunc { }) } +func SetupTrustedCluster(ctx context.Context, t *testing.T, rootServer, leafServer *service.TeleportProcess, additionalRoleMappings ...types.RoleMapping) { + rootProxyAddr, err := rootServer.ProxyWebAddr() + require.NoError(t, err) + rootProxyTunnelAddr, err := rootServer.ProxyTunnelAddr() + require.NoError(t, err) + + tc, err := types.NewTrustedCluster("root-cluster", types.TrustedClusterSpecV2{ + Enabled: true, + Token: staticToken, + ProxyAddress: rootProxyAddr.String(), + ReverseTunnelAddress: rootProxyTunnelAddr.String(), + RoleMap: append(additionalRoleMappings, + types.RoleMapping{ + Remote: "access", + Local: []string{"access"}, + }, + ), + }) + require.NoError(t, err) + + _, err = leafServer.GetAuthServer().UpsertTrustedCluster(ctx, tc) + require.NoError(t, err) + + require.EventuallyWithT(t, func(t *assert.CollectT) { + rt, err := rootServer.GetAuthServer().GetTunnelConnections("leaf") + assert.NoError(t, err) + assert.Len(t, rt, 1) + }, time.Second*10, time.Second) +} + type cliModules struct{} func (p *cliModules) GenerateAccessRequestPromotions(_ context.Context, _ modules.AccessResourcesGetter, _ types.AccessRequest) (*types.AccessRequestAllowedPromotions, error) { diff --git a/tool/tsh/common/app_aws_test.go b/tool/tsh/common/app_aws_test.go index 8efcd3025358e..24204cf892daa 100644 --- a/tool/tsh/common/app_aws_test.go +++ b/tool/tsh/common/app_aws_test.go @@ -199,6 +199,7 @@ func makeUserWithAWSRole(t *testing.T) (types.User, types.Role) { return alice, awsRole } +// deprecated: Use `tools/teleport/testenv.MakeTestServer` instead. func makeTestApplicationServer(t *testing.T, proxy *service.TeleportProcess, apps ...servicecfg.App) *service.TeleportProcess { // Proxy uses self-signed certificates in tests. lib.SetInsecureDevMode(true) diff --git a/tool/tsh/common/proxy_test.go b/tool/tsh/common/proxy_test.go index 4b7340935b9de..da2ee3dad443d 100644 --- a/tool/tsh/common/proxy_test.go +++ b/tool/tsh/common/proxy_test.go @@ -62,6 +62,7 @@ import ( "github.com/gravitational/teleport/lib/service/servicecfg" "github.com/gravitational/teleport/lib/teleagent" "github.com/gravitational/teleport/lib/tlsca" + testserver "github.com/gravitational/teleport/tool/teleport/testenv" ) // TestSSH verifies "tsh ssh" command. @@ -558,101 +559,111 @@ func TestProxySSH(t *testing.T) { // TestProxySSHJumpHost verifies "tsh proxy ssh -J" functionality. func TestProxySSHJumpHost(t *testing.T) { + ctx := context.Background() + lib.SetInsecureDevMode(true) t.Cleanup(func() { lib.SetInsecureDevMode(false) }) createAgent(t) - tests := []struct { - name string - opts []testSuiteOptionFunc - }{ - { - name: "TLS routing enabled for root and leaf", - opts: []testSuiteOptionFunc{ - withRootConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - withLeafConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - }, - }, - { - name: "TLS routing enabled for root and disabled for leaf", - opts: []testSuiteOptionFunc{ - withRootConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - withLeafConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Separate) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - }, - }, - { - name: "TLS routing disabled for root and enabled for leaf", - opts: []testSuiteOptionFunc{ - withRootConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Separate) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - withLeafConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Multiplex) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - }, - }, - { - name: "TLS routing disabled for root and leaf", - opts: []testSuiteOptionFunc{ - withRootConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Separate) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - withLeafConfigFunc(func(cfg *servicecfg.Config) { - cfg.Auth.NetworkingConfig.SetProxyListenerMode(types.ProxyListenerMode_Separate) - cfg.Auth.SessionRecordingConfig.SetMode(types.RecordOff) - }), - }, - }, + listenerModes := []types.ProxyListenerMode{ + types.ProxyListenerMode_Multiplex, + types.ProxyListenerMode_Separate, } - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() + for _, rootListenerMode := range listenerModes { + for _, leafListenerMode := range listenerModes { + rootListenerMode := rootListenerMode + leafListenerMode := leafListenerMode + t.Run(fmt.Sprintf("Proxy listener mode %s for root and %s for leaf", rootListenerMode, leafListenerMode), func(t *testing.T) { + t.Parallel() - s := newTestSuite(t, tc.opts...) + accessUser, err := types.NewUser("access") + require.NoError(t, err) + accessUser.SetRoles([]string{"access"}) - runProxySSHJump := func(opts ...CliOption) error { - proxyRequest := fmt.Sprintf("%s:%d", - s.leaf.Config.Proxy.SSHAddr.Host(), - s.leaf.Config.SSH.Addr.Port(defaults.SSHServerListenPort)) - return Run(context.Background(), []string{ - "--insecure", "proxy", "ssh", "-J", s.leaf.Config.Proxy.WebAddr.Addr, proxyRequest, - }, opts...) - } + user, err := user.Current() + require.NoError(t, err) + accessUser.SetLogins([]string{user.Name}) + + connector := mockConnector(t) + rootServerOpts := []testserver.TestServerOptFunc{ + testserver.WithBootstrap(connector, accessUser), + testserver.WithHostname("node01"), + testserver.WithClusterName(t, "root"), + testserver.WithAuthConfig( + func(cfg *servicecfg.AuthConfig) { + cfg.NetworkingConfig.SetProxyListenerMode(rootListenerMode) + // Disable session recording to prevent writing to disk after the test concludes. + cfg.SessionRecordingConfig.SetMode(types.RecordOff) + // Load all CAs on login so that leaf CA is trusted by clients. + cfg.LoadAllCAs = true + }, + ), + } + rootServer := testserver.MakeTestServer(t, rootServerOpts...) + + leafServerOpts := []testserver.TestServerOptFunc{ + testserver.WithBootstrap(accessUser), + testserver.WithHostname("node02"), + testserver.WithClusterName(t, "leaf"), + testserver.WithAuthConfig( + func(cfg *servicecfg.AuthConfig) { + cfg.NetworkingConfig.SetProxyListenerMode(leafListenerMode) + // Disable session recording to prevent writing to disk after the test concludes. + cfg.SessionRecordingConfig.SetMode(types.RecordOff) + }, + ), + } + leafServer := testserver.MakeTestServer(t, leafServerOpts...) + testserver.SetupTrustedCluster(ctx, t, rootServer, leafServer) - // login to root - tshHome, _ := mustLogin(t, s, s.root.Config.Auth.ClusterName.GetClusterName()) + rootProxyAddr, err := rootServer.ProxyWebAddr() + require.NoError(t, err) + leafProxyAddr, err := leafServer.ProxyWebAddr() + require.NoError(t, err) - // Connect to leaf node though proxy jump host. This should automatically - // reissue leaf certs from the root without explicility switching clusters. - err := runProxySSHJump(setHomePath(tshHome)) - require.NoError(t, err) + // login to root + tshHome := t.TempDir() + err = Run(ctx, []string{ + "--insecure", + "login", + "--proxy", rootProxyAddr.String(), + }, setHomePath(tshHome), func(cf *CLIConf) error { + cf.MockSSOLogin = mockSSOLogin(t, rootServer.GetAuthServer(), accessUser) + cf.AuthConnector = connector.GetName() + return nil + }) + require.NoError(t, err) - // Terminate root cluster. - err = s.root.Close() - require.NoError(t, err) + // Connect through the leaf proxy jumphost. + err = Run(ctx, []string{ + "--debug", + "proxy", + "ssh", + "--no-resume", + "-J", leafProxyAddr.String(), + "node02", + }, setHomePath(tshHome)) + require.NoError(t, err) - // Since we've already retrieved valid leaf certs, we should be able to connect without root. - err = runProxySSHJump(setHomePath(tshHome)) - require.NoError(t, err) - }) + // Terminate root cluster. + err = rootServer.Close() + require.NoError(t, err) + + // We should be able to connect without root online. + err = Run(ctx, []string{ + "--debug", + "--insecure", + "proxy", + "ssh", + "--no-resume", + "-J", leafProxyAddr.String(), + "node02", + }, setHomePath(tshHome)) + require.NoError(t, err) + }) + } } } diff --git a/tool/tsh/common/tsh_helper_test.go b/tool/tsh/common/tsh_helper_test.go index a069fbb685363..2848d0231d1a3 100644 --- a/tool/tsh/common/tsh_helper_test.go +++ b/tool/tsh/common/tsh_helper_test.go @@ -267,6 +267,7 @@ func withValidationFunc(f func(*suite) bool) testSuiteOptionFunc { } } +// deprecated: Use `tools/teleport/testenv.MakeTestServer` instead. func newTestSuite(t *testing.T, opts ...testSuiteOptionFunc) *suite { var options testSuiteOptions for _, opt := range opts { diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go index 3ed0e3af069ba..a198630b5f9a6 100644 --- a/tool/tsh/common/tsh_test.go +++ b/tool/tsh/common/tsh_test.go @@ -3460,6 +3460,7 @@ func withSSHLabel(key, value string) testServerOptFunc { }) } +// deprecated: Use `tools/teleport/testenv.MakeTestServer` instead. func makeTestSSHNode(t *testing.T, authAddr *utils.NetAddr, opts ...testServerOptFunc) *service.TeleportProcess { var options testServersOpts for _, opt := range opts { @@ -3489,6 +3490,7 @@ func makeTestSSHNode(t *testing.T, authAddr *utils.NetAddr, opts ...testServerOp return runTeleport(t, cfg) } +// deprecated: Use `tools/teleport/testenv.MakeTestServer` instead. func makeTestServers(t *testing.T, opts ...testServerOptFunc) (auth *service.TeleportProcess, proxy *service.TeleportProcess) { t.Helper()