Hi Alan, all, I'd welcome a discussion, for sure. Unfortunately there hasn't been so much participation in this yet. I think this is an item where it's hard to have a clear opinion and where it's difficult to oversee all implications it might have. Who'd be willing to have a look from security perspective? Thanks & Best regards Christoph From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 5. November 2018 11:23 To: Langer, Christoph <christoph.langer@sap.com>; core-libs-dev <core-libs-dev@openjdk.java.net>; security-dev@openjdk.java.net; Xueming Shen <xueming.shen@oracle.com> Cc: Volker Simonis <volker.simonis@gmail.com>; nio-dev <nio-dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions On 05/11/2018 07:32, Langer, Christoph wrote: Hi, Ping. May I get reviews/substantial feedback on this zipfs enhancement? It might be bit early to be asking for a code review on just one piece of this. I think the first step on this feature has to be to put all the issues on the table. There are several discussion points around ZIP vs. JAR, the impact on signed JARs, carrying permissions without a file owner, and the impact on tools. I also think that there will need to be discussion on security-dev as some of these issues around this feature have security concerns. -Alan

On 05/11/18 15:59, Alan Bateman wrote:

On 05/11/2018 13:05, Langer, Christoph wrote:

...

...

I think you'll need to do a write-up of the overall proposal so that folks can jump in and point out the implications. It's not easy to do this in a code review of a small piece of the solution.

Right, that's the main motivation for my previous questions. It is good to get a *complete* view of what is intended before reviewing the code. ( Sorry, if I've missed some of the previous context ). -Chris.

Hi Chris, yes, there's no impact on Java SE with this item. No API is changed. I've set the scope to JDK, as it affects the features that are available with the "jar" filesystem provider from module jdk.zipfs. Best regards Christoph

-----Original Message----- From: Chris Hegarty <chris.hegarty@oracle.com> Sent: Montag, 5. November 2018 13:20 To: Langer, Christoph <christoph.langer@sap.com>; core-libs-dev <core-libs- dev@openjdk.java.net>; security-dev@openjdk.java.net Cc: nio-dev <nio-dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Christoph,

On 05/11/18 07:32, Langer, Christoph wrote:

...

..

CSR: https://bugs.openjdk.java.net/browse/JDK-8213082

Can you please add a `Scope` value to the CSR?

I can't quite tell, but I assume it should be `JDK` or `Implementation`, right? I want to clarify that there is no impact on Java SE.

-Chris.

Hi Chris

The reason I asked about the CSR scope clarification is that it was unclear to me what the ultimate intentions are, given that the previous mails ( linked to from the CSR ) did have Java SE API changes ( in the java.util.zip package ).

Are you now happy to reduce the scope of the proposal to be confined to the "jar" filesystem provider, or is this more of an initial step towards ultimately adding new Java SE APIs, to zip, to access these permissions ( as was in previous emails )? As well as possibly updating the tools to add such?

