Derby
  1. Derby
  2. DERBY-5792

Make it possible to turn off encryption on an already encrypted database.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.10.1.1
    • Fix Version/s: 10.10.1.1
    • Component/s: JDBC, Store
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      Patch Available
    • Bug behavior facts:
      Security

      Description

      Currently, you can encrypt an unencrypted database and you can change the encryption key on an already encrypted database. However, Derby does not expose a way to turn off (unencrypt) an already encrypted database.

      1. derby-5792-1a-boilerplate_and_preparation.diff
        65 kB
        Kristian Waagan
      2. derby-5792-1b-boilerplate_and_preparation.diff
        65 kB
        Kristian Waagan
      3. derby-5792-2a-decryptdatabasetest.diff
        15 kB
        Kristian Waagan
      4. derby-5792-3a-decryption_feature.diff
        34 kB
        Kristian Waagan
      5. derby-5792-4a-crash_and_dbo.diff
        30 kB
        Kristian Waagan
      6. derby-5792-4b-crash_and_dbo.diff
        32 kB
        Kristian Waagan
      7. derby-5792-5a-old_container_removal_cleanup.diff
        12 kB
        Kristian Waagan
      8. derby-5792-5b-old_container_removal_cleanup.diff
        11 kB
        Kristian Waagan

        Issue Links

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          Rick Hillegas added a comment -

          I have buddy-tested this feature against the user documentation. As far as I can tell, the feature behaves as described by the user documentation (see the script below). In addition, I was not able to discover any new defects which are not also shared with re-encryption. However, I believe that un-encryption and re-encryption share some defects which we should address. I have logged the following bugs:

          DERBY-5968 - A failed connection attempt may nevertheless manage to boot the database.

          DERBY-5969 - Re-encryption and un-encryption silently fail if the database is already booted.

          DERBY-5970 - Check that connection attributes have legal values.

          connect 'jdbc:derby:db;create=true;user=test_dbo;dataEncryption=true;bootPassword=foobarwibblewombat';

          call syscs_util.syscs_create_user( 'test_dbo', 'test_dbopassword' );
          call syscs_util.syscs_create_user( 'fred', 'fredpassword' );

          call syscs_util.syscs_backup_database_and_enable_log_archive_mode( 'backups', 0 );

          – shutdown the database
          connect 'jdbc:derby:db;shutdown=true';

          – need the bootpassword to boot the database
          connect 'jdbc:derby:db;user=fred;password=fredpassword';
          select count from sys.systables;
          connect 'jdbc:derby:db;user=fred;password=fredpassword;bootPassword=foobarwibblewombat';
          select count from sys.systables;

          – only the dbo can shutdown the database
          connect 'jdbc:derby:db;shutdown=true;user=fred;password=fredpassword';
          connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword';

          – only the dbo can unencrypt the database
          connect 'jdbc:derby:db;user=fred;password=fredpassword;bootPassword=foobarwibblewombat;decryptDatabase=true';
          select count from sys.systables;

          – although the connection failed, the database is now booted so we need to shut it down. see DERBY-5968 and DERBY-5969.
          connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword';

          – should fail because log archive mode is turned on
          connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=true';
          select count from sys.systables;

          – turn off log archival mode
          connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat';
          call syscs_util.syscs_disable_log_archive_mode( 0 );

          – shutdown the database
          connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword';

          – try a bad setting for decryptDatabase. silently ignored. see DERBY-5970.
          connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=fred';

          – shutdown the database
          connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword';

          – fails because the database was not decrypted
          connect 'jdbc:derby:db;user=fred;password=fredpassword';
          select count from sys.systables;

          – now unencryption should work
          connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=true';
          select count from sys.systables;

          – shutdown the database
          connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword';

          – now anyone can boot the database without a bootpassword
          connect 'jdbc:derby:db;user=fred;password=fredpassword';
          select count from sys.systables;

          Show
          Rick Hillegas added a comment - I have buddy-tested this feature against the user documentation. As far as I can tell, the feature behaves as described by the user documentation (see the script below). In addition, I was not able to discover any new defects which are not also shared with re-encryption. However, I believe that un-encryption and re-encryption share some defects which we should address. I have logged the following bugs: DERBY-5968 - A failed connection attempt may nevertheless manage to boot the database. DERBY-5969 - Re-encryption and un-encryption silently fail if the database is already booted. DERBY-5970 - Check that connection attributes have legal values. connect 'jdbc:derby:db;create=true;user=test_dbo;dataEncryption=true;bootPassword=foobarwibblewombat'; call syscs_util.syscs_create_user( 'test_dbo', 'test_dbopassword' ); call syscs_util.syscs_create_user( 'fred', 'fredpassword' ); call syscs_util.syscs_backup_database_and_enable_log_archive_mode( 'backups', 0 ); – shutdown the database connect 'jdbc:derby:db;shutdown=true'; – need the bootpassword to boot the database connect 'jdbc:derby:db;user=fred;password=fredpassword'; select count from sys.systables; connect 'jdbc:derby:db;user=fred;password=fredpassword;bootPassword=foobarwibblewombat'; select count from sys.systables; – only the dbo can shutdown the database connect 'jdbc:derby:db;shutdown=true;user=fred;password=fredpassword'; connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword'; – only the dbo can unencrypt the database connect 'jdbc:derby:db;user=fred;password=fredpassword;bootPassword=foobarwibblewombat;decryptDatabase=true'; select count from sys.systables; – although the connection failed, the database is now booted so we need to shut it down. see DERBY-5968 and DERBY-5969 . connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword'; – should fail because log archive mode is turned on connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=true'; select count from sys.systables; – turn off log archival mode connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat'; call syscs_util.syscs_disable_log_archive_mode( 0 ); – shutdown the database connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword'; – try a bad setting for decryptDatabase. silently ignored. see DERBY-5970 . connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=fred'; – shutdown the database connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword'; – fails because the database was not decrypted connect 'jdbc:derby:db;user=fred;password=fredpassword'; select count from sys.systables; – now unencryption should work connect 'jdbc:derby:db;user=test_dbo;password=test_dbopassword;bootPassword=foobarwibblewombat;decryptDatabase=true'; select count from sys.systables; – shutdown the database connect 'jdbc:derby:db;shutdown=true;user=test_dbo;password=test_dbopassword'; – now anyone can boot the database without a bootpassword connect 'jdbc:derby:db;user=fred;password=fredpassword'; select count from sys.systables;
          Hide
          Kristian Waagan added a comment -

          Thanks, Knut.

          Attaching patch 5b, which resolves a merge conflict.
          Committed patch 5b to trunk with revision 1394522.

          Show
          Kristian Waagan added a comment - Thanks, Knut. Attaching patch 5b, which resolves a merge conflict. Committed patch 5b to trunk with revision 1394522.
          Hide
          Knut Anders Hatlen added a comment -

          5a looks like a good cleanup. I don't see any need for two different implementations either. +1

          Show
          Knut Anders Hatlen added a comment - 5a looks like a good cleanup. I don't see any need for two different implementations either. +1
          Hide
          Kristian Waagan added a comment -

          A rerun of suites.All passed without failures, so did my run of derbyall.

          Show
          Kristian Waagan added a comment - A rerun of suites.All passed without failures, so did my run of derbyall.
          Hide
          Kristian Waagan added a comment -

          Attaching patch 5a, which simplifies the method cleaning up the old container files in seg0. I haven't found a reason to keep the two implementations yet (filtering on disk and keeping list in-memory), and this isn't performance critical code.

          suites.All passed except for one failure in the compatibility test (10.3), so I'm rerunning tests.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Attaching patch 5a, which simplifies the method cleaning up the old container files in seg0. I haven't found a reason to keep the two implementations yet (filtering on disk and keeping list in-memory), and this isn't performance critical code. suites.All passed except for one failure in the compatibility test (10.3), so I'm rerunning tests. Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Committed patches 3a and 4b to trunk with revision 1393993.

          I'll continue working on the remaining tasks, but the feature itself should now be complete (except for any bugs and problems we find of course).

          Show
          Kristian Waagan added a comment - Committed patches 3a and 4b to trunk with revision 1393993. I'll continue working on the remaining tasks, but the feature itself should now be complete (except for any bugs and problems we find of course).
          Hide
          Knut Anders Hatlen added a comment -

          Patch 4b looks fine to me, and it clears up my question about DBO rights. Since you're planning to fix the messages in a follow-up patch, I'm +1 to both 3a and 4b.

          As to the messages, perhaps we could reduce the number of messages needed by using the same message for encryption, re-encryption and decryption. For example:

          Cannot encrypt, re-encrypt or decrypt a database when there is a global transaction in the prepared state.
          Cannot reconfigure encryption for a read-only database.
          ...

          Show
          Knut Anders Hatlen added a comment - Patch 4b looks fine to me, and it clears up my question about DBO rights. Since you're planning to fix the messages in a follow-up patch, I'm +1 to both 3a and 4b. As to the messages, perhaps we could reduce the number of messages needed by using the same message for encryption, re-encryption and decryption. For example: Cannot encrypt, re-encrypt or decrypt a database when there is a global transaction in the prepared state. Cannot reconfigure encryption for a read-only database. ...
          Hide
          Kristian Waagan added a comment -

          Attaching patch 4b - I forgot to update lang.ErrorCodeTest in patch 4a.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Attaching patch 4b - I forgot to update lang.ErrorCodeTest in patch 4a. Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Patch 4a adds test cases for DBO powers for decryption, it modifies the logic for crash recovery for cryptographic operations on a database, and it adds crash recovery tests for database decryption.
          Patch 4a is generated on top of 3a.

          The crash recovery logic is getting pretty involved and delicate - I'm not too happy about it...

          A correction on my own comment regarding DBO powers: patch 3a will also reject decryption attempts from non-DBO users if authentication and authorization are enabled, but it will say that the operation denied is (re-)encryption.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Patch 4a adds test cases for DBO powers for decryption, it modifies the logic for crash recovery for cryptographic operations on a database, and it adds crash recovery tests for database decryption. Patch 4a is generated on top of 3a. The crash recovery logic is getting pretty involved and delicate - I'm not too happy about it... A correction on my own comment regarding DBO powers: patch 3a will also reject decryption attempts from non-DBO users if authentication and authorization are enabled, but it will say that the operation denied is (re-)encryption. Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Thanks for the review, Knut Anders.
          Answers/comments below.

          > What does this mean exactly? Can any user decrypt the database with the current state of the patch?

          Any user that knows the boot password / encryption key can decrypt the database with patch 3a.
          Nothing will happen if the database has already been booted. Assuming the DBO boots the database by supplying the boot credentials, JOE_USER can't decrypt the database. "decryptDatabase=true" will be ignored when JOE_USER connects to the booted database. This hasn't changed with this patch, encryption attributes are handled the same way.

          I have a patch adding the restriction that only the DBO can decrypt a database, given that authentication and authorization is enabled. The code will go in toghether with changes to the DboPowersTest.

          > Is it really the case that it's possible to end up encrypting the page even if the data factory says it shouldn't be encrypted?

          I think this is a chicken and egg issue. The state of the data factory, and log factory, isn't flipped until the operation has been carried out.
          There are now three different data paths, where data goes in and out of the cache "concurrently":
          encrypt existing: [disk] -> plain -> [cache] -> encrypt -> [disk]
          re-encrypt db : [disk] -> decrypt -> [cache] -> encrypt -> [disk]
          decrypt db : [disk] -> decrypt -> [cache] -> plain -> [disk]

          I'd have to check how the databaseEncrypted flags affect the decryption process as data enters the page cache before trying to simplify/change this.
          In any case, the existing check for encryptWithNewEngine is there to deal with the case of encrypting an existing plaintext database.

          I also see that setting the encryption buffer to null is used as an optimization in RAFContainer4, and in that case DataFactory.encryptedDatabase() is always false. This is normal operation though, not a case where the whole database will transition from plaintext to ciphertext or vice versa.

          > The new DATABASE_DECRYPTION_DENIED message might be problematic to localize because it takes an English string as an argument.

          Yes, I'm aware of this. We need to nail down the messages.
          I like the point about reusing the same SQL state. The important issue here is that the decryption request was denied, not why. I say that, because it is unlikely that this failure condition can be dealt with automatically by code. A DBA would likely have to read the message - which does tell you why the request was denied - and take corrective actions anyway. Also, encrypting and decrypting a database is hopefully not something you do frequently on a given database.

          Encryption has different states for each failure condition:
          <text>Cannot encrypt the database when there is a global transaction in the prepared state.</text>
          <text>Cannot re-encrypt the database with a new boot password or an external encryption key when there is a global transaction in the prepared state.</text>
          <text>Cannot configure a read-only database for encryption.</text>
          <text>Cannot re-encrypt a read-only database with a new boot password or an external encryption key .</text>
          <text>Cannot configure a database for encryption, when database is in the log archive mode.</text>
          <text>Cannot re-encrypt a database with a new boot password or an external encryption key, when database is in the log archive mode.</text>
          <text>Encryption of an un-encrypted database failed:

          {0}</text>
          <text>Encryption of an encrypted database with a new key or a new password failed: {0}

          </text>

          Guess I'll add another four messages to the heap:
          <text>Cannot decrypt the database when there is a global transaction in the prepared state.</text>
          <text>Cannot configure a read-only database for decryption.</text>
          <text>Cannot configure a database for decryption, when database is in the log archive mode.</text>
          <text>Decryption of an encrypted database failed:

          {0}

          </text>

          The new messages follow the style of the existing messages. Are they good enough, or should they be rewritten?

          Show
          Kristian Waagan added a comment - Thanks for the review, Knut Anders. Answers/comments below. > What does this mean exactly? Can any user decrypt the database with the current state of the patch? Any user that knows the boot password / encryption key can decrypt the database with patch 3a. Nothing will happen if the database has already been booted. Assuming the DBO boots the database by supplying the boot credentials, JOE_USER can't decrypt the database. "decryptDatabase=true" will be ignored when JOE_USER connects to the booted database. This hasn't changed with this patch, encryption attributes are handled the same way. I have a patch adding the restriction that only the DBO can decrypt a database, given that authentication and authorization is enabled. The code will go in toghether with changes to the DboPowersTest. > Is it really the case that it's possible to end up encrypting the page even if the data factory says it shouldn't be encrypted? I think this is a chicken and egg issue. The state of the data factory, and log factory, isn't flipped until the operation has been carried out. There are now three different data paths, where data goes in and out of the cache "concurrently": encrypt existing: [disk] -> plain -> [cache] -> encrypt -> [disk] re-encrypt db : [disk] -> decrypt -> [cache] -> encrypt -> [disk] decrypt db : [disk] -> decrypt -> [cache] -> plain -> [disk] I'd have to check how the databaseEncrypted flags affect the decryption process as data enters the page cache before trying to simplify/change this. In any case, the existing check for encryptWithNewEngine is there to deal with the case of encrypting an existing plaintext database. I also see that setting the encryption buffer to null is used as an optimization in RAFContainer4, and in that case DataFactory.encryptedDatabase() is always false. This is normal operation though, not a case where the whole database will transition from plaintext to ciphertext or vice versa. > The new DATABASE_DECRYPTION_DENIED message might be problematic to localize because it takes an English string as an argument. Yes, I'm aware of this. We need to nail down the messages. I like the point about reusing the same SQL state. The important issue here is that the decryption request was denied, not why. I say that, because it is unlikely that this failure condition can be dealt with automatically by code. A DBA would likely have to read the message - which does tell you why the request was denied - and take corrective actions anyway. Also, encrypting and decrypting a database is hopefully not something you do frequently on a given database. Encryption has different states for each failure condition: <text>Cannot encrypt the database when there is a global transaction in the prepared state.</text> <text>Cannot re-encrypt the database with a new boot password or an external encryption key when there is a global transaction in the prepared state.</text> <text>Cannot configure a read-only database for encryption.</text> <text>Cannot re-encrypt a read-only database with a new boot password or an external encryption key .</text> <text>Cannot configure a database for encryption, when database is in the log archive mode.</text> <text>Cannot re-encrypt a database with a new boot password or an external encryption key, when database is in the log archive mode.</text> <text>Encryption of an un-encrypted database failed: {0}</text> <text>Encryption of an encrypted database with a new key or a new password failed: {0} </text> Guess I'll add another four messages to the heap: <text>Cannot decrypt the database when there is a global transaction in the prepared state.</text> <text>Cannot configure a read-only database for decryption.</text> <text>Cannot configure a database for decryption, when database is in the log archive mode.</text> <text>Decryption of an encrypted database failed: {0} </text> The new messages follow the style of the existing messages. Are they good enough, or should they be rewritten?
          Hide
          Knut Anders Hatlen added a comment -

          > Known missing tasks:
          > o logic to deal with DBO powers

          What does this mean exactly? Can any user decrypt the database with the current state of the patch?

          In RAFContainer.java, the patch makes this change:

          else
          {

          • if (dataFactory.databaseEncrypted() || encryptWithNewEngine)
            + if (encryptionBuf != null &&
            + (dataFactory.databaseEncrypted() || encryptWithNewEngine))
            {

          I was a bit surprised that the original code checked for encryptWithNewEngine here. Is it really the case that it's possible to end up encrypting the page even if the data factory says it shouldn't be encrypted? If not, perhaps it could be simplified to just check for dataFactory.databaseEncrypted(), in which case we don't need to add an extra check for encryptionBuf to support decryption?

          The new DATABASE_DECRYPTION_DENIED message might be problematic to localize because it takes an English string as an argument. We may need multiple messages to allow them to be fully translated. Sharing SQL state between the messages would be fine, though.

          Show
          Knut Anders Hatlen added a comment - > Known missing tasks: > o logic to deal with DBO powers What does this mean exactly? Can any user decrypt the database with the current state of the patch? In RAFContainer.java, the patch makes this change: else { if (dataFactory.databaseEncrypted() || encryptWithNewEngine) + if (encryptionBuf != null && + (dataFactory.databaseEncrypted() || encryptWithNewEngine)) { I was a bit surprised that the original code checked for encryptWithNewEngine here. Is it really the case that it's possible to end up encrypting the page even if the data factory says it shouldn't be encrypted? If not, perhaps it could be simplified to just check for dataFactory.databaseEncrypted(), in which case we don't need to add an extra check for encryptionBuf to support decryption? The new DATABASE_DECRYPTION_DENIED message might be problematic to localize because it takes an English string as an argument. We may need multiple messages to allow them to be fully translated. Sharing SQL state between the messages would be fine, though.
          Hide
          Kristian Waagan added a comment -

          Patch 3a adds the decryption feature:

          • iapi/reference/Attribute
            Adds the new connection URL 'decryptDatabase' (true|false).
          • iapi/store/raw/RawStoreFactory
            Adds the new minor version 10.
            Updated a comment.
          • iapi/store/raw/DataFactory
            Adds method decryptAllContainers(RawTransaction).
          • impl/jdbc/EmbeddedConnection
            Introduces notion of crypto boot, instead of looking just for encryption. Makes two-phase boot logic apply to decryption.
            Adds check for conflicting high-level cryptographic attributes. Note that the checking here is incomplete due to missing knowledge about the state of the database (for instance, is it encrypted or not?).
          • impl/store/RawStore
            Adds logic to detect decryption request.
            Denies decryption if the database is in certain states (read-only, has global prepared xact, log archived, store version too old).
            Adds logic to update the service properties, that is to remove encryption properties after decryption has happened.
            Decryption reuses the same crash recovery support as encryption uses.
          • impl/store/raw/data/BaseDataFileFactory
            Implements decryptAllContainers(RawTransaction.
          • impl/store/raw/data/RAFContainer
            Adds logic to skip encryption of page data. This is effectively where decryption happens, except that the data has already been decrypted when entering the page cache. We just don't encrypt it again before writing it out to disk.
            Updates some error messages.
          • loc/messages.xml
            Adds two new error messages.
          • shared/common/reference/SQLState
            Adds two new SQLStates.
          • tests/store/_Suite
            Enables DecryptDatabaseTest.

          Known missing tasks:
          o logic to deal with DBO powers
          o crash recovery test
          o may want to introduce a DecryptContainerOperation instead of reusing the log entry for encryption
          o some potential cleanup/refactoring
          o don't know if the error messages are satisfactory, or if we want to add separate messages for each of the failure situations
          o documentation (logged by Kim as DERBY-5939, thanks!), which should be very similar to encryption, but much simpler. There is only one knob We probably want to mention the failure situations, which are mainly conflicting attributes and cases where decryption is unsupported/denied.

          Patch ready for review.

          Show
          Kristian Waagan added a comment - Patch 3a adds the decryption feature: iapi/reference/Attribute Adds the new connection URL 'decryptDatabase' (true|false). iapi/store/raw/RawStoreFactory Adds the new minor version 10. Updated a comment. iapi/store/raw/DataFactory Adds method decryptAllContainers(RawTransaction). impl/jdbc/EmbeddedConnection Introduces notion of crypto boot, instead of looking just for encryption. Makes two-phase boot logic apply to decryption. Adds check for conflicting high-level cryptographic attributes. Note that the checking here is incomplete due to missing knowledge about the state of the database (for instance, is it encrypted or not?). impl/store/RawStore Adds logic to detect decryption request. Denies decryption if the database is in certain states (read-only, has global prepared xact, log archived, store version too old). Adds logic to update the service properties, that is to remove encryption properties after decryption has happened. Decryption reuses the same crash recovery support as encryption uses. impl/store/raw/data/BaseDataFileFactory Implements decryptAllContainers(RawTransaction. impl/store/raw/data/RAFContainer Adds logic to skip encryption of page data. This is effectively where decryption happens, except that the data has already been decrypted when entering the page cache. We just don't encrypt it again before writing it out to disk. Updates some error messages. loc/messages.xml Adds two new error messages. shared/common/reference/SQLState Adds two new SQLStates. tests/store/_Suite Enables DecryptDatabaseTest. Known missing tasks: o logic to deal with DBO powers o crash recovery test o may want to introduce a DecryptContainerOperation instead of reusing the log entry for encryption o some potential cleanup/refactoring o don't know if the error messages are satisfactory, or if we want to add separate messages for each of the failure situations o documentation (logged by Kim as DERBY-5939 , thanks!), which should be very similar to encryption, but much simpler. There is only one knob We probably want to mention the failure situations, which are mainly conflicting attributes and cases where decryption is unsupported/denied. Patch ready for review.
          Hide
          Kristian Waagan added a comment -

          Patch 2a adds a basic test for database decryption. It tests the core functionality with the default encryption algorithm and with AES/OFB/NoPadding.
          The test will be enabled when the required functionality has been added.

          Committed to trunk with revision 1392856.

          Show
          Kristian Waagan added a comment - Patch 2a adds a basic test for database decryption. It tests the core functionality with the default encryption algorithm and with AES/OFB/NoPadding. The test will be enabled when the required functionality has been added. Committed to trunk with revision 1392856.
          Hide
          Kristian Waagan added a comment -

          Committed patch 1b to trunk with revision 1390712.
          I'll follow up with the main patch that adds the functionality, and patches that adds tests (a separate test for the feature, a crash recovery test, a test for DBO powers, and possibly an upgrade test).

          Show
          Kristian Waagan added a comment - Committed patch 1b to trunk with revision 1390712. I'll follow up with the main patch that adds the functionality, and patches that adds tests (a separate test for the feature, a crash recovery test, a test for DBO powers, and possibly an upgrade test).
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1b, where I have renamed "databaseIsEncrypted" to "isEncryptedDatabase" in RawStore. Otherwise unchanged from 1a.

          Both derbyall and suites.All passed with patch 1b.

          Show
          Kristian Waagan added a comment - Attaching patch 1b, where I have renamed "databaseIsEncrypted" to "isEncryptedDatabase" in RawStore. Otherwise unchanged from 1a. Both derbyall and suites.All passed with patch 1b.
          Hide
          Kristian Waagan added a comment -

          Not sure what went wrong, but the patch I uploaded was slightly outdated. While the correct file was dated 16:57, the upload at 17:07 used an older version of the patch file. Maybe the browser cached the older version, i.e. it read it before I pressed "Attach"?

          I'm re-running the regression tests to verify that the errors go away.

          Show
          Kristian Waagan added a comment - Not sure what went wrong, but the patch I uploaded was slightly outdated. While the correct file was dated 16:57, the upload at 17:07 used an older version of the patch file. Maybe the browser cached the older version, i.e. it read it before I pressed "Attach"? I'm re-running the regression tests to verify that the errors go away.
          Hide
          Kristian Waagan added a comment -

          ---- Kim ----
          The one-time operation point seems a bit less compelling, if my guess (just a guess) is correct that "dataEncryption=true" is also a one-time operation? Or can you re-encrypt an encrypted database, changing from, say, one encryption algorithm to another? Would that work, or would it totally mess up your database? Anyway, I think reducing the possibility of confusion is desirable.


          Actually, dataEncryption isn't considered a one-time operation, although for the end user it appears to be so:
          a) We save dataEncryption=true in service.properties. Even if you specify dataEncryption=false when connecting, we will read dataEncryption=true if the database is [already] encrypted.
          b) You can re-encrypt [1] an encrypted database, but I haven't checked if you have to specify dataEncryption=true or only newBootPassword/newEncryptionKey (with bootPassword to access the database in addition) to do so.

          As for attribute handling I'm going for what seems to be the default action:
          o ignore attributes when they don't cause any trouble, for instance decryptDatabase=true on un-encrypted or booted database. One could argue the latter case deserves a warning.
          o raise exception if the attributes are truly conflicting (dataEncryption=true;decryptDatabase=true on un-encrypted database, decryptDatabase=true;createFrom=myEncryptedDb)

          [1] I'd have to look at the code / docs again to say exactly what re-encrypt entails in all cases. I seem to recall some differences between using the boot attributes and the system procedure for changing the boot password.

          Show
          Kristian Waagan added a comment - ---- Kim ---- The one-time operation point seems a bit less compelling, if my guess (just a guess) is correct that "dataEncryption=true" is also a one-time operation? Or can you re-encrypt an encrypted database, changing from, say, one encryption algorithm to another? Would that work, or would it totally mess up your database? Anyway, I think reducing the possibility of confusion is desirable. Actually, dataEncryption isn't considered a one-time operation, although for the end user it appears to be so: a) We save dataEncryption=true in service.properties. Even if you specify dataEncryption=false when connecting, we will read dataEncryption=true if the database is [already] encrypted. b) You can re-encrypt [1] an encrypted database, but I haven't checked if you have to specify dataEncryption=true or only newBootPassword/newEncryptionKey (with bootPassword to access the database in addition) to do so. As for attribute handling I'm going for what seems to be the default action: o ignore attributes when they don't cause any trouble, for instance decryptDatabase=true on un-encrypted or booted database. One could argue the latter case deserves a warning. o raise exception if the attributes are truly conflicting (dataEncryption=true;decryptDatabase=true on un-encrypted database, decryptDatabase=true;createFrom=myEncryptedDb) [1] I'd have to look at the code / docs again to say exactly what re-encrypt entails in all cases. I seem to recall some differences between using the boot attributes and the system procedure for changing the boot password.
          Hide
          Dag H. Wanvik added a comment -

          The model for attributes are unclear, but they fall into two main categories [1]

          • operation (e.g. create, shutdown, restoreFrom)
          • arguments for an operation (e.g. password)

          There is precendence for no-op operations being ignored (dataEncryption=false always, dataEncryption=true on 2..n connect) and for raising SQLWarning (create=true on 2..n connect); the latter only in the embedded driver (DERBY-5907).

          Sometimes meaningless combinations of operations/arguments give errors, others are ignored..

          I guess we can't resolve this state of things easily now, but it would be good to understand which way we should head with this....
          In any case, I think of decrypting as an operation separate from encryption, so I'd prefer it to have its own attribute.
          It may be ignored on attempts 2..n, I guess. "decryptDatabase=true" seems fine to me.

          [1]
          Operations

          create=true attribute
          createFrom=path attribute
          dataEncryption=true attribute
          failover=true attribute
          restoreFrom=path attribute
          shutdown=true attribute
          rollForwardRecoveryFrom=path attribute
          startMaster=true attribute
          startSlave=true attribute
          stopMaster=true attribute
          stopSlave=true attribute
          upgrade=true attribute
          drop=true attribute

          Arguments

          bootPassword=key attribute
          collation=collation attribute
          databaseName=nameofDatabase attribute
          deregister=false attribute
          encryptionKey=key attribute
          encryptionProvider=providerName attribute
          encryptionAlgorithm=algorithm attribute
          logDevice=logDirectoryPath attribute
          newEncryptionKey=key attribute
          newBootPassword=newPassword attribute
          password=userPassword attribute
          retrieveMessageText=false attribute
          securityMechanism=value attribute
          slaveHost=hostname attribute
          slavePort=portValue attribute
          ssl=sslMode attribute
          territory=ll_CC attribute
          traceDirectory=path attribute
          traceFile=path attribute
          traceFileAppend=true attribute
          traceLevel=value attribute
          user=userName attribute

          Show
          Dag H. Wanvik added a comment - The model for attributes are unclear, but they fall into two main categories [1] operation (e.g. create, shutdown, restoreFrom) arguments for an operation (e.g. password) There is precendence for no-op operations being ignored (dataEncryption=false always, dataEncryption=true on 2..n connect) and for raising SQLWarning (create=true on 2..n connect); the latter only in the embedded driver ( DERBY-5907 ). Sometimes meaningless combinations of operations/arguments give errors, others are ignored.. I guess we can't resolve this state of things easily now, but it would be good to understand which way we should head with this.... In any case, I think of decrypting as an operation separate from encryption, so I'd prefer it to have its own attribute. It may be ignored on attempts 2..n, I guess. "decryptDatabase=true" seems fine to me. [1] Operations create=true attribute createFrom=path attribute dataEncryption=true attribute failover=true attribute restoreFrom=path attribute shutdown=true attribute rollForwardRecoveryFrom=path attribute startMaster=true attribute startSlave=true attribute stopMaster=true attribute stopSlave=true attribute upgrade=true attribute drop=true attribute Arguments bootPassword=key attribute collation=collation attribute databaseName=nameofDatabase attribute deregister=false attribute encryptionKey=key attribute encryptionProvider=providerName attribute encryptionAlgorithm=algorithm attribute logDevice=logDirectoryPath attribute newEncryptionKey=key attribute newBootPassword=newPassword attribute password=userPassword attribute retrieveMessageText=false attribute securityMechanism=value attribute slaveHost=hostname attribute slavePort=portValue attribute ssl=sslMode attribute territory=ll_CC attribute traceDirectory=path attribute traceFile=path attribute traceFileAppend=true attribute traceLevel=value attribute user=userName attribute
          Hide
          Kim Haase added a comment -

          I agree that using "dataEncryption=false" would create confusion – currently that does seem to mean the same thing as not specifying anything, though it's not documented. Also, I think it might make more sense to use "decryptDatabase=true" than to use "dataDecryption=true". If the decryption attribute is very different from the encryption one, people will be less likely to specify the wrong attribute by accident.

          The one-time operation point seems a bit less compelling, if my guess (just a guess) is correct that "dataEncryption=true" is also a one-time operation? Or can you re-encrypt an encrypted database, changing from, say, one encryption algorithm to another? Would that work, or would it totally mess up your database? Anyway, I think reducing the possibility of confusion is desirable.

          The docs say, "The dataEncryption attribute must be combined with the bootPassword=key attribute or the newEncryptionKey=key attribute." But what it should say is "The dataEncryption=true attribute must be combined ..." It might also make sense to document the meaning of "false" explicitly. (If I specify false, I don't get an error message if I don't specify a key.)

          I'm finding various issues with the encryption documentation as I poke through it – for example, the "encryptionAlgorithm" topic doesn't say that you can use that attribute in combination with the "encryptionKey" one, though the "encryptionKey" topic has an example showing exactly that. Also, the Developer's Guide has one main section on encryption under "Derby and Security" and several more topics under "Working with the database connection URL attributes". Might be a good idea to overhaul some of this.

          Show
          Kim Haase added a comment - I agree that using "dataEncryption=false" would create confusion – currently that does seem to mean the same thing as not specifying anything, though it's not documented. Also, I think it might make more sense to use "decryptDatabase=true" than to use "dataDecryption=true". If the decryption attribute is very different from the encryption one, people will be less likely to specify the wrong attribute by accident. The one-time operation point seems a bit less compelling, if my guess (just a guess) is correct that "dataEncryption=true" is also a one-time operation? Or can you re-encrypt an encrypted database, changing from, say, one encryption algorithm to another? Would that work, or would it totally mess up your database? Anyway, I think reducing the possibility of confusion is desirable. The docs say, "The dataEncryption attribute must be combined with the bootPassword=key attribute or the newEncryptionKey=key attribute." But what it should say is "The dataEncryption=true attribute must be combined ..." It might also make sense to document the meaning of "false" explicitly. (If I specify false, I don't get an error message if I don't specify a key.) I'm finding various issues with the encryption documentation as I poke through it – for example, the "encryptionAlgorithm" topic doesn't say that you can use that attribute in combination with the "encryptionKey" one, though the "encryptionKey" topic has an example showing exactly that. Also, the Developer's Guide has one main section on encryption under "Derby and Security" and several more topics under "Working with the database connection URL attributes". Might be a good idea to overhaul some of this.
          Hide
          Kristian Waagan added a comment -

          Attaching patch 1a (second round), which makes a series of boilerplate changes and does some cleanup. Again, this patch shouldn't cause any functional changes.

          Description:
          o DataFactory.setDatabaseEncrypted: introduced boolean flag to be able to tell the data factory that encryption has been turned off
          Updated implementing method in BaseDataFileFactory
          o setDatabaseEncrypted: introduced second boolean flag to be able to tell the log factory that encryption has been turned off
          Updated implementing methods in LogToFile and ReadOnly.
          o RawContainerHandle.encryptContainer: renamed to encryptOrDecryptContainer, added boolean flag to control crypto operation
          Updated implementing method in BaseContainerHandle
          o BaseContainer.encryptContainer: renamed to encryptOrDecryptContainer, added boolean flag to control crypto operation
          Updated implementing methods in RAFContainer and InputStreamContainer
          o EncryptData: renamed to EncryptOrDecryptData, added method decryptAllContainers, whitespace changes
          o RawStore:

          • removed import
          • removed instance variable encryptDatabase
          • removed unused instance variable dataDirectory
          • renamed databaseEncrypted to databaseIsEncrypted
          • renamed configureDatabaseForEncryption to applyBulkCryptoOperation
          • made setupEncryptionEngines return a boolean: whether or not existing data must be transformed (applyBulkCryptoOperation)
          • simplified parts of the logic in setupEncryptionEngines
          • introduced isTrue/isSet for property sets
          • removed unused method privList(File)

          Patch ready for review.
          Re-running regression tests.

          Show
          Kristian Waagan added a comment - Attaching patch 1a (second round), which makes a series of boilerplate changes and does some cleanup. Again, this patch shouldn't cause any functional changes. Description: o DataFactory.setDatabaseEncrypted: introduced boolean flag to be able to tell the data factory that encryption has been turned off Updated implementing method in BaseDataFileFactory o setDatabaseEncrypted: introduced second boolean flag to be able to tell the log factory that encryption has been turned off Updated implementing methods in LogToFile and ReadOnly. o RawContainerHandle.encryptContainer: renamed to encryptOrDecryptContainer, added boolean flag to control crypto operation Updated implementing method in BaseContainerHandle o BaseContainer.encryptContainer: renamed to encryptOrDecryptContainer, added boolean flag to control crypto operation Updated implementing methods in RAFContainer and InputStreamContainer o EncryptData: renamed to EncryptOrDecryptData, added method decryptAllContainers, whitespace changes o RawStore: removed import removed instance variable encryptDatabase removed unused instance variable dataDirectory renamed databaseEncrypted to databaseIsEncrypted renamed configureDatabaseForEncryption to applyBulkCryptoOperation made setupEncryptionEngines return a boolean: whether or not existing data must be transformed (applyBulkCryptoOperation) simplified parts of the logic in setupEncryptionEngines introduced isTrue/isSet for property sets removed unused method privList(File) Patch ready for review. Re-running regression tests.
          Hide
          Kristian Waagan added a comment -

          I decided to delete the patch, as I'm changing strategy. I will first implement the changes required to support decryption, and then change existing comments and documentation as required. There are many occurrences where existing text must be changed to mention both encryption and decryption.
          As for implementing the feature, I now have a working prototype. I have tests almost ready for:
          o decryption
          o decryption on un-encrypted database
          o decryption of booted database
          o DBO requirement (following existing rules on this topic)
          o conflicting attributes (may have to change, see below)

          Missing tests:
          o feature disabled on older databases
          o connecting while decrypting? (I'm hoping this is dealt with already, testing may be a bit awkward)

          I'm wondering how to best control the feature. There are two main possibilities:
          a) Add a new URL attribute (decryptDatabase=true).
          b) Reuse an existing URL attribute (dataEncryption=false?)

          Option (b) is possible, but may be confusing. Using a binary value, one must also take care to distinguish between false and unspecified.
          Another possibility for (a) is "dataDecryption" to keep it similar to "dataEncryption". That doesn't sound as good to me, since decryption in this sense is a one-time operation, but maybe the similarity is reason good enough?

          Any opinions on the choice of URL attribute?

          Show
          Kristian Waagan added a comment - I decided to delete the patch, as I'm changing strategy. I will first implement the changes required to support decryption, and then change existing comments and documentation as required. There are many occurrences where existing text must be changed to mention both encryption and decryption. As for implementing the feature, I now have a working prototype. I have tests almost ready for: o decryption o decryption on un-encrypted database o decryption of booted database o DBO requirement (following existing rules on this topic) o conflicting attributes (may have to change, see below) Missing tests: o feature disabled on older databases o connecting while decrypting? (I'm hoping this is dealt with already, testing may be a bit awkward) I'm wondering how to best control the feature. There are two main possibilities: a) Add a new URL attribute (decryptDatabase=true). b) Reuse an existing URL attribute (dataEncryption=false?) Option (b) is possible, but may be confusing. Using a binary value, one must also take care to distinguish between false and unspecified. Another possibility for (a) is "dataDecryption" to keep it similar to "dataEncryption". That doesn't sound as good to me, since decryption in this sense is a one-time operation, but maybe the similarity is reason good enough? Any opinions on the choice of URL attribute?
          Hide
          Kristian Waagan added a comment -

          I'm looking at implementing this feature.

          Patch 1a re-formats and changes the documentation for RawStore.configureDatabaseForEncryption. Except for one paragraph the content is kept mostly unchanged. The comment is converted to JavaDoc, and the method is made private.

          I expect that the documentation will be partly rewritten again as the decryption feature is implemented.

          Show
          Kristian Waagan added a comment - I'm looking at implementing this feature. Patch 1a re-formats and changes the documentation for RawStore.configureDatabaseForEncryption. Except for one paragraph the content is kept mostly unchanged. The comment is converted to JavaDoc, and the method is made private. I expect that the documentation will be partly rewritten again as the decryption feature is implemented.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development