Clear the escrowed managed local account password on MDM re-enrollment - #50596
Conversation
host_managed_local_account_passwords is keyed by host_uuid and nothing removed it when a device re-enrolled, so a re-enrolled host inherited the previous install's escrowed password. Fleet reported status=verified and password_available=true for an account that no longer existed, handing an admin a break-glass password that silently does not work. Observed on a re-imaged Windows host that re-enrolled with the same hardware UUID. Add a `deleted` column and soft-delete the row on re-enrollment, mirroring host_recovery_key_passwords. Soft rather than hard delete so no lifecycle event destroys a password; every read filters deleted = 0 and the next successful escrow (or a device-reported failure) clears the flag and revives the row. Hooked into both platforms, since the table is shared: Windows via MDMWindowsDeleteEnrolledDeviceOnReenrollment, macOS via MDMResetEnrollment's darwin case, beside the equivalent recovery lock call and after the scepRenewalInProgress short-circuit so a SCEP renewal does not retire anything. Deliberately not hooked into MDM turn-off, unlike recovery lock: Apple wipes the device-side recovery lock when the profile is removed, whereas a local admin account survives profile removal with its password still valid. Only re-enrollment invalidates the escrow.
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds a Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/managed_local_account.go`:
- Around line 75-76: Update the upsert handling around the managed local account
row so an escrow failure that clears deleted status also replaces or invalidates
encrypted_password before GetHostManagedLocalAccountPassword can return it,
while preserving client_error. Add a regression test covering re-enrollment,
escrow failure, and subsequent password retrieval to verify the
pre-re-enrollment password is not exposed.
- Line 326: Update the fallback checkStmt in InitiateManagedLocalAccountRotation
to include the same deleted = 0 filter as the eligibility update query, so
soft-deleted accounts are treated as absent rather than returning stale
eligibility state. Add a test covering rotation initiation after soft deletion
and assert it returns the not-found error.
🪄 Autofix
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 Plus
Run ID: d7be8300-3cb8-452f-8e8e-69e5952238b5
📒 Files selected for processing (6)
server/datastore/mysql/apple_mdm.goserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/migrations/tables/20260805161502_AddManagedLocalAccountDeletedColumn.goserver/datastore/mysql/schema.sql
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR prevents stale escrowed managed local account passwords from being shown after MDM re-enrollment by introducing a soft-delete mechanism on host_managed_local_account_passwords, then retiring (soft-deleting) the row during re-enrollment flows for both Windows and macOS.
Changes:
- Add
deletedcolumn tohost_managed_local_account_passwordsand filter all reads/updates ondeleted = 0. - Soft-delete managed local account password rows on re-enrollment/reset-enrollment for Windows and macOS.
- Add unit test coverage for the soft-delete behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/datastore/mysql/schema.sql | Adds deleted column to host_managed_local_account_passwords and updates schema snapshot/migration status. |
| server/datastore/mysql/migrations/tables/20260805161502_AddManagedLocalAccountDeletedColumn.go | Migration adding deleted column to the managed local account passwords table. |
| server/datastore/mysql/microsoft_mdm.go | Soft-deletes managed local account password on Windows device re-enrollment cleanup. |
| server/datastore/mysql/managed_local_account.go | Implements DB soft-delete helper and applies deleted = 0 filtering across managed local account queries. |
| server/datastore/mysql/managed_local_account_test.go | Adds test for soft-delete + revive-on-escrow/error behavior. |
| server/datastore/mysql/apple_mdm.go | Soft-deletes managed local account password during macOS MDMResetEnrollment cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50596 +/- ##
===========================================
- Coverage 68.35% 56.28% -12.08%
===========================================
Files 3955 3691 -264
Lines 253450 228639 -24811
Branches 13330 13284 -46
===========================================
- Hits 173255 128693 -44562
- Misses 64703 87643 +22940
+ Partials 15492 12303 -3189
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/datastore/mysql/microsoft_mdm.go:597
- This adds the re-enrollment hook to retire managed local account passwords, but there’s no assertion in the existing re-enrollment cleanup test that the row is actually soft-deleted (deleted=1). Right now microsoft_mdm_test.go only checks that encrypted_password remains present; that would still pass even if the soft-delete call were removed later. Consider extending the existing test to assert deleted is set to 1 (and optionally that datastore reads treat it as not found).
// Retire the escrowed managed local account password. The row is keyed by host_uuid and survives host
// deletion on purpose, but it must not survive the enrollment that produced it: a re-enrolling device may
// have been re-imaged, in which case the password no longer opens any account and Fleet would keep
// reporting it as verified. Soft delete, so the secret is never destroyed by a lifecycle event; the next
// successful escrow clears the flag and revives the row.
server/datastore/mysql/apple_mdm.go:4680
- MDMResetEnrollment now also retires the managed local account password for darwin, but the existing MDM reset enrollment tests don’t appear to cover this new behavior. Consider extending an existing test (e.g., testMDMAppleResetEnrollment) to create a managed local account password row and assert it becomes soft-deleted on reset (and is filtered from reads).
// Same reasoning for the managed local account password, which shares the escrow model: reaching
// here means an unenroll/re-enroll or wipe/restore, and an ADE re-enrollment normally follows an
// erase, so the account the password opens is usually gone. Note the deliberate difference from
// recovery lock: we do NOT soft-delete on MDM turn-off, because a local admin account survives
// profile removal and its password stays valid, whereas Apple wipes the device-side recovery lock.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/datastore/mysql/managed_local_account.go:230
lookupManagedLocalAccountHostis used to resolve a host bypending_command_uuid, but the table schema has no index onpending_command_uuid. With the newAND deleted = 0predicate, this lookup can devolve into a full table scan during MDM command result processing on large fleets. Consider adding an index (ideally composite) to keep these lookups efficient.
// lookupManagedLocalAccountHost shares the join-to-hosts lookup used by both the
// AccountConfiguration ack (matches command_uuid) and the SetAutoAdminPassword ack
// (matches pending_command_uuid). The column name is interpolated, not parameterized,
// because callers pass a fixed identifier — never untrusted input.
func (ds *Datastore) lookupManagedLocalAccountHost(ctx context.Context, column, commandUUID string) (*fleet.Host, error) {
stmt := fmt.Sprintf(`SELECT host_uuid FROM host_managed_local_account_passwords WHERE %s = ? AND deleted = 0`, column)
server/datastore/mysql/apple_mdm.go:4676
- This comment is grammatically awkward and missing ending punctuation. Consider rephrasing to match the adjacent recovery lock comment and end with a period.
// Same reasoning for the managed local account password, as recovery lock password, which shares the escrow model
…a-password-on-reenrollment # Conflicts: # server/datastore/mysql/schema.sql
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| func (ds *Datastore) GetHostManagedLocalAccountPassword(ctx context.Context, hostUUID string) (*fleet.HostManagedLocalAccountPassword, error) { | ||
| const stmt = `SELECT encrypted_password, updated_at FROM host_managed_local_account_passwords WHERE host_uuid = ?` | ||
| const stmt = `SELECT encrypted_password, updated_at FROM host_managed_local_account_passwords WHERE host_uuid = ? AND deleted = 0` | ||
|
|
There was a problem hiding this comment.
The recovery lock has SoftDeleteRecoveryLockPasswordsForUnenrolledHosts (a cron that catches hosts where MDM was disabled without a proper CheckOut/Authenticate). There's no equivalent for MLA. If a host silently unenrolls (e.g., user manually removes MDM profile, only detected via osquery refetch), the MLA password remains deleted = 0 and visible. This may be intentionally deferred, but worth noting for completeness since the PR mirrors the recovery lock pattern in all other respects.
There was a problem hiding this comment.
@sharon-fdm @coderabbitai Recovery lock and admin password have different lifetimes. If MDM is disabled or profile is removed, the admin password is still valid, unlike the recovery lock. So, we should not soft delete the admin password.
There was a problem hiding this comment.
@getvictor, agreed. The managed local account password remains valid after MDM removal or disablement. It must remain available in that case. The recovery lock cleanup pattern does not apply here.
I am withdrawing the comment.
🐇
✏️ Learnings added
Learnt from: getvictor
Repo: fleetdm/fleet PR: 50596
File: server/datastore/mysql/managed_local_account.go:105-105
Timestamp: 2026-08-06T19:16:43.189Z
Learning: For Fleet managed local account passwords, MDM disablement or removal of the MDM profile does not invalidate the administrator password. Do not soft-delete `host_managed_local_account_passwords` merely because MDM is unenrolled or disabled. This differs from recovery lock passwords, which have a different lifetime and cleanup behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
|
||
| func Down_20260805161502(tx *sql.Tx) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing corresponding _test.go file. Fleet convention requires a test for every migration (pattern: applyUpToPrev(t) -> set up data -> applyNext(t, db) -> verify). Every other migration in the directory follows this pattern.
There was a problem hiding this comment.
@sharon-fdm No, we don't always add a _test.go for simple migrations like this one that just add a column.
|
@sharon-fdm I fixed or replied to your comments. |
Fixed unreleased bug where admin password was still shown on a wiped host after re-enrollment.
Add a
deletedcolumn and soft-delete the row on re-enrollment, mirroring host_recovery_key_passwords.Related issue: Resolves #43488
Checklist for submitter
Testing
Database migrations
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes