Improve error messages for parsing MDM config profiles via fleetctl (#19495)

changes/17316-parse-config-profile-error

@@ -0,0 +1 @@
+- Fixed issue where Windows-specific error message was displayed when failing to parse macOS configuration profiles.

server/service/client.go

@@ -303,45 +303,84 @@ func (c *Client) runAppConfigChecks(fn func(ac *fleet.EnrichedAppConfig) error)
 
 // getProfilesContents takes file paths and creates a slice of profile payloads
 // ready to batch-apply.
-func getProfilesContents(baseDir string, profiles []fleet.MDMProfileSpec, expandEnv bool) ([]fleet.MDMProfileBatchPayload, error) {
-	// map to check for duplicate names
-	extByName := make(map[string]string, len(profiles))
-	result := make([]fleet.MDMProfileBatchPayload, 0, len(profiles))
+func getProfilesContents(baseDir string, macProfiles []fleet.MDMProfileSpec, windowsProfiles []fleet.MDMProfileSpec, expandEnv bool) ([]fleet.MDMProfileBatchPayload, error) {
+	// map to check for duplicate names across all profiles
+	extByName := make(map[string]string, len(macProfiles))
+	result := make([]fleet.MDMProfileBatchPayload, 0, len(macProfiles))
 
-	for _, profile := range profiles {
-		filePath := resolveApplyRelativePath(baseDir, profile.Path)
-		fileContents, err := os.ReadFile(filePath)
-		if err != nil {
-			return nil, fmt.Errorf("applying fleet config: %w", err)
-		}
-
-		if expandEnv {
-			fileContents, err = spec.ExpandEnvBytes(fileContents)
+	// iterate over the profiles for each platform
+	for platform, profiles := range map[string][]fleet.MDMProfileSpec{
+		"macos":   macProfiles,
+		"windows": windowsProfiles,
+	} {
+		for _, profile := range profiles {
+			filePath := resolveApplyRelativePath(baseDir, profile.Path)
+			fileContents, err := os.ReadFile(filePath)
 			if err != nil {
-				return nil, fmt.Errorf("expanding environment on file %q: %w", profile.Path, err)
+				return nil, fmt.Errorf("applying custom settings: %w", err)
 			}
-		}
 
-		// by default, use the file name. macOS profiles use their PayloadDisplayName
-		ext := filepath.Ext(filePath)
-		name := strings.TrimSuffix(filepath.Base(filePath), ext)
-		if mdm.GetRawProfilePlatform(fileContents) == "darwin" && ext == ".mobileconfig" {
-			mc, err := fleet.NewMDMAppleConfigProfile(fileContents, nil)
-			if err != nil {
-				return nil, fmt.Errorf("applying fleet config: %w", err)
+			if expandEnv {
+				fileContents, err = spec.ExpandEnvBytes(fileContents)
+				if err != nil {
+					return nil, fmt.Errorf("expanding environment on file %q: %w", profile.Path, err)
+				}
 			}
-			name = strings.TrimSpace(mc.Name)
-		}
-		if e, isDuplicate := extByName[name]; isDuplicate {
-			return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext))
-		}
-		extByName[name] = ext
-		result = append(result, fleet.MDMProfileBatchPayload{
-			Name:     name,
-			Contents: fileContents,
-			Labels:   profile.Labels,
-		})
 
+			ext := filepath.Ext(filePath)
+			// by default, use the file name (for macOS mobileconfig profiles, we'll switch to
+			// their PayloadDisplayName when we parse the profile below)
+			name := strings.TrimSuffix(filepath.Base(filePath), ext)
+			// for validation errors, we want to include the platform and file name in the error message
+			prefixErrMsg := fmt.Sprintf("Couldn't edit %s_settings.custom_settings (%s%s)", platform, name, ext)
+
+			// validate macOS profiles
+			if platform == "macos" {
+				switch ext {
+				case ".mobileconfig", ".xml": // allowing .xml for backwards compatibility
+					mc, err := fleet.NewMDMAppleConfigProfile(fileContents, nil)
+					if err != nil {
+						errForMsg := errors.Unwrap(err)
+						if errForMsg == nil {
+							errForMsg = err
+						}
+						return nil, fmt.Errorf("%s: %w", prefixErrMsg, errForMsg)
+					}
+					name = strings.TrimSpace(mc.Name)
+				case ".json":
+					if mdm.GetRawProfilePlatform(fileContents) != "darwin" {
+						return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Declaration profiles should include valid JSON.")
+					}
+				default:
+					return nil, fmt.Errorf("%s: %s", prefixErrMsg, "macOS configuration profiles must be .mobileconfig or .json files.")
+				}
+			}
+
+			// validate windows profiles
+			if platform == "windows" {
+				switch ext {
+				case ".xml":
+					if mdm.GetRawProfilePlatform(fileContents) != "windows" {
+						return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Windows configuration profiles can only have <Replace> or <Add> top level elements")
+					}
+				default:
+					return nil, fmt.Errorf("%s: %s", prefixErrMsg, "Windows configuration profiles must be .xml files.")
+				}
+			}
+
+			// check for duplicate names across all profiles
+			if e, isDuplicate := extByName[name]; isDuplicate {
+				return nil, errors.New(fmtDuplicateNameErrMsg(name, e, ext))
+			}
+			extByName[name] = ext
+
+			result = append(result, fleet.MDMProfileBatchPayload{
+				Name:     name,
+				Contents: fileContents,
+				Labels:   profile.Labels,
+			})
+
+		}
 	}
 	return result, nil
 }
@@ -421,7 +460,6 @@ func (c *Client) ApplyGroup(
 	if specs.AppConfig != nil {
 		windowsCustomSettings := extractAppCfgWindowsCustomSettings(specs.AppConfig)
 		macosCustomSettings := extractAppCfgMacOSCustomSettings(specs.AppConfig)
-		allCustomSettings := append(macosCustomSettings, windowsCustomSettings...)
 
 		// if there is no custom setting but the windows and mac settings are
 		// non-nil, this means that we want to clear the existing custom settings,
@@ -430,8 +468,8 @@ func (c *Client) ApplyGroup(
 		// TODO(mna): shouldn't that be an || instead of && ? I.e. if there are no
 		// custom settings but windows is present and empty (but mac is absent),
 		// shouldn't that clear the windows ones?
-		if (windowsCustomSettings != nil && macosCustomSettings != nil) || len(allCustomSettings) > 0 {
-			fileContents, err := getProfilesContents(baseDir, allCustomSettings, opts.ExpandEnvConfigProfiles)
+		if (windowsCustomSettings != nil && macosCustomSettings != nil) || len(windowsCustomSettings)+len(macosCustomSettings) > 0 {
+			fileContents, err := getProfilesContents(baseDir, macosCustomSettings, windowsCustomSettings, opts.ExpandEnvConfigProfiles)
 			if err != nil {
 				return nil, err
 			}
@@ -520,10 +558,10 @@ func (c *Client) ApplyGroup(
 		tmMDMSettings := extractTmSpecsMDMCustomSettings(specs.Teams)
 
 		tmFileContents := make(map[string][]fleet.MDMProfileBatchPayload, len(tmMDMSettings))
-		for k, paths := range tmMDMSettings {
-			fileContents, err := getProfilesContents(baseDir, paths, opts.ExpandEnvConfigProfiles)
+		for k, profileSpecs := range tmMDMSettings {
+			fileContents, err := getProfilesContents(baseDir, profileSpecs.macos, profileSpecs.windows, opts.ExpandEnvConfigProfiles)
 			if err != nil {
-				return nil, err
+				return nil, fmt.Errorf("Team %s: %w", k, err) // TODO: consider adding team name to improve error messages generally for other parts of the config because multiple team configs can be processed at once
 			}
 			tmFileContents[k] = fileContents
 		}
@@ -837,9 +875,14 @@ func extractAppCfgScripts(appCfg interface{}) []string {
 	return scriptsStrings
 }
 
+type profileSpecsByPlatform struct {
+	macos   []fleet.MDMProfileSpec
+	windows []fleet.MDMProfileSpec
+}
+
 // returns the custom macOS and Windows settings keyed by team name.
-func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fleet.MDMProfileSpec {
-	var m map[string][]fleet.MDMProfileSpec
+func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string]profileSpecsByPlatform {
+	var m map[string]profileSpecsByPlatform
 	for _, tm := range tmSpecs {
 		var spec struct {
 			Name string `json:"name"`
@@ -866,7 +909,7 @@ func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fle
 			if len(spec.MDM.MacOSSettings.CustomSettings) > 0 ||
 				len(spec.MDM.WindowsSettings.CustomSettings) > 0 {
 				if m == nil {
-					m = make(map[string][]fleet.MDMProfileSpec)
+					m = make(map[string]profileSpecsByPlatform)
 				}
 			}
 
@@ -894,8 +937,16 @@ func extractTmSpecsMDMCustomSettings(tmSpecs []json.RawMessage) map[string][]fle
 			}
 
 			// TODO: validate equal names here and API?
+			var result profileSpecsByPlatform
+			if macOSSettings != nil {
+				result.macos = macOSSettings
+			}
+			if windowsSettings != nil {
+				result.windows = windowsSettings
+			}
+
 			if macOSSettings != nil || windowsSettings != nil {
-				m[spec.Name] = append(macOSSettings, windowsSettings...)
+				m[spec.Name] = result
 			}
 		}
 	}

server/service/client_test.go

@@ -282,7 +282,7 @@ func TestExtractTeamSpecsMDMCustomSettings(t *testing.T) {
 	cases := []struct {
 		desc string
 		yaml string
-		want map[string][]fleet.MDMProfileSpec
+		want map[string]profileSpecsByPlatform
 	}{
 		{
 			"no settings",
@@ -342,7 +342,7 @@ spec:
       windows_settings:
         custom_settings:
 `,
-			map[string][]fleet.MDMProfileSpec{"Fleet": {}, "Fleet2": {}},
+			map[string]profileSpecsByPlatform{"Fleet": {windows: []fleet.MDMProfileSpec{}, macos: []fleet.MDMProfileSpec{}}, "Fleet2": {windows: []fleet.MDMProfileSpec{}, macos: []fleet.MDMProfileSpec{}}},
 		},
 		{
 			"custom settings specified",
@@ -368,11 +368,15 @@ spec:
                - "foo"
                - baz
 `,
-			map[string][]fleet.MDMProfileSpec{"Fleet": {
-				{Path: "a", Labels: []string{"foo", "bar"}},
-				{Path: "b"},
-				{Path: "c"},
-				{Path: "d", Labels: []string{"foo", "baz"}},
+			map[string]profileSpecsByPlatform{"Fleet": {
+				macos: []fleet.MDMProfileSpec{
+					{Path: "a", Labels: []string{"foo", "bar"}},
+					{Path: "b"},
+				},
+				windows: []fleet.MDMProfileSpec{
+					{Path: "c"},
+					{Path: "d", Labels: []string{"foo", "baz"}},
+				},
 			}},
 		},
 		{
@@ -393,7 +397,13 @@ spec:
           - "c"
           - "d"
 `,
-			map[string][]fleet.MDMProfileSpec{"Fleet": {{Path: "a"}, {Path: "b"}, {Path: "c"}, {Path: "d"}}},
+			map[string]profileSpecsByPlatform{"Fleet": {
+				macos: []fleet.MDMProfileSpec{{Path: "a"}, {Path: "b"}},
+				windows: []fleet.MDMProfileSpec{
+					{Path: "c"},
+					{Path: "d"},
+				},
+			}},
 		},
 		{
 			"invalid custom settings",
@@ -423,7 +433,7 @@ spec:
           - path: 24
           - path: "y"
 `,
-			map[string][]fleet.MDMProfileSpec{},
+			map[string]profileSpecsByPlatform{},
 		},
 		{
 			"old invalid custom settings",
@@ -447,7 +457,7 @@ spec:
           - 24
           - "y"
 `,
-			map[string][]fleet.MDMProfileSpec{},
+			map[string]profileSpecsByPlatform{},
 		},
 	}
 	for _, c := range cases {
@@ -455,8 +465,13 @@ spec:
 			specs, err := spec.GroupFromBytes([]byte(c.yaml))
 			require.NoError(t, err)
 			if len(specs.Teams) > 0 {
-				got := extractTmSpecsMDMCustomSettings(specs.Teams)
-				require.Equal(t, c.want, got)
+				gotSpecs := extractTmSpecsMDMCustomSettings(specs.Teams)
+				for k, wantProfs := range c.want {
+					gotProfs, ok := gotSpecs[k]
+					require.True(t, ok)
+					require.Equal(t, wantProfs.macos, gotProfs.macos)
+					require.Equal(t, wantProfs.windows, gotProfs.windows)
+				}
 			}
 		})
 	}
@@ -502,19 +517,21 @@ func TestGetProfilesContents(t *testing.T) {
 </Replace>`
 
 	tests := []struct {
-		name        string
-		baseDir     string
-		setupFiles  [][2]string
-		labels      []string
-		environment map[string]string
-		expandEnv   bool
-		expectError bool
-		want        []fleet.MDMProfileBatchPayload
+		name          string
+		baseDir       string
+		macSetupFiles [][2]string
+		winSetupFiles [][2]string
+		labels        []string
+		environment   map[string]string
+		expandEnv     bool
+		expectError   bool
+		want          []fleet.MDMProfileBatchPayload
+		wantErr       string
 	}{
 		{
 			name:    "invalid darwin xml",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
+			macSetupFiles: [][2]string{
 				{"foo.mobileconfig", `<?xml version="1.0" encoding="UTF-8"?>`},
 			},
 			expectError: true,
@@ -523,10 +540,12 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "windows and darwin files",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
-				{"foo.xml", string(windowsProfile)},
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", string(darwinProfile)},
 			},
+			winSetupFiles: [][2]string{
+				{"foo.xml", string(windowsProfile)},
+			},
 			expectError: false,
 			want: []fleet.MDMProfileBatchPayload{
 				{Name: "foo", Contents: windowsProfile},
@@ -536,10 +555,12 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "windows and darwin files with labels",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
-				{"foo.xml", string(windowsProfile)},
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", string(darwinProfile)},
 			},
+			winSetupFiles: [][2]string{
+				{"foo.xml", string(windowsProfile)},
+			},
 			labels:      []string{"foo", "bar"},
 			expectError: false,
 			want: []fleet.MDMProfileBatchPayload{
@@ -550,10 +571,12 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "darwin files with file name != PayloadDisplayName",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
-				{"foo.xml", string(windowsProfile)},
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", string(darwinProfile)},
 			},
+			winSetupFiles: [][2]string{
+				{"foo.xml", string(windowsProfile)},
+			},
 			expectError: false,
 			want: []fleet.MDMProfileBatchPayload{
 				{Name: "foo", Contents: windowsProfile},
@@ -563,16 +586,18 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "duplicate names across windows and darwin",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
-				{"baz.xml", string(windowsProfile)},
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", string(mobileconfigForTest("baz", "I"))},
 			},
+			winSetupFiles: [][2]string{
+				{"baz.xml", string(windowsProfile)},
+			},
 			expectError: true,
 		},
 		{
 			name:    "duplicate file names",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
+			winSetupFiles: [][2]string{
 				{"baz.xml", string(windowsProfile)},
 				{"baz.xml", string(windowsProfile)},
 			},
@@ -581,8 +606,10 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "with environment variables",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", darwinProfileWithFooEnv},
+			},
+			winSetupFiles: [][2]string{
 				{"foo.xml", windowsProfileWithBarEnv},
 			},
 			environment: map[string]string{"FOO": "42", "BAR": "24"},
@@ -634,14 +661,43 @@ func TestGetProfilesContents(t *testing.T) {
 		{
 			name:    "with environment variables but not set",
 			baseDir: tempDir,
-			setupFiles: [][2]string{
+			macSetupFiles: [][2]string{
 				{"bar.mobileconfig", darwinProfileWithFooEnv},
+			},
+			winSetupFiles: [][2]string{
 				{"foo.xml", windowsProfileWithBarEnv},
 			},
 			environment: map[string]string{},
 			expandEnv:   true,
 			expectError: true,
 		},
+		{
+			name:    "with unprocessable json",
+			baseDir: tempDir,
+			macSetupFiles: [][2]string{
+				{"bar.json", string(windowsProfile)},
+			},
+			expectError: true,
+			wantErr:     "Couldn't edit macos_settings.custom_settings (bar.json): Declaration profiles should include valid JSON",
+		},
+		{
+			name:    "with unprocessable xml",
+			baseDir: tempDir,
+			winSetupFiles: [][2]string{
+				{"bar.xml", string(darwinProfile)},
+			},
+			expectError: true,
+			wantErr:     "Couldn't edit windows_settings.custom_settings (bar.xml): Windows configuration profiles can only have <Replace> or <Add> top level elements",
+		},
+		{
+			name:    "with unsupported extension",
+			baseDir: tempDir,
+			macSetupFiles: [][2]string{
+				{"bar.cfg", string(darwinProfile)},
+			},
+			expectError: true,
+			wantErr:     "Couldn't edit macos_settings.custom_settings (bar.cfg): macOS configuration profiles must be .mobileconfig or .json files",
+		},
 	}
 
 	for _, tt := range tests {
@@ -658,17 +714,27 @@ func TestGetProfilesContents(t *testing.T) {
 					})
 				}
 			}
-			paths := []fleet.MDMProfileSpec{}
-			for _, fileSpec := range tt.setupFiles {
+			macPaths := []fleet.MDMProfileSpec{}
+			for _, fileSpec := range tt.macSetupFiles {
 				filePath := filepath.Join(tempDir, fileSpec[0])
 				require.NoError(t, os.WriteFile(filePath, []byte(fileSpec[1]), 0o644))
-				paths = append(paths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels})
+				macPaths = append(macPaths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels})
 			}
 
-			profileContents, err := getProfilesContents(tt.baseDir, paths, tt.expandEnv)
+			winPaths := []fleet.MDMProfileSpec{}
+			for _, fileSpec := range tt.winSetupFiles {
+				filePath := filepath.Join(tempDir, fileSpec[0])
+				require.NoError(t, os.WriteFile(filePath, []byte(fileSpec[1]), 0o644))
+				winPaths = append(winPaths, fleet.MDMProfileSpec{Path: filePath, Labels: tt.labels})
+			}
+
+			profileContents, err := getProfilesContents(tt.baseDir, macPaths, winPaths, tt.expandEnv)
 
 			if tt.expectError {
 				require.Error(t, err)
+				if tt.wantErr != "" {
+					require.Contains(t, err.Error(), tt.wantErr)
+				}
 			} else {
 				require.NoError(t, err)
 				require.NotNil(t, profileContents)

server/service/mdm.go

@@ -1869,8 +1869,17 @@ func validateProfiles(profiles []fleet.MDMProfileBatchPayload) error {
 	for _, profile := range profiles {
 		platform := mdm.GetRawProfilePlatform(profile.Contents)
 		if platform != "darwin" && platform != "windows" {
-			// TODO(roberto): there's ongoing feedback with Marko about improving this message, as it's too windows specific
-			return fleet.NewInvalidArgumentError("mdm", "Windows configuration profiles can only have <Replace> or <Add> top level elements.")
+			// We can only display a generic error message here because at this point
+			// we don't know the file extension or whether the profile is intended
+			// for macos_settings or windows_settings. We should expecte never see this
+			// in practice because the client should be validating the profiles
+			// before sending them to the server so the client can surface  more helpful
+			// error messages to the user. However, we're validating again here just
+			// in case the client is not working as expected.
+			return fleet.NewInvalidArgumentError("mdm", fmt.Sprintf(
+				"%s is not a valid macOS or Windows configuration profile. ", profile.Name)+
+				"macOS profiles must be valid .mobileconfig or .json files. "+
+				"Windows configuration profiles can only have <Replace> or <Add> top level elements.")
 		}
 	}