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
> On Jun 30, 2019, at 7:38 PM, Philipp Kunz <philipp.k...@paratix.ch> wrote:
>
> Hi John, Max, and everyone
>
> Finally found some time again for this, please excuse the delay.
>
> Even though I think adding a keyalg parameter next to genkeypair is a
> different topic, it should not do any harm, and I added it.
>
> I find your proposal for how to print the manifests good. I preferred to
> display \n as well because I would otherwise think it was not there sooner or
> later and confuse myself. In that context also, I intentionally did not add
> an echoManifest method overload with a String parameter because it could and
> I believe also actually would confuse filenames and manifests both
> represented as String. Only a few times
> "Utils.echoManifest(Utils.readJarManifestBytes(" occurs which might be more
> verbose than necessary. I have also some doubts that all manifest are
> actually valid UTF-8. More fine-tuning might be desired.
>
> I also found running jarsigner slow which is why I suggested adding it as a
> tool like jar and others in a different thread and which was not accepted
> then. As for PreserveRawManifestEntryAndDigest, there is use of
> sun.security.tools.jarsigner.Main.main or run. Using
> sun.security.tools.jarsigner.Main re-uses loading of the keystore while
> avoiding another JVM which I believe to be almost perfect apart from the API
> used not looking very public. Also in terms of performance I think it should
> be equivalent to JarSigner::sign. When the output should be caputered, there
> is no way without another JVM (without the jarsigner tool).
>
> DigestDontIgnoreCase.getManifestBytes could reformat the manifest: agreed and
> changed accordingly. Now Utils.readJarManifestBytes. Note that it does not
> use a Manifest like before with JarFile.getManifest.
>
> EmptyIndividualSectionName.testNameEmptyTrusted: I honestly have no clue how
> this can be made to work. That is at least what I remember from when I wrote
> it half a year ago. We might as well come eventually to the conclusion that
> such a test is not necessary. The idea seems to be to invoke
> getTrustedAttributes("") on a manifest but it is not public. Help appreciated.
>
> About the several TODOs, FIXMEs, and FIXED_8217375: The tests in the patch
> have to be "kind of calibrated" to make sure things don't change with the
> patch that should not. Ideally these tests would have existed before but the
> have not. The tests that make sure that some cases still work the same now
> also contain some changes. In a usual case one would let the tests of a patch
> run against the old impl and see what has failed before, then apply the main
> source part of the patch and see something fixed, and not touching any other
> test guarantees that nothing else changes. In this particular case I found
> that not working. It is also not possible to create an independent patch with
> tests only because the tests closely relate to the patch. My idea was that
> all the FIXMEs and TODOs are removed before actually commiting the patch,
> after someone has confirmed that all the tests run fine against the previous
> impl which should make sure certain behavior has not changed. One of the
> tests does not even compile without the impl changes. After everyone has
> agreed that there are enough tests to make sure nothing changed by accident,
> we can remove those FIXMEs and TODOs and have then to pay close attention
> that no tests are removed or changed anymore. For tracking such changes, git
> would be an option, but in this case I figured I could not use it myself
> because I thought I should create a patch against the head branch and you
> could not see my branches anyway but maybe someone of you can create a branch
> and share it with me. In any case, the result should be only one commit
> ending up in the head branch (tip). If that sounds complicated it's what I
> encountered too. If someone has a better idea how to deal with it, please let
> me know.
>
> backwars - backwards - changed
>
> ManifestSigester - ManifestDigester - changed
>
> att - attr - changed
>
> Paths.get - Path.of - changed
>
> MainAttributesConfused.java - It is not necessary that a file with that name
> exists in order to show the effect. The signature gets confused if a manifest
> section exists. Such a section would be created when signing a jar file that
> contains a file with that name. I don't remember if I actually tried it with
> a file but testAddManifestMainAttributesSection shows that and how the patch
> makes a difference. Do you think another test should be created which signs a
> file with that name?
>
> PreserveRawManifestEntryAndDigest.java - I hope with the improved manifest
> encoding (\r and \n) on the console that an output overflow does not happen
> anymore. I remember having tried to reduce the number of tests as much as
> possible and justifyable already. I don't currently see a promising approach
> as how to substantially simplify it, hence, any hint appreciated, or I might
> come back to optimization at some later point. For now I was able to simplify
> getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA even though I
> would have preferred to be able to utilize OutputAnalyzer. I think to
> remember that verifying a jar file by opening it in verifying mode verifies
> all signers and in this case there is only one valid.
>
> Thanks for offering to look into the change in OutputAnalyzer.java because
> alternative would include/increase copy-paste.
>
> And from the other mail:
>
> ManifestDigester.java: if (!findSection(0, pos)) throw new
> IllegalStateException - moved exception back to JarSigner.SignatureFile.
>
> removed redundant 696 mfCreated = true;
>
> "You added a new check. Do you mean if the signer is the same then the SF
> will be rewritten anyway and there is no need to retain old MANIFEST.MF
> bytes?" - Yes, exactly. Added isBlockOrSF around line 900. See also
> WasSignedByOtherSigner test.
>
> "MainAttributesConfused.java, PreserveRawManifestEntryAndDigest.java,
> RemoveDifferentKeyAlgBlockFile.java, and SectionNameContinuedVsLineBreak.java
> fail on Windows because a JarFile object is not closed. You might want to use
> try-with-resources on them." - Hope I got them all.
>
> About tests timing out, I haven't encountered that but of course it has to
> run quick enough also on other machines. Before going into too much
> optimization I would appreciate and prefer to either get some (however remote
> or informal) confirmation that the set of test cases is appropriate or to get
> rid of superfluous tests before making them faster. Even more important is
> probably before anything else if the tests currently present are sufficient
> or if some test case has slipped and is still missing.
>
> One last note for now concerning Compatibility test. I hope it currently
> demonstrates what I think it should, verify jar signatures between jdk
> versions while preserving behavior with default parameters. The code itself
> requires probably a major clean-up before it can really be merged. I tried
> not to change more than necessary for now but it does not look now in an as
> good shape as how I originally encountered it. I'm also still thinking about
> Max's suggestion to add another test with a jarfile commited along with the
> tests to check compatibility but could still not yet convince myself that
> this would be comprehensive enough to replace the Compatibility additions. On
> the other hand, these compatibility test extensions have slightly more or
> different scope than the remainder of the patch, I guess, and leave some room
> for opinions and interpretation. Ideally, compatibility would have been
> covered with tests before and breaking signatures with increasing the
> manifest line width would have been noticed earlier. In my opinion there
> still is no better time than now to add such tests. At least we should make
> us confident not to break anything again this time.
>
> Thank you very much for all the good points. I appreciate all your input and
> find it very valuable.
>
> Regards,
> Philipp
>
>
>
>
>
> On Fri, 2019-06-21 at 11:32 +0800, Weijun Wang wrote:
>> The tests.
>>
>> - For all:
>>
>> We are thinking about removing default value for -keyalg for "keytool
>> -genkeypair" some day, so it will be better to add an explicit "-keyalg RSA"
>> or "-keyalg EC" now.
>>
>> There are several tests printing out the manifest in a sequence of
>> "[[0]=83='S', ...". While it is precise, I find it not very easy to check.
>> How about we just print out the string format but for each "\r" we just
>> print "\r" literally and keep printing "\n" with a new line, like this:
>>
>> Signature-Version: 1.0\r
>> Created-By: 13-internal (Oracle Corporation)\r
>> SHA-256-Digest-Manifest: Pxklci7ELW1zzSh2jZ+oz7TPR0WK2uTsAIiHar1m6Eo=\r
>> SHA-256-Digest-Manifest-Main-Attributes: ETfvdTNx3yN/ClSZz20wR0SM9ta8H7O\r
>> E7U0F4uZ9JV8=\r
>> \r
>> Name: abc\r
>> SHA-256-Digest: uTZ8UM3iJLPtl8W7+NEC/AC2auI7LP2Gu53ajj6jLgg=\r
>> \r
>>
>> If you find running jarsigner slow, is the JarSigner::sign API any better?
>> The verify side can also usually be replaced with open the file and read
>> everything inside unless you want to grab certain outputs, but those can
>> also be done with JarEntry::getSigners.
>>
>> - DigestDontIgnoreCase.java:
>>
>> getManifestBytes(). This could reformat the manifest (I understand it's
>> irrelevant in this test), you can just return
>>
>> jf.getInputStream(jf.getJarEntry(JarFile.MANIFEST_NAME)).readAllBytes()
>>
>> - EmptyIndividualSectionName.java:
>>
>> Are you going to do something around testNameEmptyTrusted()?
>>
>> - FindHeaderEndVsManifestDigesterFindFirstSection.java:
>>
>> Several TODO and FIXME.
>>
>> 52: typo: backwars-
>>
>> 76: No need to maintain FIXED_8217375 anymore
>>
>> 229: Typo: ManifestSigester
>>
>> - InsufficientSectionDelimiter.java:
>>
>> We usually abbreviate "attribute" to "attr" instead of "att".
>>
>> We usually use Path.of() instead of Paths.get() these days.
>>
>> - MainAttributesConfused.java:
>>
>> Have you tried adding a file named "Manifest-Main-Attributes" into a jar and
>> see if it would mess up anything?
>>
>> - PreserveRawManifestEntryAndDigest.java:
>>
>> This test is running for a long time. The output is also quite big and you
>> can see "Output overflow" inside which means if there is a failure it might
>> not show. Is it possible to simplify it?
>>
>> getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA(). The output
>> might change often. If you only focus on several lines, it's better to check
>> them with OutputAnalyzer::shouldMatch***. We can always manually construct
>> an OutputAnalyzer object.
>>
>> - DigestInput.java and FindSection.java:
>>
>> static final boolean FIXED_8217375 = true;
>>
>> Is this still needed?
>>
>> - OutputAnalyzer.java:
>>
>> This is a shared utility class and you changed the behavior. It might be
>> correct (start matching *after* the from line) but I cannot be sure if it
>> would break anything. I'll ask people about it.
>>
>> Thanks,
>> Max
>>
>>
>>
>>
>>
>>>
>>> On Jun 11, 2019, at 3:08 PM, Weijun Wang <
>>> weijun.w...@oracle.com
>>> > wrote:
>>>
>>> Hi Philipp,
>>>
>>> I'll start reviewing this code change. Since this is a P3 bug fix, we still
>>> have a month's time (before RDP 2 starts on 7/18) to work on it.
>>>
>>> Also, I've included John as a reviewer. He is the author of the
>>> Compatibility.java test.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>>
>>>> On May 23, 2019, at 9:49 AM, Weijun Wang <
>>>> weijun.w...@oracle.com
>>>> > wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> I've just uploaded your patch to
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~weijun/8217375/webrev.01/
>>>
>>>
>>>
>>
>>
>>
> <8217375-jarsignerbreaksexistingsignatures-20190630.patch>