The latter thing is true. I reduced the scope of my initial proposal to the "jar" filesystem provider here with the intention to get in this piece more quickly, if not at all. Maybe changes to java.util.zip and tools aren't feasible - we'll see. Furthermore, assuming changes can be done with JDK12, I consider the jdk.zipfs piece "backportable", at least for our OpenJDK build SapMachine (https://sapmachine.io). So having it separate would definitely ease the backporting into SapMachine 11 - which is the target for a customer of ours. Best Christoph

Hi all, here comes the updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/ I've rebased the change to the current state of the JDK depot. Thanks to Volker, the test has been enhanced and now also tests more copy operations (from zip file system to zip file system and from zip file system to default file system and back). The points below were also addressed:

ZipFileAttributeView.java - can you please throw an AssertionError() for the default branch in the switch expression in the "ZipFileAttributeView.name()".

The default branch will return "basic". Looking at the code it should not be jumped to anyway.

ZipFileSystem.java - please also put @Override annotations on the method implementations from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and a comment at the place where the implementation of the "PosixFileAttributes" methods starts.

Done, I also reordered the methods - first "basic" then "posix" then "zip".

ZipUtils.java - just a question: isn't it possible to reuse sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding constants from sun.nio.fs.UnixConstants instead of redefining them here? You could export them from java.base to jdk.zipfs but the problem is probably that at least sun.nio.fs.UnixConstants is a generated class which only gets generated on Unix systems, right ?

You've already answered yourself 😊 These classes only exist on Unix type JDKs.

ZipFileAttributes.java - it seems a little odd that you've added "setPermissions()" to ZipFileAttributes. All the attribute classes (i.e. BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it possible to implement "ZipFileAttributeView.setPermissions()" by calling "path.setPermissions()" (as this is done in "ZipFileAttributeView.setTimes()") and handle everything in ZipFileSyste (as this is done with "setTimes()").

Good catch & thanks for providing the better implementation. I think this version is quite final now and thoroughly tested. I hope to get some valid reviews soon... Thanks Christoph

Hi, I’ve amended the jdk.zipfs module documentation in src/jdk.zipfs/share/classes/module-info.java to document the new behavior (e.g. support of PosixFileAttributeView) as requested by Alan. I’ve also updated the CSR. Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.4/ CSR: https://bugs.openjdk.java.net/browse/JDK-8213082 Please review both, CSR and changeset and let me know if this is good now or what else needs to be addressed. Thanks and best regards, Christoph From: Volker Simonis <volker.simonis@gmail.com> Sent: Freitag, 21. Dezember 2018 17:34 To: Langer, Christoph <christoph.langer@sap.com> Cc: Java Core Libs <core-libs-dev@openjdk.java.net>; security-dev <security-dev@openjdk.java.net>; SHEN, XUEMING <xueming.shen@oracle.com>; Alan Bateman <Alan.Bateman@oracle.com>; nio-dev <nio-dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions Hi Christoph, thanks for updating the change. I think it is in a good state now and ready to go! Also the documentation in the CSR for this issue (https://bugs.openjdk.java.net/browse/JDK-8213082) is greatly appreciated and answers all the questions which have been raised so far. So if there are still any open questions I'd recommend that any potential reviewer consults the CSR at https://bugs.openjdk.java.net/browse/JDK-8213082 first. Thank you and best regards, Volker On Fri, Dec 21, 2018 at 3:31 PM Langer, Christoph <christoph.langer@sap.com<mailto:christoph.langer@sap.com>> wrote: Hi all, here comes the updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/ I've rebased the change to the current state of the JDK depot. Thanks to Volker, the test has been enhanced and now also tests more copy operations (from zip file system to zip file system and from zip file system to default file system and back). The points below were also addressed:

ZipFileAttributeView.java - can you please throw an AssertionError() for the default branch in the switch expression in the "ZipFileAttributeView.name()".

The default branch will return "basic". Looking at the code it should not be jumped to anyway.

ZipFileSystem.java - please also put @Override annotations on the method implementations from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and a comment at the place where the implementation of the "PosixFileAttributes" methods starts.

Done, I also reordered the methods - first "basic" then "posix" then "zip".

ZipUtils.java - just a question: isn't it possible to reuse sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding constants from sun.nio.fs.UnixConstants instead of redefining them here? You could export them from java.base to jdk.zipfs but the problem is probably that at least sun.nio.fs.UnixConstants is a generated class which only gets generated on Unix systems, right ?

You've already answered yourself 😊 These classes only exist on Unix type JDKs.

ZipFileAttributes.java - it seems a little odd that you've added "setPermissions()" to ZipFileAttributes. All the attribute classes (i.e. BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it possible to implement "ZipFileAttributeView.setPermissions()" by calling "path.setPermissions()" (as this is done in "ZipFileAttributeView.setTimes()") and handle everything in ZipFileSyste (as this is done with "setTimes()").

Good catch & thanks for providing the better implementation. I think this version is quite final now and thoroughly tested. I hope to get some valid reviews soon... Thanks Christoph

Hi Alan, thaks for looking at the javadoc/CSR. On Mon, Jan 7, 2019 at 8:10 PM Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 07/01/2019 11:13, Langer, Christoph wrote:

Hi,

I’ve amended the jdk.zipfs module documentation in src/jdk.zipfs/share/classes/module-info.java to document the new behavior (e.g. support of PosixFileAttributeView) as requested by Alan. I’ve also updated the CSR.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.4/

CSR: https://bugs.openjdk.java.net/browse/JDK-8213082

I think this is on the right path now. I'll start with the javadoc as that will take a few iterations and you'll need to get that agreed before finalizing the CSR.

The proposed update starts out "Path objects residing in a zip file system ..." isn't quite right. I think you want start with something like "File systems created by the zip file system provider support the POSIX file attributes defined by the {@link PosixFileAttributeView}".

I think the main issue that will need to be worked out is how the bulk read PosixFileAttributeView::readAttributes behaves when the zip entry doesn't have the external file attributes. UOE isn't right because UOE should be thrown or not thrown based on concrete/implementation type. One option is to throw IOE, another is to have it return a PosixFileAttributes that contains an empty permission set.

We considered this, but it is problematic because it is perfectly valid to have a file with external file attributes where none of the Posix attributes is actually set (i.e. an empty set of Posix files attributes). This wouldn't be distinguishable from the case where a file has no external file attributes. So it seems we have to resort to throwing an IOE?

The latter has the advantage that you can pass the object to anything that excepts a BasicFileAttributes. Having set/getOwner throw UOE is okay. Once we have agreement on how the bulk read behaves then it should be easy to agree the javadoc change.

-Alan.

Hi Alan, Volker, thanks for bringing up these discussion points. I agree that we shall carefully revisit that. First of all, I'd like to point out that PosixFileAttributeView::readAttributes won't ever throw UOE with the proposed changes to zipfs. It should always return an object of type PosixFileAttributes which will be an incarnation of ZipFileSystem.Entry. The returned PosixFileAttributes(ZipFileSystem.Entry) object now implements 3 additional methods: owner(), group() and permissions(). I can see that UOE should only be thrown if something is not supported by an implementation at all. So it perfectly fits to owner() and group() because this is simply not implemented in zipfs (yet...). As for permissions, I then agree, UOE probably isn't the right thing to do because permissions will now be supported by zipfs. Still, we need to distinguish between the case where no permission information is present vs. an empty set of permissions that was explicitly stored. You brought up IOException as an alternative. But I think this is not really feasible for the following main reason: IOE is no RuntimeException, so it has to be declared for a method when it is thrown. PosixFileAttributes::permissions, however, does not declare IOE so far. And I don't think we can/want to modify the PosixFileAttributes interface for the sake of that zipfs implementation change. I think, we should look into using/returning null for "no permission information present". With that, the point is: What happens if we pass null to another PosixFileAttributeView::setPermissions implementation (or to Files::setPosixFilePermissions, which ends up there). For zipfs, with the proposed changeset, this would work perfectly fine. We translate the null value into (int)-1 for the "posixPerms" field of "Entry" and will then not set permission information in the zip file. But other places in the JDK that implement PosixFileAttributeView need some rework. Those are: src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFileAttributeViewImpl And we should amend the specs/doc for the following methods to define the handling of the null value as a noop: PosixFileAttributeView::setPermissions PosixFileAttributes::permissions Files::setPosixFilePermissions Files::getPosixFilePermissions In the implementation I can see that we modify the aforementioned places to handle null by translating it to (int)-1 in UnixFileModeAttribute::toUnixMode and then using this value as noop in 'setMode' resp. 'fchmod'. Do you think this is the right way to go? Should we maybe do the spec update of the permission methods in a separate change? Best regards Christoph

-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 7. Januar 2019 21:46 To: Volker Simonis <volker.simonis@gmail.com> Cc: Langer, Christoph <christoph.langer@sap.com>; nio-dev <nio- dev@openjdk.java.net>; OpenJDK Dev list <security- dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

On 07/01/2019 19:26, Volker Simonis wrote:

...

: We considered this, but it is problematic because it is perfectly valid to have a file with external file attributes where none of the Posix attributes is actually set (i.e. an empty set of Posix files attributes). This wouldn't be distinguishable from the case where a file has no external file attributes. So it seems we have to resort to throwing an IOE?

Maybe although it would be a bit awkward to deal with. The issues around this remind me a bit about mounting fat32 file systems on Linux or Unix systems where the fields in the stat structure are populated with default values. PosixFileAttributeView::readAttributes is  essentially the equivalent of a stat call. This might be something to look at for the file owner at least.

-Alan

On 12/01/2019 13:02, Langer, Christoph wrote:

Hi Alan,

as I did not hear back from you I continued to work on the POSIX file permission support for zipfs and specified/implemented the value of 'null' for zip entries with no permission information associated (vs. UnsupportedOperationException).

I will try to get time next week to reply to you on this. Part of the issue with your current approach is that it breaks PosixFileAttribtues. There are also issues trying to force the API to optionally support a subset of POSIX attributes on some zip entries and not on others. So several issues that will need consideration. -Alan

Hi Christoph

On Jan 12, 2019, at 8:02 AM, Langer, Christoph <christoph.langer@sap.com> wrote:

Hi Alan,

as I did not hear back from you I continued to work on the POSIX file permission support for zipfs and specified/implemented the value of 'null' for zip entries with no permission information associated (vs. UnsupportedOperationException).

I updated the CSR: https://bugs.openjdk.java.net/browse/JDK-8213082 And here is an updated webrev for the change: http://cr.openjdk.java.net/~clanger/webrevs/8213031.5/

I also changed the first part of the module-info documentation for jdk.zipfs, mentioning the URI scheme 'jar' and corrected the information about how the zip file system provider can be accessed and new zip filesystems can be created.

I think the above should be addressed via JDK-8182117 which I will be addressing and keep this focused on the POSIX file permission enhancements as I think it should be in its own CSR. Best Lance

Please kindly review this.

Thank you and best regards Christoph

...

-----Original Message----- From: Langer, Christoph Sent: Dienstag, 8. Januar 2019 09:27 To: 'Alan Bateman' <Alan.Bateman@oracle.com>; Volker Simonis <volker.simonis@gmail.com> Cc: nio-dev <nio-dev@openjdk.java.net>; OpenJDK Dev list <security- dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Alan, Volker,

thanks for bringing up these discussion points. I agree that we shall carefully revisit that.

First of all, I'd like to point out that PosixFileAttributeView::readAttributes won't ever throw UOE with the proposed changes to zipfs. It should always return an object of type PosixFileAttributes which will be an incarnation of ZipFileSystem.Entry.

The returned PosixFileAttributes(ZipFileSystem.Entry) object now implements 3 additional methods: owner(), group() and permissions(). I can see that UOE should only be thrown if something is not supported by an implementation at all. So it perfectly fits to owner() and group() because this is simply not implemented in zipfs (yet...). As for permissions, I then agree, UOE probably isn't the right thing to do because permissions will now be supported by zipfs. Still, we need to distinguish between the case where no permission information is present vs. an empty set of permissions that was explicitly stored. You brought up IOException as an alternative. But I think this is not really feasible for the following main reason: IOE is no RuntimeException, so it has to be declared for a method when it is thrown. PosixFileAttributes::permissions, however, does not declare IOE so far. And I don't think we can/want to modify the PosixFileAttributes interface for the sake of that zipfs implementation change.

I think, we should look into using/returning null for "no permission information present".

With that, the point is: What happens if we pass null to another PosixFileAttributeView::setPermissions implementation (or to Files::setPosixFilePermissions, which ends up there). For zipfs, with the proposed changeset, this would work perfectly fine. We translate the null value into (int)-1 for the "posixPerms" field of "Entry" and will then not set permission information in the zip file. But other places in the JDK that implement PosixFileAttributeView need some rework. Those are: src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFile AttributeViewImpl

And we should amend the specs/doc for the following methods to define the handling of the null value as a noop: PosixFileAttributeView::setPermissions PosixFileAttributes::permissions Files::setPosixFilePermissions Files::getPosixFilePermissions

In the implementation I can see that we modify the aforementioned places to handle null by translating it to (int)-1 in UnixFileModeAttribute::toUnixMode and then using this value as noop in 'setMode' resp. 'fchmod'.

Do you think this is the right way to go? Should we maybe do the spec update of the permission methods in a separate change?

Best regards Christoph

...

-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 7. Januar 2019 21:46 To: Volker Simonis <volker.simonis@gmail.com> Cc: Langer, Christoph <christoph.langer@sap.com>; nio-dev <nio- dev@openjdk.java.net>; OpenJDK Dev list <security- dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

On 07/01/2019 19:26, Volker Simonis wrote:

...

: We considered this, but it is problematic because it is perfectly valid to have a file with external file attributes where none of the Posix attributes is actually set (i.e. an empty set of Posix files attributes). This wouldn't be distinguishable from the case where a file has no external file attributes. So it seems we have to resort to throwing an IOE?

Maybe although it would be a bit awkward to deal with. The issues around this remind me a bit about mounting fat32 file systems on Linux or Unix systems where the fields in the stat structure are populated with default values. PosixFileAttributeView::readAttributes is essentially the equivalent of a stat call. This might be something to look at for the file owner at least.

-Alan

<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>

Hi Alan, first of all, thank you for your input on this.

I think the approach to explore are:

1. zipfs supports PosixFileAttributeView without subsetting. If readAttribute(file, BasicFileAttributes.class) succeeds then readAttribute(file, PosixFileAttributes.class) should also succeed, even if there aren't permissions encoded in the zip entry's external file attributes. It would mean that owner and group return default values, and permissions may return a default value. It does mean you can't distinguish the default value from "no permissions" but there is precedence for that, e.g. mount a FAT32 file system on Linux or Unix systems and `stat` a file to have the stat structure populated with default uid, gid and mode bits.

OK, I can see the point that in a PosixFileAttributeView as it is, there's no place for optionality/null values. However, with this approach the benefits would be that Files::get/setPosixPermissions would work and that's why I think we should pursue this. The challenge will be to find reasonable defaults.

2. zipfs defines a new FileAttributeView that defines read and write access to permissions stored in a zip entry's external file attribute. As it's a new view then it can define the behavior for the case that the zip entry doesn't have permissions. Furthermore it does not need to extend BasicFileAttributeView so doesn't need to be concerned with bulk access, nor concerned with group/owner. As you know, the attributes API allows for both type safe and dynamic access so you have a choice as to whether to support both or just dynamic access. With the first then jdk.zipfs would export a package with a public interface that defines the view. If someone wants type safe access to the permissions attribute then you need to import the class. The alternative is to not export any packages but just define the view in the module-info. The view its name and define the name/type of the permissions attribute, it will also define how it behaves when the external attributes aren't populated. In usage terms it means reading the permissions will be something like Files.readAttribute(file, "zip:permissions") and casting the value to Set<PosixFilePermission> - not pretty but it avoids depending on a JDK-specific API.

For this approach, there are 2 things I dislike. The first is that I don't think we should export named packages from module jdk.zipfs that people would develop Java code against while not being in the Java API. And secondly, this way would not support using Files::set/getPosixPermissions since the specification/implementation of that utility method explicitly refers to PosixFileAttributeView. I can imagine something like this: Zipfs by default implements an own view that offers dynamic, not type safe access to "zip:permissions" and we'll document this. If a user of zipfs wants to see full PosixFileAttributeView support with default values, then we should allow for a creation attribute for the zipfs that can control this. Maybe we can even allow specifying default values for user, group and permissions via zipfs attributes. I'll work to develop the patch into this direction unless you tell me that this idea is bogus (if so, then I hope it be soon 😊) Thanks Christoph

Hi Alan, all, here comes the next proposal for POSIX support in jdk.zipfs - which hopefully represents the converged solution, at least in its overall design. http://cr.openjdk.java.net/~clanger/webrevs/8213031.6/ With that patch, the behavior is the following: a) A ZipFileSystem instance by default does NOT support the PosixFileAttributeView. However, it is possible to query for a known attribute named "zip:storedPermissions". Its value can be null to represent no permission information or any set of PosixFileAttribute. b) A ZipFileSystem instance can be created with the property "posix"=true. In that case, PosixFileAttributeView is supported with default values. -> owner: A UserPrincipal with the name of System.getProperty("user.name") if property access is allowed, "<zipfs_default>" otherwise -> group: A GroupPrincipal with the name "<zipfs_default>". -> permissions: Set.of(OWNER_READ, OWNER_WRITE, GROUP_READ) The default values can be modified by using the properties "defaultOwner", "defaultGroup" and "defaultPermissions". Implementation wise the ZipFileAttributeView always extends PosixFileAttributeView but behaves different, depending on how it was obtained. Within ZipFileSystem, the Entry inner class is not static any more but always has a reference to the Enclosing ZipFileSystem. That's needed because default owner/group/permission can be set on ZipFileSystem instance level now and the Entry, implementing the FileAttributes needs to know where it belongs to. This seems somewhat logical anyway. The new test "TestPosix" is quite extensive. I had to add 2 permissions to test.policy to allow testing in a restricted environment. Module-info and CSR have been adopted, too. Thanks in advance for reviewing. Best regards Christoph

-----Original Message----- From: Langer, Christoph Sent: Montag, 21. Januar 2019 10:17 To: 'Alan Bateman' <Alan.Bateman@oracle.com> Cc: nio-dev <nio-dev@openjdk.java.net>; OpenJDK Dev list <security- dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net>; Volker Simonis <volker.simonis@gmail.com> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Alan,

first of all, thank you for your input on this.

...

I think the approach to explore are:

1. zipfs supports PosixFileAttributeView without subsetting. If readAttribute(file, BasicFileAttributes.class) succeeds then readAttribute(file, PosixFileAttributes.class) should also succeed, even if there aren't permissions encoded in the zip entry's external file attributes. It would mean that owner and group return default values, and permissions may return a default value. It does mean you can't distinguish the default value from "no permissions" but there is precedence for that, e.g. mount a FAT32 file system on Linux or Unix systems and `stat` a file to have the stat structure populated with default uid, gid and mode bits.

OK, I can see the point that in a PosixFileAttributeView as it is, there's no place for optionality/null values. However, with this approach the benefits would be that Files::get/setPosixPermissions would work and that's why I think we should pursue this. The challenge will be to find reasonable defaults.

...

2. zipfs defines a new FileAttributeView that defines read and write access to permissions stored in a zip entry's external file attribute. As it's a new view then it can define the behavior for the case that the zip entry doesn't have permissions. Furthermore it does not need to extend BasicFileAttributeView so doesn't need to be concerned with bulk access, nor concerned with group/owner. As you know, the attributes API allows for both type safe and dynamic access so you have a choice as to whether to support both or just dynamic access. With the first then jdk.zipfs would export a package with a public interface that defines the view. If someone wants type safe access to the permissions attribute then you need to import the class. The alternative is to not export any packages but just define the view in the module-info. The view its name and define the name/type of the permissions attribute, it will also define how it behaves when the external attributes aren't populated. In usage terms it means reading the permissions will be something like Files.readAttribute(file, "zip:permissions") and casting the value to Set<PosixFilePermission> - not pretty but it avoids depending on a JDK-specific API.

For this approach, there are 2 things I dislike. The first is that I don't think we should export named packages from module jdk.zipfs that people would develop Java code against while not being in the Java API. And secondly, this way would not support using Files::set/getPosixPermissions since the specification/implementation of that utility method explicitly refers to PosixFileAttributeView.

I can imagine something like this: Zipfs by default implements an own view that offers dynamic, not type safe access to "zip:permissions" and we'll document this. If a user of zipfs wants to see full PosixFileAttributeView support with default values, then we should allow for a creation attribute for the zipfs that can control this. Maybe we can even allow specifying default values for user, group and permissions via zipfs attributes.

I'll work to develop the patch into this direction unless you tell me that this idea is bogus (if so, then I hope it be soon 😊)

Thanks Christoph

Hi Alan, thanks for taking a first look into this new edition.

I think the javadoc changes will need a few iterations but we can get to that once some of the finer details are sorted out. For example, "Posix Support" isn't quite right as this is about optional support for the POSIX view of file attributes rather than complete support for POSIX.

Ok.

Also the "Zip" view of file attributes will need to be fleshed out more (the view name for example).

I don't know if that's really necessary as the "Zip" view currently is internal to jdk.zips and I don't propose to export it.

I'm not sure about using ${user.name} and "<zipfs_default>" as default. Have you looked at using the zip file owner/group (or owner/owner on Windows) as the default? 

Hm, I guess for Unix that'd be ok. But how would I get to the owner of a file in Window's default file system?

Also just wondering if 777 might be more appropriate (maybe you have a reason for choosing 660?). It might be useful to see what Linux, macOS and other operating systems do when mounting a FAT file system.

I implemented 640, but it was just chosen without much thinking. I can agree to 777.

Did you consider using the string representation of the user, group and permissions in the configuration properties? The zip file system provider could support both of course. String might make it a bit easier to create the map of configuration properties when creating the file system e.g Map.of("enablePosixPermissions", "true", "defaultOwner", "joe", "defaultPermissions", "rw-rw---");

Sounds like a good and feasible idea. I'll try to address these points in a next iteration. Best regards Christoph

Hi Alan, here is the next iteration: http://cr.openjdk.java.net/~clanger/webrevs/8213031.7/ I focused on your comments regarding implementation details:

I'm not sure about using ${user.name} and "<zipfs_default>" as default. Have you looked at using the zip file owner/group (or owner/owner on Windows) as the default?  Also just wondering if 777 might be more appropriate (maybe you have a reason for choosing 660?). It might be useful to see what Linux, macOS and other operating systems do when mounting a FAT file system.

I'm using Files.getOwner() now as well as the default of 777 for permissions.

Did you consider using the string representation of the user, group and permissions in the configuration properties? The zip file system provider could support both of course. String might make it a bit easier to create the map of configuration properties when creating the file system e.g Map.of("enablePosixPermissions", "true", "defaultOwner", "joe", "defaultPermissions", "rw-rw---");

Implemented. I also added Lance's suggestions to the test. Are there other major implementation points left? If not I guess we should start refining the documentation... Thanks Christoph

Hi Alan, thanks for diving into this and giving a comprehensive summary. I've just read it in detail now - and as always, it contains some very good findings and suggestions. I'll try to work these in and send an update in the next days. Best regards Christoph

-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Montag, 25. Februar 2019 09:31 To: Langer, Christoph <christoph.langer@sap.com> Cc: nio-dev <nio-dev@openjdk.java.net>; Java Core Libs <core-libs- dev@openjdk.java.net> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

On 21/02/2019 15:04, Langer, Christoph wrote:

...

Hi Alan,

here is the next iteration:http://cr.openjdk.java.net/~clanger/webrevs/8213031.7/

I focused on your comments regarding implementation details:

: Are there other major implementation points left? If not I guess we should start refining the documentation...

I think it would be useful to write down a summary of the proposal as there are several detailed points that we've been discussing in this thread that reviewers will need to understand the proposal before diving into the implementation/patch.  This might also help getting the javadoc aligned, and eventually the CSR. Here's where I think we are:

1. By default, a zip file system will have no support for the "posix" and "owner" views of file attributes, this means the following will fail with UOE:

  PosixFileAttributes attrs = Files.readAttributes(file, PosixFileAttributes.class);

and the zip file system's supportedFileAttributView method will not include "posix".

2. Creating a zip file system with configuration parameter XXX set to "true" will create the zip file system that supports PosixFileAttributeView (and FileOwnerAttributeView). The current patch names the configuration parameter "posix". That name might be misleading as a configuration parameter as it conveys more than it actually does. Something like "enablePosixFileAttributes" clear conveys that it enables support for POSIX file attribute but might be a better wordy.

The value in the configuration map is documented as Boolean but I think we are allowing it to be the string representation of a Boolean, meaning it can be "true" or "false" like we have with the "create" parameter.

3. The map of configurations parameters can optionally include the parameters "defaultOwner", "defaultGroup", and "defaultPermissions" with the default owner/group/permissions to return. These names seem okay to me.

For the owner/group, the parameter type can be UserPrincipal/GroupPrincipal or strings. If the value is a String then we need to decide if there is any validation. If I read the current patch correctly then it doesn't do any validation - that might be okay but minimally maybe we should disallow the empty string.

If the defaultXXX parameters aren't present then the zip file owner/group would be a sensible default. If the zip file is on a file store that supports FileOwnerAttributeView then we've got the owner. If the file store supports PosixFileAttributeView then we have a group owner. If PosixFileAttributeView is not supported then using the owner as the group is okay. I see the current patch as a fallback to fallback to "<zipfs_default>" but I'd prefer not have that because it will be very confusing to diagnose if there are issues.

For defaultPermissions, the value is a Set<PosixFilePermissions> or the string representation. In the implementation, initPermissions can be changed to use PosixFilePermissions.fromString as it does the translation that we need.

4. As an alternative to the type safe access that PosixFileAttributeView provides, the proposal is to document the "zip" view of file attributes. Initially, it will admit to one file attribute for the permissions. The current patch uses the name "storedPermissions" but it might be simpler to use "permissions" so that "zip:permissions" and "posix:permissions" are the same when the zip entry has permissions. With this support you can write code like this:     Set<PosixFilePermission> perms = (Set<PosixFilePermission>) Files.getAttribute(file, "zip:permissions");

The cast is ugly of course but that's the consequence of not exposing ZipFileAttributes in the API so there is no type token to specify on the RHS.

Documenting the "zip" view brings up a few issues, e.g. will Files.readAttributes(file, "zip:*") reveal more than "permissions". For this API it is okay for the map to attributes beyond the specified list.

5. There will be no support for specifying as POSIX FileAttribute to createFile or createDirectory to atomically set the permissions when creating a new file/directory, UOE will thrown as is is now.

Is this a reasonable summary?

-Alan

On 20/03/2019 16:25, Langer, Christoph wrote:

Hi Alan,

I now found time to get back to the POSIX file permissions in zipfs. The goal would be to get it done for JDK13 😊

:

Here is an updated webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.8/

What's next?

I skimmed through the updated webrev and read your latest comments/replies and I think we are close. One thing that is puzzling is that ZipFileAttributeView/ZipFileAttributes extend PosixFileAttributeView/PosixFileAttributes. I don't think that will work because the "zip" view is supported by default whereas "posix" view needs the file system to be created with enablePosixFileAttributes=true. I saw your comment about naming the file permissions attribute "storedPermissions" in the zip view but if the zip and posix view are separated then I think it would be clearer to name it "permissions" in the zip view. If code is using Files.getAttribute then it will need the view name so they won't get mixed up. You asked about the fallback when defaultOwner/defaultGroup aren't set. If getOwner fails with an IOException then I think that exception should be propagated. The UOE case will fallback to the value of "user.name". I think the fallback for the group owner should be the file owner rather than " "<zipfs_default>". The default.policy file will need to be updated to grant jdk.zipfs RuntimePermission("accessUserInfo") so that you won't need to deal with security exceptions. As regards next steps then I think we need to agree the spec changes, as in the the javadoc in module-info.java. Once we have the spec agreed then the CSR can be updated and finalized. The code review can be done in parallel of course. I think Lance is going to help review the changes. -Alan

On 01/04/2019 14:38, Langer, Christoph wrote:

: Hm, when I was looking at it initially, I was also wondering if it would be cleaner either have a default ZipFileAttributeView/ZipFileAttributes implementation that doesn't extend Posix or an "Enhanced" ZipFileAttributeView/ZipFileAttributes that would extend Posix. But I saw in the implementation of ZipFileAttributeView::get that this is already the "gate" where the requester would get the ZipFileAttributeView implementation with the requested behavior set. So I was hoping that it'd be fine to handle the Posix extension this way. Do you really think that wouldn't work?

Alternatively, I could explore a different class hierarchy for ZipFileAttributeView/ZipFileAttributes... The issue with ZipFileAttributeView extending PosixFileAttributeView is that it change the attributes defined by the latter to optional, e.g. try readAttributes "zip:*" when enablePosixFileAttributes is enabled and disabled. I think it would be simpler, as least for the specification, to separate them.

:

...

As regards next steps then I think we need to agree the spec changes, as in the the javadoc in module-info.java. Once we have the spec agreed then the CSR can be updated and finalized. The code review can be done in parallel of course. I think Lance is going to help review the changes. Ok, I guess this eventually boils down to agree upon the right way of doing the "permissions" attribute or is there something more related to the spec?

We'll need to do a bit of wordsmithing too. For example, the current draft has "It is possible to query a Path object .." when it should be saying "It is possible to query a file in a Zip file system ...".  This is all straight-forward, I think the main things now are to get agreement on the relationship between the different sets of attributes and the name of the permissions that the zip view supports. -Alan

On 09/05/2019 16:16, Langer, Christoph wrote:

Hi Alan,

I've got a new iteration for the zipfs POSIX support and addressed your concerns: http://cr.openjdk.java.net/~clanger/webrevs/8213031.9/ Thanks. I think you've addressed most of my points. The only thing that isn't clear is the group owner as I thought we had converged on using the zip file's group owner.  If I read the code correctly then it uses the file owner (and makes the assumption that defaultOwner is initialized before initGroup is called).

In passing, the name of the PosixFileAttributeView implementation should probably be ZipPosixFileAttributeView rather than ZipFilePosixAttributeViewPosix. Also EntryPosix extends Entry, should probably be PosixEntry. Not important as these are internal but noticed them when scanning the changes. Also in passing, there are several places using AccessController.doPrivileged that are bit ugly due to the casting. The doPrivileged methods are awkward to use with lambda expressions (not your doing) but one approach that I find readable is to separate the creation of the action, e.g. file the zip file owner it could be: PrivilegedExceptionAction<UserPrincipal> pa = () -> Files.getOwner(zfpath); return AccessController.doPrivileged(pa). In the same area, the code in initOwner catches UOE but that will always be wrapped in PAE. Not important to the discussion here of course.

: I have updated module-info a little bit to reflect the latest changes. Is it now time to focus on the CSR?

The "For extended POSIX support ..." paragraph has the property name from a previous iteration so I assume you'll update that. I think it would be using to put quotes around the names too. It also specifies the fallback group owner to be the file owner but I think we converged on use the zip file's group owner where possible. A small bit of word smiting required but I think this is really close to writing the CSR. -Alan

Hi, here is a little update to my latest webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.11/ I had to make modifications to fix test errors and enhance testing: a) in initOwner/initGroup the UOE has to be catched and handled explicitly as it is never wrapped into PAE because it is a RuntimeException. b) updated the test to expect the zip file's group as default group c) added a sanity check to read and extract a zip file created by zipfs with posix permissions set via java.util.zip.ZipFile Best regards Christoph

