RFR 8217375: jarsigner breaks old signature with long lines in manifest

sha.jiang at oracle.com sha.jiang at oracle.com
Fri Jun 28 12:50:41 UTC 2019


Hi,
Sorry for this late reply!

I only focus on the changes on the tests in 
sun/security/tools/jarsigner/compatibility.
Compatibility.java has been modified significantly, I just had a quick 
look at it.

sun/security/tools/jarsigner/compatibility/SignTwice.java
--------------------
With my testing, this test usually got timeout in our CI testing system.
The timeout is 480 seconds.


sun/security/tools/jarsigner/compatibility/Compatibility.java
--------------------
  38 import java.io.ByteArrayOutputStream;
  44 import java.io.FilterOutputStream;
  65 import java.util.jar.Attributes;
The above import statements are unnecessary.

  82     private static JdkInfo TEST_JDK_INFO;
100     private static String[] KEY_ALGORITHMS;
111     private static String[] DIGEST_ALGORITHMS;
Now that these variables are not constant anymore, the names would be 
testJdkInfo, keyAlgorithms and digestAlgorithms.

161         System.out.println("using sigfile " + sigfileName + " for 
alias "
162                     + alias + " signing " + u + ".jar to " + s + 
".jar");
Could it be better to use System.out.printf()?

409     private static String outfile() {
410         return System.getProperty("o");
411     }
Would it be better to define a constant for outfile?

416         String[] jdkList = list("jdkList");
417         if (jdkList.length == 0) {
418             jdkList = new String[] { "TEST_JDK" };
419         }
420
421         List<JdkInfo> jdkInfoList = new ArrayList<>();
422         for (String jdkPath : jdkList) {
423             JdkInfo jdkInfo = "TEST_JDK".equalsIgnoreCase(jdkPath) ?
424                     TEST_JDK_INFO : new JdkInfo(jdkPath);
Not sure understand this update.
Now that the constant TEST_JDK is the current testing JDK path, then new 
JdkInfo(TEST_JDK) should be equivalent to TEST_JDK_INFO in this update.

435     private static List<String> keyAlgs() throws IOException {
436         if (KEY_ALGORITHMS == null) KEY_ALGORITHMS = list("keyAlgs");
437         if (KEY_ALGORITHMS.length == 0)
438             return Arrays.asList(DEFAULT_KEY_ALGORITHMS);
439         return Arrays.stream(KEY_ALGORITHMS).map(a -> a.split(";")[0])
440                 .collect(Collectors.toList());
441     }
...
467     private static List<String> digestAlgs() throws IOException {
468         if (DIGEST_ALGORITHMS == null) DIGEST_ALGORITHMS = 
list("digestAlgs");
469         if (DIGEST_ALGORITHMS.length == 0)
470             return Arrays.asList(DEFAULT_DIGEST_ALGORITHMS);
471         return Arrays.asList(DIGEST_ALGORITHMS);
472     }
It looks better that the above methods return String array rather than 
list. Then, the callings on Arrays.asList() are unnecessary.

793             System.out.println("verifyingStatus: error: exit code != 
" + expectedExitCode + ": " + outputAnalyzer.getExitValue() + " != " + 
expectedExitCode);
...
799             System.out.println("verifyingStatus: error: 
expectedSuccessMessage not found: " + expectedSuccessMessage);
...
810                 System.out.println("verifyingStatus: error: 
line.matches(" + Test.ERROR + "\" ?\"): " + line);
The above lines are too long.


sun/security/tools/jarsigner/compatibility/JdkUtils.java
--------------------
The year should be updated in the copyright notes.

Best regards,
John Jiang

On 2019/6/21 11:34, Weijun Wang wrote:
> BTW, I haven't looked at the changes in test/jdk/sun/security/tools/jarsigner/compatibility yet.
>
> John, do you have any comment in this area?
>
> Thanks,
> Max
>
>> On Jun 21, 2019, at 11:32 AM, Weijun Wang <weijun.wang at oracle.com> 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.wang at 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.wang at oracle.com> wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> I've just uploaded your patch to
>>>>
>>>> http://cr.openjdk.java.net/~weijun/8217375/webrev.01/
>


More information about the security-dev mailing list