Hi Langer, Thanks for working on this issue. I will take a look into the implementation details next week. Here are two comments regarding the "direction/approach". (1) There is a "masked" security concern regarding adding the "ownership/permission" into the jar file. "Security concern: The current signed jar spec only protects the name and content of the jar entries. If any extra info is added, its value will not be used in the signing/verifying process. This means anyone can change these info in a signed jar and the user has no way to check if the file is genuine." This is one of the reasons that it has been debated whether or not it is worth the effort to add these bits, given the permission and ownership are really platform/host/config dependent (and the jars are being downloaded/copied around across the systems with various different setup) and you really need a "root" permission to really take advantage of these bits. (2) Regarding the implementation whats the motivation of use the high byte of the "external file attributes" vs to use the info-zip as suggested in the report? I've not looked into zip/unzip implementation, which one is zip/unzip using? "A group known as INFO-zip has devised a number of different extensions for ZIP for Unix. Their first and second extension attempts added support for UID and GID but not permissions. The third Unix extension, also known as the "ASi" or "un" extension, provides for file permissions and symlinks also. These extensions have become de-facto standards, and have not changed now since 1996." -Sherman On 9/25/18, 7:57 AM, Langer, Christoph wrote:

Hi all,

I had asked for opinions regarding adding posix permission support to JDK’s zip handling libraries and tools [1]. Since I didn’t get clear “no, you can’t do this” answers, I’m now concretely proposing some enhancements in the area of java.util.zip, jdk.zipfs and jdk.jartool.

I have reopened the long standing bug report (6194856) which had been set to “Won’t fix” quite recently for this purpose.

Here are the technical details:

The ZIP format specifications by infozip and pkware ([2] and [3]) do not explicitly specify the handling of posix file permissions. But it seems to be common sense that if file attribute compatibility is set to “Unix” in the upper byte of CEN field “version made by”, the file permissions bits are stored in CEN field “external file attributes”, byte 3 and 4. My changes try to honor this in the least obtrusive way. 😊

The following changes are proposed:

java.util.zip.ZipEntry:

it will have an additional field “posixPerms”. A value of -1 means “no permission information”, positive values will contain the flag values.

There will be 2 new public methods to read/set the permission information:

public Optional<Set<PosixFilePermission>> getPosixPermissions()

public void setPosixPermissions(Set<PosixFilePermission> permissions)

The usage of type “Optional” reflects that posix permissions are not necessarily present in a zip file

java.util.zip.ZipFile:

it will have the capability to read the CEN fields and set posixPerms if available

java.util.zip.ZipOutputStream:

it will store entries with associated posix permissions as unix type in the CEN, together with the bit mask for the permissions

jdk.jartool:

I propose to add and option "--preserve-posix" or short "-o" to honor the posix bits that may be stored inside zip/jar files. By default the option is not set and hence posix permissions are ignored. If the flag is set and the file system that the jar tool is running on supports posix, posix file permissions that exist in the file system will be stored in newly created/update archives or restored to the file system if such information is present in the archive.

jdk.zipfs:

I added the capability for posix file permissions in the implementation. I decided to support PosixFileAttributes by subclassing ZipFileAttributes from this superclass as well as subclassing ZipFileAttributeView from PosixFileAttributeView. However, as PosixFileAttributes also include groups and owners, I would throw UnsupportedOperationExceptions in case of invoking methods to handle these attributes. But this approach seems to be most consistent with e.g. Files.setPosixFilePermissions and Files.getPosixFilePermissions.

java.nio.file.attribute.PosixFilePermissions:

As this class presents a collection of static helpers, I added definitions for the posix file bit masks together with methods to convert between Sets of PosixFilePermission to bit masks containing the according switches and vice versa. These definitions could theoretically also be moved inside the java.util.zip or jdk.zipfs implementations where they wouldn’t be exposed as public APIs. However, in that case the code would probably need to be duplicated.

I’ve also created two jtreg testcases for both, java.util.zip and jdk.nio.zipfs.

The changes also contain a few further code cleanups.

Here are the links:

Bug: https://bugs.openjdk.java.net/browse/JDK-6194856

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/6194856.0/ <http://cr.openjdk.java.net/%7Eclanger/webrevs/6194856.0/>

I’ll write a CSR once there’s some substantial feedback to my endeavor.

Thanks in advance for reviewing/commenting.

Best regards

Christoph

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055375.h...

[2] http://www.info-zip.org/doc/appnote-19970311-iz.zip

[3] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

