Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revise LUKS API to expect key slot and salt when escrowing successfully #23952

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+99 −93
Open

Revise LUKS API to expect key slot and salt when escrowing successfully #23952

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d41d3c
Revise LUKS API to expect key slot and salt when escrowing successfully
iansltx Nov 19, 2024
f4409ea
Fix DB charset, use named returns
iansltx Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions server/datastore/mysql/hosts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should keySlot be *uint to map cleanly to the underlying NULLable type type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Only reason key slot is nullable in the DB is we use this table for BitLocker/FileVault, which (from our perspective at least) have neither key slot nor salt. Per the Slack convo earlier today, we're guaranteed to get key slot if we have valid LUKS data, and this method is only called when we have valid LUKS data, so we can tighten the parameter type.

Likewise we're guaranteed to have passphrase and salt when persisting LUKS, but I can't enforce that easily in the type system, hence the runtime check.

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 {
Expand Down
26 changes: 10 additions & 16 deletions server/datastore/mysql/hosts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", "")
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")
// 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, 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)
Expand Down
25 changes: 25 additions & 0 deletions ...r/datastore/mysql/migrations/tables/20241116233322_AddLuksDataToHostDiskEncryptionKeys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) NOT NULL DEFAULT '' AFTER base64_encrypted,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Touché. Fixing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of note, we could save storage here by keeping the charset as-is, because the column should never be joined or sorted, and since this is base64'd even ASCII would represent this fine. But might as well be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason to use AFTER?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight cleanliness thing when I (or others) use the mysql command line. Figured I might as well put encrypted/encryption related columns visually next to each other.

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
}
23 changes: 0 additions & 23 deletions ...er/datastore/mysql/migrations/tables/20241116233322_AddSlotKeyToHostDiskEncryptionKeys.go
View file
Open in desktop

This file was deleted.

3 changes: 2 additions & 1 deletion server/datastore/mysql/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) 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,
Expand Down
5 changes: 3 additions & 2 deletions server/fleet/datastore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion server/fleet/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down
6 changes: 3 additions & 3 deletions server/mock/datastore_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -5038,11 +5038,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) {
Expand Down
33 changes: 16 additions & 17 deletions server/service/orbit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,7 +1024,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"`
}

Expand All @@ -1047,13 +1048,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)

Expand All @@ -1065,36 +1066,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) (string, string, uint, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, when having many same-typed return types, it's a good practice to use named returns:
(string, string, uint, error) -> (encryptedPassphrase string, encryptedSalt string, keySlot uint, err error)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the call-out. Will fix.

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)
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
}

/////////////////////////////////////////////////////////////////////////////////
Expand Down
52 changes: 30 additions & 22 deletions server/service/orbit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,49 +116,57 @@ 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

decryptedPassphrase, err := mdm.DecodeAndDecrypt(encryptedBase64Passphrase, key)
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, "")
require.NoError(t, err)
require.False(t, ds.ReportEscrowErrorFuncInvoked)
require.True(t, ds.SaveLUKSDataFuncInvoked)
// 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 slot key
slotKey = "baz"
ds.SaveLUKSDataFuncInvoked = false
err = svc.EscrowLUKSData(ctx, passphrase, slotKey, "")
// 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)
})

Expand All @@ -179,7 +187,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)

Expand All @@ -188,7 +196,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)
})
Expand Down
Loading