Skip to content

Properly check for SAMLIdPServiceProvider access #33190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
avatus merged 2 commits into from
Oct 11, 2023
Merged

Properly check for SAMLIdPServiceProvider access #33190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
423898a
Properly check for SAMLIdPServiceProvider access
avatus Oct 10, 2023
b75b242
Remove unneeded debug log
avatus Oct 11, 2023
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
31 changes: 11 additions & 20 deletions lib/auth/auth_with_roles.go
View file
Open in desktop
Comment thread
kimlisa marked this conversation as resolved.
Outdated
Original file line number Diff line number Diff line change
Expand Up @@ -1505,23 +1505,10 @@ func (a *ServerWithRoles) GetNode(ctx context.Context, namespace, name string) (
func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.ListUnifiedResourcesRequest) (*proto.ListUnifiedResourcesResponse, error) {
// Fetch full list of resources in the backend.
var (
elapsedFetch time.Duration
elapsedFilter time.Duration
unifiedResources types.ResourcesWithLabels
filteredResources types.ResourcesWithLabels
nextKey string
unifiedResources types.ResourcesWithLabels
nextKey string
)

defer func() {
log.WithFields(logrus.Fields{
"user": a.context.User.GetName(),
"elapsed_fetch": elapsedFetch,
"elapsed_filter": elapsedFilter,
}).Debugf(
"ListUnifiedResources(%v->%v) in %v.",
len(unifiedResources), len(filteredResources), elapsedFetch+elapsedFilter)
}()

filter := services.MatchResourceFilter{
Labels: req.Labels,
SearchKeywords: req.SearchKeywords,
Expand All @@ -1533,8 +1520,6 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L
if err != nil {
return nil, trace.Wrap(err)
}
startFetch := time.Now()
startFilter := time.Now()
if req.PinnedOnly {
prefs, err := a.authServer.GetUserPreferences(ctx, a.context.User.GetName())
if err != nil {
Expand All @@ -1555,7 +1540,15 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L
}
} else {
unifiedResources, nextKey, err = a.authServer.UnifiedResourceCache.IterateUnifiedResources(ctx, func(resource types.ResourceWithLabels) (bool, error) {
if err := resourceChecker.CanAccess(resource); err != nil {
var err error
switch r := resource.(type) {
case types.SAMLIdPServiceProvider:
err = a.action(apidefaults.Namespace, types.KindSAMLIdPServiceProvider, types.VerbList)
default:
err = resourceChecker.CanAccess(r)
}

if err != nil {
if trace.IsAccessDenied(err) {
return false, nil
}
Expand All @@ -1568,8 +1561,6 @@ func (a *ServerWithRoles) ListUnifiedResources(ctx context.Context, req *proto.L
return nil, trace.Wrap(err, "filtering unified resources")
}
}
elapsedFetch = time.Since(startFetch)
elapsedFilter = time.Since(startFilter)

paginatedResources, err := services.MakePaginatedResources(types.KindUnifiedResource, unifiedResources)
if err != nil {
Expand Down
21 changes: 17 additions & 4 deletions lib/auth/auth_with_roles_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4273,7 +4273,7 @@ func TestListUnifiedResources_WithSearch(t *testing.T) {
t.Parallel()
ctx := context.Background()
srv := newTestTLSServer(t)
names := []string{"tifa", "cloud", "aerith", "baret", "cid", "tifa2"}
names := []string{"vivi", "cloud", "aerith", "barret", "cid", "vivi2"}
for i := 0; i < 6; i++ {
name := names[i]
node, err := types.NewServerWithLabels(
Expand All @@ -4293,6 +4293,19 @@ func TestListUnifiedResources_WithSearch(t *testing.T) {
require.NoError(t, err)
require.Len(t, testNodes, 6)

sp := &types.SAMLIdPServiceProviderV1{
ResourceHeader: types.ResourceHeader{
Metadata: types.Metadata{
Name: "tifaSAML",
},
},
Spec: types.SAMLIdPServiceProviderSpecV1{
EntityDescriptor: newEntityDescriptor("tifaSAML"),
EntityID: "tifaSAML",
},
}
require.NoError(t, srv.Auth().CreateSAMLIdPServiceProvider(ctx, sp))

// create user and client
user, _, err := CreateUserAndRole(srv.Auth(), "user", nil, nil)
require.NoError(t, err)
Expand All @@ -4304,13 +4317,13 @@ func TestListUnifiedResources_WithSearch(t *testing.T) {
SortBy: types.SortBy{IsDesc: true, Field: types.ResourceMetadataName},
})
require.NoError(t, err)
require.Len(t, resp.Resources, 2)
require.Len(t, resp.Resources, 1)
Comment thread
kimlisa marked this conversation as resolved.
Outdated
require.Empty(t, resp.NextKey)

// Check that our returned resource has the correct name
for _, resource := range resp.Resources {
r := resource.GetNode()
require.True(t, strings.Contains(r.GetHostname(), "tifa"))
r := resource.GetAppServerOrSAMLIdPServiceProvider()
require.True(t, strings.Contains(r.GetName(), "tifa"))
Comment thread
codingllama marked this conversation as resolved.
Outdated
}
}

Expand Down