-----Original Message----- From: Langer, Christoph Sent: Dienstag, 21. Mai 2019 17:24 To: Alan Bateman <Alan.Bateman@oracle.com> Cc: nio-dev <nio-dev@openjdk.java.net>; Java Core Libs <core-libs- dev@openjdk.java.net> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Alan,

Thank you for your comments. Here comes the next update... increasing the turnaround time a little bit 😊

http://cr.openjdk.java.net/~clanger/webrevs/8213031.10/

...

Thanks. I think you've addressed most of my points. The only thing that isn't clear is the group owner as I thought we had converged on using the zip file's group owner.  If I read the code correctly then it uses the file owner (and makes the assumption that defaultOwner is initialized before initGroup is called).

Ok, makes sense. I've updated the coding such that the zip's file owner would be the default owner, in case it can be retrieved.

...

In passing, the name of the PosixFileAttributeView implementation should probably be ZipPosixFileAttributeView rather than ZipFilePosixAttributeViewPosix. Also EntryPosix extends Entry, should probably be PosixEntry. Not important as these are internal but noticed them when scanning the changes.

I changed the class names, hopefully to your liking.

...

Also in passing, there are several places using AccessController.doPrivileged that are bit ugly due to the casting. The doPrivileged methods are awkward to use with lambda expressions (not your doing) but one approach that I find readable is to separate the creation of the action, e.g. file the zip file owner it could be:

