Do not allow PKI-KMS workers to have names updated via API (#3143)

Do not allow PKI-KMS workers to have names updated via API

This would cause their public IDs to change, which we do not want.

Descriptions are also disallowed, since those come from the config file.

This also adds logic to allow upgrading from old KMS method to new, with
a test.

Co-authored-by: Irena Rindos <irenarindos@users.noreply.github.com>

internal/server/public_ids.go

@@ -19,11 +19,11 @@ func newWorkerId(ctx context.Context) (string, error) {
 	return id, nil
 }
 
-// newWorkerIdFromScopeAndName generates a predictable public id based on the
+// NewWorkerIdFromScopeAndName generates a predictable public id based on the
 // scope and the worker name.  This should only be used on kms workers at
 // upsert time.
-func newWorkerIdFromScopeAndName(ctx context.Context, scope, name string) (string, error) {
-	const op = "server.newWorkerIdFromScopeAndName"
+func NewWorkerIdFromScopeAndName(ctx context.Context, scope, name string) (string, error) {
+	const op = "server.NewWorkerIdFromScopeAndName"
 	id, err := db.NewPublicId(globals.WorkerPrefix, db.WithPrngValues([]string{scope, name}))
 	if err != nil {
 		return "", errors.Wrap(ctx, err, "server.newWorkerId")

internal/server/repository_worker.go

@@ -5,6 +5,7 @@ package server
 
 import (
 	"context"
+	stderrors "errors"
 	"fmt"
 	"strings"
 
@@ -22,6 +23,8 @@ import (
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
+var ErrCannotUpdateKmsWorkerViaApi = stderrors.New("cannot update a kms worker's basic information via api")
+
 // DeleteWorker will delete a worker from the repository.
 func (r *Repository) DeleteWorker(ctx context.Context, publicId string, _ ...Option) (int, error) {
 	const op = "server.(Repository).DeleteWorker"
@@ -282,7 +285,7 @@ func (r *Repository) UpsertWorkerStatus(ctx context.Context, worker *Worker, opt
 		// are treated as the same worker.  Allowing this only for kms workers
 		// also ensures that we maintain the unique name constraint between pki
 		// workers and kms workers.
-		workerId, err = newWorkerIdFromScopeAndName(ctx, worker.GetScopeId(), worker.GetName())
+		workerId, err = NewWorkerIdFromScopeAndName(ctx, worker.GetScopeId(), worker.GetName())
 		if err != nil || workerId == "" {
 			return nil, errors.Wrap(ctx, err, op, errors.WithMsg("error creating a worker id"))
 		}
@@ -299,9 +302,9 @@ func (r *Repository) UpsertWorkerStatus(ctx context.Context, worker *Worker, opt
 			switch {
 			case opts.withKeyId != "":
 				// This case goes first because a key ID being supplied should
-				// be a clear indicator that we are working with a PKI
-				// workerClone, and a lack of one a clear indication we are
-				// working with a KMS workerClone.
+				// be a clear indicator that we are working with a PKI (or
+				// KMS-PKI) workerClone, and a lack of one a clear indication we
+				// are working with a KMS workerClone.
 				//
 				// Note: unlike in the below case, this purposefully leaves out
 				// "description" since we want description changes for (non
@@ -420,11 +423,12 @@ func setWorkerTags(ctx context.Context, w db.Writer, id string, ts TagSource, ta
 
 // UpdateWorker will update a worker in the repository and return the resulting
 // worker. fieldMaskPaths provides field_mask.proto paths for fields that should
-// be updated.  Fields will be set to NULL if the field is a zero value and
+// be updated. Fields will be set to NULL if the field is a zero value and
 // included in fieldMask. Name, Description, and Address are the only updatable
 // fields, if no updatable fields are included in the fieldMaskPaths, then an
-// error is returned.  If any paths besides those listed above are included in
-// the path then an error is returned.
+// error is returned. If any paths besides those listed above are included in
+// the path then an error is returned. If the worker is a KMS worker (whether
+// via the old registration method or pki-kms) name updates will be disallowed.
 func (r *Repository) UpdateWorker(ctx context.Context, worker *Worker, version uint32, fieldMaskPaths []string, opt ...Option) (*Worker, int, error) {
 	const (
 		nameField = "name"
@@ -470,6 +474,26 @@ func (r *Repository) UpdateWorker(ctx context.Context, worker *Worker, version u
 		db.StdRetryCnt,
 		db.ExpBackoff{},
 		func(reader db.Reader, w db.Writer) error {
+			// First we need to do a lookup so we can validate that this
+			// function is not being used for workers registered via KMS-PKI
+			// means
+			wAgg := &workerAggregate{PublicId: worker.GetPublicId()}
+			if err := reader.LookupById(ctx, wAgg); err != nil {
+				return errors.Wrap(ctx, err, op)
+			}
+			// If it's a KMS-PKI worker we do not want to allow
+			// name/description/other updates via the API, it should only come
+			// in via the upsert mechanism via status updates. If the public ID
+			// is predictably generated in the KMS fashion, it's a KMS-PKI
+			// worker.
+			workerId, err := NewWorkerIdFromScopeAndName(ctx, wAgg.ScopeId, wAgg.Name)
+			if err != nil {
+				return errors.Wrap(ctx, err, op, errors.WithMsg("error generating worker id in kms-pki name check case"))
+			}
+			if workerId == worker.PublicId {
+				return errors.Wrap(ctx, ErrCannotUpdateKmsWorkerViaApi, op, errors.WithCode(errors.InvalidParameter))
+			}
+
 			worker := worker.clone()
 			rowsUpdated, err = w.Update(ctx, worker, dbMask, nullFields, db.WithVersion(&version),
 				db.WithWhere("server_worker.type = 'pki'"))
@@ -484,7 +508,7 @@ func (r *Repository) UpdateWorker(ctx context.Context, worker *Worker, version u
 				return errors.New(ctx, errors.MultipleRecords, op, "more than 1 resource would have been updated")
 			}
 
-			wAgg := &workerAggregate{PublicId: worker.GetPublicId()}
+			wAgg = &workerAggregate{PublicId: worker.GetPublicId()}
 			if err := reader.LookupById(ctx, wAgg); err != nil {
 				return errors.Wrap(ctx, err, op)
 			}

internal/server/repository_worker_test.go

@@ -297,6 +297,7 @@ func TestUpsertWorkerStatus(t *testing.T) {
 		workers, err := repo.ListWorkers(ctx, []string{scope.Global.String()}, server.WithActiveWorkers(true))
 		require.NoError(t, err)
 		assert.Len(t, workers, 1)
+		assert.Equal(t, server.KmsWorkerType.String(), workers[0].Type)
 
 		// update again with a shutdown state
 		wStatus3 := server.NewWorker(scope.Global.String(),
@@ -335,6 +336,7 @@ func TestUpsertWorkerStatus(t *testing.T) {
 		assert.Equal(t, uint32(1), worker.Version)
 		assert.Equal(t, "pki_address", worker.GetAddress())
 		assert.Equal(t, "test-version", worker.ReleaseVersion)
+		assert.Equal(t, server.PkiWorkerType.String(), worker.Type)
 	})
 
 	failureCases := []struct {
@@ -1226,6 +1228,7 @@ func TestRepository_UpdateWorker(t *testing.T) {
 	pkiCases := []struct {
 		name         string
 		modifyWorker func(*testing.T, *server.Worker)
+		newIdFunc    func(context.Context, string, string) func(context.Context) (string, error)
 		path         []string
 		assertGot    func(*testing.T, *server.Worker)
 		wantErr      bool
@@ -1245,6 +1248,20 @@ func TestRepository_UpdateWorker(t *testing.T) {
 				assert.Greater(t, w.GetUpdateTime().AsTime(), w.GetCreateTime().AsTime())
 			},
 		},
+		{
+			name: "update name against kms-pki",
+			modifyWorker: func(t *testing.T, w *server.Worker) {
+				t.Helper()
+				w.Name = "foo"
+			},
+			newIdFunc: func(ctx context.Context, scopeId, name string) func(ctx context.Context) (string, error) {
+				return func(ctx context.Context) (string, error) {
+					return server.NewWorkerIdFromScopeAndName(ctx, scopeId, name)
+				}
+			},
+			path:    []string{"Name"},
+			wantErr: true,
+		},
 		{
 			name: "update description",
 			modifyWorker: func(t *testing.T, w *server.Worker) {
@@ -1260,10 +1277,29 @@ func TestRepository_UpdateWorker(t *testing.T) {
 				assert.Greater(t, w.GetUpdateTime().AsTime(), w.GetCreateTime().AsTime())
 			},
 		},
+		{
+			name: "update description against kms-pki",
+			modifyWorker: func(t *testing.T, w *server.Worker) {
+				t.Helper()
+				w.Description = "foo"
+			},
+			newIdFunc: func(ctx context.Context, scopeId, name string) func(ctx context.Context) (string, error) {
+				return func(ctx context.Context) (string, error) {
+					return server.NewWorkerIdFromScopeAndName(ctx, scopeId, name)
+				}
+			},
+			path:    []string{"Description"},
+			wantErr: true,
+		},
 	}
 	for _, tt := range pkiCases {
 		t.Run(tt.name, func(t *testing.T) {
-			wkr := server.TestPkiWorker(t, conn, wrapper)
+			name := strings.ReplaceAll(tt.name, " ", "-")
+			opts := []server.Option{server.WithName(name)}
+			if tt.newIdFunc != nil {
+				opts = append(opts, server.WithNewIdFunc(tt.newIdFunc(ctx, scope.Global.String(), name)))
+			}
+			wkr := server.TestPkiWorker(t, conn, wrapper, opts...)
 			defer func() {
 				_, err := repo.DeleteWorker(ctx, wkr.GetPublicId())
 				require.NoError(t, err)

internal/server/repository_workerauth.go

@@ -159,7 +159,7 @@ func StoreNodeInformationTx(ctx context.Context, reader db.Reader, writer db.Wri
 			WithReleaseVersion(fmt.Sprintf("Boundary v%s", wci.BoundaryVersion)),
 		)
 		wConf.Type = PkiWorkerType.String()
-		wConf.PublicId, err = newWorkerIdFromScopeAndName(ctx, scopeId, wci.Name)
+		wConf.PublicId, err = NewWorkerIdFromScopeAndName(ctx, scopeId, wci.Name)
 		if err != nil || wConf.PublicId == "" {
 			return errors.Wrap(ctx, err, op, errors.WithMsg("error creating a worker id"))
 		}
@@ -167,6 +167,31 @@ func StoreNodeInformationTx(ctx context.Context, reader db.Reader, writer db.Wri
 			Target: db.Columns{"public_id"},
 			Action: db.SetColumns([]string{"release_version", "name", "description"}),
 		}
+
+		// Before going ahead with this, we need to see if this is an upgrade
+		// case from the old KMS auth method. If so we need to delete first as
+		// type is immutable.
+		lookupWorker := allocWorker()
+		lookupWorker.PublicId = wConf.PublicId
+		err := reader.LookupById(ctx, &lookupWorker)
+		switch {
+		case err == nil:
+			if lookupWorker.Type == KmsWorkerType.String() {
+				// Delete it
+				rowsDeleted, err := writer.Delete(ctx, lookupWorker)
+				if err != nil {
+					return errors.Wrap(ctx, err, op, errors.WithMsg("error deleting kms typed worker with matching id"))
+				}
+				if rowsDeleted != 1 {
+					return errors.New(ctx, errors.NotSpecificIntegrity, op, fmt.Sprintf("deleted %d rows, expected to delete 1", rowsDeleted))
+				}
+			}
+		case errors.Convert(err) != nil && errors.Convert(err).Code == errors.RecordNotFound:
+			// all good
+		default:
+			return errors.Wrap(ctx, err, op)
+		}
+
 		var withRowsAffected int64
 		err = writer.Create(
 			ctx,