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

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

Merged

Conversation

derwasp
Copy link
Contributor

@derwasp derwasp commented Jun 14, 2022

Fixes #17912

Proposed changes

  • When creating a ZipArchiveEntry in the Unix environment, always set the default file permissions to be 644 and directory permissions to be 755 in order for the archives to be usable after running an unzip command on them.
  • Always override the default permissions of the ZipArchiveEntry when the archive is created from the actual files and not from memory, where the permissions are not available.

Background

Recently we hit an issue where our customers saw that the artifact files in the zip archives that we produce have the permissions set to 000 if our system runs on Unix machines. When the unzip command is used, these permissions translate to the actual files also having the same permissions.
This was an inconvenience to them as if the same system is run on Windows, the files would have the 644 permissions. The customers wanted this to have the same behavior.

Causes

Based on how I see it, the zip archives have two fields that can be responsible for the file permissions.

Version made by

One of them is Version made by. (See the Central directory file header section in the wiki page ) This is what the comment says about this field in the code:

The upper byte of the "version made by" flag in the central directory header of a zip file represents the
OS of the system on which the zip was created. Any zip created with an OS byte not equal to Windows (0)
or Unix (3) will be treated as equal to the current OS.
The value of 0 more specifically corresponds to the FAT file system while NTFS is assigned a higher value. However
for historical and compatibility reasons, Windows is always assigned a 0 value regardless of file system.

So to simplify the story, we can say that we have two possible options here: 0 for Windows and 3 for Unix.

This field is written here: ZipArchiveEntry.cs#L517

The unzipping software is using this field to determine that the permissions were present in the source system. In other words: we probably need to set the permissions to 644 on the Unix system if the archive was created on Windows, where these permissions don't exist. And this is exactly the behavior with unzip that we see.

External attributes

Another field is External attributes. This field is used to store in the file permissions when the archives are created on the Unix systems. Recently, the API that works with files received support for this: PR.
As you can see from the PR, the permissions are based on the actual permissions from the files:

    static partial void SetExternalAttributes(FileStream fs, ZipArchiveEntry entry)
    {
        Interop.Sys.FileStatus status;
        Interop.CheckIo(Interop.Sys.FStat(fs.SafeFileHandle, out status), fs.Name);

        entry.ExternalAttributes |= status.Mode << 16;
    }

So now from the perspective of the unzipping software if we have a Version made by set to 3 (meaning Unix), we need to read the External attributes and apply the corresponding permissions to the files. And if Version made by is set to 0 (meaning Windows) we need determine the permissions ourself and probably set it to 644.

The issue

If the file API is used, then everything already works: the permissions are preserved and when the unpacking software is going to work with the archive it would create the files with same permissions as before.
The caviat is that when we are creating a new ZipArchiveEntry from memory on the Unix machines. In this case, there's no file to begin with, but this new ZipArchiveEntry can still be unzipped using other software, such as unzip, which would follow the logic based on the Version made by flag.

When this happens, currently, we would create an archive which, when unzipped, would result in files having 000 permissions if our zipping logic runs on Windows and 644 (probably depends on the implementation of the unzipping software) if our zipping logic runs on Unix.
As noted by @tmds we should also set the permissions to 755 for the directories as in Unix the execute permissions is required to cd into the folder.

Even if this is not techincally a bug, at least, this behavior is not consistent across the two platforms from the users point of view.

A minimal repro looks like this:

using System.IO.Compression;

using var fileStream = new FileStream(@"test.zip", FileMode.Create);
using var archive = new ZipArchive(fileStream, ZipArchiveMode.Create, true);

ZipArchiveEntry demoFile = archive.CreateEntry("inmemory_file.txt");

using var entryStream = demoFile.Open();
using var streamWriter = new StreamWriter(entryStream);
streamWriter.Write("Hello zip!");

Console.WriteLine("Done.");

After running this on a Unix machine and unizipping the result with unzip test.zip you can do an ls -la and you would see something like this:

 xxxx@xxxx  ~/Projects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0  ls -la
