diff --git a/changes/32073-cpie-integration-test2 b/changes/32073-cpie-integration-test2 new file mode 100644 index 0000000000..63f585eb93 --- /dev/null +++ b/changes/32073-cpie-integration-test2 @@ -0,0 +1,2 @@ +* `labels_exclude_any` can now be combined with `labels_include_all` or `labels_include_any` when uploading MDM configuration profiles, allowing hosts to be included by label membership and excluded by another set of labels simultaneously. +* Fleet now prevents deleting a label that is in use by an MDM configuration profile or declaration, returning an error instead of silently breaking the profile's label targeting. diff --git a/cmd/fleetctl/fleetctl/gitops.go b/cmd/fleetctl/fleetctl/gitops.go index 676aa041f6..53f809e868 100644 --- a/cmd/fleetctl/fleetctl/gitops.go +++ b/cmd/fleetctl/fleetctl/gitops.go @@ -851,24 +851,19 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) { if osSettings, ok := getCustomSettings(osSettingName); ok { for _, setting := range osSettings { var labels []string - err := fmt.Errorf("configuration profile '%s' has multiple label keys; please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`.", filepath.Base(setting.Path)) - if len(setting.LabelsIncludeAny) > 0 { labels = setting.LabelsIncludeAny } + if len(setting.LabelsIncludeAll) > 0 && len(setting.LabelsIncludeAny) > 0 { + return nil, fmt.Errorf("Couldn't edit configuration profiles. For profile '%s', only one of \"labels_include_all\" or \"labels_include_any\" can be included.", filepath.Base(setting.Path)) + } if len(setting.LabelsIncludeAll) > 0 { - if len(labels) > 0 { - return nil, err - } labels = setting.LabelsIncludeAll } - if len(setting.LabelsExcludeAny) > 0 { - if len(labels) > 0 { - return nil, err - } - labels = setting.LabelsExcludeAny + if overlap := fleet.ProfileLabelOverlap(labels, setting.LabelsExcludeAny); overlap != "" { + return nil, fmt.Errorf("configuration profile '%s': label %q cannot appear in both include and exclude lists.", filepath.Base(setting.Path), overlap) } - + labels = append(labels, setting.LabelsExcludeAny...) updateLabelUsage(labels, filepath.Base(setting.Path), "configuration profile", result) } } diff --git a/cmd/fleetctl/fleetctl/gitops_label_usage_test.go b/cmd/fleetctl/fleetctl/gitops_label_usage_test.go index 27c9a7954d..67b4fd1446 100644 --- a/cmd/fleetctl/fleetctl/gitops_label_usage_test.go +++ b/cmd/fleetctl/fleetctl/gitops_label_usage_test.go @@ -3,68 +3,115 @@ package fleetctl import ( "testing" + "github.com/fleetdm/fleet/v4/pkg/optjson" "github.com/fleetdm/fleet/v4/pkg/spec" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func makeProfileConfig(t *testing.T, profiles []fleet.MDMProfileSpec, osName string) *spec.GitOps { + t.Helper() + config := &spec.GitOps{} + switch osName { + case "macos": + config.Controls.MacOSSettings = &fleet.MacOSSettings{CustomSettings: profiles} + case "windows": + config.Controls.WindowsSettings = &fleet.WindowsSettings{CustomSettings: optjson.SetSlice(profiles)} + default: + t.Fatalf("unknown os: %s", osName) + } + return config +} + func TestGetLabelUsageProfilePathShortened(t *testing.T) { // Simulate what happens when spec parsing resolves a relative path to absolute. absPath := "/home/runner/work/Detroit-GitOps-Workshop/Detroit-GitOps-Workshop/lib/macos/configuration-profiles/disable-bluetooth-file-sharing.mobileconfig" + profiles := []fleet.MDMProfileSpec{{Path: absPath, LabelsIncludeAll: []string{"nonexistent-label"}}} - config := &spec.GitOps{ - Controls: spec.GitOpsControls{ - MacOSSettings: &fleet.MacOSSettings{ - CustomSettings: []fleet.MDMProfileSpec{ - { - Path: absPath, - LabelsIncludeAll: []string{"nonexistent-label"}, - }, - }, - }, - }, + for _, osName := range []string{"macos", "windows"} { + t.Run(osName, func(t *testing.T) { + usage, err := getLabelUsage(makeProfileConfig(t, profiles, osName)) + require.NoError(t, err) + + // The label "nonexistent-label" should be in the usage map. + entries, ok := usage["nonexistent-label"] + require.True(t, ok, "expected label to be in usage map") + require.Len(t, entries, 1) + + // The Name should be the base filename, not the full absolute path. + assert.Equal(t, "disable-bluetooth-file-sharing.mobileconfig", entries[0].Name, + "profile path should be shortened to just the filename") + + // The Type should be "configuration profile", not "MDM Profile". + assert.Equal(t, "configuration profile", entries[0].Type, + "type should say 'configuration profile' not 'MDM Profile'") + }) } - - usage, err := getLabelUsage(config) - require.NoError(t, err) - - // The label "nonexistent-label" should be in the usage map. - entries, ok := usage["nonexistent-label"] - require.True(t, ok, "expected label to be in usage map") - require.Len(t, entries, 1) - - // The Name should be the base filename, not the full absolute path. - assert.Equal(t, "disable-bluetooth-file-sharing.mobileconfig", entries[0].Name, - "profile path should be shortened to just the filename") - - // The Type should be "configuration profile", not "MDM Profile". - assert.Equal(t, "configuration profile", entries[0].Type, - "type should say 'configuration profile' not 'MDM Profile'") } func TestGetLabelUsageMultipleLabelKeysError(t *testing.T) { absPath := "/absolute/path/to/profile.mobileconfig" + profiles := []fleet.MDMProfileSpec{{ + Path: absPath, + LabelsIncludeAll: []string{"label-a"}, + LabelsIncludeAny: []string{"label-b"}, + }} - config := &spec.GitOps{ - Controls: spec.GitOpsControls{ - MacOSSettings: &fleet.MacOSSettings{ - CustomSettings: []fleet.MDMProfileSpec{ - { - Path: absPath, - LabelsIncludeAll: []string{"label-a"}, - LabelsIncludeAny: []string{"label-b"}, - }, - }, - }, - }, + // Two include modes together is still invalid. + for _, osName := range []string{"macos", "windows"} { + t.Run(osName, func(t *testing.T) { + _, err := getLabelUsage(makeProfileConfig(t, profiles, osName)) + require.Error(t, err) + + // Error should use "configuration profile" and the short filename. + assert.Contains(t, err.Error(), "configuration profile") + assert.Contains(t, err.Error(), "profile.mobileconfig") + assert.NotContains(t, err.Error(), "/absolute/path/to/") + }) + } +} + +func TestGetLabelUsageIncludeExcludeOverlapError(t *testing.T) { + absPath := "/absolute/path/to/profile.mobileconfig" + + // A label appearing in both include and exclude is rejected before any apply. + cases := []fleet.MDMProfileSpec{ + {Path: absPath, LabelsIncludeAll: []string{"shared-label"}, LabelsExcludeAny: []string{"shared-label"}}, + {Path: absPath, LabelsIncludeAny: []string{"shared-label"}, LabelsExcludeAny: []string{"shared-label"}}, + {Path: absPath, LabelsIncludeAll: []string{"ok-label", "shared-label"}, LabelsExcludeAny: []string{"shared-label", "other-label"}}, + } + for _, profileSpec := range cases { + for _, osName := range []string{"macos", "windows"} { + t.Run(osName, func(t *testing.T) { + _, err := getLabelUsage(makeProfileConfig(t, []fleet.MDMProfileSpec{profileSpec}, osName)) + require.Error(t, err) + assert.Contains(t, err.Error(), "shared-label") + assert.Contains(t, err.Error(), "profile.mobileconfig") + }) + } + } +} + +func TestGetLabelUsageIncludeAndExcludeAllowed(t *testing.T) { + absPath := "/absolute/path/to/profile.mobileconfig" + + // include + exclude combination is valid for configuration profiles. + cases := []fleet.MDMProfileSpec{ + {Path: absPath, LabelsIncludeAll: []string{"include-label"}, LabelsExcludeAny: []string{"exclude-label"}}, + {Path: absPath, LabelsIncludeAny: []string{"include-label"}, LabelsExcludeAny: []string{"exclude-label"}}, + {Path: absPath, LabelsExcludeAny: []string{"exclude-label"}}, + } + for _, profileSpec := range cases { + for _, osName := range []string{"macos", "windows"} { + t.Run(osName, func(t *testing.T) { + usage, err := getLabelUsage(makeProfileConfig(t, []fleet.MDMProfileSpec{profileSpec}, osName)) + require.NoError(t, err) + allLabels := append(profileSpec.LabelsIncludeAll, append(profileSpec.LabelsIncludeAny, profileSpec.LabelsExcludeAny...)...) //nolint:gocritic + for _, label := range allLabels { + assert.Contains(t, usage, label) + } + }) + } } - - _, err := getLabelUsage(config) - require.Error(t, err) - - // Error should use "configuration profile" and the short filename. - assert.Contains(t, err.Error(), "configuration profile") - assert.Contains(t, err.Error(), "profile.mobileconfig") - assert.NotContains(t, err.Error(), "/absolute/path/to/") } diff --git a/cmd/fleetctl/fleetctl/gitops_test.go b/cmd/fleetctl/fleetctl/gitops_test.go index 19a6ac0277..5ac0277d1e 100644 --- a/cmd/fleetctl/fleetctl/gitops_test.go +++ b/cmd/fleetctl/fleetctl/gitops_test.go @@ -3647,13 +3647,13 @@ func TestGitOpsCustomSettings(t *testing.T) { }{ {"testdata/gitops/global_macos_windows_custom_settings_valid.yml", ""}, {"testdata/gitops/global_macos_custom_settings_valid_deprecated.yml", ""}, - {"testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml", "please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`"}, - {"testdata/gitops/global_windows_custom_settings_invalid_label_mix_2.yml", "please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`"}, + {"testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml", `only one of "labels_include_all" or "labels_include_any" can be included`}, + {"testdata/gitops/global_windows_custom_settings_invalid_label_mix_2.yml", `only one of "labels_include_all" or "labels_include_any" can be included`}, {"testdata/gitops/global_windows_custom_settings_unknown_label.yml", `Please create the missing labels, or update your settings to not refer to these labels.`}, {"testdata/gitops/team_macos_windows_custom_settings_valid.yml", ""}, {"testdata/gitops/team_macos_custom_settings_valid_deprecated.yml", ""}, - {"testdata/gitops/team_macos_windows_custom_settings_invalid_labels_mix.yml", "please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`"}, - {"testdata/gitops/team_macos_windows_custom_settings_invalid_labels_mix_2.yml", "please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`"}, + {"testdata/gitops/team_macos_windows_custom_settings_invalid_labels_mix.yml", `only one of "labels_include_all", "labels_include_any" or "labels" can be included`}, + {"testdata/gitops/team_macos_windows_custom_settings_invalid_labels_mix_2.yml", `only one of "labels_include_all", "labels_include_any" or "labels" can be included`}, {"testdata/gitops/team_macos_windows_custom_settings_unknown_label.yml", `Please create the missing labels, or update your settings to not refer to these labels.`}, } for _, c := range cases { diff --git a/cmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml b/cmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml index 1a100de6f3..4c0d7f6a9a 100644 --- a/cmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml +++ b/cmd/fleetctl/fleetctl/testdata/gitops/global_windows_custom_settings_invalid_label_mix.yml @@ -4,7 +4,7 @@ controls: - path: ./lib/windows-screenlock.xml labels_include_all: - B - labels_exclude_any: + labels_include_any: - C scripts: enable_disk_encryption: false diff --git a/ee/server/service/teams.go b/ee/server/service/teams.go index 9d93b126dd..1003ae01fe 100644 --- a/ee/server/service/teams.go +++ b/ee/server/service/teams.go @@ -1440,9 +1440,9 @@ func (svc *Service) createTeamFromSpec( if enableDiskEncryption && svc.config.Server.PrivateKey == "" { return nil, ctxerr.New(ctx, "Missing required private key. Learn how to configure the private key here: https://fleetdm.com/learn-more-about/fleet-server-private-key") } - validateTeamCustomSettings(invalid, "macos", macOSSettings.CustomSettings) - validateTeamCustomSettings(invalid, "windows", spec.MDM.WindowsSettings.CustomSettings.Value) - validateTeamCustomSettings(invalid, "android", spec.MDM.AndroidSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "macos", macOSSettings.CustomSettings) + fleet.ValidateMDMProfileSpecs(invalid, "windows", spec.MDM.WindowsSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "android", spec.MDM.AndroidSettings.CustomSettings.Value) var hostExpirySettings fleet.HostExpirySettings if spec.HostExpirySettings != nil { @@ -1845,9 +1845,9 @@ func (svc *Service) editTeamFromSpec( team.Config.HostExpirySettings = *spec.HostExpirySettings } - validateTeamCustomSettings(invalid, "apple", team.Config.MDM.MacOSSettings.CustomSettings) - validateTeamCustomSettings(invalid, "windows", team.Config.MDM.WindowsSettings.CustomSettings.Value) - validateTeamCustomSettings(invalid, "android", team.Config.MDM.AndroidSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "apple", team.Config.MDM.MacOSSettings.CustomSettings) + fleet.ValidateMDMProfileSpecs(invalid, "windows", team.Config.MDM.WindowsSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "android", team.Config.MDM.AndroidSettings.CustomSettings.Value) // If host status webhook is not provided, do not change it if spec.WebhookSettings.HostStatusWebhook != nil { @@ -2061,25 +2061,6 @@ func (svc *Service) editTeamFromSpec( return nil } -func validateTeamCustomSettings(invalid *fleet.InvalidArgumentError, prefix string, customSettings []fleet.MDMProfileSpec) { - for i, prof := range customSettings { - count := 0 - for _, b := range []bool{len(prof.Labels) > 0, len(prof.LabelsIncludeAll) > 0, len(prof.LabelsIncludeAny) > 0, len(prof.LabelsExcludeAny) > 0} { - if b { - count++ - } - } - if count > 1 { - invalid.Append(fmt.Sprintf("%s_settings.configuration_profiles", prefix), - fmt.Sprintf(`Couldn't edit %s_settings.configuration_profiles. For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`, prefix)) - } - if len(prof.Labels) > 0 { - customSettings[i].LabelsIncludeAll = customSettings[i].Labels - customSettings[i].Labels = nil - } - } -} - func (svc *Service) validateTeamCalendarIntegrations( calendarIntegration *fleet.TeamGoogleCalendarIntegration, appCfg *fleet.AppConfig, dryRun bool, invalid *fleet.InvalidArgumentError, diff --git a/server/datastore/mysql/apple_mdm_test.go b/server/datastore/mysql/apple_mdm_test.go index 8f789c071c..c7b57f6bcd 100644 --- a/server/datastore/mysql/apple_mdm_test.go +++ b/server/datastore/mysql/apple_mdm_test.go @@ -309,7 +309,7 @@ func testNewMDMAppleConfigProfileDuplicateIdentifier(t *testing.T, ds *Datastore // simulate a broken label by nullifying its id in the join table // (direct DeleteLabel is now blocked when referenced by a profile) ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE label_id = ?`, lbl.ID) + _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE apple_profile_uuid = ? AND label_id = ?`, labelProf.ProfileUUID, lbl.ID) return err }) diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index 21c2baa9fc..6b0fc02c1e 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -699,6 +699,19 @@ func (ds *Datastore) DeleteLabel(ctx context.Context, name string, filter fleet. } return ctxerr.Wrap(ctx, err, "getting label id to delete") } + var usedByProfile bool + if err := sqlx.GetContext(ctx, tx, &usedByProfile, ` + SELECT EXISTS( + SELECT 1 FROM mdm_configuration_profile_labels WHERE label_id = ? + UNION ALL + SELECT 1 FROM mdm_declaration_labels WHERE label_id = ? + )`, labelID, labelID); err != nil { + return ctxerr.Wrap(ctx, err, "checking if label is used by configuration profiles") + } + if usedByProfile { + return ctxerr.Wrap(ctx, foreignKey("configuration profile labels", name), "delete label") + } + if err := deleteLabelsInTx(ctx, tx, []uint{labelID}); err != nil { if isMySQLForeignKey(err) { return ctxerr.Wrap(ctx, foreignKey("labels", name), "delete label") diff --git a/server/datastore/mysql/labels_test.go b/server/datastore/mysql/labels_test.go index 5287173b3a..d493d21cbc 100644 --- a/server/datastore/mysql/labels_test.go +++ b/server/datastore/mysql/labels_test.go @@ -1351,6 +1351,54 @@ func testDeleteLabel(t *testing.T, db *Datastore) { // Admin with team filter can delete err = db.DeleteLabel(ctx, team2Label.Name, fleet.TeamFilter{User: adminUser, TeamID: &team2.ID}) require.NoError(t, err) + + // create a label referenced by a configuration profile — deletion must be blocked + l3, err := db.NewLabel(ctx, &fleet.Label{ + Name: t.Name() + "3", + Query: "query3", + }) + require.NoError(t, err) + + prof, err := db.NewMDMAppleConfigProfile(ctx, *generateAppleCP("test-prof", "com.example.test", 0), nil) + require.NoError(t, err) + + ExecAdhocSQL(t, db, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + `INSERT INTO mdm_configuration_profile_labels (apple_profile_uuid, label_name, label_id) VALUES (?, ?, ?)`, + prof.ProfileUUID, l3.Name, l3.ID, + ) + return err + }) + + err = db.DeleteLabel(ctx, l3.Name, fleet.TeamFilter{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) + require.Error(t, err) + require.True(t, fleet.IsForeignKey(err)) + + // create a label referenced only by a declaration — deletion must also be blocked + l4, err := db.NewLabel(ctx, &fleet.Label{ + Name: t.Name() + "4", + Query: "query4", + }) + require.NoError(t, err) + + decl, err := db.NewMDMAppleDeclaration(ctx, &fleet.MDMAppleDeclaration{ + Identifier: "com.example.decl-test", + Name: "test-decl", + RawJSON: json.RawMessage(`{"Identifier": "com.example.decl-test"}`), + }, nil) + require.NoError(t, err) + + ExecAdhocSQL(t, db, func(q sqlx.ExtContext) error { + _, err := q.ExecContext(ctx, + `INSERT INTO mdm_declaration_labels (apple_declaration_uuid, label_name, label_id) VALUES (?, ?, ?)`, + decl.DeclarationUUID, l4.Name, l4.ID, + ) + return err + }) + + err = db.DeleteLabel(ctx, l4.Name, fleet.TeamFilter{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) + require.Error(t, err) + require.True(t, fleet.IsForeignKey(err)) } func testLabelsSummaryAndListTeamFiltering(t *testing.T, db *Datastore) { diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index a47831bc15..d99f9df2d8 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -2383,11 +2383,15 @@ func testGetHostMDMProfilesExpectedForVerification(t *testing.T, ds *Datastore) require.NoError(t, err) require.Len(t, profs, 3) - // Null label reference first (FK is now RESTRICT), then delete — the profile - // will appear broken and we shouldn't see it in the results - simulateBrokenLabel(t, ds, ctx, testLabel4.Name) - err = ds.DeleteLabel(ctx, testLabel4.Name, fleet.TeamFilter{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) - require.NoError(t, err) + // Simulate the label being broken — direct DeleteLabel is now blocked when + // referenced by a profile, so we nullify label_id in the join tables instead. + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + if _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE label_id = ?`, testLabel4.ID); err != nil { + return err + } + _, err := q.ExecContext(ctx, `UPDATE mdm_declaration_labels SET label_id = NULL WHERE label_id = ?`, testLabel4.ID) + return err + }) return team.ID, host } @@ -2886,11 +2890,15 @@ func testGetHostMDMProfilesExpectedForVerification(t *testing.T, ds *Datastore) require.NoError(t, err) require.Len(t, profs, 3) - // Null label reference first (FK is now RESTRICT), then delete — the profile - // will appear broken and we shouldn't see it in the results - simulateBrokenLabel(t, ds, ctx, label.Name) - err = ds.DeleteLabel(ctx, label.Name, fleet.TeamFilter{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) - require.NoError(t, err) + // Simulate the label being broken — direct DeleteLabel is now blocked when + // referenced by a profile, so we nullify label_id in the join tables instead. + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + if _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE label_id = ?`, label.ID); err != nil { + return err + } + _, err := q.ExecContext(ctx, `UPDATE mdm_declaration_labels SET label_id = NULL WHERE label_id = ?`, label.ID) + return err + }) return team.ID, host } diff --git a/server/datastore/mysql/microsoft_mdm_test.go b/server/datastore/mysql/microsoft_mdm_test.go index 88acbcd2af..be87b896c6 100644 --- a/server/datastore/mysql/microsoft_mdm_test.go +++ b/server/datastore/mysql/microsoft_mdm_test.go @@ -2851,7 +2851,7 @@ func testMDMWindowsConfigProfiles(t *testing.T, ds *Datastore) { // simulate a broken label by nullifying its id in the join table // (direct DeleteLabel is now blocked when referenced by a profile) ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE label_id = ?`, label.ID) + _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL WHERE windows_profile_uuid = ? AND label_id = ?`, profWithLabel.ProfileUUID, label.ID) return err }) diff --git a/server/fleet/mdm.go b/server/fleet/mdm.go index 852be6d9ad..529431eadd 100644 --- a/server/fleet/mdm.go +++ b/server/fleet/mdm.go @@ -1380,3 +1380,46 @@ const ( // SSOInitiatorAppleMDMSSO is used for automatic MDM Apple enrollment SSO flow. SSOInitiatorAppleMDMSSO = "mdm_sso" ) + +// ValidateMDMProfileSpecs validates the label configuration for each profile spec: exactly one +// include mode may be set, no label may appear in both include and exclude lists, and the legacy +// Labels field is normalised to LabelsIncludeAll. Errors are accumulated into invalid. +func ValidateMDMProfileSpecs(invalid *InvalidArgumentError, prefix string, customSettings []MDMProfileSpec) { + for i, prof := range customSettings { + includeCount := 0 + for _, b := range []bool{len(prof.Labels) > 0, len(prof.LabelsIncludeAll) > 0, len(prof.LabelsIncludeAny) > 0} { + if b { + includeCount++ + } + } + if includeCount > 1 { + invalid.Append(fmt.Sprintf("%s_settings.configuration_profiles", prefix), + fmt.Sprintf(`Couldn't edit %s_settings.configuration_profiles. For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`, prefix)) + } + includeLabels := slices.Concat(prof.Labels, prof.LabelsIncludeAll, prof.LabelsIncludeAny) + if overlap := ProfileLabelOverlap(includeLabels, prof.LabelsExcludeAny); overlap != "" { + invalid.Append(fmt.Sprintf("%s_settings.configuration_profiles", prefix), + fmt.Sprintf(`Couldn't edit %s_settings.configuration_profiles. Label %q cannot appear in both include and exclude lists.`, prefix, overlap)) + } + if len(prof.Labels) > 0 { + customSettings[i].LabelsIncludeAll = customSettings[i].Labels + customSettings[i].Labels = nil + } + } +} + +// ProfileLabelOverlap returns the first label name that appears in both +// the include list and the exclude list, or an empty string if there is none. +// include should be the union of labels_include_all and labels_include_any. +func ProfileLabelOverlap(include, exclude []string) string { + seen := make(map[string]struct{}, len(include)) + for _, n := range include { + seen[n] = struct{}{} + } + for _, n := range exclude { + if _, overlapExists := seen[n]; overlapExists { + return n + } + } + return "" +} diff --git a/server/fleet/service.go b/server/fleet/service.go index 7d34976a8e..f85d1f9835 100644 --- a/server/fleet/service.go +++ b/server/fleet/service.go @@ -913,9 +913,9 @@ type Service interface { GetHostDEPAssignmentDetails(ctx context.Context, hostID uint) (*HostDEPAssignment, *godep.Device, error) // NewMDMAppleConfigProfile creates a new configuration profile for the specified team. - NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labels []string, labelsMembershipMode MDMLabelsMode) (*MDMAppleConfigProfile, error) + NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labelsInclude []string, labelsMembershipMode MDMLabelsMode, labelsExcludeAny []string) (*MDMAppleConfigProfile, error) // NewMDMAppleConfigProfileWithPayload creates a new declaration for the specified team. - NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labels []string, name string, labelsMembershipMode MDMLabelsMode) (*MDMAppleDeclaration, error) + NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labelsInclude []string, name string, labelsMembershipMode MDMLabelsMode, labelsExcludeAny []string) (*MDMAppleDeclaration, error) // GetMDMAppleConfigProfileByDeprecatedID retrieves the specified Apple // configuration profile via its numeric ID. This method is deprecated and @@ -1212,7 +1212,7 @@ type Service interface { // NewMDMWindowsConfigProfile creates a new Windows configuration profile for // the specified team. - NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode MDMLabelsMode) (*MDMWindowsConfigProfile, error) + NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode MDMLabelsMode, labelsExcludeAny []string) (*MDMWindowsConfigProfile, error) // NewMDMUnsupportedConfigProfile is called when a profile with an // unsupported extension is uploaded. @@ -1248,7 +1248,7 @@ type Service interface { // Android MDM // NewMDMAndroidConfigProfile creates a new Android configuration profile - NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode MDMLabelsMode) (*MDMAndroidConfigProfile, error) + NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode MDMLabelsMode, labelsExcludeAny []string) (*MDMAndroidConfigProfile, error) // DeleteMDMAndroidConfigProfile deletes the specified Android profile. DeleteMDMAndroidConfigProfile(ctx context.Context, profileUUID string) error diff --git a/server/mock/service/service_mock.go b/server/mock/service/service_mock.go index 184f8b49de..ae3d87f421 100644 --- a/server/mock/service/service_mock.go +++ b/server/mock/service/service_mock.go @@ -574,9 +574,9 @@ type GetHostDEPAssignmentFunc func(ctx context.Context, host *fleet.Host) (*flee type GetHostDEPAssignmentDetailsFunc func(ctx context.Context, hostID uint) (*fleet.HostDEPAssignment, *godep.Device, error) -type NewMDMAppleConfigProfileFunc func(ctx context.Context, teamID uint, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleConfigProfile, error) +type NewMDMAppleConfigProfileFunc func(ctx context.Context, teamID uint, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleConfigProfile, error) -type NewMDMAppleDeclarationFunc func(ctx context.Context, teamID uint, data []byte, labels []string, name string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleDeclaration, error) +type NewMDMAppleDeclarationFunc func(ctx context.Context, teamID uint, data []byte, labelsInclude []string, name string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleDeclaration, error) type GetMDMAppleConfigProfileByDeprecatedIDFunc func(ctx context.Context, profileID uint) (*fleet.MDMAppleConfigProfile, error) @@ -744,7 +744,7 @@ type DeleteMDMWindowsConfigProfileFunc func(ctx context.Context, profileUUID str type GetMDMWindowsProfilesSummaryFunc func(ctx context.Context, teamID *uint) (*fleet.MDMProfilesSummary, error) -type NewMDMWindowsConfigProfileFunc func(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMWindowsConfigProfile, error) +type NewMDMWindowsConfigProfileFunc func(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMWindowsConfigProfile, error) type NewMDMUnsupportedConfigProfileFunc func(ctx context.Context, teamID uint, filename string) error @@ -758,7 +758,7 @@ type LinuxHostDiskEncryptionStatusFunc func(ctx context.Context, host fleet.Host type GetMDMLinuxProfilesSummaryFunc func(ctx context.Context, teamId *uint) (fleet.MDMProfilesSummary, error) -type NewMDMAndroidConfigProfileFunc func(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAndroidConfigProfile, error) +type NewMDMAndroidConfigProfileFunc func(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAndroidConfigProfile, error) type DeleteMDMAndroidConfigProfileFunc func(ctx context.Context, profileUUID string) error @@ -4233,18 +4233,18 @@ func (s *Service) GetHostDEPAssignmentDetails(ctx context.Context, hostID uint) return s.GetHostDEPAssignmentDetailsFunc(ctx, hostID) } -func (s *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleConfigProfile, error) { +func (s *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleConfigProfile, error) { s.mu.Lock() s.NewMDMAppleConfigProfileFuncInvoked = true s.mu.Unlock() - return s.NewMDMAppleConfigProfileFunc(ctx, teamID, data, labels, labelsMembershipMode) + return s.NewMDMAppleConfigProfileFunc(ctx, teamID, data, labelsInclude, labelsMembershipMode, labelsExcludeAny) } -func (s *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labels []string, name string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleDeclaration, error) { +func (s *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labelsInclude []string, name string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleDeclaration, error) { s.mu.Lock() s.NewMDMAppleDeclarationFuncInvoked = true s.mu.Unlock() - return s.NewMDMAppleDeclarationFunc(ctx, teamID, data, labels, name, labelsMembershipMode) + return s.NewMDMAppleDeclarationFunc(ctx, teamID, data, labelsInclude, name, labelsMembershipMode, labelsExcludeAny) } func (s *Service) GetMDMAppleConfigProfileByDeprecatedID(ctx context.Context, profileID uint) (*fleet.MDMAppleConfigProfile, error) { @@ -4828,11 +4828,11 @@ func (s *Service) GetMDMWindowsProfilesSummary(ctx context.Context, teamID *uint return s.GetMDMWindowsProfilesSummaryFunc(ctx, teamID) } -func (s *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMWindowsConfigProfile, error) { +func (s *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMWindowsConfigProfile, error) { s.mu.Lock() s.NewMDMWindowsConfigProfileFuncInvoked = true s.mu.Unlock() - return s.NewMDMWindowsConfigProfileFunc(ctx, teamID, profileName, data, labels, labelsMembershipMode) + return s.NewMDMWindowsConfigProfileFunc(ctx, teamID, profileName, data, labelsInclude, labelsMembershipMode, labelsExcludeAny) } func (s *Service) NewMDMUnsupportedConfigProfile(ctx context.Context, teamID uint, filename string) error { @@ -4877,11 +4877,11 @@ func (s *Service) GetMDMLinuxProfilesSummary(ctx context.Context, teamId *uint) return s.GetMDMLinuxProfilesSummaryFunc(ctx, teamId) } -func (s *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAndroidConfigProfile, error) { +func (s *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAndroidConfigProfile, error) { s.mu.Lock() s.NewMDMAndroidConfigProfileFuncInvoked = true s.mu.Unlock() - return s.NewMDMAndroidConfigProfileFunc(ctx, teamID, profileName, data, labels, labelsMembershipMode) + return s.NewMDMAndroidConfigProfileFunc(ctx, teamID, profileName, data, labelsInclude, labelsMembershipMode, labelsExcludeAny) } func (s *Service) DeleteMDMAndroidConfigProfile(ctx context.Context, profileUUID string) error { diff --git a/server/service/appconfig.go b/server/service/appconfig.go index 71de614232..f71be255f6 100644 --- a/server/service/appconfig.go +++ b/server/service/appconfig.go @@ -1680,30 +1680,7 @@ func (svc *Service) validateMDM( `Couldn't update setup_experience because MDM features aren't turned on in Fleet. Use fleetctl generate mdm-apple and then fleet serve with mdm configuration to turn on MDM features.`) } } - checkCustomSettings := func(prefix string, customSettings []fleet.MDMProfileSpec) { - for i, prof := range customSettings { - count := 0 - for _, b := range []bool{ - len(prof.Labels) > 0, - len(prof.LabelsIncludeAll) > 0, - len(prof.LabelsIncludeAny) > 0, - len(prof.LabelsExcludeAny) > 0, - } { - if b { - count++ - } - } - if count > 1 { - invalid.Append(fmt.Sprintf("%s_settings.configuration_profiles", prefix), - fmt.Sprintf(`Couldn't edit %s_settings.configuration_profiles. For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`, prefix)) - } - if len(prof.Labels) > 0 { - customSettings[i].LabelsIncludeAll = customSettings[i].Labels - customSettings[i].Labels = nil - } - } - } - checkCustomSettings("macos", mdm.MacOSSettings.CustomSettings) + fleet.ValidateMDMProfileSpecs(invalid, "macos", mdm.MacOSSettings.CustomSettings) if !mdm.WindowsEnabledAndConfigured { if mdm.WindowsSettings.CustomSettings.Set && @@ -1713,7 +1690,7 @@ func (svc *Service) validateMDM( `Couldn’t edit windows_settings.configuration_profiles. Windows MDM isn’t turned on. This can be enabled by setting "controls.windows_enabled_and_configured: true" in the default configuration. Visit https://fleetdm.com/guides/windows-mdm-setup and https://fleetdm.com/docs/configuration/yaml-files#controls to learn more about enabling MDM.`) } } - checkCustomSettings("windows", mdm.WindowsSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "windows", mdm.WindowsSettings.CustomSettings.Value) // Check oldMdm as we bypass the patching of this value, as it's enabled and disabled elsewhere. if !oldMdm.AndroidEnabledAndConfigured { @@ -1724,7 +1701,7 @@ func (svc *Service) validateMDM( `Couldn’t edit android_settings.configuration_profiles. Android MDM isn’t turned on. This can be enabled by setting "controls.android_enabled_and_configured: true" in the default configuration. Visit https://fleetdm.com/guides/android-mdm-setup and https://fleetdm.com/docs/configuration/yaml-files#controls to learn more about enabling MDM.`) } } - checkCustomSettings("android", mdm.AndroidSettings.CustomSettings.Value) + fleet.ValidateMDMProfileSpecs(invalid, "android", mdm.AndroidSettings.CustomSettings.Value) // MacOSUpdates updatingMacOSVersion := mdm.MacOSUpdates.MinimumVersion.Value != "" && diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index df59183f1a..c87180cf64 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -353,7 +353,7 @@ func newMDMAppleConfigProfileEndpoint(ctx context.Context, request interface{}, return &newMDMConfigProfileResponse{Err: err}, nil } // providing an empty set of labels since this endpoint is only maintained for backwards compat - cp, err := svc.NewMDMAppleConfigProfile(ctx, req.TeamID, data, nil, fleet.LabelsIncludeAll) + cp, err := svc.NewMDMAppleConfigProfile(ctx, req.TeamID, data, nil, fleet.LabelsIncludeAll, nil) if err != nil { return &newMDMAppleConfigProfileResponse{Err: err}, nil } @@ -362,7 +362,7 @@ func newMDMAppleConfigProfileEndpoint(ctx context.Context, request interface{}, }, nil } -func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleConfigProfile, error) { +func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleConfigProfile, error) { if err := svc.authz.Authorize(ctx, &fleet.MDMConfigProfileAuthz{TeamID: &teamID}, fleet.ActionWrite); err != nil { return nil, ctxerr.Wrap(ctx, err) } @@ -437,20 +437,20 @@ func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, d cp.Mobileconfig = data cp.SecretsUpdatedAt = secretsUpdatedAt - labelMap, err := svc.validateProfileLabels(ctx, &teamID, labels) + if overlap := fleet.ProfileLabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) + } + includeLabels, excludeLabels, err := svc.validateProfileLabelSets(ctx, &teamID, labelsInclude, labelsExcludeAny) if err != nil { return nil, ctxerr.Wrap(ctx, err, "validating labels") } switch labelsMembershipMode { case fleet.LabelsIncludeAll: - cp.LabelsIncludeAll = labelMap + cp.LabelsIncludeAll = includeLabels case fleet.LabelsIncludeAny: - cp.LabelsIncludeAny = labelMap - case fleet.LabelsExcludeAny: - cp.LabelsExcludeAny = labelMap - default: - // TODO what happens if mode is not set?s + cp.LabelsIncludeAny = includeLabels } + cp.LabelsExcludeAny = excludeLabels // Convert profile variable names to FleetVarName type varNames := make([]fleet.FleetVarName, 0, len(profileVars)) @@ -857,7 +857,7 @@ func additionalNDESValidation(contents string, ndesVars *NDESVarsFound) error { return nil } -func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labels []string, name string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAppleDeclaration, error) { +func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, data []byte, labelsInclude []string, name string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAppleDeclaration, error) { if err := svc.authz.Authorize(ctx, &fleet.MDMConfigProfileAuthz{TeamID: &teamID}, fleet.ActionWrite); err != nil { return nil, ctxerr.Wrap(ctx, err) } @@ -890,7 +890,10 @@ func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, dat teamName = tm.Name } - validatedLabels, err := svc.validateDeclarationLabels(ctx, labels, teamID) + if overlap := fleet.ProfileLabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) + } + validatedIncludeLabels, excludeLabels, err := svc.validateDeclarationLabelSets(ctx, teamID, labelsInclude, labelsExcludeAny) if err != nil { return nil, err } @@ -933,13 +936,12 @@ func (svc *Service) NewMDMAppleDeclaration(ctx context.Context, teamID uint, dat switch labelsMembershipMode { case fleet.LabelsIncludeAny: - d.LabelsIncludeAny = validatedLabels - case fleet.LabelsExcludeAny: - d.LabelsExcludeAny = validatedLabels + d.LabelsIncludeAny = validatedIncludeLabels default: // default to include all - d.LabelsIncludeAll = validatedLabels + d.LabelsIncludeAll = validatedIncludeLabels } + d.LabelsExcludeAny = excludeLabels if err := svc.handleDeclarationSoftwareUpdate(ctx, rawDecl, teamID); err != nil { return nil, ctxerr.Wrap(ctx, err, "handling declaration software update") @@ -1246,19 +1248,6 @@ func (svc *Service) batchValidateDeclarationLabels(ctx context.Context, labelNam return profLabels, nil } -func (svc *Service) validateDeclarationLabels(ctx context.Context, labelNames []string, teamID uint) ([]fleet.ConfigurationProfileLabel, error) { - labelMap, err := svc.batchValidateDeclarationLabels(ctx, labelNames, teamID) - if err != nil { - return nil, ctxerr.Wrap(ctx, err, "validating declaration labels") - } - - var declLabels []fleet.ConfigurationProfileLabel - for _, label := range labelMap { - declLabels = append(declLabels, label) - } - return declLabels, nil -} - type listMDMAppleConfigProfilesRequest struct { TeamID uint `query:"team_id,optional" renameto:"fleet_id"` } diff --git a/server/service/apple_mdm_test.go b/server/service/apple_mdm_test.go index 8ceaf039bb..50dbfb68c2 100644 --- a/server/service/apple_mdm_test.go +++ b/server/service/apple_mdm_test.go @@ -741,11 +741,11 @@ func TestMDMAppleConfigProfileAuthz(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // test authz create new profile (no team) - _, err := svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll, nil) checkShouldFail(err, tt.shouldFailGlobal) // test authz create new profile (team 1) - _, err = svc.NewMDMAppleConfigProfile(ctx, 1, mcBytes, nil, fleet.LabelsIncludeAll) + _, err = svc.NewMDMAppleConfigProfile(ctx, 1, mcBytes, nil, fleet.LabelsIncludeAll, nil) checkShouldFail(err, tt.shouldFailTeam) // test authz list profiles (no team) @@ -808,7 +808,7 @@ func TestNewMDMAppleConfigProfile(t *testing.T) { return &fleet.GroupedCertificateAuthorities{}, nil } - cp, err := svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll) + cp, err := svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll, nil) require.NoError(t, err) require.Equal(t, "Foo", cp.Name) assert.Equal(t, identifier, cp.Identifier) @@ -816,12 +816,12 @@ func TestNewMDMAppleConfigProfile(t *testing.T) { // Unsupported Fleet variable mcBytes = mcBytesForTest("Foo", identifier, "UUID${FLEET_VAR_BOZO}") - _, err = svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll) + _, err = svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll, nil) assert.ErrorContains(t, err, "Fleet variable") // Test profile with FLEET_SECRET in PayloadDisplayName mcBytes = mcBytesForTest("Profile $FLEET_SECRET_PASSWORD", "test.identifier", "UUID") - _, err = svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll) + _, err = svc.NewMDMAppleConfigProfile(ctx, 0, mcBytes, nil, fleet.LabelsIncludeAll, nil) assert.ErrorContains(t, err, "PayloadDisplayName cannot contain FLEET_SECRET variables") } @@ -870,7 +870,7 @@ func TestNewMDMAppleDeclarationFreeLicenseTeam(t *testing.T) { b := declBytesForTest("D1", "d1content") - _, err := svc.NewMDMAppleDeclaration(ctx, 1, b, nil, "name", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 1, b, nil, "name", fleet.LabelsIncludeAll, nil) assert.ErrorIs(t, err, fleet.ErrMissingLicense) } @@ -880,12 +880,12 @@ func TestNewMDMAppleDeclaration(t *testing.T) { // Unsupported Fleet variable b := declBytesForTest("D1", "d1content $FLEET_VAR_BOZO") - _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll, nil) assert.ErrorContains(t, err, "Fleet variable") // decl type missing actual type b = declarationForTestWithType("D1", "com.apple.configuration") - _, err = svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll) + _, err = svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll, nil) assert.ErrorContains(t, err, "Only configuration declarations (com.apple.configuration.) are supported") ds.NewMDMAppleDeclarationFunc = func(ctx context.Context, d *fleet.MDMAppleDeclaration, usesFleetVars []fleet.FleetVarName) (*fleet.MDMAppleDeclaration, error) { @@ -898,7 +898,7 @@ func TestNewMDMAppleDeclaration(t *testing.T) { // Good declaration b = declBytesForTest("D1", "d1content") - d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "name", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) } @@ -962,7 +962,7 @@ func TestNewMDMAppleDeclarationSkipValidation(t *testing.T) { "Type": "com.apple.configuration.management.status-subscriptions", "Identifier": "test-status-sub" }`) - _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-status-sub", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-status-sub", fleet.LabelsIncludeAll, nil) require.Error(t, err) assert.ErrorContains(t, err, "status subscription type") }) @@ -987,7 +987,7 @@ func TestNewMDMAppleDeclarationSkipValidation(t *testing.T) { "Type": "com.apple.configuration.management.status-subscriptions", "Identifier": "test-status-sub" }`) - d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-status-sub", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-status-sub", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) }) @@ -1005,7 +1005,7 @@ func TestNewMDMAppleDeclarationSkipValidation(t *testing.T) { "Type": "com.example.invalid", "Identifier": "test-invalid" }`) - _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-invalid", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-invalid", fleet.LabelsIncludeAll, nil) require.Error(t, err) assert.ErrorContains(t, err, "Only configuration declarations") }) @@ -1030,7 +1030,7 @@ func TestNewMDMAppleDeclarationSkipValidation(t *testing.T) { "Type": "com.example.invalid", "Identifier": "test-invalid" }`) - d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-invalid", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 0, b, nil, "test-invalid", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) }) @@ -1108,7 +1108,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { t.Run("non software-update declaration skips OS update checks", func(t *testing.T) { svc, ctx, ds := setup(t, true) - d, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(otherDecl), nil, "test-passcode", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(otherDecl), nil, "test-passcode", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) assert.False(t, ds.TeamMDMConfigFuncInvoked) @@ -1117,7 +1117,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { t.Run("software-update declaration requires premium license", func(t *testing.T) { svc, ctx, ds := setup(t, false) - _, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll, nil) require.ErrorIs(t, err, fleet.ErrMissingLicense) // The gate fails before the declaration is inserted. assert.False(t, ds.NewMDMAppleDeclarationFuncInvoked) @@ -1128,7 +1128,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { svc, ctx, ds := setup(t, true) ds.AppConfigFunc = appConfigWith(nil) - d, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) assert.False(t, ds.TeamMDMConfigFuncInvoked) @@ -1142,7 +1142,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { return &fleet.TeamMDM{}, nil } - d, err := svc.NewMDMAppleDeclaration(ctx, 5, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll) + d, err := svc.NewMDMAppleDeclaration(ctx, 5, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, d) assert.True(t, ds.TeamMDMConfigFuncInvoked) @@ -1160,7 +1160,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { svc, ctx, ds := setup(t, true) ds.AppConfigFunc = appConfigWith(apply) - _, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 0, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll, nil) require.Error(t, err) require.ErrorContains(t, err, "OS updates are already configured") // The gate fails before the declaration is inserted. @@ -1184,7 +1184,7 @@ func TestNewMDMAppleDeclarationSoftwareUpdate(t *testing.T) { return tc, nil } - _, err := svc.NewMDMAppleDeclaration(ctx, 5, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll) + _, err := svc.NewMDMAppleDeclaration(ctx, 5, []byte(osUpdateDecl), nil, "test-os-update", fleet.LabelsIncludeAll, nil) require.Error(t, err) require.ErrorContains(t, err, fleet.OSUpdatesAlreadyConfiguredErrorMessage) assert.False(t, ds.NewMDMAppleDeclarationFuncInvoked) diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index 29bf1788a0..d1013224c8 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -2187,42 +2187,30 @@ func (s *integrationMDMTestSuite) TestAppConfigMDMCustomSettings() { assert.Empty(t, acResp.MDM.MacOSSettings.CustomSettings) assert.Equal(t, optjson.Slice[fleet.MDMProfileSpec]{Set: true, Value: []fleet.MDMProfileSpec{}}, acResp.MDM.WindowsSettings.CustomSettings) - // mix of labels fields returns an error + // combining two include modes returns an error; combining include + exclude is valid res := s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ "mdm": { "macos_settings": { "custom_settings": [ - {"path": "foo", "labels": ["a"], "labels_exclude_any": ["b"]} + {"path": "foo", "labels_include_all": ["a"], "labels_include_any": ["b"]} ] } } }`), http.StatusUnprocessableEntity) msg := extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ "mdm": { "windows_settings": { "custom_settings": [ - {"path": "foo", "labels_include_all": ["a"], "labels_exclude_any": ["b"]} + {"path": "foo", "labels": ["a"], "labels_include_all": ["b"]} ] } } }`), http.StatusUnprocessableEntity) msg = extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) - - res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ - "mdm": { - "windows_settings": { - "custom_settings": [ - {"path": "foo", "labels_include_any": ["a"], "labels_exclude_any": ["b"]} - ] - } - } - }`), http.StatusUnprocessableEntity) - msg = extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ "mdm": { @@ -2234,7 +2222,44 @@ func (s *integrationMDMTestSuite) TestAppConfigMDMCustomSettings() { } }`), http.StatusUnprocessableEntity) msg = extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) + + res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ + "mdm": { + "windows_settings": { + "custom_settings": [ + {"path": "foo", "labels_include_all": ["a"], "labels_include_any": ["b"]} + ] + } + } + }`), http.StatusUnprocessableEntity) + msg = extractServerErrorText(res.Body) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) + + // same label in both include and exclude lists returns an error + res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ + "mdm": { + "macos_settings": { + "custom_settings": [ + {"path": "foo", "labels_include_all": ["a"], "labels_exclude_any": ["a"]} + ] + } + } + }`), http.StatusUnprocessableEntity) + msg = extractServerErrorText(res.Body) + require.Contains(t, msg, `Label "a" cannot appear in both include and exclude lists.`) + + res = s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ + "mdm": { + "windows_settings": { + "custom_settings": [ + {"path": "foo", "labels_include_any": ["b"], "labels_exclude_any": ["b"]} + ] + } + } + }`), http.StatusUnprocessableEntity) + msg = extractServerErrorText(res.Body) + require.Contains(t, msg, `Label "b" cannot appear in both include and exclude lists.`) } func (s *integrationMDMTestSuite) TestApplyTeamsMDMAppleProfiles() { @@ -2353,7 +2378,7 @@ func (s *integrationMDMTestSuite) TestApplyTeamsMDMAppleProfiles() { }}} res = s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusUnprocessableEntity) errMsg = extractServerErrorText(res.Body) - assert.Contains(t, errMsg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + assert.Contains(t, errMsg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) } func (s *integrationMDMTestSuite) TestBatchSetMDMAppleProfiles() { @@ -3461,15 +3486,21 @@ func (s *integrationMDMTestSuite) TestMDMConfigProfileCRUD() { assertWindowsProfile("win-profile-with-labels.xml", "./Test", 0, []string{"does-not-exist", "bar"}, http.StatusBadRequest, `Couldn't update. Label "does-not-exist" doesn't exist. Please remove the label from the configuration profile.`) assertAndroidProfile("android-profile-with-labels.json", 0, []string{"does-not-exist", "bar"}, http.StatusBadRequest, `Couldn't update. Label "does-not-exist" doesn't exist. Please remove the label from the configuration profile.`) - // profiles with invalid mix of labels - assertAppleProfile("apple-invalid-profile-with-labels.mobileconfig", "apple-invalid-profile-with-labels", "ident-with-labels", 0, []string{"foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertAppleProfile("apple-invalid-profile-with-labels.mobileconfig", "apple-invalid-profile-with-labels", "ident-with-labels", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertAppleDeclaration("apple-invalid-decl-with-labels.json", "ident-decl-with-labels", 0, []string{"foo", "-bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertAppleDeclaration("apple-invalid-decl-with-labels.json", "ident-decl-with-labels", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertWindowsProfile("win-invalid-profile-with-labels.xml", "./Test", 0, []string{"-foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertWindowsProfile("win-invalid-profile-with-labels.xml", "./Test", 0, []string{"-foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertAndroidProfile("android-invalid-profile-with-labels.json", 0, []string{"-foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) - assertAndroidProfile("android-invalid-profile-with-labels.json", 0, []string{"-foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`) + // profiles with invalid mix of labels (two include modes; exclude may combine with either include mode) + assertAppleProfile("apple-invalid-profile-with-labels.mobileconfig", "apple-invalid-profile-with-labels", "ident-with-labels", 0, []string{"foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertAppleProfile("apple-invalid-profile-with-labels.mobileconfig", "apple-invalid-profile-with-labels", "ident-with-labels", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertAppleDeclaration("apple-invalid-decl-with-labels.json", "ident-decl-with-labels", 0, []string{"!foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertAppleDeclaration("apple-invalid-decl-with-labels.json", "ident-decl-with-labels", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertWindowsProfile("win-invalid-profile-with-labels.xml", "./Test", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertWindowsProfile("win-invalid-profile-with-labels.xml", "./Test", 0, []string{"foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertAndroidProfile("android-invalid-profile-with-labels.json", 0, []string{"foo", "~bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + assertAndroidProfile("android-invalid-profile-with-labels.json", 0, []string{"foo", "!bar"}, http.StatusBadRequest, `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`) + + // profiles with same label in both include and exclude lists + assertAppleProfile("apple-invalid-profile-with-labels.mobileconfig", "apple-invalid-profile-with-labels", "ident-with-labels", 0, []string{"foo", "-foo"}, http.StatusBadRequest, `Label "foo" cannot appear in both include and exclude lists.`) + assertAppleDeclaration("apple-invalid-decl-with-labels.json", "ident-decl-with-labels", 0, []string{"~foo", "-foo"}, http.StatusBadRequest, `Label "foo" cannot appear in both include and exclude lists.`) + assertWindowsProfile("win-invalid-profile-with-labels.xml", "./Test", 0, []string{"bar", "-bar"}, http.StatusBadRequest, `Label "bar" cannot appear in both include and exclude lists.`) + assertAndroidProfile("android-invalid-profile-with-labels.json", 0, []string{"~bar", "-bar"}, http.StatusBadRequest, `Label "bar" cannot appear in both include and exclude lists.`) // profiles with valid labels uuidAppleWithLabel := assertAppleProfile("apple-profile-with-labels.mobileconfig", "apple-profile-with-labels", "ident-with-labels", 0, []string{"!foo"}, http.StatusOK, "") @@ -4848,7 +4879,7 @@ func (s *integrationMDMTestSuite) TestApplyTeamsMDMWindowsProfiles() { } `), http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) - assert.Contains(t, errMsg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + assert.Contains(t, errMsg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) } func (s *integrationMDMTestSuite) TestBatchSetMDMProfiles() { @@ -5066,12 +5097,19 @@ func (s *integrationMDMTestSuite) TestBatchSetMDMProfiles() { msg := extractServerErrorText(res.Body) require.Contains(t, msg, `Couldn't update. Label "no-such-label" doesn't exist. Please remove the label from the configuration profile.`) - // mix of labels fields + // two include modes in the same profile is invalid (exclude may combine with either include mode) res = s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ - {Name: "N1", Contents: mobileconfigForTest("N1", "I1"), Labels: []string{lbl1.Name}, LabelsExcludeAny: []string{lbl2.Name}}, + {Name: "N1", Contents: mobileconfigForTest("N1", "I1"), LabelsIncludeAll: []string{lbl1.Name}, LabelsIncludeAny: []string{lbl2.Name}}, }}, http.StatusUnprocessableEntity) msg = extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) + + // same label in both include and exclude is invalid + res = s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ + {Name: "N1", Contents: mobileconfigForTest("N1", "I1"), LabelsIncludeAll: []string{lbl1.Name}, LabelsExcludeAny: []string{lbl1.Name}}, + }}, http.StatusUnprocessableEntity) + msg = extractServerErrorText(res.Body) + require.Contains(t, msg, fmt.Sprintf(`Label %q cannot appear in both include and exclude lists.`, lbl1.Name)) // successful batch-set s.Do("POST", "/api/v1/fleet/mdm/profiles/batch", batchSetMDMProfilesRequest{Profiles: []fleet.MDMProfileBatchPayload{ @@ -5367,12 +5405,12 @@ func (s *integrationMDMTestSuite) TestBatchModifyMDMProfiles() { msg := extractServerErrorText(res.Body) require.Contains(t, msg, `Couldn't update. Label "no-such-label" doesn't exist. Please remove the label from the configuration profile.`) - // mix of labels fields + // two include modes in the same profile is invalid (exclude may combine with either include mode) res = s.Do("POST", "/api/latest/fleet/configuration_profiles/batch", batchModifyMDMConfigProfilesRequest{ConfigurationProfiles: []fleet.BatchModifyMDMConfigProfilePayload{ - {DisplayName: "N1", Profile: mobileconfigForTest("N1", "I1"), LabelsIncludeAll: []string{lbl1.Name}, LabelsExcludeAny: []string{lbl2.Name}}, + {DisplayName: "N1", Profile: mobileconfigForTest("N1", "I1"), LabelsIncludeAll: []string{lbl1.Name}, LabelsIncludeAny: []string{lbl2.Name}}, }}, http.StatusUnprocessableEntity) msg = extractServerErrorText(res.Body) - require.Contains(t, msg, `For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + require.Contains(t, msg, `For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) // successful batch-set s.Do("POST", "/api/latest/fleet/configuration_profiles/batch", batchModifyMDMConfigProfilesRequest{ConfigurationProfiles: []fleet.BatchModifyMDMConfigProfilePayload{ diff --git a/server/service/mdm.go b/server/service/mdm.go index 311d26c46c..84a87bda97 100644 --- a/server/service/mdm.go +++ b/server/service/mdm.go @@ -1679,27 +1679,32 @@ func (newMDMConfigProfileRequest) DecodeRequest(ctx context.Context, r *http.Req } // add labels - var existsInclAll, existsInclAny, existsExclAny, existsDepr bool + var existsInclAll, existsInclAny, existsDepr bool var deprecatedLabels []string decoded.LabelsIncludeAll, existsInclAll = r.MultipartForm.Value[string(fleet.LabelsIncludeAll)] decoded.LabelsIncludeAny, existsInclAny = r.MultipartForm.Value[string(fleet.LabelsIncludeAny)] - decoded.LabelsExcludeAny, existsExclAny = r.MultipartForm.Value[string(fleet.LabelsExcludeAny)] + decoded.LabelsExcludeAny = r.MultipartForm.Value[string(fleet.LabelsExcludeAny)] deprecatedLabels, existsDepr = r.MultipartForm.Value["labels"] - // validate that only one of the labels type is provided - var count int - for _, b := range []bool{existsInclAll, existsInclAny, existsExclAny, existsDepr} { + // validate that at most one include mode is provided; labels_exclude_any may be combined with any include mode + var includeCount int + for _, b := range []bool{existsInclAll, existsInclAny, existsDepr} { if b { - count++ + includeCount++ } } - if count > 1 { - return nil, &fleet.BadRequestError{Message: `Only one of "labels_exclude_any", "labels_include_all", "labels_include_any", or "labels" can be included.`} + if includeCount > 1 { + return nil, &fleet.BadRequestError{Message: `Only one of "labels_include_all", "labels_include_any", or "labels" can be included.`} } if existsDepr { decoded.LabelsIncludeAll = deprecatedLabels } + includeLabels := append(decoded.LabelsIncludeAll, decoded.LabelsIncludeAny...) //nolint:gocritic + if overlap := fleet.ProfileLabelOverlap(includeLabels, decoded.LabelsExcludeAny); overlap != "" { + return nil, &fleet.BadRequestError{Message: fmt.Sprintf(`Label %q cannot appear in both include and exclude lists.`, overlap)} + } + return &decoded, nil } @@ -1729,15 +1734,13 @@ func newMDMConfigProfileEndpoint(ctx context.Context, request interface{}, svc f isMobileConfig := strings.EqualFold(fileExt, ".mobileconfig") isJSON := strings.EqualFold(fileExt, ".json") + // determine include mode; labels_exclude_any may be combined with any include mode var labels []string var labelsMode fleet.MDMLabelsMode switch { case len(req.LabelsIncludeAny) > 0: labels = req.LabelsIncludeAny labelsMode = fleet.LabelsIncludeAny - case len(req.LabelsExcludeAny) > 0: - labels = req.LabelsExcludeAny - labelsMode = fleet.LabelsExcludeAny default: // default include all labels = req.LabelsIncludeAll @@ -1765,7 +1768,7 @@ func newMDMConfigProfileEndpoint(ctx context.Context, request interface{}, svc f if isMobileConfig || isAppleDeclarationJSON { // Then it's an Apple configuration file if isJSON { - decl, err := svc.NewMDMAppleDeclaration(ctx, req.TeamID, data, labels, profileName, labelsMode) + decl, err := svc.NewMDMAppleDeclaration(ctx, req.TeamID, data, labels, profileName, labelsMode, req.LabelsExcludeAny) if err != nil { errStr := err.Error() if strings.Contains(errStr, "MDMAppleDeclaration.Name") && strings.Contains(errStr, "already exists") { @@ -1782,7 +1785,7 @@ func newMDMConfigProfileEndpoint(ctx context.Context, request interface{}, svc f } - cp, err := svc.NewMDMAppleConfigProfile(ctx, req.TeamID, data, labels, labelsMode) + cp, err := svc.NewMDMAppleConfigProfile(ctx, req.TeamID, data, labels, labelsMode, req.LabelsExcludeAny) if err != nil { return &newMDMConfigProfileResponse{Err: err}, nil } @@ -1792,7 +1795,7 @@ func newMDMConfigProfileEndpoint(ctx context.Context, request interface{}, svc f } if isAndroidJSON { - cp, err := svc.NewMDMAndroidConfigProfile(ctx, req.TeamID, profileName, data, labels, labelsMode) + cp, err := svc.NewMDMAndroidConfigProfile(ctx, req.TeamID, profileName, data, labels, labelsMode, req.LabelsExcludeAny) if err != nil { return &newMDMConfigProfileResponse{Err: err}, nil } @@ -1802,7 +1805,7 @@ func newMDMConfigProfileEndpoint(ctx context.Context, request interface{}, svc f } if isWindows := strings.EqualFold(fileExt, ".xml"); isWindows { - cp, err := svc.NewMDMWindowsConfigProfile(ctx, req.TeamID, profileName, data, labels, labelsMode) + cp, err := svc.NewMDMWindowsConfigProfile(ctx, req.TeamID, profileName, data, labels, labelsMode, req.LabelsExcludeAny) if err != nil { return &newMDMConfigProfileResponse{Err: err}, nil } @@ -1837,7 +1840,7 @@ func (svc *Service) NewMDMUnsupportedConfigProfile(ctx context.Context, teamID u return &fleet.BadRequestError{Message: "Couldn't add profile. The file should be a .mobileconfig, XML, or JSON file."} } -func (svc *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMAndroidConfigProfile, error) { +func (svc *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMAndroidConfigProfile, error) { if err := svc.authz.Authorize(ctx, &fleet.MDMConfigProfileAuthz{TeamID: &teamID}, fleet.ActionWrite); err != nil { return nil, ctxerr.Wrap(ctx, err) } @@ -1872,19 +1875,21 @@ func (svc *Service) NewMDMAndroidConfigProfile(ctx context.Context, teamID uint, return nil, ctxerr.Wrap(ctx, err, "validate profile") } - labelMap, err := svc.validateProfileLabels(ctx, &teamID, labels) + if overlap := fleet.ProfileLabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) + } + includeLabels, excludeLabels, err := svc.validateProfileLabelSets(ctx, &teamID, labelsInclude, labelsExcludeAny) if err != nil { return nil, ctxerr.Wrap(ctx, err, "validating labels") } switch labelsMembershipMode { case fleet.LabelsIncludeAny: - cp.LabelsIncludeAny = labelMap - case fleet.LabelsExcludeAny: - cp.LabelsExcludeAny = labelMap + cp.LabelsIncludeAny = includeLabels default: // default include all - cp.LabelsIncludeAll = labelMap + cp.LabelsIncludeAll = includeLabels } + cp.LabelsExcludeAny = excludeLabels newCP, err := svc.ds.NewMDMAndroidConfigProfile(ctx, cp) if err != nil { @@ -1957,17 +1962,40 @@ func (svc *Service) batchValidateProfileLabels(ctx context.Context, teamID *uint return profLabels, nil } -func (svc *Service) validateProfileLabels(ctx context.Context, teamID *uint, labelNames []string) ([]fleet.ConfigurationProfileLabel, error) { - labelMap, err := svc.batchValidateProfileLabels(ctx, teamID, labelNames) +// validateDeclarationLabelSets validates both include and exclude label sets in a single DB round-trip +// and returns the resolved slices ready to assign to the declaration struct. +func (svc *Service) validateDeclarationLabelSets(ctx context.Context, teamID uint, labelsInclude, labelsExcludeAny []string) (include, exclude []fleet.ConfigurationProfileLabel, err error) { + allLabelMap, err := svc.batchValidateDeclarationLabels(ctx, slices.Concat(labelsInclude, labelsExcludeAny), teamID) if err != nil { - return nil, ctxerr.Wrap(ctx, err, "validating profile labels") + return nil, nil, err } + include = make([]fleet.ConfigurationProfileLabel, 0, len(labelsInclude)) + for _, name := range labelsInclude { + include = append(include, allLabelMap[name]) + } + exclude = make([]fleet.ConfigurationProfileLabel, 0, len(labelsExcludeAny)) + for _, name := range labelsExcludeAny { + exclude = append(exclude, allLabelMap[name]) + } + return include, exclude, nil +} - var profLabels []fleet.ConfigurationProfileLabel - for _, label := range labelMap { - profLabels = append(profLabels, label) +// validateProfileLabelSets validates both include and exclude label sets in a single DB round-trip +// and returns the resolved slices ready to assign to the profile struct. +func (svc *Service) validateProfileLabelSets(ctx context.Context, teamID *uint, labelsInclude, labelsExcludeAny []string) (include, exclude []fleet.ConfigurationProfileLabel, err error) { + allLabelMap, err := svc.batchValidateProfileLabels(ctx, teamID, slices.Concat(labelsInclude, labelsExcludeAny)) + if err != nil { + return nil, nil, ctxerr.Wrap(ctx, err, "validating labels") } - return profLabels, nil + include = make([]fleet.ConfigurationProfileLabel, 0, len(labelsInclude)) + for _, name := range labelsInclude { + include = append(include, allLabelMap[name]) + } + exclude = make([]fleet.ConfigurationProfileLabel, 0, len(labelsExcludeAny)) + for _, name := range labelsExcludeAny { + exclude = append(exclude, allLabelMap[name]) + } + return include, exclude, nil } type batchModifyMDMConfigProfilesRequest struct { @@ -2806,20 +2834,24 @@ func getAndroidProfiles(ctx context.Context, func validateProfiles(profiles map[int]fleet.MDMProfileBatchPayload) error { for _, profile := range profiles { - // validate that only one of labels, labels_include_all and labels_exclude_any is provided. - var count int + // validate that at most one include mode is provided; labels_exclude_any may be combined with any include mode + var includeCount int for _, b := range []bool{ len(profile.LabelsIncludeAll) > 0, len(profile.LabelsIncludeAny) > 0, - len(profile.LabelsExcludeAny) > 0, len(profile.Labels) > 0, } { if b { - count++ + includeCount++ } } - if count > 1 { - return fleet.NewInvalidArgumentError("mdm", `Couldn't edit configuration_profiles. For each profile, only one of "labels_exclude_any", "labels_include_all", "labels_include_any" or "labels" can be included.`) + if includeCount > 1 { + return fleet.NewInvalidArgumentError("mdm", `Couldn't edit configuration_profiles. For each profile, only one of "labels_include_all", "labels_include_any" or "labels" can be included.`) + } + + includeLabels := append(profile.LabelsIncludeAll, append(profile.Labels, profile.LabelsIncludeAny...)...) //nolint:gocritic + if overlap := fleet.ProfileLabelOverlap(includeLabels, profile.LabelsExcludeAny); overlap != "" { + return fleet.NewInvalidArgumentError("mdm", fmt.Sprintf(`Couldn't edit configuration_profiles. Label %q cannot appear in both include and exclude lists.`, overlap)) } if len(profile.Contents) > 1024*1024 { diff --git a/server/service/mdm_test.go b/server/service/mdm_test.go index bc53b97176..2d9e4c268c 100644 --- a/server/service/mdm_test.go +++ b/server/service/mdm_test.go @@ -1386,11 +1386,11 @@ func TestMDMWindowsConfigProfileAuthz(t *testing.T) { checkShouldFail(t, err, tt.shouldFailTeamRead) // test authz create new profile (no team) - _, err = svc.NewMDMWindowsConfigProfile(ctx, 0, "prof", []byte(winProfContent), nil, fleet.LabelsIncludeAll) + _, err = svc.NewMDMWindowsConfigProfile(ctx, 0, "prof", []byte(winProfContent), nil, fleet.LabelsIncludeAll, nil) checkShouldFail(t, err, tt.shouldFailGlobalWrite) // test authz create new profile (team 1) - _, err = svc.NewMDMWindowsConfigProfile(ctx, 1, "prof", []byte(winProfContent), nil, fleet.LabelsIncludeAll) + _, err = svc.NewMDMWindowsConfigProfile(ctx, 1, "prof", []byte(winProfContent), nil, fleet.LabelsIncludeAll, nil) checkShouldFail(t, err, tt.shouldFailTeamWrite) // test authz delete config profile (no team) @@ -1486,7 +1486,7 @@ func TestUploadWindowsMDMConfigProfileValidations(t *testing.T) { }, nil } ctx = test.UserContext(ctx, test.UserAdmin) - _, err := svc.NewMDMWindowsConfigProfile(ctx, c.tmID, "foo", []byte(c.profile), nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMWindowsConfigProfile(ctx, c.tmID, "foo", []byte(c.profile), nil, fleet.LabelsIncludeAll, nil) if c.wantErr != "" { require.Error(t, err) require.ErrorContains(t, err, c.wantErr) @@ -3113,7 +3113,7 @@ func TestNewMDMProfilePremiumOnlyAndroid(t *testing.T) { } ctx = license.NewContext(ctx, &fleet.LicenseInfo{Tier: tier}) - _, err := svc.NewMDMAndroidConfigProfile(ctx, tt.teamID, tt.name, []byte(tt.profile), nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMAndroidConfigProfile(ctx, tt.teamID, tt.name, []byte(tt.profile), nil, fleet.LabelsIncludeAll, nil) if tt.wantErr == "" { require.NoError(t, err) require.True(t, ds.NewMDMAndroidConfigProfileFuncInvoked) diff --git a/server/service/testing_client_test.go b/server/service/testing_client_test.go index d9d52ee7c5..2c746c20ad 100644 --- a/server/service/testing_client_test.go +++ b/server/service/testing_client_test.go @@ -180,6 +180,16 @@ func (ts *withServer) commonTearDownTest(t *testing.T) { return nil }) + // Null label_id references in MDM profile/declaration label tables before deleting labels, + // since the application layer now blocks label deletion while referenced by a profile. + mysqltest.ExecAdhocSQL(t, ts.ds, func(q sqlx.ExtContext) error { + if _, err := q.ExecContext(ctx, `UPDATE mdm_configuration_profile_labels SET label_id = NULL`); err != nil { + return err + } + _, err := q.ExecContext(ctx, `UPDATE mdm_declaration_labels SET label_id = NULL`) + return err + }) + lbls, err := ts.ds.ListLabels(ctx, filter, fleet.ListOptions{}, false) require.NoError(t, err) for _, lbl := range lbls { diff --git a/server/service/windows_mdm_profiles.go b/server/service/windows_mdm_profiles.go index e2b362a5c3..3b9873a2d9 100644 --- a/server/service/windows_mdm_profiles.go +++ b/server/service/windows_mdm_profiles.go @@ -20,7 +20,7 @@ import ( "github.com/fleetdm/fleet/v4/server/variables" ) -func (svc *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labels []string, labelsMembershipMode fleet.MDMLabelsMode) (*fleet.MDMWindowsConfigProfile, error) { +func (svc *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, profileName string, data []byte, labelsInclude []string, labelsMembershipMode fleet.MDMLabelsMode, labelsExcludeAny []string) (*fleet.MDMWindowsConfigProfile, error) { if err := svc.authz.Authorize(ctx, &fleet.MDMConfigProfileAuthz{TeamID: &teamID}, fleet.ActionWrite); err != nil { return nil, ctxerr.Wrap(ctx, err) } @@ -66,19 +66,21 @@ func (svc *Service) NewMDMWindowsConfigProfile(ctx context.Context, teamID uint, return nil, ctxerr.Wrap(ctx, err, "validate profile") } - labelMap, err := svc.validateProfileLabels(ctx, &teamID, labels) + if overlap := fleet.ProfileLabelOverlap(labelsInclude, labelsExcludeAny); overlap != "" { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("labels", fmt.Sprintf("label %q cannot appear in both include and exclude lists", overlap))) + } + includeLabels, excludeLabels, err := svc.validateProfileLabelSets(ctx, &teamID, labelsInclude, labelsExcludeAny) if err != nil { return nil, ctxerr.Wrap(ctx, err, "validating labels") } switch labelsMembershipMode { case fleet.LabelsIncludeAny: - cp.LabelsIncludeAny = labelMap - case fleet.LabelsExcludeAny: - cp.LabelsExcludeAny = labelMap + cp.LabelsIncludeAny = includeLabels default: // default include all - cp.LabelsIncludeAll = labelMap + cp.LabelsIncludeAll = includeLabels } + cp.LabelsExcludeAny = excludeLabels if err := svc.ds.ValidateEmbeddedSecrets(ctx, []string{string(cp.SyncML)}); err != nil { return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("profile", err.Error())) diff --git a/server/service/windows_mdm_profiles_test.go b/server/service/windows_mdm_profiles_test.go index 2ad245e6b9..e96b76b16c 100644 --- a/server/service/windows_mdm_profiles_test.go +++ b/server/service/windows_mdm_profiles_test.go @@ -378,7 +378,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { svc, ctx, ds := setup(t, true) ds.AppConfigFunc = appConfigWith(nil) - p, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "other", otherSyncML, nil, fleet.LabelsIncludeAll) + p, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "other", otherSyncML, nil, fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, p) assert.False(t, ds.TeamMDMConfigFuncInvoked) @@ -388,7 +388,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { svc, ctx, ds := setup(t, false) ds.AppConfigFunc = appConfigWith(nil) - _, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "other", osUpdateSyncML, nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "other", osUpdateSyncML, nil, fleet.LabelsIncludeAll, nil) require.ErrorIs(t, err, fleet.ErrMissingLicense) // The gate fails before the profile is inserted. assert.False(t, ds.NewMDMWindowsConfigProfileFuncInvoked) @@ -399,7 +399,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { svc, ctx, ds := setup(t, true) ds.AppConfigFunc = appConfigWith(nil) - p, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll) + p, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, p) assert.False(t, ds.TeamMDMConfigFuncInvoked) @@ -414,7 +414,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { return &fleet.TeamMDM{}, nil } - p, err := svc.NewMDMWindowsConfigProfile(ctx, 5, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll) + p, err := svc.NewMDMWindowsConfigProfile(ctx, 5, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll, nil) require.NoError(t, err) assert.NotNil(t, p) assert.True(t, ds.TeamMDMConfigFuncInvoked) @@ -427,7 +427,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { ac.MDM.WindowsUpdates = configuredSettings() }) - _, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMWindowsConfigProfile(ctx, 0, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll, nil) require.Error(t, err) require.ErrorContains(t, err, fleet.OSUpdatesAlreadyConfiguredErrorMessage) // The gate fails before the profile is inserted. @@ -441,7 +441,7 @@ func TestNewMDMWindowsConfigProfileSoftwareUpdate(t *testing.T) { return &fleet.TeamMDM{WindowsUpdates: configuredSettings()}, nil } - _, err := svc.NewMDMWindowsConfigProfile(ctx, 5, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll) + _, err := svc.NewMDMWindowsConfigProfile(ctx, 5, "os-update", osUpdateSyncML, nil, fleet.LabelsIncludeAll, nil) require.Error(t, err) require.ErrorContains(t, err, fleet.OSUpdatesAlreadyConfiguredErrorMessage) assert.False(t, ds.NewMDMWindowsConfigProfileFuncInvoked)