From 6ce0f70ebcf2bbf00799cc9aa3fc781297540140 Mon Sep 17 00:00:00 2001 From: Magnus Jensen Date: Tue, 4 Aug 2026 09:40:20 +0200 Subject: [PATCH] Not Now edge case fixes for Apple profiles (#50044) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves #47411 (Speculative, but we will keep investigating if we get new reports) # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] 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. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit - **Bug Fixes** - Fixed Apple MDM profile handling for devices that respond with “Not Now” by ensuring the response is issued only on first delivery and doesn’t trigger repeated retries. - Improved reconciliation so superseded InstallProfile commands are properly canceled and cleanup is correct for user-scoped and pending installs. - When host verification fails after an acknowledged install, devices now receive the appropriate RemoveProfile operation. - **Tests** - Added regression integration coverage for “Not Now” cancellation, scope changes, profile edits, undelivered installs, and failed verification cleanup. --- changes/47411-not-now-edge-cases | 1 + cmd/osquery-perf/README.md | 14 + cmd/osquery-perf/agent.go | 69 +++++ pkg/mdm/mdmtest/apple.go | 14 + server/fleet/apple_mdm.go | 6 + server/mdm/apple/reconcile.go | 54 +++- server/mdm/nanomdm/storage/mysql/queue.go | 8 +- .../service/integration_mdm_profiles_test.go | 241 ++++++++++++++++++ 8 files changed, 399 insertions(+), 8 deletions(-) create mode 100644 changes/47411-not-now-edge-cases diff --git a/changes/47411-not-now-edge-cases b/changes/47411-not-now-edge-cases new file mode 100644 index 0000000000..2ef71b99ce --- /dev/null +++ b/changes/47411-not-now-edge-cases @@ -0,0 +1 @@ +- Fixed a few edge cases for Apple profile reconciliation when devices respond with NotNow in certain scenarios. \ No newline at end of file diff --git a/cmd/osquery-perf/README.md b/cmd/osquery-perf/README.md index d71f7b380e..82a4255f04 100644 --- a/cmd/osquery-perf/README.md +++ b/cmd/osquery-perf/README.md @@ -150,6 +150,20 @@ go run agent.go --host_count 100 --mdm_prob 1.0 --mdm_scep_challenge --mdm_psso_interval 4h --mdm_psso_login_prob 1.0 --mdm_psso_key_prob 0.1 ``` +### Synthetically reproducing MDM device protocol failures + +#### NotNow'ing profiles + +> Currently only supported for macOS and `InstallProfile` commands + +To force an osquery-perf agent to respond with `NotNow` once to an `InstallProfile` command, the payload has to contain `NotNow` anywhere in the profile. It will NotNow once, then acknowledge it on next check-in. To force a new `NotNow` response, you have to change the `ProfileIdentifier`. + +#### Forcing a certain error code and failure for InstallApplication + +> Currently only supported for macOS. + +To force a certain ErrorCode and failure for an `InstallApplication` command, the `iTunesStoreID` payload field has to have a value below 100_000. The agent will respond with a failure and the specified error code, which helps QA and repro logic scenarios on certain error codes. + ## Installing software The agent can install software for "macos", "ubuntu", and "windows" OSs when running with orbit agent. The following options control the installation behavior: diff --git a/cmd/osquery-perf/agent.go b/cmd/osquery-perf/agent.go index 16f0bc57c0..84a5a9954d 100644 --- a/cmd/osquery-perf/agent.go +++ b/cmd/osquery-perf/agent.go @@ -45,8 +45,10 @@ import ( "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/service" "github.com/google/uuid" + micromdm "github.com/micromdm/micromdm/mdm/mdm" "github.com/micromdm/plist" "github.com/remitly-oss/httpsig-go" + "github.com/smallstep/pkcs7" ) var ( @@ -504,6 +506,10 @@ type agent struct { // ddmUserDeclTokens caches per-declaration tokens (identifier → serverToken). ddmUserDeclTokens map[string]string + // notNowProfiles tracks the profile identifiers (per channel) this agent has + // already responded NotNow to, so the redelivered command is acknowledged. + notNowProfiles map[string]bool + disableScriptExec bool disableFleetDesktop bool loggerTLSMaxLines int @@ -1280,6 +1286,46 @@ func (a *agent) runOrbitLoop() { } } +// profileNotNowRequested reports whether the delivered InstallProfile command +// carries a profile whose decoded content contains the marker string "NotNow" +// and this agent has not yet responded NotNow to that profile identifier on +// the given channel. When it returns true it records the identifier, so the +// redelivered command is acknowledged. Only called from the MDM loop +// goroutine, so notNowProfiles needs no locking. +func (a *agent) profileNotNowRequested(cmd *mdm.Command, channel string) bool { + var full micromdm.CommandPayload + if err := plist.Unmarshal(cmd.Raw, &full); err != nil || full.Command.InstallProfile == nil { + return false + } + profile := full.Command.InstallProfile.Payload + // The mobileconfig may be PKCS7-signed; unwrap to the raw XML plist. + if !bytes.HasPrefix(profile, []byte(" 0 { + if err := commander.BulkDeleteHostUserCommandsWithoutResults(ctx, supersededCmdToEnrollmentIDs); err != nil { + return nil, ctxerr.Wrap(ctx, err, "deleting superseded install commands") + } + } if err := ds.BulkDeleteMDMAppleHostsConfigProfiles(ctx, hostProfilesToCleanup); err != nil { return nil, ctxerr.Wrap(ctx, err, "deleting profiles that didn't change") } diff --git a/server/mdm/nanomdm/storage/mysql/queue.go b/server/mdm/nanomdm/storage/mysql/queue.go index d07bd7a9ae..66197a9efe 100644 --- a/server/mdm/nanomdm/storage/mysql/queue.go +++ b/server/mdm/nanomdm/storage/mysql/queue.go @@ -61,7 +61,8 @@ func enqueue(ctx context.Context, tx sqlx.ExtContext, ids []string, cmd *mdm.Com } func (m *MySQLStorage) EnqueueCommand(ctx context.Context, ids []string, cmd *mdm.CommandWithSubtype) (map[string]error, - error) { + error, +) { // We need to retry because this transaction may deadlock with updates to nano_enrollment.last_seen_at // Deadlock seen in 2024/12/12 loadtest: https://docs.google.com/document/d/1-Q6qFTd7CDm-lh7MVRgpNlNNJijk6JZ4KO49R1fp80U err := common_mysql.WithRetryTxx(ctx, sqlx.NewDb(m.db, ""), func(tx sqlx.ExtContext) error { @@ -272,7 +273,8 @@ func (m *MySQLStorage) BulkDeleteHostUserCommandsWithoutResults(ctx context.Cont } func (m *MySQLStorage) bulkDeleteHostUserCommandsWithoutResults(ctx context.Context, tx sqlx.ExtContext, - commandToIDs map[string][]string) error { + commandToIDs map[string][]string, +) error { stmt := ` DELETE eq @@ -281,7 +283,7 @@ FROM LEFT JOIN nano_command_results AS cr ON cr.command_uuid = eq.command_uuid AND cr.id = eq.id WHERE - cr.command_uuid IS NULL AND eq.command_uuid = ? AND eq.id IN (?);` + (cr.command_uuid IS NULL OR cr.status = 'NotNow') AND eq.command_uuid = ? AND eq.id IN (?);` // We process each commandUUID one at a time, in batches of hostUserIDs. // This is because the number of hostUserIDs can be large, and number of unique commands is normally small. diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index af01c7bf30..dea71c59ac 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -10232,3 +10232,244 @@ func (s *integrationMDMTestSuite) TestWindowsSCEPProfilePreferredVariableAccepte }}, http.StatusNoContent) } + +// mdmActiveCmdCount returns the number of active (undelivered) nano_enrollment_queue +// rows for the given command UUID. +func mdmActiveCmdCount(t *testing.T, ds *mysql.Datastore, cmdUUID string) int { + var n int + mysqltest.ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(context.Background(), q, &n, + `SELECT COUNT(*) FROM nano_enrollment_queue WHERE command_uuid = ? AND active = 1`, cmdUUID) + }) + return n +} + +// hostHasAppleProfileOp reports whether the host has an hmap row for the given identifier +// and operation type, returning its command UUID. +func hostHasAppleProfileOp(t *testing.T, ds *mysql.Datastore, hostUUID, ident string, op fleet.MDMOperationType) (bool, string) { + var cmdUUIDs []string + mysqltest.ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.SelectContext(context.Background(), q, &cmdUUIDs, + `SELECT command_uuid FROM host_mdm_apple_profiles + WHERE host_uuid = ? AND profile_identifier = ? AND operation_type = ?`, + hostUUID, ident, op) + }) + if len(cmdUUIDs) == 0 { + return false, "" + } + require.Len(t, cmdUUIDs, 1) + return true, cmdUUIDs[0] +} + +// enrollHostDrainInitialProfiles enrolls a macOS host, delivers+acks its initial +// (fleetd/CA) profiles, and clears the reconcile-dedup key so later changes reprocess. +func (s *integrationMDMTestSuite) enrollHostDrainInitialProfiles(t *testing.T) (*fleet.Host, *mdmtest.TestAppleMDMClient) { + ctx := t.Context() + host, mdmDevice := createHostThenEnrollMDM(s.ds, s.server.URL, t) + s.awaitRunAppleMDMWorkerSchedule() + checkNextPayloads(t, mdmDevice, false) + require.NoError(t, s.keyValueStore.Delete(ctx, fleet.MDMProfileProcessingKeyPrefix+":"+host.UUID)) + return host, mdmDevice +} + +// ackUntilThenNotNow acknowledges queued commands until it reaches cmdUUID, which it +// answers with NotNow. Fails the test if the queue drains without delivering cmdUUID. +func ackUntilThenNotNow(t *testing.T, device *mdmtest.TestAppleMDMClient, cmdUUID string) { + cmd, err := device.Idle() + require.NoError(t, err) + for cmd != nil { + if cmd.CommandUUID == cmdUUID { + _, err = device.NotNow(cmdUUID) + require.NoError(t, err) + return + } + cmd, err = device.Acknowledge(cmd.CommandUUID) + require.NoError(t, err) + } + t.Fatalf("command %s was never delivered", cmdUUID) +} + +// labelGateHost creates a label, makes the host a member, and marks the host's labels +// as reported so include-any gating evaluates it as a match. +func (s *integrationMDMTestSuite) labelGateHost(t *testing.T, host *fleet.Host, name string) *fleet.Label { + ctx := t.Context() + label, err := s.ds.NewLabel(ctx, &fleet.Label{Name: name, Query: "select 1;"}) + require.NoError(t, err) + host.LabelUpdatedAt = time.Now() + require.NoError(t, s.ds.UpdateHost(ctx, host)) + require.NoError(t, s.ds.AsyncBatchInsertLabelMembership(ctx, [][2]uint{{label.ID, host.ID}})) + return label +} + +// TestProfileReconcilerCancelsInstallationWhenNotNowResponseRecorded is a regression test that +// ensures a pending device-scoped InstallProfile command is cancelled when the host leaves scope after a NotNow response. +func (s *integrationMDMTestSuite) TestProfileReconcilerCancelsInstallationWhenNotNowResponseRecorded() { + t := s.T() + ctx := t.Context() + require.NoError(t, s.ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{{Secret: t.Name()}})) + host, mdmDevice := s.enrollHostDrainInitialProfiles(t) + label := s.labelGateHost(t, host, t.Name()+"-lbl") + + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P1", Contents: mobileconfigForTest("P1", "P1"), LabelsIncludeAny: []string{label.Name}}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + // take P1's install command from its hmap row (Idle may deliver other + // re-enqueued profiles first), ack up to it and answer it with NotNow + ok, i := hostHasAppleProfileOp(t, s.ds, host.UUID, "P1", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, i) + ackUntilThenNotNow(t, mdmDevice, i) + + // remove the host from the label (scope change, not a deletion), then reconcile + require.NoError(t, s.ds.AsyncBatchDeleteLabelMembership(ctx, [][2]uint{{label.ID, host.ID}})) + s.awaitTriggerProfileSchedule(t) + + // desired: the install command is cancelled; bug: the NotNow result defeats the DELETE + require.Zero(t, mdmActiveCmdCount(t, s.ds, i), "install command still active after profile left scope") +} + +// TestProfileUserScopedPendingInstallCancelled is a regression test where when label descoping a user-scoped profile, that is pending installation +// ensures the install command is cancelled. To avoid NotNow'ing producing an incorrect order and could leave profiles that Fleet is no longer tracking on the device. +func (s *integrationMDMTestSuite) TestProfileUserScopedPendingInstallCancelled() { + t := s.T() + ctx := t.Context() + require.NoError(t, s.ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{{Secret: t.Name()}})) + host, mdmDevice := s.enrollHostDrainInitialProfiles(t) + require.NoError(t, mdmDevice.UserEnroll()) + userEnr, err := s.ds.GetNanoMDMUserEnrollment(ctx, host.UUID) + require.NoError(t, err) + require.NotNil(t, userEnr) + label := s.labelGateHost(t, host, t.Name()+"-lbl") + + scope := fleet.PayloadScopeUser + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P2", Contents: scopedMobileconfigForTest("P2", "P2.user", &scope), LabelsIncludeAny: []string{label.Name}}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + // take P2's install command from its hmap row (no device interaction needed; + // this is a pure keying bug) + ok, i := hostHasAppleProfileOp(t, s.ds, host.UUID, "P2.user", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, i) + + // sanity: the queue row is keyed by the user enrollment ID, not the host UUID + var qid string + mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &qid, `SELECT id FROM nano_enrollment_queue WHERE command_uuid = ?`, i) + }) + require.Equal(t, userEnr.ID, qid) + + require.NoError(t, s.ds.AsyncBatchDeleteLabelMembership(ctx, [][2]uint{{label.ID, host.ID}})) + s.awaitTriggerProfileSchedule(t) + + // desired: the user-channel install is cancelled; bug: cleanup keyed by host UUID misses it + require.Zero(t, mdmActiveCmdCount(t, s.ds, i), "user-scoped install still active after profile left scope") +} + +// TestProfileFailedVerificationGetsRemove is a regression test for the case where osquery profile verification fails on a previous ACK'ed InstallProfile command. +// This test ensures a RemoveProfile command gets sent to ensure no lingering profiles is left. +func (s *integrationMDMTestSuite) TestProfileFailedVerificationGetsRemove() { + t := s.T() + ctx := t.Context() + require.NoError(t, s.ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{{Secret: t.Name()}})) + host, mdmDevice := s.enrollHostDrainInitialProfiles(t) + label := s.labelGateHost(t, host, t.Name()+"-lbl") + + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P3", Contents: mobileconfigForTest("P3", "P3"), LabelsIncludeAny: []string{label.Name}}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + // ack all queued installs; P3 is now genuinely on the device + checkNextPayloads(t, mdmDevice, false) + + // exactly what setMDMProfilesFailedDB produces when the verifier can't see the profile + mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + `UPDATE host_mdm_apple_profiles SET status = 'failed', detail = 'Failed, was verifying' + WHERE host_uuid = ? AND profile_identifier = 'P3' AND operation_type = 'install'`, host.UUID) + return err + }) + + require.NoError(t, s.ds.AsyncBatchDeleteLabelMembership(ctx, [][2]uint{{label.ID, host.ID}})) + s.awaitTriggerProfileSchedule(t) + + // desired: a RemoveProfile is enqueued to pull the profile off the device + ok, _ := hostHasAppleProfileOp(t, s.ds, host.UUID, "P3", fleet.MDMOperationTypeRemove) + require.True(t, ok, "failed install that left scope got no RemoveProfile; profile stranded on device") +} + +// TestProfileEditLeaksOldInstallCommand is a regression test that ensures editing a profile via gitops or edit, cancels the previous in-flight command +// to avoid NotNow responses producing out of order commands. +func (s *integrationMDMTestSuite) TestProfileEditLeaksOldInstallCommand() { + t := s.T() + ctx := t.Context() + require.NoError(t, s.ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{{Secret: t.Name()}})) + host, mdmDevice := s.enrollHostDrainInitialProfiles(t) + + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P5", Contents: mobileconfigForTest("P5", "P5")}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + // take P5's install command from its hmap row (Idle may deliver other + // re-enqueued profiles first), ack up to it and answer it with NotNow + ok, iOld := hostHasAppleProfileOp(t, s.ds, host.UUID, "P5", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, iOld) + ackUntilThenNotNow(t, mdmDevice, iOld) + + // edit P5's content (new random PayloadUUID -> new checksum), same name/identifier + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P5", Contents: mobileconfigForTest("P5", "P5")}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + ok, iNew := hostHasAppleProfileOp(t, s.ds, host.UUID, "P5", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, iNew) + + // desired: the old install command is cancelled; bug: it stays active and can apply v1 over v2 + require.Zero(t, mdmActiveCmdCount(t, s.ds, iOld), "old install command still active after profile edit") + require.Equal(t, 1, mdmActiveCmdCount(t, s.ds, iNew), "new install command not active after profile edit") +} + +// TestProfileEditCancelsUndeliveredInstallCommand is the companion to +// TestProfileEditLeaksOldInstallCommand: the device never picks up the first install +// (offline host), so the superseded command has no result row at all. This isolates the +// toInstall cancellation wiring (root cause E) from the NotNow-tolerant DELETE (fix 1) — +// if only this test goes red, the wiring broke; if only the NotNow variant goes red, the +// DELETE's NotNow guard broke. +func (s *integrationMDMTestSuite) TestProfileEditCancelsUndeliveredInstallCommand() { + t := s.T() + ctx := t.Context() + require.NoError(t, s.ds.ApplyEnrollSecrets(ctx, nil, []*fleet.EnrollSecret{{Secret: t.Name()}})) + host, _ := s.enrollHostDrainInitialProfiles(t) + + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P6", Contents: mobileconfigForTest("P6", "P6")}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + // the install is queued but the device never checks in + ok, iOld := hostHasAppleProfileOp(t, s.ds, host.UUID, "P6", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, iOld) + require.Equal(t, 1, mdmActiveCmdCount(t, s.ds, iOld)) + + // edit P6's content (new random PayloadUUID -> new checksum), same name/identifier + s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "P6", Contents: mobileconfigForTest("P6", "P6")}, + }}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + ok, iNew := hostHasAppleProfileOp(t, s.ds, host.UUID, "P6", fleet.MDMOperationTypeInstall) + require.True(t, ok) + require.NotEmpty(t, iNew) + require.NotEqual(t, iOld, iNew) + + // the undelivered v1 install must be cancelled so the host doesn't run v1 then v2 + require.Zero(t, mdmActiveCmdCount(t, s.ds, iOld), "undelivered install command still active after profile edit") + require.Equal(t, 1, mdmActiveCmdCount(t, s.ds, iNew), "new install command not active after profile edit") +}