From d33d6f0bb2357e2035c1033cf20e1b3e8440c997 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Wed, 20 Nov 2024 07:03:02 -0600 Subject: [PATCH] Revise LUKS API to expect key slot and salt when escrowing successfully (#23952) #23584 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated tests ~~- [ ] Manual QA for all new/changed functionality~~ Will be tested E2E --- server/datastore/mysql/hosts.go | 15 +++--- server/datastore/mysql/hosts_test.go | 26 ++++------ ...322_AddLuksDataToHostDiskEncryptionKeys.go | 25 +++++++++ ...3322_AddSlotKeyToHostDiskEncryptionKeys.go | 23 -------- server/datastore/mysql/schema.sql | 3 +- server/fleet/datastore.go | 5 +- server/fleet/service.go | 2 +- server/mock/datastore_mock.go | 6 +-- server/service/orbit.go | 35 ++++++------- server/service/orbit_test.go | 52 +++++++++++-------- 10 files changed, 99 insertions(+), 93 deletions(-) create mode 100644 server/datastore/mysql/migrations/tables/20241116233322_AddLuksDataToHostDiskEncryptionKeys.go delete mode 100644 server/datastore/mysql/migrations/tables/20241116233322_AddSlotKeyToHostDiskEncryptionKeys.go diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index eff646172c..542ffac282 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -3819,22 +3819,23 @@ ON DUPLICATE KEY UPDATE return err } -func (ds *Datastore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error { - if encryptedBase64Passphrase == "" { // should have been caught at service level - return errors.New("blank encrypted passphrase") +func (ds *Datastore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlot uint) error { + if encryptedBase64Passphrase == "" || encryptedBase64Salt == "" { // should have been caught at service level + return errors.New("passphrase and salt must be set") } _, err := ds.writer(ctx).ExecContext(ctx, ` INSERT INTO host_disk_encryption_keys - (host_id, base64_encrypted, base64_encrypted_slot_key, client_error, decryptable) + (host_id, base64_encrypted, base64_encrypted_salt, key_slot, client_error, decryptable) VALUES - (?, ?, ?, '', TRUE) + (?, ?, ?, ?, '', TRUE) ON DUPLICATE KEY UPDATE decryptable = TRUE, base64_encrypted = VALUES(base64_encrypted), - base64_encrypted_slot_key = VALUES(base64_encrypted_slot_key), + base64_encrypted_salt = VALUES(base64_encrypted_salt), + key_slot = VALUES(key_slot), client_error = '' -`, hostID, encryptedBase64Passphrase, encryptedBase64SlotKey) +`, hostID, encryptedBase64Passphrase, encryptedBase64Salt, keySlot) return err } func (ds *Datastore) IsHostPendingEscrow(ctx context.Context, hostID uint) bool { diff --git a/server/datastore/mysql/hosts_test.go b/server/datastore/mysql/hosts_test.go index 9f3d758e81..6fe89b4768 100644 --- a/server/datastore/mysql/hosts_test.go +++ b/server/datastore/mysql/hosts_test.go @@ -7877,31 +7877,25 @@ func testLUKSDatastoreFunctions(t *testing.T, ds *Datastore) { require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host2.ID)) require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host3.ID)) - // no change when blank key attempted to save - err = ds.SaveLUKSData(ctx, host1.ID, "", "") + // no change when blank key or salt attempted to save + err = ds.SaveLUKSData(ctx, host1.ID, "", "", 0) + require.Error(t, err) + require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host1.ID)) + err = ds.SaveLUKSData(ctx, host1.ID, "foo", "", 0) require.Error(t, err) require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host1.ID)) - // persists with just passphrase - err = ds.SaveLUKSData(ctx, host1.ID, "foobar", "") + // persists with passphrase and salt set + err = ds.SaveLUKSData(ctx, host2.ID, "bazqux", "fuzzmuffin", 0) require.NoError(t, err) - require.Error(t, ds.AssertHasNoEncryptionKeyStored(ctx, host1.ID)) - require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host2.ID)) - key, err := ds.GetHostDiskEncryptionKey(ctx, host1.ID) - require.NoError(t, err) - require.Equal(t, "foobar", key.Base64Encrypted) - - // persists with passphrase and slot key - err = ds.SaveLUKSData(ctx, host2.ID, "bazqux", "fuzzmuffin") - require.NoError(t, err) - require.Error(t, ds.AssertHasNoEncryptionKeyStored(ctx, host1.ID)) + require.NoError(t, ds.AssertHasNoEncryptionKeyStored(ctx, host1.ID)) require.Error(t, ds.AssertHasNoEncryptionKeyStored(ctx, host2.ID)) - key, err = ds.GetHostDiskEncryptionKey(ctx, host2.ID) + key, err := ds.GetHostDiskEncryptionKey(ctx, host2.ID) require.NoError(t, err) require.Equal(t, "bazqux", key.Base64Encrypted) // persists when host hasn't had anything queued - err = ds.SaveLUKSData(ctx, host3.ID, "newstuff", "") + err = ds.SaveLUKSData(ctx, host3.ID, "newstuff", "fuzzball", 1) require.NoError(t, err) require.Error(t, ds.AssertHasNoEncryptionKeyStored(ctx, host3.ID)) key, err = ds.GetHostDiskEncryptionKey(ctx, host3.ID) diff --git a/server/datastore/mysql/migrations/tables/20241116233322_AddLuksDataToHostDiskEncryptionKeys.go b/server/datastore/mysql/migrations/tables/20241116233322_AddLuksDataToHostDiskEncryptionKeys.go new file mode 100644 index 0000000000..7a790a4540 --- /dev/null +++ b/server/datastore/mysql/migrations/tables/20241116233322_AddLuksDataToHostDiskEncryptionKeys.go @@ -0,0 +1,25 @@ +package tables + +import ( + "database/sql" + "fmt" +) + +func init() { + MigrationClient.AddMigration(Up_20241116233322, Down_20241116233322) +} + +func Up_20241116233322(tx *sql.Tx) error { + _, err := tx.Exec(`ALTER TABLE host_disk_encryption_keys + ADD COLUMN base64_encrypted_salt VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '' AFTER base64_encrypted, + ADD COLUMN key_slot TINYINT UNSIGNED DEFAULT NULL AFTER base64_encrypted_salt`) + if err != nil { + return fmt.Errorf("failed to add base64_encrypted_salt and key_slot columns to host_disk_encryption_keys: %w", err) + } + + return nil +} + +func Down_20241116233322(tx *sql.Tx) error { + return nil +} diff --git a/server/datastore/mysql/migrations/tables/20241116233322_AddSlotKeyToHostDiskEncryptionKeys.go b/server/datastore/mysql/migrations/tables/20241116233322_AddSlotKeyToHostDiskEncryptionKeys.go deleted file mode 100644 index 0642229cb2..0000000000 --- a/server/datastore/mysql/migrations/tables/20241116233322_AddSlotKeyToHostDiskEncryptionKeys.go +++ /dev/null @@ -1,23 +0,0 @@ -package tables - -import ( - "database/sql" - "fmt" -) - -func init() { - MigrationClient.AddMigration(Up_20241116233322, Down_20241116233322) -} - -func Up_20241116233322(tx *sql.Tx) error { - _, err := tx.Exec(`ALTER TABLE host_disk_encryption_keys ADD COLUMN base64_encrypted_slot_key VARCHAR(255) NOT NULL DEFAULT '' AFTER base64_encrypted`) - if err != nil { - return fmt.Errorf("failed to add base64_encrypted_slot_key to host_disk_encryption_keys: %w", err) - } - - return nil -} - -func Down_20241116233322(tx *sql.Tx) error { - return nil -} diff --git a/server/datastore/mysql/schema.sql b/server/datastore/mysql/schema.sql index 5acd57f912..45718cbd1d 100644 --- a/server/datastore/mysql/schema.sql +++ b/server/datastore/mysql/schema.sql @@ -303,7 +303,8 @@ CREATE TABLE `host_device_auth` ( CREATE TABLE `host_disk_encryption_keys` ( `host_id` int unsigned NOT NULL, `base64_encrypted` text COLLATE utf8mb4_unicode_ci NOT NULL, - `base64_encrypted_slot_key` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '', + `base64_encrypted_salt` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT '', + `key_slot` tinyint unsigned DEFAULT NULL, `decryptable` tinyint(1) DEFAULT NULL, `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` timestamp NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go index 496c707b86..6dfaae64b2 100644 --- a/server/fleet/datastore.go +++ b/server/fleet/datastore.go @@ -903,8 +903,9 @@ type Datastore interface { // SetOrUpdateHostDiskEncryptionKey sets the base64, encrypted key for // a host SetOrUpdateHostDiskEncryptionKey(ctx context.Context, hostID uint, encryptedBase64Key, clientError string, decryptable *bool) error - // SaveLUKSData sets base64'd encrypted LUKS passphrase and slot key data for a host - SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error + // SaveLUKSData sets base64'd encrypted LUKS passphrase, key slot, and salt data for a host that has successfully + // escrowed LUKS data + SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlot uint) error // GetUnverifiedDiskEncryptionKeys returns all the encryption keys that // are collected but their decryptable status is not known yet (ie: diff --git a/server/fleet/service.go b/server/fleet/service.go index 902ecad0d1..bb9ab9fc73 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -394,7 +394,7 @@ type Service interface { GetMunkiIssue(ctx context.Context, munkiIssueID uint) (*MunkiIssue, error) HostEncryptionKey(ctx context.Context, id uint) (*HostDiskEncryptionKey, error) - EscrowLUKSData(ctx context.Context, passphrase string, slotKey string, clientError string) error + EscrowLUKSData(ctx context.Context, passphrase string, salt string, keySlot *uint, clientError string) error // AddLabelsToHost adds the given label names to the host's label membership. // diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go index 51bda642d3..6d8fbd8856 100644 --- a/server/mock/datastore_mock.go +++ b/server/mock/datastore_mock.go @@ -639,7 +639,7 @@ type SetOrUpdateHostDisksEncryptionFunc func(ctx context.Context, hostID uint, e type SetOrUpdateHostDiskEncryptionKeyFunc func(ctx context.Context, hostID uint, encryptedBase64Key string, clientError string, decryptable *bool) error -type SaveLUKSDataFunc func(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error +type SaveLUKSDataFunc func(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlot uint) error type GetUnverifiedDiskEncryptionKeysFunc func(ctx context.Context) ([]fleet.HostDiskEncryptionKey, error) @@ -5048,11 +5048,11 @@ func (s *DataStore) SetOrUpdateHostDiskEncryptionKey(ctx context.Context, hostID return s.SetOrUpdateHostDiskEncryptionKeyFunc(ctx, hostID, encryptedBase64Key, clientError, decryptable) } -func (s *DataStore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error { +func (s *DataStore) SaveLUKSData(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlot uint) error { s.mu.Lock() s.SaveLUKSDataFuncInvoked = true s.mu.Unlock() - return s.SaveLUKSDataFunc(ctx, hostID, encryptedBase64Passphrase, encryptedBase64SlotKey) + return s.SaveLUKSDataFunc(ctx, hostID, encryptedBase64Passphrase, encryptedBase64Salt, keySlot) } func (s *DataStore) GetUnverifiedDiskEncryptionKeys(ctx context.Context) ([]fleet.HostDiskEncryptionKey, error) { diff --git a/server/service/orbit.go b/server/service/orbit.go index 69dd27a882..0a0e852bb3 100644 --- a/server/service/orbit.go +++ b/server/service/orbit.go @@ -1034,7 +1034,8 @@ func (svc *Service) SetOrUpdateDiskEncryptionKey(ctx context.Context, encryption type orbitPostLUKSRequest struct { OrbitNodeKey string `json:"orbit_node_key"` Passphrase string `json:"passphrase"` - SlotKey string `json:"slot_key"` + Salt string `json:"salt"` + KeySlot *uint `json:"key_slot"` ClientError string `json:"client_error"` } @@ -1057,13 +1058,13 @@ func (r orbitPostLUKSResponse) Status() int { return http.StatusNoContent } func postOrbitLUKSEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) { req := request.(*orbitPostLUKSRequest) - if err := svc.EscrowLUKSData(ctx, req.Passphrase, req.SlotKey, req.ClientError); err != nil { + if err := svc.EscrowLUKSData(ctx, req.Passphrase, req.Salt, req.KeySlot, req.ClientError); err != nil { return orbitPostLUKSResponse{Err: err}, nil } return orbitPostLUKSResponse{}, nil } -func (svc *Service) EscrowLUKSData(ctx context.Context, passphrase string, slotKey string, clientError string) error { +func (svc *Service) EscrowLUKSData(ctx context.Context, passphrase string, salt string, keySlot *uint, clientError string) error { // this is not a user-authenticated endpoint svc.authz.SkipAuthorization(ctx) @@ -1075,36 +1076,34 @@ func (svc *Service) EscrowLUKSData(ctx context.Context, passphrase string, slotK if clientError != "" { return svc.ds.ReportEscrowError(ctx, host.ID, clientError) } - encryptedPassphrase, encryptedSlotKey, err := svc.validateAndEncrypt(ctx, passphrase, slotKey) + + encryptedPassphrase, encryptedSalt, validatedKeySlot, err := svc.validateAndEncrypt(ctx, passphrase, salt, keySlot) if err != nil { _ = svc.ds.ReportEscrowError(ctx, host.ID, err.Error()) return err } - return svc.ds.SaveLUKSData(ctx, host.ID, encryptedPassphrase, encryptedSlotKey) + return svc.ds.SaveLUKSData(ctx, host.ID, encryptedPassphrase, encryptedSalt, validatedKeySlot) } -func (svc *Service) validateAndEncrypt(ctx context.Context, passphrase string, slotKey string) (string, string, error) { - if passphrase == "" { - return "", "", badRequest("Blank passphrase provided") +func (svc *Service) validateAndEncrypt(ctx context.Context, passphrase string, salt string, keySlot *uint) (encryptedPassphrase string, encryptedSalt string, validatedKeySlot uint, err error) { + if passphrase == "" || salt == "" || keySlot == nil { + return "", "", 0, badRequest("passphrase, salt, and key_slot must be provided to escrow LUKS data") } if svc.config.Server.PrivateKey == "" { - return "", "", newOsqueryError("internal error: missing server private key") + return "", "", 0, newOsqueryError("internal error: missing server private key") } - encryptedPassphrase, err := mdm.EncryptAndEncode(passphrase, svc.config.Server.PrivateKey) + encryptedPassphrase, err = mdm.EncryptAndEncode(passphrase, svc.config.Server.PrivateKey) if err != nil { - return "", "", ctxerr.Wrap(ctx, err, "internal error: could not encrypt LUKS data") + return "", "", 0, ctxerr.Wrap(ctx, err, "internal error: could not encrypt LUKS data") } - var encryptedSlotKey string - if slotKey != "" { - encryptedSlotKey, err = mdm.EncryptAndEncode(slotKey, svc.config.Server.PrivateKey) - if err != nil { - return "", "", ctxerr.Wrap(ctx, err, "internal error: could not encrypt LUKS data") - } + encryptedSalt, err = mdm.EncryptAndEncode(salt, svc.config.Server.PrivateKey) + if err != nil { + return "", "", 0, ctxerr.Wrap(ctx, err, "internal error: could not encrypt LUKS data") } - return encryptedPassphrase, encryptedSlotKey, nil + return encryptedPassphrase, encryptedSalt, *keySlot, nil } ///////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/orbit_test.go b/server/service/orbit_test.go index 97b28fc478..3dc98b64aa 100644 --- a/server/service/orbit_test.go +++ b/server/service/orbit_test.go @@ -180,20 +180,21 @@ func TestOrbitLUKSDataSave(t *testing.T) { } // test reporting client errors - err := svc.EscrowLUKSData(ctx, "foo", "bar", expectedErrorMessage) + err := svc.EscrowLUKSData(ctx, "foo", "bar", nil, expectedErrorMessage) require.NoError(t, err) require.True(t, ds.ReportEscrowErrorFuncInvoked) // blank passphrase ds.ReportEscrowErrorFuncInvoked = false - expectedErrorMessage = "Blank passphrase provided" - err = svc.EscrowLUKSData(ctx, "", "bar", "") + expectedErrorMessage = "passphrase, salt, and key_slot must be provided to escrow LUKS data" + err = svc.EscrowLUKSData(ctx, "", "bar", ptr.Uint(0), "") require.Error(t, err) require.True(t, ds.ReportEscrowErrorFuncInvoked) ds.ReportEscrowErrorFuncInvoked = false - passphrase, slotKey := "foo", "" - ds.SaveLUKSDataFunc = func(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64SlotKey string) error { + passphrase, salt := "foo", "" + var keySlot *uint + ds.SaveLUKSDataFunc = func(ctx context.Context, hostID uint, encryptedBase64Passphrase string, encryptedBase64Salt string, keySlotToPersist uint) error { require.Equal(t, host.ID, hostID) key := config.TestConfig().Server.PrivateKey @@ -201,29 +202,36 @@ func TestOrbitLUKSDataSave(t *testing.T) { require.NoError(t, err) require.Equal(t, passphrase, decryptedPassphrase) - if encryptedBase64SlotKey == "" { - require.Equal(t, slotKey, encryptedBase64SlotKey) - return nil - } - decryptedSlotKey, err := mdm.DecodeAndDecrypt(encryptedBase64SlotKey, key) + decryptedSalt, err := mdm.DecodeAndDecrypt(encryptedBase64Salt, key) require.NoError(t, err) - require.Equal(t, slotKey, decryptedSlotKey) + require.Equal(t, salt, decryptedSalt) + + require.Equal(t, *keySlot, keySlotToPersist) return nil } - // with no slot key - err = svc.EscrowLUKSData(ctx, passphrase, slotKey, "") + // with no salt + err = svc.EscrowLUKSData(ctx, passphrase, salt, keySlot, "") + require.Error(t, err) + require.True(t, ds.ReportEscrowErrorFuncInvoked) + require.False(t, ds.SaveLUKSDataFuncInvoked) + + // with no key slot + ds.ReportEscrowErrorFuncInvoked = false + salt = "baz" + err = svc.EscrowLUKSData(ctx, passphrase, salt, keySlot, "") + require.Error(t, err) + require.True(t, ds.ReportEscrowErrorFuncInvoked) + require.False(t, ds.SaveLUKSDataFuncInvoked) + + // with salt and key slot + keySlot = ptr.Uint(0) + ds.ReportEscrowErrorFuncInvoked = false + err = svc.EscrowLUKSData(ctx, passphrase, salt, keySlot, "") require.NoError(t, err) require.False(t, ds.ReportEscrowErrorFuncInvoked) require.True(t, ds.SaveLUKSDataFuncInvoked) - - // with slot key - slotKey = "baz" - ds.SaveLUKSDataFuncInvoked = false - err = svc.EscrowLUKSData(ctx, passphrase, slotKey, "") - require.NoError(t, err) - require.True(t, ds.SaveLUKSDataFuncInvoked) }) t.Run("fail when no/invalid private key is set", func(t *testing.T) { @@ -243,7 +251,7 @@ func TestOrbitLUKSDataSave(t *testing.T) { cfg.Server.PrivateKey = "" svc, ctx := newTestServiceWithConfig(t, ds, cfg, nil, nil, &TestServerOpts{License: license, SkipCreateTestUsers: true}) ctx = test.HostContext(ctx, host) - err := svc.EscrowLUKSData(ctx, "foo", "bar", "") + err := svc.EscrowLUKSData(ctx, "foo", "bar", ptr.Uint(0), "") require.Error(t, err) require.True(t, ds.ReportEscrowErrorFuncInvoked) @@ -252,7 +260,7 @@ func TestOrbitLUKSDataSave(t *testing.T) { cfg.Server.PrivateKey = "invalid" svc, ctx = newTestServiceWithConfig(t, ds, cfg, nil, nil, &TestServerOpts{License: license, SkipCreateTestUsers: true}) ctx = test.HostContext(ctx, host) - err = svc.EscrowLUKSData(ctx, "foo", "bar", "") + err = svc.EscrowLUKSData(ctx, "foo", "bar", ptr.Uint(0), "") require.Error(t, err) require.True(t, ds.ReportEscrowErrorFuncInvoked) })