Implement UnixFileMode APIs #69980

tmds · 2022-05-30T09:44:25Z

Implements #67837.

dotnet-issue-labeler · 2022-05-30T09:44:31Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-05-30T09:44:37Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements #67837.

Author:	tmds
Assignees:	-
Labels:	`area-System.IO`, `new-api-needs-documentation`, `community-contribution`
Milestone:	-

tmds · 2022-05-30T09:49:47Z

Currently this only has the Unix implementation, and there are no tests yet.

The implementation relies on some methods that are added in #60507. I'll rebase when that PR is merged.

Can you give me some pointers on what APIs I should use for this on Windows (to get/set the mode as WSL/NTFS metadata)?
And, if/what we'd like to do to have a umask on Windows?

cc @eerhardt @danmoseley @jozkee @stephentoub @adamsitnik

danmoseley · 2022-05-31T03:28:26Z

I looked into what it would mean to support this APi on Windows in the context of WSL2.

Per this Linux files are stored in NTFS using a file system plugin called VolFS. WSL2 can access Windows files using a plugin called DrvFS.

DrvFS. Iff you enable the metadata option in WSL2 (eg with wsl.conf) then when writing files in the Windows filesystem (eg., in /mnt/c) from WSL, the access mask, uid and gid will be preserved by applying NTFS extended attributes (key value pairs associated with the file object, not the same as alternate data streams) whose names such as $LXMOD are documented. I'm not sure what use case they have in mind, because one could consider them an implementation detail. However, being documented, they can be read and written from Windows programs. More info
VolFS. When storing Linux files in NTFS, the access mask, etc are clearly preserved for Linux to work at all. It's not clear whether this is done in the same way, or if it is, it's apparently not projected through the "\\wsl$" mount point by which you can access them from Windows. For example, \\wsl.localhost\Ubuntu-20.04\home\dan\.bash_history appears to have no extended attributes, and attempting to set some gives STATUS_NOT_IMPLEMENTED. The "actual" storage according to that document is in %LocalAppData%\lxss, but nothing is visible there (unless possibly it is only visible in a higher integrity level)3.
Edit: I just realized this refers to WSL1. As I understand it, WSL2 is implemented as a true VM. So access via \\wsl$ is not backed by individual files in an NTFS volume so it is no surprise that EA’s aren’t present. I was testing with WSL2. I didn’t test WSL1.

To read extended attributes you can use NtQueryEaFile, but it's apparently not documented (only ZwQueryEaFile which drivers can use). I haven't found a documented Win32 that allows you to do it. @JeremyKuhne do you have insight here?

Although, even if it is documented, it's not clear to me that there is much value in wiring this up for Windows, because it apparently will only enable it for files in the Windows filesystem written by WSL distros and only then that have the non-default metadata config flag enabled. So perhaps this API should just throw on Windows.

BTW I found https://github.com/ladislav-zezula/FileTest a very helpful UI for experimenting with NtQueryEaFile.

tmds · 2022-05-31T09:23:57Z

Although, even if it is documented, it's not clear to me that there is much value in wiring this up for Windows, because it apparently will only enable it for files in the Windows filesystem written by WSL distros and only then that have the non-default metadata config flag enabled.

I also think the value is low.

If a user would like to have the unix permissions with WSL, they can set the mount options (metadata), and use Linux .NET binaries for working with the permissions.

So perhaps this API should just throw on Windows.

Proposing something:

For the APIs that create a file/directory, I think they shouldn't throw because that facilitates using them in cross-platform code.
For the API that set the mode, I think they should throw.
For the APIs that get the mode, I think we should return something. They could return 0777. We can leave out the w bit if they are readonly and return 0555.

danmoseley · 2022-05-31T13:15:19Z

For the APIs that get the mode, I think we should return something. They could return 0777. We can leave out the w bit if they are readonly and return 0555.

That seems to be what chmod will do in WSL on a Windows file if 'metadata' flag is not used. But if it is set, it would now be a misleading result, as it should be interpreting the extended attributes.

stephentoub · 2022-05-31T13:35:16Z

For the APIs that create a file/directory, I think they shouldn't throw because that facilitates using them in cross-platform code.