PrivilegedExceptionAction<UserPrincipal> pa = () -> Files.getOwner(zfpath); return AccessController.doPrivileged(pa).

I updated these places. Seems more readable indeed.

...

In the same area, the code in initOwner catches UOE but that will always be wrapped in PAE.

Fixed.

...

...

I have updated module-info a little bit to reflect the latest changes. Is it now time to focus on the CSR?

The "For extended POSIX support ..." paragraph has the property name from a previous iteration so I assume you'll update that. I think it would be using to put quotes around the names too. It also specifies the fallback group owner to be the file owner but I think we converged on use the zip file's group owner where possible.

A small bit of word smiting required but I think this is really close to writing the CSR.

I have updated the doc in module-info.java quite a bit. Please check.

Is it time to work on the CSR now? How shall we proceed there?

Thanks Christoph

Hi Alan,

...

http://cr.openjdk.java.net/~clanger/webrevs/8213031.10/ I see your other mail with v11 so I switch to that version.

Right. 😊

...

Ok, makes sense. I've updated the coding such that the zip's file owner would be the default owner, in case it can be retrieved. I think this is mostly right now. One thing to check is that you are catching UOE whereas it's the PAE's cause that might be UOE.

It's alright as it is. If you look at the source of AccessController.doPrivileged for a PrivilegedExceptionAction, you will see that RuntimeExceptions are caught and rethrown, not yielding a PAE. So I must catch UOE here explicitly because it's a RuntimeException.

