Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fleet uninstall script removes other apps from the host #22571

Closed
marko-lisica opened this issue Oct 2, 2024 · 11 comments
Closed

Fleet uninstall script removes other apps from the host #22571

marko-lisica opened this issue Oct 2, 2024 · 11 comments
Assignees
Labels
bug Something isn't working as documented ~critical bug This is a critical bug and may require a patch release. #g-endpoint-ops Endpoint ops product group P0 Prioritize as emergency :release Ready to write code. Scheduled in a release. See "Making changes" in handbook.
Milestone

Comments

@marko-lisica
Copy link
Member

marko-lisica commented Oct 2, 2024

Fleet version: 4.57.1

Web browser and operating system: Google Chrome 129.0.6668.90 on macOS (arm64)


💥  Actual behavior

I uninstalled IriunWebcam.app (IriunWebcam-2.8.8.pkg), and it uninstalled many other apps from the host (1Password, Google Chrome, Fleet Desktop, Slack, Spotify, Raycast, Zoom, VSCode and probably some more, but can't remember now what I had on the host).

🧑‍💻  Steps to reproduce

  1. Upload IriunWebcam 2.8.8 version to Fleet
  2. Install it on the host via Fleet
  3. Uninstall it from the host via Fleet

🕯️ More info (optional)

Script content and output are attached here:
Archive.zip

QA notes

Functional Test

Please test with as many PKGs as you can and report findings.
Test that only the directory containing the app is deleted (/Applications/$APP_NAME, e.g. /Applications/MicrosoftTeams.pkg).
Some uninstall scripts may not work for some PKGs, we should just document the findings for now.
We plan on iterating the dumb script.

Migration

We need to test migrating from 4.57.0/4.57.1 to 4.57.2

  1. Install 4.57.0/4.57.1
  2. Upload PKGs.
  3. Check the uninstaller script is the one that uses pkgutil.
  4. Migrate to 4.57.2.
  5. Check that the uinstaller script for the PGKs is now the new version ("rm -rf /Applications/[...]" at the end).
  6. Check that the migration hasn't changed other uninstaller scripts for DEB or MSI.
@marko-lisica marko-lisica added bug Something isn't working as documented :reproduce Involves documenting reproduction steps in the issue :incoming New issue in triage process. ~critical bug This is a critical bug and may require a patch release. and removed :reproduce Involves documenting reproduction steps in the issue labels Oct 2, 2024
@sharon-fdm sharon-fdm added the P0 Prioritize as emergency label Oct 2, 2024
@sharon-fdm
Copy link
Collaborator

sharon-fdm commented Oct 2, 2024

Adding P0.
If it turns out that this is only a problem with this specific script, we will lower the priority.
@lukeheath, feel free to override this decision.

@sharon-fdm sharon-fdm added #g-endpoint-ops Endpoint ops product group :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. labels Oct 2, 2024
@lucasmrod
Copy link
Member

@noahtalerman @lukeheath

Following is the current default uninstaller script for pkg:

#!/bin/sh
# Fleet extracts and saves package IDs.
pkg_ids=$PACKAGE_ID
# Get all files associated with package and remove them
for pkg_id in "${pkg_ids[@]}"
do
# Get volume and location of package
volume=$(pkgutil --pkg-info "$pkg_id" | grep -i "volume" | awk '{for (i=2; i<NF; i++) printf $i " "; print $NF}')
location=$(pkgutil --pkg-info "$pkg_id" | grep -i "location" | awk '{for (i=2; i<NF; i++) printf $i " "; print $NF}')
# Check if this package id corresponds to a valid/installed package
if [[ ! -z "$volume" && ! -z "$location" ]]; then
# Remove individual files/directories belonging to package
pkgutil --files "$pkg_id" | sed -e 's@^@'"$volume""$location"'/@' | tr '\n' '\0' | xargs -n 1 -0 rm -rf
# Remove receipts
pkgutil --forget "$pkg_id"
else
echo "WARNING: volume or location are empty for package ID $pkg_id"
fi
done

We've found that many packages list /Applications, /Library when invoking pkgutil --files ... on them.
This causes fleetd to remove everything under those root directories.

The team has discussed two proposals:

A. Make current default uinstall script smarter by adding checks to explicitly prevent deletion of these directories. But then we are open to some missed path or edge case that could cause similar issue.

B. Be a bit dumber when it comes to uninstalling. Change the default uninstall script to just remove /Applications/$APP_NAME. This is the safest and users can edit the uninstaller script in case they need to remove extra stuff.

IMO we should go with B (safe and dumb but flexible).

@mostlikelee
Copy link
Contributor

Copying my Slack response to supporting option B:

I like this approach for a few reasons:
1 - minimizes damage
2 - easy for admin to understand
3 - works for ~90% of apps
4 - easy to implement (put out the P0 fire)

Does it remove supporting files? no. But it doesn't need to most of the time. More complicated uninstalls (plugins/extensions) are usually handled by a vendor supplied uninstaller.

@lukeheath
Copy link
Member

Decision provided in Slack conversation to proceed with option B.

lucasmrod added a commit that referenced this issue Oct 2, 2024
…#22585)

#22571

- [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/Committing-Changes.md#changes-files)
for more information.
- [X] Added/updated tests
- [X] Manual QA for all new/changed functionality
@nonpunctual
Copy link
Contributor

nonpunctual commented Oct 2, 2024

@getvictor @lucasmrod @lukeheath

I would like to recommend using something like the logic in this script for application removals. Why?

  • Not tied to pkg receipt data so it would handle any install.
  • Uses bundle ID which is required for all Mac apps
  • Uses is-at-least zsh built-in to parse version strings for precise targeting
  • Uses Spotlight app metadata
  • Has dry run option for showing what would be removed before actual deletion
#!/bin/zsh

# notes:

# $4 Script Parameter in Jamf must be an application bundle id, e.g., com.apple.dt.Xcode
# $5 Script Parameter in Jamf must be an application bundle name, e.g., Xcode.app
# $6 Script Parameter in Jamf must be an application short version string, e.g., 14.2
# $7 Script Parameter in Jamf toggles a test mode. If populated with the string "test" the script will perform a "dry run" without removing files.


# variables (only populate these if Jamf Script Parameters are not used)

varbndl=''
varname=''
varvers=''


###############################
##### DO NOT MODIFY BELOW #####
###############################


# data

appbndl="${4:-$varbndl}"
appname="${5:-$varname}"
appvers="${6:-$varvers}"
tstmode="$7"

IFS=$'\n'
arrdata=($(/usr/bin/mdfind -0 "kMDItemCFBundleIdentifier = $appbndl" | /usr/bin/xargs -0 /usr/bin/mdls -name 'kMDItemVersion' -name 'kMDItemPath' | /usr/bin/sed 's/kMDItemPath    = //g;s/kMDItemVersion = //g'))


# functions

autoload is-at-least

appvr_ck(){ >&2 printf "\nChecking %s versions...\n" "$appname" ; }

appvr_no(){ >&2 printf "\nNo %s found. Exiting...\n" "$appname" ; }

appvr_ok(){ >&2 printf "\nFound %s\nversion = %s\nOk. Leaving %s in place...\n" "${arrdata[i-1]}" "${arrdata[i]}" "$appname" ; }

appvr_rm(){ 
appkill="$(echo "${arrdata[i-1]}" | /usr/bin/sed 's/"//g')"
>&2 printf "\nFound %s\nversion = %s\nInsecure version. Deleting %s...\n" "$appkill" "${arrdata[i]}" "$appkill"; /bin/rm -f -r "$appkill"
>&2 printf "\nValidating deleted path: %s\n" "$appkill"; /bin/ls -ls "$appkill"
exit 0
}

appvr_tm(){
>&2 printf "\nFound %s\nversion = %s\nInsecure version. Deleting %s\n" "${arrdata[i-1]}" "${arrdata[i]}" "${arrdata[i-1]}"
>&2 printf "\n!!! TEST MODE !!! Disabling test mode will remove: %s\n" "${arrdata[i-1]}"
}

not_root(){ >&2 printf "\nThis script must be executed as the root user. Exiting...\n" ; }


# operations

if [ "$EUID" != 0 ]
then
    not_root; exit
fi

if [ -z "${arrdata[*]}" ]
then
    appvr_no; exit
fi

appvr_ck
for ((i=2;i<=${#arrdata[@]};i+=2))
{
    if is-at-least "$appvers" "${arrdata[i]}"
    then
        appvr_ok; continue
    else
        case "$tstmode" in
            'test' ) appvr_tm ;;
                 * ) appvr_rm ;;
        esac
    fi
}

@lucasmrod
Copy link
Member

@xpkoala added QA notes.

georgekarrv pushed a commit that referenced this issue Oct 2, 2024
…#22585)

#22571

- [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/Committing-Changes.md#changes-files)
for more information.
- [X] Added/updated tests
- [X] Manual QA for all new/changed functionality
@lukeheath
Copy link
Member

@nonpunctual Thanks for the ideas. We are going to release an immediate quick fix to resolve the P0, and will circle back on something more permanent.

@lukeheath
Copy link
Member

For reference when we circle back after immediate fix is released. Ideas from a prospective customer:

"You might consider replicating the logic in https://github.com/munki/munki/blob/main/code/client/removepackages and https://github.com/munki/munki/blob/main/code/client/munkilib/installer/rmpkgs.py"

@nonpunctual
Copy link
Contributor

nonpunctual commented Oct 2, 2024

Well in order to do any of that you would need to partially rely on the metadata munki creates when it installs things. The spotlight metadata is independent of that. If you are only looking at metadata related to pkg installs, you are ignoring all other ways of installing apps. Bundle ID is an attribute that every app has. You don't really need install history to do .app removals unless you are using .bom to remove dependencies.

iansltx pushed a commit that referenced this issue Oct 2, 2024
#22571

`20241002104104_UpdateUninstallScript.go` will be released in v4.57.2,
thus I'm moving the unreleased migrations in main to run after it.
@PezHub
Copy link
Contributor

PezHub commented Oct 3, 2024

QA Notes:
Results thus far for functional tests can be found here

Migration test results performed for each app type are as follows:
(installers were uploaded to ver 4.57.0 --> 4.57.2)
✅ .pkg = uninstall script was replaced with the (current) proposed fix (expected)
✅ .msi = uninstall script remained unchanged (expected)
✅ .exe = uninstall script remained unchanged (expected)
✅ .deb = uninstall script remained unchanged (expected)

Will stop QA efforts here until we decide on path forward based on results from the functional tests

lucasmrod added a commit that referenced this issue Oct 3, 2024
#22571

- [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/Committing-Changes.md#changes-files)
for more information.
- [X] Added/updated tests
- [X] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [X] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [X] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [X] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [X] Manual QA for all new/changed functionality
lucasmrod added a commit that referenced this issue Oct 3, 2024
#22571

- [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/Committing-Changes.md#changes-files)
for more information.
- [X] Added/updated tests
- [X] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [X] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [X] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [X] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [X] Manual QA for all new/changed functionality
lucasmrod added a commit that referenced this issue Oct 3, 2024
@fleet-release
Copy link
Contributor

Uninstall script fixed,
Apps stay safe, undisturbed peace,
Cloud city breathes ease.

@sharon-fdm sharon-fdm removed the :incoming New issue in triage process. label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as documented ~critical bug This is a critical bug and may require a patch release. #g-endpoint-ops Endpoint ops product group P0 Prioritize as emergency :release Ready to write code. Scheduled in a release. See "Making changes" in handbook.
Projects
None yet
Development

No branches or pull requests

10 participants