From 234bb4fcc0c094b6a087b40f980faeb8fd5f8cee Mon Sep 17 00:00:00 2001 From: irenarindos Date: Mon, 3 Jun 2024 15:06:18 -0400 Subject: [PATCH 1/5] workerauth: satisfy NodeIdLoader interface --- .../01_worker_auth_invariant_trigger.up.sql | 42 ++++ .../server_worker_local_storage_state.sql | 0 .../server/server_worker_worker_auth.sql | 80 +++++++ internal/server/query.go | 19 ++ internal/server/repository_workerauth.go | 205 +++++++++++++++--- internal/server/repository_workerauth_test.go | 122 +++++++++++ 6 files changed, 439 insertions(+), 29 deletions(-) create mode 100644 internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql rename internal/db/sqltest/tests/{worker => server}/server_worker_local_storage_state.sql (100%) create mode 100644 internal/db/sqltest/tests/server/server_worker_worker_auth.sql diff --git a/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql b/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql new file mode 100644 index 0000000000..5ea53286bf --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql @@ -0,0 +1,42 @@ +-- Copyright (c) HashiCorp, Inc. +-- SPDX-License-Identifier: BUSL-1.1 + +begin; +-- update_worker_auth_authorized is a before update trigger function for the +-- worker_auth_authorized table. The worker_auth_authorized table is a child +-- table of server_worker. A row contains a set encryption keys for a +-- server_worker that are unique to that worker. A server_worker can only have +-- two rows in the worker_auth_authorized table: one with a state of 'current' +-- and one with the state of 'previous'. +-- +--- This trigger function ensures that there is only ever one entry +-- with a state of 'current' and one with a state of 'previous'. +create function update_worker_auth_authorized() returns trigger +as $$ +begin + if new.state = 'current' then + perform + from worker_auth_authorized + where state = 'current' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier; + if found then + raise 'current worker auth already exists; cannot set %s to current', new.worker_key_identifier; + end if; + end if; + if new.state = 'previous' then + perform + from worker_auth_authorized + where state = 'previous' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier; + if found then + raise 'previous worker auth already exists; cannot set %s to previous', new.worker_key_identifier; + end if; + end if; + return new; +end; +$$ language plpgsql; +comment on function update_worker_auth_authorized is + 'update_worker_auth_authorized is a before update trigger function for the worker_auth_authorized table.'; + +create trigger update_worker_auth_authorized before update on worker_auth_authorized + for each row execute function update_worker_auth_authorized(); + +commit; \ No newline at end of file diff --git a/internal/db/sqltest/tests/worker/server_worker_local_storage_state.sql b/internal/db/sqltest/tests/server/server_worker_local_storage_state.sql similarity index 100% rename from internal/db/sqltest/tests/worker/server_worker_local_storage_state.sql rename to internal/db/sqltest/tests/server/server_worker_local_storage_state.sql diff --git a/internal/db/sqltest/tests/server/server_worker_worker_auth.sql b/internal/db/sqltest/tests/server/server_worker_worker_auth.sql new file mode 100644 index 0000000000..32aff1a832 --- /dev/null +++ b/internal/db/sqltest/tests/server/server_worker_worker_auth.sql @@ -0,0 +1,80 @@ +-- Copyright (c) HashiCorp, Inc. +-- SPDX-License-Identifier: BUSL-1.1 + +begin; +select plan(15); +select wtt_load('widgets', 'iam', 'kms'); + +insert into server_worker +(public_id, scope_id, type) +values + ('w_1234567891', 'global', 'pki'); + +insert into server_worker +(public_id, scope_id, type) +values + ('w_9876543210', 'global', 'pki'); + +select is(count(*), 1::bigint) from server_worker where public_id = 'w_1234567891'; +select is(count(*), 1::bigint) from server_worker where public_id = 'w_9876543210'; + +-- Insert worker auth records, expect them to be current +insert into worker_auth_authorized +(worker_key_identifier, worker_id, worker_signing_pub_key, worker_encryption_pub_key, controller_encryption_priv_key, key_id) +values + ('key_id_w11', 'w_1234567891', 'signing_pub_key_w11', 'encryption_pub_key_w11', 'controller_encryption_priv_key_w11', 'kdkv___widget'); +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w11' and state='current'; + +insert into worker_auth_authorized +(worker_key_identifier, worker_id, worker_signing_pub_key, worker_encryption_pub_key, controller_encryption_priv_key, key_id) +values + ('key_id_w21', 'w_9876543210', 'signing_pub_key_w21', 'encryption_pub_key_w21', 'controller_encryption_priv_key_w21', 'kdkv___widget'); +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w21' and state='current'; + +-- Test rotation logic. Insert another worker auth record, expect it to be current. +-- The previous record should be marked as previous +insert into worker_auth_authorized +(worker_key_identifier, worker_id, worker_signing_pub_key, worker_encryption_pub_key, controller_encryption_priv_key, key_id) +values + ('key_id_w12', 'w_1234567891', 'signing_pub_key_w12', 'encryption_pub_key_w12', 'controller_encryption_priv_key_w12', 'kdkv___widget'); +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w11' and state='previous'; +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w12' and state='current'; + +insert into worker_auth_authorized +(worker_key_identifier, worker_id, worker_signing_pub_key, worker_encryption_pub_key, controller_encryption_priv_key, key_id) +values + ('key_id_w22', 'w_9876543210', 'signing_pub_key_w22', 'encryption_pub_key_w22', 'controller_encryption_priv_key_w22', 'kdkv___widget'); +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w21' and state='previous'; +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w22' and state='current'; + +-- Perform an update, attempting to set key_id_w11's state to current. This should fail +select throws_ok($$ update worker_auth_authorized + set state = 'current' + where worker_key_identifier = 'key_id_w11'$$); + +-- Perform an update, attempting to set key_id_w12's state to previous. This should fail +select throws_ok($$ update worker_auth_authorized + set state = 'previous' + where worker_key_identifier = 'key_id_w12'$$); + +-- Delete key_id_2 and attempt to set key_id_1 to current. This should succeed +delete from worker_auth_authorized +where worker_key_identifier = 'key_id_w12'; +update worker_auth_authorized +set state = 'current' +where worker_key_identifier = 'key_id_w11'; + +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w11' and state='current'; +select is(count(*), 0::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w12'; + +-- The other worker auth records are unaffected +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w21' and state='previous'; +select is(count(*), 1::bigint) from worker_auth_authorized where worker_key_identifier = 'key_id_w22' and state='current'; + +-- Attempt to set a bogus state. This should fail +select throws_ok($$ update worker_auth_authorized + set state = 'Alaska' + where worker_key_identifier = 'key_id_w11'$$); + +select * from finish(); +rollback; \ No newline at end of file diff --git a/internal/server/query.go b/internal/server/query.go index 9d7dfae37e..1efb06c185 100644 --- a/internal/server/query.go +++ b/internal/server/query.go @@ -34,6 +34,25 @@ const ( select * from worker_auth_authorized where worker_id in (select * from key_id_to_worker_id) ` + getWorkerAuthStateByKeyIdQuery = ` + select state from worker_auth_authorized where worker_key_identifier = @worker_key_identifier + ` + + deleteWorkerAuthByKeyId = ` + with key_id_to_worker_id as ( + select worker_id from worker_auth_authorized where worker_key_identifier = @worker_key_identifier + ) + delete from worker_auth_authorized where state = 'current' and worker_id in (select * from key_id_to_worker_id) + ` + + updateWorkerAuthStateByKeyId = ` + update worker_auth_authorized set state = 'current' where worker_key_identifier = @worker_key_identifier + ` + + getWorkerAuthsByWorkerIdQuery = ` + select * from worker_auth_authorized where worker_id = @worker_id + ` + authorizedWorkerQuery = ` select distinct w.worker_key_identifier from diff --git a/internal/server/repository_workerauth.go b/internal/server/repository_workerauth.go index 05e12ec782..30ce8fbfc3 100644 --- a/internal/server/repository_workerauth.go +++ b/internal/server/repository_workerauth.go @@ -11,9 +11,6 @@ import ( "strconv" "github.com/fatih/structs" - "google.golang.org/protobuf/types/known/structpb" - "google.golang.org/protobuf/types/known/timestamppb" - "github.com/hashicorp/boundary/internal/daemon/cluster" "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/db/timestamp" @@ -27,11 +24,13 @@ import ( "github.com/hashicorp/nodeenrollment/types" "github.com/mitchellh/mapstructure" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/structpb" + "google.golang.org/protobuf/types/known/timestamppb" ) // Ensure we implement the Storage interfaces var ( - _ nodeenrollment.Storage = (*WorkerAuthRepositoryStorage)(nil) + _ nodeenrollment.NodeIdLoader = (*WorkerAuthRepositoryStorage)(nil) ) type rootCertificatesVersion struct { @@ -113,19 +112,16 @@ func (r *WorkerAuthRepositoryStorage) Store(ctx context.Context, msg nodeenrollm // * certificate bundles are stored with a reference to the workerAuth record and issuing root certificate func StoreNodeInformationTx(ctx context.Context, reader db.Reader, writer db.Writer, kmsCache *kms.Kms, scopeId string, node *types.NodeInformation, _ ...Option) error { const op = "server.(WorkerAuthRepositoryStorage).StoreNodeInformationTx" - if isNil(reader) { + switch { + case isNil(reader): return errors.New(ctx, errors.InvalidParameter, op, "missing reader") - } - if isNil(writer) { + case isNil(writer): return errors.New(ctx, errors.InvalidParameter, op, "missing writer") - } - if isNil(kmsCache) { + case isNil(kmsCache): return errors.New(ctx, errors.InvalidParameter, op, "missing kms") - } - if scopeId == "" { + case scopeId == "": return errors.New(ctx, errors.InvalidParameter, op, "missing scope id") - } - if node == nil { + case node == nil: return errors.New(ctx, errors.InvalidParameter, op, "missing NodeInformation") } @@ -250,6 +246,43 @@ func StoreNodeInformationTx(ctx context.Context, reader db.Reader, writer db.Wri return new(types.DuplicateRecordError) } + // It's possible a connection dropped during a rotate credentials response, so the control plane's stored + // previous and current WorkerAuth records may not match what the worker has stored. + // Check what the worker indicates is its previous key and fix what we have stored before inserting the new record. + if node.PreviousEncryptionKey != nil { + query := getWorkerAuthStateByKeyIdQuery + rows, err := reader.Query(ctx, query, []any{sql.Named("worker_key_identifier", node.PreviousEncryptionKey.KeyId)}) + if err != nil { + return errors.Wrap(ctx, err, op) + } + defer rows.Close() + + var state string + for rows.Next() { + if err := reader.ScanRows(ctx, rows, &state); err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("scan row failed")) + } + } + if err := rows.Err(); err != nil { + return errors.Wrap(ctx, err, op) + } + + // If it's previous... delete current, as it's incorrect and set this one to current to + // ensure proper state rotation on store + if state == previousWorkerAuthState { + query := deleteWorkerAuthByKeyId + _, err := writer.Exec(ctx, query, []any{sql.Named("worker_key_identifier", node.PreviousEncryptionKey.KeyId)}) + if err != nil { + return errors.Wrap(ctx, err, op) + } + query = updateWorkerAuthStateByKeyId + _, err = writer.Exec(ctx, query, []any{sql.Named("worker_key_identifier", node.PreviousEncryptionKey.KeyId)}) + if err != nil { + return errors.Wrap(ctx, err, op) + } + } + } + if err := writer.Create(ctx, &nodeAuth); err != nil { return errors.Wrap(ctx, err, op) } @@ -501,21 +534,31 @@ func (r *WorkerAuthRepositoryStorage) loadNodeInformation(ctx context.Context, n return nodeenrollment.ErrNotFound } - if workerAuthorizedSet.Previous != nil { - priorKey := &types.EncryptionKey{ - KeyId: workerAuthorizedSet.Previous.WorkerKeyIdentifier, - PrivateKeyPkcs8: workerAuthorizedSet.Previous.ControllerEncryptionPrivKey, - PrivateKeyType: types.KEYTYPE_X25519, - PublicKeyPkix: workerAuthorizedSet.Previous.WorkerEncryptionPubKey, - PublicKeyType: types.KEYTYPE_X25519, - } + var thisWorkerAuth *WorkerAuth + switch { + case node.Id == workerAuthorizedSet.Current.WorkerKeyIdentifier: + thisWorkerAuth = workerAuthorizedSet.Current + if workerAuthorizedSet.Previous != nil { + priorKey := &types.EncryptionKey{ + KeyId: workerAuthorizedSet.Previous.WorkerKeyIdentifier, + PrivateKeyPkcs8: workerAuthorizedSet.Previous.ControllerEncryptionPrivKey, + PrivateKeyType: types.KEYTYPE_X25519, + PublicKeyPkix: workerAuthorizedSet.Previous.WorkerEncryptionPubKey, + PublicKeyType: types.KEYTYPE_X25519, + } - node.PreviousEncryptionKey = priorKey + node.PreviousEncryptionKey = priorKey + } + case workerAuthorizedSet.Previous != nil && node.Id == workerAuthorizedSet.Previous.WorkerKeyIdentifier: + thisWorkerAuth = workerAuthorizedSet.Previous + default: + // We shouldn't hit this based on the logic in validateWorkerAuths, but just in case... + return errors.New(ctx, errors.NotSpecificIntegrity, op, "no worker auths match the passed worker key identifier") } - node.EncryptionPublicKeyBytes = workerAuthorizedSet.Current.WorkerEncryptionPubKey - node.CertificatePublicKeyPkix = workerAuthorizedSet.Current.WorkerSigningPubKey - node.RegistrationNonce = workerAuthorizedSet.Current.Nonce + node.EncryptionPublicKeyBytes = thisWorkerAuth.WorkerEncryptionPubKey + node.CertificatePublicKeyPkix = thisWorkerAuth.WorkerSigningPubKey + node.RegistrationNonce = thisWorkerAuth.Nonce // Default values are used for key types node.EncryptionPublicKeyType = types.KEYTYPE_X25519 @@ -523,16 +566,16 @@ func (r *WorkerAuthRepositoryStorage) loadNodeInformation(ctx context.Context, n node.ServerEncryptionPrivateKeyType = types.KEYTYPE_X25519 // Decrypt private key - databaseWrapper, err := r.kms.GetWrapper(ctx, scope.Global.String(), kms.KeyPurposeDatabase, kms.WithKeyId(workerAuthorizedSet.Current.KeyId)) + databaseWrapper, err := r.kms.GetWrapper(ctx, scope.Global.String(), kms.KeyPurposeDatabase, kms.WithKeyId(thisWorkerAuth.KeyId)) if err != nil { return errors.Wrap(ctx, err, op) } - if err = workerAuthorizedSet.Current.decrypt(ctx, databaseWrapper); err != nil { + if err = thisWorkerAuth.decrypt(ctx, databaseWrapper); err != nil { return errors.Wrap(ctx, err, op) } - node.ServerEncryptionPrivateKeyBytes = workerAuthorizedSet.Current.ControllerEncryptionPrivKey + node.ServerEncryptionPrivateKeyBytes = thisWorkerAuth.ControllerEncryptionPrivKey - workerIdInfo := workerAuthWorkerId{WorkerId: workerAuthorizedSet.Current.GetWorkerId()} + workerIdInfo := workerAuthWorkerId{WorkerId: thisWorkerAuth.GetWorkerId()} s := structs.New(workerIdInfo) s.TagName = "mapstructure" state, err := structpb.NewStruct(s.Map()) @@ -716,6 +759,110 @@ func (r *WorkerAuthRepositoryStorage) loadRootCertificates(ctx context.Context, return nil } +// LoadByNodeId implements the NodeIdLoader storage interface +// LoadByNodeId loads values into the given message. The message must be populated +// with the Node ID value. If not found, the returned error should be ErrNotFound. +func (r *WorkerAuthRepositoryStorage) LoadByNodeId(ctx context.Context, msg nodeenrollment.MessageWithNodeId) error { + const op = "server.(WorkerAuthRepositoryStorage).LoadByNodeId" + if err := types.ValidateMessage(msg); err != nil { + return errors.Wrap(ctx, err, op) + } + if msg.GetNodeId() == "" { + return errors.New(ctx, errors.InvalidParameter, op, "given message cannot be loaded as it has no node ID") + } + + var err error + switch t := msg.(type) { + case *types.NodeInformations: + err = r.loadNodeInfosByNodeId(ctx, t) + default: + return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("message type %T not supported for LoadByNodeId", t)) + } + + if err != nil { + if err == nodeenrollment.ErrNotFound { + // Don't wrap as this will confuse things + return err + } + return errors.Wrap(ctx, err, op) + } + + return nil +} + +func (r *WorkerAuthRepositoryStorage) loadNodeInfosByNodeId(ctx context.Context, nodeInfos *types.NodeInformations) error { + const op = "server.(WorkerAuthRepositoryStorage).loadNodeInfosByNodeId" + if nodeInfos == nil { + return errors.New(ctx, errors.InvalidParameter, op, "missing NodeInformations") + } + + query := getWorkerAuthsByWorkerIdQuery + rows, err := r.reader.Query(ctx, query, []any{sql.Named("worker_id", nodeInfos.NodeId)}) + if err != nil { + return errors.Wrap(ctx, err, op) + } + defer rows.Close() + + var workerAuths []*WorkerAuth + for rows.Next() { + var s WorkerAuth + if err := r.reader.ScanRows(ctx, rows, &s); err != nil { + return errors.Wrap(ctx, err, op, errors.WithMsg("scan row failed")) + } + workerAuths = append(workerAuths, &s) + } + if err := rows.Err(); err != nil { + return errors.Wrap(ctx, err, op) + } + + if len(workerAuths) == 0 { + return nodeenrollment.ErrNotFound + } + + nodeInfos.Nodes = make([]*types.NodeInformation, 0, len(workerAuths)) + for _, workerAuth := range workerAuths { + node := &types.NodeInformation{ + Id: workerAuth.WorkerKeyIdentifier, + EncryptionPublicKeyBytes: workerAuth.WorkerEncryptionPubKey, + CertificatePublicKeyPkix: workerAuth.WorkerSigningPubKey, + RegistrationNonce: workerAuth.Nonce, + EncryptionPublicKeyType: types.KEYTYPE_X25519, + CertificatePublicKeyType: types.KEYTYPE_ED25519, + ServerEncryptionPrivateKeyType: types.KEYTYPE_X25519, + } + + // Decrypt private key + databaseWrapper, err := r.kms.GetWrapper(ctx, scope.Global.String(), kms.KeyPurposeDatabase, kms.WithKeyId(workerAuth.KeyId)) + if err != nil { + return errors.Wrap(ctx, err, op) + } + if err = workerAuth.decrypt(ctx, databaseWrapper); err != nil { + return errors.Wrap(ctx, err, op) + } + node.ServerEncryptionPrivateKeyBytes = workerAuth.ControllerEncryptionPrivKey + + workerIdInfo := workerAuthWorkerId{WorkerId: workerAuth.GetWorkerId()} + s := structs.New(workerIdInfo) + s.TagName = "mapstructure" + state, err := structpb.NewStruct(s.Map()) + if err != nil { + return errors.Wrap(ctx, err, op) + } + node.State = state + + // Get cert bundles from the other table + certBundles, err := r.findCertBundles(ctx, workerAuth.WorkerKeyIdentifier) + if err != nil { + return errors.Wrap(ctx, err, op) + } + node.CertificateBundles = certBundles + + nodeInfos.Nodes = append(nodeInfos.Nodes, node) + } + + return nil +} + // Remove implements the Storage interface. // Remove removes the given message. Only the ID field of the message is considered. func (r *WorkerAuthRepositoryStorage) Remove(ctx context.Context, msg nodeenrollment.MessageWithId) error { diff --git a/internal/server/repository_workerauth_test.go b/internal/server/repository_workerauth_test.go index a9636080e6..c88c39b5f1 100644 --- a/internal/server/repository_workerauth_test.go +++ b/internal/server/repository_workerauth_test.go @@ -747,6 +747,128 @@ func TestFilterToAuthorizedWorkerKeyIds(t *testing.T) { assert.Equal(t, []string{keyId2}, got) } +func TestSplitBrain(t *testing.T) { + ctx := context.Background() + require := require.New(t) + + wrapper := db.TestWrapper(t) + conn, _ := db.TestSetup(t, "postgres") + kmsCache := kms.TestKms(t, conn, wrapper) + // Ensures the global scope contains a valid root key + err := kmsCache.CreateKeys(context.Background(), scope.Global.String(), kms.WithRandomReader(rand.Reader)) + require.NoError(err) + wrapper, err = kmsCache.GetWrapper(context.Background(), scope.Global.String(), kms.KeyPurposeDatabase) + require.NoError(err) + require.NotNil(t, wrapper) + + rw := db.New(conn) + + serversRepo, err := NewRepository(ctx, rw, rw, kmsCache) + require.NoError(err) + + require.NoError(err) + wrk := NewWorker(scope.Global.String()) + wrk, err = serversRepo.CreateWorker(ctx, wrk) + require.NoError(err) + require.NotNil(wrk) + + controllerStorage, err := NewRepositoryStorage(ctx, rw, rw, kmsCache) + require.NoError(err) + + _, err = rotation.RotateRootCertificates(ctx, controllerStorage) + require.NoError(err) + + // Create struct to pass in with workerId that will be passed along to storage + state, err := AttachWorkerIdToState(ctx, wrk.PublicId) + require.NoError(err) + + // This happens on the worker + workerStorage, err := file.New(ctx) + require.NoError(err) + initCreds, err := types.NewNodeCredentials(ctx, workerStorage) + require.NoError(err) + // Create request using worker id + fetchReq, err := initCreds.CreateFetchNodeCredentialsRequest(ctx) + require.NoError(err) + registeredNode, err := registration.AuthorizeNode(ctx, controllerStorage, fetchReq, nodeenrollment.WithState(state)) + require.NoError(err) + require.NotNil(registeredNode) + + fetchResp, err := registration.FetchNodeCredentials(ctx, controllerStorage, fetchReq) + require.NoError(err) + initCreds, err = initCreds.HandleFetchNodeCredentialsResponse(ctx, workerStorage, fetchResp) + require.NoError(err) + + // Simulate the auth rotation + // Worker side ---------------------------------- + newCreds, err := types.NewNodeCredentials(ctx, workerStorage, nodeenrollment.WithSkipStorage(true)) + require.NoError(err) + + require.NoError(newCreds.SetPreviousEncryptionKey(initCreds)) + fetchReq, err = newCreds.CreateFetchNodeCredentialsRequest(ctx) + require.NoError(err) + + encFetchReq, err := nodeenrollment.EncryptMessage(ctx, fetchReq, initCreds) + require.NoError(err) + + controllerReq := &types.RotateNodeCredentialsRequest{ + CertificatePublicKeyPkix: initCreds.CertificatePublicKeyPkix, + EncryptedFetchNodeCredentialsRequest: encFetchReq, + } + + // Send request to controller + // Controller side ------------------------------ + resp, err := rotation.RotateNodeCredentials(ctx, controllerStorage, controllerReq) + require.NoError(err) + + // Send response to worker + // Worker side ---------------------------------- + // Simulate response going missing + _ = resp + + // Now simulate the subsequent auth rotation attempt + newNewCreds, err := types.NewNodeCredentials(ctx, workerStorage, nodeenrollment.WithSkipStorage(true)) + require.NoError(err) + require.NotEqual(t, newNewCreds.CertificatePublicKeyPkix, newCreds.CertificatePublicKeyPkix) + + require.NoError(newNewCreds.SetPreviousEncryptionKey(initCreds)) + fetchReq, err = newNewCreds.CreateFetchNodeCredentialsRequest(ctx) + require.NoError(err) + + encFetchReq, err = nodeenrollment.EncryptMessage(ctx, fetchReq, initCreds) + require.NoError(err) + + controllerReq = &types.RotateNodeCredentialsRequest{ + CertificatePublicKeyPkix: initCreds.CertificatePublicKeyPkix, + EncryptedFetchNodeCredentialsRequest: encFetchReq, + } + + // Send request to controller + // Controller side ------------------------------ + // Split brain would fail this, as it used the wrong creds for decryption + resp, err = rotation.RotateNodeCredentials(ctx, controllerStorage, controllerReq) + require.NoError(err) + + // Ensure new key has been stored + keyId, err := nodeenrollment.KeyIdFromPkix(newNewCreds.CertificatePublicKeyPkix) + require.NoError(err) + _, err = types.LoadNodeInformation(ctx, controllerStorage, keyId) + require.NoError(err) + + // Verify that "split brain" creds have been removed from the DB + keyId, err = nodeenrollment.KeyIdFromPkix(newCreds.CertificatePublicKeyPkix) + require.NoError(err) + _, err = types.LoadNodeInformation(ctx, controllerStorage, keyId) + require.Error(err) + require.ErrorIs(err, nodeenrollment.ErrNotFound) + + // Ensure the correct previous key is still stored + keyId, err = nodeenrollment.KeyIdFromPkix(initCreds.CertificatePublicKeyPkix) + require.NoError(err) + _, err = types.LoadNodeInformation(ctx, controllerStorage, keyId) + require.NoError(err) +} + type mockTestWrapper struct { wrapping.Wrapper decryptError bool From 294f4b643f823e69752380d7d1b7e0c6cc8a2568 Mon Sep 17 00:00:00 2001 From: irenarindos Date: Tue, 4 Jun 2024 09:29:06 -0400 Subject: [PATCH 2/5] address feedback and update Node --- go.mod | 2 +- go.sum | 4 ++-- .../postgres/88/01_worker_auth_invariant_trigger.up.sql | 9 ++++++--- internal/db/sqltest/Makefile | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 3d682313db..9fdf48e894 100644 --- a/go.mod +++ b/go.mod @@ -95,7 +95,7 @@ require ( github.com/hashicorp/go-kms-wrapping/extras/kms/v2 v2.0.0-20231219183231-6bac757bb482 github.com/hashicorp/go-rate v0.0.0-20231204194614-cc8d401f70ab github.com/hashicorp/go-version v1.6.0 - github.com/hashicorp/nodeenrollment v0.2.10 + github.com/hashicorp/nodeenrollment v0.2.12 github.com/jackc/pgx/v5 v5.5.5 github.com/jimlambrt/gldap v0.1.10 github.com/kelseyhightower/envconfig v1.4.0 diff --git a/go.sum b/go.sum index 3cd91be45c..84e1347a12 100644 --- a/go.sum +++ b/go.sum @@ -270,8 +270,8 @@ github.com/hashicorp/hcl v1.0.1-vault-5 h1:kI3hhbbyzr4dldA8UdTb7ZlVVlI2DACdCfz31 github.com/hashicorp/hcl v1.0.1-vault-5/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM= github.com/hashicorp/mql v0.1.3 h1:SZdOsocDPovwp3Q5AzoH6s000BD5zcr+hV8xAobOvuo= github.com/hashicorp/mql v0.1.3/go.mod h1:CrbXH2f2ndS1X35x0E8aHdNYc3POYrEWpx/1Q+pq+iw= -github.com/hashicorp/nodeenrollment v0.2.10 h1:KDp5z3wJ3cRmfnNdMmiDrEqN1V4FTtFaeM4AFg8FYfo= -github.com/hashicorp/nodeenrollment v0.2.10/go.mod h1:3TcYV0L7N4EmeGHIQWr/JFAAsV+yHJaX9IQjeff/w5Q= +github.com/hashicorp/nodeenrollment v0.2.12 h1:x5kaSvsXHZ2Y8j9CsRURh4V2/GZtdOFLu/HPeV4zGz8= +github.com/hashicorp/nodeenrollment v0.2.12/go.mod h1:3TcYV0L7N4EmeGHIQWr/JFAAsV+yHJaX9IQjeff/w5Q= github.com/hashicorp/vault/api v1.12.0 h1:meCpJSesvzQyao8FCOgk2fGdoADAnbDu2WPJN1lDLJ4= github.com/hashicorp/vault/api v1.12.0/go.mod h1:si+lJCYO7oGkIoNPAN8j3azBLTn9SjMGS+jFaHd1Cck= github.com/hashicorp/vault/sdk v0.11.0 h1:KP/tBUywaVcvOebAfMPNCCiXKeCNEbm3JauYmrZd7RI= diff --git a/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql b/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql index 5ea53286bf..99fbac273f 100644 --- a/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql +++ b/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql @@ -7,10 +7,13 @@ begin; -- table of server_worker. A row contains a set encryption keys for a -- server_worker that are unique to that worker. A server_worker can only have -- two rows in the worker_auth_authorized table: one with a state of 'current' --- and one with the state of 'previous'. +-- and one with the state of 'previous'. Rotation from current to previous +-- is handled by the insert trigger function, insert_worker_auth_authorized. -- ---- This trigger function ensures that there is only ever one entry --- with a state of 'current' and one with a state of 'previous'. +-- This trigger function ensures that on update there is only ever one entry +-- with a state of 'current' and one with a state of 'previous'. +-- It does this by checking that if we are inserting a `current` or `previous` +-- entry, there does not already exist an entry with the same state for the worker_id. create function update_worker_auth_authorized() returns trigger as $$ begin diff --git a/internal/db/sqltest/Makefile b/internal/db/sqltest/Makefile index 88dea22417..ee5868db77 100644 --- a/internal/db/sqltest/Makefile +++ b/internal/db/sqltest/Makefile @@ -37,7 +37,7 @@ TESTS ?= tests/setup/*.sql \ tests/purge/*.sql \ tests/pagination/*.sql \ tests/policy/*.sql \ - tests/worker/*.sql + tests/server/*.sql POSTGRES_DOCKER_IMAGE_BASE ?= postgres From c207ad443d2004c314284885032916063f4ade9d Mon Sep 17 00:00:00 2001 From: irenarindos Date: Tue, 4 Jun 2024 09:35:42 -0400 Subject: [PATCH 3/5] remove unused var --- internal/server/repository_workerauth_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/server/repository_workerauth_test.go b/internal/server/repository_workerauth_test.go index c88c39b5f1..8b9197cb1d 100644 --- a/internal/server/repository_workerauth_test.go +++ b/internal/server/repository_workerauth_test.go @@ -846,7 +846,7 @@ func TestSplitBrain(t *testing.T) { // Send request to controller // Controller side ------------------------------ // Split brain would fail this, as it used the wrong creds for decryption - resp, err = rotation.RotateNodeCredentials(ctx, controllerStorage, controllerReq) + _, err = rotation.RotateNodeCredentials(ctx, controllerStorage, controllerReq) require.NoError(err) // Ensure new key has been stored From 6025c87e34f1b7649ffbf9528d54c5e55d415957 Mon Sep 17 00:00:00 2001 From: irenarindos Date: Thu, 6 Jun 2024 09:01:37 -0400 Subject: [PATCH 4/5] remove unecessary trigger --- .../01_worker_auth_invariant_trigger.up.sql | 45 ------------------- 1 file changed, 45 deletions(-) delete mode 100644 internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql diff --git a/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql b/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql deleted file mode 100644 index 99fbac273f..0000000000 --- a/internal/db/schema/migrations/oss/postgres/88/01_worker_auth_invariant_trigger.up.sql +++ /dev/null @@ -1,45 +0,0 @@ --- Copyright (c) HashiCorp, Inc. --- SPDX-License-Identifier: BUSL-1.1 - -begin; --- update_worker_auth_authorized is a before update trigger function for the --- worker_auth_authorized table. The worker_auth_authorized table is a child --- table of server_worker. A row contains a set encryption keys for a --- server_worker that are unique to that worker. A server_worker can only have --- two rows in the worker_auth_authorized table: one with a state of 'current' --- and one with the state of 'previous'. Rotation from current to previous --- is handled by the insert trigger function, insert_worker_auth_authorized. --- --- This trigger function ensures that on update there is only ever one entry --- with a state of 'current' and one with a state of 'previous'. --- It does this by checking that if we are inserting a `current` or `previous` --- entry, there does not already exist an entry with the same state for the worker_id. -create function update_worker_auth_authorized() returns trigger -as $$ -begin - if new.state = 'current' then - perform - from worker_auth_authorized - where state = 'current' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier; - if found then - raise 'current worker auth already exists; cannot set %s to current', new.worker_key_identifier; - end if; - end if; - if new.state = 'previous' then - perform - from worker_auth_authorized - where state = 'previous' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier; - if found then - raise 'previous worker auth already exists; cannot set %s to previous', new.worker_key_identifier; - end if; - end if; - return new; -end; -$$ language plpgsql; -comment on function update_worker_auth_authorized is - 'update_worker_auth_authorized is a before update trigger function for the worker_auth_authorized table.'; - -create trigger update_worker_auth_authorized before update on worker_auth_authorized - for each row execute function update_worker_auth_authorized(); - -commit; \ No newline at end of file From 637e2338b261b3672c7e02cbeff172e267e64f8d Mon Sep 17 00:00:00 2001 From: irenarindos Date: Thu, 6 Jun 2024 10:23:04 -0400 Subject: [PATCH 5/5] river formatting --- internal/server/query.go | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/internal/server/query.go b/internal/server/query.go index 1efb06c185..05a3acc276 100644 --- a/internal/server/query.go +++ b/internal/server/query.go @@ -29,28 +29,44 @@ const ( getWorkerAuthsByWorkerKeyIdQuery = ` with key_id_to_worker_id as ( - select worker_id from worker_auth_authorized where worker_key_identifier = @worker_key_identifier + select worker_id + from worker_auth_authorized + where worker_key_identifier = @worker_key_identifier ) - select * from worker_auth_authorized where worker_id in (select * from key_id_to_worker_id) + select * + from worker_auth_authorized + where worker_id in (select * + from key_id_to_worker_id) ` getWorkerAuthStateByKeyIdQuery = ` - select state from worker_auth_authorized where worker_key_identifier = @worker_key_identifier + select state + from worker_auth_authorized + where worker_key_identifier = @worker_key_identifier ` deleteWorkerAuthByKeyId = ` with key_id_to_worker_id as ( - select worker_id from worker_auth_authorized where worker_key_identifier = @worker_key_identifier + select worker_id + from worker_auth_authorized + where worker_key_identifier = @worker_key_identifier ) - delete from worker_auth_authorized where state = 'current' and worker_id in (select * from key_id_to_worker_id) + delete + from worker_auth_authorized + where state = 'current' and worker_id in (select * + from key_id_to_worker_id) ` updateWorkerAuthStateByKeyId = ` - update worker_auth_authorized set state = 'current' where worker_key_identifier = @worker_key_identifier + update worker_auth_authorized + set state = 'current' + where worker_key_identifier = @worker_key_identifier ` getWorkerAuthsByWorkerIdQuery = ` - select * from worker_auth_authorized where worker_id = @worker_id + select * + from worker_auth_authorized + where worker_id = @worker_id ` authorizedWorkerQuery = `