...

: I have updated the doc in module-info.java quite a bit. Please check.

Is it time to work on the CSR now? How shall we proceed there?

The table items in L119-150 look fine, we just need to avoid really long lines One minor comment on L123 is that it might be clearer if you drop "created" from the sentence.

L48-78 is a "wall of text" and links that I don't think will be easy for most developers to read. Can I provide suggested wording for this part of the spec? I'm just thinking that an alternative wording might help avoid too much iteration on this.

Sure, you're very welcome 😊 So, I'll be waiting for your input on that. Thanks Christoph

Hi Alan,

The table items in L119-150 look fine, we just need to avoid really long lines One minor comment on L123 is that it might be clearer if you drop "created" from the sentence.

L48-78 is a "wall of text" and links that I don't think will be easy for most developers to read. Can I provide suggested wording for this part of the spec? I'm just thinking that an alternative wording might help avoid too much iteration on this.

I have created a new webrev to add some linebreaks and pick up your suggestion to drop the word "created" in L123. http://cr.openjdk.java.net/~clanger/webrevs/8213031.12/ Waiting on your update for the other part. Thanks Christoph

On May 31, 2019, at 12:32 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:

On 29/05/2019 13:16, Langer, Christoph wrote:

...

Hi Alan,

...

The table items in L119-150 look fine, we just need to avoid really long lines One minor comment on L123 is that it might be clearer if you drop "created" from the sentence.

