Victor 48723 windows mla fleetd - #50087
Closed
getvictor wants to merge 33 commits into
Closed
Conversation
…managed-local-account
Adds the server half of the Windows managed local admin account device flow: the orbit notification that tells fleetd to create the account, the orbit escrow endpoint that stores the device-generated password, and the host-facing endpoint changes that let admins retrieve a Windows password. - Migration: make host_managed_local_account_passwords.command_uuid nullable (Windows rows have no MDM command). - Capability windows_managed_local_account, advertised by Windows fleetd. - Notification create_windows_managed_local_account, gated on ESP phase, the team/No-team setting, the capability, and premium license. - Escrow endpoint POST /api/fleet/orbit/managed_local_account: verifies the host's Windows MDM enrollment, stores the password (verified, NULL command_uuid), and logs the created activity. Per OpenSpec it does not reject when the setting/license changed after the notification, to avoid orphaning the on-device account. - Host endpoints: retrieval and detail response support Windows without arming the auto-rotate timer; rotation stays macOS-only.
…8721-windows-mla-server # Conflicts: # server/datastore/mysql/schema.sql
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+95
to
+100
| if exists { | ||
| info := userInfo1003{Password: passwordPtr} | ||
| ret, _, _ := procNetUserSetInfo.Call( | ||
| 0, // servername: NULL means the local machine | ||
| uintptr(unsafe.Pointer(namePtr)), | ||
| userInfoPasswordOnly, |
There was a problem hiding this comment.
2. Existing accounts remain unusable 🐞 Bug ≡ Correctness
When _fleetadmin already exists, ensureUser resets only its password and does not enable the account or restore the non-expiring-password flags applied during creation. Fleet can consequently mark and escrow the account as verified even though the break-glass credentials cannot be used.
Agent Prompt
## Issue description
Provisioning an existing `_fleetadmin` account updates only its password, leaving disabled or expiring account state unchanged even though the password is subsequently escrowed as verified.
## Issue Context
Query and repair the existing account's flags on every provisioning run. Ensure the account is enabled, remains a normal account, and has password expiration disabled without unintentionally discarding unrelated required flags; add Windows-focused tests for disabled and expiring existing accounts.
## Fix Focus Areas
- orbit/pkg/managedaccount/managedaccount_windows.go[14-25]
- orbit/pkg/managedaccount/managedaccount_windows.go[78-120]
- orbit/pkg/managedaccount/managedaccount.go[115-132]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for Windows “managed local admin account” provisioning/escrow, spanning server notification gating, a new Orbit escrow endpoint, datastore persistence, and a new Windows-only fleetd (Orbit) receiver that creates/hides the _fleetadmin account and reports success/failure back to Fleet.
Changes:
- Server: gates and emits
create_windows_managed_local_accountin Orbit config for premium + capable Windows MDM hosts until per-enrollment escrow is completed. - Server: adds
/api/fleet/orbit/managed_local_accountendpoint to accept password escrow orclient_error, persists status/detail, and logs a one-time “created managed local account” activity. - Orbit (fleetd): introduces
orbit/pkg/managedaccountreceiver to generate a compliant password, provision/hide the account on Windows, and escrow/report errors.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/orbit.go | Adds notification gating, team/no-team setting resolution, and new escrow endpoint handler + service method. |
| server/service/orbit_test.go | Adds unit coverage for notification gating and escrow endpoint behavior (validation, truncation, activity). |
| server/service/hosts.go | Factors and reuses managed local account status population for host details (Windows + macOS paths). |
| server/service/hosts_test.go | Ensures Windows host details surface managed local account status. |
| server/service/handler.go | Registers the new Orbit escrow route under Windows MDM middleware. |
| server/mock/service/service_mock.go | Extends service mock with EscrowWindowsManagedLocalAccountPassword. |
| server/mock/datastore_mock.go | Extends datastore mock with escrow save/error + per-enrollment flag setter. |
| server/fleet/service.go | Adds EscrowWindowsManagedLocalAccountPassword to the service interface. |
| server/fleet/orbit.go | Adds CreateWindowsManagedLocalAccount Orbit config notification field. |
| server/fleet/microsoft_mdm.go | Extends Windows host config state with ManagedLocalAccountEscrowed flag. |
| server/fleet/hosts.go | Adds IsWindowsPlatform helper; moves/defines ManagedLocalAccountUsername + adds status detail field. |
| server/fleet/datastore.go | Adds datastore APIs for Windows escrow save/error + per-enrollment escrowed flag. |
| server/fleet/capabilities.go | Advertises CapabilityWindowsManagedLocalAccount from Windows fleetd capability set. |
| server/fleet/apple_mdm.go | Removes ManagedLocalAccountUsername from Apple-only file (now platform-neutral). |
| server/fleet/api_orbit.go | Defines Orbit request/response types for managed local account escrow. |
| server/datastore/mysql/schema.sql | Updates schema for nullable command_uuid/encrypted_password, adds client_error + managed_local_account_escrowed. |
| server/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns.go | Migration adding columns + relaxing nullability. |
| server/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns_test.go | Verifies migration works against populated tables and defaults are correct. |
| server/datastore/mysql/microsoft_mdm.go | Implements SetMDMWindowsManagedLocalAccountEscrowed + extends config-state query. |
| server/datastore/mysql/microsoft_mdm_test.go | Ensures per-enrollment escrow flag does not survive re-enrollment while password does. |
| server/datastore/mysql/managed_local_account.go | Adds escrow save + error reporting; treats NULL password as not-found; surfaces client_error as detail. |
| server/datastore/mysql/managed_local_account_test.go | Adds escrow lifecycle tests (save, retry, error, recovery, error-only row). |
| pkg/str/str.go | Adds TruncateRunes helper for rune-safe truncation prior to DB storage. |
| pkg/str/str_test.go | Adds tests for TruncateRunes behavior with ASCII and multi-byte input. |
| orbit/pkg/managedaccount/managedaccount.go | New receiver: single-flight guarded background create+escrow flow + password generator. |
| orbit/pkg/managedaccount/managedaccount_windows.go | Windows implementation using netapi32 + registry to create/reset user, add to Administrators, hide from sign-in. |
| orbit/pkg/managedaccount/managedaccount_stub.go | Non-Windows stub to prevent unsupported invocation. |
| orbit/pkg/managedaccount/managedaccount_test.go | Unit tests for receiver behavior, retry semantics, single-flight, and password complexity. |
| orbit/cmd/orbit/orbit.go | Registers the managedaccount receiver in the Windows orbitAction path. |
| orbit/changes/48723-windows-managed-local-account | Adds Orbit changelog entry for Windows managed local account support. |
| ee/server/service/hosts.go | Extends managed account password endpoint to allow Windows and blocks rotation for Windows. |
| client/orbit_client.go | Adds client method to POST the managed local account password/client_error to the server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1603
to
+1605
| if len(password) > managedLocalAccountMaxPasswordLength { | ||
| return &fleet.BadRequestError{Message: "managed local account password is too long"} | ||
| } |
Contributor
WalkthroughAdds Windows managed local account support across Orbit and the server. Orbit advertises a capability, receives configuration notifications, creates or updates and hides Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
🧹 Nitpick comments (3)
server/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns_test.go (1)
11-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the actual nullability change.
The test validates that pre-existing non-NULL data survives the migration, but never asserts that
encrypted_password/command_uuidcan beNULLpost-migration — which is the behavior this migration exists to enable. A regression here (e.g., a strayNOT NULLleft on either column) wouldn't be caught.♻️ Suggested addition after line 47
_, err = db.Exec(` INSERT INTO host_managed_local_account_passwords (host_uuid, encrypted_password, command_uuid, status) VALUES (?, NULL, NULL, ?)`, "new-host-null-cols", "pending") require.NoError(t, err, "encrypted_password and command_uuid should now accept NULL")🤖 Prompt for 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. In `@server/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns_test.go` around lines 11 - 48, Extend TestUp_20260728171642 to insert a new pending row into host_managed_local_account_passwords with both encrypted_password and command_uuid set to NULL after applyNext. Assert the insert succeeds, preserving the existing checks for migrated non-NULL data and enrollment defaults.orbit/pkg/managedaccount/managedaccount_windows.go (1)
80-131: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPassword-reset path doesn't re-affirm account flags.
ensureUser's existing-account branch only resets the password (USER_INFO_1003, password-only level). It never re-appliesufDontExpirePasswd/ufNormalAccount, unlikeaddToAdministratorsandhideFromSignInScreen, which are unconditionally reapplied on every run. If those flags are ever cleared out-of-band (GPO, admin edit, account disabled), a retry through this idempotent path silently leaves the account in a degraded state instead of self-healing it.Consider setting flags via a
USER_INFO_1008(flags-only) call, or re-running the fullUSER_INFO_1update, on the reset branch too, to match the "every step is idempotent" invariant documented aboveprovisionAccount.♻️ Suggested fix: also reaffirm account flags on the reset branch
if exists { - info := userInfo1003{Password: passwordPtr} - ret, _, _ := procNetUserSetInfo.Call( + pwInfo := userInfo1003{Password: passwordPtr} + ret, _, _ := procNetUserSetInfo.Call( 0, // servername: NULL means the local machine uintptr(unsafe.Pointer(namePtr)), userInfoPasswordOnly, - uintptr(unsafe.Pointer(&info)), + uintptr(unsafe.Pointer(&pwInfo)), 0, // parm_err ) if ret != 0 { return fmt.Errorf("resetting password for %s: %w", username, windows.Errno(ret)) } + // Reaffirm flags in case they drifted out-of-band (GPO, admin edit, disabled account). + flagsInfo := struct{ Flags uint32 }{Flags: ufScript | ufNormalAccount | ufDontExpirePasswd} + ret, _, _ = procNetUserSetInfo.Call( + 0, + uintptr(unsafe.Pointer(namePtr)), + 1008, // USER_INFO_1008: flags-only update level + uintptr(unsafe.Pointer(&flagsInfo)), + 0, + ) + if ret != 0 { + return fmt.Errorf("resetting flags for %s: %w", username, windows.Errno(ret)) + } return nil }Since this touches Win32 API usage (
USER_INFO_1008level semantics), please double-check against current MSDN docs before applying.🤖 Prompt for 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. In `@orbit/pkg/managedaccount/managedaccount_windows.go` around lines 80 - 131, Update ensureUser’s existing-account branch to re-apply ufNormalAccount and ufDontExpirePasswd after resetting the password, using the documented USER_INFO_1008 flags-only NetUserSetInfo level or an equivalent full USER_INFO_1 update. Preserve the existing password-reset behavior and return errors for the flags update so retries restore the account’s required state.orbit/pkg/managedaccount/managedaccount.go (1)
89-95: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelease the mutex before signaling
doneto avoid a synchronization gap.
close(r.done)runs before the deferredr.mu.Unlock()(defers execute after the function body). A test or future caller that unblocks on<-r.doneand immediately callsRun()again could seeTryLock()fail because the mutex may not be released yet — this is exactly the pattern used inmanagedaccount_test.go's "a failed escrow leaves nothing that would block a retry" test, which reassignsr.doneand callsRunright after<-done. Currently harmless in production (r.doneis always nil there), but it's a latent flakiness risk for this test pattern.♻️ Proposed fix: unlock before closing done
go func() { - defer r.mu.Unlock() - r.createAndEscrow() - if r.done != nil { - close(r.done) - } + defer func() { + r.mu.Unlock() + if r.done != nil { + close(r.done) + } + }() + r.createAndEscrow() }()🤖 Prompt for 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. In `@orbit/pkg/managedaccount/managedaccount.go` around lines 89 - 95, Update the goroutine in the managed-account run flow to release r.mu before signaling completion through r.done. Replace the deferred unlock ordering around createAndEscrow and close(r.done) so any caller unblocked by done can immediately retry Run without encountering the still-held mutex.
🤖 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.
Nitpick comments:
In `@orbit/pkg/managedaccount/managedaccount_windows.go`:
- Around line 80-131: Update ensureUser’s existing-account branch to re-apply
ufNormalAccount and ufDontExpirePasswd after resetting the password, using the
documented USER_INFO_1008 flags-only NetUserSetInfo level or an equivalent full
USER_INFO_1 update. Preserve the existing password-reset behavior and return
errors for the flags update so retries restore the account’s required state.
In `@orbit/pkg/managedaccount/managedaccount.go`:
- Around line 89-95: Update the goroutine in the managed-account run flow to
release r.mu before signaling completion through r.done. Replace the deferred
unlock ordering around createAndEscrow and close(r.done) so any caller unblocked
by done can immediately retry Run without encountering the still-held mutex.
In
`@server/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns_test.go`:
- Around line 11-48: Extend TestUp_20260728171642 to insert a new pending row
into host_managed_local_account_passwords with both encrypted_password and
command_uuid set to NULL after applyNext. Assert the insert succeeds, preserving
the existing checks for migrated non-NULL data and enrollment defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ba89c8e-bfb4-417a-8733-13787c795bbf
📒 Files selected for processing (32)
client/orbit_client.goee/server/service/hosts.goorbit/changes/48723-windows-managed-local-accountorbit/cmd/orbit/orbit.goorbit/pkg/managedaccount/managedaccount.goorbit/pkg/managedaccount/managedaccount_stub.goorbit/pkg/managedaccount/managedaccount_test.goorbit/pkg/managedaccount/managedaccount_windows.gopkg/str/str.gopkg/str/str_test.goserver/datastore/mysql/managed_local_account.goserver/datastore/mysql/managed_local_account_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns.goserver/datastore/mysql/migrations/tables/20260728171642_ManagedLocalAccountWindowsEscrowColumns_test.goserver/datastore/mysql/schema.sqlserver/fleet/api_orbit.goserver/fleet/apple_mdm.goserver/fleet/capabilities.goserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/microsoft_mdm.goserver/fleet/orbit.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/handler.goserver/service/hosts.goserver/service/hosts_test.goserver/service/orbit.goserver/service/orbit_test.go
💤 Files with no reviewable changes (1)
- server/fleet/apple_mdm.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50087 +/- ##
===========================================
- Coverage 68.00% 24.71% -43.29%
===========================================
Files 3930 2686 -1244
Lines 250248 112034 -138214
Branches 13172 13391 +219
===========================================
- Hits 170182 27693 -142489
- Misses 64757 83767 +19010
+ Partials 15309 574 -14735
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
server/service/orbit.go:1567
SkipAuthorizationrequires a justification comment starting withskipauth:(seeserver/authz/authz.go). The current comment doesn’t match that convention, which can lead to review/tooling failures and makes it harder to audit authorization skips.
pkg/str/str.go:42TruncateRunestruncates by doing[]rune(s)which allocates a full rune slice of the entire input. Because this helper is intended for untrusted/device-supplied text (e.g. escrowclient_error), a very large string could cause unnecessary memory pressure. You can truncate by scanning runes and slicing by byte index without allocating proportional to input size.
func TruncateRunes(s string, maxRunes int) string {
if maxRunes <= 0 {
return ""
}
if len(s) <= maxRunes {
// Fast path: a string of at most maxRunes bytes cannot exceed maxRunes characters.
return s
}
if utf8.RuneCountInString(s) <= maxRunes {
return s
}
return string([]rune(s)[:maxRunes])
}
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:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes