Derby
  1. Derby
  2. DERBY-2264

Restrict shutdown, upgrade, and encryption powers to the database owner

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: SQL
    • Labels:
      None
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Security

      Description

      This JIRA separates out the database-owner powers from the system privileges in the master security JIRA DERBY-2109. Restrict the following powers to the database owner for the moment: shutdown, upgrade, and encryption.

      1. dbaPowers.html
        10 kB
        Rick Hillegas
      2. dbaPowers.html
        11 kB
        Rick Hillegas
      3. DERBY-2264-1.stat
        2 kB
        Dag H. Wanvik
      4. DERBY-2264-1.diff
        71 kB
        Dag H. Wanvik
      5. encrypt-1b.sql
        1 kB
        Dag H. Wanvik
      6. encrypt-2.sql
        1.0 kB
        Dag H. Wanvik
      7. encrypt-3.sql
        1 kB
        Dag H. Wanvik
      8. DERBY-2264-2.diff
        10 kB
        Dag H. Wanvik
      9. DERBY-2264-2.stat
        0.4 kB
        Dag H. Wanvik
      10. DERBY-2264-3.diff
        4 kB
        Dag H. Wanvik
      11. DERBY-2264-3.stat
        0.4 kB
        Dag H. Wanvik
      12. DERBY-2264-4.diff
        37 kB
        Dag H. Wanvik
      13. DERBY-2264-4.stat
        0.5 kB
        Dag H. Wanvik
      14. DERBY-2264-5.diff
        38 kB
        Dag H. Wanvik
      15. DERBY-2264-5.stat
        0.6 kB
        Dag H. Wanvik
      16. DERBY-2264-6.diff
        1 kB
        Dag H. Wanvik
      17. DERBY-2264-6.stat
        0.2 kB
        Dag H. Wanvik
      18. DERBY-2264-6b.diff
        2 kB
        Dag H. Wanvik
      19. DERBY-2264-6b.stat
        0.2 kB
        Dag H. Wanvik
      20. DERBY-2264-7.diff
        17 kB
        Dag H. Wanvik
      21. DERBY-2264-7.stat
        0.3 kB
        Dag H. Wanvik
      22. releaseNote.html
        4 kB
        Dag H. Wanvik
      23. DERBY-2264-8.diff
        4 kB
        Dag H. Wanvik
      24. DERBY-2264-8.stat
        0.2 kB
        Dag H. Wanvik
      25. DERBY-2264-8.diff
        9 kB
        Dag H. Wanvik
      26. DERBY-2264-8.stat
        0.2 kB
        Dag H. Wanvik
      27. DERBY-2264-9.diff
        13 kB
        Dag H. Wanvik
      28. DERBY-2264-9.stat
        0.4 kB
        Dag H. Wanvik
      29. DERBY-2264-9.diff
        14 kB
        Dag H. Wanvik
      30. releaseNote.html
        5 kB
        Dag H. Wanvik
      31. extra-tests.tar.gz
        3 kB
        Dag H. Wanvik
      32. releaseNote.html
        6 kB
        Rick Hillegas
      33. dbaPowers.html
        12 kB
        Rick Hillegas
      34. releaseNote.html
        6 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          136d 4h 36m 1 Dag H. Wanvik 08/Jun/07 03:30
          Resolved Resolved Closed Closed
          161d 12h 47m 1 Dag H. Wanvik 16/Nov/07 15:17
          Gavin made changes -
          Workflow jira [ 12394812 ] Default workflow, editable Closed status [ 12798585 ]
          Dag H. Wanvik made changes -
          Issue Type New Feature [ 2 ] Improvement [ 4 ]
          Dag H. Wanvik made changes -
          Issue & fix info [Release Note Needed, Existing Application Impact] [Release Note Needed]
          Dag H. Wanvik made changes -
          Component/s Security [ 11411 ]
          Dag H. Wanvik made changes -
          Derby Categories [Security]
          Dag H. Wanvik made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Kathey Marsden made changes -
          Link This issue relates to DERBY-3038 [ DERBY-3038 ]
          Myrna van Lunteren made changes -
          Attachment releaseNote.html [ 12361337 ]
          Hide
          Myrna van Lunteren added a comment -

          attaching updated release note to remove one of doubled 'be be'. Exact same otherwise. See also DERBY-2847 for the report of the 'be be'.

          Show
          Myrna van Lunteren added a comment - attaching updated release note to remove one of doubled 'be be'. Exact same otherwise. See also DERBY-2847 for the report of the 'be be'.
          Bernt M. Johnsen made changes -
          Link This issue is related to DERBY-2451 [ DERBY-2451 ]
          Rick Hillegas made changes -
          Attachment dbaPowers.html [ 12360820 ]
          Hide
          Rick Hillegas added a comment -

          Attach version 3.0 of the functional spec reflecting the fact that these restrictions are only enforced in GRANT/REVOKE-protected databases.

          Show
          Rick Hillegas added a comment - Attach version 3.0 of the functional spec reflecting the fact that these restrictions are only enforced in GRANT/REVOKE-protected databases.
          Rick Hillegas made changes -
          Attachment releaseNote.html [ 12359932 ]
          Hide
          Rick Hillegas added a comment -

          Scrubbing release note so that it survives the release note generator. Corrected some unbalanced <li> tags.

          Show
          Rick Hillegas added a comment - Scrubbing release note so that it survives the release note generator. Corrected some unbalanced <li> tags.
          Dag H. Wanvik made changes -
          Attachment extra-tests.tar.gz [ 12359439 ]
          Hide
          Dag H. Wanvik added a comment -

          Uploading a bundle with extra manual (ij based) tests I ran to verify
          changes made as part of DERBY-2264-9.*

          They should be converted to JUnit.

          Show
          Dag H. Wanvik added a comment - Uploading a bundle with extra manual (ij based) tests I ran to verify changes made as part of DERBY-2264 -9.* They should be converted to JUnit.
          Dag H. Wanvik made changes -
          Attachment releaseNote.html [ 12359305 ]
          Hide
          Dag H. Wanvik added a comment -

          Updated releaseNote.html to reflect changes.

          Show
          Dag H. Wanvik added a comment - Updated releaseNote.html to reflect changes.
          Hide
          Stan Bradbury added a comment -

          Thanks Dag !!

          I applied DERBY-2264-9.diff to my sandbox and preformed some rudimentary upgrage tests to 10.1 and the issues I originally reported in DERBY-2728 are resolved. And it appears the patch is now committed to the codeline so all is good in my book.

          Show
          Stan Bradbury added a comment - Thanks Dag !! I applied DERBY-2264 -9.diff to my sandbox and preformed some rudimentary upgrage tests to 10.1 and the issues I originally reported in DERBY-2728 are resolved. And it appears the patch is now committed to the codeline so all is good in my book.
          Dag H. Wanvik made changes -
          Resolution Fixed [ 1 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Derby Info [Release Note Needed, Existing Application Impact, Patch Available] [Existing Application Impact, Release Note Needed]
          Hide
          Dag H. Wanvik added a comment -

          Setting as resolved, although I plan to run and upload more ad hoc upgrade tests.
          Associated doc (DERBY-2520) has been committed now, I will make a small
          upgrade to the releaseNotes.html also.

          Show
          Dag H. Wanvik added a comment - Setting as resolved, although I plan to run and upload more ad hoc upgrade tests. Associated doc ( DERBY-2520 ) has been committed now, I will make a small upgrade to the releaseNotes.html also.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-9.diff [ 12359236 ]
          Hide
          Dag H. Wanvik added a comment -

          Thanks for reviewing, Rick. I agree this "time travel" is a bit tricky, I would love to have
          more of these test scenarios covered by the upgrade tests as Mike suggested
          on the thread mentioned above. I did look briefly at it earlier and it seems a bit of work
          to get the decorator scaffolding up for these needs.. Meanwhile, I have
          run ij based test scenarios; I plan to systematize these and upload them here ASAP,
          as well as list outcomes. These could be input for new
          JUnit based tests later.

          Committed DERBY-2264-9 at svn 545370. I also upload a slightly modified
          version of DERBY-2264-9.diff because I improved some comments for clarity.
          (I like the uploaded versions of the patches to reflect exactly what goes in.)

          Show
          Dag H. Wanvik added a comment - Thanks for reviewing, Rick. I agree this "time travel" is a bit tricky, I would love to have more of these test scenarios covered by the upgrade tests as Mike suggested on the thread mentioned above. I did look briefly at it earlier and it seems a bit of work to get the decorator scaffolding up for these needs.. Meanwhile, I have run ij based test scenarios; I plan to systematize these and upload them here ASAP, as well as list outcomes. These could be input for new JUnit based tests later. Committed DERBY-2264 -9 at svn 545370. I also upload a slightly modified version of DERBY-2264 -9.diff because I improved some comments for clarity. (I like the uploaded versions of the patches to reflect exactly what goes in.)
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Dag. I have taken a quick look at the version 9 patch. It looks good to me although I must admit it's all a bit tricky. I think that's unavoidable here at the intersection of booting and upgrade, and, fortunately, you have amply commented this solution.

          Show
          Rick Hillegas added a comment - Thanks for the patch, Dag. I have taken a quick look at the version 9 patch. It looks good to me although I must admit it's all a bit tricky. I think that's unavoidable here at the intersection of booting and upgrade, and, fortunately, you have amply commented this solution.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-9.diff [ 12359188 ]
          Attachment DERBY-2264-9.stat [ 12359189 ]
          Hide
          Dag H. Wanvik added a comment -

          Uploading DERBY-2264-9.* which replaces DERBY-2264-8.*

          In addition to the changes in #8 which it replaces, it adds code to
          address the upgrade snag discussed in the email thread:

          http://www.nabble.com/Can%27t-upgrade-from-10.1-to-10.3-tf3855260.html.

          Details:

          When doing temporary soft upgrade boot used to authenticate (first
          phase of two phased hard upgrade boot introduced with this issus),
          make sure not to check for/activate any new feature in
          DataDictionaryImpl#boot / DataDictionaryImpl#checkVersion.

          This ensures that the temporary soft upgrade boot will succeed so
          authentication and hard upgrade boot can proceed.

          I bypass the feature check/activation in DataDictionaryImpl#boot() by
          passing in an internal attribute 'softUpgradeNoFeatureCheck' (from
          EmbedConnection) when required.

          As a consequence, when upgrading from 10.1 or earlier to 10.3 or
          later, there will be no check for database owner (OK, since
          database owner is not yet in created in its post 10.1 form), but
          an ordinary credentials check will happen before the hard upgrade
          takes place (nice) - so only an authenticated user can upgrade and
          become database owner (always assuming authentication is enabled
          of course), allowing us to still avoid
          https://issues.apache.org/jira/browse/DERBY-2766
          "Non-authenticated user gets to upgrade from pre-10.2 version
          databases and become database owner".

          If upgrading from 10.2 or newer we will be activate SQLAutorization
          (if set) during the temporary soft upgrade boot so we can authenticate
          database owner.

          The second phase will then do the hard upgrade and, if it is from
          a pre-10.2 database, create the (new) database owner - as before
          with upgrade to 10.2.

          Question: Is passing an internal attribute down from
          EmbedConnection to DataDictionaryImpl#boot an acceptable way to
          implement this solution? (I do make sure to remove any such
          attribute for other cases, in case somebody tries to be clever and
          pass it in from the URL..)

          I tested this approach and it works as expected for the upgrade
          scenarios I have been able to imagine and throw at it, plus derbyall
          and suites.All regression tests.

          Show
          Dag H. Wanvik added a comment - Uploading DERBY-2264 -9.* which replaces DERBY-2264 -8.* In addition to the changes in #8 which it replaces, it adds code to address the upgrade snag discussed in the email thread: http://www.nabble.com/Can%27t-upgrade-from-10.1-to-10.3-tf3855260.html . Details: When doing temporary soft upgrade boot used to authenticate (first phase of two phased hard upgrade boot introduced with this issus), make sure not to check for/activate any new feature in DataDictionaryImpl#boot / DataDictionaryImpl#checkVersion. This ensures that the temporary soft upgrade boot will succeed so authentication and hard upgrade boot can proceed. I bypass the feature check/activation in DataDictionaryImpl#boot() by passing in an internal attribute 'softUpgradeNoFeatureCheck' (from EmbedConnection) when required. As a consequence, when upgrading from 10.1 or earlier to 10.3 or later, there will be no check for database owner (OK, since database owner is not yet in created in its post 10.1 form), but an ordinary credentials check will happen before the hard upgrade takes place (nice) - so only an authenticated user can upgrade and become database owner (always assuming authentication is enabled of course), allowing us to still avoid https://issues.apache.org/jira/browse/DERBY-2766 "Non-authenticated user gets to upgrade from pre-10.2 version databases and become database owner". If upgrading from 10.2 or newer we will be activate SQLAutorization (if set) during the temporary soft upgrade boot so we can authenticate database owner. The second phase will then do the hard upgrade and, if it is from a pre-10.2 database, create the (new) database owner - as before with upgrade to 10.2. Question: Is passing an internal attribute down from EmbedConnection to DataDictionaryImpl#boot an acceptable way to implement this solution? (I do make sure to remove any such attribute for other cases, in case somebody tries to be clever and pass it in from the URL..) I tested this approach and it works as expected for the upgrade scenarios I have been able to imagine and throw at it, plus derbyall and suites.All regression tests.
          Myrna van Lunteren made changes -
          Derby Info [Release Note Needed, Existing Application Impact, Patch Available] [Patch Available, Existing Application Impact, Release Note Needed]
          Fix Version/s 10.3.0.0 [ 12310800 ]
          Dag H. Wanvik made changes -
          Derby Info [Existing Application Impact, Release Note Needed] [Patch Available, Existing Application Impact, Release Note Needed]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-8.stat [ 12358640 ]
          Attachment DERBY-2264-8.diff [ 12358639 ]
          Hide
          Dag H. Wanvik added a comment -

          Uploading a new version of DERBY-2264-8, which also fixes
          AuthenticationTest.java. That test had incorporated the dbo
          enforcement for authentication, so I had to change it back to allow
          database shutdown for any authenticated user.

          The current version of the patch ran the regression tests
          successfully (Solaris 10/x86, Sun JDK 1.5, sane jars).

          Show
          Dag H. Wanvik added a comment - Uploading a new version of DERBY-2264 -8, which also fixes AuthenticationTest.java. That test had incorporated the dbo enforcement for authentication, so I had to change it back to allow database shutdown for any authenticated user. The current version of the patch ran the regression tests successfully (Solaris 10/x86, Sun JDK 1.5, sane jars).
          Dag H. Wanvik made changes -
          Link This issue relates to DERBY-2728 [ DERBY-2728 ]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-8.stat [ 12358577 ]
          Attachment DERBY-2264-8.diff [ 12358576 ]
          Hide
          Dag H. Wanvik added a comment -

          Uploading a patch which removes the enforcement of dbo powers
          in the case where authentication is enabled, but sqlAuthorization is
          not enabled. This is done as a result of recent discussions on
          derby-dev which reveal concern that such enforcement may break too
          many existing applications.

          This patch is not ready for commit, I am running regression tests on it now,
          but I upload it now in case somebody wants to play with it.

          If we decide this is the way to go, I will commit it when all tests have
          been run. Attachment releaseNotes.html will need modification as well.

          Patch details:

          • modifies EmbedConnection.java to also require sqlAuthorization in
            addition to authentication for dbo powers enforcement
          • modifies DboPowersTest.java correspondingly (before the change to the
            test, it correctly flagged the modified behavior)
          Show
          Dag H. Wanvik added a comment - Uploading a patch which removes the enforcement of dbo powers in the case where authentication is enabled, but sqlAuthorization is not enabled. This is done as a result of recent discussions on derby-dev which reveal concern that such enforcement may break too many existing applications. This patch is not ready for commit, I am running regression tests on it now, but I upload it now in case somebody wants to play with it. If we decide this is the way to go, I will commit it when all tests have been run. Attachment releaseNotes.html will need modification as well. Patch details: modifies EmbedConnection.java to also require sqlAuthorization in addition to authentication for dbo powers enforcement modifies DboPowersTest.java correspondingly (before the change to the test, it correctly flagged the modified behavior)
          Dag H. Wanvik made changes -
          Attachment releaseNote.html [ 12358427 ]
          Hide
          Dag H. Wanvik added a comment -

          Added release note; comments welcome.

          Show
          Dag H. Wanvik added a comment - Added release note; comments welcome.
          Dag H. Wanvik made changes -
          Derby Info [Release Note Needed, Existing Application Impact, Patch Available] [Existing Application Impact, Release Note Needed]
          Hide
          Dag H. Wanvik added a comment -

          Committed DERBY-2264-7.diff at svn 528274.
          In addition to derbyall and suites.All, I also ran upgradeTests._Suite with
          all 10.1 and 10.2 releases.

          Show
          Dag H. Wanvik added a comment - Committed DERBY-2264 -7.diff at svn 528274. In addition to derbyall and suites.All, I also ran upgradeTests._Suite with all 10.1 and 10.2 releases.
          Hide
          Rick Hillegas added a comment -

          The patch looks good to me, Dag. Thanks.

          Show
          Rick Hillegas added a comment - The patch looks good to me, Dag. Thanks.
          Dag H. Wanvik made changes -
          Link This issue is blocked by DERBY-2520 [ DERBY-2520 ]
          Dag H. Wanvik made changes -
          Derby Info [Release Note Needed, Existing Application Impact] [Patch Available, Existing Application Impact, Release Note Needed]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-7.diff [ 12354869 ]
          Attachment DERBY-2264-7.stat [ 12354870 ]
          Hide
          Dag H. Wanvik added a comment -

          This patch, DERBY-2264-7.*, adds checking database owner checking for
          the hard upgrade operation as specified in
          the attached dbaPowers.html.

          With this patch, the hard upgrade operation now also uses the two
          phased boot procedure introduced for the encryption case of this
          issue: First a plain boot is performed to check the credentials, is
          this succeeds, the database is shut down and rebooted with the
          upgrade=true attribute.

          A new fixture has been added to DbaPowersTest.java to test the
          credentials checking. Note that while this exercises the checking
          code, a hard upgrade is never actually performed, since store ignores
          the upgrade flag when the underlying database is already of the same
          version as the codebase (AFAIK there is no straight forward way right
          now to get a database produced by another version of Derby in the
          existing junit framework).

          I did verify the upgrade checking behavior manually using 10.2/10.3
          databases.

          If others think this is required or a good idea, I may try to extend
          the upgradeTests/_Suite.java test with positive and negative test case
          fixtures to really verify hard upgrade when credentials are
          accepted/rejected.

          derbyall and suites.All ran without incident on Solaris 10/x86, Sun
          JDK 1.6 with the patch applied on svn revision 524545 running from
          classes, sane build.

          Show
          Dag H. Wanvik added a comment - This patch, DERBY-2264 -7.*, adds checking database owner checking for the hard upgrade operation as specified in the attached dbaPowers.html. With this patch, the hard upgrade operation now also uses the two phased boot procedure introduced for the encryption case of this issue: First a plain boot is performed to check the credentials, is this succeeds, the database is shut down and rebooted with the upgrade=true attribute. A new fixture has been added to DbaPowersTest.java to test the credentials checking. Note that while this exercises the checking code, a hard upgrade is never actually performed, since store ignores the upgrade flag when the underlying database is already of the same version as the codebase (AFAIK there is no straight forward way right now to get a database produced by another version of Derby in the existing junit framework). I did verify the upgrade checking behavior manually using 10.2/10.3 databases. If others think this is required or a good idea, I may try to extend the upgradeTests/_Suite.java test with positive and negative test case fixtures to really verify hard upgrade when credentials are accepted/rejected. derbyall and suites.All ran without incident on Solaris 10/x86, Sun JDK 1.6 with the patch applied on svn revision 524545 running from classes, sane build.
          Dag H. Wanvik made changes -
          Derby Info [Release Note Needed, Existing Application Impact, Patch Available] [Existing Application Impact, Release Note Needed]
          Hide
          A B added a comment -

          Thank you, Dag. I committed the *-6b.diff patch with svn # 521401.

          Show
          A B added a comment - Thank you, Dag. I committed the *-6b.diff patch with svn # 521401.
          Rick Hillegas made changes -
          Link This issue relates to DERBY-2470 [ DERBY-2470 ]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-6b.diff [ 12353917 ]
          Attachment DERBY-2264-6b.stat [ 12353918 ]
          Hide
          Dag H. Wanvik added a comment -

          DERBY-2462-6b fixes the import issue and adds comments
          as suggested, thanks Army!

          Show
          Dag H. Wanvik added a comment - DERBY-2462 -6b fixes the import issue and adds comments as suggested, thanks Army!
          Hide
          A B added a comment -

          FYI, the *.-6.diff patch does not compile in my environment:

          [javac] symbol : variable JDBC
          [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.DboPowersTest
          [javac] if (!JDBC.vmSupportsJSR169()) {

          Looks like it's just missing the JDBC import. Also, might be nice to add a comment explaining why the encryption tests are not running with JSR169 (so that people who are wondering later on can figure it out without having to search Jira and/or svn commit messages...).

          I added the missing import locally and ran the test with weme6.1:

          Time: 11.867

          OK (5 tests)

          If this looks right then could you post another patch with the "import" and associated comment(s)? Then I can either commit the patch, or you can use your new committer privileges to do it yourself

          Thank you for taking the time to address this follow-up issue.

          Show
          A B added a comment - FYI, the *.-6.diff patch does not compile in my environment: [javac] symbol : variable JDBC [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.DboPowersTest [javac] if (!JDBC.vmSupportsJSR169()) { Looks like it's just missing the JDBC import. Also, might be nice to add a comment explaining why the encryption tests are not running with JSR169 (so that people who are wondering later on can figure it out without having to search Jira and/or svn commit messages...). I added the missing import locally and ran the test with weme6.1: Time: 11.867 OK (5 tests) If this looks right then could you post another patch with the "import" and associated comment(s)? Then I can either commit the patch, or you can use your new committer privileges to do it yourself Thank you for taking the time to address this follow-up issue.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-6.stat [ 12353555 ]
          Attachment DERBY-2264-6.diff [ 12353554 ]
          Hide
          Dag H. Wanvik added a comment -

          This patch (DERBY-2264-6.*) adds a check that the encryption fixtures
          are not run for JSR169. I have run the test again with JDK1.6, but not
          with weme6.1. Thanks, Myrna, for pointing out that the test need not
          exclude clientServerDecorator (which tests for JSR169 itself).

          Show
          Dag H. Wanvik added a comment - This patch ( DERBY-2264 -6.*) adds a check that the encryption fixtures are not run for JSR169. I have run the test again with JDK1.6, but not with weme6.1. Thanks, Myrna, for pointing out that the test need not exclude clientServerDecorator (which tests for JSR169 itself).
          Hide
          Dag H. Wanvik added a comment -

          Hi Army,

          Looking at Dan's specification for Derby support for JSR169 support in DERBY-97, I see:

          > These features will not be modified to work on Foundation:
          >
          > * Derby Network server (could be a separate project)
          > * Encrypted Databases (no support for JCE classes in Foundation)

          So I guess the encryption fixtures should disabled, and perhaps the
          ClientServerDecorator wrapping of the dboShutdownSuite as well.

          Show
          Dag H. Wanvik added a comment - Hi Army, Looking at Dan's specification for Derby support for JSR169 support in DERBY-97 , I see: > These features will not be modified to work on Foundation: > > * Derby Network server (could be a separate project) > * Encrypted Databases (no support for JCE classes in Foundation) So I guess the encryption fixtures should disabled, and perhaps the ClientServerDecorator wrapping of the dboShutdownSuite as well .
          Hide
          A B added a comment -

          Hi Dag,

          I noticed that the DboPowersTest is failing on weme6.1 with error "Failed to start database 'singleUse/oneuse4' in many cases:

          Tests run: 15, Failures: 3, Errors: 5

          Both the "Failures" and the "Errors" are caused by an inability to start the database. The "Failures" all occur in "testEncrypt", the "Errors" all occur in "testReEncrypt".

          I looked at the derby.log file generated from the test but there was nothing helpful. I'm not sure what the best way is to unwrap the exception, but before I look into that I thought I should ask:

          Is there something about the encryption test that makes them not runnable with weme6.1? (please excuse my ignorance here)? If so then can these two fixtures just be disabled with weme6.1? Otherwise...any ideas what might be going on?

          Show
          A B added a comment - Hi Dag, I noticed that the DboPowersTest is failing on weme6.1 with error "Failed to start database 'singleUse/oneuse4' in many cases: Tests run: 15, Failures: 3, Errors: 5 Both the "Failures" and the "Errors" are caused by an inability to start the database. The "Failures" all occur in "testEncrypt", the "Errors" all occur in "testReEncrypt". I looked at the derby.log file generated from the test but there was nothing helpful. I'm not sure what the best way is to unwrap the exception, but before I look into that I thought I should ask: Is there something about the encryption test that makes them not runnable with weme6.1? (please excuse my ignorance here)? If so then can these two fixtures just be disabled with weme6.1? Otherwise...any ideas what might be going on?
          Hide
          Rick Hillegas added a comment -

          Thanks, Dag. I like your solution to the internationalization issue. Committed DERBY-2264.5.diff at subversion revision 518214.

          Show
          Rick Hillegas added a comment - Thanks, Dag. I like your solution to the internationalization issue. Committed DERBY-2264 .5.diff at subversion revision 518214.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-5.diff [ 12353288 ]
          Attachment DERBY-2264-5.stat [ 12353289 ]
          Hide
          Dag H. Wanvik added a comment -

          Thanks for review, Rick! DERBY-2264-5.

          {diff,stat}

          supercedes patch #4
          and addresses your comments, fixed some white space details and
          renamed a variable. I chose separate error messages; I think that is
          cleaner.

          Ran suites.All with no errors. derbyall has a one-line diff in the
          dblook_test.java which I believe is unrelated.

          Show
          Dag H. Wanvik added a comment - Thanks for review, Rick! DERBY-2264 -5. {diff,stat} supercedes patch #4 and addresses your comments, fixed some white space details and renamed a variable. I chose separate error messages; I think that is cleaner. Ran suites.All with no errors. derbyall has a one-line diff in the dblook_test.java which I believe is unrelated.
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Dag. This looks like solid incremental improvement. As your comments indicate, this patch introduces a new race condition: We will kill another user's connection if it sneaks in between the authenticating boot and the encrypting boot. I think this is a small edge case. It can be addressed later on if we decide that it's a problem. I believe that there are other, existing boot-time edge cases having to do with encryption and upgrade. Before patching this isolated, new case, I think we should analyze the other edge cases and see if we can come up with a model that makes sense.

          A couple comments on the patch to EmbedConnection itself:

          1) A variable called "didWait" is initialized but I can't see where it's used later on.

          2) I think that the error messages are not internationalized. It looks as though English strings are being hardcoded and will end up being inserted in text that is localized to other languages--the resulting composite text will be an odd pidgin. I can suggest 2 possible solutions to this problem:

          a) Create separate error messages for the separate error states.

          b) Continue to have one error message but expand its text so that it describes all of the error states and gives the user enough information to figure out which one applies.

          Thanks, again.

          Show
          Rick Hillegas added a comment - Thanks for the patch, Dag. This looks like solid incremental improvement. As your comments indicate, this patch introduces a new race condition: We will kill another user's connection if it sneaks in between the authenticating boot and the encrypting boot. I think this is a small edge case. It can be addressed later on if we decide that it's a problem. I believe that there are other, existing boot-time edge cases having to do with encryption and upgrade. Before patching this isolated, new case, I think we should analyze the other edge cases and see if we can come up with a model that makes sense. A couple comments on the patch to EmbedConnection itself: 1) A variable called "didWait" is initialized but I can't see where it's used later on. 2) I think that the error messages are not internationalized. It looks as though English strings are being hardcoded and will end up being inserted in text that is localized to other languages--the resulting composite text will be an odd pidgin. I can suggest 2 possible solutions to this problem: a) Create separate error messages for the separate error states. b) Continue to have one error message but expand its text so that it describes all of the error states and gives the user enough information to figure out which one applies. Thanks, again.
          Hide
          Dag H. Wanvik added a comment -

          Btw, committers, feel free to defer committing this patch
          until the community has a chance to weigh in in the
          synchronization issue I raised.

          Show
          Dag H. Wanvik added a comment - Btw, committers, feel free to defer committing this patch until the community has a chance to weigh in in the synchronization issue I raised.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-4.stat [ 12352799 ]
          Attachment DERBY-2264-4.diff [ 12352798 ]
          Hide
          Dag H. Wanvik added a comment -

          This patch (DERBY-2264-4.

          {diff,stat}

          ) adds policing of powers for
          (re)encryption and adds tests for this.

          I also extended the JUnit framework with some new methods.

          I ran derbyall on Solaris 10, x86, Sun Java 1.6, with two errors,
          which I think are unrelated:

          derbyall/derbynetmats/derbynetmats.fail:jdbcapi/setTransactionIsolation.java
          derbyall/storeall/storeall.fail:store/readlocks.sql

          The JUnit suites.All ran to completion, no failures.

          • A note on atomicity of encrypting:

          Currently, the boot and the simultaneous (re)encryption is atomic, as
          far as I understand, so another thread (#2) will either a) boot first,
          making the encrypting boot (thread #1) silently fail or b) be
          blocked by the encrypting boot and get a connection after the
          encryption is done (i.e. we have atomicity).

          The new thing introduced by this patch is that thread #11 (the
          encrypting connection thread) does three atomic steps ( [1] boot, [2]
          shutdown, [3] boot/encrypt) in stead of previously one (boot/encrypt).
          We may need/want to make those three into one atomic operation
          somehow, to avoid a race condition.

          Currently, there is no synchronization between connections before one
          hits the booting level as far as I can see . In the constructor of
          EmbedConnection, which is where the new enforcement boot/shutdown/boot
          logic is located, there is no synchronization across connections
          currently. We may have to introduce something to make the three
          operations atomic; I would appreciate others' opinion on this, I will
          make a follow-up patch if synchronization is deemed necessary.

          Race scenarios:

          If thread #2 sneaks in between [1] and [2] it will lose its
          connection, which thread #1 shuts down the database in step [2].

          If thread #2 sneaks in between [2] and [3], the (re)encryption of
          thread #1 in step [3] will silently (see DERBY-2409) fail.

          The first scenario is new and could be seen by applications. I am not
          sure it is really necessary to synchronize this further, though, since
          essentially, to (re)encrypt a DBA needs to shut down the database and
          make sure other connections don't re-boot the database in the
          meantime, in any case. In summary, I think it's an edge case.

          Show
          Dag H. Wanvik added a comment - This patch ( DERBY-2264 -4. {diff,stat} ) adds policing of powers for (re)encryption and adds tests for this. I also extended the JUnit framework with some new methods. I ran derbyall on Solaris 10, x86, Sun Java 1.6, with two errors, which I think are unrelated: derbyall/derbynetmats/derbynetmats.fail:jdbcapi/setTransactionIsolation.java derbyall/storeall/storeall.fail:store/readlocks.sql The JUnit suites.All ran to completion, no failures. A note on atomicity of encrypting: Currently, the boot and the simultaneous (re)encryption is atomic, as far as I understand, so another thread (#2) will either a) boot first, making the encrypting boot (thread #1) silently fail or b) be blocked by the encrypting boot and get a connection after the encryption is done (i.e. we have atomicity). The new thing introduced by this patch is that thread #11 (the encrypting connection thread) does three atomic steps ( [1] boot, [2] shutdown, [3] boot/encrypt) in stead of previously one (boot/encrypt). We may need/want to make those three into one atomic operation somehow, to avoid a race condition. Currently, there is no synchronization between connections before one hits the booting level as far as I can see . In the constructor of EmbedConnection, which is where the new enforcement boot/shutdown/boot logic is located, there is no synchronization across connections currently. We may have to introduce something to make the three operations atomic; I would appreciate others' opinion on this, I will make a follow-up patch if synchronization is deemed necessary. Race scenarios: If thread #2 sneaks in between [1] and [2] it will lose its connection, which thread #1 shuts down the database in step [2] . If thread #2 sneaks in between [2] and [3] , the (re)encryption of thread #1 in step [3] will silently (see DERBY-2409 ) fail. The first scenario is new and could be seen by applications. I am not sure it is really necessary to synchronize this further, though, since essentially, to (re)encrypt a DBA needs to shut down the database and make sure other connections don't re-boot the database in the meantime, in any case. In summary, I think it's an edge case.
          Hide
          Rick Hillegas added a comment -

          Looks good. Tests pass. Committed DERBY-2264-3.diff at subversion revision 514836.

          Show
          Rick Hillegas added a comment - Looks good. Tests pass. Committed DERBY-2264 -3.diff at subversion revision 514836.
          Dag H. Wanvik made changes -
          Derby Info [Release Note Needed, Existing Application Impact] [Patch Available, Existing Application Impact, Release Note Needed]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-3.stat [ 12352642 ]
          Attachment DERBY-2264-3.diff [ 12352641 ]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-3.stat [ 12352640 ]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-3.diff [ 12352639 ]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-3.stat [ 12352640 ]
          Attachment DERBY-2264-3.diff [ 12352639 ]
          Hide
          Dag H. Wanvik added a comment -

          Attached a patch, DERBY-2264-3.

          {diff,stat}

          , for the issue with the
          error message mentioned; the fix gives it session severity (the
          formatting was thus a symptom of wrong severity). With session
          severity, the message will be preformatted on the server, using the
          locale of the server, cf. DRDAConnThread#buildSqlerrmc logic.

          Ran user2.sql again in DerbyNet and DerbyNetClient frameworks to
          verify.

          Show
          Dag H. Wanvik added a comment - Attached a patch, DERBY-2264 -3. {diff,stat} , for the issue with the error message mentioned; the fix gives it session severity (the formatting was thus a symptom of wrong severity). With session severity, the message will be preformatted on the server, using the locale of the server, cf. DRDAConnThread#buildSqlerrmc logic. Ran user2.sql again in DerbyNet and DerbyNetClient frameworks to verify.
          Hide
          Dag H. Wanvik added a comment -

          The new error message for an unauthorized shutdown attempt is garbled for the clients, since SYSIBM.SQLCAMESSAGE is never called (the connection is closed) to format the message, I missed this the first time around.The new JUnit test, DboPowersTest, never discovered this, looking only at the SQL State, but it can be seen in output of users2.sql for the clients.
          Only the message parameters appear; the message text itself is missing. I will address this in a new patch.

          Show
          Dag H. Wanvik added a comment - The new error message for an unauthorized shutdown attempt is garbled for the clients, since SYSIBM.SQLCAMESSAGE is never called (the connection is closed) to format the message, I missed this the first time around.The new JUnit test, DboPowersTest, never discovered this, looking only at the SQL State, but it can be seen in output of users2.sql for the clients. Only the message parameters appear; the message text itself is missing. I will address this in a new patch.
          Hide
          Rick Hillegas added a comment -

          Thanks for the quick re-patch, Dag. I have committed it at subversion revision 510586. The white space cruft was my bad. This was my first attempt to use the patch tool on my new laptop. It has the following interesting behavior: new files added by the patch don't automatically appear in my client and, instead, provoke a quarrel with the patch tool. Next time around, I'll have to handle this more gracefully than I did with your patch: I simply moused the new file out of the patch, slapped it onto disk, then pruned out the spurious "+" signs. That clumsy workaround introduced the white-space cruft.

          Show
          Rick Hillegas added a comment - Thanks for the quick re-patch, Dag. I have committed it at subversion revision 510586. The white space cruft was my bad. This was my first attempt to use the patch tool on my new laptop. It has the following interesting behavior: new files added by the patch don't automatically appear in my client and, instead, provoke a quarrel with the patch tool. Next time around, I'll have to handle this more gracefully than I did with your patch: I simply moused the new file out of the patch, slapped it onto disk, then pruned out the spurious "+" signs. That clumsy workaround introduced the white-space cruft.
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-2.diff [ 12351821 ]
          Attachment DERBY-2264-2.stat [ 12351822 ]
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the quick review and and commit of my patch, Rick!

          This follow-on patch, DERBY-2264-2.

          {diff,stat}

          addresses the two minor
          issues you mentioned, fixes a couple of Javadoc omissions and corrects
          some whitespace inconsistency, which I don't understand how got in
          there: the uploaded patch looks correct, but after commit some
          indentations in in DboPowersTest.java lack one space here and
          there.. Any clue?

          A small note: the grammar issue, where I used the wording "only
          database owner can..." was based on "prior art", cf. state 2850E's:
          "Only database owner could issue this statement". I did not correct
          this wording, though, for compatibility concerns.

          I ran suites.All and the jdbcapi suites in embedded and Derby client
          harnesses to verify (JDK1.6, Solaris 10). No errors.

          Show
          Dag H. Wanvik added a comment - Thanks for the quick review and and commit of my patch, Rick! This follow-on patch, DERBY-2264 -2. {diff,stat} addresses the two minor issues you mentioned, fixes a couple of Javadoc omissions and corrects some whitespace inconsistency, which I don't understand how got in there: the uploaded patch looks correct, but after commit some indentations in in DboPowersTest.java lack one space here and there.. Any clue? A small note: the grammar issue, where I used the wording "only database owner can..." was based on "prior art", cf. state 2850E's: "Only database owner could issue this statement". I did not correct this wording, though, for compatibility concerns. I ran suites.All and the jdbcapi suites in embedded and Derby client harnesses to verify (JDK1.6, Solaris 10). No errors.
          Rick Hillegas made changes -
          Derby Info [Release Note Needed, Existing Application Impact, Patch Available] [Existing Application Impact, Release Note Needed]
          Hide
          Rick Hillegas added a comment -

          Turned off "Patch Available" flag.

          Show
          Rick Hillegas added a comment - Turned off "Patch Available" flag.
          Hide
          Rick Hillegas added a comment -

          Thanks for the patch, Dag. Committed DERBY-2264-1.diff at subversion revision 510173. The patch looks solid to me and derbyall passes cleanly.

          Two small points, which you may or may not want to address in a future patch:

          1) Grammar: I think that the new error message would read better if you put another "the" in the second sentence. So: "Only the database owner can perform this operation."

          2) Style: In DboPowersTest.dboSuite(), the magic array size 3 could be replaced with a constant.

          Show
          Rick Hillegas added a comment - Thanks for the patch, Dag. Committed DERBY-2264 -1.diff at subversion revision 510173. The patch looks solid to me and derbyall passes cleanly. Two small points, which you may or may not want to address in a future patch: 1) Grammar: I think that the new error message would read better if you put another "the" in the second sentence. So: "Only the database owner can perform this operation." 2) Style: In DboPowersTest.dboSuite(), the magic array size 3 could be replaced with a constant.
          Hide
          Rick Hillegas added a comment -

          Hi Dag,

          I agree with your analysis. (1) seems like a separate itch which someone else may or may not want to scratch.

          Your approach to (2) and (3) sounds good to me too. In the case that the (re-)encrypting user fails to authenticate, we will just be left with a special case of (1).

          Show
          Rick Hillegas added a comment - Hi Dag, I agree with your analysis. (1) seems like a separate itch which someone else may or may not want to scratch. Your approach to (2) and (3) sounds good to me too. In the case that the (re-)encrypting user fails to authenticate, we will just be left with a special case of (1).
          Dag H. Wanvik made changes -
          Attachment encrypt-1b.sql [ 12351702 ]
          Attachment encrypt-3.sql [ 12351704 ]
          Attachment encrypt-2.sql [ 12351703 ]
          Hide
          Dag H. Wanvik added a comment -

          Looking at (re)encryption, I see that the functional specification
          says, when describing current behavior: "anyone who can connect can
          (re)encrypt the database". It is actually worse than that, currently
          anyone (sic) can, even when authentication is on:

          1a) boot a database, even if authentication fails

          1b) boot an encrypted database just by knowing the boot password, even
          if authentication fails

          2) encrypt a database, even if authentication fails

          3) re-encrypt a database only by knowing bootPassword/encryption key,
          even if authentication fails

          These side-effects of otherwise botched connection attempts can be a
          little surprising

          (Re)encryption happens at boot time, and no authentication is
          performed until after the database is booted (user credentials may be
          database local). Scenario 2) above is especially bad, since it allows
          a rogue user to effectively make the database unavailable to all
          legitimate users once it has been shut down. I was not aware of this,
          is it pointed out in the current documentation? I uploaded repro
          scripts for the above cases 1b, 2 and 3 for illustration.

          As I see it now, we can attempt enforcement of 2) and 3) by first
          booting the database, then authenticate. If authentication succeeds
          and user is database owner, we will shutdown the database and boot it
          over again with the (re)encryption now taking place.

          Whether or not 1), the booting, should be limited by privileges is
          outside the scope of this JIRA issue, I think.

          Show
          Dag H. Wanvik added a comment - Looking at (re)encryption, I see that the functional specification says, when describing current behavior: "anyone who can connect can (re)encrypt the database". It is actually worse than that, currently anyone (sic) can, even when authentication is on: 1a) boot a database, even if authentication fails 1b) boot an encrypted database just by knowing the boot password, even if authentication fails 2) encrypt a database, even if authentication fails 3) re-encrypt a database only by knowing bootPassword/encryption key, even if authentication fails These side-effects of otherwise botched connection attempts can be a little surprising (Re)encryption happens at boot time, and no authentication is performed until after the database is booted (user credentials may be database local). Scenario 2) above is especially bad, since it allows a rogue user to effectively make the database unavailable to all legitimate users once it has been shut down. I was not aware of this, is it pointed out in the current documentation? I uploaded repro scripts for the above cases 1b, 2 and 3 for illustration. As I see it now, we can attempt enforcement of 2) and 3) by first booting the database, then authenticate. If authentication succeeds and user is database owner, we will shutdown the database and boot it over again with the (re)encryption now taking place. Whether or not 1), the booting, should be limited by privileges is outside the scope of this JIRA issue, I think.
          Dag H. Wanvik made changes -
          Derby Info [Existing Application Impact] [Patch Available, Existing Application Impact, Release Note Needed]
          Dag H. Wanvik made changes -
          Attachment DERBY-2264-1.diff [ 12351625 ]
          Attachment DERBY-2264-1.stat [ 12351624 ]
          Hide
          Dag H. Wanvik added a comment -

          Right, it seems both Dan and Rick are less concerned than me about the
          compatibility here issue, so I rest my case.

          This patch, DERBY-2264-1.

          {stat,diff}

          implements a first part of this
          JIRA, the checking of shutdown power as specified in the functional
          specification. That is, if derby.connection.requireAuthentication
          true, only the database owner ia allowed to shut down a database, see
          EmbedConnection#checkIsDBOwner.

          A new test jdbcapi/DboPowersTest.java is added which checks the
          enforcement. It runs in both embedded and client/Server frameworks,
          and should be runnable in JSR169, too, although I have no tried
          this. The idea is to add more tests to DboPowersTest.java as the work
          on this issue proceeds, but it works now for shutdown, so I added it
          to _Suite.java.

          After some head-scratching (how to stack decorators, mainly, and how
          they interact) I was able to use the current JUnit decorators to run
          the tests, but found two issues, one of which I corrected:

          a) When using DatabasePropertyTestSetup.builtinAuthentication, the
          enforcement implemented in this patch causes problems for the
          decorator's shutdown database action, unless I prepend "APP" to the
          list of users. I had to do this for NistScripts.java as well. The
          problem occurs because the wombat database was created using APP as
          owner. We might want to either document this in the decorator, add an
          "APP" user silently, or do something else.

          b) The second issue I found was that in wrapping
          TestConfiguration.clientServerDecorator around a test which is already
          decorated with sqlAuthorizationDecorator, an initial empty password
          used by sqlAuthorizationDecorator when constructing
          DatabaseChangeSetup does not work with the client (DRDA limitation I
          believe), so I changed this to a dummy password with no ill effects.

          In addition to the aforementioned NistScripts, I had to also modify
          jdbcapi/users.sql, jdbcapi/users2.sql and jdbcapi/secureUsers.sql.
          These three had to be modifed to make sure the database owner
          performed the shutdowns.

          I suspect that secureUsers.sql had probably never really worked as
          intended in that the database derbySchemeDB was never rebooted as the
          comment says it should be, due to an authentication issue. When I
          fixed this, the test failed, because authentication was turned off,
          since this property was no longer valid as inherited from the system
          properties, when derby.database.propertiesOnly took effect after the
          reboot. Not sure what the author intended here; I made it work with
          the reboot.

          I have run derbyall on Sun's JDK6 on Solaris 10 x86, with only one
          error in a compress test, which seems to be an intermittent failure.

          I ran the JUnit suites.all on Sun's JDK6 Solaris 11 x86 with no
          errors (6814 tests).

          Show
          Dag H. Wanvik added a comment - Right, it seems both Dan and Rick are less concerned than me about the compatibility here issue, so I rest my case. This patch, DERBY-2264 -1. {stat,diff} implements a first part of this JIRA, the checking of shutdown power as specified in the functional specification. That is, if derby.connection.requireAuthentication true, only the database owner ia allowed to shut down a database, see EmbedConnection#checkIsDBOwner. A new test jdbcapi/DboPowersTest.java is added which checks the enforcement. It runs in both embedded and client/Server frameworks, and should be runnable in JSR169, too, although I have no tried this. The idea is to add more tests to DboPowersTest.java as the work on this issue proceeds, but it works now for shutdown, so I added it to _Suite.java. After some head-scratching (how to stack decorators, mainly, and how they interact) I was able to use the current JUnit decorators to run the tests, but found two issues, one of which I corrected: a) When using DatabasePropertyTestSetup.builtinAuthentication, the enforcement implemented in this patch causes problems for the decorator's shutdown database action, unless I prepend "APP" to the list of users. I had to do this for NistScripts.java as well. The problem occurs because the wombat database was created using APP as owner. We might want to either document this in the decorator, add an "APP" user silently, or do something else. b) The second issue I found was that in wrapping TestConfiguration.clientServerDecorator around a test which is already decorated with sqlAuthorizationDecorator, an initial empty password used by sqlAuthorizationDecorator when constructing DatabaseChangeSetup does not work with the client (DRDA limitation I believe), so I changed this to a dummy password with no ill effects. In addition to the aforementioned NistScripts, I had to also modify jdbcapi/users.sql, jdbcapi/users2.sql and jdbcapi/secureUsers.sql. These three had to be modifed to make sure the database owner performed the shutdowns. I suspect that secureUsers.sql had probably never really worked as intended in that the database derbySchemeDB was never rebooted as the comment says it should be, due to an authentication issue. When I fixed this, the test failed, because authentication was turned off, since this property was no longer valid as inherited from the system properties, when derby.database.propertiesOnly took effect after the reboot. Not sure what the author intended here; I made it work with the reboot. I have run derbyall on Sun's JDK6 on Solaris 10 x86, with only one error in a compress test, which seems to be an intermittent failure. I ran the JUnit suites.all on Sun's JDK6 Solaris 11 x86 with no errors (6814 tests).
          Hide
          Rick Hillegas added a comment -

          Hi Dag,

          I just want to punch up the upgrade implications of what we're proposing:

          By now there must be scores of legacy applications which use Derby authentication. For many of these applications, the DBA owner is APP. When these applications upgrade to 10.3, no-one will be able to shut them down. That is because APP is probably not a real user with a real password. The affected applications have this shape:

          1) Created with authentication turned off. This is what makes APP the DBA.

          2) Authentication turned on later after users and passwords were defined.

          The workaround for these applications will be this:

          A) Create an account for APP.

          B) Change the application so that, when it shuts down, it re-connects to the database as APP, not as the current user.

          What are the imp;lications of not shutting down the database gracefully when the application exits? A long time ago this used to mean that the log file would just keep growing indefinitely. Has this behavior changed? If it's ok to shutdown gracelessly, then another workaround may be this:

          C) Re-code the application to swallow the concluding exception which says that shutdown failed.

          We seem to have some misgivings that many legacy applications will fit this profile. There seem to be two proposals for how to limit this exposure:

          i) Limit the exposure to a subset of applications created since 10.2, viz., applications which have enabled SQL authorization.

          ii) Limit the exposure to read-only applications.

          At this point, I'm not too keen on either of these techniques. To me they muddy the model laid out in the attached functional spec. I'm not happy about the affect on legacy applications. However, I think that a good Release Note might be our best approach.

          Show
          Rick Hillegas added a comment - Hi Dag, I just want to punch up the upgrade implications of what we're proposing: By now there must be scores of legacy applications which use Derby authentication. For many of these applications, the DBA owner is APP. When these applications upgrade to 10.3, no-one will be able to shut them down. That is because APP is probably not a real user with a real password. The affected applications have this shape: 1) Created with authentication turned off. This is what makes APP the DBA. 2) Authentication turned on later after users and passwords were defined. The workaround for these applications will be this: A) Create an account for APP. B) Change the application so that, when it shuts down, it re-connects to the database as APP, not as the current user. What are the imp;lications of not shutting down the database gracefully when the application exits? A long time ago this used to mean that the log file would just keep growing indefinitely. Has this behavior changed? If it's ok to shutdown gracelessly, then another workaround may be this: C) Re-code the application to swallow the concluding exception which says that shutdown failed. We seem to have some misgivings that many legacy applications will fit this profile. There seem to be two proposals for how to limit this exposure: i) Limit the exposure to a subset of applications created since 10.2, viz., applications which have enabled SQL authorization. ii) Limit the exposure to read-only applications. At this point, I'm not too keen on either of these techniques. To me they muddy the model laid out in the attached functional spec. I'm not happy about the affect on legacy applications. However, I think that a good Release Note might be our best approach.
          Dag H. Wanvik made changes -
          Assignee Dag H. Wanvik [ dagw ]
          Hide
          Dag H. Wanvik added a comment -

          Thanks for good points, Dan! I am still a little unsure of what would
          be the best approach here, though. As far as I understand it, we need
          to strike the right balance between a) plugging security holes, b)
          avoiding undue complexity and c) reducing upgrade pains as much as
          possible.

          3) and 7): Good point about readOnly users; I agree restricting DBA
          powers here plugs a sizable hole. For fullAccess users, your argument
          that the permission to e.g. shut down the database is a different
          permission from full data access is obviously true. My concern,
          however, is how to weigh the benefit of restricting this permission
          against the risk of breaking existing applications. I think this risk
          is real; I did some experiments and found that under the current
          regime for 3) and 7), the developer does not really need to worry
          about who is the database owner. It is easy to create a database whose
          owner is APP, which can not be authenticated either at the system or
          database level, and the developer would not presently notice, since no
          enforcement is in place. This is compounded by the docs not being very
          explicit about how the database owner is created. The workaround for
          an application broken by the new enforcement in such a case, would be
          to a) create a database user called APP to match the system schema
          user name effectively creating the "owner", and b) always close down
          the database connecting as APP.

          One possible way to balance the concerns is to enforce only for
          readOnly access users, thus effectively allowing fullAccess users
          database owner powers for 3) and 7). My theory is that this would
          reduce upgrade pains at slight cost. If this security is not good
          enough for new application, the user can move to sqlAuthorization.
          Downside is more slightly more complexity. What do you think?

          If we decide to go with enforcement also for fullAccess users in 3)
          and 7), we need make a good release note, at least, and clarify docs.

          Dan> I think you also cannot assume that a user authenticated by a
          Dan> database is a valid user in the system authentication. Thus I
          Dan> don't think you can drop the checking for 4) since the user may
          Dan> not be able to shut the system down.

          I agree. I assume there would be fewer upgrade issues to consider
          here, since sqlAuthorization is new in 10.2. I also don't think we
          want to base enforcement of a database level action on whether
          authentication is db local or system wide?

          Seems there are more than Rick's three toggle dimensions in this space
          that matter: system vs db level authentication, and readOnly vs
          fullAccess connection authorization

          Show
          Dag H. Wanvik added a comment - Thanks for good points, Dan! I am still a little unsure of what would be the best approach here, though. As far as I understand it, we need to strike the right balance between a) plugging security holes, b) avoiding undue complexity and c) reducing upgrade pains as much as possible. 3) and 7): Good point about readOnly users; I agree restricting DBA powers here plugs a sizable hole. For fullAccess users, your argument that the permission to e.g. shut down the database is a different permission from full data access is obviously true. My concern, however, is how to weigh the benefit of restricting this permission against the risk of breaking existing applications. I think this risk is real; I did some experiments and found that under the current regime for 3) and 7), the developer does not really need to worry about who is the database owner. It is easy to create a database whose owner is APP, which can not be authenticated either at the system or database level, and the developer would not presently notice, since no enforcement is in place. This is compounded by the docs not being very explicit about how the database owner is created. The workaround for an application broken by the new enforcement in such a case, would be to a) create a database user called APP to match the system schema user name effectively creating the "owner", and b) always close down the database connecting as APP. One possible way to balance the concerns is to enforce only for readOnly access users, thus effectively allowing fullAccess users database owner powers for 3) and 7). My theory is that this would reduce upgrade pains at slight cost. If this security is not good enough for new application, the user can move to sqlAuthorization. Downside is more slightly more complexity. What do you think? If we decide to go with enforcement also for fullAccess users in 3) and 7), we need make a good release note, at least, and clarify docs. Dan> I think you also cannot assume that a user authenticated by a Dan> database is a valid user in the system authentication. Thus I Dan> don't think you can drop the checking for 4) since the user may Dan> not be able to shut the system down. I agree. I assume there would be fewer upgrade issues to consider here, since sqlAuthorization is new in 10.2. I also don't think we want to base enforcement of a database level action on whether authentication is db local or system wide? Seems there are more than Rick's three toggle dimensions in this space that matter: system vs db level authentication, and readOnly vs fullAccess connection authorization
          Hide
          Daniel John Debrunner added a comment -

          Dag> "What I argue in my previous post is that it does not make sense to forbid shutting down the database if you already can delete all its data, which is the case for 7 for fullAccess users."

          Without any checks in 3),7) then read-only users would also be able to shut the system down (as they can today) even though they do not have the ability to delete all the data.

          I think it's somewhat dangerous in security analysis to equate permissions which are really independent of each other. For example one can have the permission to delete rows from a table but not to drop it, they are treated as separate even though they could be seen to have a similar effect.

          I think you also cannot assume that a user authenticated by a database is a valid user in the system authentication. Thus I don't think you can drop the checking for 4) since the user may not be able to shut the system down.

          Show
          Daniel John Debrunner added a comment - Dag> "What I argue in my previous post is that it does not make sense to forbid shutting down the database if you already can delete all its data, which is the case for 7 for fullAccess users." Without any checks in 3),7) then read-only users would also be able to shut the system down (as they can today) even though they do not have the ability to delete all the data. I think it's somewhat dangerous in security analysis to equate permissions which are really independent of each other. For example one can have the permission to delete rows from a table but not to drop it, they are treated as separate even though they could be seen to have a similar effect. I think you also cannot assume that a user authenticated by a database is a valid user in the system authentication. Thus I don't think you can drop the checking for 4) since the user may not be able to shut the system down.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for your analysis! I basically agree.

          Stretching it, I can see a use for 2) in a development situation, while experimenting with privileges as different users etc. In deployment, no, I agree, it does not make much sense. A ported app which uses GRANT/REVOKE for which you don't care less about authentication, perhaps. Number 6) seems even less useful.

          3) and 7) might make sense for a set of cooperating users; using GRANT/REVOKE is more rigid, after all. But as you point out, we must expect (*,on,off) is in use by legacy applications.

          What I argue in my previous post is that it does not make sense to forbid shutting down the database if you already can delete all its data, which is the case for 7 for fullAccess users. IIt can also break existing apps, which makes it even less palatable. So even if we make 7 the default for our secure-by-default server, I propose we only enforce the checks called for by this JIRA in case 4) and 8), not for 3) or 7). We could even drop the checking for 4), since the user can shut down the system, anyway, but I am not proposing that now, to keep things simple.

          Show
          Dag H. Wanvik added a comment - Thanks for your analysis! I basically agree. Stretching it, I can see a use for 2) in a development situation, while experimenting with privileges as different users etc. In deployment, no, I agree, it does not make much sense. A ported app which uses GRANT/REVOKE for which you don't care less about authentication, perhaps. Number 6) seems even less useful. 3) and 7) might make sense for a set of cooperating users; using GRANT/REVOKE is more rigid, after all. But as you point out, we must expect (*,on,off) is in use by legacy applications. What I argue in my previous post is that it does not make sense to forbid shutting down the database if you already can delete all its data, which is the case for 7 for fullAccess users. IIt can also break existing apps, which makes it even less palatable. So even if we make 7 the default for our secure-by-default server, I propose we only enforce the checks called for by this JIRA in case 4) and 8), not for 3) or 7). We could even drop the checking for 4), since the user can shut down the system, anyway, but I am not proposing that now, to keep things simple.
          Hide
          Rick Hillegas added a comment -

          Hi Dag,

          Here's a long preamble about our security model and then a brief discussion of your specific question. We seem to have 3 independent toggles for controlling Derby security:

          A) Whether Java Security is on.

          B) Whether user authentication is on.

          C) Whether SQL authorization is on.

          That gives rise to 8 security states, not all of which makes sense to me. Here they are, expressed in terms of (A, B, C):

          1) (off, off, off). This is the maximally unsecure configuration. It seems like a sensible out-of-the-box default for embedded apps.

          2) (off, off, on). Turning authorization on but turning authentication off doesn't make sense to me. We raise a warning when the customer does this, but we let them do it, nonetheless.

          3) (off, on, off). I think this is supported because we added GRANT/REVOKE long after we added user authentication. I see limited use for this configuration.

          4) (off, on, on). This case may be useful for customers who want GRANT/REVOKE protections but don't want the performance drag imposed by Java security. I don't know how significant that drag is on a Derby engine which has reached steady-state. That's an interesting experiment to run.

          5) (on, off, off). This might be useful for Derby-powered apps which are downloaded into a browser.

          6) (on, off, on). Like (2), this doesn't make much sense to me.

          7) (on, on, off). Like (3), this doesn't make much sense to me either. However, I think we are proposing this as our secure-by-default server configuration. I think we've ended up with this as a default for the same reason that we support configuration (3): this eases the upgrade problem for legacy applications.

          8) (on, on, on) This is the maximally secure configuration. It makes more sense to me as the out-of-the-box default for the network server.

          I can imagine this is a bit perplexing to customers. For my money, (1) and (8) look like two distinguished states whose semantics are very clear. Everything in-between is a little muddy

          Now on to your specific question. In terms of enforcing the database-shutdown power, the only settings which make sense to me are (7) and (8). That is because it seems odd to me to let an authenticated user shutdown the whole engine but not a single database.

          Show
          Rick Hillegas added a comment - Hi Dag, Here's a long preamble about our security model and then a brief discussion of your specific question. We seem to have 3 independent toggles for controlling Derby security: A) Whether Java Security is on. B) Whether user authentication is on. C) Whether SQL authorization is on. That gives rise to 8 security states, not all of which makes sense to me. Here they are, expressed in terms of (A, B, C): 1) (off, off, off). This is the maximally unsecure configuration. It seems like a sensible out-of-the-box default for embedded apps. 2) (off, off, on). Turning authorization on but turning authentication off doesn't make sense to me. We raise a warning when the customer does this, but we let them do it, nonetheless. 3) (off, on, off). I think this is supported because we added GRANT/REVOKE long after we added user authentication. I see limited use for this configuration. 4) (off, on, on). This case may be useful for customers who want GRANT/REVOKE protections but don't want the performance drag imposed by Java security. I don't know how significant that drag is on a Derby engine which has reached steady-state. That's an interesting experiment to run. 5) (on, off, off). This might be useful for Derby-powered apps which are downloaded into a browser. 6) (on, off, on). Like (2), this doesn't make much sense to me. 7) (on, on, off). Like (3), this doesn't make much sense to me either. However, I think we are proposing this as our secure-by-default server configuration. I think we've ended up with this as a default for the same reason that we support configuration (3): this eases the upgrade problem for legacy applications. 8) (on, on, on) This is the maximally secure configuration. It makes more sense to me as the out-of-the-box default for the network server. I can imagine this is a bit perplexing to customers. For my money, (1) and (8) look like two distinguished states whose semantics are very clear. Everything in-between is a little muddy Now on to your specific question. In terms of enforcing the database-shutdown power, the only settings which make sense to me are (7) and (8). That is because it seems odd to me to let an authenticated user shutdown the whole engine but not a single database.
          Hide
          Dag H. Wanvik added a comment -

          Does it really make sense to enforce this restriction unless
          sqlAuthorization is turned on as well (as authentication)?

          I made a trial patch for enforcing this check for shutdown and it
          broke existing tests in derbyall. So it can break existing
          applications without much benefit; without authorization on, full
          access users are fully trusted to change any data in any case.

          Show
          Dag H. Wanvik added a comment - Does it really make sense to enforce this restriction unless sqlAuthorization is turned on as well (as authentication)? I made a trial patch for enforcing this check for shutdown and it broke existing tests in derbyall. So it can break existing applications without much benefit; without authorization on, full access users are fully trusted to change any data in any case.
          Rick Hillegas made changes -
          Attachment dbaPowers.html [ 12350161 ]
          Hide
          Rick Hillegas added a comment -

          Attaching rev 2 of the functional spec. This incorporates Dan's suggestion (in a January 26, 2007 comment on DERBY-2206): only enforce these changes if authentication is turned on.

          Show
          Rick Hillegas added a comment - Attaching rev 2 of the functional spec. This incorporates Dan's suggestion (in a January 26, 2007 comment on DERBY-2206 ): only enforce these changes if authentication is turned on.
          Andrew McIntyre made changes -
          Fix Version/s 10.3.0.0 [ 12310800 ]
          Hide
          Andrew McIntyre added a comment -

          Unsetting Fix Version on unassigned issues.

          Show
          Andrew McIntyre added a comment - Unsetting Fix Version on unassigned issues.
          Daniel John Debrunner made changes -
          Derby Info [Existing Application Impact]
          Rick Hillegas made changes -
          Attachment dbaPowers.html [ 12349451 ]
          Hide
          Rick Hillegas added a comment -

          Attaching functional spec for this feature. There is nothing new in this spec. I simply carved this independent chunk of work out of the master spec for DERBY-2109.

          Show
          Rick Hillegas added a comment - Attaching functional spec for this feature. There is nothing new in this spec. I simply carved this independent chunk of work out of the master spec for DERBY-2109 .
          Hide
          Rick Hillegas added a comment -

          Mark this feature as being related to the system privileges jira DERBY-2109.

          Show
          Rick Hillegas added a comment - Mark this feature as being related to the system privileges jira DERBY-2109 .
          Rick Hillegas made changes -
          Field Original Value New Value
          Link This issue is related to DERBY-2109 [ DERBY-2109 ]
          Rick Hillegas created issue -

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Rick Hillegas
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development