mirror of https://github.com/grafana/grafana
auth: add serviceaccount proxy (#76815)
* Add proxy service template * Replace SA srv with proxy for external SA srv * Move service account prefix to a constant * Prevent deletion from external service account * Make SA validation a resusable function * Add protection for creating service accounts * Add protection when updating service accounts * Add IsExternal field for service account * Protect ext service account token generation * Add verbose errors for form name or sa name * add tests * Add logs * Adjusts tests --------- Co-authored-by: Misi <mgyongyosi@users.noreply.github.com> Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>pull/76736/head
linoman
2 years ago
committed by
GitHub
parent
f6a0f6912f
commit
359d84799e
@ -0,0 +1,109 @@ |
||||
package proxy |
||||
|
||||
import ( |
||||
"context" |
||||
"strings" |
||||
|
||||
"github.com/grafana/grafana/pkg/infra/log" |
||||
"github.com/grafana/grafana/pkg/services/apikey" |
||||
"github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" |
||||
"github.com/grafana/grafana/pkg/services/serviceaccounts" |
||||
"github.com/grafana/grafana/pkg/services/serviceaccounts/manager" |
||||
) |
||||
|
||||
// ServiceAccountsProxy is a proxy for the serviceaccounts.Service interface
|
||||
// that is used to add validations to service accounts and protects external
|
||||
// service accounts from being modified by users.
|
||||
|
||||
type ServiceAccountsProxy struct { |
||||
log log.Logger |
||||
proxiedService serviceaccounts.Service |
||||
} |
||||
|
||||
func ProvideServiceAccountsProxy( |
||||
proxiedService *manager.ServiceAccountsService, |
||||
) (*ServiceAccountsProxy, error) { |
||||
s := &ServiceAccountsProxy{ |
||||
log: log.New("serviceaccounts.proxy"), |
||||
proxiedService: proxiedService, |
||||
} |
||||
return s, nil |
||||
} |
||||
|
||||
var _ serviceaccounts.Service = (*ServiceAccountsProxy)(nil) |
||||
|
||||
func (s *ServiceAccountsProxy) AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *serviceaccounts.AddServiceAccountTokenCommand) (*apikey.APIKey, error) { |
||||
sa, err := s.proxiedService.RetrieveServiceAccount(ctx, cmd.OrgId, serviceAccountID) |
||||
if err != nil { |
||||
return nil, err |
||||
} |
||||
|
||||
if isExternalServiceAccount(sa.Login) { |
||||
s.log.Error("unable to create tokens for external service accounts", "serviceAccountID", serviceAccountID) |
||||
return nil, extsvcaccounts.ErrCannotCreateToken |
||||
} |
||||
|
||||
return s.proxiedService.AddServiceAccountToken(ctx, serviceAccountID, cmd) |
||||
} |
||||
|
||||
func (s *ServiceAccountsProxy) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { |
||||
if !isNameValid(saForm.Name) { |
||||
s.log.Error("Unable to create service account with a protected name", "name", saForm.Name) |
||||
return nil, extsvcaccounts.ErrInvalidName |
||||
} |
||||
return s.proxiedService.CreateServiceAccount(ctx, orgID, saForm) |
||||
} |
||||
|
||||
func (s *ServiceAccountsProxy) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error { |
||||
sa, err := s.proxiedService.RetrieveServiceAccount(ctx, orgID, serviceAccountID) |
||||
if err != nil { |
||||
return err |
||||
} |
||||
|
||||
if isExternalServiceAccount(sa.Login) { |
||||
s.log.Error("unable to delete external service accounts", "serviceAccountID", serviceAccountID) |
||||
return extsvcaccounts.ErrCannotBeDeleted |
||||
} |
||||
|
||||
return s.proxiedService.DeleteServiceAccount(ctx, orgID, serviceAccountID) |
||||
} |
||||
|
||||
func (s *ServiceAccountsProxy) RetrieveServiceAccount(ctx context.Context, orgID, serviceAccountID int64) (*serviceaccounts.ServiceAccountProfileDTO, error) { |
||||
sa, err := s.proxiedService.RetrieveServiceAccount(ctx, orgID, serviceAccountID) |
||||
if err != nil { |
||||
return nil, err |
||||
} |
||||
|
||||
sa.IsExternal = isExternalServiceAccount(sa.Login) |
||||
|
||||
return sa, nil |
||||
} |
||||
|
||||
func (s *ServiceAccountsProxy) RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error) { |
||||
return s.proxiedService.RetrieveServiceAccountIdByName(ctx, orgID, name) |
||||
} |
||||
|
||||
func (s *ServiceAccountsProxy) UpdateServiceAccount(ctx context.Context, orgID, serviceAccountID int64, saForm *serviceaccounts.UpdateServiceAccountForm) (*serviceaccounts.ServiceAccountProfileDTO, error) { |
||||
if !isNameValid(*saForm.Name) { |
||||
s.log.Error("Invalid service account name", "name", *saForm.Name) |
||||
return nil, extsvcaccounts.ErrInvalidName |
||||
} |
||||
sa, err := s.proxiedService.RetrieveServiceAccount(ctx, orgID, serviceAccountID) |
||||
if err != nil { |
||||
return nil, err |
||||
} |
||||
if isExternalServiceAccount(sa.Login) { |
||||
s.log.Error("unable to update external service accounts", "serviceAccountID", serviceAccountID) |
||||
return nil, extsvcaccounts.ErrCannotBeUpdated |
||||
} |
||||
|
||||
return s.proxiedService.UpdateServiceAccount(ctx, orgID, serviceAccountID, saForm) |
||||
} |
||||
|
||||
func isNameValid(name string) bool { |
||||
return !strings.HasPrefix(name, extsvcaccounts.ExtSvcPrefix) |
||||
} |
||||
|
||||
func isExternalServiceAccount(login string) bool { |
||||
return strings.HasPrefix(login, serviceaccounts.ServiceAccountPrefix+extsvcaccounts.ExtSvcPrefix) |
||||
} |
||||
@ -0,0 +1,259 @@ |
||||
package proxy |
||||
|
||||
import ( |
||||
"context" |
||||
"testing" |
||||
|
||||
"github.com/grafana/grafana/pkg/infra/log" |
||||
"github.com/grafana/grafana/pkg/services/apikey" |
||||
"github.com/grafana/grafana/pkg/services/extsvcauth/extsvcaccounts" |
||||
"github.com/grafana/grafana/pkg/services/serviceaccounts" |
||||
"github.com/stretchr/testify/assert" |
||||
) |
||||
|
||||
type FakeServiceAccountsService struct { |
||||
ExpectedServiceAccountProfileDTO *serviceaccounts.ServiceAccountProfileDTO |
||||
} |
||||
|
||||
var _ serviceaccounts.Service = (*FakeServiceAccountsService)(nil) |
||||
|
||||
func newServiceAccountServiceFake() *FakeServiceAccountsService { |
||||
return &FakeServiceAccountsService{} |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { |
||||
return nil, nil |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error { |
||||
return nil |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) RetrieveServiceAccount(ctx context.Context, orgID, serviceAccountID int64) (*serviceaccounts.ServiceAccountProfileDTO, error) { |
||||
return f.ExpectedServiceAccountProfileDTO, nil |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error) { |
||||
return 0, nil |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) UpdateServiceAccount(ctx context.Context, orgID, serviceAccountID int64, |
||||
saForm *serviceaccounts.UpdateServiceAccountForm) (*serviceaccounts.ServiceAccountProfileDTO, error) { |
||||
return nil, nil |
||||
} |
||||
|
||||
func (f *FakeServiceAccountsService) AddServiceAccountToken(ctx context.Context, serviceAccountID int64, |
||||
cmd *serviceaccounts.AddServiceAccountTokenCommand) (*apikey.APIKey, error) { |
||||
return nil, nil |
||||
} |
||||
|
||||
func TestProvideServiceAccount_DeleteServiceAccount(t *testing.T) { |
||||
testOrgId := int64(1) |
||||
testServiceAccountId := int64(1) |
||||
serviceMock := newServiceAccountServiceFake() |
||||
svc := ServiceAccountsProxy{ |
||||
log.New("test"), |
||||
serviceMock, |
||||
} |
||||
|
||||
t.Run("should create service account", func(t *testing.T) { |
||||
testCases := []struct { |
||||
description string |
||||
form serviceaccounts.CreateServiceAccountForm |
||||
expectedError error |
||||
}{ |
||||
{ |
||||
description: "should create service account and not return error", |
||||
form: serviceaccounts.CreateServiceAccountForm{ |
||||
Name: "my-service-account", |
||||
}, |
||||
expectedError: nil, |
||||
}, |
||||
{ |
||||
description: "should not allow to create a service account with extsvc prefix", |
||||
form: serviceaccounts.CreateServiceAccountForm{ |
||||
Name: "extsvc-my-service-account", |
||||
}, |
||||
expectedError: extsvcaccounts.ErrInvalidName, |
||||
}, |
||||
} |
||||
|
||||
for _, tc := range testCases { |
||||
t.Run(tc.description, func(t *testing.T) { |
||||
tc := tc |
||||
_, err := svc.CreateServiceAccount(context.Background(), testOrgId, &tc.form) |
||||
assert.Equal(t, err, tc.expectedError, tc.description) |
||||
}) |
||||
} |
||||
}) |
||||
|
||||
t.Run("should delete service account", func(t *testing.T) { |
||||
testCases := []struct { |
||||
description string |
||||
expectedError error |
||||
expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO |
||||
}{ |
||||
{ |
||||
description: "should allow to delete a service account", |
||||
expectedError: nil, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "my-service-account", |
||||
}, |
||||
}, |
||||
{ |
||||
description: "should not allow to delete a service account with sa-extsvc prefix", |
||||
expectedError: extsvcaccounts.ErrCannotBeDeleted, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "sa-extsvc-my-service-account", |
||||
}, |
||||
}, |
||||
} |
||||
|
||||
for _, tc := range testCases { |
||||
t.Run(tc.description, func(t *testing.T) { |
||||
serviceMock.ExpectedServiceAccountProfileDTO = tc.expectedServiceAccount |
||||
err := svc.DeleteServiceAccount(context.Background(), testOrgId, testServiceAccountId) |
||||
assert.Equal(t, err, tc.expectedError, tc.description) |
||||
}) |
||||
} |
||||
}) |
||||
|
||||
t.Run("should retrieve service account with IsExternal field", func(t *testing.T) { |
||||
testCases := []struct { |
||||
description string |
||||
expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO |
||||
expectedIsExternal bool |
||||
}{ |
||||
{ |
||||
description: "should not mark as external", |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "my-service-account", |
||||
}, |
||||
expectedIsExternal: false, |
||||
}, |
||||
{ |
||||
description: "should mark as external", |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "sa-extsvc-my-service-account", |
||||
}, |
||||
expectedIsExternal: true, |
||||
}, |
||||
} |
||||
|
||||
for _, tc := range testCases { |
||||
t.Run(tc.description, func(t *testing.T) { |
||||
serviceMock.ExpectedServiceAccountProfileDTO = tc.expectedServiceAccount |
||||
sa, err := svc.RetrieveServiceAccount(context.Background(), testOrgId, testServiceAccountId) |
||||
assert.NoError(t, err, tc.description) |
||||
assert.Equal(t, tc.expectedIsExternal, sa.IsExternal, tc.description) |
||||
}) |
||||
} |
||||
}) |
||||
|
||||
t.Run("should update service account", func(t *testing.T) { |
||||
nameWithoutProtectedPrefix := "my-updated-service-account" |
||||
nameWithProtectedPrefix := "extsvc-my-updated-service-account" |
||||
testCases := []struct { |
||||
description string |
||||
form serviceaccounts.UpdateServiceAccountForm |
||||
expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO |
||||
expectedError error |
||||
}{ |
||||
{ |
||||
description: "should update a non-external service account with a valid name", |
||||
form: serviceaccounts.UpdateServiceAccountForm{ |
||||
Name: &nameWithoutProtectedPrefix, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "my-service-account", |
||||
}, |
||||
expectedError: nil, |
||||
}, |
||||
{ |
||||
description: "should not allow to update a non-external service account with extsvc prefix", |
||||
form: serviceaccounts.UpdateServiceAccountForm{ |
||||
Name: &nameWithProtectedPrefix, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "my-service-account", |
||||
}, |
||||
expectedError: extsvcaccounts.ErrInvalidName, |
||||
}, |
||||
{ |
||||
description: "should not allow to update an external service account with a valid name", |
||||
form: serviceaccounts.UpdateServiceAccountForm{ |
||||
Name: &nameWithoutProtectedPrefix, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "sa-extsvc-my-service-account", |
||||
}, |
||||
expectedError: extsvcaccounts.ErrCannotBeUpdated, |
||||
}, |
||||
{ |
||||
description: "should not allow to update an external service account with a extsvc prefix", |
||||
form: serviceaccounts.UpdateServiceAccountForm{ |
||||
Name: &nameWithProtectedPrefix, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "sa-extsvc-my-service-account", |
||||
}, |
||||
expectedError: extsvcaccounts.ErrInvalidName, |
||||
}, |
||||
} |
||||
|
||||
for _, tc := range testCases { |
||||
t.Run(tc.description, func(t *testing.T) { |
||||
tc := tc |
||||
serviceMock.ExpectedServiceAccountProfileDTO = tc.expectedServiceAccount |
||||
_, err := svc.UpdateServiceAccount(context.Background(), testOrgId, testServiceAccountId, &tc.form) |
||||
assert.Equal(t, tc.expectedError, err, tc.description) |
||||
}) |
||||
} |
||||
}) |
||||
|
||||
t.Run("should add service account tokens", func(t *testing.T) { |
||||
testCases := []struct { |
||||
description string |
||||
cmd serviceaccounts.AddServiceAccountTokenCommand |
||||
expectedServiceAccount *serviceaccounts.ServiceAccountProfileDTO |
||||
expectedError error |
||||
}{ |
||||
{ |
||||
description: "should allow to create a service account token", |
||||
cmd: serviceaccounts.AddServiceAccountTokenCommand{ |
||||
OrgId: testOrgId, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "my-service-account", |
||||
}, |
||||
expectedError: nil, |
||||
}, |
||||
{ |
||||
description: "should not allow to create a service account token", |
||||
cmd: serviceaccounts.AddServiceAccountTokenCommand{ |
||||
OrgId: testOrgId, |
||||
}, |
||||
expectedServiceAccount: &serviceaccounts.ServiceAccountProfileDTO{ |
||||
Login: "sa-extsvc-my-service-account", |
||||
}, |
||||
expectedError: extsvcaccounts.ErrCannotCreateToken, |
||||
}, |
||||
} |
||||
|
||||
for _, tc := range testCases { |
||||
t.Run(tc.description, func(t *testing.T) { |
||||
tc := tc |
||||
serviceMock.ExpectedServiceAccountProfileDTO = tc.expectedServiceAccount |
||||
_, err := svc.AddServiceAccountToken(context.Background(), testServiceAccountId, &tc.cmd) |
||||
assert.Equal(t, tc.expectedError, err, tc.description) |
||||
}) |
||||
} |
||||
}) |
||||
|
||||
t.Run("should identify service account logins for being external or not", func(t *testing.T) { |
||||
assert.False(t, isExternalServiceAccount("my-service-account")) |
||||
assert.False(t, isExternalServiceAccount("sa-my-service-account")) |
||||
assert.False(t, isExternalServiceAccount("extsvc-my-service-account")) |
||||
assert.True(t, isExternalServiceAccount("sa-extsvc-my-service-account")) |
||||
}) |
||||
} |
||||
Loading…
Reference in new issue