From 1d7aab04ab00e6272a07fed3fef988e5eef7b2cb Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Fri, 22 Aug 2025 09:37:12 -0500 Subject: [PATCH] Fix GitOps dry run issue with validating profiles with secrets (#32104) Fixes #31477 Docs PR: https://github.com/fleetdm/fleet/pull/32116 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit - New Features - GitOps now supports FLEET_SECRET_ placeholders in macOS (.mobileconfig/.xml) profiles. Secrets are expanded only for validation, while remaining unexpanded in uploaded content. - Improved environment variable handling: non-secret vars expand as before; server-side secrets are preserved. - Validation enforces that profile display names cannot contain FLEET_SECRET_ values. - Bug Fixes - Resolves validation issues when FLEET_SECRET_ appears in tags by performing safe client-side expansion for validation. - More accurate error reporting during profile parsing and validation. --------- Co-authored-by: Lucas Manuel Rodriguez --- changes/31477-secrets-in-macos-profiles | 2 + .../lib/macos-password-secret.mobileconfig | 4 +- .../gitops_enterprise_integration_test.go | 237 ++++++++++++++++++ pkg/spec/spec.go | 49 +++- pkg/spec/spec_test.go | 30 +++ server/fleet/apple_mdm.go | 20 ++ server/fleet/apple_mdm_test.go | 97 +++++++ server/service/apple_mdm.go | 10 + server/service/apple_mdm_test.go | 23 ++ server/service/client.go | 45 +++- server/service/client_test.go | 159 ++++++++++++ .../service/integration_mdm_profiles_test.go | 59 +++-- 12 files changed, 702 insertions(+), 33 deletions(-) create mode 100644 changes/31477-secrets-in-macos-profiles diff --git a/changes/31477-secrets-in-macos-profiles b/changes/31477-secrets-in-macos-profiles new file mode 100644 index 0000000000..c4a344d3d1 --- /dev/null +++ b/changes/31477-secrets-in-macos-profiles @@ -0,0 +1,2 @@ +- Fixed `fleetctl gitops` issue uploading an Apple configuration profile with a FLEET_SECRET in a `` field. +- Added a check to disallow FLEET_SECRET variables in Apple configuration profile `` fields for security. diff --git a/cmd/fleetctl/fleetctl/testdata/gitops/lib/macos-password-secret.mobileconfig b/cmd/fleetctl/fleetctl/testdata/gitops/lib/macos-password-secret.mobileconfig index 03b34b6728..ad4f5d2c72 100644 --- a/cmd/fleetctl/fleetctl/testdata/gitops/lib/macos-password-secret.mobileconfig +++ b/cmd/fleetctl/fleetctl/testdata/gitops/lib/macos-password-secret.mobileconfig @@ -6,9 +6,9 @@ PayloadDescription - Configures Passcode settings + Configures Passcode settings - $FLEET_SECRET_NAME PayloadDisplayName - $FLEET_SECRET_NAME + Passcode Policy PayloadIdentifier com.github.erikberglund.ProfileCreator.F7CF282E-D91B-44E9-922F-A719634F9C8E.com.apple.mobiledevice.passwordpolicy.231DFC90-D5A7-41B8-9246-564056048AC5 PayloadOrganization diff --git a/cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go b/cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go index cc99d710a1..2e5f7e9f51 100644 --- a/cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go +++ b/cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go @@ -1270,3 +1270,240 @@ queries: require.Equal(t, secretVariables[0].Name, secretName) require.Equal(t, secretVariables[0].Value, secretValue) } + +// TestEnvSubstitutionInProfiles tests that only FLEET_SECRET_ prefixed env vars are saved as secrets +func (s *enterpriseIntegrationGitopsTestSuite) TestEnvSubstitutionInProfiles() { + t := s.T() + ctx := t.Context() + tempDir := t.TempDir() + + // Create a test configuration profile with both valid and invalid secret references + profileContent := ` + + + + PayloadContent + + + PayloadDisplayName + Test Profile + PayloadIdentifier + com.fleet.test.env + PayloadType + Configuration + PayloadUUID + 12345678-1234-1234-1234-123456789012 + PayloadVersion + 1 + TestSecretValue + $FLEET_SECRET_TEST_SECRET + TestInvalidSecret + $FLEET_DUO_CERTIFICATE_SECRET + TestPlainValue + $HOME + + + PayloadDisplayName + Test Profile + PayloadIdentifier + com.fleet.test.env + PayloadType + Configuration + PayloadUUID + 12345678-1234-1234-1234-123456789012 + PayloadVersion + 1 + +` + + // Write the profile to a file + profilePath := filepath.Join(tempDir, "test-profile.mobileconfig") + err := os.WriteFile(profilePath, []byte(profileContent), 0644) //nolint:gosec // test code + require.NoError(t, err) + + // Create a GitOps config file that references the profile + // Note: Environment variables in the YAML config itself get expanded, + // but not in the referenced profile files + gitopsConfig := fmt.Sprintf(` +org_settings: + server_settings: + server_url: %s + secrets: + - secret: test_secret +agent_options: + config: + decorators: + load: + - SELECT uuid AS host_uuid FROM system_info; +controls: + macos_settings: + custom_settings: + - path: %s +queries: [] +policies: [] +`, s.Server.URL, profilePath) + + configPath := filepath.Join(tempDir, "gitops.yml") + err = os.WriteFile(configPath, []byte(gitopsConfig), 0644) //nolint:gosec // test code + require.NoError(t, err) + + // Create a GitOps user + gitOpsUser := s.createGitOpsUser(t) + fleetctlConfig := s.createFleetctlConfig(t, gitOpsUser) + + // Set the environment variable for the valid secret + t.Setenv("FLEET_SECRET_TEST_SECRET", "super_secret_value_123") + t.Setenv("FLEET_DUO_CERTIFICATE_SECRET", "should_not_be_saved") + t.Setenv("HOME", "also_not_saved") + + // Run GitOps dry-run - should fail without the required secret + // First, unset the environment variable to trigger the error + _ = os.Unsetenv("FLEET_SECRET_TEST_SECRET") + _, err = fleetctl.RunAppNoChecks([]string{"gitops", "--config", fleetctlConfig.Name(), "-f", configPath, "--dry-run"}) + require.ErrorContains(t, err, "FLEET_SECRET_TEST_SECRET") + + // Set the env var again and run for real + t.Setenv("FLEET_SECRET_TEST_SECRET", "super_secret_value_123") + _ = fleetctl.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", configPath}) + + // Verify that the secret was saved to the server + secrets, err := s.DS.GetSecretVariables(ctx, []string{"TEST_SECRET"}) + require.NoError(t, err) + require.Len(t, secrets, 1) + assert.Equal(t, "TEST_SECRET", secrets[0].Name) + assert.Equal(t, "super_secret_value_123", secrets[0].Value) + + // Verify that non-FLEET_SECRET_ variables were NOT saved + notSaved, err := s.DS.GetSecretVariables(ctx, []string{"DUO_CERTIFICATE_SECRET", "HOME"}) + require.NoError(t, err) + assert.Empty(t, notSaved, "Non-FLEET_SECRET_ variables should not be saved") + + // Verify that the profile content has the expected substitutions: + // - $FLEET_SECRET_* variables should remain as-is (substituted at delivery time) + // - Other env vars should be expanded during GitOps + profiles, err := s.DS.ListMDMAppleConfigProfiles(ctx, nil) + require.NoError(t, err) + + foundProfile := false + for _, profile := range profiles { + t.Logf("Found profile: %s", profile.Name) + if strings.Contains(profile.Name, "test-profile") || strings.Contains(profile.Identifier, "com.fleet.test.env") { + foundProfile = true + // $FLEET_SECRET_* variables should NOT be expanded (they're expanded at delivery time) + assert.Contains(t, string(profile.Mobileconfig), "$FLEET_SECRET_TEST_SECRET") + // Non-FLEET_SECRET_/FLEET_VAR_ variables SHOULD be expanded during GitOps + assert.Contains(t, string(profile.Mobileconfig), "should_not_be_saved") // Value of $FLEET_DUO_CERTIFICATE_SECRET + assert.Contains(t, string(profile.Mobileconfig), "also_not_saved") // Value of $HOME + // The original variable names should NOT be present + assert.NotContains(t, string(profile.Mobileconfig), "$FLEET_DUO_CERTIFICATE_SECRET") + assert.NotContains(t, string(profile.Mobileconfig), "$HOME") + break + } + } + assert.True(t, foundProfile, "Profile should be uploaded to the server") +} + +// TestFleetSecretInDataTag tests that FLEET_SECRET_ variables in tags of Apple profiles +// are handled properly. +func (s *enterpriseIntegrationGitopsTestSuite) TestFleetSecretInDataTag() { + t := s.T() + tempDir := t.TempDir() + ctx := context.Background() + + // Sample certificate in base64 format (this is a dummy test certificate) + testCertBase64 := `MIIDaTCCAlGgAwIBAgIUNQLezMUpmZK18DcLKt/XTRcLlK8wDQYJKoZIhvcNAQELBQAwRDEfMB0GA1UEAwwWRHVtbXkgVGVzdCBDZXJ0aWZpY2F0ZTEUMBIGA1UECgwLRXhhbXBsZSBPcmcxCzAJBgNVBAYTAlVTMB4XDTI1MDgxOTE3NDMwN1oXDTI2MDgxOTE3NDMwN1owRDEfMB0GA1UEAwwWRHVtbXkgVGVzdCBDZXJ0aWZpY2F0ZTEUMBIGA1UECgwLRXhhbXBsZSBPcmcxCzAJBgNVBAYTAlVTMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA07Np/w5WpFVLlMKX3dZSxwo+c2uwP2glTN0HA5c/6UOQRR9c91yoGGJsD4pfqhtIMSTFw7po3n/PjhGDe/WH+utK+ZIcD0nGD6SvmOggyoohHs81eIOjJAEJjxzhk7eLTVpUI2EnPe/24ei/dgkK59As9qQyH/y+CoR8JIYbNCJH5YLC2Pa44V84QWa2I5DHKUKrUXo9WsrRp1N1JjyaG/6hxLBJZ69e0QTrxxScboreRqVUR6oIEJRTchB+rDG5dxXzCQE6/F8N3qR76t23wd3CLmrcXoEc1P2P331Qzi0KXNXjdJFf0plmfRkT/IWgfM81Vfon1QwENwRSBNmPfQIDAQABo1MwUTAdBgNVHQ4EFgQU9q7SDfQRbJ31snRt2sZzx5sdEpYwHwYDVR0jBBgwFoAU9q7SDfQRbJ31snRt2sZzx5sdEpYwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAYwH42JP45SnZejSF74OcYt8fp08jCWHOiFC3QEo3cROXVn6AWjEbzuQpOxRWF9EizNA83c4E6I+kQVztiuv9bUKGuLFeYb9lUZe8HOvH+j22MtGvZrDPygsJc8TavdkxAsu6OiNQZrYiCFzixkKS9b5p/1B93GBh62OFnV1nUBS8PzAZhOAyJ8UcEhr+GNzZG99/wOkcB0uwxmIb8x8sB3KnQ0qef/qnmgeWxlJlDc/SZ2/4PgtaluZ+noDfNPzaQn4eJNnBz0OTqZ9yuKALeE1WHk8U13zSdc1GNVLhXOrEHegPK5bBmA/lpIQ6HrkwUX7MJ3vK0AD3LjaTzXltDQ==` + + // Create a test team first + team, err := s.DS.NewTeam(ctx, &fleet.Team{ + Name: "Test Team for Secret in Data Tag", + }) + require.NoError(t, err) + + // Create a profile with $FLEET_SECRET_DUO_CERTIFICATE in a tag + // This mimics the real-world scenario where the certificate should be base64 encoded + profileContent := ` + + + + PayloadContent + + + PayloadType + com.apple.security.root + PayloadVersion + 1 + PayloadIdentifier + com.example.test.cert + PayloadUUID + 11111111-2222-3333-4444-555555555555 + PayloadDisplayName + Test Root Certificate + PayloadContent + $FLEET_SECRET_DUO_CERTIFICATE + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.test.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Test MDM Profile with Base64 + +` + + // Write the profile to a file + profilePath := filepath.Join(tempDir, "rootcert-secret.mobileconfig") + err = os.WriteFile(profilePath, []byte(profileContent), 0644) //nolint:gosec + require.NoError(t, err) + + // Create a team GitOps config file that references the profile + teamConfig := fmt.Sprintf(` +name: %s +team_settings: + secrets: + - secret: test_secret +agent_options: + config: + decorators: + load: + - SELECT uuid AS host_uuid FROM system_info; +controls: + macos_settings: + custom_settings: + - path: %s +queries: +policies: +software: +`, team.Name, profilePath) + + configPath := filepath.Join(tempDir, "team-gitops.yml") + err = os.WriteFile(configPath, []byte(teamConfig), 0644) //nolint:gosec + require.NoError(t, err) + + // Create a GitOps user + gitOpsUser := s.createGitOpsUser(t) + fleetctlConfig := s.createFleetctlConfig(t, gitOpsUser) + + // Set the environment variable with the base64-encoded certificate + t.Setenv("FLEET_SECRET_DUO_CERTIFICATE", testCertBase64) + + // The fix expands FLEET_SECRET_ variables for validation only, allowing the profile to be parsed + _, err = fleetctl.RunAppNoChecks([]string{"gitops", "--config", fleetctlConfig.Name(), "-f", configPath, "--dry-run"}) + require.NoError(t, err, "GitOps dry-run should succeed with the fix") + + // Also test without dry-run to confirm it works + _, err = fleetctl.RunAppNoChecks([]string{"gitops", "--config", fleetctlConfig.Name(), "-f", configPath}) + require.NoError(t, err, "GitOps should succeed with the fix") + + // Verify that the profile stored on the server still has the unexpanded variable + profiles, err := s.DS.ListMDMAppleConfigProfiles(ctx, &team.ID) + require.NoError(t, err) + require.Len(t, profiles, 1) + // The stored profile should still contain the unexpanded variable, not the actual secret + require.Contains(t, string(profiles[0].Mobileconfig), "$FLEET_SECRET_DUO_CERTIFICATE", + "Profile should still contain unexpanded FLEET_SECRET variable") +} diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index 55e2f38b35..11eb5db7b6 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -156,8 +156,20 @@ func generateRandomString(sizeBytes int) string { return hex.EncodeToString(b) } +// secretHandling defines how to handle FLEET_SECRET_ variables +type secretHandling int + +const ( + // secretsReject returns an error if FLEET_SECRET_ variables are found + secretsReject secretHandling = iota + // secretsIgnore leaves FLEET_SECRET_ variables as-is (for server to handle) + secretsIgnore + // secretsExpand expands FLEET_SECRET_ variables (for client-side validation only) + secretsExpand +) + func ExpandEnv(s string) (string, error) { - out, err := expandEnv(s, true) + out, err := expandEnv(s, secretsReject) return out, err } @@ -165,9 +177,8 @@ func ExpandEnv(s string) (string, error) { // $ can be escaped with a backslash, e.g. \$VAR // \$ can be escaped with another backslash, etc., e.g. \\\$VAR // $FLEET_VAR_XXX will not be expanded. These variables are expanded on the server. -// If secretsMap is not nil, $FLEET_SECRET_XXX will be evaluated and put in the map -// If secretsMap is nil, $FLEET_SECRET_XXX will cause an error. -func expandEnv(s string, failOnSecret bool) (string, error) { +// The secretMode parameter controls how $FLEET_SECRET_XXX variables are handled. +func expandEnv(s string, secretMode secretHandling) (string, error) { // Generate a random escaping prefix that doesn't exist in s. var preventEscapingPrefix string for { @@ -190,11 +201,23 @@ func expandEnv(s string, failOnSecret bool) (string, error) { // Don't expand fleet vars -- they will be expanded on the server return "", false case strings.HasPrefix(env, fleet.ServerSecretPrefix): - if failOnSecret { + switch secretMode { + case secretsExpand: + // Expand secrets for client-side validation + v, ok := os.LookupEnv(env) + if ok { + return v, true + } + // If secret not found, leave as-is for server to handle + return "", false + case secretsReject: err = multierror.Append(err, fmt.Errorf("environment variables with %q prefix are only allowed in profiles and scripts: %q", fleet.ServerSecretPrefix, env)) + return "", false + default: + // Leave as-is for server to handle + return "", false } - return "", false } // Don't expand fleet vars if they are inside an 'exclusion' zone, @@ -227,7 +250,19 @@ func ExpandEnvBytes(b []byte) ([]byte, error) { } func ExpandEnvBytesIgnoreSecrets(b []byte) ([]byte, error) { - s, err := expandEnv(string(b), false) + s, err := expandEnv(string(b), secretsIgnore) + if err != nil { + return nil, err + } + return []byte(s), nil +} + +// ExpandEnvBytesIncludingSecrets expands environment variables including FLEET_SECRET_ variables. +// This should only be used for client-side validation where the actual secrets are needed temporarily. +// The expanded secrets are never sent to the server. +// Missing FLEET_SECRET_ variables do not fail the method; they are just not expanded. +func ExpandEnvBytesIncludingSecrets(b []byte) ([]byte, error) { + s, err := expandEnv(string(b), secretsExpand) if err != nil { return nil, err } diff --git a/pkg/spec/spec_test.go b/pkg/spec/spec_test.go index eecf92e71d..813937c7b8 100644 --- a/pkg/spec/spec_test.go +++ b/pkg/spec/spec_test.go @@ -224,6 +224,36 @@ func TestLookupEnvSecrets(t *testing.T) { } } +// TestExpandEnvBytesIncludingSecrets tests that FLEET_SECRET_ variables are expanded when using ExpandEnvBytesIncludingSecrets +func TestExpandEnvBytesIncludingSecrets(t *testing.T) { + t.Setenv("FLEET_SECRET_API_KEY", "secret123") + t.Setenv("NORMAL_VAR", "normalvalue") + t.Setenv("FLEET_VAR_HOST", "hostname") + + input := []byte(`API Key: $FLEET_SECRET_API_KEY +Normal: $NORMAL_VAR +Fleet Var: $FLEET_VAR_HOST +Missing: $FLEET_SECRET_MISSING`) + + result, err := ExpandEnvBytesIncludingSecrets(input) + require.NoError(t, err) + + expected := `API Key: secret123 +Normal: normalvalue +Fleet Var: $FLEET_VAR_HOST +Missing: $FLEET_SECRET_MISSING` + + assert.Equal(t, expected, string(result)) + + // Verify that FLEET_VAR_ is not expanded (reserved for server) + assert.Contains(t, string(result), "$FLEET_VAR_HOST") + // Verify that FLEET_SECRET_ is expanded + assert.Contains(t, string(result), "secret123") + assert.NotContains(t, string(result), "$FLEET_SECRET_API_KEY") + // Verify that missing secrets are left as-is + assert.Contains(t, string(result), "$FLEET_SECRET_MISSING") +} + func TestGetExclusionZones(t *testing.T) { testCases := []struct { fixturePath []string diff --git a/server/fleet/apple_mdm.go b/server/fleet/apple_mdm.go index 3fec868f2d..74784231ea 100644 --- a/server/fleet/apple_mdm.go +++ b/server/fleet/apple_mdm.go @@ -5,9 +5,11 @@ import ( "crypto/md5" // nolint: gosec "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http" + "regexp" "strings" "time" @@ -258,6 +260,24 @@ func NewMDMAppleConfigProfile(raw []byte, teamID *uint) (*MDMAppleConfigProfile, }, nil } +// payloadDisplayNameRegex is used to extract PayloadDisplayName values from raw XML content +var payloadDisplayNameRegex = regexp.MustCompile(`PayloadDisplayName\s*([^<]*)`) + +// ValidateNoSecretsInProfileName checks if PayloadDisplayName contains FLEET_SECRET_ variables +// in the raw XML content of a profile. +func ValidateNoSecretsInProfileName(xmlContent []byte) error { + matches := payloadDisplayNameRegex.FindAllSubmatch(xmlContent, -1) + for _, match := range matches { + if len(match) > 1 { + displayName := string(match[1]) + if len(ContainsPrefixVars(displayName, ServerSecretPrefix)) > 0 { + return errors.New("PayloadDisplayName cannot contain FLEET_SECRET variables") + } + } + } + return nil +} + func (cp MDMAppleConfigProfile) ValidateUserProvided() error { // first screen the top-level object for reserved identifiers and names if _, ok := mobileconfig.FleetPayloadIdentifiers()[cp.Identifier]; ok { diff --git a/server/fleet/apple_mdm_test.go b/server/fleet/apple_mdm_test.go index f6d46ce0f4..add3bae2dc 100644 --- a/server/fleet/apple_mdm_test.go +++ b/server/fleet/apple_mdm_test.go @@ -654,3 +654,100 @@ func TestConfigurationProfileLabelEqual(t *testing.T) { "Does cmp.Equal for ConfigurationProfileLabel needs to be updated for new/updated field(s)?") assert.True(t, cmp.Equal(items[0], items[1])) } + +func TestValidateNoSecretsInProfileName(t *testing.T) { + testCases := []struct { + name string + xmlContent string + expectErr bool + errMsg string + }{ + { + name: "no secrets", + xmlContent: ` + + + + PayloadDisplayName + Test Profile + PayloadIdentifier + com.test.profile + +`, + expectErr: false, + }, + { + name: "secret in PayloadDisplayName", + xmlContent: ` + + + + PayloadDisplayName + Test $FLEET_SECRET_PASSWORD Profile + PayloadIdentifier + com.test.profile + +`, + expectErr: true, + errMsg: "PayloadDisplayName cannot contain FLEET_SECRET variables", + }, + { + name: "multiple PayloadDisplayNames with secret in one", + xmlContent: ` + + + + PayloadDisplayName + Main Profile + PayloadContent + + + PayloadDisplayName + Sub Profile $FLEET_SECRET_KEY + + + +`, + expectErr: true, + errMsg: "PayloadDisplayName cannot contain FLEET_SECRET variables", + }, + { + name: "secret in other field not PayloadDisplayName", + xmlContent: ` + + + + PayloadDisplayName + Test Profile + PayloadDescription + Description with $FLEET_SECRET_VALUE + +`, + expectErr: false, + }, + { + name: "whitespace in PayloadDisplayName value", + xmlContent: ` + + + + PayloadDisplayName + Test Profile + +`, + expectErr: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateNoSecretsInProfileName([]byte(tc.xmlContent)) + if tc.expectErr { + require.Error(t, err) + require.Contains(t, err.Error(), tc.errMsg) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 77a3d1b164..0e051a2b8b 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -390,6 +390,11 @@ func (svc *Service) NewMDMAppleConfigProfile(ctx context.Context, teamID uint, r }) } + // Check for secrets in profile name before expansion + if err := fleet.ValidateNoSecretsInProfileName(b); err != nil { + return nil, ctxerr.Wrap(ctx, fleet.NewInvalidArgumentError("profile", err.Error())) + } + // Expand and validate secrets in profile expanded, secretsUpdatedAt, err := svc.ds.ExpandEmbeddedSecretsAndUpdatedAt(ctx, string(b)) if err != nil { @@ -2594,6 +2599,11 @@ func (svc *Service) BatchSetMDMAppleProfiles(ctx context.Context, tmID *uint, tm fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), "maximum configuration profile file size is 1 MB"), ) } + // Check for secrets in profile name before expansion + if err := fleet.ValidateNoSecretsInProfileName(prof); err != nil { + return ctxerr.Wrap(ctx, + fleet.NewInvalidArgumentError(fmt.Sprintf("profiles[%d]", i), err.Error())) + } // Expand profile for validation expanded, secretsUpdatedAt, err := svc.ds.ExpandEmbeddedSecretsAndUpdatedAt(ctx, string(prof)) if err != nil { diff --git a/server/service/apple_mdm_test.go b/server/service/apple_mdm_test.go index 62f1e4e1e2..ca406269f6 100644 --- a/server/service/apple_mdm_test.go +++ b/server/service/apple_mdm_test.go @@ -778,6 +778,12 @@ func TestNewMDMAppleConfigProfile(t *testing.T) { r = bytes.NewReader(mcBytes) _, err = svc.NewMDMAppleConfigProfile(ctx, 0, r, nil, fleet.LabelsIncludeAll) assert.ErrorContains(t, err, "Fleet variable") + + // Test profile with FLEET_SECRET in PayloadDisplayName + mcBytes = mcBytesForTest("Profile $FLEET_SECRET_PASSWORD", "test.identifier", "UUID") + r = bytes.NewReader(mcBytes) + _, err = svc.NewMDMAppleConfigProfile(ctx, 0, r, nil, fleet.LabelsIncludeAll) + assert.ErrorContains(t, err, "PayloadDisplayName cannot contain FLEET_SECRET variables") } func mcBytesForTest(name, identifier, uuid string) []byte { @@ -802,6 +808,23 @@ func mcBytesForTest(name, identifier, uuid string) []byte { `, name, identifier, uuid)) } +func TestBatchSetMDMAppleProfilesWithSecrets(t *testing.T) { + svc, ctx, _ := setupAppleMDMService(t, &fleet.LicenseInfo{Tier: fleet.TierPremium}) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) + + // Test profile with FLEET_SECRET in PayloadDisplayName + profileWithSecret := mcBytesForTest("Profile $FLEET_SECRET_PASSWORD", "test.identifier", "UUID") + err := svc.BatchSetMDMAppleProfiles(ctx, nil, nil, [][]byte{profileWithSecret}, false, false) + assert.ErrorContains(t, err, "PayloadDisplayName cannot contain FLEET_SECRET variables") + + // Test multiple profiles where one has a secret in PayloadDisplayName + goodProfile := mcBytesForTest("Good Profile", "good.identifier", "UUID1") + badProfile := mcBytesForTest("Bad $FLEET_SECRET_KEY Profile", "bad.identifier", "UUID2") + err = svc.BatchSetMDMAppleProfiles(ctx, nil, nil, [][]byte{goodProfile, badProfile}, false, false) + assert.ErrorContains(t, err, "PayloadDisplayName cannot contain FLEET_SECRET variables") + assert.ErrorContains(t, err, "profiles[1]") +} + func TestNewMDMAppleDeclaration(t *testing.T) { svc, ctx, ds := setupAppleMDMService(t, &fleet.LicenseInfo{Tier: fleet.TierPremium}) ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}}) diff --git a/server/service/client.go b/server/service/client.go index 389bdafc65..782ccd7a14 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -363,15 +363,46 @@ func getProfilesContents(baseDir string, macProfiles []fleet.MDMProfileSpec, win 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 + // For validation, we need to expand FLEET_SECRET_ variables in tags so the XML parser + // can properly validate the profile structure. However, we must be careful not to expose + // secrets in the profile name. + containsSecrets := len(fleet.ContainsPrefixVars(string(fileContents), fleet.ServerSecretPrefix)) > 0 + + if containsSecrets { + // If profile contains secrets, check for secrets in PayloadDisplayName first + if err := fleet.ValidateNoSecretsInProfileName(fileContents); err != nil { + return nil, fmt.Errorf("%s: %w", prefixErrMsg, err) } - return nil, fmt.Errorf("%s: %w", prefixErrMsg, errForMsg) + + // Expand secrets for validation + validationContents, expandErr := spec.ExpandEnvBytesIncludingSecrets(fileContents) + if expandErr != nil { + return nil, fmt.Errorf("%s: expanding secrets for validation: %w", prefixErrMsg, expandErr) + } + + // Validate the profile structure with expanded secrets + mcExpanded, validationErr := fleet.NewMDMAppleConfigProfile(validationContents, nil) + if validationErr != nil { + errForMsg := errors.Unwrap(validationErr) + if errForMsg == nil { + errForMsg = validationErr + } + return nil, fmt.Errorf("%s: %w", prefixErrMsg, errForMsg) + } + + name = strings.TrimSpace(mcExpanded.Name) + } else { + // No secrets, parse normally + 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) } - 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.") diff --git a/server/service/client_test.go b/server/service/client_test.go index 9012d7d37e..fde2e66858 100644 --- a/server/service/client_test.go +++ b/server/service/client_test.go @@ -720,6 +720,165 @@ func TestGetProfilesContents(t *testing.T) { expectError: true, wantErr: "Couldn't edit macos_settings.custom_settings (bar.cfg): macOS configuration profiles must be .mobileconfig or .json files", }, + { + name: "with FLEET_SECRET in data tag", + baseDir: tempDir, + macSetupFiles: [][2]string{ + {"cert.mobileconfig", ` + + + + PayloadContent + + + PayloadType + com.apple.security.root + PayloadVersion + 1 + PayloadIdentifier + com.example.cert + PayloadUUID + 11111111-2222-3333-4444-555555555555 + PayloadDisplayName + Test Certificate + PayloadContent + $FLEET_SECRET_CERT_DATA + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Certificate Profile + +`}, + }, + environment: map[string]string{ + "FLEET_SECRET_CERT_DATA": "VGVzdENlcnREYXRhQmFzZTY0", // "TestCertDataBase64" in base64 + }, + expandEnv: true, + expectError: false, + want: []fleet.MDMProfileBatchPayload{ + { + Name: "Certificate Profile", + Contents: []byte(` + + + + PayloadContent + + + PayloadType + com.apple.security.root + PayloadVersion + 1 + PayloadIdentifier + com.example.cert + PayloadUUID + 11111111-2222-3333-4444-555555555555 + PayloadDisplayName + Test Certificate + PayloadContent + $FLEET_SECRET_CERT_DATA + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Certificate Profile + +`), + }, + }, + }, + { + name: "with FLEET_SECRET in PayloadDisplayName - should reject", + baseDir: tempDir, + macSetupFiles: [][2]string{ + {"secret_name.mobileconfig", ` + + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Profile $FLEET_SECRET_NAME + +`}, + }, + environment: map[string]string{ + "FLEET_SECRET_NAME": "SecretProfileName", + }, + expandEnv: true, + expectError: true, + wantErr: "PayloadDisplayName cannot contain FLEET_SECRET variables", + }, + { + name: "with FLEET_VAR in profile - should not expand", + baseDir: tempDir, + macSetupFiles: [][2]string{ + {"fleet_var.mobileconfig", ` + + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Profile with FLEET_VAR + SomeValue + $FLEET_VAR_HOST_END_USER_IDP_USERNAME + +`}, + }, + expandEnv: true, + expectError: false, + want: []fleet.MDMProfileBatchPayload{ + { + Name: "Profile with FLEET_VAR", + Contents: []byte(` + + + + PayloadType + Configuration + PayloadVersion + 1 + PayloadIdentifier + com.example.profile + PayloadUUID + aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee + PayloadDisplayName + Profile with FLEET_VAR + SomeValue + $FLEET_VAR_HOST_END_USER_IDP_USERNAME + +`), + }, + }, + }, } for _, tt := range tests { diff --git a/server/service/integration_mdm_profiles_test.go b/server/service/integration_mdm_profiles_test.go index 3bdee6c8f9..99407304f3 100644 --- a/server/service/integration_mdm_profiles_test.go +++ b/server/service/integration_mdm_profiles_test.go @@ -93,9 +93,9 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { PayloadContent PayloadDisplayName - $FLEET_SECRET_INVALID + My profile PayloadIdentifier - N3 + $FLEET_SECRET_INVALID PayloadType Configuration PayloadUUID @@ -110,6 +110,31 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { errMsg := extractServerErrorText(res.Body) require.Contains(t, errMsg, "$FLEET_SECRET_INVALID") + invalidSecretsProfile = []byte(` + + + + + PayloadContent + + PayloadDisplayName + $FLEET_SECRET_INVALID + PayloadIdentifier + N3 + PayloadType + Configuration + PayloadUUID + 601E0B42-0989-4FAD-A61B-18656BA3670E + PayloadVersion + 1 + + +`) + + res = s.Do("POST", "/api/v1/fleet/mdm/apple/profiles/batch", batchSetMDMAppleProfilesRequest{Profiles: [][]byte{invalidSecretsProfile}}, http.StatusUnprocessableEntity) + errMsg = extractServerErrorText(res.Body) + require.Contains(t, errMsg, "PayloadDisplayName cannot contain FLEET_SECRET variables") + // create a new team tm, err := s.ds.NewTeam(ctx, &fleet.Team{Name: "batch_set_mdm_profiles"}) require.NoError(t, err) @@ -204,7 +229,6 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { // Use secret variables in a profile secretIdentifier := "secret-identifier-1" secretType := "secret.type.1" - secretName := "secretName" secretProfile := string(mobileconfigForTest("NS1", "IS1")) req := createSecretVariablesRequest{ SecretVariables: []fleet.SecretVariable{ @@ -216,10 +240,6 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { Name: "FLEET_SECRET_TYPE", Value: secretType, }, - { - Name: "FLEET_SECRET_NAME", - Value: secretName, - }, { Name: "FLEET_SECRET_PROFILE", Value: secretProfile, @@ -233,7 +253,7 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { teamProfiles = [][]byte{ mobileconfigForTest("N4", "I4"), mobileconfigForTestWithContent("N5", "I5", "$FLEET_SECRET_IDENTIFIER", "${FLEET_SECRET_TYPE}", - "$FLEET_SECRET_NAME"), + "InnerName5"), // The whole profile is one big secret. []byte("$FLEET_SECRET_PROFILE"), } @@ -253,7 +273,6 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { // Manually replace the expected secret variables in the profile wantTeamProfiles[1] = []byte(strings.ReplaceAll(string(wantTeamProfiles[1]), "$FLEET_SECRET_IDENTIFIER", secretIdentifier)) wantTeamProfiles[1] = []byte(strings.ReplaceAll(string(wantTeamProfiles[1]), "${FLEET_SECRET_TYPE}", secretType)) - wantTeamProfiles[1] = []byte(strings.ReplaceAll(string(wantTeamProfiles[1]), "$FLEET_SECRET_NAME", secretName)) wantTeamProfiles[2] = []byte(secretProfile) // verify that we should install the team profiles s.signedProfilesMatch(wantTeamProfiles, installs) @@ -272,12 +291,16 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { require.Empty(t, removes) // Change the secret variable and upload the profiles again. We should see the profile with updated secret installed. - secretName = "newSecretName" + secretType = "new.secret.type.1" req = createSecretVariablesRequest{ SecretVariables: []fleet.SecretVariable{ { - Name: "FLEET_SECRET_NAME", - Value: secretName, // changed + Name: "FLEET_SECRET_IDENTIFIER", + Value: secretIdentifier, // did not change + }, + { + Name: "FLEET_SECRET_TYPE", + Value: secretType, // changed }, { Name: "FLEET_SECRET_PROFILE", @@ -297,7 +320,6 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "$FLEET_SECRET_IDENTIFIER", secretIdentifier)) wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "${FLEET_SECRET_TYPE}", secretType)) - wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "$FLEET_SECRET_NAME", secretName)) // verify that we should install the team profiles s.signedProfilesMatch(wantTeamProfilesChanged, installs) wantTeamProfiles[1] = wantTeamProfilesChanged[0] @@ -343,12 +365,16 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { require.Empty(t, removes) // Change the secret variable and upload the profiles again. We should see the profile with updated secret installed. - secretName = "new2SecretName" + secretType = "new2.secret.type.1" req = createSecretVariablesRequest{ SecretVariables: []fleet.SecretVariable{ { - Name: "FLEET_SECRET_NAME", - Value: secretName, // changed + Name: "FLEET_SECRET_IDENTIFIER", + Value: secretIdentifier, // did not change + }, + { + Name: "FLEET_SECRET_TYPE", + Value: secretType, // changed }, { Name: "FLEET_SECRET_PROFILE", @@ -368,7 +394,6 @@ func (s *integrationMDMTestSuite) TestAppleProfileManagement() { wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "$FLEET_SECRET_IDENTIFIER", secretIdentifier)) wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "${FLEET_SECRET_TYPE}", secretType)) - wantTeamProfilesChanged[0] = []byte(strings.ReplaceAll(string(wantTeamProfilesChanged[0]), "$FLEET_SECRET_NAME", secretName)) // verify that we should install the team profiles s.signedProfilesMatch(wantTeamProfilesChanged, installs) wantTeamProfiles[1] = wantTeamProfilesChanged[0]