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

Andrew Azores aazores at redhat.com
Thu Jan 16 09:34:00 PST 2014

On 01/16/2014 10:55 AM, Jiri Vanek wrote:
> 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>");

The html tags are there to enable line wrapping in the JLabel. We do the 
same thing in the AboutPanel, otherwise the label just shows one 
incredibly long line of text. We could also make it into a JTextArea and 
enable line wrapping directly without using html tags, but then the text 
area isn't going to actually look the same and it'll take extra work to 
make it fit in.

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

Great idea, done.

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

Also done.

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

I have no idea if what I've done is a good way to do this, but it seems 
to have worked and looks reasonable...

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

I didn't do this in the attached new patches, because I'm not sure about 
it. This is going to create temp files, eg in /tmp - do we really want 
to back up to here? If the power goes out during a test run then you 
might just lose your user policy file entirely. The patch now does the 
backup simply by renaming the policy file without moving it to any other 
directory, so that it should generally be safe and remain recoverable 
even if the machine dies during the test (barring problems out of our 
hands like filesystem corruption). The thrown exception is only there if 
we can't back up the policy at all, probably because there are too many 
existing backups. Using File.createTempFile() would be nice because it 
would simplify the backup/restore process but I don't really like the 
idea of backing up policy files to non-persistent and possibly volatile 
storage... but maybe I'm worrying too much about this.

> Tets are forbidden touse  stdouts/errs
> +            System.err/out.println
> Use ServerAccess logger instead.

Right, thanks.

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


Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: custom_policy5.patch
Type: text/x-patch
Size: 13724 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140116/fd04bbdc/custom_policy5-0001.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reproducers3.patch
Type: text/x-patch
Size: 20236 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140116/fd04bbdc/reproducers3-0001.patch 

More information about the distro-pkg-dev mailing list