RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

Please review the latest webrev that addresses the issue raised in Sherman’s comments below, with my comments interspersed in-line.  The changes between this webrev and the last one are in the definition and use of the ismultiRelease() method and the introduction of a configuration lock, the boolean configured, that prevents setting the version after an entry has been read from the jar file.  As a consequence of the configuration lock, I had to modify a couple tests and add a new one.

Issue: https://bugs.openjdk.java.net/browse/JDK-8132734
JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305
Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/

 
> On Oct 26, 2015, at 9:37 PM, Xueming Shen <xueming.shen at oracle.com> wrote:
> 
> Hi Steve,
> 
> I know I stared to sound like a broken record :-) But I would like to suggest the team one more
> time to reconsider the current decision of using the "set" methods to change the configuration
> setting/status of an existing JarFile to enable the multi-version support.
> 
> public JarFile setVersioned(int version);
> public JarFile setRuntimeVersioned();

We did reconsider it, more later.

> 
> The main concern here is the current approach basically transfers the JarFile from a read-only/
> immutable object with consistent behavior (to entry inquiry) to a mutable container of entries
> with possibility of inconsistent behavior. The newly introduced setVersioned/setRuntimeVersioned
> really have no way to guarantee A expected result from the updated version-enabled getEntry()
> method, as someone else might set an unexpected different "version" between your setting and
> getting, or even worse, in the middle of your entries() invocation, for example, in which you get
> part of your entries to version N and the rest to version M.

We’ve introduced a configuration lock.  One can change the versioning strategy as often as they wish before reading an entry.  However if setVersioned() is called after an entry has been read, an IllegalStateException is thrown.

> 
> So It might be desired to have the "versioned support" enabled in the constructor, so once you
> get that version enabled JarFile, it stays that way for its lifetime with consistent result for the
> entry inquiry, as the current API does.

We can’t do it in the constructor because we need to read the manifest, and doing that in the constructor leads to a stack overflow for some instances, such as deploy, where JarFile is subclassed and the manifest is lazily read.  Basically, the manifest is not available during construction.

> 
> I do realize that there might be use case that the getEntry invoker might not have the access to
> the creation of the corresponding jar file (such as the use scenario in that JarURLConnection?), so
> you can't create a version-enabled JarFile at the very beginning via the constructor. But doesn't
> this also make my concern more real. If you don't have the control of the lifetime of that JarFile,
> you don't really have the control of who is setting or going to set the version of that mutable JarFile,
> right?
> 
> An alternative might be to have change the setVersioned/setRuntimeVersioned() to
> 
> public jarFile getVersioned(int version);
> public jarFile getRuntimeVersioned();
> 
> to return a new copy of the existing JarFile with the desired verisoning support.

That’s a problem because we would then have two instances of JarFile, and we’d need to remember to close the underlying instance.  An alternative is to use factory methods to create the versioned JarFile.  That works, but we have to add about six of them to correspond to the current set of constructors.  But the big problem here is, as you mention, the JarFile returned from JarURLConnection.  From experiments it looks to me that the underlying cached JarFile is unlinked from the Unix filesystem and thus unavailable for replication.  This code seems very OS dependent as well as potentially fragile since subclasses can implement the abstract getJarFile() method any way they wish.

A configuration lock seems to be a good compromise.  We can mutate the JarFile versioning strategy as much as we want until we read an entry, and then no more mutations are allowed.  For what it is worth, I don’t see a real use case to set the versioning strategy more than once, even before an entry is read.  I do that in the tests, but it’s difficult to imagine it anywhere else.  The intention is to construct a JarFile and then immediately set the versioning strategy.

> Yes, it might be
> too heavy from performance perspective :-) and we might have to do some tricky stuff (it would
> be easier if ZipJarFile is interface ...) to have a light wrapper class to delegate everything to the
> real one.
> 
> That said, I'm fine to be told "the pros and cons were considered,  and this is the best for the
> supported use scenario":-)

Yes, that is the case.

