From 6b31b71c93d3ee71df845b2b1a898845ef0894fb Mon Sep 17 00:00:00 2001 From: Jahziel Villasana-Espinoza Date: Tue, 14 May 2024 15:19:14 -0400 Subject: [PATCH] 18531 failed mdm profs (#18930) > Related issue: #18531 --- changes/18531-failed-mdm-profs | 1 + server/datastore/mysql/apple_mdm.go | 9 ++- server/fleet/apple_mdm.go | 6 ++ server/service/apple_mdm.go | 7 ++ server/service/integration_mdm_test.go | 107 ++++++++++++++++++++++++- 5 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 changes/18531-failed-mdm-profs diff --git a/changes/18531-failed-mdm-profs b/changes/18531-failed-mdm-profs new file mode 100644 index 0000000000..049cbc3ac8 --- /dev/null +++ b/changes/18531-failed-mdm-profs @@ -0,0 +1 @@ +- Fixes a bug where an MDM profile that wasn't present on a host wasn't removed from it in Fleet. \ No newline at end of file diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index db2e0df8db..be5ecd2148 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -1129,7 +1129,6 @@ func upsertMDMAppleHostLabelMembershipDB(ctx context.Context, tx sqlx.ExtContext // deleteMDMOSCustomSettingsForHost deletes configuration profiles and // declarations for a host based on its platform. func (ds *Datastore) deleteMDMOSCustomSettingsForHost(ctx context.Context, tx sqlx.ExtContext, uuid, platform string) error { - tableMap := map[string][]string{ "darwin": {"host_mdm_apple_profiles", "host_mdm_apple_declarations"}, "windows": {"host_mdm_windows_profiles"}, @@ -1775,6 +1774,13 @@ func (ds *Datastore) bulkSetPendingMDMAppleHostProfilesDB( if _, ok := profileIntersection.GetMatchingProfileInDesiredState(p); ok { continue } + // If the installation failed, then we do not want to change the operation to "Remove". + // Doing so will result in Fleet attempting to remove a profile that doesn't exist on the + // host (since the installation failed). Skipping it here will lead to it being removed from + // the host in Fleet during profile reconciliation, which is what we want. + if p.FailedToInstallOnHost() { + continue + } pargs = append(pargs, p.ProfileUUID, p.HostUUID, p.ProfileIdentifier, p.ProfileName, p.Checksum, fleet.MDMOperationTypeRemove, nil, "", "") psb.WriteString("(?, ?, ?, ?, ?, ?, ?, ?, ?),") @@ -1997,6 +2003,7 @@ func (ds *Datastore) ListMDMAppleProfilesToRemove(ctx context.Context) ([]*fleet FROM %s`, generateEntitiesToRemoveQuery("profile")) var profiles []*fleet.MDMAppleProfilePayload err := sqlx.SelectContext(ctx, ds.reader(ctx), &profiles, query, fleet.MDMOperationTypeRemove) + return profiles, err } diff --git a/server/fleet/apple_mdm.go b/server/fleet/apple_mdm.go index 04f5742e61..ff4a041494 100644 --- a/server/fleet/apple_mdm.go +++ b/server/fleet/apple_mdm.go @@ -299,6 +299,12 @@ type MDMAppleProfilePayload struct { CommandUUID string `db:"command_uuid"` } +// FailedToInstallOnHost indicates whether this profile failed to be installed on the host (and +// therefore is not, as far as Fleet knows, currently on the host). +func (p *MDMAppleProfilePayload) FailedToInstallOnHost() bool { + return p.Status != nil && *p.Status == MDMDeliveryFailed && p.OperationType == MDMOperationTypeInstall +} + type MDMAppleBulkUpsertHostProfilePayload struct { ProfileUUID string ProfileIdentifier string diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 637c703044..94e77cf324 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -2946,6 +2946,13 @@ func ReconcileAppleProfiles( continue } + if p.FailedToInstallOnHost() { + // then we shouldn't send an additional remove command since it failed to install on the + // host. + hostProfilesToCleanup = append(hostProfilesToCleanup, p) + continue + } + target := removeTargets[p.ProfileUUID] if target == nil { target = &cmdTarget{ diff --git a/server/service/integration_mdm_test.go b/server/service/integration_mdm_test.go index 7dc1ee4fce..2eea049961 100644 --- a/server/service/integration_mdm_test.go +++ b/server/service/integration_mdm_test.go @@ -8359,7 +8359,7 @@ func (s *integrationMDMTestSuite) TestDontIgnoreAnyProfileErrors() { require.NoError(t, err) } - // get that host - it should report "failed" for the profiles and include the error message detail + // get that host - it should report "failed" for the profiles and include the error message detail. expectedErrs := map[string]string{ "N1": "Failed to remove: MDMClientError (89): Profile with identifier 'I1' not found.\n", "N2": "Failed to remove: MDMClientError (96): Cannot replace profile 'I2' because it was not installed by the MDM server.\n", @@ -8459,3 +8459,108 @@ func (s *integrationMDMTestSuite) TestIsServerBitlockerStatus() { require.NotNil(t, hr.Host.MDM.OSSettings.DiskEncryption.Status) require.Equal(t, fleet.DiskEncryptionEnforcing, *hr.Host.MDM.OSSettings.DiskEncryption.Status) } + +func (s *integrationMDMTestSuite) TestRemoveFailedProfiles() { + t := s.T() + + teamName := t.Name() + team := &fleet.Team{ + Name: teamName, + Description: "desc " + teamName, + } + + var createTeamResp teamResponse + s.DoJSON("POST", "/api/latest/fleet/teams", team, http.StatusOK, &createTeamResp) + require.NotZero(t, createTeamResp.Team.ID) + team.ID = createTeamResp.Team.ID + + host, mdmDevice := createHostThenEnrollMDM(s.ds, s.server.URL, t) + + ident := uuid.NewString() + + globalProfiles := [][]byte{ + mobileconfigForTest("N1", ident), + mobileconfigForTest("N2", "I2"), + } + s.Do("POST", "/api/v1/fleet/mdm/apple/profiles/batch", batchSetMDMAppleProfilesRequest{Profiles: globalProfiles}, http.StatusNoContent) + s.awaitTriggerProfileSchedule(t) + + cmd, err := mdmDevice.Idle() + require.NoError(t, err) + for cmd != nil { + if cmd.Command.RequestType == "InstallProfile" { + var fullCmd micromdm.CommandPayload + require.NoError(t, plist.Unmarshal(cmd.Raw, &fullCmd)) + + if strings.Contains(string(fullCmd.Command.InstallProfile.Payload), ident) { + var errChain []mdm.ErrorChain + errChain = append(errChain, mdm.ErrorChain{ErrorCode: -102, ErrorDomain: "CPProfile", USEnglishDescription: "The profile is either missing some required information, or contains information in an invalid format."}) + cmd, err = mdmDevice.Err(cmd.CommandUUID, errChain) + require.NoError(t, err) + continue + } + } + cmd, err = mdmDevice.Acknowledge(cmd.CommandUUID) + require.NoError(t, err) + } + + require.NoError(t, apple_mdm.VerifyHostMDMProfiles(context.Background(), s.ds, host, map[string]*fleet.HostMacOSProfile{ + "I2": {Identifier: "I2", DisplayName: "I2", InstallDate: time.Now()}, + "I1": {Identifier: "I1", DisplayName: "I1", InstallDate: time.Now()}, + })) + + // Do another trigger + command fetching cycle, since we retry when a profile fails on install. + s.awaitTriggerProfileSchedule(t) + cmd, err = mdmDevice.Idle() + require.NoError(t, err) + for cmd != nil { + if cmd.Command.RequestType == "InstallProfile" { + var fullCmd micromdm.CommandPayload + require.NoError(t, plist.Unmarshal(cmd.Raw, &fullCmd)) + + if strings.Contains(string(fullCmd.Command.InstallProfile.Payload), ident) { + var errChain []mdm.ErrorChain + errChain = append(errChain, mdm.ErrorChain{ErrorCode: -102, ErrorDomain: "CPProfile", USEnglishDescription: "The profile is either missing some required information, or contains information in an invalid format."}) + cmd, err = mdmDevice.Err(cmd.CommandUUID, errChain) + require.NoError(t, err) + continue + } + } + cmd, err = mdmDevice.Acknowledge(cmd.CommandUUID) + require.NoError(t, err) + } + + require.NoError(t, apple_mdm.VerifyHostMDMProfiles(context.Background(), s.ds, host, map[string]*fleet.HostMacOSProfile{ + "I1": {Identifier: "I1", DisplayName: "I1", InstallDate: time.Now()}, + mobileconfig.FleetdConfigPayloadIdentifier: {Identifier: mobileconfig.FleetdConfigPayloadIdentifier, DisplayName: "dn1", InstallDate: time.Now()}, + mobileconfig.FleetCARootConfigPayloadIdentifier: {Identifier: mobileconfig.FleetCARootConfigPayloadIdentifier, DisplayName: "dn2", InstallDate: time.Now()}, + })) + + // Check that the profile is marked as failed when fetching the host + getHostResp := getHostResponse{} + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d", host.ID), nil, http.StatusOK, &getHostResp) + require.NotNil(t, getHostResp.Host.MDM.Profiles) + require.Len(t, *getHostResp.Host.MDM.Profiles, 4) + for _, hm := range *getHostResp.Host.MDM.Profiles { + if hm.Name == "N1" { + require.Equal(t, fleet.MDMDeliveryFailed, *hm.Status) + continue + } + + require.Equal(t, fleet.MDMDeliveryVerified, *hm.Status) + } + + // transfer host to a team without the failed profile + + s.DoJSON("POST", "/api/latest/fleet/hosts/transfer", addHostsToTeamRequest{TeamID: &team.ID, HostIDs: []uint{host.ID}}, http.StatusOK, &addHostsToTeamResponse{}) + s.awaitTriggerProfileSchedule(t) + + // confirm that we remove the failed profile + getHostResp = getHostResponse{} + s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d", host.ID), nil, http.StatusOK, &getHostResp) + require.NotNil(t, getHostResp.Host.MDM.Profiles) + require.Len(t, *getHostResp.Host.MDM.Profiles, 3) // This would be 4 if we hadn't deleted the profile that failed to install. + for _, hm := range *getHostResp.Host.MDM.Profiles { + require.NotEqual(t, "N1", hm.Name) + } +}