Details
-
Bug
-
Status: Closed (View Workflow)
-
Critical
-
Resolution: Fixed
-
10.1(EOL), 10.2(EOL), 10.3(EOL)
Description
The fix for MDEV-10368 and MDEV-11587 introduced innodb_encryption_rotate_key_age=0 as a special value that disables key rotation entirely. This can help performance, since the key rotation checks seem to require a lot of CPU resources.
Unfortunately, when innodb_encryption_rotate_key_age=0 is set, it also seems to prevent the server from encrypting unencrypted tablespaces in the background.
To reproduce, do the following:
1.) Initialize a datadir without encryption enabled. You can ensure that the tablespaces are not encrypted by querying INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION and confirming that there are 0 rows.
MariaDB [(none)]> SELECT * FROM INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION\G
|
Empty set (0.01 sec)
|
2.) Then change the configuration to enable encryption. Be sure to set innodb_encryption_rotate_key_age=0. I used the following configuration:
plugin-load-add=file_key_management
|
file-key-management
|
file_key_management_encryption_algorithm=aes_cbc
|
file_key_management_filename = /etc/my.cnf.d//keys.enc
|
file_key_management_filekey = secret
|
innodb-encrypt-tables
|
innodb-encrypt-log
|
innodb-encryption-threads=4
|
encrypt-tmp-disk-tables=1
|
encrypt-tmp-files=1
|
encrypt-binlog=1
|
innodb_encryption_rotate_key_age = 0
|
3.) Restart the server.
4.) Check whether the tablespaces are encrypted. They still are not:
MariaDB [(none)]> SELECT * FROM INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION\G
|
Empty set (0.00 sec)
|
5.) Change the configuration so that innodb_encryption_rotate_key_age=0 is commented out.
6.) Restart the server.
7.) Check whether the tablespaces are encrypted. They are:
MariaDB [(none)]> SELECT * FROM INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION\G
|
*************************** 1. row ***************************
|
SPACE: 1
|
NAME: mysql/innodb_table_stats
|
ENCRYPTION_SCHEME: 1
|
KEYSERVER_REQUESTS: 1
|
MIN_KEY_VERSION: 1
|
CURRENT_KEY_VERSION: 1
|
KEY_ROTATION_PAGE_NUMBER: NULL
|
KEY_ROTATION_MAX_PAGE_NUMBER: NULL
|
CURRENT_KEY_ID: 1
|
ROTATING_OR_FLUSHING: 0
|
*************************** 2. row ***************************
|
SPACE: 2
|
NAME: mysql/innodb_index_stats
|
ENCRYPTION_SCHEME: 1
|
KEYSERVER_REQUESTS: 1
|
MIN_KEY_VERSION: 1
|
CURRENT_KEY_VERSION: 1
|
KEY_ROTATION_PAGE_NUMBER: NULL
|
KEY_ROTATION_MAX_PAGE_NUMBER: NULL
|
CURRENT_KEY_ID: 1
|
ROTATING_OR_FLUSHING: 0
|
*************************** 3. row ***************************
|
SPACE: 3
|
NAME: mysql/gtid_slave_pos
|
ENCRYPTION_SCHEME: 1
|
KEYSERVER_REQUESTS: 1
|
MIN_KEY_VERSION: 1
|
CURRENT_KEY_VERSION: 1
|
KEY_ROTATION_PAGE_NUMBER: NULL
|
KEY_ROTATION_MAX_PAGE_NUMBER: NULL
|
CURRENT_KEY_ID: 1
|
ROTATING_OR_FLUSHING: 0
|
*************************** 4. row ***************************
|
SPACE: 0
|
NAME: innodb_system
|
ENCRYPTION_SCHEME: 1
|
KEYSERVER_REQUESTS: 1
|
MIN_KEY_VERSION: 1
|
CURRENT_KEY_VERSION: 1
|
KEY_ROTATION_PAGE_NUMBER: NULL
|
KEY_ROTATION_MAX_PAGE_NUMBER: NULL
|
CURRENT_KEY_ID: 1
|
ROTATING_OR_FLUSHING: 0
|
4 rows in set (0.00 sec)
|
Attachments
Issue Links
- causes
-
-
- Closed
-
- relates to
-
MDEV-11657 Cross-engine transaction metadata
-
- Open
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
-
-
- Closed
-
Activity
I see that at some point we added a check, so that you aren't able to change innodb_encrypt_tables if innodb_encryption_rotate_key_age=0 is set. For example:
MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=0;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=ON;
|
ERROR 1231 (42000): Variable 'innodb_encrypt_tables' can't be set to the value of 'ON'
|
MariaDB [(none)]> SHOW WARNINGS;
|
+---------+------+-------------------------------------------------------------------------------------------------+
|
| Level | Code | Message |
|
+---------+------+-------------------------------------------------------------------------------------------------+
|
| Warning | 138 | InnoDB: cannot enable encryption, innodb_encryption_rotate_key_age=0 i.e. key rotation disabled |
|
| Error | 1231 | Variable 'innodb_encrypt_tables' can't be set to the value of 'ON' |
|
+---------+------+-------------------------------------------------------------------------------------------------+
|
2 rows in set (0.000 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=1;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=ON;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=0;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;
|
ERROR 1231 (42000): Variable 'innodb_encrypt_tables' can't be set to the value of 'OFF'
|
MariaDB [(none)]> SHOW WARNINGS;
|
+---------+------+--------------------------------------------------------------------------------------------------+
|
| Level | Code | Message |
|
+---------+------+--------------------------------------------------------------------------------------------------+
|
| Warning | 138 | InnoDB: cannot disable encryption, innodb_encryption_rotate_key_age=0 i.e. key rotation disabled |
|
| Error | 1231 | Variable 'innodb_encrypt_tables' can't be set to the value of 'OFF' |
|
+---------+------+--------------------------------------------------------------------------------------------------+
|
2 rows in set (0.000 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=1;
|
Query OK, 0 rows affected (0.00 sec)
|
|
|
MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;
|
Query OK, 0 rows affected (0.00 sec)
|
If the patch for this issue makes it so that background encryption/decryption operations are no longer internally considered key rotation operations, then I think that the patch should also remove this restriction. A lot of users like to disable key rotations because the key rotation checks constantly use CPU in the background. These key rotation checks even waste CPU resources when file_key_management plugin is used, which doesn't support key rotations at all. However, I think this would be a non-issue if MDEV-14180 were implemented.
The code has not been changed since my previous comment. The full code changes against 10.4 can be viewed with the following command:
git diff f4f8dd69aa373d7418a4148ac9eaf20e55da5a4d..472d07c5aa7f76fb16aedc4fe759d7619b95cf0d
|
If I remember correctly, this change is not distinguishing tablespaces based on the ENCRYPTION parameter. My current understanding of the design is that tables that were explicitly created or altered with ENCRYPTION=OFF or ENCRYPTION=ON should not be affected by the innodb_encrypt_tables parameter. Therefore, when keeping track of "mixed encryption status", only ENCRYPTION=DEFAULT tables should be considered.
There appears to be a less intrusive alternative that would avoid changing the format of the InnoDB system tablespace and thus be more suitable for implementation in GA releases of MariaDB. Instead, we would extend the update trigger for SET GLOBAL innodb_encrypt_tables in innodb_encrypt_tables_update() or fil_crypt_set_encrypt_tables() as follows:
- Read the first page of all .ibd files that has not been read yet, to determine the encryption status.
- If needed, read also the .frm files, and be sure to cache the results in fil_space_t::crypt_data or similar. In MariaDB 10.2 and later, the function open_purge_table() can be used for this purpose.
- Update some lists in fil_system so that the encryption threads will not do any busy work, but will be able to apply or remove encryption even if innodb_encryption_rotate_key_age=0.
The observable effects would be as follows:
-- trigger the removal of encryption of all ENCRYPTION=DEFAULT tables
|
SET GLOBAL innodb_encrypt_tables=OFF; |
-- trigger the encryption of all unencrypted ENCRYPTION=DEFAULT tables
|
SET GLOBAL innodb_encrypt_tables=ON; |
-- trigger the encryption of all unencrypted ENCRYPTION=DEFAULT tables, and prevent further creation of ENCRYPTION=OFF tables
|
SET GLOBAL innodb_encrypt_tables=FORCE; |
I agree with GeoffMontee that the check that was added to innodb_encrypt_tables_validate() in MDEV-11738/MDEV-11581 should be removed as part of this fix.
Good work! I pushed an amended version for testing.
Can we remove fil_system_t::crypt_status and related code? It was mainly needed for writing a persistent global encryption state, in the 10.4 based earlier version of the patch.
I pushed the removal of fil_system_t::crypt_status and its related code. Let's hope for the testing to give good results.
I pushed one more change that avoids releasing and re-acquiring fil_system.mutex in low-level code, and tries to update the DICT_HDR page at the higher level where appropriate.
After that, I was thinking that the global encryption status should also change if we remove the last tablespace of its kind (encrypted or unencrypted). The following patch attempts to do that, but additional tests would start to fail if I actually enable the logic (see below):
diff --git a/storage/innobase/include/fil0fil.h b/storage/innobase/include/fil0fil.hindex fd9cc91d574..4015b39982c 100644--- a/storage/innobase/include/fil0fil.h+++ b/storage/innobase/include/fil0fil.h@@ -644,12 +644,13 @@ struct fil_system_t {void close();/**- Set the encryption status of all tablespaces after a tablespace- has been added to unencrypted_spaces or encrypted_spaces.+ Set the encryption status of all tablespaces after+ unencrypted_spaces or encrypted_spaces has been changed.@param[in] encrypted whether the tablespace is encrypted+ @param[in] remove whether the tablespace is being removed@return whether dict_hdr_crypt_status_update() must be called */- inline bool crypt_enlist(bool encrypted);+ inline bool crypt_update(bool encrypted, bool remove);private:bool m_initialised;diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.ccindex 68e684b69d4..3f07a37bc17 100644--- a/storage/innobase/fil/fil0fil.cc+++ b/storage/innobase/fil/fil0fil.cc@@ -5098,7 +5098,7 @@ fil_space_set_punch_hole(node->space->punch_hole = val;}-inline bool fil_system_t::crypt_enlist(bool encrypted)+inline bool fil_system_t::crypt_update(bool encrypted, bool remove){ut_ad(is_initialised());ut_ad(this == &fil_system);@@ -5114,12 +5114,12 @@ inline bool fil_system_t::crypt_enlist(bool encrypted)switch (status) {case CRYPT_ENCRYPTED:- if (!encrypted) {+ if (!remove && !encrypted) {status = CRYPT_MIXED;}break;case CRYPT_DECRYPTED:- if (encrypted) {+ if (!remove && encrypted) {status = CRYPT_MIXED;}break;@@ -5158,12 +5158,12 @@ bool fil_space_t::crypt_enlist()if (!crypt_data || !crypt_data->min_key_version) {if (add_if_not_in_unencrypted_spaces()) {remove_if_in_encrypted_spaces();- return fil_system.crypt_enlist(false);+ return fil_system.crypt_update(false, false);}} else {if (add_if_not_in_encrypted_spaces()) {remove_if_in_unencrypted_spaces();- return fil_system.crypt_enlist(true);+ return fil_system.crypt_update(true, false);}}@@ -5178,9 +5178,18 @@ inline bool fil_space_t::crypt_delist()return false;}- if (remove_if_in_encrypted_spaces()- || remove_if_in_unencrypted_spaces()) {- return true;+ if (remove_if_in_encrypted_spaces()) {+ ut_ad(!is_in_unencrypted_spaces());+ ut_ad(true || fil_system.crypt_status+ != fil_system_t::CRYPT_DECRYPTED);+ return true || fil_system.crypt_update(true, true);+ }++ if (remove_if_in_unencrypted_spaces()) {+ ut_ad(!is_in_unencrypted_spaces());+ ut_ad(true || fil_system.crypt_status+ != fil_system_t::CRYPT_ENCRYPTED);+ return true || fil_system.crypt_update(false, true);}If you apply the patch and remove the 4 occurrences of true || at the end of the patch, then the following tests will fail (in addition to the 3 already failing tests):
thiru, please investigate and fix this.
Also, while working on the patch, I noticed that MLOG_FILE_CREATE2 is not being written (and flushed) ahead of the file creation, but behind it. That would be fixed in the course of
MDEV-18128orMDEV-18518.