> In that case, it might deserve some wording in the spec notes to
> prepare the developer the possible unexpected.
> 
> Thanks,
> Sherman
> 
> 
> On 10/26/15 10:26 AM, Steve Drach wrote:
>> Hi,
>> 
>> We’ve published another webrev for review.
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8132734
>> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305
>> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/
>> 
>> This one addresses the issues regarding CodeSigners, Certificates, verification, and other security issues raised in the last round, including whether third party verification is a supported use case.  I also partially fixed a nitpick involving performance while searching for versioned entries, by putting in a cache for previously searched entries.  And I found a way around the issue with windows being unable to delete jar files accessed through URL’s in one test.
>> 
>> Steve
>> 
>>> On Oct 21, 2015, at 12:54 AM, Wang Weijun <weijun.wang at oracle.com> wrote:
>>> 
>>>> On Oct 21, 2015, at 3:17 PM, Xueming Shen <xueming.shen at oracle.com> wrote:
>>>> 
>>>> We might want to bring in Max to take a look if what I said is really a supported use scenario.
>>> I haven't read Steve's latest code change. I will read if you think it's necessary.
>>> 
>>> First, I think we agree that the multi-release jar file feature is only making use of the existing jar file format and does not intend to introduce any change to its specification. This means a JarFile signed by one JDK release should be verified by another JDK release.
>>> 
>>> Ok, the next question is, should it modify the JarFile API? I hope not, because the JarFile API is the single entry we access a JarFile when we want to sign or verify it. I hope there is a brand new API for a multi-versioned jar file, probably a child class of JarFile, so that no matter you call getJarEntry() or entries() on it, you always get the versioned one and the unrelated ones are completely invisible.
>>> 
>>> If this is not OK, maybe we can rename the current JarFile to RawJarFile and name the new API JarFile. Signing and verification will work on RawJarFile.
>>> 
>>> Not sure if it's easy.
>>> 
>>> --Max
>>> On Oct 21, 2015, at 12:17 AM, Xueming Shen <xueming.shen at oracle.com> wrote:
>>> 
>>> Hi Steve,
>>> 
>>> The reifiedEntry() approach probably can help the default JarVerifier work as expected, but if I read the
>>> code correctly  I doubt you can get the expected CodSigner[] and Certificatte[] result from a "versioned"
>>> JarFileEntry, after having read all bytes from the input stream (obtained via jzf.getInputStream(JarFileEntry)),
>>> as the JarEntry spec suggests,. As we are passing the "reified" entry into the VerifierStream alone, without
>>> any reference to the original jar file entry.  It seems impossible for the original jar file entry can trace back to
>>> the corresponding certificates and code signers. This might be fixed by passing in the original entry together
>>> into the JarVerifier, but I doubt we might have a bigger issue here. I suspect with this approach an external
>>> verifier will have no easy way to verify the digit signature of the jar entry via java.security APIs. I would assume
>>> this is doable right now with current JarFile APIs, via a JarFile object, a Manifest and a target JarEntry. The external
>>> can get the signature via name -> manifest->attributes->signature (basically just consider to move the
>>> JarVerifier and couple sun.security.util classes out and use it as user code)... but with this implementation
>>> the name now  is the root entry, but the bytes you can read from the stream is from the versioned one.
>>> We might want to bring in Max to take a look if what I said is really a supported use scenario. I might be
>>> wrong, not a security expert :-)
>>> 
>>> Btw, for a "normal" JarEntry/ZipEntry (not a JarFileEntry), shouldn't the getInputStream(ze) simply return
>>> the stream for the root entry? The current implementation of getJarEntry(ze) does not seem right, as it
>>> returns a "versioned" JarFileEntry. I don't think you want to pass this one into VerifierStream directly,
>>> right? Again, I think it might be desired (at least the spec is not updated to say anything about "version")
>>> to simply return the input stream for the root entry.
>>> 
>>> One more "nitpick". searchForVersionedEntry() now lookups the versioned candidate via super.getEntry()
>>> from version to BASE_VERSION, if the version is the latest version 9, the base is 0, we are basically doing
>>> this search for each non-versioned-entry inside this multi-release-jar file 9 times everytime when the entry
>>> is asked. In worse case scenario, a multi-release-jar, with huge number of entries with a small portion are
>>> versioned to 9, and you are iterating it via "entries". Each lookup might be cheap, but it might be worth
>>> considering adding some optimization.
>>> 
>>> Best,
>>> Sherman
>