Compression.ZipFile support for Unix Permissions

[ianhays]

Split from dotnet/corefx#17067

Proposal

We should add support in the Unix ZipFile for working with zips that contain entries with associated file permissions information. Right now the only way to capture or check the permissions info of an entry is to do some manual bit shifting of the ExternalAttributes field. We can do better than that.

Wanting your zips to unzip with the same permissions they were zipped with is a very common scenario that we do not currently support. I propose that we do two things:

1. Make our ZipFile.Create* Apis check the permissions of a file (on Unix) and store them in the entries
2. Add new ZipFile.Extract* overloads to apply the permissions stored in a zip to the output files on disk.

New API

namespace System.IO.Compression
{
    public static partial class ZipFile
    {
        public static void ExtractToDirectory(string sourceArchiveFileName, string destinationDirectoryName, System.Text.Encoding entryNameEncoding = null, bool overwriteFiles = false, bool preservePermissions = false) { }
    }
    public static partial class ZipFileExtensions
    {
        public static void ExtractToDirectory(this System.IO.Compression.ZipArchive source, string destinationDirectoryName, bool overwriteFiles = false, bool preservePermissions = false) { }
        public static void ExtractToFile(this System.IO.Compression.ZipArchiveEntry source, string destinationFileName, bool overwrite = false, bool preservePermissions = false) { }
    }
}

Implementation

The follow-through here is relatively easy - stat files on creation, chmod them on extraction. The somewhat bothersome part is that we will need to make the System.IO.Compression.ZipFile assembly platform-specific because the new permissions support will be Unix-only.

Work-in-Progress solution available here: https://github.com/ianhays/corefx/commits/zipfile_perms

Benefit

Zips created using ZipFile on Unix will be usable interchangeably with those created using the zip command as far as permissions are concerned. Using ZipFile to unzip zips created with zip will create files that have the same permissions as they were created with. Makes our Unix users happy. @eerhardt is happy.

PTAL: @stephentoub @JeremyKuhne @terrajobst @eerhardt @ViktorHofer

[peterhuene]

Users also run into this issue when attempting to use unzip on a NuGet package that was originally created with ZipArchive. See https://github.com/dotnet/cli/issues/9410.

[ianhays]

@stephentoub @JeremyKuhne @terrajobst @eerhardt @ViktorHofer this is now ready for review, so please take a look.

I have a changeset with an implementation nearly ready to go, so I'd like to have this reviewed soon i.e. before I go on paternity leave :)

[JeremyKuhne]

What is the expected behavior if you try to do this on Windows? PlatformNotSupported?

[ianhays]

On Windows, the value of preservePermissions is ignored.

[eerhardt]

Make our ZipFile.Create* Apis check the permissions of a file (on Unix) and store them in the entries

I see there are no API changes proposed for this behavior. Is the expectation that we will always store the permissions on Unix? Or will there be a way to opt out?

[ianhays]

Is the expectation that we will always store the permissions on Unix? Or will there be a way to opt out?

Correct, we will always store the permissions when creating a zip on a Unix machine, such that roundtripping using zip or unzip in combination with ZipFile retains permissions of the stored files.

There are a few reasons why I want this to be the default behavior:

1. Including permissions by default is how info-zip (unix zip) does it, and since that's the de-facto zipper on Unix I want our behavior to match it as closely as possible
2. Capturing the file permissions in the zip entries is a very low cost procedure
3. The file permissions are ignored on non-Unix platforms

I'm not against adding an opt-out, but I've got a lot of upcoming ZipFile APIs with new behavioral options, so the API space is going to get big fast. Another option would be instead of adding a bunch of bools we can do a ZipFileOptions e.g.

public class ZipFileOptions
{
    bool Overwrite = false;
    bool preserveUnixPermissions = true;
    Encoding encoding = null;
    CompressionLevel compressionLevel = CompressionLevel.Optimal;
}

public static partial class ZipFile
{
...
    public static void CreateFromDirectory(string sourceDirectoryName, string destinationArchiveFileName, ZipFileOptions options) {}
    public static void ExtractToDirectory(string sourceArchiveFileName, string destinationDirectoryName, ZipFileOptions options) {}
...
}

public class ZipFileTests
{
...
CreateFromDirectory(somedir, outputzip, new ZipFileOptions() {overwrite = false, preserveUnixPermissions = false});
...
}

[eerhardt]

I'm fine with not having an opt out, until we have a scenario for it.

[ianhays]

Works for me :)

[terrajobst]

We'd prefer the default to be "preserve executable bit" on compression. We're leaning towards saying the same about extraction.

Open questions:

1. Is this feasible? What do other zip tools under Linux/Mac do?
2. Assuming we'd change the behavior of the existing APIs, do we still need APIs to control this behavior? If so, we should reverse the meaning of the parameter and default it to false.

@ianhays , could you follow up?

[danmoseley]

@ianhays is on leave until late Sept. So if someone cares about this, they should look at these open questions (or ping him then...)

[ianhays]

Is this feasible? What do other zip tools under Linux/Mac do?

Other zip tools all store the full permissions set when packaging and apply them when opening.

Assuming we'd change the behavior of the existing APIs, do we still need APIs to control this behavior? If so, we should reverse the meaning of the parameter and default it to false.

If we are fine with storing and applying the permissions every time, then the extra API is unnecessary.

We'd prefer the default to be "preserve executable bit" on compression. We're leaning towards saying the same about extraction.

@terrajobst Only the executable bit? I don't think I've seen that before. Seems like full RWX would be better.

[joshfree]

Tentatively moving this to the 3.0 milestone for better tracking

[danmoseley]

This is DCR level work at this point. Moving to Future.

[carlossanlop]

Triage:
There is a convention to save unix permissions in a zip file, so its feasible.

Note: We could also expose the permissions in the ZipArchiveEntry.

[eerhardt]

We could also expose the permissions in the ZipArchiveEntry.

We are already exposing ExternalAttributes on ZipArchiveEntry, which contains the permissions.

runtime/src/libraries/System.IO.Compression/ref/System.IO.Compression.cs

Line 110 in 4f9ae42

public int ExternalAttributes { get { throw null; } set { } }

It was done with dotnet/corefx#18565

[marcwittke]

How is it supposed to be used? Like this? I don't get it 🤷‍♂️

ZipArchiveEntry entry = zipArchive.CreateEntry("file.txt");
entry.ExternalAttributes = 644;

[svick]

@marcwittke The original issue (#20603) links to a Stack Exchange answer, which explains the format. It's certainly not as simple as using the octal notation of the permissions you want.

[marcwittke]

yeah, found it, read the cryptic part (hey, I am writing code in .net for not having to shift bits 🤣 ) and came up with this:

entry.ExternalAttributes = entry.ExternalAttributes | (Convert.ToInt32("664", 8) << 16);

key is that the permission is an octal literal, that does not exist out-of-the-box in .net.

[RoguePointer80]

This issue is now more than 3 years old, and the implementation is not that difficult; the conclusion from the discussion was that no API change was needed at this point since permissions save/apply on Unix is by-default, like other Unix tools (ZipInfo).
I see that Ian Hays started the work a while back, maybe I can get inspiration from that work and start a new branch.
Is there anything special holding back this issue?

[eerhardt]

Makes our Unix users happy. @eerhardt is happy.

4 years later, @eerhardt is happy 😃

[jeffhandley]

Thanks for doing this, @eerhardt!!