drwxr-xr-x  10 derwasp  staff     320 Jun 14 14:48 .
drwxr-xr-x   3 derwasp  staff      96 Jun 13 16:42 ..
-rwxr-xr-x   1 derwasp  staff  132208 Jun 14 14:48 ConsoleApp1
-rw-r--r--   1 derwasp  staff     403 Jun 14 14:48 ConsoleApp1.deps.json
-rw-r--r--   1 derwasp  staff    5120 Jun 14 14:48 ConsoleApp1.dll
-rw-r--r--   1 derwasp  staff   10480 Jun 14 14:48 ConsoleApp1.pdb
-rw-r--r--   1 derwasp  staff     139 Jun 14 14:48 ConsoleApp1.runtimeconfig.json
----------   1 derwasp  staff      10 Jun 14 14:48 inmemory_file.txt
drwxr-xr-x   3 derwasp  staff      96 Jun 14 14:48 ref
-rw-r--r--   1 derwasp  staff     150 Jun 14 14:48 test.zip

And you can see that the file permissions are set to 000.

Links

The original comment is here: #17912 (comment)

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 14, 2022
@dnfadmin
Copy link

dnfadmin commented Jun 14, 2022

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Jun 14, 2022

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

Issue Details

Fixes #17912

I will make this description proper once we finalize the approach, but for now the discussion is happening here: More details in this comment: #17912 (comment)

Author: derwasp
Assignees: -
Labels:

area-System.IO.Compression

Milestone: -

@derwasp derwasp changed the title Derwasp/zip archive entry unix permissions Make the archives created on Unix have the read permissions by default Jun 14, 2022
@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch from e897639 to 08efabb Compare June 14, 2022 16:03
@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch 2 times, most recently from b67b9d5 to 540e4e6 Compare June 17, 2022 12:07
@tmds
Copy link
Member

tmds commented Jun 18, 2022

Is something needed to ensure proper directory permissions?

@derwasp
Copy link
Contributor Author

derwasp commented Jun 22, 2022

@tmds I used my example from the issue comment and modified it to have a line that adds an archive folder like so:

using System.IO.Compression;
using ConsoleApp1;

using var fileStream = new FileStream(@"test.zip", FileMode.Create);
using var archive = new ZipArchive(fileStream, ZipArchiveMode.Create, true);

archive.CreateEntry("folder/").EnsureReadPermissions();
ZipArchiveEntry demoFile = archive.CreateEntry("inmemory_file.txt").EnsureReadPermissions();

using var entryStream = demoFile.Open();
using var streamWriter = new StreamWriter(entryStream);
streamWriter.Write("Hello zip!");

Console.WriteLine("Done.");

After running this and unzipping this is the result:

derwasp@xxxxx  ~/RiderProjects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0/out  ls -la
    drwxr-xr-x   4 derwasp  staff  128 Jun 22 09:23 .
    drwxr-xr-x  11 derwasp  staff  352 Jun 22 09:23 ..
    drw-r--r--   2 derwasp  staff   64 Jun 22 09:22 folder           <-- permissions
    -rw-r--r--   1 derwasp  staff   10 Jun 22 09:22 inmemory_file.txt

And this is the result if I don't see the permissions to the folder:

derwasp@xxxxx  ~/RiderProjects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0/out  ls -la
    drwxr-xr-x   4 derwasp  staff  128 Jun 22 09:23 .
    drwxr-xr-x  11 derwasp  staff  352 Jun 22 09:23 ..
    d---------   2 derwasp  staff   64 Jun 22 09:23 folder           <-- permissions
    -rw-r--r--   1 derwasp  staff   10 Jun 22 09:23 inmemory_file.txt

So this logic should fix the permissions for both, files and folders.

@tmds
Copy link
Member

tmds commented Jun 22, 2022

@derwasp thanks for checking.

For directories, we'll want to set the x bit. That is: use rwxr-xr-x. Otherwise a user can't cd into the extracted folders.

@derwasp
Copy link
Contributor Author

derwasp commented Jun 22, 2022

@tmds oh this is a very good catch, I didn't know that r is not enough for the directories!

