Don't require end user auth on orbit re-enrollment (#46300) - #47740
Conversation
Windows and Linux hosts that had already orbit-enrolled were prompted for end user authentication (an SSO browser tab) when fleetd re-enrolled after a service restart, node key file loss, or osquery DB rebuild. Hosts enrolled before EUA was enabled have no host_mdm_idp_accounts row, so the service-layer EUA gate treated every re-enroll like a brand-new device. Before returning END_USER_AUTH_REQUIRED, EnrollOrbit now checks whether a host matching the enrollment identifiers already exists and previously held an orbit node key (HostPreviouslyOrbitEnrolled, reusing matchHostDuringEnrollment's semantics). If so, the re-enroll proceeds without prompting. Genuinely new devices, and hosts moved to a different Fleet server, are still gated.
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
Code Review by Qodo
1. EUA bypass via spoofing
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe PR fixes Windows/Linux hosts being prompted for end-user authentication (EUA) when fleetd restarts and re-enrolls with the same credentials. A new 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)server/service/integration_enterprise_test.goThanks 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.
🧹 Nitpick comments (1)
server/service/integration_enterprise_test.go (1)
31733-31748: ⚡ Quick winMake EUA reset unconditional to avoid cross-subtest state leakage.
If any
require.*fails before Line 31745, the manual reset block never runs, so the next subtest can inheritEnableEndUserAuthentication=trueand fail for the wrong reason. Register cleanup immediately after enabling EUA so reset always happens.Suggested diff
team.Config.MDM.MacOSSetup.EnableEndUserAuthentication = true _, err = s.ds.SaveTeam(ctx, team) require.NoError(t, err) + t.Cleanup(func() { + team.Config.MDM.MacOSSetup.EnableEndUserAuthentication = false + _, cleanupErr := s.ds.SaveTeam(context.Background(), team) + require.NoError(t, cleanupErr) + }) @@ - // Reset team EUA for the next subtest's first enrollment. - team.Config.MDM.MacOSSetup.EnableEndUserAuthentication = false - _, err = s.ds.SaveTeam(ctx, team) - require.NoError(t, err) }🤖 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/service/integration_enterprise_test.go` around lines 31733 - 31748, The manual reset of EnableEndUserAuthentication to false at the end of the test block does not run if any require statement fails before it, causing the next subtest to inherit the enabled state and fail for the wrong reason. Register a cleanup function using defer immediately after setting team.Config.MDM.MacOSSetup.EnableEndUserAuthentication = true to ensure that the reset of EnableEndUserAuthentication to false and the subsequent SaveTeam call always execute, regardless of assertion failures in the test logic that follows.
🤖 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 `@server/service/integration_enterprise_test.go`:
- Around line 31733-31748: The manual reset of EnableEndUserAuthentication to
false at the end of the test block does not run if any require statement fails
before it, causing the next subtest to inherit the enabled state and fail for
the wrong reason. Register a cleanup function using defer immediately after
setting team.Config.MDM.MacOSSetup.EnableEndUserAuthentication = true to ensure
that the reset of EnableEndUserAuthentication to false and the subsequent
SaveTeam call always execute, regardless of assertion failures in the test logic
that follows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 837feeea-f2e4-488d-966b-5ed394070988
📒 Files selected for processing (7)
changes/46300-orbit-reenroll-euaserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_enterprise_test.goserver/service/orbit.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR adjusts Orbit enrollment behavior so that Linux/Windows hosts that have previously Orbit-enrolled are not re-prompted for End User Authentication (EUA) during re-enrollment events (service restart, node key loss, osquery DB rebuild), while still gating genuinely new enrollments.
Changes:
- Add a “previously Orbit-enrolled” check (based on existing host match + prior orbit node key) before returning
END_USER_AUTH_REQUIRED. - Introduce
HostPreviouslyOrbitEnrolledon the datastore interface with a MySQL implementation that mirrorsEnrollOrbithost-matching semantics. - Add integration and datastore-level tests covering the re-enrollment EUA bypass behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/orbit.go | Skip EUA prompt for re-enrolling hosts that previously had an orbit node key. |
| server/service/integration_enterprise_test.go | Adds an integration test covering the regression scenario from #46300. |
| server/mock/datastore_mock.go | Extends datastore mock with HostPreviouslyOrbitEnrolled. |
| server/fleet/datastore.go | Adds HostPreviouslyOrbitEnrolled to the datastore interface contract. |
| server/datastore/mysql/hosts.go | Implements HostPreviouslyOrbitEnrolled using matchHostDuringEnrollment semantics. |
| server/datastore/mysql/hosts_test.go | Adds unit tests for HostPreviouslyOrbitEnrolled. |
| changes/46300-orbit-reenroll-eua | Release note entry (content excluded by policy). |
Files excluded by content exclusion policy (1)
- changes/46300-orbit-reenroll-eua
💡 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 #47740 +/- ##
==========================================
+ Coverage 67.15% 67.20% +0.05%
==========================================
Files 3616 3619 +3
Lines 229030 229475 +445
Branches 11787 11787
==========================================
+ Hits 153805 154229 +424
- Misses 61370 61381 +11
- Partials 13855 13865 +10
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:
|
![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.
| serialToMatch = "" | ||
| } | ||
|
|
||
| matched, err := matchHostDuringEnrollment(ctx, ds.reader(ctx), orbitEnroll, isMDMEnabled, hostInfo.OsqueryIdentifier, |
There was a problem hiding this comment.
Is it safe to use the reader here? Doesn't EnrollOrbit write the orbit_node_key to the primary so if orbit crashes and turns back on is there a possibility the EUA will come back incorrectly?
There was a problem hiding this comment.
yes, it is safe. The timing makes this almost impossible in practice. Even if it did happen, we fail-safe to a EUA prompt. We never wrongly skip auth.
| // Otherwise report the unauthenticated host and let Orbit handle it (e.g. by prompting the user to authenticate). | ||
| return "", fleet.NewOrbitIDPAuthRequiredError() | ||
| } | ||
| svc.logger.InfoContext(ctx, "allowing re-enrollment without end-user authentication: host previously orbit-enrolled", |
There was a problem hiding this comment.
One other thing, should this be info or debug? Does this log every single time orbit restarts?
There was a problem hiding this comment.
This only fires on re-enroll, and only when re-enrolling the same device (with EUA enabled). Under normal operation, orbit should never re-enroll. I put it at info because it would help catch unexpected re-enrollments in prod (which doesn't run with debug).
|
@ksykulev ready for re-review |
Windows and Linux hosts that had already orbit-enrolled were prompted for end user authentication (an SSO browser tab) when fleetd re-enrolled after a service restart, node key file loss, or osquery DB rebuild. Hosts enrolled before EUA was enabled have no host_mdm_idp_accounts row, so the service-layer EUA gate treated every re-enroll like a brand-new device.
Before returning END_USER_AUTH_REQUIRED, EnrollOrbit now checks whether a host matching the enrollment identifiers already exists and previously held an orbit node key (HostPreviouslyOrbitEnrolled, reusing matchHostDuringEnrollment's semantics). If so, the re-enroll proceeds without prompting. Genuinely new devices, and hosts moved to a different Fleet server, are still gated.
Related issue: Resolves #46300
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes