[rfc][icedtea-web] policytool in itweb-settings

[Jacob Wisor]

On 01/14/2014 07:16 PM, Andrew Azores wrote:
> On 01/14/2014 10:34 AM, Jiri Vanek wrote:
>> On 01/14/2014 03:16 PM, Jacob Wisor wrote:
>>> On 01/14/2014 01:42 PM, Jiri Vanek wrote:
>>>> On 01/14/2014 12:33 AM, Jacob Wisor wrote:
>>>>> Hello there!
>>>>>
>>>>> On 01/13/2014 11:20 PM, Andrew Azores wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This small patch hooks the JDK policytool into itweb-settings. It can then be
>>>>>> used to set up a custom user-level JNLP policy - this, in combination with
>>>>>> the
>>>>>> Run in Sandbox patch, will allow for quite a lot more flexibility in how
>>>>>> permissions are handled with signed applets/applications.
>>>>>>
>>>>>> A nicer, more user-friendly editor to replace the policytool will
>>>>>> hopefully come
>>>>>> later on.
>>>>>
>>>>> Oooooooh yes, please! This would be awesome! :-)
>>>>
>>>> Yes this would be :))
>>>> But it is different task. And Quite complex. Especially it must pass upstream
>>>> (openjdk). And that is *the* task!
>>>
>>> Well, it does not need to replace or displace OpenJDK's policytool. It should
>>> be probably enough
>>> that it complements it. ;-) You know, it's neither forbidden nor against any
>>> spec to build
>>> augmenting tools around OpenJDK.
>>
>> But still contribute to Openjdk itself is the right thing to do.
>>
>> ...
>>>
>>> At first, I thought that the problem would rather be that some system
>>> configurations may be missing
>>> a PATH environment variable entry to policytool and thus launching it may
>>> fail. But, Jiri is right.
>>> The best approach here would probably be to call directly into policytool's
>>> main() method with
>>> user.home as its current working directory. policytool is part of rt.jar, not
>>> tools.jar, so it
>>> should already be on bootclasspath. But, you may have to investigate deeper
>>> into it because the
>>
>> Great!
>>
>> But I doubt "with  user.home as its current working directory" is possible.
>> Or do we misunderstand each other?
>>
>> I had in my mind simple PolicyTool.main(arg,aerg,arg) call in itw-settings.
>>
>>
>>> package names of some tools have changed from OpenJDK 6 over to OpenJDK 7.
>>
>> I doubt policy tool was touched in last 10 years ;)
>>
>> J.
>>
>
> [...]
>
> The actual PolicyPanel is now launching the PolicyTool by invoking its main
> method, rather than exec'ing a new process.

After I have hit the send button for my last e-mail, I have realized that it may 
be desirable to run policytool in a separate process for security reasons, 
although calling directly into PolicyTool.main(). Maybe it's just me being 
paranoid, but authoring policies is a security sensitive task. In the current 
patch policytool gets loaded into the same JVM as itw-settings. If the current 
JVM or itw-settings has been compromised for the current user, he/she may be 
editing a heck of secure policy but could end up with a policy file on disk, 
granting AllPermissions to some malicious code bearing URL. Sure, policy files 
are simple human readable text files that can be checked before applying. But, 
who really does that?
Of course, I do realize that if one's system JRE has been compromised then 
probably all other JVM instances and the applications running on them are going 
to get compromised sooner or later on that system. I don't know, I just wanted 
to share some wired thoughts on policy authoring security. :-D

> I've put this into a new thread, but tbh I'm not sure if that was entirely necessary.

In this case, I had rather something like SwingUtilities.invokeLater() or 
EventQueue.invokeLater() in mind. In general, only activities related to 
manipulating and displaying UI elements should be placed on the AWT thread. Work 
code should either be put into invokeLater() or custom working threads (like 
SwingWorker for example), although the later has some more caveats in most 
cases. invokeLater() actually does indeed create a thread too but it also neatly 
ties it into the UI toolkit and takes care of most of those caveats, so 
developers are really advised to use it. You may want to read the tiny 
discussion on "Swing's Threading Policy" at the bottom of javax.swing's package 
API documentation regarding this topic. ;-)

> The "View Policy" button has a tooltip now that indicates the location of the file that will be opened,
> although this is also displayed in the policytool itself if you launch it.

Right, that's actually why I have remained silent on that matter. It's a good 
move to indicate the policy file's location before launching policytool.

> The View Button's action listener is no longer an anonymous inner class, since I
> don't really mind the style either way personally.

Yey, I love inner classes! :-D Declaring a member variable holding the 
ActionListener's reference would have been enough too, but hey, going all the 
way through is even cooler. 8-)

> Its implementation does have an anonymous inner Runnable inside its Thread, though.

An anonymous Runnable implementation without keeping a reference to it is okay 
here because in any case, there can only be one Runnable object per Thread 
object, hence a reference to that anonymous Runnable object is always 
unambiguously retrievable. Nevertheless, you may want to prefer invokeLater() 
over that custom thread.

> Oh, and there's also now an error dialog if for some reason the policy file can't be opened (eg it exists
> but you don't have read permission, or it exists but is a directory, or
> whatever). If the file doesn't already exist then we attempt to create a blank
> one there first, since the PolicyTool itself doesn't seem to do this.

Although all JOptionPane.showXxxDialog() dialogs are modal, please pass it a 
reference to itw-settings' JFrame for the sake of completeness, before it grabs 
some undesired /default/ Compenent - what ever this might be - or perhaps even 
the desktop window.

I hope you are sure about what you have been probably intending to accomplish 
with canOpenPolicyFile() actually does what you had in mind. File.canRead() and 
File.canWrite() are not platform agnostic and they are only checking file system 
access permissions, not physical read/write capability. Those methods are so 
misleading in their semantics and should not have found their way into the Java 
API in the first place. The JCP board would be well advised to deprecate these 
methods as soon as possible for being highly platform dependent and having 
misleading semantics. I have already covered this in an earlier post to this 
mailing list. ;-)
Besides File.canRead() and File.canWrite() also test for existence so you may 
want to simplify and drop some redundant tests in you current code. I may be 
mistaken, but there may already be code present in 
net.sourceforge.jnlp.util.FileUtils for your intended purpose.
IMHO, you should probably drop these tests anyway because it's on the policytool 
to do those tests (from a software engineering point of view).

> [...]

Jacob