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

[Andrew Azores]

On 01/14/2014 06:34 PM, Jacob Wisor wrote:
> 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 think this is just being a little too paranoid maybe :) if the JVM is 
already compromised then there are already some big problems, like you 
said, and I think the way in which we launch the policytool (or later to 
come "friendly editor") is the least of our worries.

>
>> 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. ;-)

I'm really not very good/experienced with GUI stuff, so I really 
appreciate all the help here. Thanks!

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

Is the tooltip agreeable enough or would you prefer some other more 
immediately obvious indicator of the file location?

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

Great, thanks again for the input on this fancy GUI stuff ;)

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

I know that the checks I'm using are not necessarily hard guarantees 
that the file will truly be accessible (meaning we have actual physical 
read/write as well as file system permissions etc) in the end, and that 
it's policytool's job to actually handle the problem in the event that 
it isn't. But something policytool doesn't do if the file isn't 
accessible is show a nice error notifying the user! So these checks are 
just there so we can show the user an error message (the JOptionPane 
above) if we suspect that policytool won't be able to properly open the 
file. Otherwise, in order to test if policytool actually can 
successfully edit the file, we'll need to do something like go back to 
launching it by exec'ing a new process and checking its exit code once 
it terminates, which I think might be undesirable and a little overly 
complex for what we really need here. The next policytool should 
probably display a dialog like this on its own when it can't open the 
specified file, and then we can remove this approximation from the 
PolicyPanel.

I have looked in our FileUtils class and the only thing I saw of use in 
there for this purpose was createParentDir, which I am using in this 
same method.

Also, which redundant tests do you mean? The "if (!policyFile.exists())" 
check when there's also canRead() and canWrite() after? The exists() 
check is there so we can "touch" (createNewFile()) before launching 
policytool. This is necessary because the existing policytool doesn't 
play nice if you give it a path to a file that doesn't yet exist - it 
will launch as if you started it without specifying any path at all, so 
when you finish creating your new policy and go to save it, it will open 
a file chooser dialog. Since the policy file has a well-defined expected 
location, I'm doing this work before launching the policytool to try to 
ensure, loosely, that the policytool will actually save the file to the 
correct location. Again, once we have a nicer editor, then this work 
will not be necessary and can be removed, but right now it is necessary IMO.

Attached is a new patch using SwingUtilities.invokeLater, passing the 
ControlPanel JFrame reference to the JOptionPane.showMessageDialog, and 
with an improved View Policy button tooltip.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: custom_policy3.patch
Type: text/x-patch
Size: 9949 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140115/6effc775/custom_policy3-0001.patch