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

[Danesh Dadachanji]

On Thu, Oct 11, 2012 at 4:02 PM, Adam Domurad <adomurad@<adomurad at redhat.com>
redhat.com <adomurad at redhat.com>> wrote:
> 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).
>
>>> +    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);
> +    }
>

Oh yeah that should definitely be immutable. Very good catch, sorry that
slipped through!

>
>
>>
>>> +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.
>>

Heh that was my doing. I was following the Parser unit tests' formats for
multiple classes per functionality. I recall you mentioning that to me too
now, just slipped through the cracks though. :S

>
> OK for HEAD?
>

I've gone over all of the above patches, all the changes make sense to me
too. I just one nitpick for your third patch, the
FixForEmptySignedReproducer:

+        if (numSignableEntriesInJar == 0) {
+            // Allow jars with no signable entries to simply be considered
signed.
+            // There should be no security risk in doing so.
+            result = VerifyResult.SIGNED_OK;
+        }
+

Could you merge that if-statement with the ones above? I would even start
with this one and change the if(allEntriesSignedBySingleCert) to an else
if.

I can't quite okay this for HEAD because I have a lot of bias in this
review. =) Thanks for picking this up Adam!

Regards,
Danesh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121016/0296c818/attachment.html