diff --git a/integration/integration_test.go b/integration/integration_test.go index b8c83a8b22b48..9a5f6eadb8ad0 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -2441,7 +2441,7 @@ func testInvalidLogins(t *testing.T, suite *integrationTestSuite) { require.NoError(t, err) err = tc.SSH(context.Background(), cmd, false) - require.ErrorIs(t, err, trace.NotFound("failed to dial target host\n\tcluster \"wrong-site\" is not found")) + require.ErrorIs(t, err, trace.NotFound("failed to dial target host\n\tlooking up remote cluster \"wrong-site\"\n\t\tnot found")) } // TestTwoClustersTunnel creates two teleport clusters: "a" and "b" and creates a diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 9185debd91d46..05b15b0bbb272 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -4947,7 +4947,7 @@ func (a *ServerWithRoles) GetRemoteCluster(ctx context.Context, clusterName stri } cluster, err := a.authServer.GetRemoteCluster(ctx, clusterName) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } if err := a.context.Checker.CheckAccessToRemoteCluster(cluster); err != nil { return nil, utils.OpaqueAccessDenied(err) diff --git a/lib/auth/presence/presencev1/service.go b/lib/auth/presence/presencev1/service.go index 2a55ffd8279f0..cebb0ba1f6940 100644 --- a/lib/auth/presence/presencev1/service.go +++ b/lib/auth/presence/presencev1/service.go @@ -126,7 +126,7 @@ func (s *Service) GetRemoteCluster( rc, err := s.backend.GetRemoteCluster(ctx, req.Name) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } if err := authCtx.Checker.CheckAccessToRemoteCluster(rc); err != nil { diff --git a/lib/auth/presence/presencev1/service_test.go b/lib/auth/presence/presencev1/service_test.go index 38b0720a781d8..4c46e85a058a2 100644 --- a/lib/auth/presence/presencev1/service_test.go +++ b/lib/auth/presence/presencev1/service_test.go @@ -28,6 +28,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/gravitational/trace" + "github.com/gravitational/trace/trail" "github.com/jonboulle/clockwork" "github.com/stretchr/testify/require" "google.golang.org/protobuf/testing/protocmp" @@ -143,7 +144,6 @@ func TestGetRemoteCluster(t *testing.T) { Name: matchingRC.GetName(), }, assertError: func(t require.TestingT, err error, i ...interface{}) { - // Opaque no permission presents as not found require.True(t, trace.IsAccessDenied(err), "error should be access denied") }, }, @@ -154,6 +154,7 @@ func TestGetRemoteCluster(t *testing.T) { Name: notMatchingRC.GetName(), }, assertError: func(t require.TestingT, err error, i ...interface{}) { + // Opaque no permission presents as not found require.True(t, trace.IsNotFound(err), "error should be not found") }, }, @@ -175,7 +176,7 @@ func TestGetRemoteCluster(t *testing.T) { Name: "non-existent", }, assertError: func(t require.TestingT, err error, i ...interface{}) { - require.True(t, trace.IsNotFound(err), "error should be bad parameter") + require.True(t, trace.IsNotFound(err), "error should be not found") }, }, } @@ -184,14 +185,33 @@ func TestGetRemoteCluster(t *testing.T) { client, err := srv.NewClient(auth.TestUser(tt.user)) require.NoError(t, err) - bot, err := client.PresenceServiceClient().GetRemoteCluster(ctx, tt.req) + rc, err := client.PresenceServiceClient().GetRemoteCluster(ctx, tt.req) tt.assertError(t, err) if tt.want != nil { - // Check that the returned bot matches - require.Empty(t, cmp.Diff(tt.want, bot, protocmp.Transform())) + // Check that the returned remote cluster matches + require.Empty(t, cmp.Diff(tt.want, rc, protocmp.Transform())) } }) } + + t.Run("doesnt exist and no permissions errors match", func(t *testing.T) { + client, err := srv.NewClient(auth.TestUser(user.GetName())) + require.NoError(t, err) + + _, doesntExistError := client.PresenceServiceClient().GetRemoteCluster(ctx, &presencev1pb.GetRemoteClusterRequest{ + Name: "non-existent", + }) + require.Error(t, doesntExistError) + _, noPermissionsError := client.PresenceServiceClient().GetRemoteCluster(ctx, &presencev1pb.GetRemoteClusterRequest{ + Name: notMatchingRC.GetName(), + }) + require.Error(t, noPermissionsError) + + require.Equal(t, doesntExistError.Error(), noPermissionsError.Error(), + "the error message returned when the rc doesn't exist or when the user has no permission to see it should be indistinguishable") + require.Equal(t, trail.ToGRPC(doesntExistError), trail.ToGRPC(noPermissionsError), + "the gRPC error returned when the rc doesn't exist or when the user has no permission to see it should be indistinguishable") + }) } // TestListRemoteClusters is an integration test that uses a real gRPC diff --git a/lib/proxy/router.go b/lib/proxy/router.go index bbd67acf6d351..35b9e39e1c198 100644 --- a/lib/proxy/router.go +++ b/lib/proxy/router.go @@ -224,7 +224,7 @@ func (r *Router) DialHost(ctx context.Context, clientSrcAddr, clientDstAddr net. if clusterName != r.clusterName { remoteSite, err := r.getRemoteCluster(ctx, clusterName, accessChecker) if err != nil { - return nil, trace.Wrap(err) + return nil, trace.Wrap(err, "looking up remote cluster %q", clusterName) } site = remoteSite } @@ -365,12 +365,12 @@ func (r *Router) getRemoteCluster(ctx context.Context, clusterName string, check site, err := r.siteGetter.GetSite(clusterName) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } rc, err := r.clusterGetter.GetRemoteCluster(ctx, clusterName) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } if err := checker.CheckAccessToRemoteCluster(rc); err != nil { diff --git a/lib/reversetunnelclient/api_with_roles.go b/lib/reversetunnelclient/api_with_roles.go index fea878149a067..4b4eeff886871 100644 --- a/lib/reversetunnelclient/api_with_roles.go +++ b/lib/reversetunnelclient/api_with_roles.go @@ -94,14 +94,14 @@ func (t *TunnelWithRoles) GetSite(clusterName string) (RemoteSite, error) { ctx := context.TODO() cluster, err := t.tunnel.GetSite(clusterName) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } if t.localCluster == cluster.GetName() { return cluster, nil } rc, err := t.access.GetRemoteCluster(ctx, clusterName) if err != nil { - return nil, trace.Wrap(err) + return nil, utils.OpaqueAccessDenied(err) } if err := t.accessChecker.CheckAccessToRemoteCluster(rc); err != nil { return nil, utils.OpaqueAccessDenied(err) diff --git a/lib/utils/utils.go b/lib/utils/utils.go index 6a62b9775a482..ecf13fd63d0d9 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -414,10 +414,11 @@ func IsCertExpiredError(err error) bool { return strings.Contains(trace.Unwrap(err).Error(), "ssh: cert has expired") } -// OpaqueAccessDenied returns a generic NotFound instead of AccessDenied -// so as to avoid leaking the existence of secret resources. +// OpaqueAccessDenied returns a generic [trace.NotFoundError] if [err] is a [trace.NotFoundError] or +// a [trace.AccessDeniedError] so as to avoid leaking the existence of secret resources, +// for other error types it returns the original error. func OpaqueAccessDenied(err error) error { - if trace.IsAccessDenied(err) { + if trace.IsNotFound(err) || trace.IsAccessDenied(err) { return trace.NotFound("not found") } return trace.Wrap(err)