fleetdm / fleet

Open-source platform for IT, security, and infrastructure teams. (Linux, macOS, Chrome, Windows, cloud, data center)
https://fleetdm.com
Other
3.16k stars 432 forks source link

Fleet uninstall script removes other apps from the host #22571

Closed marko-lisica closed 1 month ago

marko-lisica commented 1 month ago

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.
sharon-fdm commented 1 month ago

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.

lucasmrod commented 1 month ago

@noahtalerman @lukeheath

Following is the current default uninstaller script for pkg: https://github.com/fleetdm/fleet/blob/8cfa35c1c55c7e830c7375910959770324579d0f/pkg/file/scripts/uninstall_pkg.sh#L1-L21

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 commented 1 month ago

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 commented 1 month ago

Decision provided in Slack conversation to proceed with option B.

nonpunctual commented 1 month ago

@getvictor @lucasmrod @lukeheath

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

#!/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 commented 1 month ago

@xpkoala added QA notes.

lukeheath commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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.

PezHub commented 1 month ago

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

fleet-release commented 1 month ago

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