ProductUpgrader: platform specific upgrade directories by jamill · Pull Request #1224 · microsoft/VFSForGit

jamill · 2019-05-30T17:26:56Z

Different platforms have different file system layouts / capabilities. The existing upgrade logic is Windows centric and does not match the behaviors of other systems.

Upgrade components that interact with the fileystem:

Component	description	data scope	Windows	Mac
Upgrader Application	Temporary copy of upgrader application upgrade main application	Protected	ProgramData\GVFS\GVFS.Upgrade\Tools	/usr/local/vfsforgit_upgrader/Tools
gvfs.config	config file for upgrade related settings	Protected	ProgramData\GVFS\gvfs.config	/usr/local/vfsforgit/gvfs.config
Upgrade Package Download	Location to download upgrade package	Protected	ProgramData\GVFS\GVFS.Upgrade\Download	/usr/local/vfsforgit_upgrader/Download
Upgrader Package Unpack	Location to unpack the downloaded update package	Protected	ProgramData\GVFS\GVFS.Upgrade\Download\InstallerTemp	/usr/local/vfsforgit_upgrader/Download/InstallerTemp
Upgrader Logs	Directory to write upgrade logs to	Writable by user account	ProgramData\GVFS\GVFS.Upgrade\UpgraderLogs	~/Library/Application Support/GVFS/GVFS.Upgrade/UpgraderLogs
Upgrade Available File	File indicating whether an upgrade is available	Written by GVFS.Service process Windows: protected Mac: writable by user account	ProgramData\GVFS\GVFS.Upgrade\Tools\HighestAvailableVersion	~/Library/Application Support/GVFS/GVFS.Upgrade/HighestAvailableVersion

Data Scopes

This is not generally applicable across platforms, but is a model for how things are set up on Mac.

Scope	Usage	Windows	Mac
Non-protected Data	Unprivileged data that user accounts can access.	Logs directory	Logs and HighestAvailableVersion file. ~/Library/Application Support/GVFS/GVFS.Upgrade
Protected Data	Requires root / elevation to mutate	ProgramData\GVFS\GVFS.Upgrade	/usr/local/vfsforgit_upgrader

Motivation

On Windows, the upgrade related directories are located under the "service" level directory structure, and is system scoped and requires elevated permissions to modify (except for specific exemptions). The base directory is ProgramData\GVFS\GVFS.Upgrade. We have to tweak the permissions on the UpgraderLogs directory in order to allow non-elevated access, as non-elevated processes need to be able to write logs here.

On Mac, the root of the "service" data directories is a user scoped, and not system scoped. As the upgrade flow uses ACLs to protect the data, we need to find another directory.

This is a proposed change to enable VFS for Git to account for the platform differences.

Mac File System Hierarchy

There are several candidates for where we can place "protected" data, including /usr/local, or /var. The proposed location for the Upgrader app and related data is: /usr/local/vfsforgit_upgrader, but I could see putting the data (and / or) application in /var as well.

derrickstolee · 2019-05-30T20:44:00Z

@jamill Thanks for the detailed PR description! It is truly enlightening all the things that are going on here and how we need to change things for Mac.

nickgra · 2019-05-31T21:48:27Z

I reviewed the description/design and it looks good to me!

Your model works for protecting the vfsforgit_upgrader folder if the user hasn't messed with the permissions on the /usr/local folder, but if the mode was modified to allow anyone to write to the folder, we could be vulnerable to the attack where someone could replace the vfsforgit_upgrader folder with their own content and have VFS4G execute it as admin. I don't know enough about ACLs on Windows, but we might be vulnerable in the same way there.
(I don't have any immediate ideas on how we could fix this off the top of my head, besides resetting the permissions on /usr/local before we lay down the upgrader folder. I'll keep thinking on it though.)

Also, there are some places in your PR description where you have /usr/local.vfsforgit_upgrader instead of /usr/local/vfsforgit_upgrader.

alameenshah

The approach looks good. Couple of things about Protected Data that can be potentially optimized on the Mac.

Is Protected Data directory really needed on a Mac? Alternate options to consider -

The pattern of copying upgrader tool to a temporary directory on Windows exists on the premise that assemblies that are open get locked and an upgrade installer cannot overwrite them (as long as they remain open). By copying upgrader tool to a temp directory and shutting down running instances of gvfs, we ensure that none of the installed gvfs assemblies remain open during upgrade. However, on the Mac, this does not apply - it is possible to overwrite/delete an binary that is open & running. So /usr/local/vfsforgit/gvfs upgrade can run and upgrade itself.
Upgrade Download & Unpack (the upgrade payload). Can we utilize the fact that our installers are code signed using Microsoft certificate, to avoid the need for additional protection? On the newest Macs, Apple installer will not let user install an unsigned installer package, unless -allowUntrusted command line option is specified. This should enable us to download the installer payload in an unprotected temp directory.

alameenshah · 2019-06-03T16:13:32Z

