Derby
  1. Derby
  2. DERBY-5631

Extend SecurityManagerSetup to add extra privileges to the set of default privileges (merge two policy files)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.9.1.0
    • Fix Version/s: 10.9.1.0
    • Component/s: Test
    • Labels:
      None

      Description

      When moving ProtocolTest into suites all (see DERBY-2031), I needed to allow the test code to create a socket. There was already a policy file for the old harness for doing this, but when using this with the JUnit framework I ended up adding privilege after privilege to get the framework itself running.
      Instead of creating a large policy file duplicating all the privileges that the framework requires to function properly, I think it would be better to make the SecurityManagerSetup capable of merging the test specific policy file with the default policy file.
      This mode of operation can be used when you need a few extra privileges to execute the test, but there are probably also tests where you want full control of the privileges - in which case you use the existing mode of operation.

      I'm not 100% sure this approach will always work, but basic testing has shown promising results.

      1. derby-5631-2a-introduce_NO_POLICY_constant.diff
        3 kB
        Kristian Waagan
      2. derby-5631-1f-merge_policy_files_fix-priv.diff
        5 kB
        Kristian Waagan
      3. derby-5631-1e-merge_policy_files-fix-url.diff
        2 kB
        Kristian Waagan
      4. derby-5631-1d-merge_policy_files-fix.diff
        2 kB
        Kristian Waagan
      5. derby-5631-1c-merge_policy_files.diff
        9 kB
        Kristian Waagan
      6. derby-5631-1b-merge_policy_files.diff
        9 kB
        Kristian Waagan
      7. derby-5631-1a-merge_policy_files.diff
        10 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        Attaching patch 1a, which adds a new constructor with a third boolean argument to SecurityManagerSetup.
        If true, the specified policy file is appended to the default policy (written to a new file) and the merged policy installed.

        I chose "system/var" as the home for these merged policy files. I haven't added any removal-code for these files.

        I tested the patch manually (modified AssertFailureTest), I plan to use the functionality in the coming patch for DERBY-2031.

        I also did some minor cleanup/refactoring (JavaDoc, added NO_POLICY constant).

        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 1a, which adds a new constructor with a third boolean argument to SecurityManagerSetup. If true, the specified policy file is appended to the default policy (written to a new file) and the merged policy installed. I chose "system/var" as the home for these merged policy files. I haven't added any removal-code for these files. I tested the patch manually (modified AssertFailureTest), I plan to use the functionality in the coming patch for DERBY-2031 . I also did some minor cleanup/refactoring (JavaDoc, added NO_POLICY constant). Patch ready for review.
        Hide
        Dag H. Wanvik added a comment -

        Looks like a useful addition. Now we should go back a remove stuff from the default test policy file
        Nits:

        • introduced new or changed lines with leading tabs
        • Might it be cleaner to have the two policies as arguments to mergeWithDefaultPolicyFile? Perhaps by storing one (added==null) or two policies as members and drop the boolean member mergeWithDefault?
        Show
        Dag H. Wanvik added a comment - Looks like a useful addition. Now we should go back a remove stuff from the default test policy file Nits: introduced new or changed lines with leading tabs Might it be cleaner to have the two policies as arguments to mergeWithDefaultPolicyFile? Perhaps by storing one (added==null) or two policies as members and drop the boolean member mergeWithDefault?
        Hide
        Kristian Waagan added a comment -

        Thanks, Dag.

        I pulled all the NO_POLICY changes into patch 2a and committed it to trunk with revision 1294088.
        I'll have a look at your suggestions, and at the very least post a new patch which addresses the whitespace issues.

        Show
        Kristian Waagan added a comment - Thanks, Dag. I pulled all the NO_POLICY changes into patch 2a and committed it to trunk with revision 1294088. I'll have a look at your suggestions, and at the very least post a new patch which addresses the whitespace issues.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1b, which replaces patch 1a.

        I've fixed the whitespace issues commented on by Dag, and I've also rewritten the method as suggested by Dag. Made the method static, updated some comments, and added a debug println:
        DEBUG:

        {SecurityManagerSetup} installed policy file:/C:/.../derby/test/system/var/AssertFailureTest.policy-MERGED_WITH-derby_tests.policy
        DEBUG: {SecurityManagerSetup}

        installed policy jar:file:/C:/.../derby/jars/sane/derbyTesting.jar!/org/apache/derbyTesting/unitTests/junit/AssertFailureTest1.policy

        Patch ready for review.
        I hope to commit patch 1b soon.

        Show
        Kristian Waagan added a comment - Attaching patch 1b, which replaces patch 1a. I've fixed the whitespace issues commented on by Dag, and I've also rewritten the method as suggested by Dag. Made the method static, updated some comments, and added a debug println: DEBUG: {SecurityManagerSetup} installed policy file:/C:/.../derby/test/system/var/AssertFailureTest.policy-MERGED_WITH-derby_tests.policy DEBUG: {SecurityManagerSetup} installed policy jar: file:/C:/.../derby/jars/sane/derbyTesting.jar!/org/apache/derbyTesting/unitTests/junit/AssertFailureTest1.policy Patch ready for review. I hope to commit patch 1b soon.
        Hide
        Dag H. Wanvik added a comment -

        Patch 1b looks good to me. I have one more small nit: I'd prefer the test on policy2 == null to be outside of mergePolicies, since in that case no merging takes place making the name of the method a bit misleading. Alternately, change the name to cater for both one and two policies, e.g. "getEffectivePolicyResourceString". Up to you, +1 in any case.

        Show
        Dag H. Wanvik added a comment - Patch 1b looks good to me. I have one more small nit: I'd prefer the test on policy2 == null to be outside of mergePolicies, since in that case no merging takes place making the name of the method a bit misleading. Alternately, change the name to cater for both one and two policies, e.g. "getEffectivePolicyResourceString". Up to you, +1 in any case.
        Hide
        Kristian Waagan added a comment -

        Thanks for the additional review, Dag.
        I've made some further changes based on your feedback, see attached patch 1c.

        Committed patch 1c to trunk with revision 1295436.

        Show
        Kristian Waagan added a comment - Thanks for the additional review, Dag. I've made some further changes based on your feedback, see attached patch 1c. Committed patch 1c to trunk with revision 1295436.
        Hide
        Kristian Waagan added a comment -

        Resolving issue.
        I don't plan to backport.

        Show
        Kristian Waagan added a comment - Resolving issue. I don't plan to backport.
        Hide
        Kristian Waagan added a comment -

        Auch, my last commit broke all tests using no security policy (<NONE>). Mental note: run tests properly - run all tests and preferably with the correct patch applied...
        Committed patch 1d to trunk with revision 1295507.

        Sorry for the noise.

        Show
        Kristian Waagan added a comment - Auch, my last commit broke all tests using no security policy (<NONE>). Mental note: run tests properly - run all tests and preferably with the correct patch applied... Committed patch 1d to trunk with revision 1295507. Sorry for the noise.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1e, which committed already on Thursday.
        It deals with more variations of denoting a policy resource, i.e. how to get from String to URL.

        Committed to trunk with revision 1295609.

        Hopefully the last commit on this issue. The new functionality will be run as part of suites.All when DERBY-2031 goes in.

        Show
        Kristian Waagan added a comment - Attaching patch 1e, which committed already on Thursday. It deals with more variations of denoting a policy resource, i.e. how to get from String to URL. Committed to trunk with revision 1295609. Hopefully the last commit on this issue. The new functionality will be run as part of suites.All when DERBY-2031 goes in.
        Hide
        Kristian Waagan added a comment -

        Reopening to add a missing doPrivileged-block.

        Show
        Kristian Waagan added a comment - Reopening to add a missing doPrivileged-block.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1f, which adds a missing privileged block in mergePolicies.

        Changes in order of importance:
        o added doPrivileged-block when converting from File to URI to URL to String
        o replaced code block with utility method in setSecurityPolicy
        o imported some security classes and removed package prefix (i.e. new java.security.PrivilegedAction to new PrivilegedAction)
        o added a missing word to a @throws tag

        suites.All passed on both Solaris 11 and Linux with this patch (and the new ProtocolTest enabled).
        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 1f, which adds a missing privileged block in mergePolicies. Changes in order of importance: o added doPrivileged-block when converting from File to URI to URL to String o replaced code block with utility method in setSecurityPolicy o imported some security classes and removed package prefix (i.e. new java.security.PrivilegedAction to new PrivilegedAction) o added a missing word to a @throws tag suites.All passed on both Solaris 11 and Linux with this patch (and the new ProtocolTest enabled). Patch ready for review.
        Hide
        Rick Hillegas added a comment -

        Thanks for the patch, Kristian. These changes look like good defensive logic to me. +1

        Show
        Rick Hillegas added a comment - Thanks for the patch, Kristian. These changes look like good defensive logic to me. +1
        Hide
        Kristian Waagan added a comment -

        Thank you, Rick.
        Committed patch 1f to trunk with revision 1298787. Will re-enable the test under DERBY-2031.

        Show
        Kristian Waagan added a comment - Thank you, Rick. Committed patch 1f to trunk with revision 1298787. Will re-enable the test under DERBY-2031 .
        Hide
        Kristian Waagan added a comment -

        Resolving, the tinderbox test passed [1].
        Any further problems can be dealt with as separate issues.

        [1] http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/Limited/testSummary-1298791.html

        Show
        Kristian Waagan added a comment - Resolving, the tinderbox test passed [1] . Any further problems can be dealt with as separate issues. [1] http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/Limited/testSummary-1298791.html
        Hide
        Kristian Waagan added a comment -

        Closing issue.

        Show
        Kristian Waagan added a comment - Closing issue.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development