Hi Philipp,
I just took a brief look at the new patch. Looks like the src change has no
behavior change (even in JarSigner.java with so many line changes). This is
good.
John and I have reviewed all the tests and we sent out some feedbacks in
previous mails, and it's definitely a part of the whole review process. The
single reason I integrated your changeset earlier this week is because RDP 2
starts at July 18 and after the date only P1-P2 src change would be approved
but we can still modify the tests. Before pushing the change, I ran all tests,
modified 2 regex patterns, and added one to problem list to make sure it makes
no (or, as little as possible) noise (esp for other people running the tests).
I haven't made any big change to the tests (including removing those
FIXED_xxxxxxx flags) myself because I was still waiting for your reply. Thanks
for coming back, so that we can start enhancing the tests now. Some test (Ex:
PreserveRawManifestEntryAndDigest.java) still spends a lot of time and could
intermittently time out.
I'll file a new bug and post the interdiff between the integrated change and
your new patch as the webrev.00 of it.
Thanks,
Max
> On Jul 21, 2019, at 4:49 AM, Philipp Kunz <philipp.k...@paratix.ch> wrote:
>
> Hi Max and everyone,
>
> • Indeed, [^\"] does not work with quotes inside CN. I agree to your
> change.
> • I went through the patch again myself and tried to understand what I
> did half a year ago and added and changed some comments and renamed a few
> identifiers so that I hope it becomes clearer. Particularly
> PreserveRawManifestEntryAndDigest is worth mentioning and had a bug in
> lineWidth70. Noteworthy is the test case with emptyJarTrailingSeqAddFile
> which is now better documented.
> • I also renamed atts to attrs.
> • It also still works without the Resources changes.
> • I removed testNameEmptyTrusted from EmptyIndividualSectionName after
> having convinced myself it is not necessary. Any other opinion on this
> welcome.
> • I moved some common functions used in many tests to Utils.
> Utils.echoManifest now prints manifests with non-printable characters escaped
> in Java style but that was in the previous patch already I think. It's used
> in more tests now than before.
> • I added a test testAddManifestMainAttributesFile in
> MainAttributesConfused that shows where the problems were with a file named
> Manifest-Main-Attributes inside a signed jar and that it works now.
> • I increased the timeout of PreserveRawManifestEntryAndDigest after it
> timed out on my machine a few times. The current timeout value is still
> pretty much arbitrary.
> • I defined KS_ARGS in various tests to be re-used with repeated
> jarsigner command line arguments.
>
>
>
> I'm sorry for recently not responding quickly. I admit that my personal
> interest has not increased with the latest development of webstart and I also
> have other duties or distractions. I welcome everyone to contribute missing
> parts or a review or an opinion. As I currently see it, the following parts
> are not yet complete.
>
> 1. I suggest Compatibility test be run with different JDK versions configured
> just like before with a focus on TSA now also with jar signinig compatibility
> across JDK versions on a central and powerful test infrastructure which is
> achieved by invoking Compatibility test again with different arguments.
> SignTwice is an example for that with only one JDK version.
>
> 2. Compatibility test should probably be thoroughly refactored. This can be
> done well after the merge in my opinion if no seriously harmful changes are
> revealed in a review.
>
> 3. I still did not have time to pick up Max's idea to add a jar file in
> binary form to the sources that would allow to test a current JDK version
> against it by trying to reproduce it identically. That would at least make
> incompatibilities detected earlier than with Compatibility test. How much
> such a test would overlap with the current Compatibility test once integrated
> in a build like before with TSA can be told only after it exists. In my
> opinion this makes a lot of sense in any case.
>
> 4. Somehow I still have not the feeling that someone has thoroughly reviewed
> the whole patch. It was mentioned that the main source changes look good but
> is that what a review takes? There are so many subtle changes mostly in
> ManifestDigester that I doubt there is way around a close look into the
> tests. It may be all right to check the details only after all the trivial
> checks passed but that patch has accumulated so much effort until now I
> really would appreciate some confirmation that everything looks somewhat
> promising so far, however non-committal. I might have missed a hint and feel
> still quite new as openjdk contributor.
>
> 5. Remove all the FIXED_8217375 stuff. In my opinion this requires that after
> the patch is more or less stable that someone verifies that all the test
> cases that did not exist before the patch continue to pass unchanged and test
> coverage is appropriate to calibrate existing behavior. Usually tests should
> exist before but this here is certainly one and hopefully a rare exception.
> It would be of no use if some tests were wrong claiming some behavior has not
> changed and with signed jars and digesting manifests every single difference
> invalidates the signatures without mercy. Actually removing the FIXED_8217375
> stuff is then trivial.
>
>
> Lot of fuzzy talk written, please accept my apology for spamming this way. I
> always appreciate any comment, hint, advice or anything.
>
> Regards,
> Philipp
>
>
>
> On Tue, 2019-07-16 at 21:29 +0800, Weijun Wang wrote:
>> I made another small change
>>
>> diff -iu
>> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java
>> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java
>> --- b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java
>> +++ b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java
>> @@ -717,7 +717,7 @@
>> + " Digest algorithm: " + signItem.expectedDigestAlg()
>> + (signItem.tsaIndex < 0 ? "" :
>> ")|("
>> - + "Timestamped by \"[^\"]+\" on .*"
>> + + "Timestamped by \".+\" on .*"
>> + ")|("
>> + " Timestamp digest algorithm: "
>> + signItem.expectedTsaDigestAlg()
>>
>> This is because there can be quotation marks inside CN. For example,
>>
>> Timestamped by "CN="Sectigo RSA Time Stamping Signer #1", O=Sectigo
>> Limited, L=Salford, ST=Greater Manchester, C=GB" on Tue Jul 16 12:23:29 UTC
>> 2019
>>
>> Thanks,
>> Max
>>
>>
>>
>>>
>>> On Jul 16, 2019, at 2:13 PM, Weijun Wang <
>>> weijun.w...@oracle.com
>>> > wrote:
>>>
>>> I'd also like to revert the changes to
>>> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java.
>>> Your change strips some trailing whitespaces but not all of them. This will
>>> trigger a re-translation of all Resources files and I think that is
>>> unnecessary.
>>>
>>> Will any of your new tests fail because of the revert? I'll run all tests
>>> soon.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>
>>>>
>>>> On Jul 16, 2019, at 11:34 AM, Weijun Wang <
>>>> weijun.w...@oracle.com
>>>> > wrote:
>>>>
>>>> 7/18 is near. If there is no new webrev, I'll push webrev.02 with
>>>> SignTwice.java disabled. We can still modify tests before 8/8.
>>>>
>>>> Thanks,
>>>> Max
>>>>
>>>>
>>>>>
>>>>> On Jul 10, 2019, at 10:34 AM, Weijun Wang <
>>>>> weijun.w...@oracle.com
>>>>> > wrote:
>>>>>
>>>>> [off the list]
>>>>>
>>>>> Hi Philipp,
>>>>>
>>>>> Do you have a new patch? The RDP 2 [1] starts on 7/18 and after that we
>>>>> will only be able to fix P1-P2 bugs after approvals.
>>>>>
>>>>> Let's get the changeset integrated before RDP 2 once there is no problem
>>>>> in src/. We can adjust the tests before RC (8/8).
>>>>>
>>>>> Thanks,
>>>>> Max
>>>>>
>>>>> [1]
>>>>> https://openjdk.java.net/jeps/3#rdp-2
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> On Jul 3, 2019, at 6:27 PM, Weijun Wang <
>>>>>> weijun.w...@oracle.com
>>>>>> > wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> On Jul 3, 2019, at 4:06 PM, Philipp Kunz <
>>>>>>> philipp.k...@paratix.ch
>>>>>>> > wrote:
>>>>>>>
>>>>>>> Hi Max,
>>>>>>>
>>>>>>> Do I understand correctly that your question refers to
>>>>>>> if (e == null && MF_MAIN_ATTRS.equals(name)) {
>>>>>>> e = getMainAttsEntry();
>>>>>>> }
>>>>>>> ?
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> If so, because this sticks closest to the previous behavior. According
>>>>>>> to src/java.base/share/classes/module-info.java, ManifestDigester is
>>>>>>> not publicly exported but how can it then be used then in JarVerifier
>>>>>>> in java.util.jar? If you or someone can confirm that we can identify
>>>>>>> all callers, I'd prefer to remove above three lines of code.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> It's not publicly exported therefore cannot be used by an application,
>>>>>> but it's accessible by java.util.jar which is also in the java.base
>>>>>> module.
>>>>>>
>>>>>> It can also be accessed by JDK modules listed in the "exports
>>>>>> sun.security.util to" block of java.base/share/classes/module-info.java.
>>>>>>
>>>>>> Anyway any caller must be inside JDK. IntelliJ IDEA tells me its only
>>>>>> SignatureFileVerifier and JarSigner. Hopefully whatever IDE you are
>>>>>> using it gives the same result.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Otherwise, with above three lines, main attributes and an entry with
>>>>>>> name MF_MAIN_ATTRS will still be confused for all those code continuing
>>>>>>> to call get as before the patch if an entry with such a name exists
>>>>>>> just like before. However, with the patch, all (known) places that call
>>>>>>> ManifestDigester.get* are lines 539 and 606 in SignatureFileVerifier
>>>>>>> with the patch, 539 calling getMainAttsEntry without any ambiguity and
>>>>>>> 606 calling get(String, b) always expecting the entry for a section
>>>>>>> that is present and would otherwise not be known to pass as an argument
>>>>>>> anyway, provided the signature file does not contain more entries than
>>>>>>> the manifest or entries have not been removed from the manifest after
>>>>>>> signing, should work correctly, I guess. If we cannot get rid of those
>>>>>>> three lines, another test for those edge cases should probably be
>>>>>>> added, shouldn't it, if not to support the previous confusion at least
>>>>>>> to show it now works as expected?
>>>>>>>
>>>>>>> What exactly did you mean with "coding error"?
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Not sure. I don't know if any bad thing would happen if a file named
>>>>>> "Manifest-Main-Attributes" is out inside a jar file.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Along with removing above three lines of code, two tests would have to
>>>>>>> be adjusted. FindHeaderEndVsManifestDigesterFindFirstSection on lne 235
>>>>>>> and DigestInput on line 270.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> That's OK. No real app should call methods in this class directly.
>>>>>>
>>>>>> Thanks,
>>>>>> Max
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Philipp
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, 2019-07-01 at 15:41 +0800, Weijun Wang wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> https://cr.openjdk.java.net/~weijun/8217375/webrev.02
>>>>>>>>
>>>>>>>> uploaded. There are still several trailing spaces and I've removed
>>>>>>>> them.
>>>>>>>>
>>>>>>>> I just ran a test job: SignTwice.java failed on Windows with `failed
>>>>>>>> to clean up files after test`. Most likely a file is not closed.
>>>>>>>>
>>>>>>>> The src change looks good. In fact, I am wondering if we need to
>>>>>>>> support ManifestDigester.get(MF_MAIN_ATTRS, b) at all. Is it possible
>>>>>>>> we miss a coding error because of it?
>>>>>>>>
>>>>>>>> I'll take a look at the tests.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Max
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
> <8217375-jarsignerbreaksexistingsignatures-20190719.patch>
