Service account: Ensure that you can revert only service accounts which you can access (#52626)

* Service account: Ensure that you can revert only service accounts which you can access

* Remove prettier messup with docs

* Remove prettier messup with docs

* Prettier run
This commit is contained in:
Vardan Torosyan 2022-07-22 10:35:01 +02:00 committed by GitHub
parent 5d05d26e12
commit 18867d6d78
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 69 additions and 38 deletions

View File

@ -138,9 +138,9 @@ Content-Type: application/json
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| -------------------- | -------------------- |
| serviceaccounts:read | serviceaccounts:id:1 |
| Action | Scope |
| -------------------- | --------------------- |
| serviceaccounts:read | serviceaccounts:id:\* |
**Example Request**:
@ -179,9 +179,9 @@ Content-Type: application/json
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| --------------------- | -------------------- |
| serviceaccounts:write | serviceaccounts:id:1 |
| Action | Scope |
| --------------------- | --------------------- |
| serviceaccounts:write | serviceaccounts:id:\* |
**Example Request**:
@ -227,9 +227,9 @@ Content-Type: application/json
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| -------------------- | -------------------- |
| serviceaccounts:read | serviceaccounts:id:1 |
| Action | Scope |
| -------------------- | --------------------- |
| serviceaccounts:read | serviceaccounts:id:\* |
**Example Request**:
@ -267,9 +267,9 @@ Content-Type: application/json
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| --------------------- | -------------------- |
| serviceaccounts:write | serviceaccounts:id:1 |
| Action | Scope |
| --------------------- | --------------------- |
| serviceaccounts:write | serviceaccounts:id:\* |
**Example Request**:
@ -306,9 +306,9 @@ Content-Type: application/json
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| --------------------- | -------------------- |
| serviceaccounts:write | serviceaccounts:id:1 |
| Action | Scope |
| --------------------- | --------------------- |
| serviceaccounts:write | serviceaccounts:id:\* |
**Example Request**:
@ -332,7 +332,7 @@ Content-Type: application/json
## Revert service account token to API key
`DELETE /api/serviceaccounts/revert/:keyId`
`DELETE /api/serviceaccounts/:serviceAccountId/revert/:keyId`
This operation will delete the service account and create a legacy API Key for the given `keyId`.
@ -340,14 +340,14 @@ This operation will delete the service account and create a legacy API Key for t
See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation.
| Action | Scope |
| ---------------------- | ----- |
| serviceaccounts:delete | n/a |
| Action | Scope |
| ---------------------- | --------------------- |
| serviceaccounts:delete | serviceaccounts:id:\* |
**Example Request**:
```http
DELETE /api/serviceaccounts/revert/glsa_VVQjot0nijQ59lun6pMZRtsdBXxnFQ9M_77c34a79 HTTP/1.1
DELETE /api/serviceaccounts/1/revert/glsa_VVQjot0nijQ59lun6pMZRtsdBXxnFQ9M_77c34a79 HTTP/1.1
Accept: application/json
Content-Type: application/json
Authorization: Basic YWRtaW46YWRtaW4=

View File

@ -75,8 +75,8 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints() {
accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.MigrateApiKeysToServiceAccounts))
serviceAccountsRoute.Post("/migrate/:keyId", auth(middleware.ReqOrgAdmin,
accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.ConvertToServiceAccount))
serviceAccountsRoute.Post("/revert/:keyId", auth(middleware.ReqOrgAdmin,
accesscontrol.EvalPermission(serviceaccounts.ActionDelete)), routing.Wrap(api.RevertApiKey))
serviceAccountsRoute.Post("/:serviceAccountId/revert/:keyId", auth(middleware.ReqOrgAdmin,
accesscontrol.EvalPermission(serviceaccounts.ActionDelete, serviceaccounts.ScopeID)), routing.Wrap(api.RevertApiKey))
})
}
@ -307,13 +307,17 @@ func (api *ServiceAccountsAPI) ConvertToServiceAccount(ctx *models.ReqContext) r
func (api *ServiceAccountsAPI) RevertApiKey(ctx *models.ReqContext) response.Response {
keyId, err := strconv.ParseInt(web.Params(ctx.Req)[":keyId"], 10, 64)
if err != nil {
return response.Error(http.StatusBadRequest, "Key ID is invalid", err)
return response.Error(http.StatusBadRequest, "key ID is invalid", err)
}
serviceAccountId, err := strconv.ParseInt(web.Params(ctx.Req)[":serviceAccountId"], 10, 64)
if err != nil {
return response.Error(http.StatusBadRequest, "service account ID is invalid", err)
}
if err := api.store.RevertApiKey(ctx.Req.Context(), keyId); err != nil {
return response.Error(http.StatusInternalServerError, "Error reverting to API key", err)
if err := api.store.RevertApiKey(ctx.Req.Context(), serviceAccountId, keyId); err != nil {
return response.Error(http.StatusInternalServerError, "error reverting to API key", err)
}
return response.Success("Reverted service account to API key")
return response.Success("reverted service account to API key")
}
func (api *ServiceAccountsAPI) getAccessControlMetadata(c *models.ReqContext, saIDs map[string]bool) map[string]accesscontrol.Metadata {

View File

@ -463,7 +463,7 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Co
}
// RevertApiKey converts service account token to old API key
func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64) error {
func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, saId int64, keyId int64) error {
query := models.GetApiKeyByIdQuery{ApiKeyId: keyId}
if err := s.sqlStore.GetApiKeyById(ctx, &query); err != nil {
return err
@ -474,6 +474,10 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64
return fmt.Errorf("API key is not service account token")
}
if *key.ServiceAccountId != saId {
return ErrServiceAccountAndTokenMismatch
}
tokens, err := s.ListTokens(ctx, key.OrgId, *key.ServiceAccountId)
if err != nil {
return fmt.Errorf("cannot revert token: %w", err)

View File

@ -2,6 +2,7 @@ package database
import (
"context"
"math/rand"
"testing"
"github.com/grafana/grafana/pkg/infra/kvstore"
@ -270,15 +271,22 @@ func TestStore_MigrateAllApiKeys(t *testing.T) {
func TestStore_RevertApiKey(t *testing.T) {
cases := []struct {
desc string
key tests.TestApiKey
expectedErr error
desc string
key tests.TestApiKey
forceMismatchServiceAccount bool
expectedErr error
}{
{
desc: "service account token should be reverted to api key",
key: tests.TestApiKey{Name: "Test1", Role: models.ROLE_EDITOR, OrgId: 1},
expectedErr: nil,
},
{
desc: "should fail reverting to api key when the token is assigned to a different service account",
key: tests.TestApiKey{Name: "Test1", Role: models.ROLE_EDITOR, OrgId: 1},
forceMismatchServiceAccount: true,
expectedErr: ErrServiceAccountAndTokenMismatch,
},
}
for _, c := range cases {
@ -291,9 +299,23 @@ func TestStore_RevertApiKey(t *testing.T) {
require.NoError(t, err)
key := tests.SetupApiKey(t, db, c.key)
err = store.MigrateApiKey(context.Background(), key.OrgId, key.Id)
err = store.CreateServiceAccountFromApikey(context.Background(), key)
require.NoError(t, err)
err = store.RevertApiKey(context.Background(), key.Id)
var saId int64
if c.forceMismatchServiceAccount {
saId = rand.Int63()
} else {
serviceAccounts, err := store.SearchOrgServiceAccounts(context.Background(), key.OrgId, "", "all", 1, 50, &models.SignedInUser{UserId: 1, OrgId: 1, Permissions: map[int64]map[string][]string{
key.OrgId: {
"serviceaccounts:read": {"serviceaccounts:id:*"},
},
}})
require.NoError(t, err)
saId = serviceAccounts.ServiceAccounts[0].Id
}
err = store.RevertApiKey(context.Background(), saId, key.Id)
if c.expectedErr != nil {
require.ErrorIs(t, err, c.expectedErr)

View File

@ -5,8 +5,9 @@ import (
)
var (
ErrServiceAccountAlreadyExists = errors.New("service account already exists")
ErrServiceAccountTokenNotFound = errors.New("service account token not found")
ErrInvalidTokenExpiration = errors.New("invalid SecondsToLive value")
ErrDuplicateToken = errors.New("service account token with given name already exists in the organization")
ErrServiceAccountAlreadyExists = errors.New("service account already exists")
ErrServiceAccountTokenNotFound = errors.New("service account token not found")
ErrInvalidTokenExpiration = errors.New("invalid SecondsToLive value")
ErrDuplicateToken = errors.New("service account token with given name already exists in the organization")
ErrServiceAccountAndTokenMismatch = errors.New("API token does not belong to the given service account")
)

View File

@ -26,7 +26,7 @@ type Store interface {
HideApiKeysTab(ctx context.Context, orgID int64) error
MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error
MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error
RevertApiKey(ctx context.Context, keyId int64) error
RevertApiKey(ctx context.Context, saId int64, keyId int64) error
ListTokens(ctx context.Context, orgID int64, serviceAccount int64) ([]*models.ApiKey, error)
DeleteServiceAccountToken(ctx context.Context, orgID, serviceAccountID, tokenID int64) error
AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *AddServiceAccountTokenCommand) error

View File

@ -174,7 +174,7 @@ func (s *ServiceAccountsStoreMock) MigrateApiKey(ctx context.Context, orgID int6
return nil
}
func (s *ServiceAccountsStoreMock) RevertApiKey(ctx context.Context, keyId int64) error {
func (s *ServiceAccountsStoreMock) RevertApiKey(ctx context.Context, saId int64, keyId int64) error {
s.Calls.RevertApiKey = append(s.Calls.RevertApiKey, []interface{}{ctx})
return nil
}