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

[Adam Domurad]

Hey Danesh, thanks very much for the look-over.

On 10/16/2012 10:51 AM, Danesh Dadachanji wrote:
>
> On Thu, Oct 11, 2012 at 4:02 PM, Adam Domurad <adomurad@ 
> <mailto:adomurad at redhat.com>redhat.com <mailto: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.
>

Yes, this is probably a bit better. Updated patch attached.

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

Yet you're the most qualified to :)
Thanks for the extra set of eyes!

> Regards,
> Danesh
>

Cheers,
- Adam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121016/ff83eecd/attachment.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: FixForEmptySigned2.patch
Type: text/x-patch
Size: 2861 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121016/ff83eecd/FixForEmptySigned2.patch