Code Review Request: running signed modules with SecurityManager

[Sean Mullan]

On 6/4/10 2:31 PM, mark.reinhold at oracle.com wrote:
>> Date: Tue, 01 Jun 2010 09:50:21 -0400
>> From: sean.mullan at oracle.com
>
>> I posted a new webrev addressing the comments I have received so far:
>>
>> http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/SecurityManager2/webrev.01/
>
> Mostly looks good, but I'd like to see a few changes before you push.
>
> SimpleLibrary.java
>
>    [55] Is there a reason to put signer.ser in its own subdirectory?

Just trying to think ahead - we'll likely want to store additional security 
information such as the module's granted permissions, and potentially other 
certificate related information such as CRLs.

> If not, please change that.  Also, please name it "signer", without the
>    extension.

Ok.

>    [522] Serialization is a pretty heavyweight (and brittle) solution
>    here.  I can live with it for now, but in a future changeset please
>    modify this code to store and load CodeSigners using the existing
>    conventions for metadata files.

Ok, I will address this in a subsequent changeset.

>
>    [540] Casts should not be separated from the expression being cast,
>    i.e., (CodeSigner)ois.readObject(), not (CodeSigner) ois.readObject() .

Ok.

> Loader.java
>
>    The codeSourceForName map is awkward.  Please add a codeSource property
>    to java.lang.reflect.Module instead.  That will obviate the need for
>    this map and also make the CodeSource available for other uses.

Sounds good.

>    [196] If there's no CodeSigner then shouldn't you pass null as the last
>    argument to defineModule, rather than new CodeSource(null, null)?

No, I don't believe so. There is a subtle difference. A CodeSource of (null, 
null) will still be granted permissions where the URL/certs don't matter, ex the 
permissions of the sandbox policy. But a null CodeSource won't be granted any 
permissions.

> ModuleFileFormat.java
>
>    [831] Knowledge of the format of the signer file should be encapsulated
>    in one place.  When installing a module file the SimpleLibrary.install
>    method already gets the signers, by invoking Reader.verifySignature,
>    so it'd be better to store the signers at that point rather than in the
>    module-file reader.  Moving this code now will also make it possible to
>    eliminate serialization later.

Yes, good point.

I'll try to post an updated webrev later today.

Thanks,
Sean