Skip to content

Commit

Permalink
Only include system annotations that are present in the requested rol…
Browse files Browse the repository at this point in the history
…es in an access request (#41821)

* fix for greedily applied system annotations

check a user can do a review in SetAccessRequestState

add test for where clause, ensure correct annotations are permitted

test if review conditions are set and deny if access_request can be updated

Where conditions are working as intended, update docs to avoid confusion

* Check search as roles if request is a resource request

* Update test for search_as_roles

* update test and comment SystemAnnotations function

* Add more testcases, sort and deduplicate annotations
  • Loading branch information
lxea committed May 21, 2024
1 parent bbde991 commit 9637086
Show file tree
Hide file tree
Showing 7 changed files with 375 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ spec:
allow:
rules:
- resources: ['access_request']
verbs: ['list', 'read', 'update']
verbs: ['list', 'read']
- resources: ['access_plugin_data']
verbs: ['update']
review_requests:
Expand Down
283 changes: 283 additions & 0 deletions lib/auth/auth_with_roles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7250,6 +7250,289 @@ func TestCreateAccessRequest(t *testing.T) {
}
}

func TestAccessRequestNonGreedyAnnotations(t *testing.T) {
t.Parallel()

paymentsRequester, err := types.NewRole("payments-requester", types.RoleSpecV6{
Allow: types.RoleConditions{
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"services": {"payments"},
"requesting": {"role"},
},
Roles: []string{"payments-access"},
},
},
})
require.NoError(t, err)

paymentsResourceRequester, err := types.NewRole("payments-resource-requester", types.RoleSpecV6{
Allow: types.RoleConditions{
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"services": {"payments"},
"requesting": {"resources"},
},
SearchAsRoles: []string{"payments-access"},
},
},
})
require.NoError(t, err)

paymentsAccess, err := types.NewRole("payments-access", types.RoleSpecV6{
Allow: types.RoleConditions{
NodeLabels: types.Labels{"service": []string{"payments"}},
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"never-get-this": {"true"},
},
},
},
})
require.NoError(t, err)

identityRequester, err := types.NewRole("identity-requester", types.RoleSpecV6{
Allow: types.RoleConditions{
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"services": {"identity"},
"requesting": {"role"},
},
Roles: []string{"identity-access"},
},
},
})
require.NoError(t, err)

identityResourceRequester, err := types.NewRole("identity-resource-requester", types.RoleSpecV6{
Allow: types.RoleConditions{
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"services": {"identity"},
"requesting": {"resources"},
},
SearchAsRoles: []string{"identity-access"},
},
},
})
require.NoError(t, err)

identityAccess, err := types.NewRole("identity-access", types.RoleSpecV6{
Allow: types.RoleConditions{
NodeLabels: types.Labels{"service": []string{"identity"}},
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"never-get-this": {"true"},
},
},
},
})
require.NoError(t, err)

anyResourceRequester, err := types.NewRole("any-requester", types.RoleSpecV6{
Allow: types.RoleConditions{
Request: &types.AccessRequestConditions{
Annotations: map[string][]string{
"any-requestor": {"true"},
},
SearchAsRoles: []string{"identity-access", "payments-access"},
Roles: []string{"identity-access", "payments-access"},
},
},
})

require.NoError(t, err)

roles := []types.Role{
paymentsRequester, paymentsResourceRequester, paymentsAccess,
identityRequester, identityResourceRequester, identityAccess,
anyResourceRequester,
}

paymentsServer, err := types.NewServer("server-payments", types.KindNode, types.ServerSpecV2{})
require.NoError(t, err)
paymentsServer.SetStaticLabels(map[string]string{"service": "payments"})

idServer, err := types.NewServer("server-identity", types.KindNode, types.ServerSpecV2{})
require.NoError(t, err)
idServer.SetStaticLabels(map[string]string{"service": "payments"})

ctx := context.Background()
srv := newTestTLSServer(t)
for _, role := range roles {
_, err := srv.Auth().CreateRole(ctx, role)
require.NoError(t, err)
}

for _, server := range []types.Server{paymentsServer, idServer} {
_, err := srv.Auth().UpsertNode(ctx, server)
require.NoError(t, err)
}