/// For Unix the default external attributes are an equivalent of (Convert.ToInt32("755", 8) &lt;&lt; 16)
/// where 755 are rwxr-xr-x unix file permissions.
/// </summary>
internal const uint DefaultDirectoryExternalAttributes = 493 << 16;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlossanlop similar to the change here, we probably need a default for directories also in the Tar implementation.

@tmds
Copy link
Member

tmds commented Jun 22, 2022

Looking at this PR made me realize Zip doesn't store/restore directory permissions.
I've created a ticket for that: #71140.

@tmds
Copy link
Member

tmds commented Jun 22, 2022

@derwasp can you add a test that verifies the default attributes are used?

@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch from 525315d to c8f793d Compare June 23, 2022 11:48
@derwasp
Copy link
Contributor Author

derwasp commented Jun 23, 2022

@tmds I made a test to verify the file permissions. Please see if you have any more suggestions.
And thank you for your help with this PR.

@derwasp derwasp marked this pull request as ready for review June 23, 2022 12:04
@derwasp derwasp requested review from danmoseley and tmds June 23, 2022 12:48
await CreateArchive();
await UnpackArchive();

EnsureFilePermissions(Path.Combine(outputFolder, nestedFolder, file), "644");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and check folder is 755?

Copy link
Contributor Author

@derwasp derwasp Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, yes. But with the current unzipping implementation, we can't do that: the folders are just created using Directory.CreateDirectory . When unzip is used, the permissions are fully respected of course.
IMO changing the unzipping logic fits this issue better:

EDIT
Just to clarify: the way it currently works already sets enough permissions on the folders so that the files are acccessible, so the original issue described in #17912 is not affecting us here. However, these are not the correct exact permissions which we set in the archive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reduce what these tests are doing to: creating a ZipArchiveEntry, and verifying ExternalAttributes has the expected value.
That will also work for directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you say if I will just add a separate test to verify this for both directories and files? I would still love to have the test I wrote to test the roundtrip.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing UnixExtractSetsFilePermissionsFromExternalAttributes verifies ExternalAttributes gets stored and restored.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmds I made a very simple test instead of the one I had previously. Is this what you had in mind?

@derwasp derwasp requested a review from danmoseley June 24, 2022 06:39
/// For Unix the default external attributes are an equivalent of (Convert.ToInt32("644", 8) &lt;&lt; 16)
/// where 644 are rw-r--r-- unix file permissions.
/// </summary>
internal const uint DefaultFileExternalAttributes = 420 << 16;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@derwasp can you use the UnixFileMode that got just merged, and use the same style as:

internal const UnixFileMode DefaultMode = // 644 in octal
UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.GroupRead | UnixFileMode.OtherRead;

@carlossanlop @eerhardt @danmoseley should we try to put these consts in a file common to the Tar and Zip implementations to ensure they use the same values?

Copy link
Contributor Author

@derwasp derwasp Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmds Yeah, I just saw that your PR went in, awesome timing! Looking at the API right now.

/// The default external file attributes are used to support zip archives on multiple platforms.
/// Since Windows doesn't use file permissions, there's no default value needed.
/// </summary>
internal const uint DefaultFileExternalAttributes = 0;
Copy link
Member