L48-78 is a "wall of text" and links that I don't think will be easy for most developers to read. Can I provide suggested wording for this part of the spec? I'm just thinking that an alternative wording might help avoid too much iteration on this. I have created a new webrev to add some linebreaks and pick up your suggestion to drop the word "created" in L123.

http://cr.openjdk.java.net/~clanger/webrevs/8213031.12/

Waiting on your update for the other part. Attached is alternative wording for the "Support for POSIX file permissions" section. My concern with the proposed in webrev.12 is that it's dense and not easy to read. It also misses a few things - one important one is that access permissions aren't enforced.

I think the wording below is looking good. A few minor suggestions below.

So overall I think you've got this feature into reasonable shape (I realize it has taken 7 months to get here, this is perhaps a good example of something that needs a lot of up front discussion before going near the code).

Once we finalize the CSR, we should look towards getting the changes in early in the JDK 14 cycle so that we have time to vet/catch potential issues.

-Alan

import java.nio.file.Files; import java.nio.file.LinkOption; import java.nio.file.Path; import java.nio.file.attribute.FileAttributeView; import java.nio.file.attribute.PosixFileAttributes; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFileAttributeView; import java.util.Set;

* <h2> POSIX file attributes </h2> * * <p> A Zip file system supports a file attribute {@link FileAttributeView view} * named "{@code zip}" that defines the following file attribute: * * <blockquote> * <table class="striped"> * <caption style="display:none">Supported attributes</caption> * <thead> * <tr> * <th scope="col"> Name </th> * <th scope="col"> Type </th> * </tr> * </thead> * <tbody> * <tr> * <th scope="row"> permissions </th> * <td> {@link Set}<{@link PosixFilePermission}> </td> * </tr> * </tbody> * </table> * </blockquote> * * The "permissions" attribute is the set of access permissions that are optionally * stored for entries in a Zip file. The value of the attribute is {@code null} * for entries that do not have access permissions. Zip file systems do not * enforce access permissions. * * <p> The "permissions" attribute can be read and set using the

