Bitlocker: do not decrypt already encrypted drive.#43130
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the Windows BitLocker encrypt/decrypt loop described in #40809 by improving how Fleet detects/represents BitLocker state and by changing orbit’s behavior for pre-encrypted disks.
Changes:
- Server: Windows disk encryption detail query now ingests both
conversion_statusandprotection_status, and persistsbitlocker_protection_statusinhost_disksto better classify Verified vs Action required. - Server: Datastore API updated (
SetOrUpdateHostDisksEncryption) to optionally persist BitLocker protection status; MySQL schema + migration added. - Orbit: When the OS volume is already encrypted, rotate/escrow a new recovery key instead of decrypting and re-encrypting.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Updates Windows disk encryption query + adds Windows-specific ingestion (conversion/protection status). |
| server/service/osquery_utils/queries_test.go | Updates mock datastore signature for disk encryption ingestion tests. |
| server/fleet/datastore.go | Extends datastore interface for BitLocker protection status. |
| server/mock/datastore_mock.go | Updates mock datastore method signature and forwarding. |
| server/datastore/mysql/schema.sql | Adds bitlocker_protection_status column and migration tracking updates. |
| server/datastore/mysql/migrations/tables/20260407120000_AddBitlockerProtectionStatusToHostDisks.go | Adds MySQL migration for new host_disks column. |
| server/datastore/mysql/hosts.go | Persists optional BitLocker protection status alongside encrypted flag. |
| server/datastore/mysql/microsoft_mdm.go | Uses protection status in BitLocker status classification and adds better “action required” detail. |
| server/datastore/mysql/test.go + server/service/integration_test.go | Updates call sites for new datastore method signature. |
| orbit/pkg/update/notifications.go | Switches behavior for pre-encrypted disks to rotate recovery key instead of decrypting. |
| orbit/pkg/update/notifications_test.go | Updates tests to cover key rotation flow. |
| orbit/pkg/bitlocker/bitlocker_worker_*.go | Adds RotateRecoveryKey wiring on Windows and no-op stubs elsewhere. |
| orbit/pkg/bitlocker/bitlocker_management_windows.go | Implements COM-thread recovery key rotation + protector cleanup. |
| changes/issue-40809-bitlocker-loop | User-visible change note for server-side fix and UI status improvements. |
| orbit/changes/issue-40809-bitlocker-key-rotation | User-visible change note for orbit behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR implements a fix for a Windows BitLocker encryption/decryption loop on systems with secondary drives using auto-unlock. The solution introduces recovery key rotation for already-encrypted volumes, preventing repeated decryption attempts that fail with Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
orbit/pkg/update/notifications.go (1)
613-615:⚠️ Potential issue | 🟡 MinorRemove unused
decryptVolumemethod andexecDecryptVolumeFnfield.The
decryptVolumemethod at lines 613-615 is never called. The new rotation-based approach (lines 524-528) explicitly avoids decryption entirely. All tests verify that decryption is never invoked. Remove the unused method and theexecDecryptVolumeFnfield from thewindowsMDMBitlockerConfigReceiverstruct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@orbit/pkg/update/notifications.go` around lines 613 - 615, Remove the unused decryptVolume method and the execDecryptVolumeFn field from the windowsMDMBitlockerConfigReceiver: delete the decryptVolume(targetVolume string) method and remove the execDecryptVolumeFn field declaration from the windowsMDMBitlockerConfigReceiver struct (and any assignments in constructors or initialization like where execDecryptVolumeFn might be set); ensure there are no remaining references to execDecryptVolumeFn elsewhere (search for execDecryptVolumeFn and decryptVolume) and run tests to confirm the rotation-based code paths still pass.
🧹 Nitpick comments (5)
server/datastore/mysql/hosts_test.go (1)
10049-10063: Consider adding one non-nilcoverage case for the new argument.These updates compile and keep legacy behavior, but this test still exercises only the
nilbranch ofbitlockerProtectionStatus. A single assertion with a concrete value would better guard regressions in the new persistence path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/hosts_test.go` around lines 10049 - 10063, The test only covers the nil branch for the new bitlockerProtectionStatus argument; update the test around SetOrUpdateHostDisksEncryption to add one call that passes a non-nil concrete value (e.g., a pointer to a specific protection status) for bitlockerProtectionStatus and assert the persisted Host.DiskEncryptionEnabled and the stored bitlocker status when reloading via ds.Host; modify the call that currently uses nil (for host2) to include a non-nil value and add a require.Equal/require.True/require.False assertion against the retrieved host's bitlocker protection status to exercise the non-nil persistence path.server/service/integration_core_test.go (1)
13600-13600: Consider adding one Windows case with non-nilBitLocker protection status.This file now compiles against the new signature, but it only exercises the
nilpath. A single non-nilWindows assertion here would better guard the BitLocker-specific behavior this PR targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_core_test.go` at line 13600, Add a test case that exercises the non-nil BitLocker path: call s.ds.SetOrUpdateHostDisksEncryption(ctx, host.ID, true, &bitlockerStatus) using a Windows host (set host.Platform = "windows") and a non-nil bitlockerStatus value, then fetch the host/encryption record (e.g., via s.ds.Host or the datastore read helper used elsewhere in this test file) and assert the stored BitLockerProtectionStatus equals the value you passed; ensure you reference the same host.ID and SetOrUpdateHostDisksEncryption function used in the existing test so the new assertion covers the Windows non-nil branch.server/service/osquery_utils/queries_test.go (1)
1956-1958: AssertbitlockerProtectionStatusis nil in non-Windows ingestion testsThese tests were correctly updated for the new method signature, but they currently don’t validate the new argument. Adding
assert.Nil(t, bitlockerProtectionStatus)would harden the contract for non-Windows paths.Suggested test hardening
ds.SetOrUpdateHostDisksEncryptionFunc = func(ctx context.Context, id uint, encrypted bool, bitlockerProtectionStatus *int32) error { assert.Equal(t, expectEncrypted, encrypted) + assert.Nil(t, bitlockerProtectionStatus) return nil }Also applies to: 1985-1987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_utils/queries_test.go` around lines 1956 - 1958, The mock SetOrUpdateHostDisksEncryptionFunc used in tests should also assert that the new bitlockerProtectionStatus argument is nil for non-Windows ingestion paths; update the anonymous function assigned to ds.SetOrUpdateHostDisksEncryptionFunc (the mock that currently asserts expectEncrypted) to include assert.Nil(t, bitlockerProtectionStatus) so the test validates the new parameter is nil (apply the same change to the other occurrence around lines referenced).server/datastore/mysql/labels_test.go (1)
2110-2110: Consider adding explicit BitLocker protection-status coverage in this Windows testLine 2110 now passes
nilforbitlockerProtectionStatus, which updates the signature but doesn’t exercise the new Windows-specific status path. A follow-up subtest with a concrete protection status would better lock in the intended behavior for the new column/filtering logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/labels_test.go` at line 2110, The test currently calls SetOrUpdateHostDisksEncryption(ctx, h1.ID, true, nil) which updates the signature but doesn't exercise the Windows-specific bitlockerProtectionStatus branch; add a follow-up subtest in labels_test.go that calls SetOrUpdateHostDisksEncryption with a concrete non-nil bitlockerProtectionStatus value (e.g. "protected" or the enum/const your code expects) for the same host (h1.ID), assert NoError on the call, and then assert the host's disk encryption label/state and any filtering behavior that depends on bitlockerProtectionStatus to validate the new column/path is exercised (use the same helper assertions used elsewhere in the file).orbit/pkg/bitlocker/bitlocker_management_windows.go (1)
563-570: Consider logging TPM protector failures at a higher level.When adding a TPM protector fails with an error other than
ErrorCodeProtectorExists, it's logged atDebuglevel and the operation continues. This is likely intentional to handle pre-encrypted disks that may use different unlock mechanisms, but a non-TPM protected disk could leave the system in a less secure state (recovery key only).Consider logging at
InfoorWarnlevel so administrators are aware when TPM protection cannot be added.Suggested change to improve observability
// Ensure a TPM protector exists (some pre-encrypted disks may not have one). if err := vol.protectWithTPM(nil); err != nil { // ErrorCodeProtectorExists is expected if a TPM protector is already present. var encErr *EncryptionError if !errors.As(err, &encErr) || encErr.Code() != ErrorCodeProtectorExists { - log.Debug().Err(err).Msg("could not add TPM protector, continuing") + log.Info().Err(err).Msg("could not add TPM protector during key rotation, continuing without TPM protection") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@orbit/pkg/bitlocker/bitlocker_management_windows.go` around lines 563 - 570, The current handling in the vol.protectWithTPM(nil) block swallows non-ErrorCodeProtectorExists errors at Debug level; update the logging so failures to add a TPM protector (when errors.As(err, &encErr) && encErr.Code() != ErrorCodeProtectorExists) are logged at a higher level (Info or Warn) instead of log.Debug() to improve observability while still continuing execution; locate the vol.protectWithTPM(nil) call and the error handling that builds an EncryptionError and replace the log.Debug().Err(err).Msg(...) invocation with log.Info() or log.Warn() as appropriate, preserving the existing error context and message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 884-885: The bitlocker_protection_status value 2 (unknown) is not
handled by the predicates, so encrypted hosts with status=2 fall through to a
failed state; update the predicates used by GetMDMWindowsBitLockerStatus by
modifying whereProtectionOn to include "= 2" (i.e., treat NULL, 1, or 2 as
accepted/unknown) and keep whereProtectionOff matching only explicit 0 values
(hd.bitlocker_protection_status IS NOT NULL AND hd.bitlocker_protection_status =
0) so status=2 is treated as unknown/accepted in the existing logic.
In `@server/service/osquery_utils/queries.go`:
- Around line 2651-2659: The code currently ignores strconv.ParseInt errors
causing empty/malformed osquery fields to be coerced to 0; change the logic in
the block that parses conversion_status and protection_status so you check the
returned errors from strconv.ParseInt: only set `encrypted` when parsing
conversion_status succeeds (if parsing fails, skip/abort the disk-encryption
update rather than treating it as decrypted), and only build and pass a non-nil
protectionStatus pointer to SetOrUpdateHostDisksEncryption when parsing
protection_status succeeds (if parsing protection_status fails, pass nil for the
protection status). Locate the parsing of
`conversion_status`/`protection_status` and the call to
SetOrUpdateHostDisksEncryption and add explicit error checks around
strconv.ParseInt for those symbols.
---
Outside diff comments:
In `@orbit/pkg/update/notifications.go`:
- Around line 613-615: Remove the unused decryptVolume method and the
execDecryptVolumeFn field from the windowsMDMBitlockerConfigReceiver: delete the
decryptVolume(targetVolume string) method and remove the execDecryptVolumeFn
field declaration from the windowsMDMBitlockerConfigReceiver struct (and any
assignments in constructors or initialization like where execDecryptVolumeFn
might be set); ensure there are no remaining references to execDecryptVolumeFn
elsewhere (search for execDecryptVolumeFn and decryptVolume) and run tests to
confirm the rotation-based code paths still pass.
---
Nitpick comments:
In `@orbit/pkg/bitlocker/bitlocker_management_windows.go`:
- Around line 563-570: The current handling in the vol.protectWithTPM(nil) block
swallows non-ErrorCodeProtectorExists errors at Debug level; update the logging
so failures to add a TPM protector (when errors.As(err, &encErr) &&
encErr.Code() != ErrorCodeProtectorExists) are logged at a higher level (Info or
Warn) instead of log.Debug() to improve observability while still continuing
execution; locate the vol.protectWithTPM(nil) call and the error handling that
builds an EncryptionError and replace the log.Debug().Err(err).Msg(...)
invocation with log.Info() or log.Warn() as appropriate, preserving the existing
error context and message.
In `@server/datastore/mysql/hosts_test.go`:
- Around line 10049-10063: The test only covers the nil branch for the new
bitlockerProtectionStatus argument; update the test around
SetOrUpdateHostDisksEncryption to add one call that passes a non-nil concrete
value (e.g., a pointer to a specific protection status) for
bitlockerProtectionStatus and assert the persisted Host.DiskEncryptionEnabled
and the stored bitlocker status when reloading via ds.Host; modify the call that
currently uses nil (for host2) to include a non-nil value and add a
require.Equal/require.True/require.False assertion against the retrieved host's
bitlocker protection status to exercise the non-nil persistence path.
In `@server/datastore/mysql/labels_test.go`:
- Line 2110: The test currently calls SetOrUpdateHostDisksEncryption(ctx, h1.ID,
true, nil) which updates the signature but doesn't exercise the Windows-specific
bitlockerProtectionStatus branch; add a follow-up subtest in labels_test.go that
calls SetOrUpdateHostDisksEncryption with a concrete non-nil
bitlockerProtectionStatus value (e.g. "protected" or the enum/const your code
expects) for the same host (h1.ID), assert NoError on the call, and then assert
the host's disk encryption label/state and any filtering behavior that depends
on bitlockerProtectionStatus to validate the new column/path is exercised (use
the same helper assertions used elsewhere in the file).
In `@server/service/integration_core_test.go`:
- Line 13600: Add a test case that exercises the non-nil BitLocker path: call
s.ds.SetOrUpdateHostDisksEncryption(ctx, host.ID, true, &bitlockerStatus) using
a Windows host (set host.Platform = "windows") and a non-nil bitlockerStatus
value, then fetch the host/encryption record (e.g., via s.ds.Host or the
datastore read helper used elsewhere in this test file) and assert the stored
BitLockerProtectionStatus equals the value you passed; ensure you reference the
same host.ID and SetOrUpdateHostDisksEncryption function used in the existing
test so the new assertion covers the Windows non-nil branch.
In `@server/service/osquery_utils/queries_test.go`:
- Around line 1956-1958: The mock SetOrUpdateHostDisksEncryptionFunc used in
tests should also assert that the new bitlockerProtectionStatus argument is nil
for non-Windows ingestion paths; update the anonymous function assigned to
ds.SetOrUpdateHostDisksEncryptionFunc (the mock that currently asserts
expectEncrypted) to include assert.Nil(t, bitlockerProtectionStatus) so the test
validates the new parameter is nil (apply the same change to the other
occurrence around lines referenced).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0482736d-9a22-49a3-a4ad-4fc888e372dc
📒 Files selected for processing (21)
changes/issue-40809-bitlocker-looporbit/changes/issue-40809-bitlocker-key-rotationorbit/pkg/bitlocker/bitlocker_management_windows.goorbit/pkg/bitlocker/bitlocker_worker_notwindows.goorbit/pkg/bitlocker/bitlocker_worker_windows.goorbit/pkg/update/notifications.goorbit/pkg/update/notifications_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260407120000_AddBitlockerProtectionStatusToHostDisks.goserver/datastore/mysql/schema.sqlserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_test.goserver/service/osquery_utils/queries.goserver/service/osquery_utils/queries_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43130 +/- ##
=======================================
Coverage 66.87% 66.87%
=======================================
Files 2588 2590 +2
Lines 207563 207603 +40
Branches 9321 9311 -10
=======================================
+ Hits 138803 138840 +37
- Misses 56123 56124 +1
- Partials 12637 12639 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Member
Author
|
@claude review |
# Conflicts: # server/datastore/mysql/schema.sql
Member
Author
|
@ksykulev Claude review keeps finding new issues and it takes ~45 minutes to review. I'll ping you when this is ready for re-review. Part of our issue is that we don't have unit tests that run on Windows. I'll plan on adding Windows to our CI. |
# Conflicts: # server/datastore/mysql/schema.sql
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
# Conflicts: # server/datastore/mysql/schema.sql
# Conflicts: # server/datastore/mysql/schema.sql
sgress454
approved these changes
Apr 9, 2026
Contributor
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
host vitals doc matches new query 👍
ksykulev
approved these changes
Apr 9, 2026
ksykulev
pushed a commit
that referenced
this pull request
Apr 13, 2026
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #40809 **Orbit agent: key rotation replaces decrypt-then-re-encrypt:** - When the disk is already encrypted, orbit now adds a new Fleet-managed recovery key protector, removes old ones, and escrows the new key. The disk is never decrypted. - If key escrow fails, the rotated key is cached in memory and retried on subsequent ticks without rotating again. - Removes `DecryptVolume` and `decrypt()` (no longer called from production code). **Server: osquery query returns both protection_status and conversion_status:** - The `disk_encryption_windows` query now returns both columns instead of just checking `protection_status = 1`. This lets the server correctly identify a disk as encrypted via `conversion_status = 1` even when `protection_status = 0`. - New `directIngestDiskEncryptionWindows` function parses both values, handles parse errors, and normalizes `protection_status = 2` (unknown) to NULL. **Server: new `bitlocker_protection_status` column and status logic:** - Adds `bitlocker_protection_status` column to `host_disks` (DB migration). - When a disk is encrypted and key is escrowed but protection is off, the host shows "Action required" with a detail message explaining the issue, instead of misleadingly showing "Verified." - `protection_status = 2` (unknown) and `NULL` (older orbit hosts) are treated as protection on for backward compatibility. - The `profiles_verified` and `profiles_verifying` branches in the combined profiles+BitLocker summary now handle `bitlocker_action_required`, counting those hosts as "pending". Contributor docs updates: #43241 Public docs updates: https://github.com/fleetdm/fleet/pull/43243/changes # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Database migrations - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. ## fleetd/orbit/Fleet Desktop - [x] Verified compatibility with the latest released version of Fleet (see [Must rule](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/workflows/fleetd-development-and-release-strategy.md)) - [x] If the change applies to only one platform, confirmed that `runtime.GOOS` is used as needed to isolate changes - [x] Verified that fleetd runs on macOS, Linux and Windows - [x] Verified auto-update works from the released version of component to the new version (see [tools/tuf/test](../tools/tuf/test/README.md)) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed Windows BitLocker encryption/decryption request loop on systems with secondary drives and auto-unlock. * **New Features** * Added BitLocker recovery key rotation capability, allowing safe key updates without full disk re-encryption. * Enhanced BitLocker protection status tracking to correctly display "Action required" when protection is disabled. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #40809
Orbit agent: key rotation replaces decrypt-then-re-encrypt:
DecryptVolumeanddecrypt()(no longer called from production code).Server: osquery query returns both protection_status and conversion_status:
disk_encryption_windowsquery now returns both columns instead of just checkingprotection_status = 1. This lets the server correctly identify a disk as encrypted viaconversion_status = 1even whenprotection_status = 0.directIngestDiskEncryptionWindowsfunction parses both values, handles parse errors, and normalizesprotection_status = 2(unknown) to NULL.Server: new
bitlocker_protection_statuscolumn and status logic:bitlocker_protection_statuscolumn tohost_disks(DB migration).protection_status = 2(unknown) andNULL(older orbit hosts) are treated as protection on for backward compatibility.profiles_verifiedandprofiles_verifyingbranches in the combined profiles+BitLocker summary now handlebitlocker_action_required, counting those hosts as "pending".Contributor docs updates: #43241
Public docs updates: https://github.com/fleetdm/fleet/pull/43243/changes
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Database migrations
fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
Release Notes
Bug Fixes
New Features