diff --git a/changes/43389-patch-policy-gitops-bugs b/changes/43389-patch-policy-gitops-bugs new file mode 100644 index 0000000000..a21562110c --- /dev/null +++ b/changes/43389-patch-policy-gitops-bugs @@ -0,0 +1,2 @@ +- Fixed bug where renaming a patch policy in a GitOps file caused it to be deleted initially. +- Fixed a nil pointer dereference in the contributor api spec/policies. diff --git a/server/datastore/mysql/policies.go b/server/datastore/mysql/policies.go index 67b1bea07b..6bf39dfc97 100644 --- a/server/datastore/mysql/policies.go +++ b/server/datastore/mysql/policies.go @@ -1478,6 +1478,11 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs // generate new up-to-date patch policy if spec.Type == fleet.PolicyTypePatch { + if fmaTitleID == nil { + return ctxerr.Wrap(ctx, &fleet.BadRequestError{ + Message: fmt.Sprintf("fleet_maintained_app_slug must be set for patch policy: %s", spec.Name), + }) + } installer, err := ds.getPatchPolicyInstaller(ctx, ptr.ValOrZero(teamID), *fmaTitleID) if err != nil { return ctxerr.Wrap(ctx, err, "getting patch policy installer") @@ -1517,6 +1522,7 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs var ( shouldRemoveAllPolicyMemberships bool removePolicyStats bool + shouldUpdatePatchPolicyName bool ) if insertOnDuplicateDidInsertOrUpdate(res) { // Figure out if the query, platform, software installer, VPP app, or script changed. @@ -1548,7 +1554,16 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs if teamID == nil { err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id is NULL", spec.Name) } else { - err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id = ?", spec.Name, teamID) + // Patch policies are unique by patch_software_title_id so we need to get them by that, and update their name + // so that it doesn't get deleted later. + if spec.Type == fleet.PolicyTypePatch { + err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE patch_software_title_id = ? AND team_id = ?", fmaTitleID, teamID) + if _, ok := teamIDToPoliciesByName[teamID][spec.Name]; !ok { + shouldUpdatePatchPolicyName = true + } + } else { + err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id = ?", spec.Name, teamID) + } } if err != nil { return ctxerr.Wrap(ctx, err, "select policies id") @@ -1595,6 +1610,11 @@ func (ds *Datastore) ApplyPolicySpecs(ctx context.Context, authorID uint, specs return ctxerr.Wrap(ctx, err, "setting needs_full_membership_cleanup flag") } } + if shouldUpdatePatchPolicyName { + if _, err := tx.ExecContext(ctx, `UPDATE policies SET name = ?, checksum = `+policiesChecksumComputedColumn()+` WHERE id = ?`, spec.Name, policyID); err != nil { + return ctxerr.Wrap(ctx, err, "setting name for patch policy") + } + } // Defer cleanup outside the transaction to avoid long-held row locks on // policy_membership. pendingCleanups = append(pendingCleanups, policyCleanupArgs{ diff --git a/server/datastore/mysql/policies_test.go b/server/datastore/mysql/policies_test.go index 46cc3b2541..ef8c6f790a 100644 --- a/server/datastore/mysql/policies_test.go +++ b/server/datastore/mysql/policies_test.go @@ -7720,6 +7720,108 @@ func testTeamPatchPolicy(t *testing.T, ds *Datastore) { require.Equal(t, "Windows - Maintained2 up to date", p5.Name) require.Equal(t, "windows", p5.Platform) require.Equal(t, "SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM programs WHERE name = 'Maintained2' AND version_compare(version, '1.0') < 0);", p5.Query) + + ////////////////////////////////////////////// + // ApplyPolicySpecs + + // Test ApplyPolicySpecs with patch policies using a separate team. + team2, err := ds.NewTeam(ctx, &fleet.Team{Name: "team2"}) + require.NoError(t, err) + + // Set up an FMA installer on team2 for the valid slug test. + team2Payload := &fleet.UploadSoftwareInstallerPayload{ + InstallScript: "hello", + PreInstallQuery: "SELECT 1", + PostInstallScript: "world", + StorageID: "storage-team2", + Filename: "maintained1-team2", + Title: "Maintained1", + Version: "1.0", + Source: "apps", + Platform: "darwin", + BundleIdentifier: "fleet.maintained1", + UserID: user1.ID, + TeamID: &team2.ID, + ValidatedLabels: &fleet.LabelIdentsWithScope{}, + FleetMaintainedAppID: &maintainedApp.ID, + } + _, _, err = ds.MatchOrCreateSoftwareInstaller(ctx, team2Payload) + require.NoError(t, err) + + // ApplyPolicySpecs with type=patch and empty fleet_maintained_app_slug should return an error. + err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{ + { + Name: "patch-no-slug", + Query: "SELECT 1;", + Team: "team2", + Type: fleet.PolicyTypePatch, + }, + }) + require.Error(t, err) + + // ApplyPolicySpecs with type=patch and a non-existent fleet_maintained_app_slug should return an error. + err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{ + { + Name: "patch-bad-slug", + Query: "SELECT 1;", + Team: "team2", + Type: fleet.PolicyTypePatch, + FleetMaintainedAppSlug: "nonexistent-slug", + }, + }) + require.Error(t, err) + + // ApplyPolicySpecs with type=patch and a valid fleet_maintained_app_slug should succeed. + err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{ + { + Name: "patch-valid-slug", + Query: "SELECT 1;", + Team: "team2", + Type: fleet.PolicyTypePatch, + FleetMaintainedAppSlug: "maintained1", + }, + }) + require.NoError(t, err) + + // Verify the policy was created with the expected auto-generated query and platform. + policies, _, err := ds.ListTeamPolicies(ctx, team2.ID, fleet.ListOptions{}, fleet.ListOptions{}, "") + require.NoError(t, err) + require.Len(t, policies, 1) + require.Equal(t, "patch-valid-slug", policies[0].Name) + require.Equal(t, fleet.PolicyTypePatch, policies[0].Type) + require.Equal(t, "darwin", policies[0].Platform) + require.Contains(t, policies[0].Query, "fleet.maintained1") + + // Renaming a patch policy via ApplyPolicySpecs should update it, not delete it. + previousID := policies[0].ID + var previousChecksum []byte + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &previousChecksum, `SELECT checksum FROM policies WHERE id = ?`, previousID) + }) + + err = ds.ApplyPolicySpecs(ctx, user1.ID, []*fleet.PolicySpec{ + { + Name: "patch-renamed", + Query: "SELECT 1;", + Team: "team2", + Type: fleet.PolicyTypePatch, + FleetMaintainedAppSlug: "maintained1", + }, + }) + require.NoError(t, err) + + policies, _, err = ds.ListTeamPolicies(ctx, team2.ID, fleet.ListOptions{}, fleet.ListOptions{}, "") + require.NoError(t, err) + require.Len(t, policies, 1) + require.Equal(t, previousID, policies[0].ID) + require.Equal(t, "patch-renamed", policies[0].Name) + require.Equal(t, fleet.PolicyTypePatch, policies[0].Type) + + var newChecksum []byte + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &newChecksum, `SELECT checksum FROM policies WHERE id = ?`, previousID) + }) + require.NotEqual(t, previousChecksum, newChecksum) } func testTeamPolicyAutomationFilter(t *testing.T, ds *Datastore) { diff --git a/server/fleet/policies.go b/server/fleet/policies.go index 5bbb98dace..db7ca64047 100644 --- a/server/fleet/policies.go +++ b/server/fleet/policies.go @@ -117,6 +117,7 @@ var ( errPolicyPatchAndQuerySet = errors.New("If the \"type\" is \"patch\", the \"query\" field is not supported.") errPolicyPatchAndPlatformSet = errors.New("If the \"type\" is \"patch\", the \"platform\" field is not supported.") errPolicyPatchNoTitleID = errors.New("If the \"type\" is \"patch\", the \"patch_software_title_id\" field is required.") + errPatchPolicyRequiresTeam = errors.New("If the \"type\" is \"patch\", the \"team\" field is required.") errPolicyQueryUpdated = errors.New("\"query\" can't be updated") errPolicyPlatformUpdated = errors.New("\"platform\" can't be updated") errPolicyConditionalAccessEnabledInvalidPlatform = errors.New("\"conditional_access_enabled\" is only valid on \"darwin\" and \"windows\" policies") @@ -207,6 +208,13 @@ func verifyPolicyPlatforms(platforms string) error { return nil } +func verifyPatchPolicy(team string, typ string) error { + if typ == PolicyTypePatch && emptyString(team) { + return errPatchPolicyRequiresTeam + } + return nil +} + func PolicyVerifyConditionalAccess(conditionalAccessEnabled bool, platform string) error { if conditionalAccessEnabled && !strings.Contains(platform, "darwin") && !strings.Contains(platform, "windows") { return errPolicyConditionalAccessEnabledInvalidPlatform @@ -473,6 +481,7 @@ type PolicySpec struct { Type string `json:"type"` FleetMaintainedAppSlug string `json:"fleet_maintained_app_slug"` + PatchSoftwareTitleID uint `json:"-"` } // PolicySoftwareTitle contains software title data for policies. @@ -507,6 +516,9 @@ func (p PolicySpec) Verify() error { if err := PolicyVerifyConditionalAccess(p.ConditionalAccessEnabled, p.Platform); err != nil { return err } + if err := verifyPatchPolicy(p.Team, p.Type); err != nil { + return err + } return nil } diff --git a/server/service/client.go b/server/service/client.go index 751298ea52..d71a7f25d2 100644 --- a/server/service/client.go +++ b/server/service/client.go @@ -3000,6 +3000,11 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers [] } config.Policies[i].ScriptID = &scriptID } + + // Get patch policy title IDs for the team + for i := range config.Policies { + config.Policies[i].PatchSoftwareTitleID = softwareTitleIDsBySlug[config.Policies[i].FleetMaintainedAppSlug] + } } // Get the ids and names of current policies to figure out which ones to delete @@ -3037,6 +3042,7 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers [] } } } + var policiesToDelete []uint for _, oldItem := range policies { found := false @@ -3045,6 +3051,13 @@ func (c *Client) doGitOpsPolicies(config *spec.GitOps, teamSoftwareInstallers [] found = true break } + // patch policies are unique by patch_software_title_id so matching by name doesn't always work + if newItem.Type == fleet.PolicyTypePatch && oldItem.PatchSoftware != nil { + if oldItem.PatchSoftware.SoftwareTitleID == newItem.PatchSoftwareTitleID { + found = true + break + } + } } if !found { policiesToDelete = append(policiesToDelete, oldItem.ID) diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index f5f86af0a3..603ac5ca9d 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -27438,6 +27438,19 @@ func (s *integrationEnterpriseTestSuite) TestPatchPolicies() { require.Equal(t, "dynamic", getPolicyResp.Policy.Type) require.Empty(t, getPolicyResp.Policy.PatchSoftware) require.Empty(t, getPolicyResp.Policy.PatchSoftwareTitleID) + + // Should not be able to apply global patch policy + spec := &fleet.PolicySpec{ + Name: "team patch policy", + Query: "SELECT 1", + Type: fleet.PolicyTypePatch, + FleetMaintainedAppSlug: "zoom/windows", + } + applyResp := fleet.ApplyPolicySpecsResponse{} + s.DoJSON("POST", "/api/latest/fleet/spec/policies", + fleet.ApplyPolicySpecsRequest{Specs: []*fleet.PolicySpec{spec}}, + http.StatusBadRequest, &applyResp, + ) }) t.Run("patch_policy_lifecycle", func(t *testing.T) {