Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/auth/auth_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion lib/auth/presence/presencev1/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
27 changes: 22 additions & 5 deletions lib/auth/presence/presencev1/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,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")
},
},
Expand All @@ -154,6 +153,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")
},
},
Expand All @@ -175,7 +175,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")
},
},
}
Expand All @@ -184,14 +184,31 @@ 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(),
Comment thread
espadolini marked this conversation as resolved.
"the 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
Expand Down
4 changes: 2 additions & 2 deletions lib/proxy/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions lib/reversetunnelclient/api_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions lib/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 NotFound error if [err] is a NotFound or
// AccessDenied error so as to avoid leaking the existence of secret resources,
// for other error types it returns the original error.
Comment thread
nklaassen marked this conversation as resolved.
Outdated
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)
Expand Down