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

[Jiri Vanek]

On 01/15/2014 11:09 PM, Andrew Azores wrote:
> On 01/15/2014 10:24 AM, Jiri Vanek wrote:
>> On 01/15/2014 12:34 AM, Jacob Wisor wrote:
>>> 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
>>
>> Ugh. I hope this is not true. But Ido not know.
>> Btw - by specifying policies - the itw-settings will be still unaffected correct?
>
> What do you mean?
>
>>>
>>>> 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. ;-)
>>
>> +1!
>>
>>>
>>>> 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.
>>
>> as above
>>>
>>>> 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.
>> +1
>
> All this was already taken care of in the '3' patch, I think.
>
>>
>>>
>>> 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).
>>
>> This have already appeared, Jacob, have not? :o)
>>
>> +    public static boolean canOpenPolicyFile(final File policyFile) throws IOException {
>> +        FileUtils.createParentDir(policyFile);
>> +        if (!policyFile.exists()) {
>> +            policyFile.createNewFile();
>> +        }
>> +        return policyFile.isFile() && policyFile.canRead() && policyFile.canWrite();
>> +    }
>>
>>
>> For a directory you can use DirectroyValidator class in itw. It do what you wont to do in full scale.
>>
>> imho the policyFile.canWrite(); is not necessary (the message "can not write" is worthy anyway )
>> . The toll is useful also for read only viewing..
>> The messages "is not file" or "can not be read" or "cannot be written" are better then one generic
>> one.
>>
>> Thanx both of you for contributions!
>>
>>
>> J.
>
> Okay, the "couldNotOpenFileDialog" is more informative about the specific reason that it's appearing
> now, and I'm using a DirectoryValidator to check that we'll be able to make/edit a policy file. I've
> also removed the requirement that the file be writeable - you're right, it does still make sense to
> let the tool launch, even if the file is read-only. And it's policytool's responsibility to deal
> with the problem if it turns out the file is read-only. Besides, I did label the button as "View
> Policy", not "Edit Policy" ;)
Please remove unnecessary html from:
+        JLabel aboutLabel = new JLabel("<html>" + R("CPPolicyDetail") + "</html>");

Also I would like two things here:
  - If file is not writeable, warn user and lunch policy tool anyway
  - otherwise show proper mesasge end do nothing as you do now.

Please rename button to: "view/edit by policy tool" - it is what is really does.

>
>> few more nits:
>>
>> Now the polici file is visibl ein policy tool, in plolicy file  in readonly textfield (I'm
>> wondering why it was not visible before). Thats good, but I would like to see it in itw settings
>> itself:( Please add similar readonly jtextfield to police settings pane.
>> You can keep also current tool tip text;)
>
> I have done this, but it's kind of ugly looking. And I really don't know how to make things look
> pretty. :)

My suggesttion:

****text***t*t*t*t*t*t
**text**t**tt*tt*t*t*t
****text***t*t*t*t*t*t
**text**t**tt*tt*t*t*t
[textfield with path] [button]

What do you mean?

I think that in future we may end up with something like

****text***t*t*t*t*t*t
**text**t**tt*tt*t*t*t
****text***t*t*t*t*t*t
**text**t**tt*tt*t*t*t
[textfield with path to local file  ] [simple edit button][policy tool button]
[textfield with path to global  file] [simple edit button][policy tool button]

Maybe ot soon, nor ever. But stil it will be nicer :) And you will discover a bit of swing O:)

>> +    private static File policy = new File(System.getProperty("user.home") +
>> "/.config/icedtea-web/security/java.policy"),
>> +             policyBackup = new File(System.getProperty("user.home") +
>> "/.config/icedtea-web/security/java.policy.bak");
>>
>> nope - you have to use value from deployment configuration
>
> Yea, this was just carelessness :( wrote it as a quick hack and then forgot to go back and actually
> fix it properly. Classic story, isn't it?
>
>>
>> +        if (policy.isFile()) {
>> +            if (policyBackup.exists()) {
>> +                throw new RuntimeException("Backup policy file already exists");
>> +            }
>>
>>
>> Instead of this (this can simple become blocker if tests are killed) I would recommand to use some
>> random-suffixed.bak  file. Of course feel free to throw out if backuping fail and log out worning
>> if old backups exists.
>>
>>   try {
>> +            out = new FileWriter(policy, false /*no append*/);
>> +        } catch (IOException e) {
>> +            throw new RuntimeException(e);
>> +        }
>> +... and much similar
>>
>> Its not worthy to catch. Please let already thrown exception flow. Of course try to close in
>> finally. (especially you will avoid try{}finally{try{}catch{}} nastyness.
>>
>> Both your testcases should share shared code (namely backupPolicy and restorePolicy ;) ) And also
>> yor assersert* methods (just with negation ;) )
>>
>> Otherwise the tests looks really good!
>>
>
> Well, the reason I had the tests split into two testcases files like that was because one set of

Oh I did not wont you to join files, nor reuse before/after method themselves. Just logic of them.
I'm terribly sorry for not being clear!

Please split the files again. They are compiled together, so they can reuse thirs package 
private/public methods or extends themselves.

The previous before/after backup was nicer then current one.


     if (policy.isFile()) {
+            for (int i = 0; i < MAX_BACKUPS; ++i) {
+                if (!policyBackup.exists()) {
+                    break;
+                } else {
+                    policyBackup = getRandomizedBackup();
+                }
+            }


:DDD (sorry :) )

Please use File.createTmpfile(dir,pattern) instead :)
It guarants you creation of the file, so you do not need to throw.


Tets are forbidden touse  stdouts/errs

+            System.err/out.println

Use ServerAccess logger instead.


> them required a specific policy file, and the other set required there to be no policy file at all.
> So I used @BeforeClass and @AfterClass in each testcase so that I could do the work of setting up
> and restoring the policy files just once per set. Now, with them all combined into one testcase
> file, the backup/restore has to be done wrapped around every single test using @Before and @After,
> with the tests that require the specific policy writing a new one each time, since the order the
> tests run in is not well defined. But on the plus side, they all use the same backup/restore methods
> and many of the same assertions, too.


hm.. jsut finished the review. If you do not wont to split file again, go on with current 
before/after + write solution. Its not so bad as it seemed. Just not standard. But please fix the 
rest. The number of test in single file is.. on edge ,  But we can survive it.


J.