From 2f54879f2a6ee62ec29ec7428990c8aae2a87c0b Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 5 Nov 2024 11:58:31 -0500 Subject: [PATCH] Bugfix: config profile deployment based on label exclusion (#23533) --- .../22162-exclude-labels-fix-default-behavior | 1 + server/datastore/mysql/apple_mdm.go | 22 ++++++++---- server/datastore/mysql/mdm_test.go | 34 ++++++++++++++++--- server/datastore/mysql/microsoft_mdm.go | 22 ++++++++---- .../service/integration_mdm_profiles_test.go | 9 ++++- 5 files changed, 71 insertions(+), 17 deletions(-) create mode 100644 changes/22162-exclude-labels-fix-default-behavior diff --git a/changes/22162-exclude-labels-fix-default-behavior b/changes/22162-exclude-labels-fix-default-behavior new file mode 100644 index 000000000000..41524c8c0399 --- /dev/null +++ b/changes/22162-exclude-labels-fix-default-behavior @@ -0,0 +1 @@ +* Fixed the MDM configuration profiles deployment when based on excluded labels - prior to this fix, hosts were considered "not a member" of the label by default, even if they had not yet returned results for the excluded labels. The fix checks the label's creation time vs the host's last reported label results timestamp to prevent deploying a configuration profile if it does not yet know if the host is a member or not of those labels. diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index d2587f076418..e7d18cbdb69d 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -2318,7 +2318,8 @@ func generateDesiredStateQuery(entityType string) string { mae.checksum as checksum, 0 as ${countEntityLabelsColumn}, 0 as count_non_broken_labels, - 0 as count_host_labels + 0 as count_host_labels, + 0 as count_host_updated_after_labels FROM ${mdmAppleEntityTable} mae JOIN hosts h @@ -2350,7 +2351,8 @@ func generateDesiredStateQuery(entityType string) string { mae.checksum as checksum, COUNT(*) as ${countEntityLabelsColumn}, COUNT(mel.label_id) as count_non_broken_labels, - COUNT(lm.label_id) as count_host_labels + COUNT(lm.label_id) as count_host_labels, + 0 as count_host_updated_after_labels FROM ${mdmAppleEntityTable} mae JOIN hosts h @@ -2374,7 +2376,10 @@ func generateDesiredStateQuery(entityType string) string { UNION -- label-based entities where the host is NOT a member of any of the labels (exclude-any). - -- explicitly ignore profiles with broken excluded labels so that they are never applied. + -- explicitly ignore profiles with broken excluded labels so that they are never applied, + -- and ignore profiles that depend on labels created _after_ the label_updated_at timestamp + -- of the host (because we don't have results for that label yet, the host may or may not be + -- a member). SELECT mae.${entityUUIDColumn}, h.uuid as host_uuid, @@ -2384,7 +2389,10 @@ func generateDesiredStateQuery(entityType string) string { mae.checksum as checksum, COUNT(*) as ${countEntityLabelsColumn}, COUNT(mel.label_id) as count_non_broken_labels, - COUNT(lm.label_id) as count_host_labels + COUNT(lm.label_id) as count_host_labels, + -- this helps avoid the case where the host is not a member of a label + -- just because it hasn't reported results for that label yet. + SUM(CASE WHEN lbl.created_at IS NOT NULL AND h.label_updated_at >= lbl.created_at THEN 1 ELSE 0 END) as count_host_updated_after_labels FROM ${mdmAppleEntityTable} mae JOIN hosts h @@ -2393,6 +2401,8 @@ func generateDesiredStateQuery(entityType string) string { ON ne.device_id = h.uuid JOIN ${mdmEntityLabelsTable} mel ON mel.${appleEntityUUIDColumn} = mae.${entityUUIDColumn} AND mel.exclude = 1 + LEFT OUTER JOIN labels lbl + ON lbl.id = mel.label_id LEFT OUTER JOIN label_membership lm ON lm.label_id = mel.label_id AND lm.host_id = h.id WHERE @@ -2403,8 +2413,8 @@ func generateDesiredStateQuery(entityType string) string { GROUP BY mae.${entityUUIDColumn}, h.uuid, h.platform, mae.identifier, mae.name, mae.checksum HAVING - -- considers only the profiles with labels, without any broken label, and with the host not in any label - ${countEntityLabelsColumn} > 0 AND ${countEntityLabelsColumn} = count_non_broken_labels AND count_host_labels = 0 + -- considers only the profiles with labels, without any broken label, with results reported after all labels were created and with the host not in any label + ${countEntityLabelsColumn} > 0 AND ${countEntityLabelsColumn} = count_non_broken_labels AND ${countEntityLabelsColumn} = count_host_updated_after_labels AND count_host_labels = 0 `, func(s string) string { return dynamicNames[s] }) } diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index 760b2dac58f8..43e94d71dfca 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -7358,7 +7358,7 @@ func testBulkSetPendingMDMHostProfilesExcludeAny(t *testing.T, ds *Datastore) { } allProfs := getProfs(nil) - // create an Apple and Windows hosts, not members of any host + // create an Apple and Windows hosts, not members of any label var i int winHost, err := ds.NewHost(ctx, &fleet.Host{ Hostname: fmt.Sprintf("win-host%d-name", i), @@ -7381,14 +7381,33 @@ func testBulkSetPendingMDMHostProfilesExcludeAny(t *testing.T, ds *Datastore) { require.NoError(t, err) nanoEnroll(t, ds, appleHost, false) - // do a sync, they get all platform-specific profiles since they are not part - // of any label + // at this point the hosts have not reported any label results, so a sync + // does NOT install the exclude any profiles as we don't know yet if the + // hosts will be members or not + updates, err = ds.BulkSetPendingMDMHostProfiles(ctx, []uint{winHost.ID, appleHost.ID}, nil, nil, nil) + require.NoError(t, err) + assert.False(t, updates.AppleConfigProfile) + assert.False(t, updates.WindowsConfigProfile) + assert.False(t, updates.AppleDeclaration) + assertHostProfiles(t, ds, map[*fleet.Host][]anyProfile{ + appleHost: {}, + winHost: {}, + }) + + // setting the LabelsUpdatedAt timestamp means that labels results were reported, so now + // the profiles will be installed as the hosts are not members of the excluded labels. + winHost.LabelUpdatedAt = time.Now() + appleHost.LabelUpdatedAt = time.Now() + err = ds.UpdateHost(ctx, winHost) + require.NoError(t, err) + err = ds.UpdateHost(ctx, appleHost) + require.NoError(t, err) + updates, err = ds.BulkSetPendingMDMHostProfiles(ctx, []uint{winHost.ID, appleHost.ID}, nil, nil, nil) require.NoError(t, err) assert.True(t, updates.AppleConfigProfile) assert.True(t, updates.WindowsConfigProfile) assert.True(t, updates.AppleDeclaration) - assertHostProfiles(t, ds, map[*fleet.Host][]anyProfile{ appleHost: { { @@ -7555,6 +7574,13 @@ func testBulkSetPendingMDMHostProfilesExcludeAny(t *testing.T, ds *Datastore) { require.NoError(t, err) nanoEnroll(t, ds, appleHost2, false) + winHost2.LabelUpdatedAt = time.Now() + appleHost2.LabelUpdatedAt = time.Now() + err = ds.UpdateHost(ctx, winHost2) + require.NoError(t, err) + err = ds.UpdateHost(ctx, appleHost2) + require.NoError(t, err) + updates, err = ds.BulkSetPendingMDMHostProfiles(ctx, []uint{winHost.ID, appleHost.ID, winHost2.ID, appleHost2.ID}, nil, nil, nil) require.NoError(t, err) assert.False(t, updates.AppleConfigProfile) diff --git a/server/datastore/mysql/microsoft_mdm.go b/server/datastore/mysql/microsoft_mdm.go index 112d60b86811..a260f4e15bba 100644 --- a/server/datastore/mysql/microsoft_mdm.go +++ b/server/datastore/mysql/microsoft_mdm.go @@ -1131,7 +1131,8 @@ const windowsMDMProfilesDesiredStateQuery = ` h.uuid as host_uuid, 0 as count_profile_labels, 0 as count_non_broken_labels, - 0 as count_host_labels + 0 as count_host_labels, + 0 as count_host_updated_after_labels FROM mdm_windows_configuration_profiles mwcp JOIN hosts h @@ -1158,7 +1159,8 @@ const windowsMDMProfilesDesiredStateQuery = ` h.uuid as host_uuid, COUNT(*) as count_profile_labels, COUNT(mcpl.label_id) as count_non_broken_labels, - COUNT(lm.label_id) as count_host_labels + COUNT(lm.label_id) as count_host_labels, + 0 as count_host_updated_after_labels FROM mdm_windows_configuration_profiles mwcp JOIN hosts h @@ -1180,14 +1182,20 @@ const windowsMDMProfilesDesiredStateQuery = ` UNION -- label-based entities where the host is NOT a member of any of the labels (exclude-any). - -- explicitly ignore profiles with broken excluded labels so that they are never applied. + -- explicitly ignore profiles with broken excluded labels so that they are never applied, + -- and ignore profiles that depend on labels created _after_ the label_updated_at timestamp + -- of the host (because we don't have results for that label yet, the host may or may not be + -- a member). SELECT mwcp.profile_uuid, mwcp.name, h.uuid as host_uuid, COUNT(*) as count_profile_labels, COUNT(mcpl.label_id) as count_non_broken_labels, - COUNT(lm.label_id) as count_host_labels + COUNT(lm.label_id) as count_host_labels, + -- this helps avoid the case where the host is not a member of a label + -- just because it hasn't reported results for that label yet. + SUM(CASE WHEN lbl.created_at IS NOT NULL AND h.label_updated_at >= lbl.created_at THEN 1 ELSE 0 END) as count_host_updated_after_labels FROM mdm_windows_configuration_profiles mwcp JOIN hosts h @@ -1196,6 +1204,8 @@ const windowsMDMProfilesDesiredStateQuery = ` ON mwe.host_uuid = h.uuid JOIN mdm_configuration_profile_labels mcpl ON mcpl.windows_profile_uuid = mwcp.profile_uuid AND mcpl.exclude = 1 + LEFT OUTER JOIN labels lbl + ON lbl.id = mcpl.label_id LEFT OUTER JOIN label_membership lm ON lm.label_id = mcpl.label_id AND lm.host_id = h.id WHERE @@ -1204,8 +1214,8 @@ const windowsMDMProfilesDesiredStateQuery = ` GROUP BY mwcp.profile_uuid, mwcp.name, h.uuid HAVING - -- considers only the profiles with labels, without any broken label, and with the host not in any label - count_profile_labels > 0 AND count_profile_labels = count_non_broken_labels AND count_host_labels = 0 + -- considers only the profiles with labels, without any broken label, with results reported after all labels were created and with the host not in any label + count_profile_labels > 0 AND count_profile_labels = count_non_broken_labels AND count_profile_labels = count_host_updated_after_labels AND count_host_labels = 0 ` func (ds *Datastore) ListMDMWindowsProfilesToInstall(ctx context.Context) ([]*fleet.MDMWindowsProfilePayload, error) { diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index a9a5ad60aa82..dc9250ca4919 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -4653,6 +4653,13 @@ func (s *integrationMDMTestSuite) TestHostMDMProfilesExcludeLabels() { require.NoError(t, err) labels[i] = label } + // simulate reporting label results for those hosts + appleHost.LabelUpdatedAt = time.Now() + windowsHost.LabelUpdatedAt = time.Now() + err := s.ds.UpdateHost(ctx, appleHost) + require.NoError(t, err) + err = s.ds.UpdateHost(ctx, windowsHost) + require.NoError(t, err) // set an Apple profile and declaration and a Windows profile s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ @@ -4694,7 +4701,7 @@ func (s *integrationMDMTestSuite) TestHostMDMProfilesExcludeLabels() { }) // mark some profiles as verified (despite accepting a HostMacOSProfile struct, it supports Windows too) - err := apple_mdm.VerifyHostMDMProfiles(ctx, s.ds, appleHost, map[string]*fleet.HostMacOSProfile{ + err = apple_mdm.VerifyHostMDMProfiles(ctx, s.ds, appleHost, map[string]*fleet.HostMacOSProfile{ "A1": {Identifier: "A1", DisplayName: "A1", InstallDate: time.Now()}, }) require.NoError(t, err)