From 74e8491f854fc898cbc4284020c65f6062744e5c Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 8 Jun 2023 12:17:45 -0400 Subject: [PATCH] Disallow updates on all types of KMS workers (#3295) This also slightly changes delete logic and ensures authorized actions match what is allowable. --- internal/daemon/common/worker_list.go | 8 +- .../handlers/workers/worker_service.go | 73 +++++++--- .../handlers/workers/worker_service_test.go | 127 +++++++++++++----- 3 files changed, 157 insertions(+), 51 deletions(-) diff --git a/internal/daemon/common/worker_list.go b/internal/daemon/common/worker_list.go index 0a348eb0b..99a17186f 100644 --- a/internal/daemon/common/worker_list.go +++ b/internal/daemon/common/worker_list.go @@ -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 +} diff --git a/internal/daemon/controller/handlers/workers/worker_service.go b/internal/daemon/controller/handlers/workers/worker_service.go index ea43ad2aa..09d3eddc2 100644 --- a/internal/daemon/controller/handlers/workers/worker_service.go +++ b/internal/daemon/controller/handlers/workers/worker_service.go @@ -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() diff --git a/internal/daemon/controller/handlers/workers/worker_service_test.go b/internal/daemon/controller/handlers/workers/worker_service_test.go index 77d68d54c..144dbe223 100644 --- a/internal/daemon/controller/handlers/workers/worker_service_test.go +++ b/internal/daemon/controller/handlers/workers/worker_service_test.go @@ -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))) }) } }