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

[Andrew Azores]

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

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

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

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: custom_policy4.patch
Type: text/x-patch
Size: 12727 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140115/e70614c7/custom_policy4-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reproducers2.patch
Type: text/x-patch
Size: 20166 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140115/e70614c7/reproducers2-0001.patch