Hi Langer, Here are some of my comments. ZipEntry: Optional<Set<...>> getPosixPermissions() (1) Is "Optional" really necessary here. it's a little inconsistent compared to the rest of methods in the class. a "null" return might be just fine? (2) Needs a <p> to separate the first line of the spec from the rest (3) The wording probably needs more consideration. For example, it might be more straightforward to use some similar wording from other methods, for example ... When read from a ZIP file, this is the posix permission stored in the {@codeexternal file attributes} field of the zip file entry'scentral directory record, if there is one. Also, a @ImplNote might be the better place for "it's not available when read from ZIS"? void setPosixPermissions( Set<PosixFilePermission> permissions) I can see the possible use case of using "null" as a special value to reset the permission field (and my proposal of simply returning a null from getPosixPermissions() when there is no one, brings some symmetry here?) ZipUtil: (1) those POSIX_... constants don't need to be public? (2) I like the "stream" style implementation of permsTo/FromFlags pair, but have some concerns regarding their cost. "stream" is relative expensive (when you take a look at those supporting class underneath), as these two are supposed to be invoked for every entry inside a zip file, they can be hundreds and thousands ... just wonder, given most of the entries likely to have the "same" permission set inside a "normal" zip/jar file, is it worth to put in some cache mechanism here, especially for the "get" method, is it designed to return a read-only set of permission? (in which we can use some mechanism here). That said PosixFileAttributes.getPermissions() actually purposely returns a modifiable set of permissions. It might be worth some discussion here, as the ZipEntry.getPosixPermission() needs to specify it, if it's going to return a immutable set. Test: These days it is discouraged to check in a binary zip file as a test target. It is preferred to create the testing zip file on the fly. -Sherman On 9/25/18, 7:57 AM, Langer, Christoph wrote:

Hi all,

I had asked for opinions regarding adding posix permission support to JDK’s zip handling libraries and tools [1]. Since I didn’t get clear “no, you can’t do this” answers, I’m now concretely proposing some enhancements in the area of java.util.zip, jdk.zipfs and jdk.jartool.

I have reopened the long standing bug report (6194856) which had been set to “Won’t fix” quite recently for this purpose.

Here are the technical details:

The ZIP format specifications by infozip and pkware ([2] and [3]) do not explicitly specify the handling of posix file permissions. But it seems to be common sense that if file attribute compatibility is set to “Unix” in the upper byte of CEN field “version made by”, the file permissions bits are stored in CEN field “external file attributes”, byte 3 and 4. My changes try to honor this in the least obtrusive way. 😊

The following changes are proposed:

java.util.zip.ZipEntry:

it will have an additional field “posixPerms”. A value of -1 means “no permission information”, positive values will contain the flag values.

There will be 2 new public methods to read/set the permission information:

public Optional<Set<PosixFilePermission>> getPosixPermissions()

public void setPosixPermissions(Set<PosixFilePermission> permissions)

The usage of type “Optional” reflects that posix permissions are not necessarily present in a zip file

java.util.zip.ZipFile:

it will have the capability to read the CEN fields and set posixPerms if available

java.util.zip.ZipOutputStream:

it will store entries with associated posix permissions as unix type in the CEN, together with the bit mask for the permissions

jdk.jartool:

I propose to add and option "--preserve-posix" or short "-o" to honor the posix bits that may be stored inside zip/jar files. By default the option is not set and hence posix permissions are ignored. If the flag is set and the file system that the jar tool is running on supports posix, posix file permissions that exist in the file system will be stored in newly created/update archives or restored to the file system if such information is present in the archive.

jdk.zipfs:

I added the capability for posix file permissions in the implementation. I decided to support PosixFileAttributes by subclassing ZipFileAttributes from this superclass as well as subclassing ZipFileAttributeView from PosixFileAttributeView. However, as PosixFileAttributes also include groups and owners, I would throw UnsupportedOperationExceptions in case of invoking methods to handle these attributes. But this approach seems to be most consistent with e.g. Files.setPosixFilePermissions and Files.getPosixFilePermissions.

java.nio.file.attribute.PosixFilePermissions:

As this class presents a collection of static helpers, I added definitions for the posix file bit masks together with methods to convert between Sets of PosixFilePermission to bit masks containing the according switches and vice versa. These definitions could theoretically also be moved inside the java.util.zip or jdk.zipfs implementations where they wouldn’t be exposed as public APIs. However, in that case the code would probably need to be duplicated.

I’ve also created two jtreg testcases for both, java.util.zip and jdk.nio.zipfs.

The changes also contain a few further code cleanups.

Here are the links:

Bug: https://bugs.openjdk.java.net/browse/JDK-6194856

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/6194856.0/ <http://cr.openjdk.java.net/%7Eclanger/webrevs/6194856.0/>

I’ll write a CSR once there’s some substantial feedback to my endeavor.

Thanks in advance for reviewing/commenting.

Best regards

Christoph

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055375.h...

[2] http://www.info-zip.org/doc/appnote-19970311-iz.zip

[3] https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT