Prevent EPERM error hit when trying to open installer.exe #1855

nagilson · 2024-06-25T23:12:39Z

Changes to Permissions

fs.chmod does not set execute permissions on windows https://nodejs.org/api/fs.html#fschmodpath-mode-callback. This means we should also rely on icacls to try to do so.
If we still dont have permission, we can collect more information so we know how to fix it better going forward.

This also makes each installer get its own unique folder. SHA is used since it is an algorithm widely available on multiple OS. https://support.globalsign.com/ssl/ssl-certificates-life-cycle/sha-256-compatibility
If this does not work, we can ask for elevation using the vscode library on windows if permission to run the installer is denied.

Command Exeuctor Private Interface Changes

Make Command Executor return an object instead of string. This prevents us from having to run commands twice to get the status and stdout.

Caution must be taken to make sure that the assumptions here in what is returned as stdout vs stderr are correct.

This prevents us from having to run commands twice to get the status and stdout. Caution must be taken to make sure that the assumptions here in what is returned as stdout vs stderr are correct. Furthermore, when looking at the code, I saw something that looked just plain incorrect, so I will need to investigate that by adding the TODO before merge

vscode-dotnet-runtime-library/src/Acquisition/GenericDistroSDKProvider.ts

…de-dotnet-runtime into nagilson-fix-eperm

vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts

MiYanni · 2024-07-03T18:50:02Z

vscode-dotnet-runtime-library/src/Acquisition/DotnetInstall.ts

