[icedtea-web][rfc] Update on Danesh's major rework of JarCertVerifier

[Adam Domurad]

Thanks for the review!

Attached is an updated patch to the base patch by Danesh (with your 
recommended changes + a change to JCV to use specific imports instead of 
full-package imports).

Specific answers/changes included inline.

On 10/10/2012 12:26 PM, Omair Majid wrote:
> On 09/25/2012 10:59 AM, Adam Domurad wrote:
>> +     * @param bad
>> +     *            3 booleans to show if the KeyUsage, ExtendedKeyUsage, NetscapeCertType has codeSigning flag turned on. If null, the class field badKeyUsage, badExtendedKeyUsage, badNetscapeCertType will be set.

Line wrapped now, as it was prior to the patch.

>
> Please consider line-wrapping this.
>
>> +    public Map<String, Integer> getJarSignableEntries() {
>> +        return jarSignableEntries;
>> +    }
> If untrusted code were able to invoke this method, would that compromise
> the security of the system? Should we be returning a mutable data
> structure here?

Changed to:
+    public Map<String, Integer> getJarSignableEntries() {
+        return Collections.unmodifiableMap(jarSignableEntries);
+    }


>
>> +public class VerifyJarEntryCertsTest {
> Could you rename this class to ${CLASS_IT_TESTS}Test? That would be
> JarCertVerifierTest. Otherwise it will be harder to find this the next
> time someone is updating JarCertVerifier.
>
>> +        // This calls ReadPropertiesSigned with user.home, it is not easy to think of a pattern to match this
>> +        // Instead we make sure _something_ was printed
>> +        Assert.assertFalse("stdout should NOT be empty, but was", pr.stdout.isEmpty());
>> +        Assert.assertFalse("stderr should NOT contains `" + accExcString + "`, but did", pr.stderr.contains(accExcString));
>> +    }

Done

> Maybe you can you some other system property that's more predictable
> (like, say "java.vm.specification.name") ?

Not done (review to relevant patch in separate answer), as discussed, 
this property does not exercise signed privileges.

>
> Rest looks okay to me.

OK for HEAD?

>
> Cheers,
> Omair

Cheers,
- Adam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jcv3.patch
Type: text/x-patch
Size: 82979 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121011/0929dc6b/jcv3.patch