Fix Recovery Lock password desync on MDM re-enrollment#43827
Fix Recovery Lock password desync on MDM re-enrollment#43827mostlikelee merged 14 commits intomainfrom
Conversation
Adds the datastore-layer coverage for the MDMResetEnrollment soft-delete and the DeleteHost non-cascade: reset behavior on every row state, null-propagation guards against re-animation leaks, a table-driven host-detail status matrix, and notFound contracts for view/rotate readers on soft-deleted rows.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43827 +/- ##
==========================================
- Coverage 65.10% 65.09% -0.01%
==========================================
Files 2604 2604
Lines 253883 253772 -111
Branches 9252 9334 +82
==========================================
- Hits 165284 165199 -85
+ Misses 75800 75768 -32
- Partials 12799 12805 +6
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:
|
Extends the re-enroll fix to the two other paths where MDM is disabled for a single host: - MDMTurnOff (device CheckOut + admin API unenroll) — both converge here and now soft-delete the recovery lock row. - SetOrUpdateMDMData (osquery refetch detecting manual profile removal) — wrapped in a tx that detects the darwin enrolled 1->0 transition and conditionally soft-deletes. Skips NULL->0, 0->0, 1->1, and non-darwin to avoid wiping a password that an admin still needs. The soft-delete SQL is now shared via softDeleteHostRecoveryLockPassword so all three lifecycle hooks (re-enroll, explicit unenroll, refetch) behave identically.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds logic to soft-delete stored Apple MDM recovery lock passwords when a host is detected as unenrolled and nulls pending/rotation/view fields. Soft-deletes are performed from MDM unenroll flows (MDM turn-off and reset) and via a new datastore-backed cron sweep invoked before sending recovery-lock commands. The datastore interface and mock were extended 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx`:
- Around line 105-106: The Prettier indentation error comes from the broken line
break in the boolean expression comparing cellProps.row.original.profile_uuid to
FLEET_ANDROID_CERTIFICATE_TEMPLATE_PROFILE_ID inside OSSettingsTableConfig (the
expression currently spans two lines); fix it by putting the entire comparison
on a single properly indented line or by wrapping the expression with
parentheses and aligning the second line to match project style so
Prettier/ESLint stop flagging it—locate the usage of
cellProps.row.original.profile_uuid and
FLEET_ANDROID_CERTIFICATE_TEMPLATE_PROFILE_ID in OSSettingsTableConfig.tsx and
reformat that comparison accordingly.
In `@server/service/integration_mdm_test.go`:
- Around line 23293-23297: Add an assertion to verify the recovery lock password
endpoint is blocked after re-enrollment: after calling
getHostRecoveryLockStatus(host.ID) and checking rlpStatus fields, perform the
same GET /recovery_lock_password check used in the admin-unenroll flow (the test
helper that fetches the recovery lock password for host.ID) and assert it
returns 404/not found. Apply the identical addition to the other flow around
lines 23399-23401 so both invalidation paths assert that recovery lock password
access is denied.
- Around line 23429-23447: Add an assertion that the recovery-lock row backing
the host was soft-deleted by the cron sweep: before calling
runRecoveryLockCron(t) optionally assert the recovery lock row for host.ID is
present/live (via the datastore method that reads recovery lock rows, e.g., the
same storage used by SoftDeleteRecoveryLockPasswordsForUnenrolledHosts), then
after runRecoveryLockCron(t) call the datastore read and assert the row's
deleted flag is set (or that the row is not returned as live). Locate the
relevant code around SetOrUpdateMDMData, runRecoveryLockCron,
SoftDeleteRecoveryLockPasswordsForUnenrolledHosts and getHostRecoveryLockStatus
to add these direct datastore assertions.
🪄 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: 92a2921c-4db3-4f5c-a38b-d98a257270d9
📒 Files selected for processing (12)
changes/43786-recovery-lock-reenroll-desynccmd/fleet/cron.gofrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsxfrontend/pages/hosts/details/cards/HostSummary/HostSummary.tsxserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/fleet/datastore.goserver/mdm/apple/apple_mdm.goserver/mock/datastore_mock.goserver/service/integration_mdm_test.go
mostlikelee
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
mostlikelee
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
There was a problem hiding this comment.
Pull request overview
This PR addresses stale macOS Recovery Lock passwords persisting in Fleet after an MDM unenroll/re-enroll lifecycle event by soft-deleting the stored password when a host leaves MDM, and hiding the Recovery Lock row in the UI when MDM is not “On”.
Changes:
- Soft-delete
host_recovery_key_passwordsrows on MDM lifecycle hooks (re-enroll/reset, turn-off/checkout) plus a cron-driven sweep for hosts reportinghost_mdm.enrolled=0. - Increase the Recovery Lock cron cadence to 30s and add integration/datastore tests covering unenroll/re-enroll and soft-delete reader behavior.
- UI guard: hide the Recovery Lock row unless the host’s MDM enrollment status is “On (…)”.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Adds integration coverage for re-enrollment, admin unenroll, CheckOut, and cron sweep soft-delete behaviors. |
| server/mock/datastore_mock.go | Extends datastore mock interface with SoftDeleteRecoveryLockPasswordsForUnenrolledHosts. |
| server/mdm/apple/apple_mdm.go | Adds cron-time sweep call to soft-delete recovery lock rows for unenrolled hosts. |
| server/fleet/datastore.go | Adds datastore interface method for cron-driven soft-delete sweep. |
| server/datastore/mysql/hosts_test.go | Trailing whitespace/newline adjustment. |
| server/datastore/mysql/hosts.go | Documents intentional preservation of host_recovery_key_passwords across host deletion. |
| server/datastore/mysql/apple_mdm_test.go | Adds datastore-level tests for reset/turn-off soft-delete semantics and reader behavior. |
| server/datastore/mysql/apple_mdm.go | Implements soft-delete helper, hooks it into MDMTurnOff/MDMResetEnrollment, and adds the unenrolled-host sweep query. |
| frontend/pages/hosts/details/cards/HostSummary/HostSummary.tsx | Hides Recovery Lock row unless MDM enrollment status is “On (…)”. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx | Same UI hiding behavior for the OS settings modal table. |
| cmd/fleet/cron.go | Changes Recovery Lock cron default interval from 5m to 30s. |
| changes/43786-recovery-lock-reenroll-desync | Release note for the Recovery Lock desync fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/integration_mdm_test.go (1)
23266-23291: Add a re-enroll case that leavesSetRecoveryLockpending.Line 23273 acknowledges the command before re-enrollment, so this only covers verified-row invalidation. The PR also depends on
MDMResetEnrollmentcleaning up stuck pending rows abandoned when nanomdm clears the queue; add a sibling subtest that runs cron, receivesSetRecoveryLock, intentionally does not acknowledge it, then callsmdmClient.Reenroll()and asserts status/password access are cleared.Test coverage sketch
+ t.Run("re-enrollment soft-deletes abandoned pending recovery lock password", func(t *testing.T) { + s.DoJSON("PATCH", "/api/latest/fleet/config", map[string]any{ + "mdm": map[string]any{"enable_recovery_lock_password": true}, + }, http.StatusOK, &appConfigResponse{}) + + host, mdmClient := createAppleSiliconHost(t) + runRecoveryLockCron(t) + + cmd, err := mdmClient.Idle() + require.NoError(t, err) + require.NotNil(t, cmd) + require.Equal(t, "SetRecoveryLock", cmd.Command.RequestType) + + // Do not acknowledge; re-enrollment should clear the abandoned pending row. + require.NoError(t, mdmClient.Reenroll()) + + rlpStatus := getHostRecoveryLockStatus(host.ID) + assert.Nil(t, rlpStatus.Status) + assert.False(t, rlpStatus.PasswordAvailable) + s.Do("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d/recovery_lock_password", host.ID), nil, http.StatusNotFound) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_mdm_test.go` around lines 23266 - 23291, Add a sibling subtest in integration_mdm_test.go that exercises the re-enroll path which leaves SetRecoveryLock pending: call runRecoveryLockCron(t), then mdmClient.Idle() to receive the SetRecoveryLock command but do NOT call mdmClient.Acknowledge(cmd.CommandUUID), then call mdmClient.Reenroll(), and finally assert that getHostRecoveryLockStatus(host.ID) returns a nil Status and PasswordAvailable == false and that the GET /api/latest/fleet/hosts/{id}/recovery_lock_password either returns no RecoveryLockPassword or a 404 (use s.DoJSON and assert accordingly). Use the same helpers (runRecoveryLockCron, mdmClient.Idle, mdmClient.Reenroll, getHostRecoveryLockStatus, s.DoJSON) and mirror the structure of the existing verified-case subtest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/integration_mdm_test.go`:
- Around line 23266-23291: Add a sibling subtest in integration_mdm_test.go that
exercises the re-enroll path which leaves SetRecoveryLock pending: call
runRecoveryLockCron(t), then mdmClient.Idle() to receive the SetRecoveryLock
command but do NOT call mdmClient.Acknowledge(cmd.CommandUUID), then call
mdmClient.Reenroll(), and finally assert that getHostRecoveryLockStatus(host.ID)
returns a nil Status and PasswordAvailable == false and that the GET
/api/latest/fleet/hosts/{id}/recovery_lock_password either returns no
RecoveryLockPassword or a 404 (use s.DoJSON and assert accordingly). Use the
same helpers (runRecoveryLockCron, mdmClient.Idle, mdmClient.Reenroll,
getHostRecoveryLockStatus, s.DoJSON) and mirror the structure of the existing
verified-case subtest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9bb1d1ef-92cd-4d0f-9e95-8371aca2508c
📒 Files selected for processing (5)
frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsxfrontend/pages/hosts/details/cards/HostSummary/HostSummary.tsxserver/datastore/mysql/apple_mdm.goserver/mdm/apple/apple_mdm_test.goserver/service/integration_mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/apple_mdm.go
JordanMontgomery
left a comment
JordanMontgomery
left a comment
There was a problem hiding this comment.
Just as a general comment you may need to add some special treatment for SCEP renewal. It's worth testing. When a device goes through SCEP renewal it has to reinstall the enrollment profile, get a new MDM certificate and afterwards it does an Authenticate and a TokenUpdate
There are some integration tests for this but for testing on an actual system you'll want to modify the not_after(or something like that) column in nano_cert_auth_associations for the latest row for the host. If below 6 months it should trigger a SCEP renewal next time cleanups_then_aggregations runs.
Would be interesting to know if it somehow clears the recovery lock on the host(it shouldn't) and make sure that doesn't clear it in the fleet db. We have some special handling in device lifecycle and authenticate, so it shouldn't, but good to test as we've had other bugs around SCEP renewal clearing things it shouldn't
# Conflicts: # server/fleet/datastore.go # server/mock/datastore_mock.go
@JordanMontgomery great catch! in theory we should be covered here since recovery passwords are removed during re-enrollment, but i'll make sure to test. |
|
Just to follow up, the scep renewal didn't seem to touch the recovery lock password. it kept the password in place and didn't need to resend the command.
|
Related issue: Resolves #43786
Summary
Apple wipes a macOS host's Recovery Lock password when the MDM profile is removed, but Fleet's
host_recovery_key_passwordsstate machine had no hook into MDM lifecycle events, so the stored password stayedverifiedeven after it was gone from the device. Admins viewing or rotating the password would act on a stale value.Fleet now soft-deletes the stored password whenever MDM is turned off for a host:
MDMResetEnrollment(fires on everyAuthenticate). Also cleans up stuck-pending rows whoseSetRecoveryLockcommand was abandoned by nanomdm'sClearQueue.MDMTurnOff(fires on device CheckOut and adminPATCH /mdm/hosts/{id}/unenroll).host_mdm.enrolled=0on the next tick. The recovery-lock cron interval is bumped from 5 min → 30 s to match the other MDM profile-manager crons, so the API-staleness window is ~30 s.On (*).All four hooks share a single
softDeleteHostRecoveryLockPasswordhelper (soft-delete + nullpending_encrypted_password/pending_error_message/auto_rotate_atto prevent leakage into the re-animated row on re-enroll). Host deletion continues to preserve the row — the device may still be enrolled and an Orbit re-enroll should retain the stored password.Checklist
changes/43786-recovery-lock-reenroll-desync).SELECT *.Testing
host_uuid.Database migrations
New Fleet configuration settings
fleetd/orbit/Fleet Desktop
Summary by CodeRabbit