can-> may

* {@linkplain Files#getAttribute(Path, String, LinkOption...) Files.getAttribute} and * {@linkplain Files#setAttribute(Path, String, Object, LinkOption...) Files.setAttribute} * methods. The following example uses these methods to read and set the attribute: * <pre> {@code * Set<PosixFilePermission> perms = Files.getAttribute(entry, "zip:permissions"); * if (perms == null) { * perms = PosixFilePermissions.fromString("rw-rw-rw-"); * Files.setAttribute(entry, "zip:permissions", perms); * } * } </pre> * * <p> In addition to the "{@code zip}" view, a Zip file system optionally supports * the {@link PosixFileAttributeView POSIX file attribute view} ("{@code posix}"). * This view extends the "{@code basic}" view with type safe access to the * {@link PosixFileAttributes#owner() owner}, {@link PosixFileAttributes#group() group-owner}, * and {@link PosixFileAttributes#permissions() permissions} attributes. The * "{@code posix}" view is only supported when the Zip file system is created with * the provider property "{@code enablePosixFileAttributes}" set to "{@code true}". * The following creates a file system with this property and reads the access * permissions of a file: * <pre> {@code * var env = Map.of("enablePosixFileAttributes", "true"); * try (FileSystem fs = FileSystems.newFileSystem(file, env) { * Path entry = fs.getPath("entry"); * Set<PosixFilePermission> perms = Files.getPosixFilePermissions(entry); * } * } </pre> * * <p> The file owner and group owner attributes are not persisted, meaning they are * not stored in the zip file. The "{@code defaultOwner}" and "{@code defaultGroup}" * provider properties (listed below) can be used to configure the default values * for these attributes. If these properties are not set then the file owner * defaults to the owner of the zip file, and the group owner defaults to the * zip file's group owner (or the file owner on platforms that don't support a * group owner). * * <p> The "{@code permissions}" attribute is not optional in the "{@code posix}" * view so a default of set of permissions are used for entries that do not have ^^^ of seems out of place * access permissions stored in the Zip file. The default set of permissions * is {@link PosixFilePermission#OWNER_READ OWNER_READ}, {@link PosixFilePermission#OWNER_WRITE is -> are * OWNER_WRITE} and {@link PosixFilePermission#GROUP_READ GROUP_READ}. perhaps consider using a <UL> when listing the permissions? The default * permissions can be configured with the "{@code defaultPermissions}" property * described below.

Best Lance <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>

On 02/06/2019 22:35, Langer, Christoph wrote:

Hi Alan, Lance,

thanks for the updated wording in module-info, that really looks good. I incorporated it into my change, no we’d be here:

http://cr.openjdk.java.net/~clanger/webrevs/8213031.13/

To be honest, I was hoping it would still make it into JDK13.

I guess now I shall update the CSR to get it reviewed, correct?

The spec looks good. The CSR is the next step, it can happen while the changes are being reviewed here. -Alan

On 04/06/2019 15:25, Langer, Christoph wrote:

Hi Alan,

I made some updates to the CSR. The main update was that I transferred the new documentation part from module-info.java to the CSR’s Specification section.

Can you please review the CSR?

The CSR mostly looks good. One thing is that the Specification section doesn't have the changes to the "Zip File System Properties" part of the spec so I think that should be added. I'll add myself as Reviewer to the CSR so you can finalize when you are done. -Alan

Hi Lance, Alan, et al., as it seems we are getting close to CSR approval, the changeset should also be finally reviewed. Here is the most current version: http://cr.openjdk.java.net/~clanger/webrevs/8213031.14/ I have made some updates to ZipConstants as suggested by Lance in a private mail. Best regards Christoph

Hi Christoph, I have not had a chance to completely go through the latest webrev. I hope to get back to it Monday

On Jun 14, 2019, at 2:18 PM, Langer, Christoph <christoph.langer@sap.com> wrote:

Alan, Lance,

the CSR was approved yesterday.

So, is the webrev (below) also ok and reviewed from your end?

Thanks Christoph

From: Langer, Christoph Sent: Mittwoch, 5. Juni 2019 16:45 To: Alan Bateman <Alan.Bateman@oracle.com>; Lance Andersen <lance.andersen@oracle.com> Cc: nio-dev <nio-dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Lance, Alan, et al.,

as it seems we are getting close to CSR approval, the changeset should also be finally reviewed.

Here is the most current version: http://cr.openjdk.java.net/~clanger/webrevs/8213031.14/ <http://cr.openjdk.java.net/~clanger/webrevs/8213031.14/>

I have made some updates to ZipConstants as suggested by Lance in a private mail.

Best regards Christoph

<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>

Hi Christoph, Just starting to take a peek at this and noticed one quick thing in your test: ------------ Paths.get(System.getProperty("test.dir", "."), "testPosix.zip") —————— You do not need the test.dir property or the permission added to test.policy to access it, just reference the jar and it will be created in user.dir which is also writable. Best Lance

On Feb 12, 2019, at 4:57 PM, Langer, Christoph <christoph.langer@sap.com> wrote:

Hi Alan, all,

here comes the next proposal for POSIX support in jdk.zipfs - which hopefully represents the converged solution, at least in its overall design.

http://cr.openjdk.java.net/~clanger/webrevs/8213031.6/

With that patch, the behavior is the following: a) A ZipFileSystem instance by default does NOT support the PosixFileAttributeView. However, it is possible to query for a known attribute named "zip:storedPermissions". Its value can be null to represent no permission information or any set of PosixFileAttribute. b) A ZipFileSystem instance can be created with the property "posix"=true. In that case, PosixFileAttributeView is supported with default values. -> owner: A UserPrincipal with the name of System.getProperty("user.name") if property access is allowed, "<zipfs_default>" otherwise -> group: A GroupPrincipal with the name "<zipfs_default>". -> permissions: Set.of(OWNER_READ, OWNER_WRITE, GROUP_READ)

