From c42f8230f7e4c382bfa2574e9e63a080c9fb1277 Mon Sep 17 00:00:00 2001 From: gillespi314 <73313222+gillespi314@users.noreply.github.com> Date: Thu, 10 Aug 2023 17:36:34 -0500 Subject: [PATCH] Check assigned DEP in Orbit MDM migration (#13232) --- .../13102-check-assigned-enrollment-profile | 3 + orbit/cmd/desktop/desktop.go | 1 - orbit/pkg/profiles/profiles_darwin.go | 120 ++++++++++++++++++ orbit/pkg/profiles/profiles_darwin_test.go | 91 ++++++++++++- orbit/pkg/profiles/profiles_notdarwin.go | 4 + orbit/pkg/profiles/profiles_notdarwin_test.go | 5 + orbit/pkg/update/notifications.go | 24 ++++ orbit/pkg/update/notifications_test.go | 34 +++++ 8 files changed, 279 insertions(+), 3 deletions(-) create mode 100644 orbit/changes/13102-check-assigned-enrollment-profile diff --git a/orbit/changes/13102-check-assigned-enrollment-profile b/orbit/changes/13102-check-assigned-enrollment-profile new file mode 100644 index 0000000000..0f76ddf362 --- /dev/null +++ b/orbit/changes/13102-check-assigned-enrollment-profile @@ -0,0 +1,3 @@ +- Updated MDM migration flow to include checking the output of `profiles show -type enrollment` + as a pre-condition for `profiles renew -type enrollment` to mitigate issues where caching or other + unexpected delays in Apple DEP profile assignment could cause the wrong profile to be renewed. diff --git a/orbit/cmd/desktop/desktop.go b/orbit/cmd/desktop/desktop.go index a6755e11da..4c61dfab76 100644 --- a/orbit/cmd/desktop/desktop.go +++ b/orbit/cmd/desktop/desktop.go @@ -326,7 +326,6 @@ func main() { migrateMDMItem.Hide() } } - }() go func() { diff --git a/orbit/pkg/profiles/profiles_darwin.go b/orbit/pkg/profiles/profiles_darwin.go index 8d591bdcec..1b90999066 100644 --- a/orbit/pkg/profiles/profiles_darwin.go +++ b/orbit/pkg/profiles/profiles_darwin.go @@ -5,9 +5,11 @@ package profiles import ( "bytes" "encoding/json" + "errors" "fmt" "net/url" "os/exec" + "strings" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mdm/apple/mobileconfig" @@ -104,3 +106,121 @@ var getMDMInfoFromProfilesCmd = func() ([]byte, error) { cmd := exec.Command("/usr/bin/profiles", "status", "-type", "enrollment") return cmd.Output() } + +// CheckAssignedEnrollmentProfile runs the `profiles show -type enrollment` command to get the assigned +// MDM enrollment profile and reports if the hostname of the MDM server +// in the assigned profile the device matches the hostname of the provided URL. +func CheckAssignedEnrollmentProfile(expectedURL string) error { + expected, err := url.Parse(expectedURL) + if err != nil { + return fmt.Errorf("parsing expected URL: %w", err) + } + + out, err := showEnrollmentProfileCmd() + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + return fmt.Errorf("show enrollment profile command: %w: %s", err, exitErr.Stderr) + } + return fmt.Errorf("show enrollment profile command: %w", err) + } + + // If an enrollment profile is assigned, the output of the command is in the form: + // + // ``` + // Device Enrollment configuration: + // { + // AllowPairing = 1; + // AutoAdvanceSetup = 0; + // AwaitDeviceConfigured = 0; + // ConfigurationURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + // ConfigurationWebURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + // ... + // } + // ``` + // + // If the host is not enrolled into an MDM, the output of the command is in the form: + // + // ``` + // Device Enrollment configuration: + // (null) + // ``` + // + // We will check that the output is at least 2 lines and contains the expected URL + + lines := bytes.Split(bytes.TrimSpace(out), []byte("\n")) + if len(lines) < 2 { + return fmt.Errorf("parsing profiles output: expected at least 2 lines but got %d", len(lines)) + } + if !bytes.Equal(lines[0], []byte("Device Enrollment configuration:")) { + return errors.New("parsing profiles output: does not match expected device enrollment configuration format") + } + if bytes.Equal(lines[1], []byte("(null)")) { + return errors.New("parsing profiles output: received null device enrollment configuration") + } + + var assignedURL string + for _, line := range lines { + // Note the output may contain both ConfigurationURL and ConfigurationWebURL but we check only + // the latter for backwards compatibility. + // See https://github.com/fleetdm/fleet/blob/963b2438537de14e7e16f1f18857ed8a66d51bfc/server/mdm/apple/apple_mdm.go#L195 + v, ok := parseEnrollmentProfileValue(line, "ConfigurationWebURL") + if ok { + assignedURL = v + break + } + } + + if assignedURL == "" { + return errors.New("parsing profiles output: missing or empty configuration web url") + } + + assigned, err := url.Parse(assignedURL) + if err != nil { + return fmt.Errorf("parsing profiles output: unable to parse configuration web url: %w", err) + } + + if assigned.Hostname() != expected.Hostname() { + return fmt.Errorf(`matching configuration web url: expected '%s' but found '%s'`, expected.Hostname(), assigned.Hostname()) + } + + return nil +} + +func parseEnrollmentProfileValue(line []byte, key string) (string, bool) { + // Output lines of `profiles show -type enrollment` take the form below: + // ``` + // Device Enrollment configuration: + // { + // AllowPairing = 1; + // AutoAdvanceSetup = 0; + // AwaitDeviceConfigured = 0; + // ConfigurationURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + // ConfigurationWebURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + // ... + // } + + // We are interested in the key-value pairs, which feature the separator " = ". + // Note that we want to include the spaces around the equals sign to avoid further splitting + // values, e.g., the url value may also contain an equals sign in the query string. + parts := bytes.SplitN(line, []byte(" = "), 3) + if len(parts) != 2 { + return "", false + } + + k := strings.TrimSpace(string(parts[0])) + if k == key { + // The value may be quoted and may contain a trailing semicolon. Remove both. + v := strings.TrimSpace(string(parts[1])) + v = strings.TrimSuffix(v, `;`) + v = strings.Trim(v, `"`) + return v, true + } + + return "", false +} + +// showEnrollmentProfileCmd is declared as a variable so it can be overwritten by tests. +var showEnrollmentProfileCmd = func() ([]byte, error) { + cmd := exec.Command("/usr/bin/profiles", "show", "-type", "enrollment") + return cmd.Output() +} diff --git a/orbit/pkg/profiles/profiles_darwin_test.go b/orbit/pkg/profiles/profiles_darwin_test.go index cfdaf315ce..9de61d927f 100644 --- a/orbit/pkg/profiles/profiles_darwin_test.go +++ b/orbit/pkg/profiles/profiles_darwin_test.go @@ -67,7 +67,6 @@ func TestGetFleetdConfig(t *testing.T) { } require.Equal(t, c.wantOut, out) } - } func TestIsEnrolledIntoMatchingURL(t *testing.T) { @@ -137,5 +136,93 @@ MDM server: https://valid.com/mdm/apple/mdm } require.Equal(t, c.wantOut, out) } - +} + +func TestCheckAssignedEnrollmentProfile(t *testing.T) { + fleetURL := "https://valid.com" + cases := []struct { + name string + cmdOut *string + cmdErr error + wantOut bool + wantErr error + }{ + { + "command error", + nil, + errors.New("some command error"), + false, + errors.New("some command error"), + }, + { + "empty output", + ptr.String(""), + nil, + false, + errors.New("parsing profiles output: expected at least 2 lines but got 1"), + }, + { + "null profile", + ptr.String(`Device Enrollment configuration: +(null) + `), + nil, + false, + errors.New("parsing profiles output: received null device enrollment configuration"), + }, + { + "mismatch profile", + ptr.String(`Device Enrollment configuration: +{ + AllowPairing = 1; + AutoAdvanceSetup = 0; + AwaitDeviceConfigured = 0; + ConfigurationURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + ConfigurationWebURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + ... +} + `), + nil, + false, + errors.New(`configuration web url: expected 'valid.com' but found 'test.example.com'`), + }, + { + "match profile", + ptr.String(`Device Enrollment configuration: +{ + AllowPairing = 1; + AutoAdvanceSetup = 0; + AwaitDeviceConfigured = 0; + ConfigurationURL = "https://test.example.com/mdm/apple/enroll?token=1234"; + ConfigurationWebURL = "https://valid.com?token=1234"; + ... +} + `), + nil, + false, + nil, + }, + } + + origCmd := showEnrollmentProfileCmd + t.Cleanup(func() { showEnrollmentProfileCmd = origCmd }) + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + showEnrollmentProfileCmd = func() ([]byte, error) { + if c.cmdOut == nil { + return nil, c.cmdErr + } + var buf bytes.Buffer + buf.WriteString(*c.cmdOut) + return []byte(*c.cmdOut), nil + } + + err := CheckAssignedEnrollmentProfile(fleetURL) + if c.wantErr != nil { + require.ErrorContains(t, err, c.wantErr.Error()) + } else { + require.NoError(t, err) + } + }) + } } diff --git a/orbit/pkg/profiles/profiles_notdarwin.go b/orbit/pkg/profiles/profiles_notdarwin.go index 39f5758ea0..241c2502c6 100644 --- a/orbit/pkg/profiles/profiles_notdarwin.go +++ b/orbit/pkg/profiles/profiles_notdarwin.go @@ -11,3 +11,7 @@ func GetFleetdConfig() (*fleet.MDMAppleFleetdConfig, error) { func IsEnrolledIntoMatchingURL(u string) (bool, error) { return false, ErrNotImplemented } + +func CheckAssignedEnrollmentProfile(expectedURL string) error { + return ErrNotImplemented +} diff --git a/orbit/pkg/profiles/profiles_notdarwin_test.go b/orbit/pkg/profiles/profiles_notdarwin_test.go index 55c4297751..45edea54bc 100644 --- a/orbit/pkg/profiles/profiles_notdarwin_test.go +++ b/orbit/pkg/profiles/profiles_notdarwin_test.go @@ -19,3 +19,8 @@ func TestIsEnrolledIntoMatchingURL(t *testing.T) { require.ErrorIs(t, ErrNotImplemented, err) require.False(t, enrolled) } + +func TestCheckAssignedEnrollmentProfile(t *testing.T) { + err := CheckAssignedEnrollmentProfile("https://test.example.com") + require.ErrorIs(t, ErrNotImplemented, err) +} diff --git a/orbit/pkg/update/notifications.go b/orbit/pkg/update/notifications.go index b9303e43cc..7510f50b8a 100644 --- a/orbit/pkg/update/notifications.go +++ b/orbit/pkg/update/notifications.go @@ -14,6 +14,8 @@ type runCmdFunc func() error type checkEnrollmentFunc func(url string) (bool, error) +type checkAssignedEnrollmentProfileFunc func(url string) error + // renewEnrollmentProfileConfigFetcher is a kind of middleware that wraps an // OrbitConfigFetcher and detects if the fleet server sent a notification to // renew the enrollment profile. If so, it runs the command (as root) to @@ -38,6 +40,9 @@ type renewEnrollmentProfileConfigFetcher struct { // enrollment checkEnrollmentFn checkEnrollmentFunc + // for tests, to be able to mock the function that checks for the assigned enrollment profile + checkAssignedEnrollmentProfileFn checkAssignedEnrollmentProfileFunc + // ensures only one command runs at a time, protects access to lastRun cmdMu sync.Mutex lastRun time.Time @@ -83,11 +88,30 @@ func (h *renewEnrollmentProfileConfigFetcher) GetConfig() (*fleet.OrbitConfig, e return cfg, nil } + // we perform this check locally on the client too to avoid showing the + // dialog if the Fleet enrollment profile has not been assigned to the device in + // Apple Business Manager. + assignedFn := h.checkAssignedEnrollmentProfileFn + if assignedFn == nil { + assignedFn = profiles.CheckAssignedEnrollmentProfile + } + if err := assignedFn(h.fleetURL); err != nil { + log.Error().Err(err).Msg("checking assigned enrollment profile") + log.Info().Msg("a request to renew the enrollment profile was processed but not executed because there was an error checking the assigned enrollment profile.") + // TODO: Design a better way to backoff `profiles show` so that the device doesn't get rate + // limited by Apple. For now, wait at least 2 minutes before retrying. + h.lastRun = time.Now().Add(-h.Frequency).Add(2 * time.Minute) + return cfg, nil + } + fn := h.runCmdFn if fn == nil { fn = runRenewEnrollmentProfile } if err := fn(); err != nil { + // TODO: Look into whether we should increment lastRun here or implement a + // backoff to avoid unnecessary user notification popups and mitigate rate + // limiting by Apple. log.Info().Err(err).Msg("calling /usr/bin/profiles to renew enrollment profile failed") } else { h.lastRun = time.Now() diff --git a/orbit/pkg/update/notifications_test.go b/orbit/pkg/update/notifications_test.go index d0f9cf1b35..0cfc130ae0 100644 --- a/orbit/pkg/update/notifications_test.go +++ b/orbit/pkg/update/notifications_test.go @@ -2,6 +2,7 @@ package update import ( "bytes" + "errors" "fmt" "io" "testing" @@ -41,6 +42,7 @@ func TestRenewEnrollmentProfile(t *testing.T) { } var cmdGotCalled bool + var depAssignedCheckGotCalled bool renewFetcher := &renewEnrollmentProfileConfigFetcher{ Fetcher: fetcher, Frequency: time.Hour, // doesn't matter for this test @@ -51,6 +53,10 @@ func TestRenewEnrollmentProfile(t *testing.T) { checkEnrollmentFn: func(url string) (bool, error) { return false, nil }, + checkAssignedEnrollmentProfileFn: func(url string) error { + depAssignedCheckGotCalled = true + return nil + }, } cfg, err := renewFetcher.GetConfig() @@ -58,6 +64,7 @@ func TestRenewEnrollmentProfile(t *testing.T) { require.Equal(t, fetcher.cfg, cfg) // the renew enrollment wrapper properly returns the expected config require.Equal(t, c.wantCmdCalled, cmdGotCalled) + require.Equal(t, c.wantCmdCalled, depAssignedCheckGotCalled) require.Contains(t, logBuf.String(), c.wantLog) }) } @@ -76,6 +83,7 @@ func TestRenewEnrollmentProfilePrevented(t *testing.T) { var cmdCallCount int isEnrolled := false + isAssigned := true chProceed := make(chan struct{}) renewFetcher := &renewEnrollmentProfileConfigFetcher{ Fetcher: fetcher, @@ -88,6 +96,13 @@ func TestRenewEnrollmentProfilePrevented(t *testing.T) { <-chProceed // will be unblocked only when allowed return isEnrolled, nil }, + checkAssignedEnrollmentProfileFn: func(url string) error { + <-chProceed // will be unblocked only when allowed + if !isAssigned { + return errors.New("not assigned") + } + return nil + }, } assertResult := func(cfg *fleet.OrbitConfig, err error) { @@ -137,6 +152,25 @@ func TestRenewEnrollmentProfilePrevented(t *testing.T) { assertResult(cfg, err) require.Equal(t, 2, cmdCallCount) // the initial call and the one after sleep + + // wait for the fetcher's frequency to pass + time.Sleep(renewFetcher.Frequency) + + // this call doesn't execute the command since the assigned profile check fails + isAssigned = false + isEnrolled = false + cfg, err = renewFetcher.GetConfig() + assertResult(cfg, err) + + require.Equal(t, 2, cmdCallCount) // the initial call and the one after sleep + + // wait for the fetcher's frequency to pass + time.Sleep(renewFetcher.Frequency) + + // this next call won't execute the command because the backoff + // for a failed assigned check is always 2 minutes + cfg, err = renewFetcher.GetConfig() + assertResult(cfg, err) } func TestWindowsMDMEnrollment(t *testing.T) {