Set recovery lock password - mdm commands

[mostlikelee]

Related issue: Resolves #40656

• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Database migrations

• Checked schema for all modified table for columns that will auto-update timestamps during migration.
• Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

Summary by CodeRabbit

• New Features
  • macOS recovery lock password support: secure generation, encrypted storage, and per-device delivery
  • Automatic scheduler sending recovery lock commands every 5 minutes to eligible enrolled devices
  • Delivery status tracking with verification and failure handling for recovery lock commands
  • Host-scoped secret expansion so device-specific secrets (e.g., recovery passwords) are injected at command delivery

[getvictor]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

server/service/apple_mdm.go (1)

7352-7375: ⚠️ Potential issue | 🟠 Major

Scope recovery-lock transitions to the command UUID.

Line 7354 already has the command_uuid, but Lines 7361 and 7374 ignore it. A late or duplicated result from an older SetRecoveryLock can still flip the current pending password to verified/failed after a newer rotation has been queued for the same host. Please make the datastore mutation conditional on results.UUID() matching the host's pending command.

🧩 Suggested change

-			if err := ds.SetRecoveryLockVerified(ctx, hostUUID); err != nil {
+			if err := ds.SetRecoveryLockVerified(ctx, hostUUID, results.UUID()); err != nil {
 				return ctxerr.Wrap(ctx, err, "SetRecoveryLock handler: set recovery lock verified")
 			}
...
-			if err := ds.SetRecoveryLockFailed(ctx, hostUUID, errorMsg); err != nil {
+			if err := ds.SetRecoveryLockFailed(ctx, hostUUID, results.UUID(), errorMsg); err != nil {
 				return ctxerr.Wrap(ctx, err, "SetRecoveryLock handler: set recovery lock failed")
 			}

The datastore methods should no-op unless the pending recovery-lock command UUID matches results.UUID().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/service/apple_mdm.go` around lines 7352 - 7375, The SetRecoveryLock
handling must avoid flipping a newer pending rotation by ensuring datastore
updates only apply to the command that produced the result: change the mutations
so they are conditional on results.UUID(). Specifically, update the calls to
ds.SetRecoveryLockVerified and ds.SetRecoveryLockFailed (or their
implementations) to accept the command UUID (results.UUID()) and verify the
host's current pending recovery-lock command matches that UUID before making any
state change; if it doesn't match, return no-op. Ensure the verification uses
the host's pending recovery-lock command id stored in the datastore and keep the
existing error wrapping/logging behavior.

server/mdm/apple/apple_mdm.go (1)

1658-1677: ⚠️ Potential issue | 🟠 Major

Persisting pending before enqueue still leaves a stuck-state window.

If the process dies after Line 1661 but before Line 1677, the host keeps a stored password plus pending status without any queued command. At that point GetHostsForRecoveryLockAction won't select it again, so the recovery-lock flow can stall permanently. This needs either atomic “persist + enqueue” behavior or a recovery path for stale pending rows.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/mdm/apple/apple_mdm.go` around lines 1658 - 1677, The current flow
writes passwords with status='pending' via SetHostsRecoveryLockPasswords then
calls commander.SetRecoveryLock, leaving a window where the process can die and
hosts remain pending forever; fix by making the persist+enqueue atomic or adding
a recovery for stale pendings: either (A) change SetHostsRecoveryLockPasswords
to accept and store the cmdUUID (pass cmdUUID into SetHostsRecoveryLockPasswords
and persist passwords together with that cmdUUID in one DB transaction so the
enqueue (commander.SetRecoveryLock) records the same cmd and you can atomically
commit both sides), or (B) add a pending_timestamp column when setting pending
and update GetHostsForRecoveryLockAction to include pending rows older than a
configured timeout (and/or add a background cleaner that resets stale pending
rows), updating references to SetHostsRecoveryLockPasswords,
commander.SetRecoveryLock, GetHostsForRecoveryLockAction, and ExpandHostSecrets
accordingly.

🤖 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/apple_mdm_test.go`:
- Around line 10245-10398: Add two test cases inside
testGetHostsForRecoveryLockAction: (1) cover the "unset" path by creating a host
that already has a stored/verified recovery lock (use
SetHostsRecoveryLockPasswords and SetRecoveryLockVerified on a darwin/arm host
created via test.NewHost and nanoEnrollAndSetHostMDMData), then turn off the
recovery-lock config (call createTeamWithRecoveryLock(..., true) then SaveTeam
to disable or call setAppConfigRecoveryLock(false)) and assert
GetHostsForRecoveryLockAction no longer returns that host; (2) cover the
duplicate-selection case by creating a macOS host that has both device and user
Nano enrollments (call nanoEnrollAndSetHostMDMData and nanoEnroll so both
enrollments exist) and assert GetHostsForRecoveryLockAction returns that host at
most once (no duplicate UUIDs in the result). Use the existing helpers
(createTeamWithRecoveryLock, setAppConfigRecoveryLock,
nanoEnrollAndSetHostMDMData, nanoEnroll, SetHostsRecoveryLockPasswords,
SetRecoveryLockVerified, GetHostsForRecoveryLockAction) to implement these
checks.

In `@server/datastore/mysql/secret_variables.go`:
- Around line 441-477: The ExpandHostSecrets function inserts decrypted host
secrets verbatim into plist XML causing malformed output for characters like &
and <; modify ExpandHostSecrets so that when handling
fleet.HostSecretRecoveryLockPassword (from getHostRecoveryLockPasswordDecrypted)
you XML-escape the password the same way the server-embedded secret path does
before storing it in secretValues (or during the MaybeExpand callback), i.e.,
apply the existing XML-escaping utility used by ExpandEmbeddedSecrets to the
decrypted value so the expanded plist content is always XML-safe.

In `@server/mdm/nanomdm/service/nanomdm/service.go`:
- Around line 289-318: The SetRecoveryLock branch updates cmd.Raw with the
expanded plist but never reparses it into cmd.Command (and cmd.Raw is ignored
during serialization per mdm/command.go's `Raw` plist:"-"), so the device still
receives the unexpanded payload; after getting hostExpanded from
s.store.ExpandHostSecrets in the SetRecoveryLock branch, re-decode/parses
hostExpanded into cmd.Command (or otherwise populate the fields used for
serialization) before returning so the outbound command sent by the codepath
that returns &cmd.Command contains the expanded plist (reference symbols:
s.store.ExpandHostSecrets, cmd.Raw, cmd.Command, SetRecoveryLockCmdName).

---

Duplicate comments:
In `@server/mdm/apple/apple_mdm.go`:
- Around line 1658-1677: The current flow writes passwords with status='pending'
via SetHostsRecoveryLockPasswords then calls commander.SetRecoveryLock, leaving
a window where the process can die and hosts remain pending forever; fix by
making the persist+enqueue atomic or adding a recovery for stale pendings:
either (A) change SetHostsRecoveryLockPasswords to accept and store the cmdUUID
(pass cmdUUID into SetHostsRecoveryLockPasswords and persist passwords together
with that cmdUUID in one DB transaction so the enqueue
(commander.SetRecoveryLock) records the same cmd and you can atomically commit
both sides), or (B) add a pending_timestamp column when setting pending and
update GetHostsForRecoveryLockAction to include pending rows older than a
configured timeout (and/or add a background cleaner that resets stale pending
rows), updating references to SetHostsRecoveryLockPasswords,
commander.SetRecoveryLock, GetHostsForRecoveryLockAction, and ExpandHostSecrets
accordingly.

In `@server/service/apple_mdm.go`:
- Around line 7352-7375: The SetRecoveryLock handling must avoid flipping a
newer pending rotation by ensuring datastore updates only apply to the command
that produced the result: change the mutations so they are conditional on
results.UUID(). Specifically, update the calls to ds.SetRecoveryLockVerified and
ds.SetRecoveryLockFailed (or their implementations) to accept the command UUID
(results.UUID()) and verify the host's current pending recovery-lock command
matches that UUID before making any state change; if it doesn't match, return
no-op. Ensure the verification uses the host's pending recovery-lock command id
stored in the datastore and keep the existing error wrapping/logging behavior.

🪄 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: 483a474f-9877-4335-ae2b-26841483629d

📥 Commits

Reviewing files that changed from the base of the PR and between 3c68840 and 2542ae5.

📒 Files selected for processing (10)

• cmd/fleet/cron.go
• server/datastore/mysql/apple_mdm_test.go
• server/datastore/mysql/secret_variables.go
• server/fleet/datastore.go
• server/mdm/apple/apple_mdm.go
• server/mdm/apple/apple_mdm_test.go
• server/mdm/nanomdm/service/nanomdm/service.go
• server/mock/datastore_mock.go
• server/service/apple_mdm.go
• server/service/apple_mdm_cmd_results_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• server/mdm/apple/apple_mdm_test.go

[getvictor]

@mostlikelee Looks good overall. Please address any remaining comments and ping me to approve.

[mostlikelee]

@getvictor pending CI, but comments are addressed