8215788: Clarify JarInputStream Manifest access #10045
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -31,51 +31,50 @@ | ||
| import jdk.internal.util.jar.JarIndex; | ||
|
|
||
| /** | ||
| * The {@code JarInputStream} class, which extends {@link ZipInputStream}, | ||
| * is used to read the contents of a JAR file from an input stream. | ||
| * It provides support for reading an optional {@link JarFile#MANIFEST_NAME Manifest} | ||
| * entry. The {@code Manifest} can be used to store | ||
| * meta-information about the JAR file and its entries. | ||
LanceAndersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * <h2>Accessing the Manifest</h2> | ||
| * <p> | ||
| * The {@link #getManifest} method will return the {@code Manifest} when it is | ||
| * the first entry in the stream or {@code META-INF/} is the first entry and | ||
| * the {@code Manifest} is the second entry within the stream. When the | ||
|
There was a problem hiding this comment. I think you can insert a comma after "when it is the first entry in the stream"? I think that would make it a bit clearer that there are two cases. Also I'm wondering if the paragraph should be split into two, meaning "When the Manifest ..." can be the start of a new paragraph. The reason is that the text is trying to explain two things, the first is that the manifest must be at the start of the JAR file, the second is that the ordering that methods are invoked will influence how other methods behave. There was a problem hiding this comment.
Done
Moved to a new paragraph per your suggestion |
||
| * {@code Manifest} is returned by {@code getManifest()}, the {@link #getNextEntry()} | ||
| * and {@link #getNextJarEntry()} methods will not return the {@code Manifest}. | ||
| * If {@code META-INF/} is the first entry in the input stream it will be | ||
| * also not be returned by {@link #getNextEntry()} and | ||
| * {@link #getNextJarEntry()}. | ||
|
There was a problem hiding this comment. JAR files will almost always have the manifest as the first or second entry. It's very hazardous to have getNextEntry/getNextJarEntry work differently when the manifest is present but not at the start. This makes it really hard to specify too. I wonder what the impact would be of changing the implementation so that getNextEntry/getNextJarEntry consistently skip the manifest rather than only when it's at the start. I can't think of anything right now that could dependent on the current behavior where it might or might be returned. There was a problem hiding this comment. I tend to agree but am reluctant to change 20+ year behavior via this PR as we don't know what we don't know. There was a problem hiding this comment. Okay, in which case what would you think about just saying that the getNextEntry/getNextJarEntry method do not return the Manifest when it's at the start of the stream, and it's unspecified whether they return the Manifest when it located later in the stream. I think this would give us wriggle room to change it in the future. There was a problem hiding this comment.
Is this any better: There was a problem hiding this comment. It's a bit better but I think we can make it clearer and also link the JAR Manifest section of the JAR file spec. Can you try this: I think we also have to update getManifest method to align with the above as doesn't say anything about the manifest needing to be at the beginning of the stream. There was a problem hiding this comment.
Revised using the above
Ok, I added some verbiage similar to what I had originally before we decided to update the class description. Please let me know if this is what you had in mind Thank you again for your feedback |
||
| * </p> | ||
| * <p> | ||
| * {@link JarEntry#getAttributes()} will return the {@code Manifest}'s | ||
| * attributes for the current JAR file entry, if any, providing | ||
| * {@code getManifest()} returns a {@code Manifest} for the JAR file. | ||
|
There was a problem hiding this comment. Per-entry attributes is an advanced feature to attempt to bring into the class description. I think it would be simpler to just drop this paragraph. If you really want something on this topic then it would require first describing main vs. per entry attributes and then explaining that the per-entry attributes are obtained with JarEntry::getAttributes when the manifest is at the beginning of the stream. There was a problem hiding this comment. I can remove, but I am not sure I agree we need to describe main vs attribute here given we are pointing to the Jar spec and if there is any discussion of Pre-entry attributes, it should be in JarEntry IMHO. I guess the clarification I was trying to make, apparently unsuccessfully is that There was a problem hiding this comment.
Wording it is hard. The draft wording made it look that must call getManifest, ignore the return value, and then subsequent calls to JarEntry::getAttributes will return attributes for the JAR file entry. I think to properly describe would require more setup to explain that a manifest can optionally include per entry attributes and these are read with JarEntry::getAttributes when the manifest is found at the beginning of the stream.. There was a problem hiding this comment.
Thinking about this some more, would the following be any better: There was a problem hiding this comment. Does this mean that the "Verifying a JarInputStream" should also avoid mentioning "getManifest method returns the manifest"? I understand precisely it should be "getManifest method is able to return the manifest if you call it". It almost sounds like we should first define the concepts of "well-formed JAR file" and "well-formed signed JAR" and then specify what these methods behave. There was a problem hiding this comment.
See Alan's comment below. I will be copying the wording regarding the Manifest being the 1st/2nd entry
The Manifest location requirement is unique to JarInputStream so really want to try to keep these updates to a minimum if at all possible so that we are not copying parts of the Jar spec into the javadoc. |
||
| * </p> | ||
| * | ||
| * <h2>Verifying a JarInputStream</h2> | ||
LanceAndersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * {@link #JarInputStream(InputStream, boolean)} may be used to | ||
|
There was a problem hiding this comment. I realise you've had a few iterations with Max on this section but I'm concerned that the text is telling the reader that they should use the 2-arg constructor to verify the signatures when a JAR is signed. The default is to verify and the main reason to use the 2-arg constructor is when you want to opt out, not opt-in. I think the intro to this section will need to start with a sentence to say that JAR files can be signed (link to specs/jar/jar.html#signed-jar-file) and that JarInputStream can read a signed JAR from the input stream. As per the description further up, the manifest must be at the start of the stream. There was a problem hiding this comment.
OK, will make another pass at this today There was a problem hiding this comment.
I looked at the latest draft (2bafc00). I think it would help if the section "Verifying a JarInputStream" were renamed to "Signed JAR files". The link to getManifest makes the reader wonder if they have to call this method whereas I think what you want to say that the manifest must be at the start of the stream (as per the first section) and then followed by signature entries. There was a problem hiding this comment.
OK, I will change as you suggest The link to getManifest makes the reader wonder if they have to call this method whereas I think what you want to say that the manifest must be at the start of the stream (as per the first section) and then followed by signature entries. The reason I used the getManifest wording is I felt it was easier and less redundant than copying the wording about the Manifest needing to be either the first or second entry (assuming META-INF/ is the first in the stream). However if you prefer that, I will make that change. There was a problem hiding this comment. Thanks for the updates in 6913871. Just one comment on the updated text, and specifically this sentence:
The signatures are verified by default so I think you can reduce this down to: A {@code JarInputStream} verifies the signatures of entries in a Signed JAR file when: We could say that a program could opt-out of verification by using the 2-arg constructor but that is probably too much to try to fit in here. There was a problem hiding this comment.
Updated per your suggestion above. |
||
| * verify the signatures of a signed {@code JarInputStream} assuming the | ||
| * following requirements are met: | ||
| * <ul> | ||
| * <li> | ||
| * The {@link #getManifest()} returns a {@code Manifest} for the JAR | ||
| * file | ||
| * </li> | ||
| * <li> | ||
| * All signature-related entries immediately follow the {@code Manifest} | ||
| * </li> | ||
| * </ul> | ||
| * Once the {@code JarEntry} has been completely verified, which is done by | ||
| * reading until the end of the entry's input stream, | ||
| * {@link JarEntry#getCertificates()} may be called to obtain the certificates | ||
| * for this entry and {@link JarEntry#getCodeSigners()} may be called to obtain | ||
| * the signers. | ||
LanceAndersen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * <p> | ||
| * <b>Note:</b>If a {@code JarEntry} is modified after the Jar file is signed, | ||
|
There was a problem hiding this comment. Add a space before |
||
| * a {@link SecurityException} will be thrown when the entry is read. | ||
| * </p> | ||
| * | ||
| * @author David Connelly | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think about linking this to {@docroot}/../specs/jar/jar.html#jar-manifest rather tan JarFile#MANIFEST_NAME?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure if that is your preference.