Compression.ZipArchive: Expose ExternalFileAttributes for Entries

[ianhays]

Problem

The ExternalFileAttributes field of a ZipArchiveEntry is currently hidden within the implementation and not accessible. This has been fine in the Windows world since it's not used but it is used by Unix's zip/unzip programs to store file permissions of an entry. As it currently stands, there is no way to carry Unix RWX permissions within a zip, so at unzip time the unzipper has to guess at permissions and manually chmod the entries.

https://github.com/dotnet/corefx/issues/14853
https://github.com/dotnet/corefx/issues/15516
https://github.com/dotnet/corefx/issues/10223
NuGet/Home#4424

Solution

Permissions isn't an officially supported feature of the zip specification but it is used widely enough that we should add support for it. There's a pretty wide range of things we can do here that go from simply exposing the field all the way to implementing AccessControl for Unix and accepting something from there as input. Since the latter option requires a huge amount of work/new API and marries us to a Zip permissions structure, I suggest we take the simple/future-proof route here and just expose the entire field:

public partial class ZipArchiveEntry
{
    public int ExternalAttributes {get; set}
}

This also has the benefit of potentially supporting other implementations that use the external file attributes field for reasons other than permissions. I'm not aware of any of these off the top of my head, but they ostensibly exist.

Usage

The unfortunate aspect of exposing the raw field is that usage becomes a bit more complicated. However, without implementing our own UnixPermissions structure that's somewhat unavoidable. The nice thing is that it leaves us open to implement an explicit UnixPermissions structure in the future (pending the completion of #17540) without conflictions with the ExternalAttributes.

This StackOverflow post does a better job describing the format than I can so I'm just going to link it. The permissions values themselves are the same as the ones you get from stat.

cc: @stephentoub @JeremyKuhne

[eerhardt]

I like it. Thanks for writing this up.

I also like that making me happy is a benefit. 😆

[eerhardt]

One quick question, the zip spec you pointed out just lists the "external file attributes" as "4 bytes". Would it make more sense to expose this as a uint?

[ianhays]

Would it make more sense to expose this as a uint?

It would make more sense, yes. I get yelled at with CLS-Compliance errors when I switch it to uint though.

[eerhardt]

I get yelled at with CLS-Compliance errors when I switch it to uint though.

😢

[stephentoub]

I get yelled at with CLS-Compliance errors when I switch it to uint though

@terrajobst, how much do we care about CLS compliance around exposing uint from public surface area?

[ianhays]

This is ready to go when @terrajobst decides on the int/uint API, so it'll be in for 2.0.0.

[terrajobst]

We've just discussed this in great detail. There are multiple layers to this feature:

1. Exposing the external attributes and round tripping it. We've no concerns and we believe this is a good idea.
2. Recording permissions when zipping up on Unix. Seems useful, but we may want to make this opt-in or at least opt-out.
3. Applying permission when extracting on Unix. Seems useful, but we may want to make this opt-in or at least opt-out.

The concern would be that (2) and (3) seems to complicate the API quit a bit. We're hesitant to just do it in all cases, because we can't easily change that later (without changing behavior). It seems the narrow is a bit too narrow to be worth adding new APIs. Thus, we should split this issue in (1) and (2)/(3). We'd just approve (1).

Questions:

• Are there any other usages of external attributes that we may want to expose later? If so, we should design APIs with that in mind.
• How do most zip/unzip libraries and tools on Unix/Linux deal with permissions?

[ianhays]

We've no concerns and we believe this is a good idea.
Thus, we should split this issue in (1) and (2)/(3). We'd just approve (1).

Thanks for taking a look @terrajobst, I've split the ZipFile support to https://github.com/dotnet/corefx/issues/17342 and edited the OP to only be for the ExternalAttribute exposure.

[eerhardt]

@terrajobst - you didn't answer the questions:

Would it make more sense to expose this as a uint?
how much do we care about CLS compliance around exposing uint from public surface area?

[terrajobst]

Would it make more sense to expose this as a uint?

Good point, we discussed this and decided that uint is generally not worth the hassle. Especially for APIs that are essentially pass through for most devs. So we approved this API shape:

public partial class ZipArchiveEntry
{
    public int ExternalAttributes {get; set}
}

We still care about CLS compliance.

[danmoseley]

Leaving in 2.0 for a couple more days since @ianhays says he has the change ready :)

[ianhays]

Sure do! It'll be up today.