One more thought about using /usr/local.vfsforgit_upgrader/Download/InstallerTemp to store temp files. There is this directory /private/var/tmp which is special directory with a sticky bit set. We wouldn't need to set custom ACLs for protection.

jamill · 2019-06-03T16:14:10Z

Thanks for the feedback!

@alameenshah -

Is Protected Data directory really needed on a Mac?

Good question. As you pointed out, locked files are not an issue on Mac, but I was not sure if there could be other problems. For instance, could an operation cause an assembly load after we have overwritten the original application directory, and result in an "unexpected" assembly being found?

Can we utilize the fact that our installers are code signed using Microsoft certificate, to avoid the need for additional protection?

Another good point - this will help with the installer(s) themselves, but there could be scripts or other artifacts included with the installer package.

@nickgra -

but if the mode was modified to allow anyone to write to the folder, we could be vulnerable to the attack

Good point. We had an assumption that the user would not tweak this part of their system. We could include an additional check that the system directories are ACL'ed as we expect, and handle the cases where it isn't (maybe refuse to install?)

kewillford

I think we need to come up with the right terminology for these and make sure it is consistent throughout the code base or things get confusing.

jamill · 2019-06-13T23:00:10Z

        {
            this.Config = upgraderConfig;
-
-            string upgradesDirectoryPath = ProductUpgraderInfo.GetUpgradesDirectoryPath();


TODO: follow up on why we were creating this directory, and see if we still need to create a directory here.

I'm not sure if something else would have created the directory by the time RecordHighestAvailableVersion runs, but it looks like if the directory did not exist for RecordHighestAvailableVersion the call to WriteAllText in RecordHighestAvailableVersion would throw a DirectoryNotFoundException.

There might be other methods that GetUpgradesDirectoryPath that similarly assume the directory already exists.

wilbaker

Overall I think the changes look good, just left some minor questions/comments

wilbaker · 2019-06-14T17:24:59Z

        {
            this.Config = upgraderConfig;
-
-            string upgradesDirectoryPath = ProductUpgraderInfo.GetUpgradesDirectoryPath();


I'm not sure if something else would have created the directory by the time RecordHighestAvailableVersion runs, but it looks like if the directory did not exist for RecordHighestAvailableVersion the call to WriteAllText in RecordHighestAvailableVersion would throw a DirectoryNotFoundException.

There might be other methods that GetUpgradesDirectoryPath that similarly assume the directory already exists.

wilbaker · 2019-06-14T17:28:54Z

+                ProductUpgraderInfo.DownloadDirectory);
+        }
+
+        public static string GetHighestAvailableVersionDirectory()


Can/should this method be removed and replaced with calls to GVFSPlatform.Instance.GetUpgradeHighestAvailableVersionDirectory() directly?

wilbaker · 2019-06-14T17:29:45Z

+                ProductUpgraderInfo.ApplicationDirectory);
+        }
+
+        public static string GetParentLogDirectoryPath()


Can/should this method be removed and replaced with calls to GVFSPlatform.Instance.GetUpgradeLogDirectoryParentDirectory() directly?

wilbaker · 2019-06-14T17:39:52Z

