Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Tue, 20 Sep 2022 17:37:50 GMT, Alan Bateman wrote:
>>> > OK, will make another pass at this today
>>>
>>> I looked at the latest draft
>>> ([2bafc00](https://github.com/openjdk/jdk/commit/2bafc00cc462b7af3f724371ac1bef5fd99c989c)).
>>> I think it would help if the section "Verifying a JarInputStream" were
>>> renamed to "Signed JAR files".
>>
>> 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.
>
> Thanks for the updates in 69138715. Just one comment on the updated text,
> and specifically this sentence:
>
> + * A {@code JarInputStream} may be used to verify the signatures of a
> + * signed JAR
> file
> + * assuming the following requirements are met:
>
> 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 href="{@docRoot}/../specs/jar/jar.html#signed-jar-file">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.
> Thanks for the updates in
> [6913871](https://github.com/openjdk/jdk/commit/691387157f01602ee25a1887223e00f880d071d1).
> Just one comment on the updated text, and specifically this sentence:
>
> * * A {@code JarInputStream} may be used to verify the signatures of a
> * * [signed JAR file]({@docRoot}/../specs/jar/jar.html#signed-jar-file)
> * * assuming the following requirements are met:
>
> 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]({@docRoot}/../specs/jar/jar.html#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.
Updated per your suggestion above.
-
PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Tue, 20 Sep 2022 10:49:59 GMT, Lance Andersen wrote:
>>> OK, will make another pass at this today
>>
>> I looked at the latest draft (2bafc00c). 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.
>
>> > OK, will make another pass at this today
>>
>> I looked at the latest draft
>> ([2bafc00](https://github.com/openjdk/jdk/commit/2bafc00cc462b7af3f724371ac1bef5fd99c989c)).
>> I think it would help if the section "Verifying a JarInputStream" were
>> renamed to "Signed JAR files".
>
> 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.
Thanks for the updates in 69138715. Just one comment on the updated text, and
specifically this sentence:
+ * A {@code JarInputStream} may be used to verify the signatures of a
+ * signed JAR
file
+ * assuming the following requirements are met:
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.
-
PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Tue, 20 Sep 2022 06:56:49 GMT, Alan Bateman wrote: >>> 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. >> >> OK, will make another pass at this today > >> OK, will make another pass at this today > > I looked at the latest draft (2bafc00c). 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. > > OK, will make another pass at this today > > I looked at the latest draft > ([2bafc00](https://github.com/openjdk/jdk/commit/2bafc00cc462b7af3f724371ac1bef5fd99c989c)). > I think it would help if the section "Verifying a JarInputStream" were > renamed to "Signed JAR files". 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. - PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Mon, 19 Sep 2022 10:21:30 GMT, Lance Andersen wrote: > OK, will make another pass at this today I looked at the latest draft (2bafc00c). 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. - PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Mon, 19 Sep 2022 06:45:13 GMT, Alan Bateman wrote: > 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. OK, will make another pass at this today - PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
On Sun, 18 Sep 2022 20:43:25 GMT, Lance Andersen wrote:
>> Please review this PR which updates the JarInputStream class description to
>> clarify when the Manifest is accessible via JarInputStream::getManifest and
>> JarInputStream::get[Jar]Entry.
>>
>> It is worth noting that with this update, we are finally documenting
>> behavior that dates back to when this class was added to JDK 1.2
>>
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated to address latest feedback
src/java.base/share/classes/java/util/jar/JarInputStream.java line 60:
> 58: *
> 59: * Verifying a JarInputStream
> 60: * {@link #JarInputStream(InputStream, boolean)} may be used to
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 a 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.
-
PR: https://git.openjdk.org/jdk/pull/10045
Re: RFR: 8215788: Clarify JarInputStream Manifest access [v7]
> Please review this PR which updates the JarInputStream class description to > clarify when the Manifest is accessible via JarInputStream::getManifest and > JarInputStream::get[Jar]Entry. > > It is worth noting that with this update, we are finally documenting > behavior that dates back to when this class was added to JDK 1.2 > > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updated to address latest feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10045/files - new: https://git.openjdk.org/jdk/pull/10045/files/a0b7ae03..c16e5bb8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10045=06 - incr: https://webrevs.openjdk.org/?repo=jdk=10045=05-06 Stats: 8 lines in 1 file changed: 1 ins; 5 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10045.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10045/head:pull/10045 PR: https://git.openjdk.org/jdk/pull/10045
