> On Jul 21, 2019, at 8:08 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
> 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.
Ah, I noticed the ManifestDigester::get will not return the main attributes
anymore. That's a clean up and I think the current code would not reach that
path.
--Max
>
> 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>
>