@tmds tmds Jun 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious. What happens when you extract the ZipVersionMadeByPlatform.Windows file on Unix using the system tools. What permissions do you get for files, and for directories?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// If the permissions weren't set at all, don't write the file's permissions,
// since the .zip could have been made using a previous version of .NET, which didn't
// include the permissions, or was made on Windows.
if (permissions != 0)
{
#pragma warning disable CA1416 // Validate platform compatibility
File.SetUnixFileMode(fs.SafeFileHandle, (UnixFileMode)permissions);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmds it took me a while to test this: was trying to understand how to set up the dev environment on my old windows laptop.

Here's the result unpacked on a mac machine:

 derwasp@xxxxx  ~/Downloads/ziptest > unzip archive_created_on_win.zip -d win_arch
Archive:  archive_created_on_win.zip
   creating: win_arch/folder/
  inflating: win_arch/folder/inmemory_file.txt
 derwasp@xxxxx  ~/Downloads/ziptest > cd win_arch
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch > ls -la
total 0
drwxr-xr-x   3 derwasp  staff   96 Jun 24 22:49 .
drwxr-xr-x  14 derwasp  staff  448 Jun 24 22:49 ..
drwxr-xr-x   3 derwasp  staff   96 Jun 24 13:45 folder
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch > cd folder
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch/folder > ls -la
total 8
drwxr-xr-x  3 derwasp  staff  96 Jun 24 13:45 .
drwxr-xr-x  3 derwasp  staff  96 Jun 24 22:49 ..
-rw-r--r--  1 derwasp  staff  56 Jun 24 13:45 inmemory_file.txt

I also verified that the default values are not set like so:

var arch = "/Users/derwasp/Downloads/ziptest/archive_created_on_win.zip";

await using (var fileStream = new FileStream(arch, FileMode.Open))
using (var archive = new ZipArchive(fileStream, ZipArchiveMode.Read, true))
{
    foreach (var e in archive.Entries)
    {
        Console.WriteLine($"{e.FullName}, \t\t attributes = {(e.ExternalAttributes >> 16)}");

    }
}

And this is the output:

folder/,                 attributes = 0
folder/inmemory_file.txt,                attributes = 0

So the unzip utility does it the way that we expected: 644 for the files and 755 for the folders.

@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch from c8f793d to 0df9a77 Compare June 24, 2022 09:06
@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch 2 times, most recently from 7c2cdb2 to fb48511 Compare June 28, 2022 07:10
@derwasp derwasp requested review from tmds and eerhardt June 28, 2022 08:47
@@ -115,7 +117,11 @@ internal ZipArchiveEntry(ZipArchive archive, string entryName)

_compressedSize = 0; // we don't know these yet
_uncompressedSize = 0;
_externalFileAttr = 0;
var defaultEntryPermissions = entryName.EndsWith(Path.DirectorySeparatorChar) || entryName.EndsWith(Path.AltDirectorySeparatorChar)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eerhardt if you're adding stuff from file system, you go through a function that replaces Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar with /:

// '/' is a more broadly recognized directory separator on all platforms (eg: mac, linux)
// We don't use Path.DirectorySeparatorChar or AltDirectorySeparatorChar because this is
// explicitly trying to standardize to '/'
for (int i = 0; i < length; i++)
{
char ch = buffer[i];
if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar)
buffer[i] = PathSeparatorChar;
}

On Linux, \ is a filename char, and we're replacing it there. Maybe we shouldn't?

For entries created using ZipArchive.CreateEntry we're not performing any substitutions.
Maybe we assume the user of this API uses proper separators?
Should we limit to the Path.DirectorySeparatorChar for this check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit to the Path.DirectorySeparatorChar for this check?

On Unix, both Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar are /.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the checks are fine then.

The only thing to consider is also to the substitution on the ZipArchive.CreateEntry, so that on Windows, \ gets replaced by /.

@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch from 22fe615 to 71cc5bd Compare June 29, 2022 11:28
Move the new zip constants to the common folder and use the constants files in both IO.Compression and IO.Compression.ZipFile
@derwasp derwasp force-pushed the derwasp/zip-archive-entry-unix-permissions branch from 71cc5bd to 5c8920b Compare June 29, 2022 11:31
@derwasp derwasp requested review from eerhardt and tmds June 30, 2022 10:22
Copy link
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @derwasp!

@adamsitnik
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you @derwasp for the contribution!

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution @derwasp !

@adamsitnik adamsitnik merged commit bce9415 into dotnet:main Jul 1, 2022
@adamsitnik adamsitnik added this to the 7.0.0 milestone Jul 1, 2022
@derwasp derwasp deleted the derwasp/zip-archive-entry-unix-permissions branch July 1, 2022 16:12
@derwasp
Copy link
Contributor Author

derwasp commented Jul 1, 2022

Thank you for merging this in and for your help @tmds @eerhardt @adamsitnik :)

@danmoseley
Copy link
Member

@derwasp we'd welcome another contribution if you're interested!
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs

@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZipArchive permissions on linux
7 participants