CyberCoveLLC/threshold
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Disallow updates on all types of KMS workers (#3295)

Browse Source 
This also slightly changes delete logic and ensures authorized actions
match what is allowable.
This commit is contained in:
Jeff Mitchell authored and GitHub committed 2023-06-08 12:17:45 -04:00
1 parent ebcd6d2828
commit 74e8491f85
3 files changed
+157 -51
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

internal/daemon/common/worker_list.go
+6 -2
View File
@@ -89,8 +89,7 @@ func SeparateManagedWorkers(workers WorkerList) (managedWorkers, nonManagedWorke
managedWorkers = make([]*server.Worker, 0, len(workers))
nonManagedWorkers = make([]*server.Worker, 0, len(workers))
for _, worker := range workers {
tags := worker.CanonicalTags()
if len(tags[ManagedWorkerTag]) != 0 {
if IsManagedWorker(worker) {
managedWorkers = append(managedWorkers, worker)
} else {
nonManagedWorkers = append(nonManagedWorkers, worker)
@@ -98,3 +97,8 @@ func SeparateManagedWorkers(workers WorkerList) (managedWorkers, nonManagedWorke
}
return managedWorkers, nonManagedWorkers
}
// IsManagedWorker indicates whether the given worker is managed
func IsManagedWorker(worker *server.Worker) bool {
return len(worker.CanonicalTags()[ManagedWorkerTag]) != 0
}
internal/daemon/controller/handlers/workers/worker_service.go
+54 -19
View File
@@ -324,8 +324,7 @@ func (s Service) DeleteWorker(ctx context.Context, req *pbs.DeleteWorkerRequest)
return nil, err
}
managed, _ := wl.SeparateManagedWorkers(wl.WorkerList{w})
if len(managed) == 1 {
if wl.IsManagedWorker(w) {
return nil, handlers.InvalidArgumentErrorf("Error in provided request.", map[string]string{"id": "Managed workers cannot be deleted."})
}
@@ -348,23 +347,42 @@ func (s Service) UpdateWorker(ctx context.Context, req *pbs.UpdateWorkerRequest)
return nil, authResults.Error
}
w, err := s.updateInRepo(ctx, authResults.Scope.GetId(), req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
w, err := s.getFromRepo(ctx, req.GetId())
if err != nil {
return nil, err
}
possibleKmsWorkerId, err := server.NewWorkerIdFromScopeAndName(ctx, w.GetScopeId(), w.GetName())
if err != nil {
return nil, err
}
// NOTE on the second case: because KMS-authed workers have predictable IDs
// generated from the scope and name, it's functionally equivalent to
// checking the type, but works for both KMS-PKI and old-style KMS workers.
switch {
case wl.IsManagedWorker(w):
return nil, handlers.InvalidArgumentErrorf(
"Error in provided request.",
map[string]string{"id": "Managed workers cannot be updated."},
)
case possibleKmsWorkerId == w.GetPublicId():
return nil, handlers.InvalidArgumentErrorf(
"Error in provided request.",
map[string]string{"id": "KMS workers cannot be updated through the API and must be updated via their configuration file."},
)
}
w, err = s.updateInRepo(ctx, authResults.Scope.GetId(), req.GetId(), req.GetUpdateMask().GetPaths(), req.GetItem())
switch {
case err == nil:
case stderrors.Is(err, server.ErrCannotUpdateKmsWorkerViaApi):
// Treat this like a "bad field" error on name even though we couldn't
// return it in validation without having to make an additional call and
// a lot of additional logic
return nil, handlers.ValidateUpdateRequest(req, req.GetItem(), func() map[string]string {
badFields := make(map[string]string)
if req.GetItem().GetName().GetValue() != "" {
badFields[globals.NameField] = "KMS-registered workers cannot have their name updated via the API."
}
if req.GetItem().GetDescription().GetValue() != "" {
badFields[globals.DescriptionField] = "KMS-registered workers cannot have their description updated via the API."
}
return badFields
}, globals.WorkerPrefix)
// This is an extra check on the repo side, although it should be caught
// by the logic above.
return nil, handlers.InvalidArgumentErrorf(
"Error in provided request.",
map[string]string{"id": "KMS workers cannot be updated through the API and must be updated via their configuration file."},
)
default:
return nil, err
}
@@ -419,6 +437,7 @@ func (s Service) AddWorkerTags(ctx context.Context, req *pbs.AddWorkerTagsReques
if outputFields.Has(globals.AuthorizedActionsField) {
outputOpts = append(outputOpts, handlers.WithAuthorizedActions(authResults.FetchActionSetForId(ctx, w.GetPublicId(), IdActions).Strings()))
}
item, err := s.toProto(ctx, w, outputOpts...)
if err != nil {
return nil, err
@@ -455,6 +474,7 @@ func (s Service) SetWorkerTags(ctx context.Context, req *pbs.SetWorkerTagsReques
if outputFields.Has(globals.AuthorizedActionsField) {
outputOpts = append(outputOpts, handlers.WithAuthorizedActions(authResults.FetchActionSetForId(ctx, w.GetPublicId(), IdActions).Strings()))
}
item, err := s.toProto(ctx, w, outputOpts...)
if err != nil {
return nil, err
@@ -491,6 +511,7 @@ func (s Service) RemoveWorkerTags(ctx context.Context, req *pbs.RemoveWorkerTags
if outputFields.Has(globals.AuthorizedActionsField) {
outputOpts = append(outputOpts, handlers.WithAuthorizedActions(authResults.FetchActionSetForId(ctx, w.GetPublicId(), IdActions).Strings()))
}
item, err := s.toProto(ctx, w, outputOpts...)
if err != nil {
return nil, err
@@ -831,10 +852,14 @@ func (s Service) toProto(ctx context.Context, in *server.Worker, opt ...handlers
if outputFields.Has(globals.ScopeField) {
out.Scope = opts.WithScope
}
if outputFields.Has(globals.AuthorizedActionsField) {
if outputFields.Has(globals.AuthorizedActionsField) && opts.WithAuthorizedActions != nil {
out.AuthorizedActions = opts.WithAuthorizedActions
if in.Type == KmsWorkerType && out.AuthorizedActions != nil {
// KMS workers cannot be updated through the API
possibleKmsWorkerId, err := server.NewWorkerIdFromScopeAndName(ctx, in.GetScopeId(), in.GetName())
if err != nil {
return nil, errors.Wrap(ctx, err, op)
}
// KMS workers cannot be updated through the API
if possibleKmsWorkerId == in.GetPublicId() {
allActions := out.AuthorizedActions
out.AuthorizedActions = make([]string, 0, len(allActions))
for _, act := range allActions {
@@ -843,6 +868,16 @@ func (s Service) toProto(ctx context.Context, in *server.Worker, opt ...handlers
}
}
}
// Managed workers cannot be deleted
if wl.IsManagedWorker(in) {
allActions := out.AuthorizedActions
out.AuthorizedActions = make([]string, 0, len(allActions))
for _, act := range allActions {
if act != action.Delete.String() {
out.AuthorizedActions = append(out.AuthorizedActions, act)
}
}
}
}
if outputFields.Has(globals.AddressField) && in.GetAddress() != "" {
out.Address = in.GetAddress()
internal/daemon/controller/handlers/workers/worker_service_test.go
+97 -30
View File
@@ -107,29 +107,29 @@ func TestGet(t *testing.T) {
return workerAuthRepo, nil
}
kmsWorker := server.TestKmsWorker(t, conn, wrap,
server.WithName("test kms worker names"),
server.WithDescription("test kms worker description"),
server.WithAddress("test kms worker address"),
deprecatedKmsWorker := server.TestKmsWorker(t, conn, wrap,
server.WithName("test deprecated kms worker names"),
server.WithDescription("test deprecated kms worker description"),
server.WithAddress("test deprecated kms worker address"),
server.WithWorkerTags(&server.Tag{Key: "key", Value: "val"}))
kmsAuthzActions := make([]string, len(testAuthorizedActions))
copy(kmsAuthzActions, testAuthorizedActions)
deprecatedKmsAuthzActions := make([]string, len(testAuthorizedActions))
copy(deprecatedKmsAuthzActions, testAuthorizedActions)
wantKmsWorker := &pb.Worker{
Id: kmsWorker.GetPublicId(),
ScopeId: kmsWorker.GetScopeId(),
Scope: &scopes.ScopeInfo{Id: kmsWorker.GetScopeId(), Type: scope.Global.String(), Name: scope.Global.String(), Description: "Global Scope"},
CreatedTime: kmsWorker.CreateTime.GetTimestamp(),
UpdatedTime: kmsWorker.UpdateTime.GetTimestamp(),
Version: kmsWorker.GetVersion(),
Name: wrapperspb.String(kmsWorker.GetName()),
Description: wrapperspb.String(kmsWorker.GetDescription()),
Address: kmsWorker.GetAddress(),
wantDeprecatedKmsWorker := &pb.Worker{
Id: deprecatedKmsWorker.GetPublicId(),
ScopeId: deprecatedKmsWorker.GetScopeId(),
Scope: &scopes.ScopeInfo{Id: deprecatedKmsWorker.GetScopeId(), Type: scope.Global.String(), Name: scope.Global.String(), Description: "Global Scope"},
CreatedTime: deprecatedKmsWorker.CreateTime.GetTimestamp(),
UpdatedTime: deprecatedKmsWorker.UpdateTime.GetTimestamp(),
Version: deprecatedKmsWorker.GetVersion(),
Name: wrapperspb.String(deprecatedKmsWorker.GetName()),
Description: wrapperspb.String(deprecatedKmsWorker.GetDescription()),
Address: deprecatedKmsWorker.GetAddress(),
ActiveConnectionCount: &wrapperspb.UInt32Value{Value: 0},
AuthorizedActions: strutil.StrListDelete(kmsAuthzActions, action.Update.String()),
LastStatusTime: kmsWorker.GetLastStatusTime().GetTimestamp(),
ReleaseVersion: kmsWorker.ReleaseVersion,
AuthorizedActions: strutil.StrListDelete(deprecatedKmsAuthzActions, action.Update.String()),
LastStatusTime: deprecatedKmsWorker.GetLastStatusTime().GetTimestamp(),
ReleaseVersion: deprecatedKmsWorker.ReleaseVersion,
CanonicalTags: map[string]*structpb.ListValue{
"key": structListValue(t, "val"),
},
@@ -148,7 +148,7 @@ func TestGet(t *testing.T) {
// Add config tags to the created worker
pkiWorker, err = repo.UpsertWorkerStatus(context.Background(),
server.NewWorker(pkiWorker.GetScopeId(),
server.WithAddress("test kms worker address"),
server.WithAddress("test pki worker address"),
server.WithWorkerTags(&server.Tag{
Key: "config",
Value: "test",
@@ -182,6 +182,62 @@ func TestGet(t *testing.T) {
DirectlyConnectedDownstreamWorkers: connectedDownstreams,
}
var managedPkiWorkerKeyId string
managedPkiWorkerName := "test managed pki worker name"
managedPkiWorker := server.TestPkiWorker(t, conn, wrap,
server.WithName("test managed pki worker name"),
server.WithDescription("test managed pki worker description"),
server.WithTestPkiWorkerAuthorizedKeyId(&managedPkiWorkerKeyId),
// Passing this function for the ID will allow us to trick it into
// considering it a KMS-authed worker, so we can verify that both update
// and delete are removed
server.WithNewIdFunc(func(context.Context) (string, error) {
return server.NewWorkerIdFromScopeAndName(ctx, scope.Global.String(), managedPkiWorkerName)
}),
)
// Add config tags to the created worker
managedPkiWorker, err = repo.UpsertWorkerStatus(context.Background(),
server.NewWorker(managedPkiWorker.GetScopeId(),
server.WithAddress("test managed pki worker address"),
server.WithWorkerTags(&server.Tag{
Key: wl.ManagedWorkerTag,
Value: "true",
})),
server.WithUpdateTags(true),
server.WithPublicId(managedPkiWorker.GetPublicId()),
server.WithKeyId(managedPkiWorkerKeyId))
require.NoError(t, err)
managedPkiAuthzActions := make([]string, len(testAuthorizedActions))
copy(managedPkiAuthzActions, testAuthorizedActions)
managedPkiAuthzActions = strutil.StrListDelete(managedPkiAuthzActions, action.Update.String())
managedPkiAuthzActions = strutil.StrListDelete(managedPkiAuthzActions, action.Delete.String())
wantManagedPkiWorker := &pb.Worker{
Id: managedPkiWorker.GetPublicId(),
ScopeId: managedPkiWorker.GetScopeId(),
Scope: &scopes.ScopeInfo{Id: managedPkiWorker.GetScopeId(), Type: scope.Global.String(), Name: scope.Global.String(), Description: "Global Scope"},
CreatedTime: managedPkiWorker.CreateTime.GetTimestamp(),
UpdatedTime: managedPkiWorker.UpdateTime.GetTimestamp(),
Version: managedPkiWorker.GetVersion(),
Name: wrapperspb.String(managedPkiWorker.GetName()),
Description: wrapperspb.String(managedPkiWorker.GetDescription()),
Address: managedPkiWorker.GetAddress(),
AuthorizedActions: managedPkiAuthzActions,
ActiveConnectionCount: &wrapperspb.UInt32Value{Value: 0},
LastStatusTime: managedPkiWorker.GetLastStatusTime().GetTimestamp(),
ReleaseVersion: managedPkiWorker.ReleaseVersion,
CanonicalTags: map[string]*structpb.ListValue{
wl.ManagedWorkerTag: structListValue(t, "true"),
},
ConfigTags: map[string]*structpb.ListValue{
wl.ManagedWorkerTag: structListValue(t, "true"),
},
Type: PkiWorkerType,
DirectlyConnectedDownstreamWorkers: connectedDownstreams,
}
cases := []struct {
name string
scopeId string
@@ -190,10 +246,10 @@ func TestGet(t *testing.T) {
err error
}{
{
name: "Get an Existing KMS Worker",
scopeId: kmsWorker.GetScopeId(),
req: &pbs.GetWorkerRequest{Id: kmsWorker.GetPublicId()},
res: &pbs.GetWorkerResponse{Item: wantKmsWorker},
name: "Get an Existing Deprecated KMS Worker",
scopeId: deprecatedKmsWorker.GetScopeId(),
req: &pbs.GetWorkerRequest{Id: deprecatedKmsWorker.GetPublicId()},
res: &pbs.GetWorkerResponse{Item: wantDeprecatedKmsWorker},
},
{
name: "Get an Existing PKI Worker",
@@ -201,6 +257,12 @@ func TestGet(t *testing.T) {
req: &pbs.GetWorkerRequest{Id: pkiWorker.GetPublicId()},
res: &pbs.GetWorkerResponse{Item: wantPkiWorker},
},
{
name: "Get a managed worker",
scopeId: managedPkiWorker.GetScopeId(),
req: &pbs.GetWorkerRequest{Id: managedPkiWorker.GetPublicId()},
res: &pbs.GetWorkerResponse{Item: wantManagedPkiWorker},
},
{
name: "Get a non-existent Worker",
req: &pbs.GetWorkerRequest{Id: globals.WorkerPrefix + "_DoesntExis"},
@@ -954,7 +1016,7 @@ func TestUpdate(t *testing.T) {
// error here.
if wkr.PublicId == pkiKmsWkr.PublicId && basicInfoChange && tc.err == nil {
require.Error(t, gErr)
assert.Contains(t, gErr.Error(), "KMS-registered workers cannot have their")
assert.Contains(t, gErr.Error(), "KMS workers cannot be updated through the API")
assert.True(t, errors.Is(gErr, handlers.ApiErrorWithCode(codes.InvalidArgument)))
return
}
@@ -990,7 +1052,7 @@ func TestUpdate(t *testing.T) {
}
}
func TestUpdate_KMS(t *testing.T) {
func TestUpdate_DeprecatedKMS(t *testing.T) {
conn, _ := db.TestSetup(t, "postgres")
wrapper := db.TestWrapper(t)
kms := kms.TestKms(t, conn, wrapper)
@@ -1024,10 +1086,10 @@ func TestUpdate_KMS(t *testing.T) {
require.NoError(t, err)
cases := []struct {
name string
req *pbs.UpdateWorkerRequest
res *pbs.UpdateWorkerResponse
err error
name string
req *pbs.UpdateWorkerRequest
res *pbs.UpdateWorkerResponse
errContains string
}{
{
name: "Cant set name",
@@ -1039,6 +1101,7 @@ func TestUpdate_KMS(t *testing.T) {
Name: wrapperspb.String("name"),
},
},
errContains: "KMS workers cannot be updated through the API",
},
{
name: "Cant set description",
@@ -1050,6 +1113,7 @@ func TestUpdate_KMS(t *testing.T) {
Description: wrapperspb.String("description"),
},
},
errContains: "KMS workers cannot be updated through the API",
},
{
name: "Cant set address",
@@ -1061,6 +1125,7 @@ func TestUpdate_KMS(t *testing.T) {
Address: "address",
},
},
errContains: "This is a read only field.",
},
}
for _, tc := range cases {
@@ -1071,6 +1136,8 @@ func TestUpdate_KMS(t *testing.T) {
got, gErr := workerService.UpdateWorker(auth.DisabledAuthTestContext(iamRepoFn, scope.Global.String()), req)
assert.Error(t, gErr)
assert.Nil(t, got)
assert.Contains(t, gErr.Error(), tc.errContains)
assert.True(t, errors.Is(gErr, handlers.ApiErrorWithCode(codes.InvalidArgument)))
})
}
}