AuthZ: Further protect admin endpoints #86285
Conversation
IevaVasiljeva
requested review from
diegommm,
mildwonkey and
undef1nd
and removed request for
a team
| @@ -353,6 +353,7 @@ func (s *Service) resolveIdenity(ctx context.Context, orgID int64, namespaceID a | |||
| ID: namespaceID.String(), | |||
| OrgID: orgID, | |||
| ClientParams: authn.ClientParams{ | |||
| AllowGlobalOrg: true, | |||
There was a problem hiding this comment.
Are we suppose to allow service account to use gloabl org?
There was a problem hiding this comment.
They shouldn't be able to do anything in the global org, so any checks in the global org should fail. What happens now though is that FetchSyncedUserHook sets user's org ID to their current org when the signed in user is fetched (code) and it doesn't get reset to 0 by FetchSyncedUserHook (here) because AllowGlobalOrg is set to false. So when we evaluate permissions, we do it in the current org instead of the global org, and the evaluation passes.
pkg/api/admin_users.go
Outdated
| if !c.SignedInUser.IsGrafanaAdmin { | ||
| return response.Error(http.StatusForbidden, "You must be a Grafana Admin to grant and revoke Grafana Admin permissions", nil) | ||
| } |
There was a problem hiding this comment.
An alternative would be to use this middleware for this endpoint. Either way is fine
There was a problem hiding this comment.
Ah nice, I forgot about that one. I'll use the middleware then to slightly centralise the access checks.
pkg/api/api.go
Outdated
| adminUserRoute.Post("/", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersCreate)), routing.Wrap(hs.AdminCreateUser)) | ||
| adminUserRoute.Put("/:id/password", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersPasswordUpdate, userIDScope)), routing.Wrap(hs.AdminUpdateUserPassword)) | ||
| adminUserRoute.Put("/:id/permissions", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersPermissionsUpdate, userIDScope)), routing.Wrap(hs.AdminUpdateUserPermissions)) | ||
| adminUserRoute.Delete("/:id", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersDelete, userIDScope)), routing.Wrap(hs.AdminDeleteUser)) | ||
| adminUserRoute.Post("/:id/disable", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersDisable, userIDScope)), routing.Wrap(hs.AdminDisableUser)) | ||
| adminUserRoute.Post("/:id/enable", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersEnable, userIDScope)), routing.Wrap(hs.AdminEnableUser)) | ||
| adminUserRoute.Get("/:id/quotas", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersQuotasList, userIDScope)), routing.Wrap(hs.GetUserQuotas)) | ||
| adminUserRoute.Put("/:id/quotas/:target", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersQuotasUpdate, userIDScope)), routing.Wrap(hs.UpdateUserQuota)) | ||
|
|
||
| adminUserRoute.Post("/:id/logout", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersLogout, userIDScope)), routing.Wrap(hs.AdminLogoutUser)) | ||
| adminUserRoute.Get("/:id/auth-tokens", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersAuthTokenList, userIDScope)), routing.Wrap(hs.AdminGetUserAuthTokens)) | ||
| adminUserRoute.Post("/:id/revoke-auth-token", authorizeInOrg(ac.UseGlobalOrg, ac.EvalPermission(ac.ActionUsersAuthTokenUpdate, userIDScope)), routing.Wrap(hs.AdminRevokeUserAuthToken)) |
There was a problem hiding this comment.
I believe this fix would allow us to change the scopes to users:id instead of a special global scope! But we would have to confirm that
There was a problem hiding this comment.
Oh, good point! I didn't even think about that. I think you're right.
What is this feature?
Add several security improvements, notably:
AllowGlobalOrgfor service accounts).Why do we need this feature?
To make sure that users and service accounts with local admin permissions can't carry out actions with global (cross-organization) impact.
Which issue(s) does this PR fix?:
Fixes https://github.com/grafana/identity-access-team/issues/644