From fedeab6130dd6dcb469a9eaec5e1de2a6ad8afd7 Mon Sep 17 00:00:00 2001 From: Roberto Dip Date: Tue, 16 Jan 2024 12:45:23 -0300 Subject: [PATCH] attempt to decrypt the disk before performing a BitLocker encryption (#16097) for #15711, this attempts to decrypt the disk if it was previously encrypted and Fleet doesn't have the key. --- orbit/changes/15916-bitlocker-decrypt | 1 + orbit/pkg/update/notifications.go | 42 ++++++++++++++++ orbit/pkg/update/notifications_test.go | 70 +++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 orbit/changes/15916-bitlocker-decrypt diff --git a/orbit/changes/15916-bitlocker-decrypt b/orbit/changes/15916-bitlocker-decrypt new file mode 100644 index 0000000000..1c1d22f2db --- /dev/null +++ b/orbit/changes/15916-bitlocker-decrypt @@ -0,0 +1 @@ +* Attempt to automatically decrypt the disk before performing a BitLocker encryption if it was previously encrypted and Fleet doesn't have the key. diff --git a/orbit/pkg/update/notifications.go b/orbit/pkg/update/notifications.go index d190da297f..174b2d76c3 100644 --- a/orbit/pkg/update/notifications.go +++ b/orbit/pkg/update/notifications.go @@ -416,6 +416,12 @@ type execEncryptVolumeFunc func(volumeID string) (recoveryKey string, err error) // encryption status of a volume, and an error if the operation fails. type execGetEncryptionStatusFunc func() (status []bitlocker.VolumeStatus, err error) +// execDecryptVolumeFunc handles the decryption of a volume identified by its +// string identifier (e.g., "C:") +// +// It returns an error if the process fails. +type execDecryptVolumeFunc func(volumeID string) error + type windowsMDMBitlockerConfigFetcher struct { // Fetcher is the OrbitConfigFetcher that will be wrapped. It is responsible // for actually returning the orbit configuration or an error. @@ -441,6 +447,10 @@ type windowsMDMBitlockerConfigFetcher struct { // for tests, to be able to mock API commands. If nil, will use // bitlocker.GetEncryptionStatus execGetEncryptionStatusFn execGetEncryptionStatusFunc + + // for tests, to be able to mock the decryption process. If nil, will use + // bitlocker.DecryptVolume + execDecryptVolumeFn execDecryptVolumeFunc } func ApplyWindowsMDMBitlockerFetcherMiddleware( @@ -499,6 +509,29 @@ func (w *windowsMDMBitlockerConfigFetcher) attemptBitlockerEncryption(notifs fle return } + // if the disk is encrypted, try to decrypt it first. + if encryptionStatus != nil && + encryptionStatus.ConversionStatus == bitlocker.ConversionStatusFullyEncrypted { + log.Debug().Msg("disk was previously encrypted. Attempting to decrypt it") + + if err := w.decryptVolume(targetVolume); err != nil { + log.Error().Err(err).Msg("decryption failed") + + if serverErr := w.updateFleetServer("", err); serverErr != nil { + log.Error().Err(serverErr).Msg("failed to send decryption failure to Fleet Server") + return + } + } + + // return regardless of the operation output. + // + // the decryption process takes an unknown amount of time (depending on + // factors outside of our control) and the next tick will be a noop if the + // disk is not ready to be encrypted yet (due to the + // w.bitLockerActionInProgress check above) + return + } + recoveryKey, encryptionErr := w.performEncryption(targetVolume) // before reporting the error to the server, check if the error we've got is valid. // see the description of w.isMisreportedDecryptionError and issue #15916. @@ -570,6 +603,15 @@ func (w *windowsMDMBitlockerConfigFetcher) performEncryption(volume string) (str return recoveryKey, nil } +func (w *windowsMDMBitlockerConfigFetcher) decryptVolume(targetVolume string) error { + fn := w.execDecryptVolumeFn + if fn == nil { + fn = bitlocker.DecryptVolume + } + + return fn(targetVolume) +} + // isMisreportedDecryptionError checks whether the given error is a potentially // misreported decryption error. // diff --git a/orbit/pkg/update/notifications_test.go b/orbit/pkg/update/notifications_test.go index 191b09be3f..bcbb1c3815 100644 --- a/orbit/pkg/update/notifications_test.go +++ b/orbit/pkg/update/notifications_test.go @@ -595,7 +595,10 @@ func TestBitlockerOperations(t *testing.T) { var ( shouldEncrypt = true shouldFailEncryption = false + shouldFailDecryption = false shouldFailServerUpdate = false + encryptFnCalled = false + decryptFnCalled = false ) fetcher := &dummyConfigFetcher{ @@ -625,13 +628,28 @@ func TestBitlockerOperations(t *testing.T) { return []bitlocker.VolumeStatus{}, nil }, execEncryptVolumeFn: func(string) (string, error) { + encryptFnCalled = true if shouldFailEncryption { - return "", errors.New("error") + return "", errors.New("error encrypting") } return "123456", nil }, + execDecryptVolumeFn: func(string) error { + decryptFnCalled = true + if shouldFailDecryption { + return errors.New("error decrypting") + } + + return nil + }, } + shouldEncrypt = true + shouldFailEncryption = false + shouldFailDecryption = false + shouldFailServerUpdate = false + encryptFnCalled = false + decryptFnCalled = false clientMock.SetOrUpdateDiskEncryptionKeyInvoked = false logBuf.Reset() } @@ -640,6 +658,7 @@ func TestBitlockerOperations(t *testing.T) { setupTest() shouldEncrypt = true shouldFailEncryption = false + shouldFailDecryption = false cfg, err := enrollFetcher.GetConfig() require.NoError(t, err) // the dummy fetcher never returns an error require.Equal(t, fetcher.cfg, cfg) // the bitlocker wrapper properly returns the expected config @@ -652,6 +671,8 @@ func TestBitlockerOperations(t *testing.T) { cfg, err := enrollFetcher.GetConfig() require.NoError(t, err) // the dummy fetcher never returns an error require.Equal(t, fetcher.cfg, cfg) // the bitlocker wrapper properly returns the expected config + require.True(t, encryptFnCalled, "encryption function should have been called") + require.False(t, decryptFnCalled, "decryption function should not be called") }) t.Run("bitlocker encryption returns an error", func(t *testing.T) { @@ -661,6 +682,8 @@ func TestBitlockerOperations(t *testing.T) { cfg, err := enrollFetcher.GetConfig() require.NoError(t, err) // the dummy fetcher never returns an error require.Equal(t, fetcher.cfg, cfg) // the bitlocker wrapper properly returns the expected config + require.True(t, encryptFnCalled, "encryption function should have been called") + require.False(t, decryptFnCalled, "decryption function should not be called") }) t.Run("encryption skipped based on various current statuses", func(t *testing.T) { @@ -683,6 +706,8 @@ func TestBitlockerOperations(t *testing.T) { require.NoError(t, err) require.Equal(t, fetcher.cfg, cfg) require.Contains(t, logBuf.String(), "skipping encryption as the disk is not available") + require.False(t, encryptFnCalled, "encryption function should not be called") + require.False(t, decryptFnCalled, "decryption function should not be called") logBuf.Reset() // Reset the log buffer for the next iteration }) } @@ -701,7 +726,42 @@ func TestBitlockerOperations(t *testing.T) { cfg, err := enrollFetcher.GetConfig() require.NoError(t, err) require.Equal(t, fetcher.cfg, cfg) - require.Contains(t, logBuf.String(), "disk encryption failed due to previous unsuccessful attempt") + require.Contains(t, logBuf.String(), "disk encryption failed due to previous unsuccessful attempt, user action required") + require.False(t, encryptFnCalled, "encryption function should not be called") + require.False(t, decryptFnCalled, "decryption function should not be called") + }) + + t.Run("decrypts the disk if previously encrypted", func(t *testing.T) { + setupTest() + mockStatus := &bitlocker.EncryptionStatus{ConversionStatus: bitlocker.ConversionStatusFullyEncrypted} + enrollFetcher.execGetEncryptionStatusFn = func() ([]bitlocker.VolumeStatus, error) { + return []bitlocker.VolumeStatus{{DriveVolume: "C:", Status: mockStatus}}, nil + } + cfg, err := enrollFetcher.GetConfig() + require.NoError(t, err) + require.Equal(t, fetcher.cfg, cfg) + require.Contains(t, logBuf.String(), "disk was previously encrypted. Attempting to decrypt it") + require.False(t, clientMock.SetOrUpdateDiskEncryptionKeyInvoked) + require.False(t, encryptFnCalled, "encryption function should not have been called") + require.True(t, decryptFnCalled, "decryption function should have been called") + }) + + t.Run("reports to the server if decryption fails", func(t *testing.T) { + setupTest() + shouldFailDecryption = true + mockStatus := &bitlocker.EncryptionStatus{ConversionStatus: bitlocker.ConversionStatusFullyEncrypted} + enrollFetcher.execGetEncryptionStatusFn = func() ([]bitlocker.VolumeStatus, error) { + return []bitlocker.VolumeStatus{{DriveVolume: "C:", Status: mockStatus}}, nil + } + + cfg, err := enrollFetcher.GetConfig() + require.NoError(t, err) + require.Equal(t, fetcher.cfg, cfg) + require.Contains(t, logBuf.String(), "disk was previously encrypted. Attempting to decrypt it") + require.Contains(t, logBuf.String(), "decryption failed") + require.True(t, clientMock.SetOrUpdateDiskEncryptionKeyInvoked) + require.False(t, encryptFnCalled, "encryption function should not be called") + require.True(t, decryptFnCalled, "decryption function should have been called") }) t.Run("encryption skipped if last run too recent", func(t *testing.T) { @@ -713,6 +773,8 @@ func TestBitlockerOperations(t *testing.T) { require.NoError(t, err) require.Equal(t, fetcher.cfg, cfg) require.Contains(t, logBuf.String(), "skipped encryption process, last run was too recent") + require.False(t, encryptFnCalled, "encryption function should not be called") + require.False(t, decryptFnCalled, "decryption function should not be called") }) t.Run("successful fleet server update", func(t *testing.T) { @@ -727,6 +789,8 @@ func TestBitlockerOperations(t *testing.T) { require.NoError(t, err) require.Equal(t, fetcher.cfg, cfg) require.True(t, clientMock.SetOrUpdateDiskEncryptionKeyInvoked) + require.True(t, encryptFnCalled, "encryption function should have been called") + require.False(t, decryptFnCalled, "decryption function should not be called") }) t.Run("failed fleet server update", func(t *testing.T) { @@ -743,5 +807,7 @@ func TestBitlockerOperations(t *testing.T) { require.Equal(t, fetcher.cfg, cfg) require.Contains(t, logBuf.String(), "failed to send encryption result to Fleet Server") require.True(t, clientMock.SetOrUpdateDiskEncryptionKeyInvoked) + require.True(t, encryptFnCalled, "encryption function should have been called") + require.False(t, decryptFnCalled, "decryption function should not be called") }) }