for _, tc := range []struct {
name string
roles []string
requestedRoles []string
requestedResourceIDs []string
expectedAnnotations map[string][]string
errfn require.ErrorAssertionFunc
}{
{
name: "payments-requester requests role, receives payment annotations",
roles: []string{"payments-requester"},
requestedRoles: []string{"payments-access"},
expectedAnnotations: map[string][]string{
"services": {"payments"},
"requesting": {"role"},
},
},
{
name: "payments-resource-requester requests resource, receives payment annotations",
roles: []string{"payments-resource-requester"},
requestedRoles: []string{"payments-access"},
requestedResourceIDs: []string{"server-payments"},
expectedAnnotations: map[string][]string{
"services": {"payments"},
"requesting": {"resources"},
},
},
{
name: "payments-requester requests identity role, receives error",
roles: []string{"payments-requester"},
requestedRoles: []string{"identity-access"},
errfn: require.Error,
},
{
name: "payments-resource-requester requests identity resource, receives error",
roles: []string{"payments-resource-requester"},
requestedRoles: []string{"identity-access"},
requestedResourceIDs: []string{"server-identity"},
errfn: require.Error,
},
{
name: "identity-requester requests role, receives identity annotations",
roles: []string{"identity-requester"},
requestedRoles: []string{"identity-access"},
expectedAnnotations: map[string][]string{
"services": {"identity"},
"requesting": {"role"},
},
},
{
name: "identity-resource-requester requests resource, receives identity annotations",
roles: []string{"identity-resource-requester"},
requestedRoles: []string{"identity-access"},
requestedResourceIDs: []string{"server-identity"},
expectedAnnotations: map[string][]string{
"services": {"identity"},
"requesting": {"resources"},
},
},
{
name: "identity-requester requests paymen role, receives error",
roles: []string{"identity-requester"},
requestedRoles: []string{"payments-access"},
errfn: require.Error,
},
{
name: "identity-resource-requester requests payment resource, receives error",
roles: []string{"identity-resource-requester"},
requestedRoles: []string{"payment-access"},
requestedResourceIDs: []string{"server-identity"},
errfn: require.Error,
},
{
name: "any-requester requests role, receives annotations",
roles: []string{"any-requester"},
requestedRoles: []string{"payments-access"},
expectedAnnotations: map[string][]string{
"any-requestor": {"true"},
},
},
{
name: "any-requester requests role, receives annotations",
roles: []string{"any-requester"},
requestedRoles: []string{"payments-access"},
requestedResourceIDs: []string{"server-payments"},
expectedAnnotations: map[string][]string{
"any-requestor": {"true"},
},
},
{
name: "both payments and identity-requester requests payments role, receives payments annotations",
roles: []string{"identity-requester", "payments-requester"},
requestedRoles: []string{"payments-access"},
expectedAnnotations: map[string][]string{
"requesting": {"role"},
"services": {"payments"},
},
},
{
name: "all requester roles, requests payments role, receives payments and any annotations",
roles: []string{
"identity-requester", "payments-requester",
"identity-resource-requester", "payments-resource-requester",
"any-requester"},
requestedRoles: []string{"payments-access"},
expectedAnnotations: map[string][]string{
"requesting": {"role"},
"services": {"payments"},
"any-requestor": {"true"},
},
},
{
name: "all requester roles, requests payments resource, receives payments and any annotations",
roles: []string{
"identity-requester", "payments-requester",
"identity-resource-requester", "payments-resource-requester",
"any-requester"},
requestedRoles: []string{"payments-access"},
requestedResourceIDs: []string{"server-payments"},
expectedAnnotations: map[string][]string{
"requesting": {"resources"},
"services": {"payments"},
"any-requestor": {"true"},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
user, err := types.NewUser("requester")
require.NoError(t, err)
user.SetRoles(tc.roles)
_, err = srv.Auth().UpsertUser(ctx, user)
require.NoError(t, err)

var req types.AccessRequest
if len(tc.requestedResourceIDs) == 0 {
req, err = types.NewAccessRequest(uuid.NewString(), user.GetName(), tc.requestedRoles...)
} else {
var resourceIds []types.ResourceID
for _, id := range tc.requestedResourceIDs {
resourceIds = append(resourceIds, types.ResourceID{
ClusterName: srv.ClusterName(),
Kind: types.KindNode,
Name: id,
})
}
req, err = types.NewAccessRequestWithResources(uuid.NewString(), user.GetName(), tc.requestedRoles, resourceIds)
}
require.NoError(t, err)

client, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)
res, err := client.CreateAccessRequestV2(ctx, req)
if tc.errfn == nil {
require.NoError(t, err)
require.Equal(t, tc.expectedAnnotations, res.GetSystemAnnotations())
} else {
tc.errfn(t, err)
}

})
}

}