+        {
+            get
+            {
+                return Path.Combine(this.Constants.GVFSBinDirectoryPath, LocalGVFSConfig.FileName);


Note: I am not an expert on how file permissions work on Mac, so there might be no issues here!

Does sudo care at all about what group a user is in? I'm wondering if two users are modifying /usr/local/vfsforgit/gvfs.config is there any risk that one of them will block the other from being able to make further changes (using sudo)?

Similarly, is there any chance that one user's changes might prevent another users from being able to read the file (without using sudo)?

Great question(s)!

Root is its own account, so when running a command via sudo, root will be the owner of this file. The mode bits on the file will be such that root is the only account that can modify the file, while it will be readable by everyone. If two different users attempt to modify this file via a sudo command, it will be the root account that is making changes.

/cc @jeffhostetler to check my understanding :)

nickgra

This is looking good to me, I agree with Kevin and William on the naming suggestions to ensure that we're using the same terminology throughout the codebase.

kewillford

Looks good to me. Just a couple of naming things.

The config file location currently exists in the same directory as the GVFS service data directory. On Mac, the GVFS Service directory is per user, and not per machine. Even if the service is per user on Mac, we still would like the config to be per machine. This change enables the GVFS config file to exist at a location independent of the GVFS Service data directory. On Windows, the location will not change, but on macOS, it will now exist in the GVFS binary location.

Different platform have different structures for where the upgrade application and logs need to go. This change introduces the architecture for making this platform specific. Description of different components involved in upgrade and how they interact with different platforms. --- Component | description ----------------------- | ----------- Upgrader Application | Temporary copy of upgrader application upgrade main application gvfs.config | config file for upgrade related settings Upgrade Package Download | Location to download upgrade package Upgrader Package Unpack | Location to unpack the downloaded update package Upgrader Logs | Directory to write upgrade logs to Upgrade Available File | File indicating whether an upgrade is available Component | data scope | Windows | Mac ------------------------ | ------------------------------- | ----------------------------------------------------------- | ------------------------------------------------------------------------ Upgrader Application | Protected | ProgramData\GVFS\GVFS.Upgrade\Tools | /usr/local/vfsforgit_upgrader/Tools gvfs.config | Protected | ProgramData\GVFS\gvfs.config | /usr/local/vfsforgit/gvfs.config Upgrade Package Download | Protected | ProgramData\GVFS\GVFS.Upgrade\Download | /usr/local/vfsforgit_upgrader/Download Upgrader Package Unpack | Protected | ProgramData\GVFS\GVFS.Upgrade\Download\InstallerTemp | /usr/local/vfsforgit_upgrader/Download/InstallerTemp Upgrader Logs | Writable by user account | ProgramData\GVFS\GVFS.Upgrade\UpgraderLogs | ~/Library/Application Support/GVFS/GVFS.Upgrade/UpgraderLogs Upgrade Available File | Written by GVFS.Service process | ProgramData\GVFS\GVFS.Upgrade\Tools\HighestAvailableVersion | ~/Library/Application Support/GVFS/GVFS.Upgrade/HighestAvailableVersion

jamill added the WIP label May 30, 2019

jamill requested review from alameenshah, kewillford, mjcheetham, nickgra and wilbaker May 30, 2019 17:26

kewillford reviewed May 30, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/MacProductUpgraderInfo.cs Outdated

kewillford reviewed May 30, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/ProductUpgraderInfo.cs Outdated

jamill force-pushed the upgrader_tasks/directory_structure branch from 37c5dea to 5c744fa Compare May 31, 2019 15:59

jamill commented May 31, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/ProductUpgraderInfoImpl.cs Outdated

jamill mentioned this pull request May 31, 2019

Upgrader for macOS MVP #1129

Closed

15 tasks

jrbriggs added domain: upgrade platform: macOS platform: windows labels Jun 1, 2019

alameenshah reviewed Jun 3, 2019

View reviewed changes

alameenshah requested a review from pmj June 3, 2019 16:14

jamill force-pushed the upgrader_tasks/directory_structure branch 2 times, most recently from c4549b0 to 4bfd666 Compare June 3, 2019 21:26

jamill changed the title ~~WIP: ProductUpgrader: platform specific upgrade directories~~ ProductUpgrader: platform specific upgrade directories Jun 4, 2019

jamill removed the WIP label Jun 4, 2019

jamill force-pushed the upgrader_tasks/directory_structure branch 2 times, most recently from d7f39bd to cfed1bd Compare June 11, 2019 02:32

jamill force-pushed the upgrader_tasks/directory_structure branch 2 times, most recently from bc76497 to b412425 Compare June 13, 2019 17:03

jamill commented Jun 13, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/GitHubUpgrader.cs Outdated

kewillford suggested changes Jun 13, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/ProductUpgraderInfo.cs

Comment thread GVFS/GVFS.Platform.Mac/MacProductUpgraderPlatformStrategy.cs Outdated

jamill commented Jun 13, 2019

View reviewed changes

Comment thread GVFS/GVFS/CommandLine/DiagnoseVerb.cs

jamill commented Jun 13, 2019

View reviewed changes

jamill force-pushed the upgrader_tasks/directory_structure branch 2 times, most recently from c70aaf7 to a194f05 Compare June 14, 2019 17:38

wilbaker reviewed Jun 14, 2019

View reviewed changes

jamill force-pushed the upgrader_tasks/directory_structure branch 5 times, most recently from 7950398 to c299152 Compare June 14, 2019 20:41

nickgra reviewed Jun 17, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/GVFSPlatform.cs Outdated

kewillford approved these changes Jun 17, 2019

View reviewed changes

Comment thread GVFS/GVFS.Common/ProductUpgrader.cs Outdated

Comment thread GVFS/GVFS.Common/ProductUpgrader.cs Outdated

jamill force-pushed the upgrader_tasks/directory_structure branch from c299152 to 96a51c1 Compare June 17, 2019 19:52

jamill force-pushed the upgrader_tasks/directory_structure branch from 96a51c1 to 0e5fb4a Compare June 17, 2019 20:37

jamill merged commit 8a9038a into microsoft:master Jun 18, 2019

jrbriggs added this to the M155 milestone Jun 21, 2019

Conversation

jamill commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Upgrade components that interact with the fileystem:

Data Scopes

Motivation

Mac File System Hierarchy

Uh oh!

derrickstolee commented May 30, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickgra commented May 31, 2019

Uh oh!

alameenshah left a comment

Choose a reason for hiding this comment

Uh oh!

alameenshah commented Jun 3, 2019

Uh oh!

jamill commented Jun 3, 2019

Uh oh!

Uh oh!

kewillford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamill Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilbaker Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilbaker Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

wilbaker Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

jamill Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

nickgra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kewillford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jamill commented May 30, 2019 •

edited

Loading