From a24500c937e910f22e377b178a3227b70ec1b4bc Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Mon, 28 Jul 2025 13:58:19 -0500 Subject: [PATCH] Skip software installers for which we can't, or don't need to, parse package IDs/create uninstall scripts (#31347) Fixes #30565. Applies to FMA-only extensions (DMG, ZIP), EXEs, and tarballs. This means that MSI/PKG FMAs will still have package IDs populated a day after server start if they aren't filled in, on the off chance that admins use $PACKAGE_ID on uninstall scripts on either of those, replicating existing behavior. # 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. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually --- changes/30565-cron-errors | 1 + ee/server/service/software_installers.go | 6 ++-- server/datastore/mysql/software_installers.go | 7 ++-- server/fleet/datastore.go | 6 ++-- server/mock/datastore_mock.go | 2 +- server/service/integration_enterprise_test.go | 32 +++++++++++++++++++ 6 files changed, 44 insertions(+), 10 deletions(-) create mode 100644 changes/30565-cron-errors diff --git a/changes/30565-cron-errors b/changes/30565-cron-errors new file mode 100644 index 0000000000..8b13f66687 --- /dev/null +++ b/changes/30565-cron-errors @@ -0,0 +1 @@ +* Fixed cases where the uninstall script population job introduced in Fleet 4.57.0 would attempt to extract package IDs on software that we don't generate uninstall scripts for, causing errors in logs and retries of the job. diff --git a/ee/server/service/software_installers.go b/ee/server/service/software_installers.go index e96dddf7f7..bb7a1a3f45 100644 --- a/ee/server/service/software_installers.go +++ b/ee/server/service/software_installers.go @@ -2365,10 +2365,10 @@ func UninstallSoftwareMigration( softwareInstallStore fleet.SoftwareInstallerStore, logger kitlog.Logger, ) error { - // Find software installers without package_id - idMap, err := ds.GetSoftwareInstallersWithoutPackageIDs(ctx) + // Find software installers that should have their uninstall script populated + idMap, err := ds.GetSoftwareInstallersPendingUninstallScriptPopulation(ctx) if err != nil { - return ctxerr.Wrap(ctx, err, "getting software installers without package_id") + return ctxerr.Wrap(ctx, err, "getting software installers to modufy") } if len(idMap) == 0 { return nil diff --git a/server/datastore/mysql/software_installers.go b/server/datastore/mysql/software_installers.go index cfe4cbaa98..571250edf7 100644 --- a/server/datastore/mysql/software_installers.go +++ b/server/datastore/mysql/software_installers.go @@ -2353,10 +2353,9 @@ func (ds *Datastore) GetDetailsForUninstallFromExecutionID(ctx context.Context, return result.Name, result.SelfService, nil } -func (ds *Datastore) GetSoftwareInstallersWithoutPackageIDs(ctx context.Context) (map[uint]string, error) { - query := ` - SELECT id, storage_id FROM software_installers WHERE package_ids = '' - ` +func (ds *Datastore) GetSoftwareInstallersPendingUninstallScriptPopulation(ctx context.Context) (map[uint]string, error) { + query := `SELECT id, storage_id FROM software_installers WHERE package_ids = '' + AND extension NOT IN ('exe', 'tar.gz', 'dmg', 'zip')` type result struct { ID uint `db:"id"` StorageID string `db:"storage_id"` diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go index 3b6e802576..69ea7b232d 100644 --- a/server/fleet/datastore.go +++ b/server/fleet/datastore.go @@ -1901,8 +1901,10 @@ type Datastore interface { // (if set) post-install scripts, otherwise those fields are left empty. GetSoftwareInstallerMetadataByTeamAndTitleID(ctx context.Context, teamID *uint, titleID uint, withScriptContents bool) (*SoftwareInstaller, error) - // GetSoftwareInstallersWithoutPackageIDs returns a map of software installers to storage ids that do not have a package ID. - GetSoftwareInstallersWithoutPackageIDs(ctx context.Context) (map[uint]string, error) + // GetSoftwareInstallersPendingUninstallScriptPopulation returns a map of software installers to storage IDs that: + // 1. need uninstall scripts populated + // 2. can have uninstall scripts auto-generated by Fleet + GetSoftwareInstallersPendingUninstallScriptPopulation(ctx context.Context) (map[uint]string, error) // GetMSIInstallersWithoutUpgradeCode returns a map of MSI software installers to storage ids that do not have an upgrade code set. GetMSIInstallersWithoutUpgradeCode(ctx context.Context) (map[uint]string, error) diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go index b83568f23a..6056923a59 100644 --- a/server/mock/datastore_mock.go +++ b/server/mock/datastore_mock.go @@ -7700,7 +7700,7 @@ func (s *DataStore) GetSoftwareInstallerMetadataByTeamAndTitleID(ctx context.Con return s.GetSoftwareInstallerMetadataByTeamAndTitleIDFunc(ctx, teamID, titleID, withScriptContents) } -func (s *DataStore) GetSoftwareInstallersWithoutPackageIDs(ctx context.Context) (map[uint]string, error) { +func (s *DataStore) GetSoftwareInstallersPendingUninstallScriptPopulation(ctx context.Context) (map[uint]string, error) { s.mu.Lock() s.GetSoftwareInstallersWithoutPackageIDsFuncInvoked = true s.mu.Unlock() diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 9378f97de8..ac65494b4c 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -11350,6 +11350,38 @@ func (s *integrationEnterpriseTestSuite) TestSoftwareInstallerUploadDownloadAndD err = eeservice.UninstallSoftwareMigration(context.Background(), s.ds, s.softwareInstallStore, logger) require.NoError(t, err) + // Update DB by clearing package ids and swapping extension to one we skip + mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + if _, err = q.ExecContext(context.Background(), `UPDATE software_installers SET package_ids = '', extension = 'tar.gz' WHERE id = ?`, + installerID); err != nil { + return err + } + return nil + }) + + // Running the migration again causes no issues. + err = eeservice.UninstallSoftwareMigration(context.Background(), s.ds, s.softwareInstallStore, logger) + require.NoError(t, err) + + // Package ID and extension should not have been modified + mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + var packageIDs string + if err := sqlx.GetContext(context.Background(), q, &packageIDs, `SELECT package_ids FROM software_installers WHERE id = ?`, + installerID); err != nil { + return err + } + assert.Empty(t, packageIDs) + + var extension string + if err := sqlx.GetContext(context.Background(), q, &extension, `SELECT extension FROM software_installers WHERE id = ?`, + installerID); err != nil { + return err + } + assert.Equal(t, "tar.gz", extension) + + return nil + }) + // delete the installer s.Do("DELETE", fmt.Sprintf("/api/latest/fleet/software/titles/%d/available_for_install", titleID), nil, http.StatusNoContent, "team_id", fmt.Sprintf("%d", *payload.TeamID))