<html><head></head><body><div>Hi Max and everyone,</div><div><br></div><ul><li>Indeed, [^\"] does not work with quotes inside CN. I agree to your change.</li><li>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.</li><li>I also renamed atts to attrs.</li><li>It also still works without the Resources changes.</li><li>I removed testNameEmptyTrusted from EmptyIndividualSectionName after having convinced myself it is not necessary. Any other opinion on this welcome.</li><li>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.</li><li>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.</li><li>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.</li><li>I defined KS_ARGS in various tests to be re-used with repeated jarsigner command line arguments.</li></ul><div><br></div><div><br></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>Lot of fuzzy talk written, please accept my apology for spamming this way. I always appreciate any comment, hint, advice or anything.</div><div><br></div><div>Regards,</div><div>Philipp</div><div><br></div><div><br></div><div><br></div><div>On Tue, 2019-07-16 at 21:29 +0800, Weijun Wang wrote:</div><blockquote type="cite"><pre>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
<blockquote type="cite">
On Jul 16, 2019, at 2:13 PM, Weijun Wang <<a href="mailto:weijun.wang@oracle.com">weijun.wang@oracle.com</a>> 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
<blockquote type="cite">
On Jul 16, 2019, at 11:34 AM, Weijun Wang <<a href="mailto:weijun.wang@oracle.com">weijun.wang@oracle.com</a>> 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
<blockquote type="cite">
On Jul 10, 2019, at 10:34 AM, Weijun Wang <<a href="mailto:weijun.wang@oracle.com">weijun.wang@oracle.com</a>> 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] <a href="https://openjdk.java.net/jeps/3#rdp-2">https://openjdk.java.net/jeps/3#rdp-2</a>
<blockquote type="cite">
On Jul 3, 2019, at 6:27 PM, Weijun Wang <<a href="mailto:weijun.wang@oracle.com">weijun.wang@oracle.com</a>> wrote:
<blockquote type="cite">
On Jul 3, 2019, at 4:06 PM, Philipp Kunz <<a href="mailto:philipp.kunz@paratix.ch">philipp.kunz@paratix.ch</a>> wrote:
Hi Max,
Do I understand correctly that your question refers to
if (e == null && MF_MAIN_ATTRS.equals(name)) {
e = getMainAttsEntry();
}
?
</blockquote>
Yes.
<blockquote type="cite">
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.
</blockquote>
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.
<blockquote type="cite">
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"?
</blockquote>
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.
<blockquote type="cite">
Along with removing above three lines of code, two tests would have to be adjusted. FindHeaderEndVsManifestDigesterFindFirstSection on lne 235 and DigestInput on line 270.
</blockquote>
That's OK. No real app should call methods in this class directly.
Thanks,
Max
<blockquote type="cite">
Regards,
Philipp
On Mon, 2019-07-01 at 15:41 +0800, Weijun Wang wrote:
<blockquote type="cite">
<a href="https://cr.openjdk.java.net/~weijun/8217375/webrev.02">https://cr.openjdk.java.net/~weijun/8217375/webrev.02</a>
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
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</pre></blockquote></body></html>