This is akin to someone asking that files have a specific permission level set and then silently ignoring it, e.g. "create this file so that only I can read it" and then it silently enables everyone to read it. That seems like a security incident waiting to happen.
cc: @GrabYourPitchforks

tmds · 2022-05-31T14:42:26Z

That seems to be what chmod will do in WSL on a Windows file if 'metadata' flag is not used. But if it is set, it would now be a misleading result, as it should be interpreting the extended attributes.

ok, let's avoid confusion and make it throw.

This is akin to someone asking that files have a specific permission level set and then silently ignoring it, e.g. "create this file so that only I can read it" and then it silently enables everyone to read it. That seems like a security incident waiting to happen.

The property/argument is named unixCreateMode to indicate it is only applied on Unix, and only when a new file is created.

If we're not going to ignore it on Windows, a user will need to conditionalize for Windows to make their code work xplat.

stephentoub · 2022-05-31T14:44:02Z

If we're not going to ignore it on Windows, a user will need to conditionalize for Windows to make their code work xplat.

Presumably if they're trying to lock the file down, they'd need to do this anyway to use ACLs on Windows.

tmds · 2022-05-31T15:33:33Z

I like it when code that works for me on Linux doesn't throw PNSE on Windows.

Also, I think/hope a user setting unixCreateMode knows what it means.

And, not all use-cases that require unix permissions need a Windows ACL, like the tar extracting, for example.

I'll implement it to throw PNSE. We can revisit it later, if we want

danmoseley · 2022-05-31T17:01:24Z

I agree with failing on Windows for now -- perhaps we will learn more from WSL team, or their support will evolve, which might make it more interesting.

src/libraries/System.Private.CoreLib/src/System/IO/UnixFileMode.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileSystemInfo.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs

src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs

JeremyKuhne · 2022-06-02T18:28:59Z

To read extended attributes you can use NtQueryEaFile, but it's apparently not documented (only ZwQueryEaFile which drivers can use)

@danmoseley As long as the Zw* version is documented, the Nt* version is fair game.

tmds · 2022-06-21T08:30:52Z

...aphy/src/System/Security/Cryptography/X509Certificates/OpenSslDirectoryBasedStoreProvider.cs

+                    if (actualMode != UserReadWrite)
+                    {
+                        throw new CryptographicException(SR.Format(SR.Cryptography_InvalidFilePermissions, stream.Name));
+                    }


note: I've omitted the owner check that was in EnsureFilePermissions since it is implicit from only the owner having permissions and us being able to open the file.

adamsitnik

LGTM, big thanks for your contribution @tmds!

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

adamsitnik · 2022-06-21T12:29:45Z

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Unix.cs

@@ -53,7 +54,9 @@ public abstract partial class TarEntry
            // If the permissions weren't set at all, don't write the file's permissions.
            if (permissions != 0)
            {
-                Interop.CheckIo(Interop.Sys.FChMod(handle, permissions), destinationFileName);
+#pragma warning disable CA1416 // Validate platform compatibility


@buyaa-n @ViktorHofer the File.SetUnixFileMode method is marked as [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("windows")] and this particular C# file is compiled only for Unix:

runtime/src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj

Lines 52 to 53 in e55011d

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Unix'">

<Compile Include="System\Formats\Tar\TarEntry.Unix.cs" />

Is it expected that the compiler produces warning?

src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetUnixFileMode.cs

src/libraries/System.IO.FileSystem/tests/Directory/CreateDirectory_UnixFileMode.Unix.cs

src/libraries/System.IO.FileSystem/tests/FileStream/ctor_options.Unix.cs

src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamOptions.cs

src/libraries/System.IO.FileSystem/tests/FileSystemTest.cs

adamsitnik · 2022-06-21T14:53:25Z

src/libraries/System.Private.CoreLib/src/System/IO/File.cs

+        [UnsupportedOSPlatform("windows")]
+        public static void SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode mode)
+            => SetUnixFileModeCore(fileHandle, mode);
+


All these methods throw PlatformNotSupportedException on Windows, I think that it would be good to include this information in the docs.