func mustAccessRequest(t *testing.T, user string, state types.RequestState, created, expires time.Time, roles []string, resourceIDs []types.ResourceID) types.AccessRequest {
t.Helper()

Expand Down
50 changes: 44 additions & 6 deletions lib/services/access_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,11 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
// incoming requests must have system annotations attached
// before being inserted into the backend. this is how the
// RBAC system propagates sideband information to plugins.
req.SetSystemAnnotations(m.SystemAnnotations())
systemAnnotations, err := m.SystemAnnotations(req)
if err != nil {
return trace.Wrap(err)
}
req.SetSystemAnnotations(systemAnnotations)

// if no suggested reviewers were provided by the user, then
// use the defaults suggested by the user's static roles.
Expand Down Expand Up @@ -1699,21 +1703,55 @@ Outer:

// SystemAnnotations calculates the system annotations for a pending
// access request.
func (m *RequestValidator) SystemAnnotations() map[string][]string {
func (m *RequestValidator) SystemAnnotations(req types.AccessRequest) (map[string][]string, error) {
annotations := make(map[string][]string)

// allowedAnnotations keeps track of annotations an access request
// can be granted by the roles requested.
allowedAnnotations := make(map[string][]string)
for _, userRole := range m.userState.GetRoles() {
role, err := m.getter.GetRole(context.Background(), userRole)
if err != nil {
return nil, trace.Wrap(err)
}

acr := role.GetAccessRequestConditions(types.Allow)

for _, reqRole := range req.GetRoles() {
// if the requested role is a resource request, the roles
// granted in `search_as_roles` must be used to make the
// access request and so only annotations from those roles should be included
roles := acr.Roles
if len(req.GetRequestedResourceIDs()) != 0 {
roles = acr.SearchAsRoles
}
if slices.Contains(roles, reqRole) {
for k, v := range acr.Annotations {
vals := allowedAnnotations[k]
allowedAnnotations[k] = slices.Concat(vals, v)
}
}
}
}

for k, va := range m.Annotations.Allow {
var filtered []string
for _, v := range va {
if !slices.Contains(m.Annotations.Deny[k], v) {
filtered = append(filtered, v)
if slices.Contains(m.Annotations.Deny[k], v) {
continue
}
if !slices.Contains(allowedAnnotations[k], v) {
continue
}
filtered = append(filtered, v)
}
if len(filtered) == 0 {
continue
}
annotations[k] = filtered
slices.Sort(filtered)
annotations[k] = slices.Compact(filtered)
}
return annotations
return annotations, nil
}

type ValidateRequestOption func(*RequestValidator)
Expand Down
2 changes: 2 additions & 0 deletions lib/services/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,8 @@ func ParseShortcut(in string) (string, error) {
return types.KindCrownJewel, nil
case types.KindVnetConfig:
return types.KindVnetConfig, nil
case types.KindAccessRequest, types.KindAccessRequest + "s", "accessrequest", "accessrequests":
return types.KindAccessRequest, nil
}
return "", trace.BadParameter("unsupported resource: %q - resources should be expressed as 'type/name', for example 'connector/github'", in)
}
Expand Down
5 changes: 5 additions & 0 deletions lib/services/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,11 @@ func TestParseShortcut(t *testing.T) {

"SamL_IDP_sERVICe_proVidER": {expectedOutput: types.KindSAMLIdPServiceProvider},

"access_request": {expectedOutput: types.KindAccessRequest},
"access_requests": {expectedOutput: types.KindAccessRequest},
"accessrequest": {expectedOutput: types.KindAccessRequest},
"accessrequests": {expectedOutput: types.KindAccessRequest},

"unknown_type": {expectedErr: true},
}

Expand Down
Loading

0 comments on commit 9637086

Please sign in to comment.