-export function InstallToStrings(key: DotnetInstall | null) {
-    if (!key) {
-        return { installKey: '', version: '', architecture: '', isGlobal: '', installMode: '' };
-    }


nit: I believe the convention for TS is to have the curly braces at the end of the line, like what was here previously. Scripting languages tend to follow that convention (JS, TS, etc.) while C-style languages put it in a new line by itself. But I understand you're changing it to be consistent with everything else in this repo. I believe there is an .editorconfig setting to can enable to do this for you automatically.

A similar inconsistency I see is if ( vs if(. But I don't know which is more stylistically the convention for TS.

That's a great point. EditorConfig is pretty awesome, but the javascript and typescript settings appear to be less standardized than the C# and .NET settings for brackets. Anywho, I will make that config file in another PR upcoming soon :)

That is the convention, but I disagree with the convention. I always think about apples SSL bug https://www.codecentric.de/wissens-hub/blog/curly-braces#:~:text=To%20sum%20up%20the%20debacle,To%20say%20the%20least. and that is a large part of my reasoning for my opinion here.

Personally, learning to read/write in the conventions of another language is very useful and transferrable. I tried to write in a C#-style in ColdFusion and it didn't work whatsoever. Not just stylistically, but conceptually. But TS is pretty loose on this particular convention. However, any TS I've worked on always had the curly braces at the end of the line, to match the JavaScipt style.

MiYanni · 2024-07-03T18:52:41Z

vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts

+
+            if(os.platform() === 'win32') // Windows does not have chmod +x ability with nodejs.
+            {
+                const commandRes = await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', ['/grant', `"${installerPath}"`, `"%username%":F`, '/t', '/c']), null, false);


What permissions does node need to run icacls?

That's a very good question.

I did some digging, the internet seems to suggest you need administrator rights to run icalcs. That makes sense. But I just tried to do it without admin rights on a local user file without administrator rights and it also worked. It did not work on a non-user-local file. This also makes sense, so it appears the final answer is that it's scoped similarly to areas on disk that are already writeable to.

This is being run by a sub-process of vscode, which should have the same privileges as vscode. If it is an admin it is not a concern. If it is not admin it should be running under the user account. The file its trying to add +x to is also a user local file, so my understanding is that it should function properly and be permitted.

Unfortunately we dont have any actual customer reports of EPERM issues, just that its showing up in our error data. I think either way though, if this does not work, the primary option we have is just to ask for elevation right away.

joeloff · 2024-07-03T19:50:31Z

vscode-dotnet-runtime-library/src/Acquisition/WinMacGlobalInstaller.ts

+
+            if(os.platform() === 'win32') // Windows does not have chmod +x ability with nodejs.
+            {
+                const commandRes = await this.commandRunner.execute(CommandExecutor.makeCommand('icacls', ['/grant', `"${installerPath}"`, `"%username%":F`, '/t', '/c']), null, false);


Are these parameters ordered? According to https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls#syntax the syntax should be icacls <filename> [/grant[:r] <sid>:<perm>[...]]

Also note that because you're not using grant:r you will be adding and retaining existing perms, which is different from the explicit 744 that chmod is doing. Also, should the chmod be moved into a separate if for non-windows and then explicitly set the ACL.

Can you share the results of this? Run the code, then from Powershell, run Get-Acl <path-to-filename> | Select-Object -ExpandProperty Access and just strip out any PII before pasting in it (or ping me offline).

Interesting, that's a good point. It was working for me with this ordering, but I will fix the ordering.

I will also condition the chmod. It has a side-effect of allowing us to change the write permissions, but I suppose if icacl works then it doesn't need to be called.

ACL before code calling:

FileSystemRights : FullControl AccessControlType : Allow IdentityReference : NT AUTHORITY\SYSTEM IsInherited : True InheritanceFlags : None PropagationFlags : None FileSystemRights : FullControl AccessControlType : Allow IdentityReference : BUILTIN\Administrators IsInherited : True InheritanceFlags : None PropagationFlags : None FileSystemRights : FullControl AccessControlType : Allow IdentityReference : REDACT IsInherited : True InheritanceFlags : None PropagationFlags : None

ACL After Code Calling:

FileSystemRights : FullControl AccessControlType : Allow IdentityReference : REDACT IsInherited : False InheritanceFlags : None PropagationFlags : None FileSystemRights : FullControl AccessControlType : Allow IdentityReference : NT AUTHORITY\SYSTEM IsInherited : True InheritanceFlags : None PropagationFlags : None FileSystemRights : FullControl AccessControlType : Allow IdentityReference : BUILTIN\Administrators IsInherited : True InheritanceFlags : None PropagationFlags : None FileSystemRights : FullControl AccessControlType : Allow IdentityReference : REDACT IsInherited : True InheritanceFlags : None PropagationFlags : None

)" This reverts commit 8cabdcf.

nagilson and others added 8 commits June 25, 2024 16:11

Merge branch 'main' into nagilson-fix-eperm

b34f1a8

Fix Execution Type Parsing

330ee2e

fix linting issue

ec9fcff

Fix line parsing logic

9292386

Merge branch 'main' into nagilson-fix-eperm

ab2a7ef

Fix tests

c554a2c

Run icacls to set execute permissions

a3f0c2b

nagilson changed the title ~~Elevate if EPERM error hit when trying to open installer.exe~~ Prevent EPERM error hit when trying to open installer.exe Jul 2, 2024

nagilson added 2 commits July 2, 2024 11:35

move to sha256

a7c1c87

remove unnecessary code

12753fb

nagilson commented Jul 2, 2024

View reviewed changes

vscode-dotnet-runtime-library/src/Acquisition/GenericDistroSDKProvider.ts Outdated Show resolved Hide resolved

nagilson commented Jul 2, 2024

View reviewed changes

vscode-dotnet-runtime-library/src/Acquisition/GenericDistroSDKProvider.ts Outdated Show resolved Hide resolved

nagilson added 6 commits July 2, 2024 11:41

Remove todo that should go in another PR

eb83ca0

Remove redundant trim

e8ada86

rename class CommandProcessorOutput since it didnt match CommandExecutor

05c57a1

Merge branch 'nagilson-fix-eperm' of https://github.com/nagilson/vsco…

4f3efdd

…de-dotnet-runtime into nagilson-fix-eperm

Rename class file

a75d493

Report json object for failed permissions.

29f7e2d

nagilson commented Jul 2, 2024

View reviewed changes

vscode-dotnet-runtime-library/src/Utils/CommandExecutor.ts Show resolved Hide resolved

nagilson added 3 commits July 2, 2024 13:14

Fix test to use correct folder

cc2ae05

fix lint

ba3580f

Fix installer file deletion

3fdb95b

nagilson requested review from a team and MiYanni July 2, 2024 23:58

respond to linter

f4b7098

MiYanni approved these changes Jul 3, 2024

View reviewed changes

joeloff reviewed Jul 3, 2024

View reviewed changes

nagilson merged commit 8cabdcf into dotnet:main Jul 3, 2024
9 checks passed

nagilson added a commit that referenced this pull request Jul 3, 2024

Revert "Prevent EPERM error hit when trying to open installer.exe (#1855

5bf013e

)" This reverts commit 8cabdcf.

nagilson mentioned this pull request Jul 3, 2024

Fix file permissions command #1860

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent EPERM error hit when trying to open installer.exe #1855

Prevent EPERM error hit when trying to open installer.exe #1855

nagilson commented Jun 25, 2024 •

edited

Loading

MiYanni Jul 3, 2024

nagilson Jul 3, 2024 •

edited

Loading

MiYanni Jul 3, 2024

MiYanni Jul 3, 2024

nagilson Jul 3, 2024 •

edited

Loading

joeloff Jul 3, 2024

nagilson Jul 3, 2024

Prevent EPERM error hit when trying to open installer.exe #1855

Prevent EPERM error hit when trying to open installer.exe #1855

Conversation

nagilson commented Jun 25, 2024 • edited Loading

Changes to Permissions

Command Exeuctor Private Interface Changes

MiYanni Jul 3, 2024

Choose a reason for hiding this comment

nagilson Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

MiYanni Jul 3, 2024

Choose a reason for hiding this comment

MiYanni Jul 3, 2024

Choose a reason for hiding this comment

nagilson Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

joeloff Jul 3, 2024

Choose a reason for hiding this comment

nagilson Jul 3, 2024

Choose a reason for hiding this comment

nagilson commented Jun 25, 2024 •

edited

Loading

nagilson Jul 3, 2024 •

edited

Loading

nagilson Jul 3, 2024 •

edited

Loading