/// <exception cref="PlatformNotSupportedException">Not supported on Windows.</exception>

Does it need to be added when it is marked with the UnsupportedOSPlatformAttribute?

@buyaa-n what is the guidance for such scenarios?

Idk what is the guidance for documenting, though I know that the attribute would showed up on the API doc like this:

https://docs.microsoft.com/en-us/dotnet/api/system.console.in?view=net-6.0

I'm only seeing 11 places that do this in the current codebase, but we throw PNSE in many, many other places. So I'm not sure this is strictly necessary.

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

Co-authored-by: Adam Sitnik <[email protected]>

eerhardt · 2022-06-22T15:06:41Z

...pression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs

@@ -22,7 +22,9 @@ public static partial class ZipFileExtensions
            // include the permissions, or was made on Windows.
            if (permissions != 0)
            {
-                Interop.CheckIo(Interop.Sys.FChMod(fs.SafeFileHandle, permissions), fs.Name);


Can the Interop file be removed from this .csproj?

eerhardt · 2022-06-22T15:12:02Z

src/libraries/System.Formats.Tar/src/System.Formats.Tar.csproj

@@ -59,7 +59,6 @@
    <Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.FChMod.cs" Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />


Interop.FChMod.cs

Can the Interop.FChMod.cs file also be deleted from this .csproj?

eerhardt · 2022-06-22T15:13:34Z

         Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />

Also remove Interop.FChMod.cs from this .csproj

Refers to: src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj:587 in 168bdf5. [](commit_id = 168bdf5, deletion_comment = False)

eerhardt

This looks great! Thank you for all the great work here, @tmds!

My latest comments can be addressed in a follow up PR if we don't want to reset CI and want to merge today in order to make the preview 6 cutoff.

carlossanlop · 2022-06-23T04:33:06Z

@eerhardt @adamsitnik we can merge this and then try to send the backport to prev6 via Tactics. We can request the build wizards to wait for this change before they generate a build.

cc @wtgodbe @vseanreesermsft

* Implement UnixFileMode APIs on Unix. * Throw PNSE on Windows, add UnsupportedOSPlatform. * Fix API compat issue. * Borrow a few things from SafeFileHandle API PR to this compiles. * Fix System.IO.FileSystem.AccessControl compilation. * Add xml docs. * Replace Interop.Sys.Permissions to System.IO.UnixFileMode. * Throw PNSE immediately on Windows. * Add ODE to xml docs of methods that accept a handle. * Don't throw (PNSE) from FileSystemInfo.UnixFileMode getter on Windows. * Minor style fix. * Get rid of some casts. Co-authored-by: Eric Erhardt <[email protected]> * Add tests for creating a file/directory with UnixFileMode. * Some CI envs don't have a umask exe, try retrieving via a shell builtin. * Update expected test mode values. * Fix OSX * Fix Windows build. * Add ArgumentException tests. * Fix Windows build. * Add get/set tests. * Update test for Windows. * Make setters target link instead of link target. * Linux: fix SetUnixFileMode * Fix OSX compilation. * Try make all tests pass in CI. * For link, operate on target permissions. * Skip tests on Browser. * Add tests for 'Get' that doesn't use a 'Set' first. * Don't perform exist check for handles. * Fix Get test for wasm. * Review xml comments. * Add comment to test. * GetUnixFileMode for handle won't throw UnauthorizedAccessException. * Apply suggestions from code review Co-authored-by: Eric Erhardt <[email protected]> * PR feedback. * Update enum doc to say 'owner' instead of 'user'. * Use UnixFileMode in library. * Use UnixFileMode in library tests. * Fix Windows build. * Fix missing FileAccess when changing to FileStreamOptions API. * PR feedback. * Fix Argument_InvalidUnixCreateMode message. Co-authored-by: Adam Sitnik <[email protected]> Co-authored-by: Eric Erhardt <[email protected]> Co-authored-by: Adam Sitnik <[email protected]>

Related dotnet#69980 (comment)

* Implement UnixFileMode APIs (#69980) * Implement UnixFileMode APIs on Unix. * Throw PNSE on Windows, add UnsupportedOSPlatform. * Fix API compat issue. * Borrow a few things from SafeFileHandle API PR to this compiles. * Fix System.IO.FileSystem.AccessControl compilation. * Add xml docs. * Replace Interop.Sys.Permissions to System.IO.UnixFileMode. * Throw PNSE immediately on Windows. * Add ODE to xml docs of methods that accept a handle. * Don't throw (PNSE) from FileSystemInfo.UnixFileMode getter on Windows. * Minor style fix. * Get rid of some casts. Co-authored-by: Eric Erhardt <[email protected]> * Add tests for creating a file/directory with UnixFileMode. * Some CI envs don't have a umask exe, try retrieving via a shell builtin. * Update expected test mode values. * Fix OSX * Fix Windows build. * Add ArgumentException tests. * Fix Windows build. * Add get/set tests. * Update test for Windows. * Make setters target link instead of link target. * Linux: fix SetUnixFileMode * Fix OSX compilation. * Try make all tests pass in CI. * For link, operate on target permissions. * Skip tests on Browser. * Add tests for 'Get' that doesn't use a 'Set' first. * Don't perform exist check for handles. * Fix Get test for wasm. * Review xml comments. * Add comment to test. * GetUnixFileMode for handle won't throw UnauthorizedAccessException. * Apply suggestions from code review Co-authored-by: Eric Erhardt <[email protected]> * PR feedback. * Update enum doc to say 'owner' instead of 'user'. * Use UnixFileMode in library. * Use UnixFileMode in library tests. * Fix Windows build. * Fix missing FileAccess when changing to FileStreamOptions API. * PR feedback. * Fix Argument_InvalidUnixCreateMode message. Co-authored-by: Adam Sitnik <[email protected]> Co-authored-by: Eric Erhardt <[email protected]> Co-authored-by: Adam Sitnik <[email protected]> * UnixFileMode: address remaining feedback. (#71188) Co-authored-by: Tom Deseyn <[email protected]> Co-authored-by: Eric Erhardt <[email protected]>

carlossanlop · 2022-06-23T18:24:00Z

Hey @tmds, from this discussion #69980 (comment)

I can take care of replacing TarFileMode with UnixFileMode, since I am still actively working on Tar stuff. Just wanted to let you know so we don't step on each other toes.

Implement UnixFileMode APIs on Unix.

11c2422

ghost added the community-contribution Indicates that the PR has been added by a community member label May 30, 2022

dotnet-issue-labeler bot added area-System.IO new-api-needs-documentation labels May 30, 2022

eerhardt reviewed Jun 1, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/UnixFileMode.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/FileSystemInfo.cs Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs Outdated Show resolved Hide resolved

Throw PNSE on Windows, add UnsupportedOSPlatform.

b182b4d

tmds commented Jun 2, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/Directory.cs Show resolved Hide resolved

tmds commented Jun 2, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamOptions.cs Show resolved Hide resolved

tmds commented Jun 2, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs Show resolved Hide resolved

tmds added 4 commits June 2, 2022 13:30

Fix API compat issue.

f143499

Borrow a few things from SafeFileHandle API PR to this compiles.

5138601

Fix System.IO.FileSystem.AccessControl compilation.

4d6c524

Add xml docs.

813e24a

eerhardt reviewed Jun 2, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Unix.cs Outdated Show resolved Hide resolved

tmds added 4 commits June 3, 2022 09:12

Replace Interop.Sys.Permissions to System.IO.UnixFileMode.

7bd2ae3

Throw PNSE immediately on Windows.

d67d738

Add ODE to xml docs of methods that accept a handle.

3841bfb

Don't throw (PNSE) from FileSystemInfo.UnixFileMode getter on Windows.

d1e7ea8

tmds commented Jun 21, 2022

View reviewed changes

tmds added 3 commits June 21, 2022 11:02

Use UnixFileMode in library tests.

7046ea9

Fix Windows build.

e55011d

Fix missing FileAccess when changing to FileStreamOptions API.

bc7383b

adamsitnik approved these changes Jun 21, 2022

View reviewed changes

PR feedback.

d1f043d

tmds force-pushed the filemode branch from 0496e08 to d1f043d Compare June 22, 2022 09:21

adamsitnik reviewed Jun 22, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

Fix Argument_InvalidUnixCreateMode message.

168bdf5

Co-authored-by: Adam Sitnik <[email protected]>

adamsitnik added this to the 7.0.0 milestone Jun 22, 2022

eerhardt reviewed Jun 22, 2022

View reviewed changes

eerhardt approved these changes Jun 22, 2022

View reviewed changes

carlossanlop approved these changes Jun 23, 2022

View reviewed changes

carlossanlop merged commit ddc4f95 into dotnet:main Jun 23, 2022

teo-tsirpanis mentioned this pull request Jun 23, 2022

[API Proposal]: APIs for working with unix file mode #67837

Closed

tmds mentioned this pull request Jun 23, 2022

UnixFileMode: address remaining feedback. #71188

Merged

adamsitnik mentioned this pull request Jun 23, 2022

[release/7.0-preview6] UnixFileMode backport #71199

Merged

ViktorHofer added a commit to ViktorHofer/runtime that referenced this pull request Jun 23, 2022

Update SupportedPlatform heuristic

65f3025

Related dotnet#69980 (comment)

ViktorHofer mentioned this pull request Jun 23, 2022

Update SupportedPlatform heuristic ViktorHofer/runtime#2

Closed

carlossanlop mentioned this pull request Jun 23, 2022

Replace TarFileMode with UnixFileMode #71222

Merged

derwasp mentioned this pull request Jun 24, 2022

Make the archives created on Unix have the read permissions by default #70735

Merged

ViktorHofer mentioned this pull request Jun 24, 2022

SupportedPlatform calculation for platform compatibility analyzer is wrong #71251

Open

danmoseley mentioned this pull request Jul 14, 2022

What's new in .NET 7 Preview 7 [WIP] dotnet/core#7455

Closed

ghost locked as resolved and limited conversation to collaborators Jul 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement UnixFileMode APIs #69980

Implement UnixFileMode APIs #69980

tmds commented May 30, 2022

dotnet-issue-labeler bot commented May 30, 2022

ghost commented May 30, 2022

tmds commented May 30, 2022

danmoseley commented May 31, 2022 •

edited

Loading

tmds commented May 31, 2022

danmoseley commented May 31, 2022

stephentoub commented May 31, 2022

tmds commented May 31, 2022

stephentoub commented May 31, 2022

tmds commented May 31, 2022

danmoseley commented May 31, 2022

JeremyKuhne commented Jun 2, 2022

tmds Jun 21, 2022

adamsitnik left a comment

adamsitnik Jun 21, 2022

adamsitnik Jun 21, 2022

tmds Jun 21, 2022

adamsitnik Jun 21, 2022

buyaa-n Jun 21, 2022

eerhardt Jun 22, 2022

eerhardt Jun 22, 2022

eerhardt Jun 22, 2022

eerhardt commented Jun 22, 2022

eerhardt left a comment

carlossanlop commented Jun 23, 2022

carlossanlop commented Jun 23, 2022

	<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Unix'">
	<Compile Include="System\Formats\Tar\TarEntry.Unix.cs" />

		@@ -59,7 +59,6 @@
		<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.FChMod.cs" Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />

Implement UnixFileMode APIs #69980

Implement UnixFileMode APIs #69980

Conversation

tmds commented May 30, 2022

dotnet-issue-labeler bot commented May 30, 2022

ghost commented May 30, 2022

tmds commented May 30, 2022

danmoseley commented May 31, 2022 • edited Loading

tmds commented May 31, 2022

danmoseley commented May 31, 2022

stephentoub commented May 31, 2022

tmds commented May 31, 2022

stephentoub commented May 31, 2022

tmds commented May 31, 2022

danmoseley commented May 31, 2022

JeremyKuhne commented Jun 2, 2022

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Jun 22, 2022

eerhardt left a comment

Choose a reason for hiding this comment

carlossanlop commented Jun 23, 2022

carlossanlop commented Jun 23, 2022

danmoseley commented May 31, 2022 •

edited

Loading