The default values can be modified by using the properties "defaultOwner", "defaultGroup" and "defaultPermissions".

Implementation wise the ZipFileAttributeView always extends PosixFileAttributeView but behaves different, depending on how it was obtained.

Within ZipFileSystem, the Entry inner class is not static any more but always has a reference to the Enclosing ZipFileSystem. That's needed because default owner/group/permission can be set on ZipFileSystem instance level now and the Entry, implementing the FileAttributes needs to know where it belongs to. This seems somewhat logical anyway.

The new test "TestPosix" is quite extensive. I had to add 2 permissions to test.policy to allow testing in a restricted environment.

Module-info and CSR have been adopted, too.

Thanks in advance for reviewing.

Best regards Christoph

...

-----Original Message----- From: Langer, Christoph Sent: Montag, 21. Januar 2019 10:17 To: 'Alan Bateman' <Alan.Bateman@oracle.com> Cc: nio-dev <nio-dev@openjdk.java.net>; OpenJDK Dev list <security- dev@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net>; Volker Simonis <volker.simonis@gmail.com> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Alan,

first of all, thank you for your input on this.

...

I think the approach to explore are:

1. zipfs supports PosixFileAttributeView without subsetting. If readAttribute(file, BasicFileAttributes.class) succeeds then readAttribute(file, PosixFileAttributes.class) should also succeed, even if there aren't permissions encoded in the zip entry's external file attributes. It would mean that owner and group return default values, and permissions may return a default value. It does mean you can't distinguish the default value from "no permissions" but there is precedence for that, e.g. mount a FAT32 file system on Linux or Unix systems and `stat` a file to have the stat structure populated with default uid, gid and mode bits.

OK, I can see the point that in a PosixFileAttributeView as it is, there's no place for optionality/null values. However, with this approach the benefits would be that Files::get/setPosixPermissions would work and that's why I think we should pursue this. The challenge will be to find reasonable defaults.

...

2. zipfs defines a new FileAttributeView that defines read and write access to permissions stored in a zip entry's external file attribute. As it's a new view then it can define the behavior for the case that the zip entry doesn't have permissions. Furthermore it does not need to extend BasicFileAttributeView so doesn't need to be concerned with bulk access, nor concerned with group/owner. As you know, the attributes API allows for both type safe and dynamic access so you have a choice as to whether to support both or just dynamic access. With the first then jdk.zipfs would export a package with a public interface that defines the view. If someone wants type safe access to the permissions attribute then you need to import the class. The alternative is to not export any packages but just define the view in the module-info. The view its name and define the name/type of the permissions attribute, it will also define how it behaves when the external attributes aren't populated. In usage terms it means reading the permissions will be something like Files.readAttribute(file, "zip:permissions") and casting the value to Set<PosixFilePermission> - not pretty but it avoids depending on a JDK-specific API.

For this approach, there are 2 things I dislike. The first is that I don't think we should export named packages from module jdk.zipfs that people would develop Java code against while not being in the Java API. And secondly, this way would not support using Files::set/getPosixPermissions since the specification/implementation of that utility method explicitly refers to PosixFileAttributeView.

I can imagine something like this: Zipfs by default implements an own view that offers dynamic, not type safe access to "zip:permissions" and we'll document this. If a user of zipfs wants to see full PosixFileAttributeView support with default values, then we should allow for a creation attribute for the zipfs that can control this. Maybe we can even allow specifying default values for user, group and permissions via zipfs attributes.

I'll work to develop the patch into this direction unless you tell me that this idea is bogus (if so, then I hope it be soon 😊)

Thanks Christoph

<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>

Hi Christoph

On Feb 13, 2019, at 5:30 PM, Langer, Christoph <christoph.langer@sap.com> wrote:

Hi Lance,

thanks for looking.

...

Just starting to take a peek at this and noticed one quick thing in your test: ------------ Paths.get(System.getProperty("test.dir", "."), "testPosix.zip") ——————

You do not need the test.dir property or the permission added to test.policy to access it, just reference the jar and it will be created in user.dir which is also writable.

Hm, I thought I didn't want to mess around in "user.dir" as it can be some more global directory where you wouldn't want to leave artefacts... To me "test.dir" feels cleaner. Are there other opinions about that?

user.dir points to the scratch directory that test uses, so it is where you want to create the tests. Workspaces can sometimes be read only: For example: —————— @Test public void test000() throws IOException { System.out.println("test.dir = " + System.getProperty("test.dir", ".")); System.out.println("user.dir = " + System.getProperty("user.dir", ".")); System.out.println( Paths.get(System.getProperty("test.dir", "."), "basic.jar").toAbsolutePath() ); } ----------------- Results in: -------------------- test.dir = . user.dir = /Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch /Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch/./basic.jar Please see http://openjdk.java.net/jtreg/tag-spec.html for the system properties. I do not see test.dir there. ————————— I would just do: ————————— Path foo = Path.of("test.zip"); System.out.println("test.zip path=" + foo.toAbsolutePath()); -------------------------- which results in the output: <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>test.zip path=/Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch/test.zip Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>